diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index dfe229526d..a19e8312bf 100644 --- a/app/controllers/account_controller.rb +++ b/app/controllers/account_controller.rb @@ -88,6 +88,7 @@ class AccountController < ApplicationController @user.must_change_passwd = false if @user.save @token.destroy + @user.reset_failed_login_attempts! Mailer.deliver_password_updated(@user, User.current) flash[:notice] = l(:notice_account_password_updated) redirect_to signin_path @@ -186,6 +187,20 @@ class AccountController < ApplicationController redirect_to signin_path end + # Token based account unlock + def unlock + token = Token.find_token('unlock', params[:token].to_s) + (redirect_to(home_url); return) unless token and !token.expired? + user = token.user + (redirect_to(home_url); return) unless user.locked? + user.reset_failed_login_attempts! + if user.save + token.destroy + flash[:notice] = l(:notice_account_unlocked) + end + redirect_to signin_path + end + # Sends a new account activation email def activation_email if session[:registered_user_id] && Setting.self_registration == '1' @@ -229,9 +244,15 @@ class AccountController < ApplicationController # prevent brute force attacks on the one-time password elsif session[:twofa_tries_counter] > 3 destroy_twofa_session + @user.add_failed_login_attempts! + account_locked(@user) and return if @user.locked? + flash[:error] = l('twofa_too_many_tries') redirect_to home_url else + @user.add_failed_login_attempts! + account_locked(@user) and return if @user.locked? + flash[:error] = l('twofa_invalid_code') redirect_to account_twofa_confirm_path end @@ -330,6 +351,8 @@ class AccountController < ApplicationController end def successful_authentication(user) + user.reset_failed_login_attempts! + logger.info "Successful authentication for '#{user.login}' from #{request.remote_ip} at #{Time.now.utc}" # Valid user self.logged_user = user @@ -366,6 +389,10 @@ class AccountController < ApplicationController end def invalid_credentials + user = User.find_by_login(params[:username].to_s.strip) + user.add_failed_login_attempts! if user + account_locked(user) and return if user && user.locked? + logger.warn "Failed login for '#{params[:username]}' from #{request.remote_ip} at #{Time.now.utc}" flash.now[:error] = l(:notice_account_invalid_credentials) end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 77ec1a6013..63105f4e55 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -168,6 +168,8 @@ class UsersController < ApplicationController was_activated = (@user.status_change == [User::STATUS_REGISTERED, User::STATUS_ACTIVE]) # TODO: Similar to My#account @user.pref.safe_attributes = params[:pref] + # Reset failed_login_attempts if changed from other status to active + @user.failed_login_attempts = 0 if @user.will_save_change_to_status? && @user.active? if @user.save @user.pref.save diff --git a/app/models/mailer.rb b/app/models/mailer.rb index 558c280ec8..a326ce640c 100644 --- a/app/models/mailer.rb +++ b/app/models/mailer.rb @@ -459,6 +459,22 @@ class Mailer < ActionMailer::Base register(user, token).deliver_later end + # Builds a mail to user with account unlock link. + def locked(user, token) + @token = token + @url = url_for(controller: 'account', action: 'unlock', token: token.value) + mail :to => user.mail, + :subject => l(:mail_subject_locked, Setting.app_title) + end + + # Sends an mail to user with account unlock link. + # + # Exemple: + # Mailer.deliver_locked(user, token) + def self.deliver_locked(user, token) + locked(user, token).deliver_later + end + # Build a mail to user and the additional recipients given in # options[:recipients] about a security related event made by sender. # diff --git a/app/models/token.rb b/app/models/token.rb index 1fd9c228b1..e8ae62c6ba 100644 --- a/app/models/token.rb +++ b/app/models/token.rb @@ -41,6 +41,7 @@ class Token < ActiveRecord::Base add_action :feeds, max_instances: 1, validity_time: nil add_action :recovery, max_instances: 1, validity_time: Proc.new {Token.validity_time} add_action :register, max_instances: 1, validity_time: Proc.new {Token.validity_time} + add_action :unlock, max_instances: 1, validity_time: Proc.new {Token.validity_time} add_action :session, max_instances: 10, validity_time: nil add_action :twofa_backup_code, max_instances: 10, validity_time: nil diff --git a/app/models/user.rb b/app/models/user.rb index 7180191d93..8bb555dd34 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -284,6 +284,7 @@ class User < Principal end def activate + self.failed_login_attempts = 0 self.status = STATUS_ACTIVE end @@ -296,6 +297,7 @@ class User < Principal end def activate! + self.failed_login_attempts = 0 update_attribute(:status, STATUS_ACTIVE) end @@ -897,6 +899,43 @@ class User < Principal User.where("created_on < ? AND status = ?", Time.now - age, STATUS_REGISTERED).destroy_all end + def reset_failed_login_attempts! + return if failed_login_attempts.zero? && self.active? + + self.failed_login_attempts = 0 + self.activate if self.locked? + self.save! + end + + def add_failed_login_attempts! + self.failed_login_attempts = self.failed_login_attempts + 1 + + if Setting.max_login_attempts.present? && self.failed_login_attempts > Setting.max_login_attempts.to_i && self.active? + self.lock_by_failed_login_attempts! + else + self.save! + end + end + + # Execute in the event of consecutive failed login attempts. + # Lock user, create unlock token, send email to locked user and send security notification emails to administrators. + def lock_by_failed_login_attempts! + self.lock + token = Token.new(user: self, action: 'unlock') + self.save! and token.save! + Mailer.deliver_locked(self, token) + + options = { + field: User.current.admin? ? :field_admin : :field_user, + value: login, + title: :label_user_plural, + url: {controller: 'users', action: 'index', status: STATUS_LOCKED}, + message: :mail_body_security_notification_locked + } + users = User.active.where(admin: true).to_a + Mailer.deliver_security_notification(users, User.current, options) + end + protected def validate_password_length diff --git a/app/views/mailer/locked.html.erb b/app/views/mailer/locked.html.erb new file mode 100644 index 0000000000..7f69490555 --- /dev/null +++ b/app/views/mailer/locked.html.erb @@ -0,0 +1,2 @@ +

<%= simple_format(l(:mail_body_locked)) %>
+<%= link_to @url, @url %>

diff --git a/app/views/mailer/locked.text.erb b/app/views/mailer/locked.text.erb new file mode 100644 index 0000000000..b930a359a3 --- /dev/null +++ b/app/views/mailer/locked.text.erb @@ -0,0 +1,2 @@ +<%= l(:mail_body_locked) %> +<%= @url %> diff --git a/app/views/settings/_authentication.html.erb b/app/views/settings/_authentication.html.erb index fc20dd03db..cf034fce3a 100644 --- a/app/views/settings/_authentication.html.erb +++ b/app/views/settings/_authentication.html.erb @@ -41,6 +41,9 @@

+

+ <%= setting_text_field :max_login_attempts, :size => 6 %> +

diff --git a/config/locales/en.yml b/config/locales/en.yml index 5397a521d3..c802121ba9 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -159,6 +159,7 @@ en: notice_account_register_done: Account was successfully created. An email containing the instructions to activate your account was sent to %{email}. notice_account_not_activated_yet: You haven't activated your account yet. If you want to receive a new activation email, please click this link. notice_account_locked: Your account is locked. + notice_account_unlocked: Your account is unlocked. notice_can_t_change_password: This account uses an external authentication source. Impossible to change the password. notice_account_lost_email_sent: An email with instructions to choose a new password has been sent to you. notice_account_activated: Your account has been activated. You can now log in. @@ -251,6 +252,8 @@ en: mail_body_lost_password_validity: 'Please be aware that you may change the password only once using this link.' mail_subject_register: "Your %{value} account activation" mail_body_register: 'To activate your account, click on the following link:' + mail_subject_locked: "Your %{value} account locked" + mail_body_locked: "The account was locked because the number of failed login attempts exceeded a certain number of times.\nIf you are not aware of any login failures, reset your password.\n\nTo unlock your account, click on the following link:" mail_body_account_information_external: "You can use your %{value} account to log in." mail_body_account_information: Your account information mail_subject_account_activation_request: "%{value} account activation request" @@ -266,6 +269,7 @@ en: mail_body_security_notification_change_to: "%{field} was changed to %{value}." mail_body_security_notification_add: "%{field} %{value} was added." mail_body_security_notification_remove: "%{field} %{value} was removed." + mail_body_security_notification_locked: "%{field} %{value} was locked." mail_body_security_notification_notify_enabled: "Email address %{value} now receives notifications." mail_body_security_notification_notify_disabled: "Email address %{value} no longer receives notifications." mail_body_settings_updated: "The following settings were changed:" @@ -471,6 +475,7 @@ en: setting_password_min_length: Minimum password length setting_password_required_char_classes : Required character classes for passwords setting_lost_password: Allow password reset via email + setting_max_login_attempts: Max login attempts setting_new_project_user_role_id: Role given to a non-admin user who creates a project setting_default_projects_modules: Default enabled modules for new projects setting_issue_done_ratio: Calculate the issue done ratio with diff --git a/config/routes.rb b/config/routes.rb index 00eadf6795..67e8729283 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -28,6 +28,7 @@ Rails.application.routes.draw do match 'account/register', :to => 'account#register', :via => [:get, :post], :as => 'register' match 'account/lost_password', :to => 'account#lost_password', :via => [:get, :post], :as => 'lost_password' match 'account/activate', :to => 'account#activate', :via => :get + match 'account/unlock', :to => 'account#unlock', :via => :get get 'account/activation_email', :to => 'account#activation_email', :as => 'activation_email' match '/news/preview', :controller => 'previews', :action => 'news', :as => 'preview_news', :via => [:get, :post, :put, :patch] diff --git a/config/settings.yml b/config/settings.yml index 6db94a50f9..67eff52816 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -52,6 +52,10 @@ password_max_age: format: int default: 0 security_notifications: 1 +max_login_attempts: + format: int + default: + security_notifications: 1 # Maximum number of additional email addresses per user max_additional_emails: format: int diff --git a/db/migrate/20230808010454_add_failed_login_attempts_to_users.rb b/db/migrate/20230808010454_add_failed_login_attempts_to_users.rb new file mode 100644 index 0000000000..a6d2358876 --- /dev/null +++ b/db/migrate/20230808010454_add_failed_login_attempts_to_users.rb @@ -0,0 +1,5 @@ +class AddFailedLoginAttemptsToUsers < ActiveRecord::Migration[6.1] + def change + add_column :users, :failed_login_attempts, :integer, default: 0, null: false + end +end diff --git a/test/functional/account_controller_test.rb b/test/functional/account_controller_test.rb index 034094e7b1..975938fe8a 100644 --- a/test/functional/account_controller_test.rb +++ b/test/functional/account_controller_test.rb @@ -21,6 +21,7 @@ require_relative '../test_helper' class AccountControllerTest < Redmine::ControllerTest fixtures :users, :email_addresses, :roles + include Redmine::I18n def setup User.current = nil @@ -172,6 +173,24 @@ class AccountControllerTest < Redmine::ControllerTest assert_select 'input[name=username][value=admin]' assert_select 'input[name=password]' assert_select 'input[name=password][value]', 0 + assert_equal 1, User.find(1).failed_login_attempts + end + + def test_login_should_reset_failed_login_attempts_when_password_is_correct + user = User.find(1) + user.update!(failed_login_attempts: 5, status: User::STATUS_ACTIVE) + + with_settings max_login_attempts: 5 do + post( + :login, + :params => { + :username => 'admin', + :password => 'admin' + } + ) + assert_equal 0, user.reload.failed_login_attempts + assert user.active? + end end def test_login_with_locked_account_should_fail @@ -180,7 +199,7 @@ class AccountControllerTest < Redmine::ControllerTest :login, :params => { :username => 'jsmith', - :password => 'jsmith' + :password => 'bad' } ) assert_redirected_to '/login' @@ -638,6 +657,39 @@ class AccountControllerTest < Redmine::ControllerTest assert_redirected_to '/' end + def test_unlock_should_unlock_user_with_valid_token + user = User.find(1) + user.lock + token = Token.new(user: user, action: 'unlock') + user.save! and token.save! + + get :unlock, params: { token: token.value } + assert_redirected_to signin_path + assert_equal l(:notice_account_unlocked), flash[:notice] + assert_not user.reload.locked? + assert_nil Token.find_by(id: token.id) + end + + def test_unlock_should_redirect_to_home_when_token_is_not_found + user = User.find(1) + user.lock + token = Token.new(user: user, action: 'unlock') + user.save! and token.save! + + get :unlock, params: { token: 'invalid_token' } + assert_redirected_to home_url + end + + def test_unlock_should_redirect_to_home_when_token_is_expired + user = User.find(1) + user.lock + token = Token.new(user: user, action: 'unlock', created_on: 2.days.ago) + user.save! and token.save! + + get :unlock, params: { token: token.value } + assert_redirected_to home_url + end + def test_activation_email_should_send_an_activation_email User.find(2).update_attribute :status, User::STATUS_REGISTERED @request.session[:registered_user_id] = 2 diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index bb108549bc..75c7d6a9d9 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -1358,4 +1358,39 @@ class UserTest < ActiveSupport::TestCase User.prune(7) end end + + def test_reset_failed_login_attempts_should_set_failed_login_attempts_to_zero_and_activate + user = User.find(1) + user.update!(failed_login_attempts: 10, status: User::STATUS_LOCKED) + + user.reset_failed_login_attempts! + assert_equal 0, user.reload.failed_login_attempts + assert user.active? + end + + def test_add_failed_login_attempts_should_increment_failed_attempts_by_one + user = User.find(1) + user.update!(failed_login_attempts: 1) + + with_settings max_login_attempts: 5 do + user.add_failed_login_attempts! + assert_equal 2, user.failed_login_attempts + assert user.active? + end + end + + def test_add_failed_login_attempts_should_lock_user_after_max_attempts + user = User.find(1) + user.update!(failed_login_attempts: 5) + + with_settings max_login_attempts: 5 do + Mailer.expects(:deliver_locked).at_least_once # call mailer when user locked + Mailer.expects(:deliver_security_notification).at_least_once # call mailer when user locked + + user.add_failed_login_attempts! + assert_equal 6, user.failed_login_attempts + assert user.locked? + assert Token.exists?(user: user, action: 'unlock') # token for unlock + end + end end