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.

lib/backup_restore/s3_backup_store.rb

@@ -83,14 +83,7 @@ module BackupRestore
     end
 
     def ensure_cors!
-      rule = {
+      @s3_helper.ensure_cors!([S3CorsRulesets::BACKUP_DIRECT_UPLOAD])
-        allowed_headers: ["*"],
-        allowed_methods: ["PUT"],
-        allowed_origins: [Discourse.base_url_no_prefix],
-        max_age_seconds: 3000
-      }
-
-      @s3_helper.ensure_cors!([rule])
     end
 
     def cleanup_allowed?

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

lib/s3_helper.rb

@@ -140,12 +140,7 @@ class S3Helper
     end
 
     unless rule
-      rules = [{
+      rules = [S3CorsRulesets::ASSETS] if rules.nil?
-        allowed_headers: ["Authorization"],
-        allowed_methods: ["GET", "HEAD"],
-        allowed_origins: ["*"],
-        max_age_seconds: 3000
-      }] if rules.nil?
 
       s3_resource.client.put_bucket_cors(
         bucket: @s3_bucket_name,

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