From a059c7251f0f8d2fa4668ca5e9d350e015371d9d Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Mon, 1 Nov 2021 08:23:13 +1000
Subject: [PATCH] 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 |  9 +------
 lib/s3_cors_rulesets.rb               | 17 ++++++++++++
 lib/s3_helper.rb                      |  7 +----
 spec/components/s3_helper_spec.rb     | 37 +++++++++++++++++++++++++++
 4 files changed, 56 insertions(+), 14 deletions(-)
 create mode 100644 lib/s3_cors_rulesets.rb

diff --git a/lib/backup_restore/s3_backup_store.rb b/lib/backup_restore/s3_backup_store.rb
index 71c368405df..cb2fcd72dea 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 00000000000..88f76f33527
--- /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 f5a57732953..253269b5d9a 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 daf229fc6fe..39df6ffeeca 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