From a865b62efc0f7025f789f4413a03ccacd57db7bb Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 9 Dec 2017 14:20:02 +0100 Subject: [PATCH] Rate limit by user instead of IP when API user is authenticated (#5923) * Fix #668 - Rate limit by user instead of IP when API user is authenticated * Fix code style issue * Use request decorator provided by Doorkeeper --- .../concerns/rate_limit_headers.rb | 3 +- config/initializers/rack_attack.rb | 55 ++++++++++++++----- .../concerns/rate_limit_headers_spec.rb | 2 +- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/app/controllers/concerns/rate_limit_headers.rb b/app/controllers/concerns/rate_limit_headers.rb index 36cb910753..ac9b58f5df 100644 --- a/app/controllers/concerns/rate_limit_headers.rb +++ b/app/controllers/concerns/rate_limit_headers.rb @@ -44,7 +44,8 @@ module RateLimitHeaders end def api_throttle_data - request.env['rack.attack.throttle_data']['api'] + request.env['rack.attack.throttle_data']['throttle_authenticated_api'] || + request.env['rack.attack.throttle_data']['throttle_unauthenticated_api'] end def request_time diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 53cb106cac..fddcbc52c3 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -1,6 +1,41 @@ # frozen_string_literal: true class Rack::Attack + class Request + def authenticated_token + return @token if defined?(@token) + + @token = Doorkeeper::OAuth::Token.authenticate( + Doorkeeper::Grape::AuthorizationDecorator.new(self), + *Doorkeeper.configuration.access_token_methods + ) + end + + def authenticated_user_id + authenticated_token&.resource_owner_id + end + + def unauthenticated? + !authenticated_user_id + end + + def api_request? + path.start_with?('/api') + end + + def web_request? + !api_request? + end + end + + PROTECTED_PATHS = %w( + /auth/sign_in + /auth + /auth/password + ).freeze + + PROTECTED_PATHS_REGEX = Regexp.union(PROTECTED_PATHS.map { |path| /\A#{Regexp.escape(path)}/ }) + # Always allow requests from localhost # (blocklist & throttles are skipped) Rack::Attack.safelist('allow from localhost') do |req| @@ -8,24 +43,16 @@ class Rack::Attack '127.0.0.1' == req.ip || '::1' == req.ip end - # Rate limits for the API - throttle('api', limit: 300, period: 5.minutes) do |req| - req.ip if req.path =~ /\A\/api\/v/ - end - - # Rate limit logins - throttle('login', limit: 5, period: 5.minutes) do |req| - req.ip if req.path == '/auth/sign_in' && req.post? + throttle('throttle_authenticated_api', limit: 300, period: 5.minutes) do |req| + req.api_request? && req.authenticated_user_id end - # Rate limit sign-ups - throttle('register', limit: 5, period: 5.minutes) do |req| - req.ip if req.path == '/auth' && req.post? + throttle('throttle_unauthenticated_api', limit: 300, period: 5.minutes) do |req| + req.ip if req.api_request? && req.unauthenticated? end - # Rate limit forgotten passwords - throttle('reminder', limit: 5, period: 5.minutes) do |req| - req.ip if req.path == '/auth/password' && req.post? + throttle('protected_paths', limit: 5, period: 5.minutes) do |req| + req.ip if req.post? && req.path =~ PROTECTED_PATHS_REGEX end self.throttled_response = lambda do |env| diff --git a/spec/controllers/concerns/rate_limit_headers_spec.rb b/spec/controllers/concerns/rate_limit_headers_spec.rb index 719978dc23..00a9a2080d 100644 --- a/spec/controllers/concerns/rate_limit_headers_spec.rb +++ b/spec/controllers/concerns/rate_limit_headers_spec.rb @@ -34,7 +34,7 @@ describe ApplicationController do let(:start_time) { DateTime.new(2017, 1, 1, 12, 0, 0).utc } before do - request.env['rack.attack.throttle_data'] = { 'api' => { limit: 100, count: 20, period: 10 } } + request.env['rack.attack.throttle_data'] = { 'throttle_authenticated_api' => { limit: 100, count: 20, period: 10 } } travel_to start_time do get 'show' end