Skip to content

Properly fail LDAP logins if GitLab user not persisted

Stan Hu requested to merge sh-fix-ldapmain-nil-login-errors into master

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

image

After

image

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

  1. Enable LDAP (gdk config set openldap.enabled true)
  2. gdk reconfigure
  3. gdk restart rails
  4. In /admin/application_settings, set up sign-up restrictions:
    • Allow sign-up for non-ldap users for specific domains
    • Example:
      • externals.com
      • *.externals.com
  5. Attempt to login with a new LDAP user (e.g. john and password)

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Stan Hu

Merge request reports

Loading