Revert "FIX: Don't rate limit admin and staff constraints when matching routes."

This reverts commit 651b50b1a159258588ebd716f678035db2239b5a.
This commit is contained in:
Guo Xiang Tan
2018-09-04 14:17:05 +08:00
parent e4498d2a8a
commit 3b337bfc6b
4 changed files with 12 additions and 36 deletions

View File

@ -8,8 +8,7 @@ class AdminConstraint
def matches?(request) def matches?(request)
return false if @require_master && RailsMultisite::ConnectionManagement.current_db != "default" return false if @require_master && RailsMultisite::ConnectionManagement.current_db != "default"
provider = Discourse.current_user_provider.new(request.env, rate_limit: false) provider = Discourse.current_user_provider.new(request.env)
provider.current_user && provider.current_user &&
provider.current_user.admin? && provider.current_user.admin? &&
custom_admin_check(request) custom_admin_check(request)

View File

@ -17,10 +17,9 @@ class Auth::DefaultCurrentUserProvider
BAD_TOKEN ||= "_DISCOURSE_BAD_TOKEN" BAD_TOKEN ||= "_DISCOURSE_BAD_TOKEN"
# do all current user initialization here # do all current user initialization here
def initialize(env, rate_limit: true) def initialize(env)
@env = env @env = env
@request = Rack::Request.new(env) @request = Rack::Request.new(env)
@rate_limit = rate_limit
end end
# our current user, return nil if none is found # our current user, return nil if none is found
@ -63,7 +62,7 @@ class Auth::DefaultCurrentUserProvider
if !current_user if !current_user
@env[BAD_TOKEN] = true @env[BAD_TOKEN] = true
begin begin
limiter.performed! if @rate_limit limiter.performed!
rescue RateLimiter::LimitExceeded rescue RateLimiter::LimitExceeded
raise Discourse::InvalidAccess.new( raise Discourse::InvalidAccess.new(
'Invalid Access', 'Invalid Access',
@ -86,7 +85,7 @@ class Auth::DefaultCurrentUserProvider
# we do not run this rate limiter while profiling # we do not run this rate limiter while profiling
if Rails.env != "profile" 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 = RateLimiter.new(nil, "admin_api_min_#{api_key}", GlobalSetting.max_admin_api_reqs_per_key_per_minute, 60)
limiter_min.performed! if @rate_limit limiter_min.performed!
end end
end end
@ -97,19 +96,19 @@ class Auth::DefaultCurrentUserProvider
limiter_day = RateLimiter.new(nil, "user_api_day_#{user_api_key}", GlobalSetting.max_user_api_reqs_per_day, 86400) limiter_day = RateLimiter.new(nil, "user_api_day_#{user_api_key}", GlobalSetting.max_user_api_reqs_per_day, 86400)
unless limiter_day.can_perform? unless limiter_day.can_perform?
limiter_day.performed! if @rate_limit limiter_day.performed!
end end
unless limiter_min.can_perform? unless limiter_min.can_perform?
limiter_min.performed! if @rate_limit limiter_min.performed!
end end
current_user = lookup_user_api_user_and_update_key(user_api_key, @env[USER_API_CLIENT_ID]) 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 unless current_user
raise Discourse::InvalidAccess if current_user.suspended? || !current_user.active raise Discourse::InvalidAccess if current_user.suspended? || !current_user.active
limiter_min.performed! if @rate_limit limiter_min.performed!
limiter_day.performed! if @rate_limit limiter_day.performed!
@env[USER_API_KEY_ENV] = true @env[USER_API_KEY_ENV] = true
end end

View File

@ -3,8 +3,7 @@ require_dependency 'current_user'
class StaffConstraint class StaffConstraint
def matches?(request) def matches?(request)
provider = Discourse.current_user_provider.new(request.env, rate_limit: false) provider = Discourse.current_user_provider.new(request.env)
provider.current_user && provider.current_user &&
provider.current_user.staff? && provider.current_user.staff? &&
custom_staff_check(request) custom_staff_check(request)

View File

@ -2,19 +2,18 @@ require 'rails_helper'
require_dependency 'auth/default_current_user_provider' require_dependency 'auth/default_current_user_provider'
describe Auth::DefaultCurrentUserProvider do describe Auth::DefaultCurrentUserProvider do
let(:rate_limit) { true }
class TestProvider < Auth::DefaultCurrentUserProvider class TestProvider < Auth::DefaultCurrentUserProvider
attr_reader :env attr_reader :env
def initialize(env, rate_limit: true) def initialize(env)
super(env, rate_limit: rate_limit) super(env)
end end
end end
def provider(url, opts = nil) def provider(url, opts = nil)
opts ||= { method: "GET" } opts ||= { method: "GET" }
env = Rack::MockRequest.env_for(url, opts) env = Rack::MockRequest.env_for(url, opts)
TestProvider.new(env, rate_limit: rate_limit) TestProvider.new(env)
end end
it "can be used to pretend that a user doesn't exist" do it "can be used to pretend that a user doesn't exist" do
@ -146,26 +145,6 @@ describe Auth::DefaultCurrentUserProvider do
provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user provider("/?api_key=#{key}&api_username=#{user.username.downcase}").current_user
end 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
end end