From 51a40c97eb0315fd2a5dd86dfd7333f9d8d15d92 Mon Sep 17 00:00:00 2001 From: Markus Kahl Date: Mon, 25 Jan 2021 09:33:05 +0000 Subject: [PATCH] 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 | 107 +++++++++++------- app/uploaders/fog_file_uploader.rb | 10 ++ .../attachments_by_budget_resource_spec.rb | 10 +- .../attachments_by_documents_resource_spec.rb | 10 +- .../attachment_resource_shared_examples.rb | 39 ++++--- spec/requests/api/v3/attachments_spec.rb | 4 +- 6 files changed, 116 insertions(+), 64 deletions(-) diff --git a/app/uploaders/direct_fog_uploader.rb b/app/uploaders/direct_fog_uploader.rb index 037a3cea8b8..a40748d2799 100644 --- a/app/uploaders/direct_fog_uploader.rb +++ b/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 diff --git a/app/uploaders/fog_file_uploader.rb b/app/uploaders/fog_file_uploader.rb index ecff0bca84e..253339de010 100644 --- a/app/uploaders/fog_file_uploader.rb +++ b/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. # diff --git a/modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb b/modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb index 911c3958c98..207eb52a2ab 100644 --- a/modules/costs/spec/requests/api/attachments/attachments_by_budget_resource_spec.rb +++ b/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')}" } diff --git a/modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb b/modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb index 5d842fb5940..9d331d7926e 100644 --- a/modules/documents/spec/requests/api/v3/attachments/attachments_by_documents_resource_spec.rb +++ b/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')}" } diff --git a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb index cf4a6fbb82d..1f3a1af56cb 100644 --- a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb +++ b/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')}" } diff --git a/spec/requests/api/v3/attachments_spec.rb b/spec/requests/api/v3/attachments_spec.rb index 3940f034232..dd1f7c4eb1d 100644 --- a/spec/requests/api/v3/attachments_spec.rb +++ b/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