Project

General

Profile

Feature #3096 » login_attempts_v2.patch

Mizuki ISHIKAWA, 2023-08-28 04:56

View differences:

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
(3-3/3)