FIX: Don't rate limit admin and staff constraints when matching routes.
* When an error is raised when checking route constraints, we can only return true/false which either lets the request through or return a 404 error. Therefore, we just skip rate limiting here and let the controller handle the rate limiting.
This commit is contained in:
parent
edbcc992d4
commit
651b50b1a1
|
@ -8,7 +8,8 @@ class AdminConstraint
|
|||
|
||||
def matches?(request)
|
||||
return false if @require_master && RailsMultisite::ConnectionManagement.current_db != "default"
|
||||
provider = Discourse.current_user_provider.new(request.env)
|
||||
provider = Discourse.current_user_provider.new(request.env, rate_limit: false)
|
||||
|
||||
provider.current_user &&
|
||||
provider.current_user.admin? &&
|
||||
custom_admin_check(request)
|
||||
|
|
|
@ -17,9 +17,10 @@ class Auth::DefaultCurrentUserProvider
|
|||
BAD_TOKEN ||= "_DISCOURSE_BAD_TOKEN"
|
||||
|
||||
# do all current user initialization here
|
||||
def initialize(env)
|
||||
def initialize(env, rate_limit: true)
|
||||
@env = env
|
||||
@request = Rack::Request.new(env)
|
||||
@rate_limit = rate_limit
|
||||
end
|
||||
|
||||
# our current user, return nil if none is found
|
||||
|
@ -62,7 +63,7 @@ class Auth::DefaultCurrentUserProvider
|
|||
if !current_user
|
||||
@env[BAD_TOKEN] = true
|
||||
begin
|
||||
limiter.performed!
|
||||
limiter.performed! if @rate_limit
|
||||
rescue RateLimiter::LimitExceeded
|
||||
raise Discourse::InvalidAccess.new(
|
||||
'Invalid Access',
|
||||
|
@ -85,7 +86,7 @@ class Auth::DefaultCurrentUserProvider
|
|||
# we do not run this rate limiter while profiling
|
||||
if Rails.env != "profile"
|
||||
limiter_min = RateLimiter.new(nil, "admin_api_min_#{api_key}", GlobalSetting.max_admin_api_reqs_per_key_per_minute, 60)
|
||||
limiter_min.performed!
|
||||
limiter_min.performed! if @rate_limit
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -96,19 +97,19 @@ class Auth::DefaultCurrentUserProvider
|
|||
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!
|
||||
limiter_day.performed! if @rate_limit
|
||||
end
|
||||
|
||||
unless limiter_min.can_perform?
|
||||
limiter_min.performed!
|
||||
limiter_min.performed! if @rate_limit
|
||||
end
|
||||
|
||||
current_user = lookup_user_api_user_and_update_key(user_api_key, @env[USER_API_CLIENT_ID])
|
||||
raise Discourse::InvalidAccess unless current_user
|
||||
raise Discourse::InvalidAccess if current_user.suspended? || !current_user.active
|
||||
|
||||
limiter_min.performed!
|
||||
limiter_day.performed!
|
||||
limiter_min.performed! if @rate_limit
|
||||
limiter_day.performed! if @rate_limit
|
||||
|
||||
@env[USER_API_KEY_ENV] = true
|
||||
end
|
||||
|
|
|
@ -3,7 +3,8 @@ require_dependency 'current_user'
|
|||
class StaffConstraint
|
||||
|
||||
def matches?(request)
|
||||
provider = Discourse.current_user_provider.new(request.env)
|
||||
provider = Discourse.current_user_provider.new(request.env, rate_limit: false)
|
||||
|
||||
provider.current_user &&
|
||||
provider.current_user.staff? &&
|
||||
custom_staff_check(request)
|
||||
|
|
|
@ -2,18 +2,19 @@ require 'rails_helper'
|
|||
require_dependency 'auth/default_current_user_provider'
|
||||
|
||||
describe Auth::DefaultCurrentUserProvider do
|
||||
let(:rate_limit) { true }
|
||||
|
||||
class TestProvider < Auth::DefaultCurrentUserProvider
|
||||
attr_reader :env
|
||||
def initialize(env)
|
||||
super(env)
|
||||
def initialize(env, rate_limit: true)
|
||||
super(env, rate_limit: rate_limit)
|
||||
end
|
||||
end
|
||||
|
||||
def provider(url, opts = nil)
|
||||
opts ||= { method: "GET" }
|
||||
env = Rack::MockRequest.env_for(url, opts)
|
||||
TestProvider.new(env)
|
||||
TestProvider.new(env, rate_limit: rate_limit)
|
||||
end
|
||||
|
||||
it "can be used to pretend that a user doesn't exist" do
|
||||
|
@ -145,6 +146,26 @@ describe Auth::DefaultCurrentUserProvider do
|
|||
provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user
|
||||
|
||||
end
|
||||
|
||||
describe 'when rate limit is disabled' do
|
||||
let(:rate_limit) { false }
|
||||
|
||||
it 'should not raise any rate limit errors' do
|
||||
global_setting :max_admin_api_reqs_per_key_per_minute, 1
|
||||
|
||||
freeze_time
|
||||
|
||||
key = SecureRandom.hex
|
||||
api_key = ApiKey.create!(key: key, created_by_id: -1)
|
||||
|
||||
2.times do
|
||||
provider(
|
||||
"/?api_key=#{key}&api_username=system",
|
||||
nil
|
||||
).current_user
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
end
|
||||
|
|
Loading…
Reference in New Issue