smolnar82 commented on code in PR #1503:
URL: https://github.com/apache/knox/pull/1503#discussion_r4536362209
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptor.java:
##########
@@ -163,8 +174,10 @@ private boolean isAllGroupsFilterForAncestor(ExprNode
filter) {
children.stream().allMatch(this::isAllGroupsFilter);
}
if (filter instanceof OrNode) {
- // An OR is an all groups filter only if every child is.
- return ((OrNode)
filter).getChildren().stream().allMatch(this::isGroupObjectClassFilter);
+ // Remove any (objectClass=referral) nodes. An OR is an all groups
filter only if every child is.
+ return ((OrNode) filter).getChildren().stream()
+ .filter(node -> !isReferralObjectClassFilter(node))
Review Comment:
ApacheDS's `SearchRequestHandler.modifyFilter` doesn't flatten the filter.
It builds `new OrNode(clientFilter, (objectClass=referral))`. Removing the
referral child here only helps when the client filter is a single objectClass
equality, because the remaining child still has to pass
`isGroupObjectClassFilter`. Once wrapped, these root searches are still not
detected as role listings (I checked with a unit test that wraps the filter the
same way):
- `(&(objectClass=groupOfNames)(cn=*))`
- `(&(objectClass=groupOfNames)(|(cn=viewer)(cn=*)))`
- `(|(objectClass=groupOfUniqueNames)(objectClass=groupOfNames))`
Could we follow the JIRA instead and unwrap once in `isAllGroupsSearch`
(line 118)? If the filter is a 2-child `OrNode` and one child is
`(objectClass=referral)`, use the other child as `searchFilter`. Then the
existing `isAllGroupsFilter`/`isAllGroupsFilterForAncestor` logic works
unchanged, and this OR branch can go back to how it was.
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/ldap/interceptor/LDAPRolesLookupInterceptorTest.java:
##########
@@ -667,7 +667,9 @@ public void
testAllGroupsSearchFromRootWithGroupObjectClassFilter() throws Excep
"(objectClass=groupOfUniqueNames)",
"(&(objectClass=groupOfNames)(cn=*))",
"(&(objectClass=groupOfNames)(|(cn=viewer)(cn=*)))",
-
"(|(objectClass=groupOfUniqueNames)(objectClass=groupOfNames))")) {
+
"(|(objectClass=groupOfUniqueNames)(objectClass=groupOfNames))",
+ "(|(objectClass=groupOfNames)(objectClass=referral))",
Review Comment:
These two cases hand-write the wrapped form for the simple filters only. The
other entries in this list (the AND and nested-OR ones) still go to the
interceptor unwrapped, which never happens in production unless the client
sends ManageDsaIT. That's the same blind spot the JIRA describes. Could we add
a helper that wraps the way ApacheDS does, e.g. `new
OrNode(FilterParser.parse(schemaManager, filter),
FilterParser.parse(schemaManager, "(objectClass=referral)"))`, and use it for
every filter in this test? The JIRA also asks for the negative side: please
wrap the filters in `testRootSearchWithNonGroupFilterIsNotAllGroupsSearch`
(line 688) as well, so non-group filters are still not treated as listings once
wrapped.
--
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]
