be explicit about inline vs. download in safe files redirect

if we're redirecting to safe files and it's not for download, it's for
inline and we need to include that in the redirect url. also, be
explicit about wanting to download when clicking a link labeled
"Download".

fixes #5433

test-plan:
  - have s3 storage enabled and a safe files domain
  - upload an image as a file submission
    - view the submission in speedgrader; image should be displayed
      inline rather than being downloaded
  - upload an image on the files tab
    - click the download link; should download the image rather than
      being shown in browser
    - preview the uploaded image from the files tab; it should display
      inline
  - upload a flash file on the files tab
    - preview the uploaded image from the files tab; it should display
      inline (independent check necessary because flash is touchy about
      content-disposition headers)
    - click the download link; should download the file rather than
      being shown in browser

Change-Id: Ibd48eb4629aa26052d3d1bce90444146033b9eeb
Reviewed-on: https://gerrit.instructure.com/8275
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>

Author: lukfugl

app/controllers/application_controller.rb

@@ -990,9 +990,21 @@ def safe_domain_file_url(attachment, host=nil, verifier = nil, download = false)
     # of content.
     opts = { :user_id => @current_user.try(:id), :ts => ts, :sf_verifier => sig }
     opts[:verifier] = verifier if verifier.present?
-    opts[:download_frd] = 1 if download
+
+    if download
+      # download "for realz, dude" (see later comments about :download)
+      opts[:download_frd] = 1
+    else
+      # don't set :download here, because file_download_url won't like it. see
+      # comment below for why we'd want to set :download
+      opts[:inline] = 1
+    end
 
     if @context && Attachment.relative_context?(@context.class.base_ar_class) && @context == attachment.context
+      # so yeah, this is right. :inline=>1 wants :download=>1 to go along with
+      # it, so we're setting :download=>1 *because* we want to display inline.
+      opts[:download] = 1 unless download
+
       # if the context is one that supports relative paths (which requires extra
       # routes and stuff), then we'll build an actual named_context_url with the
       # params for show_relative

app/views/files/show.html.erb

@@ -24,7 +24,7 @@
       </object>
       <!--<![endif]-->
     </object>
-    <a style="display:block;" href="<%= safe_domain_file_url(@attachment) %>">Download <%= @attachment.display_name %></a>
+    <a style="display:block;" href="<%= safe_domain_file_url(@attachment, nil, nil, true) %>">Download <%= @attachment.display_name %></a>
   <% elsif @attachment.inline_content? %>
     <% jammit_js :file_inline %>
     <iframe id="file_content" src="<%= safe_domain_file_url(@attachment) %>" style="width: 100%; height: 400px;"></iframe>

spec/controllers/application_controller_spec.rb

@@ -42,6 +42,53 @@
     end
   end
 
+  describe "safe_domain_file_user" do
+    before :each do
+      # safe_domain_file_url wants to use request.protocol
+      @controller.stubs(:request).returns(mock(:protocol => ''))
+
+      @user = User.create!
+      @attachment = @user.attachments.new(:filename => 'foo.png')
+      @attachment.content_type = 'image/png'
+      @attachment.save!
+
+      @common_params = {
+        :user_id => nil,
+        :ts => nil,
+        :sf_verifier => nil,
+        :only_path => true
+      }
+    end
+
+    it "should include inline=1 in url by default" do
+      @controller.expects(:file_download_url).
+        with(@attachment, @common_params.merge(:inline => 1)).
+        returns('')
+      @controller.send(:safe_domain_file_url, @attachment)
+    end
+
+    it "should include :download=>1 in inline urls for relative contexts" do
+      @controller.instance_variable_set(:@context, @attachment.context)
+      @controller.stubs(:named_context_url).returns('')
+      url = @controller.send(:safe_domain_file_url, @attachment)
+      url.should match(/[\?&]download=1(&|$)/)
+    end
+
+    it "should not include :download=>1 in download urls for relative contexts" do
+      @controller.instance_variable_set(:@context, @attachment.context)
+      @controller.stubs(:named_context_url).returns('')
+      url = @controller.send(:safe_domain_file_url, @attachment, nil, nil, true)
+      url.should_not match(/[\?&]download=1(&|$)/)
+    end
+
+    it "should include download_frd=1 and not include inline=1 in url when specified as for download" do
+      @controller.expects(:file_download_url).
+        with(@attachment, @common_params.merge(:download_frd => 1)).
+        returns('')
+      @controller.send(:safe_domain_file_url, @attachment, nil, nil, true)
+    end
+  end
+
 end