diff options
author | Rémy Coutable <remy@rymai.me> | 2016-10-04 15:04:57 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-10-05 16:41:42 +0200 |
commit | 9264816a34c45fb36c0117fe95bdf9e9af3287c8 (patch) | |
tree | ef1d493cce1d003b6f9f1d60985831188b917c49 | |
parent | 372c8a541bcc25a4c4bddfe75481f021f0e8d1de (diff) | |
download | gitlab-ce-9264816a34c45fb36c0117fe95bdf9e9af3287c8.tar.gz |
Merge branch 'restrict-failed-2fa-attempts' into 'master'
Restrict failed login attempts from users with 2FA enabled.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/19799.
See merge request !6668
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/concerns/authenticates_with_two_factor.rb | 15 | ||||
-rw-r--r-- | app/models/user.rb | 16 | ||||
-rw-r--r-- | spec/controllers/sessions_controller_spec.rb | 38 |
4 files changed, 68 insertions, 2 deletions
diff --git a/CHANGELOG b/CHANGELOG index 66d484d5855..6813fd3bdf8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -8,6 +8,7 @@ v 8.12.4 (unreleased) - Fix type mismatch bug when closing Jira issue. !6619 - Fix lint-doc error. !6623 - Skip wiki creation when GitHub project has wiki enabled. !6665 + - Restrict failed login attempts for users with 2FA enabled. !6668 v 8.12.3 - Update Gitlab Shell to support low IO priority for storage moves diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb index d5a8a962662..4c497711fc0 100644 --- a/app/controllers/concerns/authenticates_with_two_factor.rb +++ b/app/controllers/concerns/authenticates_with_two_factor.rb @@ -23,15 +23,24 @@ module AuthenticatesWithTwoFactor # # Returns nil def prompt_for_two_factor(user) + return locked_user_redirect(user) if user.access_locked? + session[:otp_user_id] = user.id setup_u2f_authentication(user) render 'devise/sessions/two_factor' end + def locked_user_redirect(user) + flash.now[:alert] = 'Invalid Login or password' + render 'devise/sessions/new' + end + def authenticate_with_two_factor user = self.resource = find_user - if user_params[:otp_attempt].present? && session[:otp_user_id] + if user.access_locked? + locked_user_redirect(user) + elsif user_params[:otp_attempt].present? && session[:otp_user_id] authenticate_with_two_factor_via_otp(user) elsif user_params[:device_response].present? && session[:otp_user_id] authenticate_with_two_factor_via_u2f(user) @@ -50,8 +59,9 @@ module AuthenticatesWithTwoFactor remember_me(user) if user_params[:remember_me] == '1' sign_in(user) else + user.increment_failed_attempts! flash.now[:alert] = 'Invalid two-factor code.' - render :two_factor + prompt_for_two_factor(user) end end @@ -65,6 +75,7 @@ module AuthenticatesWithTwoFactor remember_me(user) if user_params[:remember_me] == '1' sign_in(user) else + user.increment_failed_attempts! flash.now[:alert] = 'Authentication via U2F device failed.' prompt_for_two_factor(user) end diff --git a/app/models/user.rb b/app/models/user.rb index 6996740eebd..7f5a8562907 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -827,6 +827,22 @@ class User < ActiveRecord::Base todos_pending_count(force: true) end + # This is copied from Devise::Models::Lockable#valid_for_authentication?, as our auth + # flow means we don't call that automatically (and can't conveniently do so). + # + # See: + # <https://github.com/plataformatec/devise/blob/v4.0.0/lib/devise/models/lockable.rb#L92> + # + def increment_failed_attempts! + self.failed_attempts ||= 0 + self.failed_attempts += 1 + if attempts_exceeded? + lock_access! unless access_locked? + else + save(validate: false) + end + end + private def projects_union(min_access_level = nil) diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 8f27e616c3e..48d69377461 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -109,6 +109,44 @@ describe SessionsController do end end + context 'when the user is on their last attempt' do + before do + user.update(failed_attempts: User.maximum_attempts.pred) + end + + context 'when OTP is valid' do + it 'authenticates correctly' do + authenticate_2fa(otp_attempt: user.current_otp) + + expect(subject.current_user).to eq user + end + end + + context 'when OTP is invalid' do + before { authenticate_2fa(otp_attempt: 'invalid') } + + it 'does not authenticate' do + expect(subject.current_user).not_to eq user + end + + it 'warns about invalid login' do + expect(response).to set_flash.now[:alert] + .to /Invalid Login or password/ + end + + it 'locks the user' do + expect(user.reload).to be_access_locked + end + + it 'keeps the user locked on future login attempts' do + post(:create, user: { login: user.username, password: user.password }) + + expect(response) + .to set_flash.now[:alert].to /Invalid Login or password/ + end + end + end + context 'when another user does not have 2FA enabled' do let(:another_user) { create(:user) } |