Ensure LDAP filters are always parentheses-wrapped and run with &

[uberbrady]

We've gotten a few weird problems with LDAP filters under v6, and it mostly looks due to configuration quirks in how v5 handled filters versus how we do it in v6.

The way we used to assemble the filters before was:

• start with an opening (
• add the contents of the LDAP filter query, which we assumed started with &
• add a second-level (
• put in the auth query
• close the second-level with a )
• add the final close paren )

But under v5, we started to allow Filters that were wrapped with parentheses.

So we need to support filters that look like:

• &(cn=*)
• or (cn=*)

So what this does is assemble the query like this:

• (&, then the ensure-parentheses-wrapped-version of the filter, then the parentheses-wrapped auth query

So for the first example, a sample LDAP auth query might look like:

(&(&(cn=*))(uid=testuser))

I've tested that, and that is a valid lookup, and gets you the results you'd expect.

For the second example, what you'd end up with is:

(&(cn=*)(uid=testuser))

Caveats

1. This messes with LDAP which is scary. I've tested it but my brain is fried right now and I want to try again.
2. I don't like that if you use the 'correct' format that we have documented, you get 'punished' with a jankier-looking LDAP filter for lookup. I'm hoping the LDAP servers don't care and will optimize the jank out?
3. I still can't explain the uid=samaccountname auth query, at all. I can't figure out where this got introduced, and I can't figure out how it ever worked, and I can't replicate anything with it at all. It seems to be a filter that will always return zero results, at least, on AD. Very weird.

[uberbrady]

Okay, so for Caveat number 3, I now understand what's going on.

First off, 'where it got introduced' was actually at the database-level; uid=samaccountname was set as the column default for the ldap_auth_filter_query. It never was a 'placeholder' - it was a DB column default, and if you left the field blank, you would get that database default as your setting.

Second off, 'why did it work'

• During LDAP sync, the auth filter query wasn't used - which makes sense. The only 'auth' is of the Admin user, and that's done directly with their chosen username/password (no 'auth filter query' needed). After that, you just run an ldap_search().
• During LDAP login, under AD, the Auth Filter Query would be completely ignored during the 'Bind' portion of the login. This was true under v5 as well as v4, because the AD checkbox in both cases does not prepend anything to the username. Then, during the 'find the user in the directory' portion, v5's LDAP would do a 'search' based on the defined 'username' attribute - it was not using the Auth Filter Query to do it.

So I'm trying to think of a fix for that uid=samaccountname issue that gives us a little of the best of both worlds, but also looking forward towards #11041 which I think is going to really improve our LDAP situation. I'm thinking of potentially fixing it in code, or via migration, or perhaps both. Regardless of how I want to do that 'fix', this PR stands alone for me, as it improves our situation a little bit, without necessarily addressing the uid=samaccountname problem. I think I might handle that in a separate PR.

[uberbrady]

We went a different way that's way less dangerous than this.