DEV: Move logic for rate limiting user second factor to one place (#11941)

This moves all the rate limiting for user second factor (based on `params[:second_factor_token]` existing) to the one place, which rate limits by IP and also by username if a user is found.

app/controllers/application_controller.rb

@@ -542,6 +542,16 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  def rate_limit_second_factor!(user)
+    return if params[:second_factor_token].blank?
+
+    RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 6, 1.minute).performed!
+
+    if user
+      RateLimiter.new(nil, "second-factor-min-#{user.username}", 6, 1.minute).performed!
+    end
+  end
+
   private
 
   def locale_from_header

app/controllers/session_controller.rb

@@ -3,7 +3,6 @@
 class SessionController < ApplicationController
   before_action :check_local_login_allowed, only: %i(create forgot_password)
   before_action :rate_limit_login, only: %i(create email_login)
-  before_action :rate_limit_second_factor_totp, only: %i(create email_login)
   skip_before_action :redirect_to_login_if_required
   skip_before_action :preload_json, :check_xhr, only: %i(sso sso_login sso_provider destroy one_time_password)
 
@@ -277,7 +276,10 @@ class SessionController < ApplicationController
 
     return invalid_credentials if params[:password].length > User.max_password_length
 
-    if user = User.find_by_username_or_email(normalized_login_param)
+    user = User.find_by_username_or_email(normalized_login_param)
+    rate_limit_second_factor!(user)
+
+    if user.present?
 
       # If their password is correct
       unless user.confirm_password?(params[:password])
@@ -355,6 +357,8 @@ class SessionController < ApplicationController
 
     check_local_login_allowed(user: user, check_login_via_email: true)
 
+    rate_limit_second_factor!(user)
+
     if user.present? && !authenticate_second_factor(user)
       return render(json: @second_factor_failure_payload)
     end
@@ -599,14 +603,6 @@ class SessionController < ApplicationController
     ).performed!
   end
 
-  def rate_limit_second_factor_totp
-    return if params[:second_factor_token].blank?
-    RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
-    if normalized_login_param.present?
-      RateLimiter.new(nil, "second-factor-min-#{normalized_login_param}", 3, 1.minute).performed!
-    end
-  end
-
   def render_sso_error(status:, text:)
     @sso_error = text
     render status: status, layout: 'no_ember'

app/controllers/users_controller.rb

@@ -729,9 +729,7 @@ class UsersController < ApplicationController
     token = params[:token]
     password_reset_find_user(token, committing_change: true)
 
-    if params[:second_factor_token].present?
+    rate_limit_second_factor!(@user)
-      RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
-    end
 
     # no point doing anything else if we can't even find
     # a user from the token
@@ -1389,9 +1387,7 @@ class UsersController < ApplicationController
     totp_data = secure_session["staged-totp-#{current_user.id}"]
     totp_object = current_user.get_totp_object(totp_data)
 
-    [request.remote_ip, current_user.id].each do |key|
+    rate_limit_second_factor!(current_user)
-      RateLimiter.new(nil, "second-factor-min-#{key}", 3, 1.minute).performed!
-    end
 
     authenticated = !auth_token.blank? && totp_object.verify(
       auth_token,

app/controllers/users_email_controller.rb

@@ -77,7 +77,7 @@ class UsersEmailController < ApplicationController
 
     redirect_url = path("/u/confirm-new-email/#{params[:token]}")
 
-    RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed! if params[:second_factor_token].present?
+    rate_limit_second_factor!(@user)
 
     if !@error
       # this is needed becase the form posts this field as JSON and it can be a

spec/requests/session_controller_spec.rb

@@ -1735,11 +1735,12 @@ RSpec.describe SessionController do
         RateLimiter.enable
         RateLimiter.clear_all!
 
-        3.times do |x|
+        6.times do |x|
           post "/session.json", params: {
             login: "#{user.username}#{x}",
             password: 'myawesomepassword',
-            second_factor_token: '000000'
+            second_factor_token: '000000',
+            second_factor_method: UserSecondFactor.methods[:totp]
           }
           expect(response.status).to eq(200)
         end
@@ -1747,7 +1748,8 @@ RSpec.describe SessionController do
         post "/session.json", params: {
           login: user.username,
           password: 'myawesomepassword',
-          second_factor_token: '000000'
+          second_factor_token: '000000',
+          second_factor_method: UserSecondFactor.methods[:totp]
         }
 
         expect(response.status).to eq(429)
@@ -1759,11 +1761,12 @@ RSpec.describe SessionController do
         RateLimiter.enable
         RateLimiter.clear_all!
 
-        3.times do |x|
+        6.times do |x|
           post "/session.json", params: {
             login: user.username,
             password: 'myawesomepassword',
-            second_factor_token: '000000'
+            second_factor_token: '000000',
+            second_factor_method: UserSecondFactor.methods[:totp]
           }, env: { "REMOTE_ADDR": "1.2.3.#{x}" }
 
           expect(response.status).to eq(200)
@@ -1773,7 +1776,8 @@ RSpec.describe SessionController do
           post "/session.json", params: {
             login: username,
             password: 'myawesomepassword',
-            second_factor_token: '000000'
+            second_factor_token: '000000',
+            second_factor_method: UserSecondFactor.methods[:totp]
           }, env: { "REMOTE_ADDR": "1.2.4.#{x}" }
 
           expect(response.status).to eq(429)

spec/requests/users_controller_spec.rb

@@ -323,7 +323,6 @@ describe UsersController do
       end
 
       context "rate limiting" do
-
         before { RateLimiter.clear_all!; RateLimiter.enable }
         after  { RateLimiter.disable }
 
@@ -332,7 +331,7 @@ describe UsersController do
 
           token = user.email_tokens.create!(email: user.email).token
 
-          3.times do
+          6.times do
             put "/u/password-reset/#{token}", params: {
               second_factor_token: 123456,
               second_factor_method: 1
@@ -349,6 +348,27 @@ describe UsersController do
           expect(response.status).to eq(429)
         end
 
+        it "rate limits reset passwords by username" do
+          freeze_time
+
+          token = user.email_tokens.create!(email: user.email).token
+
+          6.times do |x|
+            put "/u/password-reset/#{token}", params: {
+              second_factor_token: 123456,
+              second_factor_method: 1
+            }, env: { "REMOTE_ADDR": "1.2.3.#{x}" }
+
+            expect(response.status).to eq(200)
+          end
+
+          put "/u/password-reset/#{token}", params: {
+            second_factor_token: 123456,
+            second_factor_method: 1
+          }, env: { "REMOTE_ADDR": "1.2.3.4" }
+
+          expect(response.status).to eq(429)
+        end
       end
 
       context '2 factor authentication required' do
@@ -3997,6 +4017,36 @@ describe UsersController do
       expect(user.user_second_factors.count).to eq(1)
     end
 
+    it "rate limits by IP address" do
+      RateLimiter.enable
+      RateLimiter.clear_all!
+
+      create_totp
+      staged_totp_key = read_secure_session["staged-totp-#{user.id}"]
+      token = ROTP::TOTP.new(staged_totp_key).now
+
+      7.times do |x|
+        post "/users/enable_second_factor_totp.json", params: { name: "test", second_factor_token: token  }
+      end
+
+      expect(response.status).to eq(429)
+    end
+
+    it "rate limits by username" do
+      RateLimiter.enable
+      RateLimiter.clear_all!
+
+      create_totp
+      staged_totp_key = read_secure_session["staged-totp-#{user.id}"]
+      token = ROTP::TOTP.new(staged_totp_key).now
+
+      7.times do |x|
+        post "/users/enable_second_factor_totp.json", params: { name: "test", second_factor_token: token  }, env: { "REMOTE_ADDR": "1.2.3.#{x}"  }
+      end
+
+      expect(response.status).to eq(429)
+    end
+
     context "when an incorrect token is provided" do
       before do
         create_totp

spec/requests/users_email_controller_spec.rb

@@ -131,6 +131,57 @@ describe UsersEmailController do
           user.reload
           expect(user.email).to eq("new.n.cool@example.com")
         end
+
+        context "rate limiting" do
+          before { RateLimiter.clear_all!; RateLimiter.enable }
+          after  { RateLimiter.disable }
+
+          it "rate limits by IP" do
+            freeze_time
+
+            6.times do
+              put "/u/confirm-new-email", params: {
+                token: "blah",
+                second_factor_token: "000000",
+                second_factor_method: UserSecondFactor.methods[:totp]
+              }
+
+              expect(response.status).to eq(302)
+            end
+
+            put "/u/confirm-new-email", params: {
+              token: "blah",
+              second_factor_token: "000000",
+              second_factor_method: UserSecondFactor.methods[:totp]
+            }
+
+            expect(response.status).to eq(429)
+          end
+
+          it "rate limits by username" do
+            freeze_time
+
+            6.times do |x|
+              user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:complete])
+              put "/u/confirm-new-email", params: {
+                token: user.email_tokens.last.token,
+                second_factor_token: "000000",
+                second_factor_method: UserSecondFactor.methods[:totp]
+              }, env: { "REMOTE_ADDR": "1.2.3.#{x}" }
+
+              expect(response.status).to eq(302)
+            end
+
+            user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:authorizing_new])
+            put "/u/confirm-new-email", params: {
+              token: user.email_tokens.last.token,
+              second_factor_token: "000000",
+              second_factor_method: UserSecondFactor.methods[:totp]
+            }, env: { "REMOTE_ADDR": "1.2.3.4" }
+
+            expect(response.status).to eq(429)
+          end
+        end
       end
 
       context "security key required" do