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
This commit is contained in:
commit
b8005b6112
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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) }
|
||||
|
||||
|
|
Loading…
Reference in New Issue