Respect invalid login attempts when evaluating 2FA codes

https://community.openproject.org/work_packages/73313

Author: oliverguenther

modules/two_factor_authentication/app/controllers/concerns/two_factor_authentication/backup_codes.rb

@@ -21,6 +21,7 @@ def verify_backup_code
       if result.success?
         complete_stage_redirect
       else
+        @authenticated_user.log_failed_login
         fail_login(t("two_factor_authentication.error_invalid_backup_code"))
       end
     end

modules/two_factor_authentication/app/controllers/two_factor_authentication/authentication_controller.rb

@@ -8,6 +8,7 @@ class AuthenticationController < ApplicationController
     include ::TwoFactorAuthentication::BackupCodes
     # Webauthn relying party based on domain
     include ::TwoFactorAuthentication::WebauthnRelyingParty
+
     # Include global layout helper
     layout "no_menu"
 
@@ -24,9 +25,15 @@ class AuthenticationController < ApplicationController
     before_action :only_post, only: :confirm_otp
 
     # Require authenticated user from the core to be present
+    # rubocop:disable Rails/LexicallyScopedActionFilter
     before_action :require_authenticated_user,
                   only: %i(request_otp enter_backup_code verify_backup_code confirm_otp retry webauthn_challenge)
 
+    # Prevent additional tries when login attempts exceeded
+    before_action :check_brute_force_protection,
+                  only: %i[confirm_otp verify_backup_code]
+    # rubocop:enable Rails/LexicallyScopedActionFilter
+
     before_action :ensure_valid_configuration, only: [:request_otp]
 
     ##
@@ -179,6 +186,7 @@ def login_if_otp_token_valid(user:, otp_token:, webauthn_credential:)
         set_remember_token!
         complete_stage_redirect
       else
+        user.log_failed_login
         fail_login(I18n.t(:notice_account_otp_invalid))
       end
     end
@@ -193,6 +201,17 @@ def only_post
       end
     end
 
+    ##
+    # Block 2FA submission if the user has exceeded the brute force attempt threshold
+    def check_brute_force_protection
+      return unless @authenticated_user
+
+      if @authenticated_user.failed_too_many_recent_login_attempts?
+        flash[:error] = I18n.t(:notice_account_invalid_credentials_or_blocked)
+        failure_stage_redirect
+      end
+    end
+
     ##
     # fail the login
     def fail_login(msg)

modules/two_factor_authentication/spec/controllers/two_factor_authentication/authentication_controller_spec.rb

@@ -116,4 +116,79 @@
       expect(response.response_code).to eq(405)
     end
   end
+
+  describe "brute force protection",
+           with_settings: {
+             "plugin_openproject_two_factor_authentication" => { active_strategies: %i[developer totp] },
+             brute_force_block_after_failed_logins: 5,
+             brute_force_block_minutes: 30
+           } do
+    let!(:device) { create(:two_factor_authentication_device_totp, user:, channel: :totp) }
+
+    before do
+      session[:authenticated_user_id] = user.id
+    end
+
+    describe "POST confirm_otp with an invalid token" do
+      before do
+        post :confirm_otp, params: { otp: "000000" }
+      end
+
+      it "increments the failed_login_count" do
+        expect(user.reload.failed_login_count).to eq 1
+      end
+
+      it "redirects to the stage failure path" do
+        expect(response).to redirect_to stage_failure_path(stage: :two_factor_authentication)
+      end
+    end
+
+    describe "POST confirm_otp when the user is already brute-force-blocked" do
+      before do
+        user.update!(failed_login_count: 5, last_failed_login_on: Time.zone.now)
+        post :confirm_otp, params: { otp: "000000" }
+      end
+
+      it "redirects to the stage failure path" do
+        expect(response).to redirect_to stage_failure_path(stage: :two_factor_authentication)
+      end
+
+      it "sets the blocked error message" do
+        expect(flash[:error]).to eq I18n.t(:notice_account_invalid_credentials_or_blocked)
+      end
+
+      it "does not increment failed_login_count further" do
+        expect(user.reload.failed_login_count).to eq 5
+      end
+    end
+
+    describe "POST verify_backup_code with an invalid code" do
+      before do
+        post :verify_backup_code, params: { backup_code: "invalid-backup-code" }
+      end
+
+      it "increments the failed_login_count" do
+        expect(user.reload.failed_login_count).to eq 1
+      end
+
+      it "redirects to the stage failure path" do
+        expect(response).to redirect_to stage_failure_path(stage: :two_factor_authentication)
+      end
+    end
+
+    describe "POST verify_backup_code when the user is already brute-force-blocked" do
+      before do
+        user.update!(failed_login_count: 5, last_failed_login_on: Time.zone.now)
+        post :verify_backup_code, params: { backup_code: "any-code" }
+      end
+
+      it "redirects to the stage failure path" do
+        expect(response).to redirect_to stage_failure_path(stage: :two_factor_authentication)
+      end
+
+      it "sets the blocked error message" do
+        expect(flash[:error]).to eq I18n.t(:notice_account_invalid_credentials_or_blocked)
+      end
+    end
+  end
 end