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

WIP: Trying to make LDAP login search subtrees for accounts to log into #11041

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

uberbrady
Copy link
Collaborator

Possible fix for #9903 - allowing logins to access the entire subtree.

Since we already do a query to grab the attributes for the user in question, but we do so after we have bound as that user, we can flip that around, and do a search first, and then do the 'bind', as that found user, and that should work.

As I was thinking about it, I figured that I might be able to do this as just one change that modifies how the current system works. But I think it's going to break too many people - way too many - and is going to set our helpdesk (and GitHub) on fire. So I'm probably going to have to add that "Classic-vs-Subtree" mode-switch in there somewhere, anyways. I hate that, because I hate to hold on to that bit of complexity - but I also don't want to break existing, working configs.

But, regardless, if you want to see what the subtree logic might look like, this would be it.

Lots of TODO and FIXME and tons of debugging that I should rip out, and definitely a few unanswered questions that I should probably handle, too. But, this is what I got so far. Just would love some eyes on this to see if it possibly handles some use cases that are out there.

app/Models/Ldap.php Outdated Show resolved Hide resolved
@uberbrady
Copy link
Collaborator Author

Ok, I've now added a setting - default-on - that will append the "," and the baseDN - which is classically how Snipe-IT has worked in the past. Anyone who's doing this subtree login business will probably want to turn that off, I figure?

@snipe snipe added 🆘 testers-needed This is a feature/bugfix that has been completed but needs testing. ldap labels May 10, 2022
@uberbrady
Copy link
Collaborator Author

I should make sure to take into considerations the changes in #11303 - that affects some LDAP stuff and I want to make sure this reflects those. Hopefully they'll come up in the conflict-hell that I expect will result.

@snipe
Copy link
Owner

snipe commented Dec 15, 2022

Are we still going in this direction? Should we merge or close?

@uberbrady
Copy link
Collaborator Author

Yes, this is still a going concern and still affects some of our users. I'll see if there's some way to get it out of WIP.

@uberbrady
Copy link
Collaborator Author

This comment is really interesting: #9903 (comment) - and I am wondering if we can just do that small change and leave out the other stuff - like, would that work for everyone, or is that just for OpenLDAP people? Maybe I'm being too over-cautious here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend frontend ldap 🆘 testers-needed This is a feature/bugfix that has been completed but needs testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants