Commit 695a5a58 authored by James Edwards-Jones's avatar James Edwards-Jones

Avoid 500 error for locked users on Group SSO

What: Redirects locked users to the SSO page instead of generic sign
in when accessed via Group SAML.

This avoids a 500 error caused by attempting to access a missing
captcha_enabled? method that is not present in the
OmniauthCallbacksController, and instead displays an account locked
flash message.

Changes `locked_user_redirect` to display more accurate message
when a user's account is locked.

We also clear `session[otp_user_id]` to avoid future locked messages
from assuming we are still trying to log in the previous user.

Why: Users were getting a 500 error after incorrectly entering a 2FA
code many times.
parent a8c080b4
......@@ -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
......
......@@ -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
......
---
title: Group SSO handles locked users gracefully instead of showing 500 error
merge_request: 20329
author:
type: fixed
......@@ -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) }
......
......@@ -21835,6 +21835,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 ""
......
......@@ -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
......
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