summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2016-10-04 15:04:57 +0000
committerRémy Coutable <remy@rymai.me>2016-10-04 15:04:57 +0000
commitb8005b6112d7322ff8b2cf0a1e55e6c56f0fcba3 (patch)
treea802cabde9fe3fe9efe4a25cc09e7360399b27b8
parent385817a11f568ca8fa165eaf57fa88789fc6fcd5 (diff)
parent194fbc3c3d4b068f191fca75488b986df88c5333 (diff)
downloadgitlab-ce-b8005b6112d7322ff8b2cf0a1e55e6c56f0fcba3.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
-rw-r--r--CHANGELOG3
-rw-r--r--app/controllers/concerns/authenticates_with_two_factor.rb15
-rw-r--r--app/models/user.rb16
-rw-r--r--spec/controllers/sessions_controller_spec.rb38
4 files changed, 69 insertions, 3 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 895ec17a678..dd3b15f513b 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -45,12 +45,13 @@ v 8.13.0 (unreleased)
- Notify the Merger about merge after successful build (Dimitris Karakasilis)
- Fix broken repository 500 errors in project list
- Close todos when accepting merge requests via the API !6486 (tonygambone)
- - Changed Slack service user referencing from full name to username (Sebastian Poxhofer)
+ - Changed Slack service user referencing from full name to username (Sebastian Poxhofer)
- Add Container Registry on/off status to Admin Area !6638 (the-undefined)
v 8.12.4 (unreleased)
- Fix type mismatch bug when closing Jira issue
- Fix issues importing services via Import/Export
+ - Restrict failed login attempts for users with 2FA enabled
- Fix "Copy to clipboard" tooltip to say "Copied!" when clipboard button is clicked. (lukehowell)
v 8.12.3
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) }