Properly fail LDAP logins if GitLab user not persisted
What does this MR do and why?
Previously if restricted e-mail domains (#366269) or password limit restrictions were set (#22399), newly-signed in LDAP users would see an obscure failure message:
Could not authenticate you from Ldapmain because "Undefined method `provider' for nil:nilclass".
This was failing in Gitlab::Auth::Ldap::Access.open
because user.ldap_identity
was nil
. The application settings would cause validations of newly-signed in LDAP users to fail, preventing the user from persisting to the database.
The OmniAuth callback controller attempted to detect this by calling auth_user.valid_sign_in?
, but this was failing because Gitlab::Auth::LdapUser#valid_sign_in?
did not properly check that the user entry was persisted to the database before it contacted the LDAP server.
To avoid this obscure error, we just need to swap the order of the check: if the user entry is valid and persisted, then we can safely contact the LDAP server. If the user entry is not valid, an "Access Denied for your LDAP account" message will be shown to the user, and a message in the application log will list the reason why it failed.
This doesn't actually fix the sign-in problem: that will be addressed in another merge request.
Screenshots or screen recordings
Before
After
With a log message:
2022-07-09T06:40:44.027Z: (LDAP) Error saving user uid=john0,ou=people,dc=example,dc=com (john0.doe@example.com): ["Email is not allowed for sign-up. Please use your regular email address. Check with your administrator."]
How to set up and validate locally
- Enable LDAP (
gdk config set openldap.enabled true
) gdk reconfigure
gdk restart rails
- In
/admin/application_settings
, set up sign-up restrictions:- Allow sign-up for non-ldap users for specific domains
- Example:
externals.com
*.externals.com
- Attempt to login with a new LDAP user (e.g.
john
andpassword
)
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.