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

diff --git a/app/controllers/concerns/rate_limit_headers.rb b/app/controllers/concerns/rate_limit_headers.rb
index 36cb91075343c7a6480dd36ed0c770660424ec3c..ac9b58f5dfb47f9667755c95b1efef29bb56651c 100644 (file)
--- 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 53cb106cac427b42687a108f588c6b68bcc73fb0..fddcbc52c3df418ba80ca8fa0099c13595b9fae0 100644 (file)
--- 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 719978dc23fc794d87b2265bdf081427da1fc15a..00a9a2080d6837cf1e8de19245d1e63b5d227d9b 100644 (file)
--- 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