DEV: Don't enforce per ip rate limits for admin api requests (#27500)

lib/auth/default_current_user_provider.rb

@@ -165,7 +165,15 @@ class Auth::DefaultCurrentUserProvider
               )
       end
       raise Discourse::InvalidAccess if current_user.suspended? || !current_user.active
-      admin_api_key_limiter.performed! if !Rails.env.profile?
+
+      if !Rails.env.profile?
+        admin_api_key_limiter.performed!
+
+        # Don't enforce the default per ip limits for authenticated admin api
+        # requests
+        (@env["DISCOURSE_RATE_LIMITERS"] || []).each(&:rollback!)
+      end
+
       @env[API_KEY_ENV] = true
     end
 

spec/requests/api/rate_limits_spec.rb

@@ -0,0 +1,155 @@
+# frozen_string_literal: true
+
+class MockRateLimiter
+  LimitExceeded = RateLimiter::LimitExceeded
+
+  def self.disable
+  end
+
+  def self.enable
+  end
+
+  def self.performed!(*args, **kwargs)
+  end
+
+  def self.rollback!(*args, **kwargs)
+  end
+
+  def initialize(*args, **kwargs)
+    @args = args
+    @kwargs = kwargs
+  end
+
+  def can_perform?
+    true
+  end
+
+  def performed!
+    MockRateLimiter.performed!(*@args, **@kwargs)
+  end
+
+  def rollback!
+    MockRateLimiter.rollback!(*@args, **@kwargs)
+  end
+end
+
+RSpec.describe "rate limits" do
+  before_all { @key = Fabricate(:api_key).key }
+
+  let(:api_key) { @key }
+  let!(:api_username) { "system" }
+
+  use_redis_snapshotting
+
+  around(:each) { |example| stub_const(Object, :RateLimiter, MockRateLimiter) { example.run } }
+
+  it "doesn't rate limit authenticated admin api requests" do
+    MockRateLimiter
+      .expects(:performed!)
+      .with(
+        nil,
+        "global_limit_60_192.0.2.1",
+        200,
+        60,
+        global: true,
+        error_code: "ip_60_secs_limit",
+        aggressive: true,
+      )
+      .once
+    MockRateLimiter
+      .expects(:performed!)
+      .with(
+        nil,
+        "global_limit_10_192.0.2.1",
+        50,
+        10,
+        global: true,
+        error_code: "ip_10_secs_limit",
+        aggressive: true,
+      )
+      .once
+
+    MockRateLimiter
+      .expects(:performed!)
+      .with(nil, "admin_api_min", 60, 60, error_code: "admin_api_key_rate_limit")
+      .once
+
+    MockRateLimiter
+      .expects(:rollback!)
+      .with(
+        nil,
+        "global_limit_60_192.0.2.1",
+        200,
+        60,
+        global: true,
+        error_code: "ip_60_secs_limit",
+        aggressive: true,
+      )
+      .once
+    MockRateLimiter
+      .expects(:rollback!)
+      .with(
+        nil,
+        "global_limit_10_192.0.2.1",
+        50,
+        10,
+        global: true,
+        error_code: "ip_10_secs_limit",
+        aggressive: true,
+      )
+      .once
+
+    get(
+      "/admin/backups.json",
+      headers: {
+        "Api-Key" => api_key,
+        "Api-Username" => api_username,
+      },
+      env: {
+        REMOTE_ADDR: "192.0.2.1",
+      },
+    )
+
+    expect(response.status).to eq(200)
+  end
+
+  it "doesn't rollback rate limits for unauthenticated admin api requests" do
+    MockRateLimiter
+      .expects(:performed!)
+      .with(
+        nil,
+        "global_limit_60_192.0.2.1",
+        200,
+        60,
+        global: true,
+        error_code: "ip_60_secs_limit",
+        aggressive: true,
+      )
+      .once
+    MockRateLimiter
+      .expects(:performed!)
+      .with(
+        nil,
+        "global_limit_10_192.0.2.1",
+        50,
+        10,
+        global: true,
+        error_code: "ip_10_secs_limit",
+        aggressive: true,
+      )
+      .once
+
+    get(
+      "/admin/backups.json",
+      headers: {
+        "Api-Key" => "bogus key",
+        "Api-Username" => api_username,
+      },
+      env: {
+        REMOTE_ADDR: "192.0.2.1",
+      },
+    )
+
+    expect(response.status).to eq(404)
+  end
+end