From 4134173bbf5573da5b771de4f852c6df34c5e502 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 3 Jun 2021 10:52:43 +0100 Subject: [PATCH] FEATURE: Add global admin api key rate limiter (#12527) --- config/discourse_defaults.conf | 2 +- lib/auth/default_current_user_provider.rb | 20 +++++++++++++------ .../default_current_user_provider_spec.rb | 5 +++-- spec/integration/rate_limiting_spec.rb | 5 +++-- 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf index fbca7c499b0..e278d790e51 100644 --- a/config/discourse_defaults.conf +++ b/config/discourse_defaults.conf @@ -225,7 +225,7 @@ s3_install_cors_rule = max_user_api_reqs_per_minute = 20 max_user_api_reqs_per_day = 2880 -max_admin_api_reqs_per_key_per_minute = 60 +max_admin_api_reqs_per_minute = 60 max_reqs_per_ip_per_minute = 200 max_reqs_per_ip_per_10_seconds = 50 diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 2fe829c6cdf..8c16392b3aa 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -131,7 +131,7 @@ class Auth::DefaultCurrentUserProvider raise Discourse::InvalidAccess.new(I18n.t('invalid_api_credentials'), nil, custom_message: "invalid_api_credentials") unless current_user raise Discourse::InvalidAccess if current_user.suspended? || !current_user.active @env[API_KEY_ENV] = true - rate_limit_admin_api_requests(api_key) + rate_limit_admin_api_requests! end # user api key handling @@ -377,15 +377,23 @@ class Auth::DefaultCurrentUserProvider !!@env[HEADER_API_KEY] end - def rate_limit_admin_api_requests(api_key) + def rate_limit_admin_api_requests! return if Rails.env == "profile" - RateLimiter.new( + limit = GlobalSetting.max_admin_api_reqs_per_minute.to_i + if GlobalSetting.respond_to?(:max_admin_api_reqs_per_key_per_minute) + Discourse.deprecate("DISCOURSE_MAX_ADMIN_API_REQS_PER_KEY_PER_MINUTE is deprecated. Please use DISCOURSE_MAX_ADMIN_API_REQS_PER_MINUTE") + limit = [ GlobalSetting.max_admin_api_reqs_per_key_per_minute.to_i, limit].max + end + + global_limit = RateLimiter.new( nil, - "admin_api_min_#{ApiKey.hash_key(api_key)}", - GlobalSetting.max_admin_api_reqs_per_key_per_minute, + "admin_api_min", + limit, 60 - ).performed! + ) + + global_limit.performed! end def can_write? diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 1ff3e6e6f9c..b8da06fc582 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -195,10 +195,11 @@ describe Auth::DefaultCurrentUserProvider do RateLimiter.enable end - it "rate limits api requests per api key" do - global_setting :max_admin_api_reqs_per_key_per_minute, 3 + it "rate limits admin api requests" do + global_setting :max_admin_api_reqs_per_minute, 3 freeze_time + RateLimiter.new(nil, "admin_api_min", 3, 60).clear! api_key = ApiKey.create!(created_by_id: -1) params = { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => user.username.downcase } diff --git a/spec/integration/rate_limiting_spec.rb b/spec/integration/rate_limiting_spec.rb index e7bb8f3f352..294bf62b59e 100644 --- a/spec/integration/rate_limiting_spec.rb +++ b/spec/integration/rate_limiting_spec.rb @@ -49,12 +49,13 @@ describe 'rate limiter integration' do it 'can cleanly limit requests and sets a Retry-After header' do freeze_time - #request.set_header("action_dispatch.show_exceptions", true) + + RateLimiter.clear_all! admin = Fabricate(:admin) api_key = Fabricate(:api_key, user: admin) - global_setting :max_admin_api_reqs_per_key_per_minute, 1 + global_setting :max_admin_api_reqs_per_minute, 1 get '/admin/api/keys.json', headers: { HTTP_API_KEY: api_key.key,