Feature #3096 » login_attempts_v2.patch
app/controllers/account_controller.rb | ||
---|---|---|
88 | 88 |
@user.must_change_passwd = false |
89 | 89 |
if @user.save |
90 | 90 |
@token.destroy |
91 |
@user.reset_failed_login_attempts! |
|
91 | 92 |
Mailer.deliver_password_updated(@user, User.current) |
92 | 93 |
flash[:notice] = l(:notice_account_password_updated) |
93 | 94 |
redirect_to signin_path |
... | ... | |
186 | 187 |
redirect_to signin_path |
187 | 188 |
end |
188 | 189 | |
190 |
# Token based account unlock |
|
191 |
def unlock |
|
192 |
token = Token.find_token('unlock', params[:token].to_s) |
|
193 |
(redirect_to(home_url); return) unless token and !token.expired? |
|
194 |
user = token.user |
|
195 |
(redirect_to(home_url); return) unless user.locked? |
|
196 |
user.reset_failed_login_attempts! |
|
197 |
if user.save |
|
198 |
token.destroy |
|
199 |
flash[:notice] = l(:notice_account_unlocked) |
|
200 |
end |
|
201 |
redirect_to signin_path |
|
202 |
end |
|
203 | ||
189 | 204 |
# Sends a new account activation email |
190 | 205 |
def activation_email |
191 | 206 |
if session[:registered_user_id] && Setting.self_registration == '1' |
... | ... | |
229 | 244 |
# prevent brute force attacks on the one-time password |
230 | 245 |
elsif session[:twofa_tries_counter] > 3 |
231 | 246 |
destroy_twofa_session |
247 |
@user.add_failed_login_attempts! |
|
248 |
account_locked(@user) and return if @user.locked? |
|
249 | ||
232 | 250 |
flash[:error] = l('twofa_too_many_tries') |
233 | 251 |
redirect_to home_url |
234 | 252 |
else |
253 |
@user.add_failed_login_attempts! |
|
254 |
account_locked(@user) and return if @user.locked? |
|
255 | ||
235 | 256 |
flash[:error] = l('twofa_invalid_code') |
236 | 257 |
redirect_to account_twofa_confirm_path |
237 | 258 |
end |
... | ... | |
330 | 351 |
end |
331 | 352 | |
332 | 353 |
def successful_authentication(user) |
354 |
user.reset_failed_login_attempts! |
|
355 | ||
333 | 356 |
logger.info "Successful authentication for '#{user.login}' from #{request.remote_ip} at #{Time.now.utc}" |
334 | 357 |
# Valid user |
335 | 358 |
self.logged_user = user |
... | ... | |
366 | 389 |
end |
367 | 390 | |
368 | 391 |
def invalid_credentials |
392 |
user = User.find_by_login(params[:username].to_s.strip) |
|
393 |
user.add_failed_login_attempts! if user |
|
394 |
account_locked(user) and return if user && user.locked? |
|
395 | ||
369 | 396 |
logger.warn "Failed login for '#{params[:username]}' from #{request.remote_ip} at #{Time.now.utc}" |
370 | 397 |
flash.now[:error] = l(:notice_account_invalid_credentials) |
371 | 398 |
end |
app/controllers/users_controller.rb | ||
---|---|---|
168 | 168 |
was_activated = (@user.status_change == [User::STATUS_REGISTERED, User::STATUS_ACTIVE]) |
169 | 169 |
# TODO: Similar to My#account |
170 | 170 |
@user.pref.safe_attributes = params[:pref] |
171 |
# Reset failed_login_attempts if changed from other status to active |
|
172 |
@user.failed_login_attempts = 0 if @user.will_save_change_to_status? && @user.active? |
|
171 | 173 | |
172 | 174 |
if @user.save |
173 | 175 |
@user.pref.save |
app/models/mailer.rb | ||
---|---|---|
459 | 459 |
register(user, token).deliver_later |
460 | 460 |
end |
461 | 461 | |
462 |
# Builds a mail to user with account unlock link. |
|
463 |
def locked(user, token) |
|
464 |
@token = token |
|
465 |
@url = url_for(controller: 'account', action: 'unlock', token: token.value) |
|
466 |
mail :to => user.mail, |
|
467 |
:subject => l(:mail_subject_locked, Setting.app_title) |
|
468 |
end |
|
469 | ||
470 |
# Sends an mail to user with account unlock link. |
|
471 |
# |
|
472 |
# Exemple: |
|
473 |
# Mailer.deliver_locked(user, token) |
|
474 |
def self.deliver_locked(user, token) |
|
475 |
locked(user, token).deliver_later |
|
476 |
end |
|
477 | ||
462 | 478 |
# Build a mail to user and the additional recipients given in |
463 | 479 |
# options[:recipients] about a security related event made by sender. |
464 | 480 |
# |
app/models/token.rb | ||
---|---|---|
41 | 41 |
add_action :feeds, max_instances: 1, validity_time: nil |
42 | 42 |
add_action :recovery, max_instances: 1, validity_time: Proc.new {Token.validity_time} |
43 | 43 |
add_action :register, max_instances: 1, validity_time: Proc.new {Token.validity_time} |
44 |
add_action :unlock, max_instances: 1, validity_time: Proc.new {Token.validity_time} |
|
44 | 45 |
add_action :session, max_instances: 10, validity_time: nil |
45 | 46 |
add_action :twofa_backup_code, max_instances: 10, validity_time: nil |
46 | 47 |
app/models/user.rb | ||
---|---|---|
284 | 284 |
end |
285 | 285 | |
286 | 286 |
def activate |
287 |
self.failed_login_attempts = 0 |
|
287 | 288 |
self.status = STATUS_ACTIVE |
288 | 289 |
end |
289 | 290 | |
... | ... | |
296 | 297 |
end |
297 | 298 | |
298 | 299 |
def activate! |
300 |
self.failed_login_attempts = 0 |
|
299 | 301 |
update_attribute(:status, STATUS_ACTIVE) |
300 | 302 |
end |
301 | 303 | |
... | ... | |
897 | 899 |
User.where("created_on < ? AND status = ?", Time.now - age, STATUS_REGISTERED).destroy_all |
898 | 900 |
end |
899 | 901 | |
902 |
def reset_failed_login_attempts! |
|
903 |
return if failed_login_attempts.zero? && self.active? |
|
904 | ||
905 |
self.failed_login_attempts = 0 |
|
906 |
self.activate if self.locked? |
|
907 |
self.save! |
|
908 |
end |
|
909 | ||
910 |
def add_failed_login_attempts! |
|
911 |
self.failed_login_attempts = self.failed_login_attempts + 1 |
|
912 | ||
913 |
if Setting.max_login_attempts.present? && self.failed_login_attempts > Setting.max_login_attempts.to_i && self.active? |
|
914 |
self.lock_by_failed_login_attempts! |
|
915 |
else |
|
916 |
self.save! |
|
917 |
end |
|
918 |
end |
|
919 | ||
920 |
# Execute in the event of consecutive failed login attempts. |
|
921 |
# Lock user, create unlock token, send email to locked user and send security notification emails to administrators. |
|
922 |
def lock_by_failed_login_attempts! |
|
923 |
self.lock |
|
924 |
token = Token.new(user: self, action: 'unlock') |
|
925 |
self.save! and token.save! |
|
926 |
Mailer.deliver_locked(self, token) |
|
927 | ||
928 |
options = { |
|
929 |
field: User.current.admin? ? :field_admin : :field_user, |
|
930 |
value: login, |
|
931 |
title: :label_user_plural, |
|
932 |
url: {controller: 'users', action: 'index', status: STATUS_LOCKED}, |
|
933 |
message: :mail_body_security_notification_locked |
|
934 |
} |
|
935 |
users = User.active.where(admin: true).to_a |
|
936 |
Mailer.deliver_security_notification(users, User.current, options) |
|
937 |
end |
|
938 | ||
900 | 939 |
protected |
901 | 940 | |
902 | 941 |
def validate_password_length |
app/views/mailer/locked.html.erb | ||
---|---|---|
1 |
<p><%= simple_format(l(:mail_body_locked)) %><br /> |
|
2 |
<%= link_to @url, @url %></p> |
app/views/mailer/locked.text.erb | ||
---|---|---|
1 |
<%= l(:mail_body_locked) %> |
|
2 |
<%= @url %> |
app/views/settings/_authentication.html.erb | ||
---|---|---|
41 | 41 |
</em> |
42 | 42 |
</p> |
43 | 43 | |
44 |
<p> |
|
45 |
<%= setting_text_field :max_login_attempts, :size => 6 %> |
|
46 |
</p> |
|
44 | 47 |
</div> |
45 | 48 | |
46 | 49 |
<fieldset class="box"> |
config/locales/en.yml | ||
---|---|---|
159 | 159 |
notice_account_register_done: Account was successfully created. An email containing the instructions to activate your account was sent to %{email}. |
160 | 160 |
notice_account_not_activated_yet: You haven't activated your account yet. If you want to receive a new activation email, please <a href="%{url}">click this link</a>. |
161 | 161 |
notice_account_locked: Your account is locked. |
162 |
notice_account_unlocked: Your account is unlocked. |
|
162 | 163 |
notice_can_t_change_password: This account uses an external authentication source. Impossible to change the password. |
163 | 164 |
notice_account_lost_email_sent: An email with instructions to choose a new password has been sent to you. |
164 | 165 |
notice_account_activated: Your account has been activated. You can now log in. |
... | ... | |
251 | 252 |
mail_body_lost_password_validity: 'Please be aware that you may change the password only once using this link.' |
252 | 253 |
mail_subject_register: "Your %{value} account activation" |
253 | 254 |
mail_body_register: 'To activate your account, click on the following link:' |
255 |
mail_subject_locked: "Your %{value} account locked" |
|
256 |
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:" |
|
254 | 257 |
mail_body_account_information_external: "You can use your %{value} account to log in." |
255 | 258 |
mail_body_account_information: Your account information |
256 | 259 |
mail_subject_account_activation_request: "%{value} account activation request" |
... | ... | |
266 | 269 |
mail_body_security_notification_change_to: "%{field} was changed to %{value}." |
267 | 270 |
mail_body_security_notification_add: "%{field} %{value} was added." |
268 | 271 |
mail_body_security_notification_remove: "%{field} %{value} was removed." |
272 |
mail_body_security_notification_locked: "%{field} %{value} was locked." |
|
269 | 273 |
mail_body_security_notification_notify_enabled: "Email address %{value} now receives notifications." |
270 | 274 |
mail_body_security_notification_notify_disabled: "Email address %{value} no longer receives notifications." |
271 | 275 |
mail_body_settings_updated: "The following settings were changed:" |
... | ... | |
471 | 475 |
setting_password_min_length: Minimum password length |
472 | 476 |
setting_password_required_char_classes : Required character classes for passwords |
473 | 477 |
setting_lost_password: Allow password reset via email |
478 |
setting_max_login_attempts: Max login attempts |
|
474 | 479 |
setting_new_project_user_role_id: Role given to a non-admin user who creates a project |
475 | 480 |
setting_default_projects_modules: Default enabled modules for new projects |
476 | 481 |
setting_issue_done_ratio: Calculate the issue done ratio with |
config/routes.rb | ||
---|---|---|
28 | 28 |
match 'account/register', :to => 'account#register', :via => [:get, :post], :as => 'register' |
29 | 29 |
match 'account/lost_password', :to => 'account#lost_password', :via => [:get, :post], :as => 'lost_password' |
30 | 30 |
match 'account/activate', :to => 'account#activate', :via => :get |
31 |
match 'account/unlock', :to => 'account#unlock', :via => :get |
|
31 | 32 |
get 'account/activation_email', :to => 'account#activation_email', :as => 'activation_email' |
32 | 33 | |
33 | 34 |
match '/news/preview', :controller => 'previews', :action => 'news', :as => 'preview_news', :via => [:get, :post, :put, :patch] |
config/settings.yml | ||
---|---|---|
52 | 52 |
format: int |
53 | 53 |
default: 0 |
54 | 54 |
security_notifications: 1 |
55 |
max_login_attempts: |
|
56 |
format: int |
|
57 |
default: |
|
58 |
security_notifications: 1 |
|
55 | 59 |
# Maximum number of additional email addresses per user |
56 | 60 |
max_additional_emails: |
57 | 61 |
format: int |
db/migrate/20230808010454_add_failed_login_attempts_to_users.rb | ||
---|---|---|
1 |
class AddFailedLoginAttemptsToUsers < ActiveRecord::Migration[6.1] |
|
2 |
def change |
|
3 |
add_column :users, :failed_login_attempts, :integer, default: 0, null: false |
|
4 |
end |
|
5 |
end |
test/functional/account_controller_test.rb | ||
---|---|---|
21 | 21 | |
22 | 22 |
class AccountControllerTest < Redmine::ControllerTest |
23 | 23 |
fixtures :users, :email_addresses, :roles |
24 |
include Redmine::I18n |
|
24 | 25 | |
25 | 26 |
def setup |
26 | 27 |
User.current = nil |
... | ... | |
172 | 173 |
assert_select 'input[name=username][value=admin]' |
173 | 174 |
assert_select 'input[name=password]' |
174 | 175 |
assert_select 'input[name=password][value]', 0 |
176 |
assert_equal 1, User.find(1).failed_login_attempts |
|
177 |
end |
|
178 | ||
179 |
def test_login_should_reset_failed_login_attempts_when_password_is_correct |
|
180 |
user = User.find(1) |
|
181 |
user.update!(failed_login_attempts: 5, status: User::STATUS_ACTIVE) |
|
182 | ||
183 |
with_settings max_login_attempts: 5 do |
|
184 |
post( |
|
185 |
:login, |
|
186 |
:params => { |
|
187 |
:username => 'admin', |
|
188 |
:password => 'admin' |
|
189 |
} |
|
190 |
) |
|
191 |
assert_equal 0, user.reload.failed_login_attempts |
|
192 |
assert user.active? |
|
193 |
end |
|
175 | 194 |
end |
176 | 195 | |
177 | 196 |
def test_login_with_locked_account_should_fail |
... | ... | |
180 | 199 |
:login, |
181 | 200 |
:params => { |
182 | 201 |
:username => 'jsmith', |
183 |
:password => 'jsmith'
|
|
202 |
:password => 'bad'
|
|
184 | 203 |
} |
185 | 204 |
) |
186 | 205 |
assert_redirected_to '/login' |
... | ... | |
638 | 657 |
assert_redirected_to '/' |
639 | 658 |
end |
640 | 659 | |
660 |
def test_unlock_should_unlock_user_with_valid_token |
|
661 |
user = User.find(1) |
|
662 |
user.lock |
|
663 |
token = Token.new(user: user, action: 'unlock') |
|
664 |
user.save! and token.save! |
|
665 | ||
666 |
get :unlock, params: { token: token.value } |
|
667 |
assert_redirected_to signin_path |
|
668 |
assert_equal l(:notice_account_unlocked), flash[:notice] |
|
669 |
assert_not user.reload.locked? |
|
670 |
assert_nil Token.find_by(id: token.id) |
|
671 |
end |
|
672 | ||
673 |
def test_unlock_should_redirect_to_home_when_token_is_not_found |
|
674 |
user = User.find(1) |
|
675 |
user.lock |
|
676 |
token = Token.new(user: user, action: 'unlock') |
|
677 |
user.save! and token.save! |
|
678 | ||
679 |
get :unlock, params: { token: 'invalid_token' } |
|
680 |
assert_redirected_to home_url |
|
681 |
end |
|
682 | ||
683 |
def test_unlock_should_redirect_to_home_when_token_is_expired |
|
684 |
user = User.find(1) |
|
685 |
user.lock |
|
686 |
token = Token.new(user: user, action: 'unlock', created_on: 2.days.ago) |
|
687 |
user.save! and token.save! |
|
688 | ||
689 |
get :unlock, params: { token: token.value } |
|
690 |
assert_redirected_to home_url |
|
691 |
end |
|
692 | ||
641 | 693 |
def test_activation_email_should_send_an_activation_email |
642 | 694 |
User.find(2).update_attribute :status, User::STATUS_REGISTERED |
643 | 695 |
@request.session[:registered_user_id] = 2 |
test/unit/user_test.rb | ||
---|---|---|
1358 | 1358 |
User.prune(7) |
1359 | 1359 |
end |
1360 | 1360 |
end |
1361 | ||
1362 |
def test_reset_failed_login_attempts_should_set_failed_login_attempts_to_zero_and_activate |
|
1363 |
user = User.find(1) |
|
1364 |
user.update!(failed_login_attempts: 10, status: User::STATUS_LOCKED) |
|
1365 | ||
1366 |
user.reset_failed_login_attempts! |
|
1367 |
assert_equal 0, user.reload.failed_login_attempts |
|
1368 |
assert user.active? |
|
1369 |
end |
|
1370 | ||
1371 |
def test_add_failed_login_attempts_should_increment_failed_attempts_by_one |
|
1372 |
user = User.find(1) |
|
1373 |
user.update!(failed_login_attempts: 1) |
|
1374 | ||
1375 |
with_settings max_login_attempts: 5 do |
|
1376 |
user.add_failed_login_attempts! |
|
1377 |
assert_equal 2, user.failed_login_attempts |
|
1378 |
assert user.active? |
|
1379 |
end |
|
1380 |
end |
|
1381 | ||
1382 |
def test_add_failed_login_attempts_should_lock_user_after_max_attempts |
|
1383 |
user = User.find(1) |
|
1384 |
user.update!(failed_login_attempts: 5) |
|
1385 | ||
1386 |
with_settings max_login_attempts: 5 do |
|
1387 |
Mailer.expects(:deliver_locked).at_least_once # call mailer when user locked |
|
1388 |
Mailer.expects(:deliver_security_notification).at_least_once # call mailer when user locked |
|
1389 | ||
1390 |
user.add_failed_login_attempts! |
|
1391 |
assert_equal 6, user.failed_login_attempts |
|
1392 |
assert user.locked? |
|
1393 |
assert Token.exists?(user: user, action: 'unlock') # token for unlock |
|
1394 |
end |
|
1395 |
end |
|
1361 | 1396 |
end |
- « Previous
- 1
- 2
- 3
- Next »