diff --git a/app/controllers/concerns/accounts/user_password_change.rb b/app/controllers/concerns/accounts/user_password_change.rb index 4da16df0dac..405ddb4fd4c 100644 --- a/app/controllers/concerns/accounts/user_password_change.rb +++ b/app/controllers/concerns/accounts/user_password_change.rb @@ -42,8 +42,15 @@ module Accounts::UserPasswordChange # auth sources in the admin UI, so this shouldn't normally happen. return if redirect_if_password_change_not_allowed(user) + # Check if user is locked due to too many failed attempts + if user.failed_too_many_recent_login_attempts? + flash_and_log_invalid_credentials(is_logged_in: !show_user_name) + return render_password_change(user, nil, show_user_name:) + end + # Ensure the current password is validated unless user.check_password?(params[:password], update_legacy:) + user.log_failed_login flash_and_log_invalid_credentials(is_logged_in: !show_user_name) return render_password_change(user, nil, show_user_name:) end @@ -53,6 +60,9 @@ module Accounts::UserPasswordChange # Yield the success to the caller if call.success? + # Reset failed login count on successful password change + User.reset_failed_login_count_for(user) + response = yield call call.apply_flash_message!(flash) diff --git a/spec/controllers/account_controller_spec.rb b/spec/controllers/account_controller_spec.rb index f25a87be97b..9890a83de0b 100644 --- a/spec/controllers/account_controller_spec.rb +++ b/spec/controllers/account_controller_spec.rb @@ -515,6 +515,171 @@ RSpec.describe AccountController, :skip_2fa_stage do expect(response).to have_http_status :not_found end end + + context "with brute force protection", + with_settings: { brute_force_block_minutes: 30, brute_force_block_after_failed_logins: 20 } do + shared_let(:user) { create(:user, login: "testuser", password: "ValidPass123!", password_confirmation: "ValidPass123!") } + + describe "blocks password change attempts after too many failures" do + before do + user.update_columns( + failed_login_count: 20, + last_failed_login_on: 1.minute.ago + ) + + post :change_password, + params: { + password_change_user: user.login, + password: "ValidPass123!", + new_password: "NewPass123!", + new_password_confirmation: "NewPass123!" + } + end + + it "blocks the attempt even with correct password" do + expect(response).to have_http_status :unprocessable_entity + end + + it "does not change the password" do + user.reload + expect(user.check_password?("ValidPass123!")).to be true + expect(user.check_password?("NewPass123!")).to be false + end + + it "shows an error message" do + if Setting.brute_force_block_after_failed_logins.to_i > 0 + expected_message = I18n.t(:notice_account_invalid_credentials_or_blocked) + else + expected_message = I18n.t(:notice_account_invalid_credentials) + end + expect(flash[:error]).to eq(expected_message) + end + end + + describe "logs failed password attempts" do + before do + user.update_columns( + failed_login_count: 0, + last_failed_login_on: nil + ) + + post :change_password, + params: { + password_change_user: user.login, + password: "WrongPassword!", + new_password: "NewPass123!", + new_password_confirmation: "NewPass123!" + } + end + + it "increments failed login count" do + user.reload + expect(user.failed_login_count).to eq(1) + end + + it "updates last failed login timestamp" do + user.reload + expect(user.last_failed_login_on).to be_within(1.second).of(Time.zone.now) + end + + it "does not change the password" do + user.reload + expect(user.check_password?("ValidPass123!")).to be true + expect(user.check_password?("NewPass123!")).to be false + end + end + + describe "accumulates multiple failed attempts" do + it "blocks after reaching the threshold" do + user.update_columns( + failed_login_count: 0, + last_failed_login_on: nil + ) + + # Make 20 failed attempts + 20.times do + post :change_password, + params: { + password_change_user: user.login, + password: "WrongPassword!", + new_password: "NewPass123!", + new_password_confirmation: "NewPass123!" + } + end + + user.reload + expect(user.failed_login_count).to eq(20) + + # Next attempt should be blocked even with correct password + post :change_password, + params: { + password_change_user: user.login, + password: "ValidPass123!", + new_password: "NewPass123!", + new_password_confirmation: "NewPass123!" + } + + user.reload + expect(user.check_password?("ValidPass123!")).to be true + expect(user.check_password?("NewPass123!")).to be false + end + end + + describe "resets failed login count on successful password change" do + before do + user.update_columns( + failed_login_count: 5, + last_failed_login_on: 1.minute.ago + ) + + post :change_password, + params: { + password_change_user: user.login, + password: "ValidPass123!", + new_password: "NewPass123!", + new_password_confirmation: "NewPass123!" + } + end + + it "resets the failed login count to zero" do + user.reload + expect(user.failed_login_count).to eq(0) + end + + it "changes the password successfully" do + user.reload + expect(user.check_password?("NewPass123!")).to be true + expect(user.check_password?("ValidPass123!")).to be false + end + end + + describe "allows password change after block time expires" do + before do + user.update_columns( + failed_login_count: 20, + last_failed_login_on: 31.minutes.ago + ) + + post :change_password, + params: { + password_change_user: user.login, + password: "ValidPass123!", + new_password: "NewPass123!", + new_password_confirmation: "NewPass123!" + } + end + + it "allows the password change" do + user.reload + expect(user.check_password?("NewPass123!")).to be true + end + + it "resets the failed login count" do + user.reload + expect(user.failed_login_count).to eq(0) + end + end + end end describe "POST #lost_password" do diff --git a/spec/controllers/my_controller_spec.rb b/spec/controllers/my_controller_spec.rb index c3c63df2d93..da4ce6deb69 100644 --- a/spec/controllers/my_controller_spec.rb +++ b/spec/controllers/my_controller_spec.rb @@ -120,6 +120,87 @@ RSpec.describe MyController do assert User.try_to_login(user.login, "adminADMIN!New") end end + + describe "with brute force protection", + with_settings: { brute_force_block_minutes: 30, brute_force_block_after_failed_logins: 20 } do + describe "blocks password change attempts after too many failures" do + before do + user.update_columns( + failed_login_count: 20, + last_failed_login_on: 1.minute.ago + ) + + post :change_password, + params: { + password: "adminADMIN!", + new_password: "adminADMIN!New", + new_password_confirmation: "adminADMIN!New" + } + end + + it "blocks the attempt even with correct password" do + expect(response).to have_http_status :unprocessable_entity + end + + it "does not change the password" do + user.reload + expect(user.check_password?("adminADMIN!")).to be true + expect(user.check_password?("adminADMIN!New")).to be false + end + end + + describe "logs failed password attempts" do + before do + user.update_columns( + failed_login_count: 0, + last_failed_login_on: nil + ) + + post :change_password, + params: { + password: "WrongPassword!", + new_password: "adminADMIN!New", + new_password_confirmation: "adminADMIN!New" + } + end + + it "increments failed login count" do + user.reload + expect(user.failed_login_count).to eq(1) + end + + it "updates last failed login timestamp" do + user.reload + expect(user.last_failed_login_on).to be_within(1.second).of(Time.zone.now) + end + end + + describe "resets failed login count on successful password change" do + before do + user.update_columns( + failed_login_count: 5, + last_failed_login_on: 1.minute.ago + ) + + post :change_password, + params: { + password: "adminADMIN!", + new_password: "adminADMIN!New", + new_password_confirmation: "adminADMIN!New" + } + end + + it "resets the failed login count to zero" do + user.reload + expect(user.failed_login_count).to eq(0) + end + + it "changes the password successfully" do + user.reload + expect(user.check_password?("adminADMIN!New")).to be true + end + end + end end describe "account" do