DEV: Add tests to S3Helper.ensure_cors and move rules to class (#14767)

DEV: Add tests to S3Helper.ensure_cors and move rules to class (#14767)

In preparation for adding automatic CORS rules creation for direct S3 uploads, I am adding tests here and moving the CORS rule definitions into a dedicated class so they are all in the one place.

There is a problem with ensure_cors! as well – if there is already a CORS rule defined (presumably the asset one) then we do nothing and do not apply the new rule. This means that the S3BackupStore.ensure_cors method does nothing right now if the assets rule is already defined, and it will mean the same for any direct S3 upload rules I add for uppy. We need to be able to add more rules, not just one.

This is not a problem on our hosting because we define the rules at an infra level.

diff --git a/lib/backup_restore/s3_backup_store.rb b/lib/backup_restore/s3_backup_store.rb
index 71c3684..cb2fcd7 100644
--- a/lib/backup_restore/s3_backup_store.rb
+++ b/lib/backup_restore/s3_backup_store.rb
@@ -83,14 +83,7 @@ module BackupRestore
     end
 
     def ensure_cors!
-      rule = {
-        allowed_headers: ["*"],
-        allowed_methods: ["PUT"],
-        allowed_origins: [Discourse.base_url_no_prefix],
-        max_age_seconds: 3000
-      }
-
-      @s3_helper.ensure_cors!([rule])
+      @s3_helper.ensure_cors!([S3CorsRulesets::BACKUP_DIRECT_UPLOAD])
     end
 
     def cleanup_allowed?
diff --git a/lib/s3_cors_rulesets.rb b/lib/s3_cors_rulesets.rb
new file mode 100644
index 0000000..88f76f3
--- /dev/null
+++ b/lib/s3_cors_rulesets.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class S3CorsRulesets
+  ASSETS = {
+    allowed_headers: ["Authorization"],
+    allowed_methods: ["GET", "HEAD"],
+    allowed_origins: ["*"],
+    max_age_seconds: 3000
+  }.freeze
+
+  BACKUP_DIRECT_UPLOAD = {
+    allowed_headers: ["*"],
+    allowed_methods: ["PUT"],
+    allowed_origins: [Discourse.base_url_no_prefix],
+    max_age_seconds: 3000
+  }.freeze
+end
diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb
index f5a5773..253269b 100644
--- a/lib/s3_helper.rb
+++ b/lib/s3_helper.rb
@@ -140,12 +140,7 @@ class S3Helper
     end
 
     unless rule
-      rules = [{
-        allowed_headers: ["Authorization"],
-        allowed_methods: ["GET", "HEAD"],
-        allowed_origins: ["*"],
-        max_age_seconds: 3000
-      }] if rules.nil?
+      rules = [S3CorsRulesets::ASSETS] if rules.nil?
 
       s3_resource.client.put_bucket_cors(
         bucket: @s3_bucket_name,
diff --git a/spec/components/s3_helper_spec.rb b/spec/components/s3_helper_spec.rb
index daf229f..39df6ff 100644
--- a/spec/components/s3_helper_spec.rb
+++ b/spec/components/s3_helper_spec.rb
@@ -129,4 +129,41 @@ describe "S3Helper" do
       expect(response.second).to eq("etag")
     end
   end
+
+  describe "#ensure_cors" do
+    let(:s3_helper) { S3Helper.new("test-bucket", "", client: client) }
+
+    it "does nothing if !s3_install_cors_rule" do
+      SiteSetting.s3_install_cors_rule = false
+      s3_helper.expects(:s3_resource).never
+      s3_helper.ensure_cors!
+    end
+
+    it "creates the assets rule if no rule exists" do
+      s3_helper.s3_client.stub_responses(:get_bucket_cors, Aws::S3::Errors::NoSuchCORSConfiguration.new("", {}))
+      s3_helper.s3_client.expects(:put_bucket_cors).with(
+        bucket: s3_helper.s3_bucket_name,
+        cors_configuration: {
+          cors_rules: [S3CorsRulesets::ASSETS]
+        }
+      )
+      s3_helper.ensure_cors!
+    end
+
+    it "does nothing if a rule already exists" do
+      s3_helper.s3_client.stub_responses(:get_bucket_cors, {
+        cors_rules: [S3CorsRulesets::ASSETS]
+      })
+      s3_helper.s3_client.expects(:put_bucket_cors).never
+      s3_helper.ensure_cors!
+    end
+
+    it "does not apply the passed in rules if a different rule already exists" do
+      s3_helper.s3_client.stub_responses(:get_bucket_cors, {
+        cors_rules: [S3CorsRulesets::ASSETS]
+      })
+      s3_helper.s3_client.expects(:put_bucket_cors).never
+      s3_helper.ensure_cors!([S3CorsRulesets::BACKUP_DIRECT_UPLOAD])
+    end
+  end
 end

GitHub sha: a059c7251f0f8d2fa4668ca5e9d350e015371d9d

This commit appears in #14767 which was approved by CvX. It was merged by martin.