DEV: Add S3 upload system specs using minio (discourse#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 discourse#22381

Related PRs:

* discourse/minio_runner#1
* discourse/minio_runner#2
* discourse/minio_runner#3

.github/workflows/tests.yml

@@ -30,6 +30,7 @@ jobs:
       PGPASSWORD: discourse
       USES_PARALLEL_DATABASES: ${{ matrix.build_type == 'backend' || matrix.build_type == 'system' }}
       CAPYBARA_DEFAULT_MAX_WAIT_TIME: 10
+      MINIO_RUNNER_LOG_LEVEL: DEBUG
 
     strategy:
       fail-fast: false
@@ -106,6 +107,11 @@ jobs:
         if: matrix.target == 'plugins'
         run: bin/rake plugin:pull_compatible_all
 
+      - name: Add hosts to /etc/hosts, otherwise Chrome cannot reach minio
+        run: |
+          echo "127.0.0.1 minio.local" | sudo tee -a /etc/hosts
+          echo "127.0.0.1 discoursetest.minio.local" | sudo tee -a /etc/hosts
+
       - name: Fetch app state cache
         uses: actions/cache@v3
         id: app-cache

Gemfile

@@ -144,6 +144,7 @@ group :test do
   gem "selenium-webdriver", "~> 4.11", require: false
   gem "test-prof"
   gem "rails-dom-testing", require: false
+  gem "minio_runner", require: false
 end
 
 group :test, :development do

Gemfile.lock

@@ -243,6 +243,7 @@ GEM
     mini_sql (1.5.0)
     mini_suffix (0.3.3)
       ffi (~> 1.9)
+    minio_runner (0.1.1)
     minitest (5.19.0)
     mocha (2.1.0)
       ruby2_keywords (>= 0.0.5)
@@ -605,6 +606,7 @@ DEPENDENCIES
   mini_scheduler
   mini_sql
   mini_suffix
+  minio_runner
   minitest
   mocha
   multi_json

app/assets/javascripts/discourse/app/components/avatar-uploader.hbs

@@ -2,6 +2,8 @@
   class="btn btn-default btn-icon-text"
   disabled={{this.uploading}}
   title={{i18n "user.change_avatar.upload_title"}}
+  data-uploaded={{this.customAvatarUploaded}}
+  data-avatar-upload-id={{this.uploadedAvatarId}}
 >
   {{d-icon "far-image"}}
   {{#if this.uploading}}

app/assets/javascripts/discourse/app/components/avatar-uploader.js

@@ -1,4 +1,5 @@
 import Component from "@ember/component";
+import { isBlank } from "@ember/utils";
 import UppyUploadMixin from "discourse/mixins/uppy-upload";
 import discourseComputed from "discourse-common/utils/decorators";
 
@@ -7,6 +8,11 @@ export default Component.extend(UppyUploadMixin, {
   tagName: "span",
   imageIsNotASquare: false,
 
+  @discourseComputed("uploading", "uploadedAvatarId")
+  customAvatarUploaded() {
+    return !this.uploading && !isBlank(this.uploadedAvatarId);
+  },
+
   validateUploadedFilesOptions() {
     return { imagesOnly: true };
   },

app/assets/javascripts/discourse/app/mixins/uppy-upload.js

@@ -397,6 +397,7 @@ export default Mixin.create(UppyS3Multipart, ExtendableUploader, {
               method: "put",
               url: response.url,
               headers: {
+                ...response.signed_headers,
                 "Content-Type": file.type,
               },
             };

app/assets/javascripts/discourse/app/templates/preferences/account.hbs

@@ -8,6 +8,12 @@
     <label class="control-label" id="profile-picture">{{i18n
         "user.avatar.title"
       }}</label>
+    <input
+      type="hidden"
+      id="user-avatar-uploads"
+      data-custom-avatar-upload-id={{this.model.custom_avatar_upload_id}}
+      data-system-avatar-upload-id={{this.model.system_avatar_upload_id}}
+    />
     <div class="controls">
       {{! we want the "huge" version even though we're downsizing it in CSS }}
       {{bound-avatar this.model "huge"}}
@@ -16,6 +22,7 @@
         @actionParam={{this.model}}
         @class="btn-default pad-left"
         @icon="pencil-alt"
+        id="edit-avatar"
       />
     </div>
   </div>

app/services/external_upload_manager.rb

@@ -28,7 +28,7 @@ def self.user_banned?(user)
 
   def self.create_direct_upload(current_user:, file_name:, file_size:, upload_type:, metadata: {})
     store = store_for_upload_type(upload_type)
-    url = store.signed_url_for_temporary_upload(file_name, metadata: metadata)
+    url, signed_headers = store.signed_request_for_temporary_upload(file_name, metadata: metadata)
     key = store.s3_helper.path_from_url(url)
 
     upload_stub =
@@ -40,7 +40,12 @@ def self.create_direct_upload(current_user:, file_name:, file_size:, upload_type
         filesize: file_size,
       )
 
-    { url: url, key: key, unique_identifier: upload_stub.unique_identifier }
+    {
+      url: url,
+      key: key,
+      unique_identifier: upload_stub.unique_identifier,
+      signed_headers: signed_headers,
+    }
   end
 
   def self.create_direct_multipart_upload(
@@ -191,11 +196,18 @@ def external_sha1
 
   def download(key, type)
     url = @store.signed_url_for_path(external_upload_stub.key)
+    uri = URI(url)
     FileHelper.download(
       url,
       max_file_size: DOWNLOAD_LIMIT,
       tmp_file_name: "discourse-upload-#{type}",
       follow_redirect: true,
+      # Local S3 servers (like minio) do not use port 80, and the Aws::Sigv4::Signer
+      # includes the port number in the Host header when presigning URLs if the
+      # port is not 80, so we have to make sure the Host header sent by
+      # FinalDestination includes the port, otherwise we will get a
+      # `SignatureDoesNotMatch` error.
+      include_port_in_host_header: uri.scheme == "http" && uri.port != 80,
     )
   end
 end

lib/external_upload_helpers.rb

@@ -355,7 +355,7 @@ def validate_part_number(part_number)
   def debug_upload_error(err, friendly_message)
     return if !SiteSetting.enable_upload_debug_mode
     Discourse.warn_exception(err, message: friendly_message)
-    Rails.env.development? ? friendly_message : I18n.t("upload.failed")
+    (Rails.env.development? || Rails.env.test?) ? friendly_message : I18n.t("upload.failed")
   end
 
   def multipart_store(upload_type)

lib/file_helper.rb

@@ -49,7 +49,8 @@ def self.download(
     skip_rate_limit: false,
     verbose: false,
     validate_uri: true,
-    retain_on_max_file_size_exceeded: false
+    retain_on_max_file_size_exceeded: false,
+    include_port_in_host_header: false
   )
     url = "https:" + url if url.start_with?("//")
     raise Discourse::InvalidParameters.new(:url) unless url =~ %r{\Ahttps?://}
@@ -64,6 +65,7 @@ def self.download(
         verbose: verbose,
         validate_uri: validate_uri,
         timeout: read_timeout,
+        include_port_in_host_header: include_port_in_host_header,
       )
 
     fd.get do |response, chunk, uri|

lib/file_store/s3_store.rb

@@ -252,13 +252,13 @@ def signed_url_for_path(
       presigned_get_url(key, expires_in: expires_in, force_download: force_download)
     end
 
-    def signed_url_for_temporary_upload(
+    def signed_request_for_temporary_upload(
       file_name,
       expires_in: S3Helper::UPLOAD_URL_EXPIRES_AFTER_SECONDS,
       metadata: {}
     )
       key = temporary_upload_path(file_name)
-      s3_helper.presigned_url(
+      s3_helper.presigned_request(
         key,
         method: :put_object,
         expires_in: expires_in,

lib/final_destination.rb

@@ -49,6 +49,7 @@ def initialize(url, opts = nil)
     @default_user_agent = @opts[:default_user_agent] || DEFAULT_USER_AGENT
     @opts[:max_redirects] ||= 5
     @https_redirect_ignore_limit = @opts[:initial_https_redirect_ignore_limit]
+    @include_port_in_host_header = @opts[:include_port_in_host_header] || false
 
     @max_redirects = @opts[:max_redirects]
     @limit = @max_redirects
@@ -114,7 +115,7 @@ def request_headers
       "User-Agent" => @user_agent,
       "Accept" => "*/*",
       "Accept-Language" => "*",
-      "Host" => @uri.hostname,
+      "Host" => @uri.hostname + (@include_port_in_host_header ? ":#{@uri.port}" : ""),
     }
 
     result["Cookie"] = @cookie if @cookie
@@ -397,7 +398,15 @@ def validate_uri
   def validate_uri_format
     return false unless @uri && @uri.host
     return false unless %w[https http].include?(@uri.scheme)
-    return false if @uri.scheme == "http" && @uri.port != 80
+
+    # In some cases (like local/test environments) we may want to allow http URLs
+    # to be used for internal hosts, but only if it's the case that the host is
+    # explicitly used for SiteSetting.s3_endpoint. This is to allow for local
+    # S3 providers like minio.
+    #
+    # In all other cases, we should not be allowing http calls to anything except
+    # port 80.
+    return false if @uri.scheme == "http" && !http_port_ok?
     return false if @uri.scheme == "https" && @uri.port != 443
 
     # Disallow IP based crawling
@@ -410,6 +419,23 @@ def validate_uri_format
     ).nil?
   end
 
+  def http_port_ok?
+    return true if @uri.port == 80
+
+    allowed_internal_hosts =
+      SiteSetting.allowed_internal_hosts&.split(/[|\n]/).filter_map { |aih| aih.strip.presence }
+    return false if allowed_internal_hosts.empty? || SiteSetting.s3_endpoint.blank?
+    return false if allowed_internal_hosts.none? { |aih| hostname_matches_s3_endpoint?(aih) }
+
+    true
+  end
+
+  def hostname_matches_s3_endpoint?(allowed_internal_host)
+    s3_endpoint_uri = URI(SiteSetting.s3_endpoint)
+    hostname_matches?("http://#{allowed_internal_host}") && @uri.port == s3_endpoint_uri.port &&
+      @uri.hostname.end_with?(s3_endpoint_uri.hostname)
+  end
+
   def hostname
     @uri.hostname
   end
@@ -451,7 +477,11 @@ def safe_get(uri)
     headers_subset = Struct.new(:location, :set_cookie).new
 
     safe_session(uri) do |http|
-      headers = request_headers.merge("Accept-Encoding" => "gzip", "Host" => uri.host)
+      headers =
+        request_headers.merge(
+          "Accept-Encoding" => "gzip",
+          "Host" => uri.hostname + (@include_port_in_host_header ? ":#{uri.port}" : ""),
+        )
 
       req = FinalDestination::HTTP::Get.new(uri.request_uri, headers)
 

lib/s3_helper.rb

@@ -353,6 +353,19 @@ def presigned_url(key, method:, expires_in: S3Helper::UPLOAD_URL_EXPIRES_AFTER_S
     )
   end
 
+  # Returns url, headers in a tuple which is needed in some cases.
+  def presigned_request(
+    key,
+    method:,
+    expires_in: S3Helper::UPLOAD_URL_EXPIRES_AFTER_SECONDS,
+    opts: {}
+  )
+    Aws::S3::Presigner.new(client: s3_client).presigned_request(
+      method,
+      { bucket: s3_bucket_name, key: key, expires_in: expires_in }.merge(opts),
+    )
+  end
+
   private
 
   def fetch_bucket_cors_rules

script/install_minio_binaries.rb

@@ -0,0 +1,7 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require "minio_runner"
+
+ENV["MINIO_RUNNER_LOG_LEVEL"] = "DEBUG"
+MinioRunner.install_binaries

script/local_minio_s3.rb

@@ -0,0 +1,92 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require "minio_runner"
+
+class ExecuteLocalMinioS3
+  def run
+    begin
+      start_minio
+      save_old_config
+      change_to_minio_settings
+      puts "Press any key when done..."
+      gets
+      restore_old
+    rescue SystemExit, Interrupt
+      restore_old
+      raise
+    end
+  end
+
+  def start_minio
+    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
+    puts "Starting minio..."
+    MinioRunner.start
+  end
+
+  def save_old_config
+    puts "Temporarily using minio config for S3. Current settings:"
+    @current_s3_endpoint = puts_current(:s3_endpoint)
+    @current_s3_upload_bucket = puts_current(:s3_upload_bucket)
+    @current_s3_access_key_id = puts_current(:s3_access_key_id)
+    @current_s3_secret_access_key = puts_current(:s3_secret_access_key)
+    @current_allowed_internal_hosts = puts_current(:allowed_internal_hosts)
+  end
+
+  def change_to_minio_settings
+    puts "Changing to minio settings..."
+    SiteSetting.s3_upload_bucket = "discoursetest"
+    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
+    SiteSetting.allowed_internal_hosts =
+      MinioRunner.config.minio_urls.map { |url| URI.parse(url).host }.join("|")
+
+    puts_current(:s3_endpoint)
+    puts_current(:s3_upload_bucket)
+    puts_current(:s3_access_key_id)
+    puts_current(:s3_secret_access_key)
+    puts_current(:allowed_internal_hosts)
+  end
+
+  def restore_old
+    puts "Restoring old S3 settings..."
+    SiteSetting.s3_upload_bucket = @current_s3_upload_bucket
+    SiteSetting.s3_access_key_id = @current_s3_access_key_id
+    SiteSetting.s3_secret_access_key = @current_s3_secret_access_key
+    SiteSetting.s3_endpoint = @current_s3_endpoint
+    SiteSetting.allowed_internal_hosts = @current_allowed_internal_hosts
+
+    puts_current(:s3_endpoint)
+    puts_current(:s3_upload_bucket)
+    puts_current(:s3_access_key_id)
+    puts_current(:s3_secret_access_key)
+    puts_current(:allowed_internal_hosts)
+    puts "Done!"
+  end
+
+  def puts_current(setting)
+    printf "%-40s %s\n", "  > Current #{setting}:", SiteSetting.send(setting)
+    SiteSetting.send(setting)
+  end
+end
+
+ExecuteLocalMinioS3.new.run