fix(ldap): use DN instead of uid in basicAuth GetLDAPUser call#1015
Open
tsushanth wants to merge 1 commit into
Open
fix(ldap): use DN instead of uid in basicAuth GetLDAPUser call#1015tsushanth wants to merge 1 commit into
tsushanth wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe LDAP BasicAuth path now retrieves user details using the username returned by the LDAP search. ChangesLDAP authentication
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
basicAuthandcookieAuthboth callSearchUserto resolve an incoming username, then callGetLDAPUserwith the result — but they pass different arguments:GetLDAPUsercookieAuth(LDAP branch, line 197)search.Username(the DN returned bySearchUser)basicAuth(LDAP branch, line 285)username(the raw uid from the HTTPAuthorizationheader)GetLDAPUserbuilds a group-membership query usinguniquemember=<value>. LDAPuniquememberattributes store full Distinguished Names (e.g.uid=alice,ou=users,dc=example,dc=com), not bare uids. Passing a raw uid produces zero matches, souser.Groupsis always empty for any LDAP user authenticating via Basic Auth. All group-based ACL rules are silently bypassed or denied depending on policy.cookieAuthhas the correct pattern: it usessearch.Username, which is the DN thatSearchUserreturns.basicAuthshould do the same.Fix
One-line change in
internal/middleware/context_middleware.go,basicAuthLDAP case:searchis already in scope — it is the*model.UserSearchreturned by theSearchUsercall earlier in the same function.Closes #848
Summary by CodeRabbit