Merge branch 'jej/fix-2fa-reset-with-group-sso' into 'master'

Redirect locked Group SSO users to SSO page instead of 500 error

See merge request gitlab-org/gitlab!20329

app/controllers/concerns/authenticates_with_two_factor.rb

@@ -21,21 +21,28 @@ module AuthenticatesWithTwoFactor
     # Set @user for Devise views
     @user = user # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
-    return locked_user_redirect(user) unless user.can?(:log_in)
+    return handle_locked_user(user) unless user.can?(:log_in)
 
     session[:otp_user_id] = user.id
     setup_u2f_authentication(user)
     render 'devise/sessions/two_factor'
   end
 
+  def handle_locked_user(user)
+    clear_two_factor_attempt!
+
+    locked_user_redirect(user)
+  end
+
   def locked_user_redirect(user)
-    flash.now[:alert] = _('Invalid Login or password')
+    flash.now[:alert] = locked_user_redirect_alert(user)
+
     render 'devise/sessions/new'
   end
 
   def authenticate_with_two_factor
     user = self.resource = find_user
-    return locked_user_redirect(user) unless user.can?(:log_in)
+    return handle_locked_user(user) unless user.can?(:log_in)
 
     if user_params[:otp_attempt].present? && session[:otp_user_id]
       authenticate_with_two_factor_via_otp(user)
@@ -48,6 +55,14 @@ module AuthenticatesWithTwoFactor
 
   private
 
+  def locked_user_redirect_alert(user)
+    user.access_locked? ? _('Your account is locked.') : _('Invalid Login or password')
+  end
+
+  def clear_two_factor_attempt!
+    session.delete(:otp_user_id)
+  end
+
   def authenticate_with_two_factor_via_otp(user)
     if valid_otp_attempt?(user)
       # Remove any lingering user data from login

ee/app/controllers/groups/omniauth_callbacks_controller.rb

@@ -69,6 +69,13 @@ class Groups::OmniauthCallbacksController < OmniauthCallbacksController
     super
   end
 
+  override :locked_user_redirect
+  def locked_user_redirect(user)
+    flash[:alert] = locked_user_redirect_alert(user)
+
+    redirect_to sso_group_saml_providers_path(@unauthenticated_group, token: @unauthenticated_group.saml_discovery_token)
+  end
+
   def store_active_saml_session
     Gitlab::Auth::GroupSaml::SsoEnforcer.new(@saml_provider).update_session
   end

ee/changelogs/unreleased/jej-fix-2fa-reset-with-group-sso.yml

+---
+title: Group SSO handles locked users gracefully instead of showing 500 error
+merge_request: 20329
+author:
+type: fixed

ee/spec/features/groups/saml_providers_spec.rb

@@ -259,6 +259,37 @@ describe 'SAML provider settings' do
         end
       end
 
+      context 'when user has access locked' do
+        before do
+          user.lock_access!
+          identity = create(:group_saml_identity, saml_provider: saml_provider, user: user)
+          mock_group_saml(uid: identity.extern_uid)
+        end
+
+        it 'warns user that their account is locked' do
+          visit sso_group_saml_providers_path(group)
+
+          click_link 'Sign in with Single Sign-On'
+
+          expect(page).to have_content('Your account is locked.')
+        end
+
+        context 'with 2FA' do
+          before do
+            user.update!(otp_required_for_login: true)
+          end
+
+          it 'warns user their account is locked' do
+            visit sso_group_saml_providers_path(group)
+
+            click_link 'Sign in with Single Sign-On'
+
+            expect(page).to have_content('Your account is locked.')
+            expect(current_path).to eq sso_group_saml_providers_path(group)
+          end
+        end
+      end
+
       context 'for a private group' do
         let(:group) { create(:group, :private) }
 

locale/gitlab.pot

@@ -21925,6 +21925,9 @@ msgstr ""
 msgid "Your account has been deactivated by your administrator. Please log back in to reactivate your account."
 msgstr ""
 
+msgid "Your account is locked."
+msgstr ""
+
 msgid "Your account uses dedicated credentials for the \"%{group_name}\" group and can only be updated through SSO."
 msgstr ""
 

spec/controllers/sessions_controller_spec.rb

@@ -394,8 +394,7 @@ describe SessionsController do
               end
 
               it 'warns about invalid login' do
-                expect(response).to set_flash.now[:alert]
-                  .to /Invalid Login or password/
+                expect(response).to set_flash.now[:alert].to /Your account is locked./
               end
 
               it 'locks the user' do
@@ -405,8 +404,7 @@ describe SessionsController do
               it 'keeps the user locked on future login attempts' do
                 post(:create, params: { user: { login: user.username, password: user.password } })
 
-                expect(response)
-                  .to set_flash.now[:alert].to /Invalid Login or password/
+                expect(response).to set_flash.now[:alert].to /Your account is locked./
               end
             end
           end