From 68d3c2c74f6ebd5524d0baf829e99066a779f816 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 11 Dec 2017 11:07:22 +1100 Subject: [PATCH] FEATURE: add global rate limiter for admin api 60 per minute Also move configuration of admin and user api rate limiting into global settings. This is not intended to be configurable per site --- config/discourse_defaults.conf | 7 + config/locales/server.en.yml | 2 - config/site_settings.yml | 4 - lib/auth/default_current_user_provider.rb | 9 +- .../default_current_user_provider_spec.rb | 139 +++++++++++------- 5 files changed, 102 insertions(+), 59 deletions(-) diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf index 2f1ba286d47..7809dbedf59 100644 --- a/config/discourse_defaults.conf +++ b/config/discourse_defaults.conf @@ -168,3 +168,10 @@ s3_access_key_id = s3_secret_access_key = s3_use_iam_profile = false s3_cdn_url = + + +### rate limits apply to all sites +max_user_api_reqs_per_minute = 20 +max_user_api_reqs_per_day = 2880 + +max_admin_api_reqs_per_key_per_minute = 60 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index bc442bfaf8d..098461856ba 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1562,8 +1562,6 @@ en: retain_web_hook_events_period_days: "Number of days to retain web hook event records." - max_user_api_reqs_per_day: "Maximum number of user API requests per key per day" - max_user_api_reqs_per_minute: "Maximum number of user API requests per key per minute" allow_user_api_keys: "Allow generation of user API keys" allow_user_api_key_scopes: "List of scopes allowed for user API keys" max_api_keys_per_user: "Maximum number of user API keys per user" diff --git a/config/site_settings.yml b/config/site_settings.yml index 59c4f8dd161..d56639939f3 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1448,10 +1448,6 @@ api: default: 30 user_api: - max_user_api_reqs_per_day: - default: 2880 - max_user_api_reqs_per_minute: - default: 20 allow_user_api_keys: default: true allow_user_api_key_scopes: diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index f454c67e8a4..ecbd7bce1b9 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_dependency "auth/current_user_provider" require_dependency "rate_limiter" @@ -79,13 +81,16 @@ 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 + + limiter_min = RateLimiter.new(nil, "admin_api_min_#{api_key}", GlobalSetting.max_admin_api_reqs_per_key_per_minute, 60) + limiter_min.performed! end # user api key handling if user_api_key - limiter_min = RateLimiter.new(nil, "user_api_min_#{user_api_key}", SiteSetting.max_user_api_reqs_per_minute, 60) - limiter_day = RateLimiter.new(nil, "user_api_day_#{user_api_key}", SiteSetting.max_user_api_reqs_per_day, 86400) + limiter_min = RateLimiter.new(nil, "user_api_min_#{user_api_key}", GlobalSetting.max_user_api_reqs_per_minute, 60) + limiter_day = RateLimiter.new(nil, "user_api_day_#{user_api_key}", GlobalSetting.max_user_api_reqs_per_day, 86400) unless limiter_day.can_perform? limiter_day.performed! diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 93fe83cccd9..d8c1ce2fb01 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -16,69 +16,106 @@ describe Auth::DefaultCurrentUserProvider do TestProvider.new(env) end - it "raises errors for incorrect api_key" do - expect { - provider("/?api_key=INCORRECT").current_user - }.to raise_error(Discourse::InvalidAccess, /API username or key is invalid/) - end + context "server api" do - it "finds a user for a correct per-user api key" do - user = Fabricate(:user) - ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) - expect(provider("/?api_key=hello").current_user.id).to eq(user.id) + it "raises errors for incorrect api_key" do + expect { + provider("/?api_key=INCORRECT").current_user + }.to raise_error(Discourse::InvalidAccess, /API username or key is invalid/) + end - user.update_columns(active: false) + it "finds a user for a correct per-user api key" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) + expect(provider("/?api_key=hello").current_user.id).to eq(user.id) - expect { - provider("/?api_key=hello").current_user - }.to raise_error(Discourse::InvalidAccess) + user.update_columns(active: false) - user.update_columns(active: true, suspended_till: 1.day.from_now) + expect { + provider("/?api_key=hello").current_user + }.to raise_error(Discourse::InvalidAccess) - expect { - provider("/?api_key=hello").current_user - }.to raise_error(Discourse::InvalidAccess) - end + user.update_columns(active: true, suspended_till: 1.day.from_now) - it "raises for a user pretending" do - user = Fabricate(:user) - user2 = Fabricate(:user) - ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) + expect { + provider("/?api_key=hello").current_user + }.to raise_error(Discourse::InvalidAccess) + end - expect { - provider("/?api_key=hello&api_username=#{user2.username.downcase}").current_user - }.to raise_error(Discourse::InvalidAccess) - end + it "raises for a user pretending" do + user = Fabricate(:user) + user2 = Fabricate(:user) + ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) - it "raises for a user with a mismatching ip" do - user = Fabricate(:user) - ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['10.0.0.0/24']) + expect { + provider("/?api_key=hello&api_username=#{user2.username.downcase}").current_user + }.to raise_error(Discourse::InvalidAccess) + end - expect { - provider("/?api_key=hello&api_username=#{user.username.downcase}", "REMOTE_ADDR" => "10.1.0.1").current_user - }.to raise_error(Discourse::InvalidAccess) + it "raises for a user with a mismatching ip" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['10.0.0.0/24']) - end + expect { + provider("/?api_key=hello&api_username=#{user.username.downcase}", "REMOTE_ADDR" => "10.1.0.1").current_user + }.to raise_error(Discourse::InvalidAccess) - it "allows a user with a matching ip" do - user = Fabricate(:user) - ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24']) + end - found_user = provider("/?api_key=hello&api_username=#{user.username.downcase}", - "REMOTE_ADDR" => "100.0.0.22").current_user + it "allows a user with a matching ip" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24']) - expect(found_user.id).to eq(user.id) + found_user = provider("/?api_key=hello&api_username=#{user.username.downcase}", + "REMOTE_ADDR" => "100.0.0.22").current_user - found_user = provider("/?api_key=hello&api_username=#{user.username.downcase}", - "HTTP_X_FORWARDED_FOR" => "10.1.1.1, 100.0.0.22").current_user - expect(found_user.id).to eq(user.id) + expect(found_user.id).to eq(user.id) - end + found_user = provider("/?api_key=hello&api_username=#{user.username.downcase}", + "HTTP_X_FORWARDED_FOR" => "10.1.1.1, 100.0.0.22").current_user + expect(found_user.id).to eq(user.id) + + end + + it "finds a user for a correct system api key" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + expect(provider("/?api_key=hello&api_username=#{user.username.downcase}").current_user.id).to eq(user.id) + end + + context "rate limiting" do + before do + RateLimiter.enable + end + + after do + RateLimiter.disable + end + + it "rate limits api requests per api key" do + global_setting :max_admin_api_reqs_per_key_per_minute, 3 + + user = Fabricate(:user) + key = SecureRandom.hex + api_key = ApiKey.create!(key: key, created_by_id: -1) + + provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user + provider("/?api_key=#{key}&api_username=system").current_user + provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user + + expect do + provider("/?api_key=#{key}&api_username=system").current_user + end.to raise_error(RateLimiter::LimitExceeded) + + # should not rake limit a random key + api_key.destroy + key = SecureRandom.hex + ApiKey.create!(key: key, created_by_id: -1) + provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user + + end + end - it "finds a user for a correct system api key" do - user = Fabricate(:user) - ApiKey.create!(key: "hello", created_by_id: -1) - expect(provider("/?api_key=hello&api_username=#{user.username.downcase}").current_user.id).to eq(user.id) end it "should not update last seen for ajax calls without Discourse-Visible header" do @@ -320,8 +357,8 @@ describe Auth::DefaultCurrentUserProvider do limiter1.clear! limiter2.clear! - SiteSetting.max_user_api_reqs_per_day = 3 - SiteSetting.max_user_api_reqs_per_minute = 4 + global_setting :max_user_api_reqs_per_day, 3 + global_setting :max_user_api_reqs_per_minute, 4 params = { "REQUEST_METHOD" => "GET", @@ -336,8 +373,8 @@ describe Auth::DefaultCurrentUserProvider do provider("/", params).current_user }.to raise_error(RateLimiter::LimitExceeded) - SiteSetting.max_user_api_reqs_per_day = 4 - SiteSetting.max_user_api_reqs_per_minute = 3 + global_setting :max_user_api_reqs_per_day, 4 + global_setting :max_user_api_reqs_per_minute, 3 limiter1.clear! limiter2.clear!