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!