-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix LDAP login for dn which is not uid=${username},${base} #14137
base: master
Are you sure you want to change the base?
Conversation
💖 Thanks for this pull request! 💖 We use semantic commit messages to streamline the release process and easily generate changelogs between versions. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix if it doesn't have one already. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
PR Summary
|
|
||
// FIND the user first, then attempt to log in as them using the appropriate attribute | ||
$filterQuery = $settings->ldap_auth_filter_query.$username; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of concatenating the $username
, I think it would be better to provide a string template in the config and then inserting the username into the template, this would allow ldap_auth_filter_query
like:
| (uid=$username) (mail=$username)
which would enable people to loin with either uid or email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also enable using only a single config field for what is now "ldap_filter" and "ldap_auth_filter_query"
Flow is now: * Query users using the provided username to get dn * Bind using that received DN
thanks, this fixes my issue with LDAP bind |
Description
Fixes #9903, can be used as a basis to potentially fix more LDAP issues.
The existing code assumes that a user's dn (distinguished name) is build from their uid like so:
which is not the case for our LDAP setup. This assumption is completely unnecessary to make, the correct solution is to:
This also allows for example login using the email using a filter like:
Which would require a change in the ldap options, so I didn't do that for now.
Note: I removed for now the special cases for active directory. I think these are not necessary with the fixed flow, but I have no AD I can use to test.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Manually tested using using local LDAP server.
Checklist: