handavid commented on code in PR #1454:
URL: https://github.com/apache/knox/pull/1454#discussion_r4322445212


##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -86,6 +96,90 @@ public EntryFilteringCursor search(SearchOperationContext 
ctx) throws LdapExcept
             }
         }
 
+        if (isAllGroupsSearch(ctx)) {
+            return handleAllGroupsSearch(ctx);
+        } else {
+            return handleSearch(ctx);
+        }
+    }
+
+    private boolean isAllGroupsSearch(SearchOperationContext ctx) {
+        Dn searchDn = ctx.getDn();
+        if (searchDn == null) {
+            return false;
+        }
+
+        if (ctx.getScope() == SearchScope.OBJECT || 
!isAllGroupsFilter(ctx.getFilter())) {
+            return false;
+        }
+        // Either the proxy groups base, or the root with an explicit group 
filter
+        return searchDn.toString().equalsIgnoreCase(proxyGroupSearchBase) || 
searchDn.size() <= 2;

Review Comment:
   fixed to use separate criteria for the group search base and ancestor DNs.



##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -86,6 +96,90 @@ public EntryFilteringCursor search(SearchOperationContext 
ctx) throws LdapExcept
             }
         }
 
+        if (isAllGroupsSearch(ctx)) {
+            return handleAllGroupsSearch(ctx);
+        } else {
+            return handleSearch(ctx);
+        }
+    }
+
+    private boolean isAllGroupsSearch(SearchOperationContext ctx) {
+        Dn searchDn = ctx.getDn();
+        if (searchDn == null) {
+            return false;
+        }
+
+        if (ctx.getScope() == SearchScope.OBJECT || 
!isAllGroupsFilter(ctx.getFilter())) {
+            return false;
+        }
+        // Either the proxy groups base, or the root with an explicit group 
filter
+        return searchDn.toString().equalsIgnoreCase(proxyGroupSearchBase) || 
searchDn.size() <= 2;
+    }
+
+    private boolean isAllGroupsFilter(ExprNode filter) {
+        // Case 1: Simple objectClass evaluation (e.g., 
objectClass=groupOfNames)
+        if (filter instanceof EqualityNode) {
+            EqualityNode<?> eqNode = (EqualityNode<?>) filter;
+            if ("objectClass".equalsIgnoreCase(eqNode.getAttribute())) {
+                String value = eqNode.getValue().toString();
+                return "groupOfNames".equalsIgnoreCase(value) ||
+                        "groupOfUniqueNames".equalsIgnoreCase(value);
+            }
+        }
+
+        // Case 2a: Wildcard match on all common names and object classes 
(e.g., cn=*, objectClass=*)
+        if (filter instanceof PresenceNode) {
+            PresenceNode presNode = (PresenceNode) filter;
+            return "cn".equalsIgnoreCase(presNode.getAttribute()) ||
+                    "objectClass".equalsIgnoreCase(presNode.getAttribute());
+        }
+        // Case 2b: Wildcard match on object class (e.g., objectClass=*)
+        if (filter instanceof ObjectClassNode) {
+            return true;
+        }
+
+        // Case 3: Compound filters. An AND is an all groups filter only if 
every child is, an OR
+        // if any child is, e.g. (&(objectClass=groupOfNames)(cn=*)) or 
(|(uid=alice)(cn=*)).
+        // A NOT is never an all groups filter: negating a condition never 
positively selects groups.
+        if (filter instanceof AndNode) {
+            List<ExprNode> children = ((AndNode) filter).getChildren();
+            return !children.isEmpty() && 
children.stream().allMatch(this::isAllGroupsFilter);
+        }
+        if (filter instanceof OrNode) {
+            return ((OrNode) 
filter).getChildren().stream().anyMatch(this::isAllGroupsFilter);

Review Comment:
   `anyMatch` is correct for searches on the `proxyGroupSearchBase`. This 
criteria is too loose for searches on the ancestors or the root. This is solved 
by splitting the filter examination depending on whether the search base is the 
complete `proxyGroupSearchBase` or an ancestor.



##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -576,6 +586,54 @@ public List<Entry> search(String searchBase, SearchScope 
searchScope, String fil
         }
     }
 
+    private String mapMemberOfExpression(String filter, SchemaManager 
schemaManager) throws Exception {
+        if (useMemberOf || !filter.toLowerCase(ROOT).contains("memberof")) {
+            return filter;
+        }
+        // Convert filter if it contains "memberOf" but the backend doesn't 
support "memberOf"
+        MemberOfMappingVisitor visitor = new 
MemberOfMappingVisitor(schemaManager, this::lookupGroup, this::isUser);
+        ExprNode node = FilterParser.parse(schemaManager, filter);
+        // Baseline in the same form as the visitor output so an unchanged 
visit exits on the first pass
+        String current = node.toString();
+        for (int i = 0; i < recursiveGroupResolutionMaxDepth; ++i) {
+            Object visitedNode = node.accept(visitor);
+            if (!(visitedNode instanceof ExprNode)) {
+                throw new Exception("Unable to transform memberOf expressions 
in filter: " + filter);
+            }
+            node = (ExprNode) visitedNode;
+            String updated = node.toString();
+            if (updated.equals(current)) {
+                break;
+            }
+            current = updated;
+        }
+        return current;
+    }
+
+    private Entry lookupGroup(String dn, SchemaManager schemaManager) throws 
Exception {
+        String remoteDn = remoteSchemaConverter.convertProxyDnToRemoteDn(dn);
+        LdapConnection connection = null;
+        try {
+            connection = getConnection();
+            Entry entry = connection.lookup(remoteDn);
+            if  (entry != null) {
+                return 
remoteSchemaConverter.convertRemoteEntryToProxyEntry(entry, schemaManager);
+            } else {
+                return null;
+            }
+        } finally {
+            releaseConnection(connection);
+        }
+    }
+
+    private boolean isUser(String userDn) {
+        if (userDn == null) {
+            return false;
+        }
+        String lowerCaseUserDn = userDn.toLowerCase(ROOT);
+        return lowerCaseUserDn.contains(proxyUserSearchBase) || 
lowerCaseUserDn.contains(remoteUserObjectClass);

Review Comment:
   switched string handling to DN handling and check against both proxy and 
backend user search base.



-- 
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]

Reply via email to