From 244836ddd4287ee6aeaf400a3497002578f92ee7 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Tue, 10 May 2022 11:14:26 +1000
Subject: [PATCH] FIX: Use hidden site setting for batch presign rate limit
 (#16692)

This was causing issues on some sites, having the const, because this really is heavily
dependent on upload speed. We request 5-10 URLs at a time with this endpoint; for
a 1.5GB upload with 5mb parts this could mean 60 requests to the server to get all
the part URLs. If the user's upload speed is super fast they may request all 60
batches in a minute, if it is slow they may request 5 batches in a minute.

The other external upload endpoints are not hit as often, so they can stay as constant
values for now. This commit also increases the default to 20 requests/minute.
---
 config/site_settings.yml                 |  3 +++
 lib/external_upload_helpers.rb           | 12 ++++++++++--
 spec/requests/uploads_controller_spec.rb | 25 ++++++++++++------------
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/config/site_settings.yml b/config/site_settings.yml
index bffff294ebc..45f16b69b21 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1886,6 +1886,9 @@ rate_limits:
   max_allowed_secondary_emails:
     default: 10
     hidden: true
+  max_batch_presign_multipart_per_minute:
+    default: 20
+    hidden: true
 
 developer:
   force_hostname:
diff --git a/lib/external_upload_helpers.rb b/lib/external_upload_helpers.rb
index ff187e5de38..5b0e43f5ab3 100644
--- a/lib/external_upload_helpers.rb
+++ b/lib/external_upload_helpers.rb
@@ -10,7 +10,6 @@ module ExternalUploadHelpers
   PRESIGNED_PUT_RATE_LIMIT_PER_MINUTE = 10
   CREATE_MULTIPART_RATE_LIMIT_PER_MINUTE = 10
   COMPLETE_MULTIPART_RATE_LIMIT_PER_MINUTE = 10
-  BATCH_PRESIGN_RATE_LIMIT_PER_MINUTE = 10
 
   included do
     before_action :external_store_check, only: [
@@ -112,8 +111,17 @@ module ExternalUploadHelpers
     part_numbers = params.require(:part_numbers)
     unique_identifier = params.require(:unique_identifier)
 
+    ##
+    # NOTE: This is configurable by hidden site setting because this really is heavily
+    # dependent on upload speed. We request 5-10 URLs at a time with this endpoint; for
+    # a 1.5GB upload with 5mb parts this could mean 60 requests to the server to get all
+    # the part URLs. If the user's upload speed is super fast they may request all 60
+    # batches in a minute, if it is slow they may request 5 batches in a minute.
+    #
+    # The other external upload endpoints are not hit as often, so they can stay as constant
+    # values for now.
     RateLimiter.new(
-      current_user, "batch-presign", ExternalUploadHelpers::BATCH_PRESIGN_RATE_LIMIT_PER_MINUTE, 1.minute
+      current_user, "batch-presign", SiteSetting.max_batch_presign_multipart_per_minute, 1.minute
     ).performed!
 
     part_numbers = part_numbers.map do |part_number|
diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb
index 46b0e198752..d54cd871449 100644
--- a/spec/requests/uploads_controller_spec.rb
+++ b/spec/requests/uploads_controller_spec.rb
@@ -1056,23 +1056,22 @@ describe UploadsController do
       it "rate limits" do
         RateLimiter.enable
         RateLimiter.clear_all!
+        SiteSetting.max_batch_presign_multipart_per_minute = 1
 
-        stub_const(ExternalUploadHelpers, "BATCH_PRESIGN_RATE_LIMIT_PER_MINUTE", 1) do
-          stub_list_multipart_request
-          post "/uploads/batch-presign-multipart-parts.json", params: {
-            unique_identifier: external_upload_stub.unique_identifier,
-            part_numbers: [1, 2, 3]
-          }
+        stub_list_multipart_request
+        post "/uploads/batch-presign-multipart-parts.json", params: {
+          unique_identifier: external_upload_stub.unique_identifier,
+          part_numbers: [1, 2, 3]
+        }
 
-          expect(response.status).to eq(200)
+        expect(response.status).to eq(200)
 
-          post "/uploads/batch-presign-multipart-parts.json", params: {
-            unique_identifier: external_upload_stub.unique_identifier,
-            part_numbers: [1, 2, 3]
-          }
+        post "/uploads/batch-presign-multipart-parts.json", params: {
+          unique_identifier: external_upload_stub.unique_identifier,
+          part_numbers: [1, 2, 3]
+        }
 
-          expect(response.status).to eq(429)
-        end
+        expect(response.status).to eq(429)
       end
     end