From d9a422cf61738f9b5135b142633b31953dbc697b Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Fri, 8 Dec 2023 12:58:45 +1000
Subject: [PATCH] FIX: Do not attempt S3 ACL call if secure status did not
 change (#24785)

When rebaking and in various other places for posts, we
run through the uploads and call `update_secure_status` on
each of them.

However, if the secure status didn't change, we were still
calling S3 to change the ACL, which would have been a noop
in many cases and takes ~1 second per call, slowing things
down a lot.

Also, we didn't account for the s3_acls_enabled site setting
being false here, and in the specs doing an assertion
that `Discourse.store.update_ACL` is not called doesn't
work; `Discourse.store` isn't a singleton, it re-initializes
`FileStore::S3Store.new` every single time.
---
 app/models/upload.rb       |  2 +-
 spec/models/upload_spec.rb | 16 ++++++++++++++++
 spec/tasks/uploads_spec.rb |  2 +-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/app/models/upload.rb b/app/models/upload.rb
index 60b39315c3c..e1fda0ead3a 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -470,7 +470,7 @@ class Upload < ActiveRecord::Base
     secure_status_did_change = self.secure? != mark_secure
     self.update(secure_params(mark_secure, reason, source))
 
-    if Discourse.store.external?
+    if secure_status_did_change && SiteSetting.s3_use_acls && Discourse.store.external?
       begin
         Discourse.store.update_upload_ACL(self)
       rescue Aws::S3::Errors::NotImplemented => err
diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb
index e1fab6b426b..e644146753a 100644
--- a/spec/models/upload_spec.rb
+++ b/spec/models/upload_spec.rb
@@ -444,6 +444,7 @@ RSpec.describe Upload do
       end
 
       it "marks the upload as not secure if its access control post is a public post" do
+        FileStore::S3Store.any_instance.expects(:update_upload_ACL).with(upload)
         upload.update!(secure: true, access_control_post: Fabricate(:post))
         upload.update_secure_status
         expect(upload.secure).to eq(false)
@@ -455,6 +456,21 @@ RSpec.describe Upload do
         expect(upload.secure).to eq(true)
       end
 
+      it "does not attempt to change the ACL if the secure status has not changed" do
+        FileStore::S3Store.any_instance.expects(:update_upload_ACL).with(upload).never
+        upload.update!(secure: true, access_control_post: Fabricate(:private_message_post))
+        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_ACL).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)
diff --git a/spec/tasks/uploads_spec.rb b/spec/tasks/uploads_spec.rb
index 0f1a3693e1e..f2470377deb 100644
--- a/spec/tasks/uploads_spec.rb
+++ b/spec/tasks/uploads_spec.rb
@@ -146,7 +146,7 @@ RSpec.describe "tasks/uploads" do
           end
 
           it "does not attempt to update the acl" do
-            Discourse.store.expects(:update_upload_ACL).with(upload2).never
+            FileStore::S3Store.any_instance.expects(:update_upload_ACL).with(upload2).never
             invoke_task
           end
         end