smolnar82 commented on code in PR #1428:
URL: https://github.com/apache/knox/pull/1428#discussion_r4102842324
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/FilterMappingVisitor.java:
##########
@@ -68,6 +74,17 @@ private Object handleLeafNode(LeafNode leafNode) {
leafNode.setAttributeType(schemaManager.getAttributeType(userIdentifierAttribute));
}
+ // Map the dn-valued attributes from the proxy base dn to remote base
dn
+ if
(DN_VALUED_ATTRIBUTES.contains(currentAttribute.toLowerCase(Locale.ROOT))) {
Review Comment:
Possible NPE is `currentAttribute` is `null`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -401,6 +401,7 @@ public class GatewayConfigImpl extends Configuration
implements GatewayConfig {
public static final int DEFAULT_LDAP_MAX_SIZE_LIMIT = 1000;
/* The default max time for LDAP search in milliseconds */
public static final int DEFAULT_LDAP_MAX_TIME_LIMIT = 60 * 1000;
+ public static final boolean DEFAULT_LDAP_DN_MAPPING_ENABLED = true;
Review Comment:
This is just a comment to remember: flips returned-DN behavior for every
existing LDAP-proxy deployment on upgrade with no config change.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -78,21 +80,80 @@ public EntryFilteringCursor search(SearchOperationContext
ctx) throws LdapExcept
throw new LdapException(e);
}
- if (!entries.isEmpty()) {
- for (Entry entry : entries) {
- try {
+ final List<Entry> roleEntries = new ArrayList<>();
+ final List<Entry> resultEntries = new ArrayList<>(entries.size());
+ for (Entry entry : entries) {
+ try {
+ if (LdapUtils.isGroupEntry(entry)) {
+ roleEntries.addAll(translateGroupEntry(entry));
+ } else {
final String username =
LdapUtils.extractUsernameFromEntry(entry, "uid", "cn");
final Set<String> groups = fetchGroups(entry);
final Collection<String> roles =
rolesLookupService.lookupRoles(username, groups);
modifyEntry(entry, roles);
- } catch (Exception e) {
- LOG.ldapRolesLookupFailed("Error while updating entry with
roles lookup results", e);
- throw new LdapException(e);
+ resultEntries.add(entry);
+ }
+ } catch (Exception e) {
+ LOG.ldapRolesLookupFailed(entry.getDn().getName(), e);
+ throw new LdapException(e);
+ }
+ }
+ resultEntries.addAll(deduplicate(roleEntries));
+
+ return new EntryFilteringCursorImpl(new ListCursor<>(resultEntries),
ctx, ctx.getSession().getDirectoryService().getSchemaManager());
+ }
+
+ private Collection<? extends Entry> deduplicate(List<Entry> roleEntries)
throws LdapException {
+ Map<Dn, Entry> dedup = new HashMap<>();
+ for (Entry entry : roleEntries) {
+ Dn dn = entry.getDn();
+ if (!dedup.containsKey(dn)) {
+ dedup.put(dn, entry);
+ } else {
+ combineGroupEntry(dedup.get(dn), entry);
+ }
+ }
+ return dedup.values();
+ }
+
+ private void combineGroupEntry(Entry entry1, Entry entry2) throws
LdapException {
Review Comment:
```
Attribute entry1Member = entry1.get("member");
Attribute entry2Member = entry2.get("member");
if (entry1Member == null && entry2Member != null) { ... }
else if (entry1Member != null && entry2Member != null) { ... }
```
And `LdapUtils.isGroupEntry` (line 35) routes both `groupOfNames` and
`groupOfUniqueNames` into this path. For `groupOfUniqueNames`, membership lives
in `uniqueMember`, so both `get("member")` calls return null → neither branch
runs → the second group's members are gone. The collision is real:
`translateGroupEntry` renames each group's DN to `cn=<role>,<same parent>`
(line 147), so two groups mapping to one role produce the identical DN and hit
combineGroupEntry at line 113. Silent, no log, defeats the PR's own purpose.
--
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]
