Commit 194fbc3c authored by Sean McGivern's avatar Sean McGivern

Restrict failed login attempts for users with 2FA

Copy logic from `Devise::Models::Lockable#valid_for_authentication?`, as
our custom login flow with two pages doesn't call this method. This will
increment the failed login counter, and lock the user's account once
they exceed the number of failed attempts.

Also ensure that users who are locked can't continue to submit 2FA
codes.
parent 66613f1a
...@@ -42,11 +42,12 @@ v 8.13.0 (unreleased) ...@@ -42,11 +42,12 @@ v 8.13.0 (unreleased)
- Notify the Merger about merge after successful build (Dimitris Karakasilis) - Notify the Merger about merge after successful build (Dimitris Karakasilis)
- Fix broken repository 500 errors in project list - Fix broken repository 500 errors in project list
- Close todos when accepting merge requests via the API !6486 (tonygambone) - 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) - Add Container Registry on/off status to Admin Area !6638 (the-undefined)
v 8.12.4 (unreleased) v 8.12.4 (unreleased)
- Fix issues importing services via Import/Export - 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) - Fix "Copy to clipboard" tooltip to say "Copied!" when clipboard button is clicked. (lukehowell)
v 8.12.3 v 8.12.3
......
...@@ -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