Commit 9264816a authored by Rémy Coutable's avatar Rémy Coutable

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: default avatarRémy Coutable <remy@rymai.me>
parent 372c8a54
...@@ -8,6 +8,7 @@ v 8.12.4 (unreleased) ...@@ -8,6 +8,7 @@ v 8.12.4 (unreleased)
- Fix type mismatch bug when closing Jira issue. !6619 - Fix type mismatch bug when closing Jira issue. !6619
- Fix lint-doc error. !6623 - Fix lint-doc error. !6623
- Skip wiki creation when GitHub project has wiki enabled. !6665 - Skip wiki creation when GitHub project has wiki enabled. !6665
- Restrict failed login attempts for users with 2FA enabled. !6668
v 8.12.3 v 8.12.3
- Update Gitlab Shell to support low IO priority for storage moves - Update Gitlab Shell to support low IO priority for storage moves
......
...@@ -23,15 +23,24 @@ module AuthenticatesWithTwoFactor ...@@ -23,15 +23,24 @@ module AuthenticatesWithTwoFactor
# #
# Returns nil # Returns nil
def prompt_for_two_factor(user) def prompt_for_two_factor(user)
return locked_user_redirect(user) if user.access_locked?
session[:otp_user_id] = user.id session[:otp_user_id] = user.id
setup_u2f_authentication(user) setup_u2f_authentication(user)
render 'devise/sessions/two_factor' render 'devise/sessions/two_factor'
end end
def locked_user_redirect(user)
flash.now[:alert] = 'Invalid Login or password'
render 'devise/sessions/new'
end
def authenticate_with_two_factor def authenticate_with_two_factor
user = self.resource = find_user 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) authenticate_with_two_factor_via_otp(user)
elsif user_params[:device_response].present? && session[:otp_user_id] elsif user_params[:device_response].present? && session[:otp_user_id]
authenticate_with_two_factor_via_u2f(user) authenticate_with_two_factor_via_u2f(user)
...@@ -50,8 +59,9 @@ module AuthenticatesWithTwoFactor ...@@ -50,8 +59,9 @@ module AuthenticatesWithTwoFactor
remember_me(user) if user_params[:remember_me] == '1' remember_me(user) if user_params[:remember_me] == '1'
sign_in(user) sign_in(user)
else else
user.increment_failed_attempts!
flash.now[:alert] = 'Invalid two-factor code.' flash.now[:alert] = 'Invalid two-factor code.'
render :two_factor prompt_for_two_factor(user)
end end
end end
...@@ -65,6 +75,7 @@ module AuthenticatesWithTwoFactor ...@@ -65,6 +75,7 @@ module AuthenticatesWithTwoFactor
remember_me(user) if user_params[:remember_me] == '1' remember_me(user) if user_params[:remember_me] == '1'
sign_in(user) sign_in(user)
else else
user.increment_failed_attempts!
flash.now[:alert] = 'Authentication via U2F device failed.' flash.now[:alert] = 'Authentication via U2F device failed.'
prompt_for_two_factor(user) prompt_for_two_factor(user)
end end
......
...@@ -827,6 +827,22 @@ class User < ActiveRecord::Base ...@@ -827,6 +827,22 @@ class User < ActiveRecord::Base
todos_pending_count(force: true) todos_pending_count(force: true)
end 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 private
def projects_union(min_access_level = nil) def projects_union(min_access_level = nil)
......
...@@ -109,6 +109,44 @@ describe SessionsController do ...@@ -109,6 +109,44 @@ describe SessionsController do
end end
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 context 'when another user does not have 2FA enabled' do
let(:another_user) { create(:user) } let(:another_user) { create(:user) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment