Improved find-and-bind for complex LDAP directory structures#17832
Improved find-and-bind for complex LDAP directory structures#17832uberbrady wants to merge 369 commits intogrokability:developfrom
Conversation
| Log::debug('Filter query: '.$filterQuery); | ||
|
|
||
| // only try this if we have an Admin username set; otherwise use the 'legacy' method | ||
| if ($settings->ldap_uname) { |
There was a problem hiding this comment.
Why do you allow the correct method only for the case with a system bind dn and not with anonymous access to the LDAP?
This seems orthogonal to me.
There was a problem hiding this comment.
My logic here was to try to minimize as much as possible the disruption for people who have working systems, today. I want this change to be as low-impact as possible, and mostly "additive".
If it is actually a thing that comes up where the anonymous bind user has more privileges than the individual user, then we would probably want to make a setting or .env that will override this check.
What I'm desperately trying to avoid is a situation where someone has a working install, runs an update, and their install is broken. So I'm being conservative - and possibly too much so; that definitely could happen.
Is this a situation you are in, or one you know of? I'm happy to make changes if we can implement them very, very carefully.
Thank you for following up @maxnoe !!!
There was a problem hiding this comment.
@maxnoe gentle ping for followup on @uberbrady's question! :)
…bel-to-asset-view Added status label to asset view
…icenses Show inactive licenses
…cost-to-unit Rewords Purchase cost to Unit Cost for Accessories, Components, Consumables
…orm-digit-separator Fixed grokability#17204 - replace Form::digit_separator macro
# Conflicts: # resources/macros/macros.php
…d-time-display-macros Removed date and time display format form macros
…earch Normalize advanced search
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3 to 4. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v3...v4) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: '4' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
|
updates on this as potential fix for this github.com//issues/17917 ? |
…ate-files Added migration to move asset model private uploads
…ub_actions/develop/github/codeql-action-4 Bump github/codeql-action from 3 to 4
…ories-suppliers-manufacturers
…bulk-deletion-of-asset-categories-suppliers-manufacturers Fixed grokability#8709 - Bulk Deletion of Categories, Suppliers, Manufa...
…r-notification Fixed bulk error display to be more consistent and correct HTML
…g-for-bulk-deletes Fixed grokability#18034 - Shows success message on partial bulk delete success
Added missing IF statements for address, city, state, and ZIP code. Previously, this information would be pulled via LDAP sync but would not update the user data due to the missing IF statements for these attributes.
Fixed LDAP sync not syncing all user fields
|
This looks great - can you please address those failing tests though, @uberbrady? I know they're related to the mocking we have to do for unit tests, but I can't merge it until we're all green. |
|
I have absolutely zero clue how I managed to donk up this branch/PR as hard as I did, but I definitely did do that. I'm going to close this one out in favor of the fixed one. But the references here within this branch to the other branches are still valid. |
11 participants
This is, unfortunately, the cleanest way I can figure out for how to change the ordering of our bind/fetch user attributes sequence. I tried a couple of different ways of doing it without making as much change to the code, but it just made reading everything too hard and was even more confusing.
Instead, I split out the new way of doing it into its own method, and call that method from the original
findAndBindUserLdap()method. If anything goes wrong there (including just a regular failed login), we fall back to the 'old way' of doing things. Also, if there is no LDAP Bind Username set, we just fall straight back to the legacy way.This also has some interesting side-effects. It means that the binding user no longer needs to have permission to search their own attributes, or to search for themselves. They do still need to actually be able to bind, but that was already the case before.
In trying to mentally model where this might break things, that should only happen if the LDAP Admin User has some kind of restricted permissions to search the directory - which certainly would've prevented the ability to do LDAP syncs anyways. But if someone was misconfigured and just "suffering" with that, this could break their login flow. At which point I would probably recommend that they delete their LDAP Admin username, which will get the legacy flow working again.
Edit - I should mention that I tested this where I was authenticating in the 'new' way, and then was able to blank out my
ldap_unamefield and tested that login continued to work in the 'legacy' way as well.(Also fixed some typos my IDE was complaining about)