From 72bf8bf4a968a3d256420d9235a122e24be5ce9c Mon Sep 17 00:00:00 2001
From: Jay Joshua <7008757+jayjay-w@users.noreply.github.com>
Date: Mon, 5 Aug 2024 13:53:34 +0200
Subject: [PATCH] Do not rate limit successful login attempts (#1968)

When a login attempt is successful, we should not increment the counter
for rate limiting.
---
 config/initializers/devise.rb      |  7 ++++
 config/initializers/rack_attack.rb |  2 ++
 test/lib/check_rack_attack_test.rb | 55 +++++++++++++++++++++++++++---
 3 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb
index 0c43abae93..795bf0eb2e 100644
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@@ -1,4 +1,5 @@
 require 'error_codes'
+require 'redis'
 
 class CustomFailure < Devise::FailureApp
   def respond
@@ -49,6 +50,12 @@ def http_auth_body
   end
   config.mailer = 'DeviseMailer'
   config.invite_for = 1.month
+
+  Warden::Manager.after_authentication do |user, auth, opts|
+    @redis = Redis.new(REDIS_CONFIG)
+    ip = auth.request.ip
+    @redis.decr("track:#{ip}")
+  end
 end
 
 AuthTrail.geocode = false
diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb
index 8fe9ef61fc..ba10bb09da 100644
--- a/config/initializers/rack_attack.rb
+++ b/config/initializers/rack_attack.rb
@@ -36,6 +36,8 @@ def self.authenticated?(req)
         count = redis.incr("track:#{ip}")
         redis.expire("track:#{ip}", 3600) # Set the expiration time to 1 hour
 
+        redis.set("track:#{ip}", 0) if count < 0
+
         # Add IP to blocklist if count exceeds the threshold
         if count.to_i >= CheckConfig.get('login_block_limit', 100, :integer)
           redis.set("block:#{ip}", true)  # No expiration
diff --git a/test/lib/check_rack_attack_test.rb b/test/lib/check_rack_attack_test.rb
index 006b6aa3ab..efd8a46466 100644
--- a/test/lib/check_rack_attack_test.rb
+++ b/test/lib/check_rack_attack_test.rb
@@ -2,8 +2,12 @@
 
 class ThrottlingTest < ActionDispatch::IntegrationTest
   setup do
-    redis = Redis.new(REDIS_CONFIG)
-    redis.flushdb
+    @redis = Redis.new(REDIS_CONFIG)
+    @redis.flushdb
+  end
+
+  def real_ip(request)
+    request.get_header('HTTP_CF_CONNECTING_IP') || request.remote_ip
   end
 
   test "should throttle excessive requests to /api/graphql" do
@@ -22,7 +26,7 @@ class ThrottlingTest < ActionDispatch::IntegrationTest
     stub_configs({ 'login_block_limit' => 2 }) do
       user_params = { api_user: { email: 'user@example.com', password: random_complex_password } }
 
-      2.times do
+      3.times do
         post api_user_session_path, params: user_params, as: :json
       end
 
@@ -48,7 +52,7 @@ class ThrottlingTest < ActionDispatch::IntegrationTest
       # Test blocking for /api/users/sign_in via Cloudflare
       user_params = { api_user: { email: 'user@example.com', password: random_complex_password } }
 
-      2.times do
+      3.times do
         post api_user_session_path, params: user_params, as: :json, headers: { 'CF-Connecting-IP' => '1.2.3.4' }
       end
 
@@ -66,7 +70,6 @@ class ThrottlingTest < ActionDispatch::IntegrationTest
       user = create_user password: password
       user_params = { api_user: { email: user.email, password: password } }
 
-
       post api_user_session_path, params: user_params, as: :json
       assert_response :success
 
@@ -77,6 +80,48 @@ class ThrottlingTest < ActionDispatch::IntegrationTest
 
       post api_graphql_path
       assert_response :too_many_requests
+
+      delete destroy_api_user_session_path, as: :json
+      assert_response :success
+    end
+  end
+
+  test "should not increment counter on successful login" do
+    stub_configs({ 'login_block_limit' => 3 }) do
+      password = random_complex_password
+      user = create_user password: password
+      user_params = { api_user: { email: user.email, password: password } }
+
+      # Successful logins
+      2.times do
+        post api_user_session_path, params: user_params, as: :json
+        assert_response :success
+      end
+
+      ip = real_ip(@request)
+      counter_value = @redis.get("track:#{ip}")
+      assert_equal "0", counter_value, "Counter should not be incremented for successful logins"
+
+      delete destroy_api_user_session_path, as: :json
+      assert_response :success
+
+      # Unsuccessful login attempts
+      2.times do
+        post api_user_session_path, params: { api_user: { email: user.email, password: 'wrong_password' } }, as: :json
+        assert_response :unauthorized
+      end
+
+      # Check counter value after unsuccessful logins
+      counter_value = @redis.get("track:#{ip}")
+      assert_equal "2", counter_value, "Counter should be incremented for unsuccessful logins"
+
+      # Ensure that the IP is not blocked after successful logins
+      post api_user_session_path, params: user_params, as: :json
+      assert_response :success
+
+      # Subsequent unsuccessful login attempts should result in a block
+      post api_user_session_path, params: { api_user: { email: user.email, password: 'wrong_password' } }, as: :json
+      assert_response :forbidden
     end
   end
 end