Improved find-and-bind for complex LDAP directory structures

[uberbrady]

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_uname field and tested that login continued to work in the 'legacy' way as well.

(Also fixed some typos my IDE was complaining about)

[maxnoe]

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.

[uberbrady]

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 !!!

[snipe]

@maxnoe gentle ping for followup on @uberbrady's question! :)

[maxnoe]

Fine for me.