smolnar82 commented on code in PR #1431:
URL: https://github.com/apache/knox/pull/1431#discussion_r4119304940
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -81,7 +81,15 @@ public EntryFilteringCursor search(SearchOperationContext
ctx) throws LdapExcept
if (!entries.isEmpty()) {
for (Entry entry : entries) {
try {
- final String username =
LdapUtils.extractUsernameFromEntry(entry, "uid", "cn");
+ // Key the role lookup on the stable uid, never a display
name. The entry's DN
+ // (uid=...,ou=people,...) is always present, whereas the
uid *attribute* is
+ // stripped when the LDAP client (e.g. Hadoop
LdapGroupsMapping) does not request
+ // it, leaving only cn and resolving the wrong roles.
Prefer the uid from the DN,
+ // falling back to entry attributes for non-uid-based DNs.
+ String username =
LdapUtils.extractUsernameFromDn(entry.getDn());
Review Comment:
Good catch, @handavid! You're right that the DN-first approach breaks on AD.
The AD DN is `CN=<display name>,…,` the real id is `sAMAccountName` (which
LdapProxyBackend maps onto `uid`), so keying off the RDN would resolve the
display name and look up the wrong roles.
I've dropped the DN-first change and gone with your suggestion: force `uid`
into the search's returning attributes, key the lookup on it, then strip it
back out of the entries when the client didn't ask for it. Restoring the
original returning attributes afterwards too, so downstream sees exactly what
it requested. A `*` (all-user-attributes) request or one that already names
`uid` is left untouched.
I also generalized the plumbing rather than hard-coding `uid`: there's now a
small interceptor-owned `REQUIRED_ATTRIBUTES` list, and the augment/strip logic
adds whichever of those the client omitted and strips exactly those back out
(per-attribute, using `ctx.contains(...)` so the `*`/`+`/`1.1` flags are
honored). It's deliberately a code constant, not topology config: an attribute
only earns a place there if code in the interceptor actually reads it, so the
augment set and its consumer can't drift apart. That makes the `objectClass`
case you mentioned (telling user vs group entries apart, and the overlap with
#1428) a one-line addition here plus the consuming logic, whenever we want to
tackle it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
