Skip to content
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

Make AD configurations ignore ldap_auth_filter_query #11319

Closed
wants to merge 1 commit into from

Conversation

uberbrady
Copy link
Collaborator

As mentioned in #11303 , we do have many AD users with grossly-incorrect LDAP Auth Filter Query settings. This is partially our fault, as we had a rather yucky (and incorrect) database default set for that field.

And Snipe-IT v6 uses a more-strict syntax for how it handles LDAP Auth Filter Query settings. We've seen plenty of upgrading users have problems with LDAP logins right here on GitHub as well. While those are quite solvable with some simple settings changes - and the users' current settings are quite incorrect and don't match our documentation - we should still try to reduce the amount of pain our users have to go through.

So, in Snipe-IT v5 (AdLdap2), once you dig through the code enough, you realize that the LDAP Auth Filter Query is completely ignored. This PR purports to revert to the previous behavior, mimicking how we did AD-based LDAP login in v5 - e.g. re-ignoring the LDAP Auth Filter Query setting.

My logic on why this would be a good change is that it worked fine under v5, and I don't think we hit any limitations where users weren't able to do what they needed to do, except in more complex 'forest' environments that we never were able to support anyways. And it, of course, will reduce the amount of pain our users have to go through during upgrades.

My concern, however, is that it will mess up things like #11041 , which I think should probably be the direction that Snipe-IT LDAP takes in the future. I'm still not sure, though, if this will limit our ability to do that. That being said, I haven't seen an AD setup that does anything other than plain binding as either a sAMAccountName or userPrinicpalName - so I can't see why it wouldn't work.

I'm going to think about this a little bit more before I decide to take this out of Draft.

@uberbrady
Copy link
Collaborator Author

Having just worked with a customer who was still on v5, I think we shouldn't do this. The customer noted that the filter did work on the ldap sync part, but users who did not match the filter were still able to log in and be created. That's not an intuitive way for this system to work, so I think we will need to just endure the pain during upgrades. I don't think I could be comfortable with dynamically modifying people's Auth filters either. So I'm going to close this out.

@uberbrady uberbrady closed this Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant