diff --git a/app/models/upload.rb b/app/models/upload.rb index 800ce160e03..b7c38ccd249 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -493,7 +493,7 @@ class Upload < ActiveRecord::Base secure_status_did_change = self.secure? != mark_secure self.update(secure_params(mark_secure, reason, source)) - if secure_status_did_change && SiteSetting.s3_use_acls && Discourse.store.external? + if secure_status_did_change && Discourse.store.external? Discourse.store.update_upload_access_control(self) end diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index aab0a0ef8e6..2ca5abc92b6 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -395,7 +395,11 @@ module FileStore end def self.default_s3_options(secure:) - options = { acl: acl_option_value(secure:) } + options = {} + + if acl_value = acl_option_value(secure:) + options[:acl] = acl_value + end if tagging_option_value = visibility_tagging_option_value(secure:) options[:tagging] = tagging_option_value @@ -438,13 +442,15 @@ module FileStore end def update_access_control(key, secure) - begin - object = object_from_path(key).acl.put(acl: acl_option_value(secure:)) - rescue Aws::S3::Errors::NotImplemented => err - Discourse.warn_exception( - err, - message: "The file store object storage provider does not support setting ACLs", - ) + if acl = self.class.acl_option_value(secure:) + begin + object = object_from_path(key).acl.put(acl:) + rescue Aws::S3::Errors::NotImplemented => err + Discourse.warn_exception( + err, + message: "The file store object storage provider does not support setting ACLs", + ) + end end if tagging_option_value = diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index f7e6afff244..6e253925688 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -306,7 +306,7 @@ class S3Helper s3_client.abort_multipart_upload(bucket: s3_bucket_name, key: key, upload_id: upload_id) end - def create_multipart(key, content_type, metadata: {}, acl:, tagging: nil) + def create_multipart(key, content_type, metadata: {}, acl: nil, tagging: nil) response = s3_client.create_multipart_upload( acl:, diff --git a/spec/lib/file_store/s3_store_spec.rb b/spec/lib/file_store/s3_store_spec.rb index 3a1dc2170ac..16d2786f245 100644 --- a/spec/lib/file_store/s3_store_spec.rb +++ b/spec/lib/file_store/s3_store_spec.rb @@ -191,7 +191,6 @@ RSpec.describe FileStore::S3Store do .expects(:put) .with( { - acl: nil, cache_control: "max-age=31556952, public, immutable", content_type: "application/pdf", content_disposition: "inline; filename=\"small.pdf\"; filename*=UTF-8''small.pdf", @@ -563,26 +562,47 @@ RSpec.describe FileStore::S3Store do describe ".update_upload_access_control" do let(:upload) { Fabricate(:upload, original_filename: "small.pdf", extension: "pdf") } - it "sets acl to public by default" do - s3_helper.expects(:s3_bucket).returns(s3_bucket) - expect_upload_access_control_update(upload, FileStore::S3Store::CANNED_ACL_PUBLIC_READ) + before { s3_helper.stub_client_responses! } + it "sets acl to public by default" do expect(store.update_upload_access_control(upload)).to be_truthy + + put_object_acl_request = + store.s3_helper.s3_client.api_requests.find do |api_request| + api_request[:operation_name] == :put_object_acl + end + + expect(put_object_acl_request[:context].params[:acl]).to eq( + FileStore::S3Store::CANNED_ACL_PUBLIC_READ, + ) end it "sets acl to private when upload is marked secure" do upload.update!(secure: true) - s3_helper.expects(:s3_bucket).returns(s3_bucket) - expect_upload_access_control_update(upload, FileStore::S3Store::CANNED_ACL_PRIVATE) expect(store.update_upload_access_control(upload)).to be_truthy + + put_object_acl_request = + store.s3_helper.s3_client.api_requests.find do |api_request| + api_request[:operation_name] == :put_object_acl + end + + expect(put_object_acl_request[:context].params[:acl]).to eq( + FileStore::S3Store::CANNED_ACL_PRIVATE, + ) + end + + it "does not set acl when `s3_use_acls` site setting is disabled" do + SiteSetting.s3_use_acls = false + + upload.update!(secure: true) + + expect(store.update_upload_access_control(upload)).to be_truthy + expect(s3_helper.s3_client.api_requests).to be_empty end describe "when `s3_enable_access_control_tags` site setting is enabled" do - before do - SiteSetting.s3_enable_access_control_tags = true - s3_helper.stub_client_responses! - end + before { SiteSetting.s3_enable_access_control_tags = true } it "set the right tagging option for a public upload" do upload.update!(secure: false) diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 69bb519f1cf..6a21a7af44f 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -475,15 +475,6 @@ RSpec.describe Upload do upload.update_secure_status end - it "does not attempt to change the ACL if s3_use_acls is disabled" do - SiteSetting.secure_uploads = false - SiteSetting.s3_use_acls = false - FileStore::S3Store.any_instance.expects(:update_upload_access_control).with(upload).never - upload.update!(secure: true, access_control_post: Fabricate(:post)) - upload.update_secure_status - expect(upload.secure).to eq(false) - end - it "marks an image upload as secure if login_required is enabled" do SiteSetting.login_required = true upload.update!(secure: false)