Merge pull request #7456 from opf/feature/s3_content_disposition

Attachments content disposition in APIv3

[ci skip]

app/models/attachment.rb

@@ -31,6 +31,7 @@
 require 'digest/md5'
 
 class Attachment < ActiveRecord::Base
+  ALLOWED_TEXT_TYPES = %w[text/plain].freeze
   ALLOWED_IMAGE_TYPES = %w[image/gif image/jpeg image/png image/tiff image/bmp].freeze
 
   belongs_to :container, polymorphic: true
@@ -59,7 +60,7 @@ class Attachment < ActiveRecord::Base
   # Returns an URL if the attachment is stored in an external (fog) attachment storage
   # or nil otherwise.
   def external_url
-    url = URI.parse file.download_url # returns a path if local
+    url = URI.parse file.download_url(content_disposition: content_disposition) # returns a path if local
 
     url if url.host
   rescue URI::InvalidURIError
@@ -80,7 +81,7 @@ class Attachment < ActiveRecord::Base
   end
 
   def content_disposition
-    inlineable? ? 'inline' : 'attachment'
+    inlineable? ? "inline" : "attachment; filename=#{self[:file]}"
   end
 
   def visible?(user = User.current)
@@ -97,7 +98,11 @@ class Attachment < ActiveRecord::Base
 
   # images are sent inline
   def inlineable?
-    is_image?
+    is_plain_text? || is_image?
+  end
+
+  def is_plain_text?
+    ALLOWED_TEXT_TYPES.include?(content_type)
   end
 
   def is_image?

app/uploaders/file_uploader.rb

@@ -49,7 +49,7 @@ module FileUploader
     file.to_file
   end
 
-  def download_url
+  def download_url(options = {})
     file.is_path? ? file.path : file.url
   end
 

app/uploaders/fog_file_uploader.rb

@@ -57,8 +57,20 @@ class FogFileUploader < CarrierWave::Uploader::Base
     super
   end
 
-  def download_url
-    remote_file.url
+  def download_url(options = {})
+    url_options = {}
+
+    if options[:content_disposition].present?
+      url_options[:query] = {
+        # Passing this option to S3 will make it serve the file with the
+        # respective content disposition. Without it no content disposition
+        # header is sent. This only works for S3 but we don't support
+        # anything else anyway (see carrierwave.rb).
+        "response-content-disposition" => options[:content_disposition]
+      }
+    end
+
+    remote_file.url url_options
   end
 
   ##

lib/api/helpers/attachment_renderer.rb

@@ -44,7 +44,7 @@ module API
           redirect attachment.external_url.to_s
         else
           content_type attachment.content_type
-          header['Content-Disposition'] = "attachment; filename=#{attachment.filename}"
+          header['Content-Disposition'] = attachment.content_disposition
           env['api.format'] = :binary
           attachment.diskfile.read
         end

spec/models/attachment_spec.rb

@@ -233,4 +233,66 @@ describe Attachment, type: :model do
       expect(File.exists?(attachment.file.path)).to eq false
     end
   end
+
+  describe "#external_url" do
+    let(:author) { FactoryBot.create :user }
+
+    let(:image_path) { Rails.root.join("spec/fixtures/files/image.png") }
+    let(:text_path) { Rails.root.join("spec/fixtures/files/testfile.txt") }
+    let(:binary_path) { Rails.root.join("spec/fixtures/files/textfile.txt.gz") }
+
+    let(:fog_attachment_class) do
+      class FogAttachment < Attachment
+        # Remounting the uploader overrides the original file setter taking care of setting,
+        # among other things, the content type. So we have to restore that original
+        # method this way.
+        # We do this in a new, separate class, as to not interfere with any other specs.
+        alias_method :set_file, :file=
+        mount_uploader :file, FogFileUploader
+        alias_method :file=, :set_file
+      end
+
+      FogAttachment
+    end
+
+    let(:image_attachment) { fog_attachment_class.new author: author, file: File.open(image_path) }
+    let(:text_attachment) { fog_attachment_class.new author: author, file: File.open(text_path) }
+    let(:binary_attachment) { fog_attachment_class.new author: author, file: File.open(binary_path) }
+
+    before do
+      Fog.mock!
+
+      connection = Fog::Storage.new provider: "AWS"
+      connection.directories.create key: "my-bucket"
+
+      CarrierWave::Configuration.configure_fog! credentials: {}, directory: "my-bucket", public: false
+    end
+
+    describe "for an image file" do
+      before { image_attachment.save! }
+
+      it "should make S3 use content_disposition inline" do
+        expect(image_attachment.content_disposition).to eq "inline"
+        expect(image_attachment.external_url.to_s).to include "response-content-disposition=inline"
+      end
+    end
+
+    describe "for a text file" do
+      before { text_attachment.save! }
+
+      it "should make S3 use content_disposition inline" do
+        expect(text_attachment.content_disposition).to eq "inline"
+        expect(text_attachment.external_url.to_s).to include "response-content-disposition=inline"
+      end
+    end
+
+    describe "for a binary file" do
+      before { binary_attachment.save! }
+
+      it "should make S3 use content_disposition 'attachment; filename=...'" do
+        expect(binary_attachment.content_disposition).to eq "attachment; filename=textfile.txt.gz"
+        expect(binary_attachment.external_url.to_s).to include "response-content-disposition=attachment%3B%20filename%3Dtextfile.txt.gz"
+      end
+    end
+  end
 end

spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb

@@ -252,8 +252,10 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j
     subject(:response) { last_response }
 
     context 'with required permissions' do
-      context 'for a local file' do
-        let(:mock_file) { FileHelpers.mock_uploaded_file name: 'foobar.txt' }
+      shared_examples 'for a local file' do
+        let(:mock_file) { raise "define mock_file" }
+        let(:content_disposition) { raise "define content_disposition" }
+
         let(:attachment) do
           att = FactoryBot.create(:attachment, container: container, file: mock_file)
 
@@ -270,7 +272,7 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j
 
         it 'has the necessary headers' do
           expect(subject.headers['Content-Disposition'])
-            .to eql "attachment; filename=#{mock_file.original_filename}"
+            .to eql content_disposition
 
           expect(subject.headers['Content-Type'])
             .to eql mock_file.content_type
@@ -282,6 +284,20 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j
         end
       end
 
+      context 'for a local text file' do
+        it_behaves_like 'for a local file' do
+          let(:mock_file) { FileHelpers.mock_uploaded_file name: 'foobar.txt' }
+          let(:content_disposition) { "inline" }
+        end
+      end
+
+      context 'for a local binary file' do
+        it_behaves_like 'for a local file' do
+          let(:mock_file) { FileHelpers.mock_uploaded_file name: 'foobar.dat', content_type: "application/octet-stream" }
+          let(:content_disposition) { "attachment; filename=#{mock_file.original_filename}" }
+        end
+      end
+
       context 'for a remote file' do
         let(:external_url) { 'http://some_service.org/blubs.gif' }
         let(:mock_file) { FileHelpers.mock_uploaded_file name: 'foobar.txt' }