DEV: Clean up S3 specs, stubs, and helpers

Extracted commonly used spec helpers into spec/support/uploads_helpers.rb, removed unused stubs and let definitions. Makes it easier to write new S3-related specs without copy and pasting setup steps from other specs.

spec/components/cooked_post_processor_spec.rb

@@ -4,17 +4,6 @@
 require "cooked_post_processor"
 require "file_store/s3_store"
 
-def s3_setup
-  Rails.configuration.action_controller.stubs(:asset_host).returns("https://local.cdn.com")
-
-  SiteSetting.s3_upload_bucket = "some-bucket-on-s3"
-  SiteSetting.s3_access_key_id = "s3-access-key-id"
-  SiteSetting.s3_secret_access_key = "s3-secret-access-key"
-  SiteSetting.s3_cdn_url = "https://s3.cdn.com"
-  SiteSetting.enable_s3_uploads = true
-  SiteSetting.authorized_extensions = "png|jpg|gif|mov|ogg|"
-end
-
 describe CookedPostProcessor do
   fab!(:upload) { Fabricate(:upload) }
   let(:upload_path) { Discourse.store.upload_path }
@@ -483,17 +472,16 @@ def s3_setup
 
         context "s3_uploads" do
           let(:upload) { Fabricate(:secure_upload_s3) }
+
           before do
-            s3_setup
+            setup_s3
+            SiteSetting.s3_cdn_url = "https://s3.cdn.com"
+            SiteSetting.authorized_extensions = "png|jpg|gif|mov|ogg|"
+
             stored_path = Discourse.store.get_path_for_upload(upload)
             upload.update_column(:url, "#{SiteSetting.Upload.absolute_base_url}/#{stored_path}")
 
-            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
-            stub_request(
-              :put,
-              "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/optimized/1X/#{upload.sha1}_2_#{optimized_size}.#{upload.extension}"
-            )
-            stub_request(:get, /#{SiteSetting.s3_upload_bucket}\.s3\.amazonaws\.com/)
+            stub_upload(upload)
 
             SiteSetting.login_required = true
             SiteSetting.secure_media = true
@@ -1049,11 +1037,11 @@ def s3_setup
 
       context "when the post is with_secure_media and the upload is secure and secure media is enabled" do
         before do
+          setup_s3
           upload.update(secure: true)
+
           SiteSetting.login_required = true
-          s3_setup
           SiteSetting.secure_media = true
-          stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
         end
 
         it "does not use the direct URL, uses the cooked URL instead (because of the private ACL preventing w/h fetch)" do
@@ -1269,7 +1257,11 @@ def s3_setup
 
       context "s3_uploads" do
         before do
-          s3_setup
+          Rails.configuration.action_controller.stubs(:asset_host).returns("https://local.cdn.com")
+
+          setup_s3
+          SiteSetting.s3_cdn_url = "https://s3.cdn.com"
+          SiteSetting.authorized_extensions = "png|jpg|gif|mov|ogg|"
 
           uploaded_file = file_from_fixtures("smallest.png")
           upload_sha1 = Digest::SHA1.hexdigest(File.read(uploaded_file))
@@ -1546,9 +1538,7 @@ def s3_setup
       end
 
       it "doesn't disable download_remote_images_to_local if site uses S3" do
-        SiteSetting.s3_access_key_id = "s3-access-key-id"
-        SiteSetting.s3_secret_access_key = "s3-secret-access-key"
-        SiteSetting.enable_s3_uploads = true
+        setup_s3
         cpp.disable_if_low_on_disk_space
 
         expect(SiteSetting.download_remote_images_to_local).to eq(true)

spec/components/email/sender_spec.rb

@@ -408,25 +408,10 @@
     end
 
     context "when secure media enabled" do
-      def enable_s3_uploads
-        SiteSetting.enable_s3_uploads = true
-        SiteSetting.s3_upload_bucket = "s3-upload-bucket"
-        SiteSetting.s3_access_key_id = "some key"
-        SiteSetting.s3_secret_access_key = "some secrets3_region key"
-        stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
-        stub_request(
-          :put,
-          "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/original/1X/#{image.sha1}.#{image.extension}?acl"
-        )
-        store = FileStore::S3Store.new
-        s3_helper = store.instance_variable_get(:@s3_helper)
-        client = Aws::S3::Client.new(stub_responses: true)
-        s3_helper.stubs(:s3_client).returns(client)
-        Discourse.stubs(:store).returns(store)
-      end
-
       before do
-        enable_s3_uploads
+        setup_s3
+        stub_s3_store
+
         SiteSetting.secure_media = true
         SiteSetting.login_required = true
         SiteSetting.email_total_attachment_size_limit_kb = 14_000

spec/components/file_store/base_store_spec.rb

@@ -47,12 +47,7 @@
 
   describe '#download' do
     before do
-      SiteSetting.enable_s3_uploads = true
-      SiteSetting.s3_upload_bucket = "s3-upload-bucket"
-      SiteSetting.s3_access_key_id = "some key"
-      SiteSetting.s3_secret_access_key = "some secret key"
-      SiteSetting.s3_region = "us-east-1"
-
+      setup_s3
       stub_request(:get, upload_s3.url).to_return(status: 200, body: "Hello world")
     end
 
@@ -78,7 +73,7 @@
     end
 
     it "should return the file when s3 cdn enabled" do
-      SiteSetting.s3_cdn_url = "https://cdn.s3.amazonaws.com"
+      SiteSetting.s3_cdn_url = "https://cdn.s3.#{SiteSetting.s3_region}.amazonaws.com"
       stub_request(:get, Discourse.store.cdn_url(upload_s3.url)).to_return(status: 200, body: "Hello world")
 
       file = store.download(upload_s3)
@@ -90,7 +85,7 @@
       SiteSetting.login_required = true
       SiteSetting.secure_media = true
 
-      stub_request(:head, "https://s3-upload-bucket.s3.amazonaws.com/")
+      stub_request(:head, "https://s3-upload-bucket.s3.#{SiteSetting.s3_region}.amazonaws.com/")
       signed_url = Discourse.store.signed_url_for_path(upload_s3.url)
       stub_request(:get, signed_url).to_return(status: 200, body: "Hello world")
 

spec/components/file_store/s3_store_spec.rb

@@ -5,42 +5,26 @@
 require 'file_store/local_store'
 
 describe FileStore::S3Store do
-
   let(:store) { FileStore::S3Store.new }
   let(:s3_helper) { store.instance_variable_get(:@s3_helper) }
-  fab!(:upload) { Fabricate(:upload) }
-  let(:uploaded_file) { file_from_fixtures("logo.png") }
+  let(:client) { Aws::S3::Client.new(stub_responses: true) }
+  let(:resource) { Aws::S3::Resource.new(client: client) }
+  let(:s3_bucket) { resource.bucket("s3-upload-bucket") }
+  let(:s3_object) { stub }
 
   fab!(:optimized_image) { Fabricate(:optimized_image) }
   let(:optimized_image_file) { file_from_fixtures("logo.png") }
-
-  before(:each) do
-    SiteSetting.s3_upload_bucket = "s3-upload-bucket"
-    SiteSetting.s3_access_key_id = "s3-access-key-id"
-    SiteSetting.s3_secret_access_key = "s3-secret-access-key"
-    SiteSetting.enable_s3_uploads = true
+  let(:uploaded_file) { file_from_fixtures("logo.png") }
+  fab!(:upload) do
+    Fabricate(:upload, sha1: Digest::SHA1.hexdigest('secreet image string'))
   end
 
-  shared_context 's3 helpers' do
-    fab!(:upload) do
-      Fabricate(:upload, sha1: Digest::SHA1.hexdigest('secreet image string'))
-    end
-
-    let(:store) { FileStore::S3Store.new }
-    let(:client) { Aws::S3::Client.new(stub_responses: true) }
-    let(:resource) { Aws::S3::Resource.new(client: client) }
-    let(:s3_bucket) { resource.bucket("s3-upload-bucket") }
-    let(:s3_helper) { store.instance_variable_get(:@s3_helper) }
-
-    before do
-      SiteSetting.s3_region = 'us-west-1'
-    end
+  before do
+    setup_s3
+    SiteSetting.s3_region = 'us-west-1'
   end
 
   context 'uploading to s3' do
-    include_context "s3 helpers"
-
-    let(:s3_object) { stub }
     let(:etag) { "etag" }
 
     describe "#store_upload" do
@@ -159,8 +143,6 @@
   end
 
   context 'copying files in S3' do
-    include_context "s3 helpers"
-
     describe '#copy_file' do
       it "copies the from in S3 with the right paths" do
         s3_helper.expects(:s3_bucket).returns(s3_bucket)
@@ -186,8 +168,6 @@
   end
 
   context 'removal from s3' do
-    include_context "s3 helpers"
-
     describe "#remove_upload" do
       it "removes the file from s3 with the right paths" do
         store.expects(:get_depth_for).with(upload.id).returns(0)
@@ -243,8 +223,6 @@
     end
 
     describe "#remove_optimized_image" do
-      fab!(:optimized_image) { Fabricate(:optimized_image, upload: upload) }
-
       let(:image_path) do
         FileStore::BaseStore.new.get_path_for_optimized_image(optimized_image)
       end
@@ -303,23 +281,22 @@
   end
 
   describe ".has_been_uploaded?" do
-
     it "doesn't crash for invalid URLs" do
       expect(store.has_been_uploaded?("https://site.discourse.com/#bad#6")).to eq(false)
     end
 
     it "doesn't crash if URL contains non-ascii characters" do
-      expect(store.has_been_uploaded?("//s3-upload-bucket.s3.dualstack.us-east-1.amazonaws.com/漢1337.png")).to eq(true)
+      expect(store.has_been_uploaded?("//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/漢1337.png")).to eq(true)
       expect(store.has_been_uploaded?("//s3-upload-bucket.s3.amazonaws.com/漢1337.png")).to eq(false)
     end
 
     it "identifies S3 uploads" do
-      expect(store.has_been_uploaded?("//s3-upload-bucket.s3.dualstack.us-east-1.amazonaws.com/1337.png")).to eq(true)
+      expect(store.has_been_uploaded?("//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/1337.png")).to eq(true)
     end
 
     it "does not match other s3 urls" do
       expect(store.has_been_uploaded?("//s3-upload-bucket.s3.amazonaws.com/1337.png")).to eq(false)
-      expect(store.has_been_uploaded?("//s3-upload-bucket.s3-us-east-1.amazonaws.com/1337.png")).to eq(false)
+      expect(store.has_been_uploaded?("//s3-upload-bucket.s3-us-west-1.amazonaws.com/1337.png")).to eq(false)
       expect(store.has_been_uploaded?("//s3.amazonaws.com/s3-upload-bucket/1337.png")).to eq(false)
       expect(store.has_been_uploaded?("//s4_upload_bucket.s3.amazonaws.com/1337.png")).to eq(false)
     end
@@ -328,7 +305,7 @@
 
   describe ".absolute_base_url" do
     it "returns a lowercase schemaless absolute url" do
-      expect(store.absolute_base_url).to eq("//s3-upload-bucket.s3.dualstack.us-east-1.amazonaws.com")
+      expect(store.absolute_base_url).to eq("//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com")
     end
 
     it "uses the proper endpoint" do
@@ -353,12 +330,10 @@
   end
 
   describe ".purge_tombstone" do
-
     it "updates tombstone lifecycle" do
       s3_helper.expects(:update_tombstone_lifecycle)
       store.purge_tombstone(1.day)
     end
-
   end
 
   describe ".path_for" do
@@ -380,9 +355,6 @@ def assert_path(path, expected)
   end
 
   context 'update ACL' do
-    include_context "s3 helpers"
-    let(:s3_object) { stub }
-
     before do
       SiteSetting.authorized_extensions = "pdf|png"
     end
@@ -411,7 +383,6 @@ def assert_path(path, expected)
   end
 
   describe '.cdn_url' do
-
     it 'supports subfolder' do
       SiteSetting.s3_upload_bucket = 's3-upload-bucket/livechat'
       SiteSetting.s3_cdn_url = 'https://rainbow.com'
@@ -420,25 +391,19 @@ def assert_path(path, expected)
       # subfolder should not leak into uploads
       set_subfolder "/community"
 
-      url = "//s3-upload-bucket.s3.dualstack.us-east-1.amazonaws.com/livechat/original/gif.png"
+      url = "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/livechat/original/gif.png"
 
       expect(store.cdn_url(url)).to eq("https://rainbow.com/original/gif.png")
     end
   end
 
   describe ".download_url" do
-    include_context "s3 helpers"
-    let(:s3_object) { stub }
-
     it "returns correct short URL with dl=1 param" do
       expect(store.download_url(upload)).to eq("#{upload.short_path}?dl=1")
     end
   end
 
   describe ".url_for" do
-    include_context "s3 helpers"
-    let(:s3_object) { stub }
-
     it "returns signed URL with content disposition when requesting to download image" do
       s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
       s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object)
@@ -454,9 +419,6 @@ def assert_path(path, expected)
   end
 
   describe ".signed_url_for_path" do
-    include_context "s3 helpers"
-    let(:s3_object) { stub }
-
     it "returns signed URL for a given path" do
       s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
       s3_bucket.expects(:object).with("special/optimized/file.png").returns(s3_object)
@@ -469,5 +431,4 @@ def assert_path(path, expected)
       expect(store.signed_url_for_path("special/optimized/file.png")).not_to eq(upload.url)
     end
   end
-
 end

spec/components/post_creator_spec.rb

@@ -1591,20 +1591,10 @@
     fab!(:public_topic) { Fabricate(:topic) }
 
     before do
-      SiteSetting.enable_s3_uploads = true
+      setup_s3
       SiteSetting.authorized_extensions = "png|jpg|gif|mp4"
-      SiteSetting.s3_upload_bucket = "s3-upload-bucket"
-      SiteSetting.s3_access_key_id = "some key"
-      SiteSetting.s3_secret_access_key = "some secret key"
-      SiteSetting.s3_region = "us-east-1"
       SiteSetting.secure_media = true
-
-      stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
-
-      stub_request(
-        :put,
-        "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/original/1X/#{image_upload.sha1}.#{image_upload.extension}?acl"
-      )
+      stub_upload(image_upload)
     end
 
     it "links post uploads" do

spec/components/pretty_text_spec.rb

@@ -902,11 +902,8 @@ def strip_image_wrapping(html)
 
     describe "#strip_secure_media" do
       before do
-        SiteSetting.s3_upload_bucket = "some-bucket-on-s3"
-        SiteSetting.s3_access_key_id = "s3-access-key-id"
-        SiteSetting.s3_secret_access_key = "s3-secret-access-key"
+        setup_s3
         SiteSetting.s3_cdn_url = "https://s3.cdn.com"
-        SiteSetting.enable_s3_uploads = true
         SiteSetting.secure_media = true
         SiteSetting.login_required = true
       end

spec/components/s3_helper_spec.rb

@@ -6,11 +6,8 @@
 describe "S3Helper" do
   let(:client) { Aws::S3::Client.new(stub_responses: true) }
 
-  before(:each) do
-    SiteSetting.enable_s3_uploads = true
-    SiteSetting.s3_access_key_id = "abc"
-    SiteSetting.s3_secret_access_key = "def"
-    SiteSetting.s3_region = "us-east-1"
+  before do
+    setup_s3
 
     @lifecycle = <<~XML
       <?xml version="1.0" encoding="UTF-8"?>
@@ -41,10 +38,10 @@
     stub_request(:get, "http://169.254.169.254/latest/meta-data/iam/security-credentials/").
       to_return(status: 404, body: "", headers: {})
 
-    stub_request(:get, "https://bob.s3.amazonaws.com/?lifecycle").
+    stub_request(:get, "https://bob.s3.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle").
       to_return(status: 200, body: @lifecycle, headers: {})
 
-    stub_request(:put, "https://bob.s3.amazonaws.com/?lifecycle").
+    stub_request(:put, "https://bob.s3.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle").
       with do |req|
 
       hash = Hash.from_xml(req.body.to_s)