FIX: Upload#update_secure_status not updating s3 access control (#33232)

This commit is a follow-up to b02bc707de
where `Upload#update_secure_status` does not call
`FileStore::S3Store#update_upload_access_control` if
`s3_use_acls` is disabled. This is no longer correct as an upload's
access control on S3 can now be based on tags if the
`s3_enable_access_control_tags` site setting is
enabled.

To fix this, this commit removes the `s3_use_acls` check in
`Upload#update_secure_status` and updates
`FileStore::S3Store#default_s3_options`
to not set the `acl` option if the `s3_use_acls` site setting is
disabled.

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
 

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 =

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:,

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)

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)