set content type of attachments in S3 (#8938)

* set content type of attachments in S3

* refactoring to allow amending and reading given metadata more easily

* confirm attachment content type is sent to S3

app/uploaders/direct_fog_uploader.rb

@@ -3,50 +3,79 @@ require_relative 'fog_file_uploader'
 class DirectFogUploader < FogFileUploader
   include CarrierWaveDirect::Uploader
 
-  def self.for_attachment(attachment)
-    for_uploader attachment.file
-  end
-
-  def self.for_uploader(fog_file_uploader)
-    raise ArgumentError, "FogFileUploader expected" unless fog_file_uploader.is_a? FogFileUploader
-
-    uploader = self.new
-
-    uploader.instance_variable_set "@file", fog_file_uploader.file
-    uploader.instance_variable_set "@key", fog_file_uploader.path
-
-    uploader
-  end
-
   ##
-  # Generates the direct upload form for the given attachment.
-  #
-  # @param attachment [Attachment] The attachment for which a file is to be uploaded.
-  # @param success_action_redirect [String] URL to redirect to if successful (none by default, using status).
-  # @param success_action_status [String] The HTTP status to return on success (201 by default).
-  # @param max_file_size [Integer] The maximum file size to be allowed in bytes.
-  def self.direct_fog_hash(
-    attachment:,
-    success_action_redirect: nil,
-    success_action_status: "201",
-    max_file_size: Setting.attachment_max_size * 1024
-  )
-    uploader = for_attachment attachment
+  # This needs to be true so that the necessary condition is included
+  # in S3 upload policy (only relevant for direct uploads).
+  def will_include_content_type
+    true
+  end
 
-    if success_action_redirect.present?
-      uploader.success_action_redirect = success_action_redirect
-      uploader.use_action_status = false
-    else
-      uploader.success_action_status = success_action_status
-      uploader.use_action_status = true
+  class << self
+    def for_attachment(attachment)
+      for_uploader attachment.file
     end
 
-    hash = uploader.direct_fog_hash(enforce_utf8: false, max_file_size: max_file_size)
+    def for_uploader(fog_file_uploader)
+      raise ArgumentError, "FogFileUploader expected" unless fog_file_uploader.is_a? FogFileUploader
 
-    if success_action_redirect.present?
-      hash.merge(success_action_redirect: success_action_redirect)
-    else
-      hash.merge(success_action_status: success_action_status)
+      uploader = self.new
+
+      uploader.instance_variable_set "@file", fog_file_uploader.file
+      uploader.instance_variable_set "@key", fog_file_uploader.path
+      uploader.instance_variable_set "@model", fog_file_uploader.model
+
+      uploader
+    end
+
+    ##
+    # Generates the direct upload form for the given attachment.
+    #
+    # @param attachment [Attachment] The attachment for which a file is to be uploaded.
+    # @param success_action_redirect [String] URL to redirect to if successful (none by default, using status).
+    # @param success_action_status [String] The HTTP status to return on success (201 by default).
+    # @param max_file_size [Integer] The maximum file size to be allowed in bytes.
+    def direct_fog_hash(
+      attachment:,
+      success_action_redirect: nil,
+      success_action_status: "201",
+      max_file_size: Setting.attachment_max_size * 1024
+    )
+      uploader = direct_fog_hash_uploader attachment, success_action_redirect, success_action_status
+      hash = uploader
+        .direct_fog_hash(enforce_utf8: false, max_file_size: max_file_size)
+        .merge(extra_fog_hash_attributes(uploader: uploader))
+
+      if success_action_redirect.present?
+        hash.merge(success_action_redirect: success_action_redirect)
+      else
+        hash.merge(success_action_status: success_action_status)
+      end
+    end
+
+    def extra_fog_hash_attributes(uploader:)
+      return {} unless include_content_type?(uploader)
+
+      {
+        "Content-Type": uploader.fog_attributes[:"Content-Type"]
+      }
+    end
+
+    private
+
+    def include_content_type?(uploader)
+      uploader.will_include_content_type && uploader.fog_attributes.include?(:"Content-Type")
+    end
+
+    def direct_fog_hash_uploader(attachment, success_action_redirect, success_action_status)
+      for_attachment(attachment).tap do |uploader|
+        if success_action_redirect.present?
+          uploader.success_action_redirect = success_action_redirect
+          uploader.use_action_status = false
+        else
+          uploader.success_action_status = success_action_status
+          uploader.use_action_status = true
+        end
+      end
     end
   end
 end

app/uploaders/fog_file_uploader.rb

@@ -57,6 +57,16 @@ class FogFileUploader < CarrierWave::Uploader::Base
     super
   end
 
+  ##
+  # This is necessary for carrierwave to set the Content-Type in the S3 metadata for instance.
+  def fog_attributes
+    content_type = model.content_type
+
+    return super if content_type.blank?
+
+    super.merge "Content-Type": content_type
+  end
+
   ##
   # Generates a download URL for this file.
   #

modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb

@@ -68,8 +68,8 @@ describe 'API v3 Attachments by budget resource', type: :request do
     let(:permissions) { %i[view_budgets edit_budgets] }
 
     let(:request_path) { api_v3_paths.attachments_by_budget budget.id }
-    let(:request_parts) { { metadata: metadata, file: file } }
-    let(:metadata) { { fileName: 'cat.png' }.to_json }
+    let(:request_parts) { { metadata: metadata.to_json, file: file } }
+    let(:metadata) { { fileName: 'cat.png' } }
     let(:file) { mock_uploaded_file(name: 'original-filename.txt') }
     let(:max_file_size) { 1 } # given in kiB
 
@@ -99,19 +99,19 @@ describe 'API v3 Attachments by budget resource', type: :request do
     context 'file section is missing' do
       # rack-test won't send a multipart request without a file being present
       # however as long as we depend on correctly named sections this test should do just fine
-      let(:request_parts) { { metadata: metadata, wrongFileSection: file } }
+      let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } }
 
       it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error')
     end
 
     context 'metadata section is no valid JSON' do
-      let(:metadata) { '"fileName": "cat.png"' }
+      let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } }
 
       it_behaves_like 'parse error'
     end
 
     context 'metadata is missing the fileName' do
-      let(:metadata) { Hash.new.to_json }
+      let(:metadata) { Hash.new }
 
       it_behaves_like 'constraint violation' do
         let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" }

modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb

@@ -69,8 +69,8 @@ describe 'API v3 Attachments by document resource', type: :request do
     let(:permissions) { %i[view_documents manage_documents] }
 
     let(:request_path) { api_v3_paths.attachments_by_document document.id }
-    let(:request_parts) { { metadata: metadata, file: file } }
-    let(:metadata) { { fileName: 'cat.png' }.to_json }
+    let(:request_parts) { { metadata: metadata.to_json, file: file } }
+    let(:metadata) { { fileName: 'cat.png' } }
     let(:file) { mock_uploaded_file(name: 'original-filename.txt') }
     let(:max_file_size) { 1 } # given in kiB
 
@@ -100,19 +100,19 @@ describe 'API v3 Attachments by document resource', type: :request do
     context 'file section is missing' do
       # rack-test won't send a multipart request without a file being present
       # however as long as we depend on correctly named sections this test should do just fine
-      let(:request_parts) { { metadata: metadata, wrongFileSection: file } }
+      let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } }
 
       it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error')
     end
 
     context 'metadata section is no valid JSON' do
-      let(:metadata) { '"fileName": "cat.png"' }
+      let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } }
 
       it_behaves_like 'parse error'
     end
 
     context 'metadata is missing the fileName' do
-      let(:metadata) { Hash.new.to_json }
+      let(:metadata) { Hash.new }
 
       it_behaves_like 'constraint violation' do
         let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" }

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

@@ -42,8 +42,8 @@ shared_examples 'it supports direct uploads' do
   end
 
   describe 'POST /prepare', with_settings: { attachment_max_size: 512 } do
-    let(:request_parts) { { metadata: metadata, file: file } }
-    let(:metadata) { { fileName: 'cat.png', fileSize: file.size }.to_json }
+    let(:request_parts) { { metadata: metadata.to_json, file: file } }
+    let(:metadata) { { fileName: 'cat.png', fileSize: file.size, contentType: 'image/png' } }
     let(:file) { mock_uploaded_file(name: 'original-filename.txt') }
 
     def request!
@@ -68,7 +68,7 @@ shared_examples 'it supports direct uploads' do
       end
 
       context 'with no filesize metadata' do
-        let(:metadata) { { fileName: 'cat.png' }.to_json }
+        let(:metadata) { { fileName: 'cat.png' } }
 
         it 'should respond with 422 due to missing file size metadata' do
           expect(subject.status).to eq(422)
@@ -125,8 +125,21 @@ shared_examples 'it supports direct uploads' do
                   "success_action_status"
                 )
 
+                expect(fields["Content-Type"]).to eq metadata[:contentType]
+
                 expect(fields["key"]).to end_with "cat.png"
               end
+
+              it 'should also include the content type and the necessary policy in the form fields' do
+                fields = link["form_fields"]
+
+                expect(fields).to include("policy", "Content-Type")
+                expect(fields["Content-Type"]).to eq metadata[:contentType]
+
+                policy = Base64.decode64 fields["policy"]
+
+                expect(policy).to include '["starts-with","$Content-Type",""]'
+              end
             end
           end
         end
@@ -214,8 +227,8 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j
     let(:permissions) { Array(update_permission) }
 
     let(:request_path) { api_v3_paths.attachments }
-    let(:request_parts) { { metadata: metadata, file: file } }
-    let(:metadata) { { fileName: 'cat.png' }.to_json }
+    let(:request_parts) { { metadata: metadata.to_json, file: file } }
+    let(:metadata) { { fileName: 'cat.png' } }
     let(:file) { mock_uploaded_file(name: 'original-filename.txt') }
     let(:max_file_size) { 1 } # given in kiB
 
@@ -248,19 +261,19 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j
     context 'file section is missing' do
       # rack-test won't send a multipart request without a file being present
       # however as long as we depend on correctly named sections this test should do just fine
-      let(:request_parts) { { metadata: metadata, wrongFileSection: file } }
+      let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } }
 
       it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error')
     end
 
     context 'metadata section is no valid JSON' do
-      let(:metadata) { '"fileName": "cat.png"' }
+      let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } }
 
       it_behaves_like 'parse error'
     end
 
     context 'metadata is missing the fileName' do
-      let(:metadata) { Hash.new.to_json }
+      let(:metadata) { Hash.new }
 
       it_behaves_like 'constraint violation' do
         let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" }
@@ -496,8 +509,8 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j
 
     describe '#post' do
       let(:request_path) { api_v3_paths.send "attachments_by_#{attachment_type}", container.id }
-      let(:request_parts) { { metadata: metadata, file: file } }
-      let(:metadata) { { fileName: 'cat.png' }.to_json }
+      let(:request_parts) { { metadata: metadata.to_json, file: file } }
+      let(:metadata) { { fileName: 'cat.png' } }
       let(:file) { mock_uploaded_file(name: 'original-filename.txt') }
       let(:max_file_size) { 1 } # given in kiB
 
@@ -527,19 +540,19 @@ shared_examples 'an APIv3 attachment resource', type: :request, content_type: :j
       context 'file section is missing' do
         # rack-test won't send a multipart request without a file being present
         # however as long as we depend on correctly named sections this test should do just fine
-        let(:request_parts) { { metadata: metadata, wrongFileSection: file } }
+        let(:request_parts) { { metadata: metadata.to_json, wrongFileSection: file } }
 
         it_behaves_like 'invalid request body', I18n.t('api_v3.errors.multipart_body_error')
       end
 
       context 'metadata section is no valid JSON' do
-        let(:metadata) { '"fileName": "cat.png"' }
+        let(:request_parts) { { metadata: '"fileName": "cat.png"', file: file } }
 
         it_behaves_like 'parse error'
       end
 
       context 'metadata is missing the fileName' do
-        let(:metadata) { Hash.new.to_json }
+        let(:metadata) { Hash.new }
 
         it_behaves_like 'constraint violation' do
           let(:message) { "fileName #{I18n.t('activerecord.errors.messages.blank')}" }

spec/requests/api/v3/attachments_spec.rb

@@ -50,8 +50,8 @@ describe API::V3::Attachments::AttachmentsAPI, type: :request do
     let(:permissions) { [] }
 
     let(:request_path) { api_v3_paths.prepare_new_attachment_upload }
-    let(:request_parts) { { metadata: metadata, file: file } }
-    let(:metadata) { { fileName: 'cat.png' }.to_json }
+    let(:request_parts) { { metadata: metadata.to_json, file: file } }
+    let(:metadata) { { fileName: 'cat.png' } }
     let(:file) { mock_uploaded_file(name: 'original-filename.txt') }
 
     before do