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

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) }