DEV: Add S3 upload system specs using minio (#22975)

This commit adds some system specs to test uploads with
direct to S3 single and multipart uploads via uppy. This
is done with minio as a local S3 replacement. We are doing
this to catch regressions when uppy dependencies need to
be upgraded or we change uppy upload code, since before
this there was no way to know outside manual testing whether
these changes would cause regressions.

Minio's server lifecycle and the installed binaries are managed
by the https://github.com/discourse/minio_runner gem, though the
binaries are already installed on the discourse_test image we run
GitHub CI from.

These tests will only run in CI unless you specifically use the
CI=1 or RUN_S3_SYSTEM_SPECS=1 env vars.

For a history of experimentation here see https://github.com/discourse/discourse/pull/22381

Related PRs:

* https://github.com/discourse/minio_runner/pull/1
* https://github.com/discourse/minio_runner/pull/2
* https://github.com/discourse/minio_runner/pull/3

spec/lib/final_destination_spec.rb

@@ -546,15 +546,31 @@ RSpec.describe FinalDestination do
       expect(fd(nil).validate_uri_format).to eq(false)
     end
 
-    it "returns false for invalid ports" do
-      expect(fd("http://eviltrout.com:21").validate_uri_format).to eq(false)
+    it "returns false for invalid https ports" do
       expect(fd("https://eviltrout.com:8000").validate_uri_format).to eq(false)
     end
 
-    it "returns true for valid ports" do
+    it "returns true for valid http and https ports" do
       expect(fd("http://eviltrout.com:80").validate_uri_format).to eq(true)
       expect(fd("https://eviltrout.com:443").validate_uri_format).to eq(true)
     end
+
+    it "returns false for invalid http port" do
+      expect(fd("http://eviltrout.com:21").validate_uri_format).to eq(false)
+    end
+
+    context "when s3_endpoint defined" do
+      before { SiteSetting.s3_endpoint = "http://minio.local:9000" }
+
+      it "returns false if the host is not in allowed_internal_hosts" do
+        expect(fd("http://discoursetest.minio.local:9000").validate_uri_format).to eq(false)
+      end
+
+      it "returns true if the host is in allowed_internal_hosts" do
+        SiteSetting.allowed_internal_hosts = %w[minio.local discoursetest.minio.local].join("|")
+        expect(fd("http://discoursetest.minio.local:9000").validate_uri_format).to eq(true)
+      end
+    end
   end
 
   describe "https cache" do

spec/multisite/s3_store_spec.rb

@@ -384,7 +384,7 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do
     end
   end
 
-  describe "#signed_url_for_temporary_upload" do
+  describe "#signed_request_for_temporary_upload" do
     before { setup_s3 }
 
     let(:store) { FileStore::S3Store.new }
@@ -392,19 +392,26 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do
     context "for a bucket with no folder path" do
       before { SiteSetting.s3_upload_bucket = "s3-upload-bucket" }
 
-      it "returns a presigned url with the correct params and the key for the temporary file" do
-        url = store.signed_url_for_temporary_upload("test.png")
+      it "returns a presigned url and headers with the correct params and the key for the temporary file" do
+        url, signed_headers = store.signed_request_for_temporary_upload("test.png")
         key = store.s3_helper.path_from_url(url)
+        expect(signed_headers).to eq("x-amz-acl" => "private")
         expect(url).to match(/Amz-Expires/)
         expect(key).to match(
           /temp\/uploads\/default\/test_[0-9]\/[a-zA-z0-9]{0,32}\/[a-zA-z0-9]{0,32}.png/,
         )
       end
 
-      it "presigned url contains the metadata when provided" do
-        url =
-          store.signed_url_for_temporary_upload("test.png", metadata: { "test-meta": "testing" })
-        expect(url).to include("&x-amz-meta-test-meta=testing")
+      it "presigned url headers contains the metadata when provided" do
+        url, signed_headers =
+          store.signed_request_for_temporary_upload(
+            "test.png",
+            metadata: {
+              "test-meta": "testing",
+            },
+          )
+        expect(signed_headers).to eq("x-amz-acl" => "private", "x-amz-meta-test-meta" => "testing")
+        expect(url).not_to include("&x-amz-meta-test-meta=testing")
       end
     end
 
@@ -412,7 +419,7 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do
       before { SiteSetting.s3_upload_bucket = "s3-upload-bucket/site" }
 
       it "returns a presigned url with the correct params and the key for the temporary file" do
-        url = store.signed_url_for_temporary_upload("test.png")
+        url, _signed_headers = store.signed_request_for_temporary_upload("test.png")
         key = store.s3_helper.path_from_url(url)
         expect(url).to match(/Amz-Expires/)
         expect(key).to match(
@@ -426,7 +433,7 @@ RSpec.describe "Multisite s3 uploads", type: :multisite do
 
       it "returns a presigned url with the correct params and the key for the temporary file" do
         test_multisite_connection("second") do
-          url = store.signed_url_for_temporary_upload("test.png")
+          url, _signed_headers = store.signed_request_for_temporary_upload("test.png")
           key = store.s3_helper.path_from_url(url)
           expect(url).to match(/Amz-Expires/)
           expect(key).to match(

spec/rails_helper.rb

@@ -23,6 +23,7 @@ require "fabrication"
 require "mocha/api"
 require "certified"
 require "webmock/rspec"
+require "minio_runner"
 
 class RspecErrorTracker
   def self.last_exception=(ex)
@@ -267,7 +268,34 @@ RSpec.configure do |config|
 
     SiteSetting.provider = TestLocalProcessProvider.new
 
-    WebMock.disable_net_connect!(allow_localhost: true)
+    # Used for S3 system specs, see also setup_s3_system_test.
+    MinioRunner.config do |minio_runner_config|
+      minio_runner_config.minio_domain = ENV["MINIO_RUNNER_MINIO_DOMAIN"] || "minio.local"
+      minio_runner_config.buckets =
+        (
+          if ENV["MINIO_RUNNER_BUCKETS"]
+            ENV["MINIO_RUNNER_BUCKETS"].split(",")
+          else
+            ["discoursetest"]
+          end
+        )
+      minio_runner_config.public_buckets =
+        (
+          if ENV["MINIO_RUNNER_PUBLIC_BUCKETS"]
+            ENV["MINIO_RUNNER_PUBLIC_BUCKETS"].split(",")
+          else
+            ["discoursetest"]
+          end
+        )
+    end
+
+    WebMock.disable_net_connect!(
+      allow_localhost: true,
+      allow: [
+        *MinioRunner.config.minio_urls,
+        URI(MinioRunner::MinioBinary.platform_binary_url).host,
+      ],
+    )
 
     if ENV["CAPYBARA_DEFAULT_MAX_WAIT_TIME"].present?
       Capybara.default_max_wait_time = ENV["CAPYBARA_DEFAULT_MAX_WAIT_TIME"].to_i
@@ -420,6 +448,7 @@ RSpec.configure do |config|
   config.after(:suite) do
     FileUtils.remove_dir(concurrency_safe_tmp_dir, true) if SpecSecureRandom.value
     Downloads.clear
+    MinioRunner.stop
   end
 
   config.around :each do |example|

spec/requests/api/schemas/json/upload_generate_presigned_put_response.json

@@ -11,6 +11,14 @@
       "description": "A presigned PUT URL which must be used to upload the file binary blob to.",
       "example": "https://file-uploads.s3.us-west-2.amazonaws.com/temp/site/uploads/default/123/456.jpg?x-amz-acl=private&x-amz-meta-sha1-checksum=sha1&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AAAAus-west-2%2Fs3%2Faws4_request&X-Amz-Date=20211221T011246Z&X-Amz-Expires=600&X-Amz-SignedHeaders=host&X-Amz-Signature=12345678"
     },
+    "signed_headers": {
+      "type": "object",
+      "description": "A map of headers that must be sent with the PUT request.",
+      "example": {
+        "x-amz-acl": "private",
+        "x-amz-meta-sha1-checksum": "sha1"
+      }
+    },
     "unique_identifier": {
       "type": "string",
       "description": "A unique string that identifies the external upload. This must be stored and then sent in the /complete-external-upload endpoint to complete the direct upload.",

spec/requests/uploads_controller_spec.rb

@@ -756,7 +756,7 @@ RSpec.describe UploadsController do
         expect(result["url"]).to include("Amz-Expires")
       end
 
-      it "includes accepted metadata in the presigned url when provided" do
+      it "includes accepted metadata in the response when provided" do
         post "/uploads/generate-presigned-put.json",
              **{
                params: {
@@ -772,8 +772,12 @@ RSpec.describe UploadsController do
         expect(response.status).to eq(200)
 
         result = response.parsed_body
-        expect(result["url"]).to include("&x-amz-meta-sha1-checksum=testing")
+        expect(result["url"]).not_to include("&x-amz-meta-sha1-checksum=testing")
         expect(result["url"]).not_to include("&x-amz-meta-blah=wontbeincluded")
+        expect(result["signed_headers"]).to eq(
+          "x-amz-acl" => "private",
+          "x-amz-meta-sha1-checksum" => "testing",
+        )
       end
 
       describe "rate limiting" do
@@ -1517,7 +1521,9 @@ RSpec.describe UploadsController do
                unique_identifier: external_upload_stub.unique_identifier,
              }
         expect(response.status).to eq(422)
-        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed"))
+        expect(response.parsed_body["errors"].first).to eq(
+          I18n.t("upload.checksum_mismatch_failure"),
+        )
       end
 
       it "handles SizeMismatchError" do
@@ -1530,7 +1536,9 @@ RSpec.describe UploadsController do
                unique_identifier: external_upload_stub.unique_identifier,
              }
         expect(response.status).to eq(422)
-        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed"))
+        expect(response.parsed_body["errors"].first).to eq(
+          I18n.t("upload.size_mismatch_failure", additional_detail: "expected: 10, actual: 1000"),
+        )
       end
 
       it "handles CannotPromoteError" do
@@ -1543,7 +1551,7 @@ RSpec.describe UploadsController do
                unique_identifier: external_upload_stub.unique_identifier,
              }
         expect(response.status).to eq(422)
-        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed"))
+        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.cannot_promote_failure"))
       end
 
       it "handles DownloadFailedError and Aws::S3::Errors::NotFound" do
@@ -1556,7 +1564,7 @@ RSpec.describe UploadsController do
                unique_identifier: external_upload_stub.unique_identifier,
              }
         expect(response.status).to eq(422)
-        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed"))
+        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.download_failure"))
         ExternalUploadManager
           .any_instance
           .stubs(:transform!)
@@ -1566,7 +1574,7 @@ RSpec.describe UploadsController do
                unique_identifier: external_upload_stub.unique_identifier,
              }
         expect(response.status).to eq(422)
-        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.failed"))
+        expect(response.parsed_body["errors"].first).to eq(I18n.t("upload.download_failure"))
       end
 
       it "handles a generic upload failure" do

spec/support/system_helpers.rb

@@ -35,6 +35,11 @@ module SystemHelpers
     SiteSetting.disable_avatar_education_message = true
     SiteSetting.enable_user_tips = false
     SiteSetting.splash_screen = false
+    SiteSetting.allowed_internal_hosts =
+      (
+        SiteSetting.allowed_internal_hosts.to_s.split("|") +
+          MinioRunner.config.minio_urls.map { |url| URI.parse(url).host }
+      ).join("|")
   end
 
   def try_until_success(timeout: Capybara.default_max_wait_time, frequency: 0.01)
@@ -138,4 +143,25 @@ module SystemHelpers
 
     page.execute_script(js, selector, start, offset)
   end
+
+  def setup_s3_system_test
+    SiteSetting.enable_s3_uploads = true
+
+    SiteSetting.s3_upload_bucket = "discoursetest"
+    SiteSetting.enable_upload_debug_mode = true
+
+    SiteSetting.s3_access_key_id = MinioRunner.config.minio_root_user
+    SiteSetting.s3_secret_access_key = MinioRunner.config.minio_root_password
+    SiteSetting.s3_endpoint = MinioRunner.config.minio_server_url
+
+    MinioRunner.start
+  end
+
+  def skip_unless_s3_system_specs_enabled!
+    if !ENV["CI"] && !ENV["RUN_S3_SYSTEM_SPECS"]
+      skip(
+        "S3 system specs are disabled in this environment, set CI=1 or RUN_S3_SYSTEM_SPECS=1 to enable them.",
+      )
+    end
+  end
 end

spec/system/s3_uploads_spec.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+describe "Uploading files to S3", type: :system do
+  fab!(:current_user) { Fabricate(:admin) }
+
+  let(:modal) { PageObjects::Modals::Base.new }
+  let(:composer) { PageObjects::Components::Composer.new }
+
+  describe "direct S3 uploads" do
+    before { SiteSetting.enable_direct_s3_uploads = true }
+
+    describe "single part uploads" do
+      it "uploads custom avatars to S3" do
+        skip_unless_s3_system_specs_enabled!
+
+        setup_s3_system_test
+        sign_in(current_user)
+
+        visit "/my/preferences/account"
+
+        find("#edit-avatar").click
+        find("#uploaded-avatar").click
+        attach_file(File.absolute_path(file_from_fixtures("logo.jpg"))) do
+          find("#avatar-uploader").click
+        end
+        expect(page).to have_css(".avatar-uploader label[data-uploaded]")
+        modal.click_primary_button
+        expect(page).to have_css(
+          "#user-avatar-uploads[data-custom-avatar-upload-id]",
+          visible: false,
+        )
+        puts page.driver.browser.logs.get(:browser).map(&:message)
+        expect(current_user.reload.uploaded_avatar_id).to eq(
+          find("#user-avatar-uploads", visible: false)["data-custom-avatar-upload-id"].to_i,
+        )
+      end
+    end
+
+    describe "multipart uploads" do
+      it "uploads a file in the post composer" do
+        skip_unless_s3_system_specs_enabled!
+
+        setup_s3_system_test
+        sign_in(current_user)
+
+        visit "/new-topic"
+
+        file_path = file_from_fixtures("logo.png", "images").path
+        attach_file(file_path) { composer.click_toolbar_button("upload") }
+
+        expect(page).to have_no_css("#file-uploading")
+        expect(composer.preview).to have_css(".image-wrapper")
+      end
+    end
+  end
+end