smolnar82 commented on code in PR #1454:
URL: https://github.com/apache/knox/pull/1454#discussion_r4294989077
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -80,6 +83,8 @@ public class LdapProxyBackend implements LdapBackend {
private static final String[] ALL_ATTRIBUTES = new String[]{"*"};
private static final String[] ALL_ATTRIBUTES_AND_MEMBEROF = new
String[]{"*", "memberOf"};
+ private static final String DEFAULT_MAX_FILTER_LENGTH = "2000";
Review Comment:
- `buildMultipleGroupMemberFilters` also splits the existing
group-membership resolution, not only the new memberOf expansion. At roughly
150 characters per POSIX clause, every `useMemberOf=false` deployment makes one
extra round trip for about every 13 users.
- Real server limits are far higher (OpenLDAP 256KB anonymous / 4MB
authenticated, AD 10MB).
I'd default to `0` (unlimited) or to something much larger.
##########
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:
The comment's intent ("the proxy groups base, or the root with an explicit
group filter") makes sense, but the code doesn't enforce either half:
- `searchDn.size() <= 2` only means "root" when the base DN has exactly two
RDNs. With `gateway.ldap.base.dn=o=acme`, `ou=users,o=acme` also matches, so a
user search with `(cn=*)` returns roles. With `dc=acme,dc=co,dc=uk`, the root
never matches. Please compare normalized `Dn`s against the configured base DN
and `ou=groups,<baseDn>`, which also removes the spacing problem with
`toString().equalsIgnoreCase`.
- `isAllGroupsFilter` accepts `(cn=*)` and `(objectClass=*)`, which aren't
explicit group filters. At the root, the default `ldapsearch -b
dc=proxy,dc=com` uses `(objectClass=*)` and now returns only roles, with no
users. Please accept those two filters only when the base is
`ou=groups,<baseDn>`, and require `objectClass=groupOfNames|groupOfUniqueNames`
at the root.
- Minor: a ONELEVEL search at the root returns roles, which sit one level
lower, under `ou=groups`. With `objectClass=*` accepted, this breaks tree
browsing in LDAP browsers.
##########
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:
With "any child matches", `(|(uid=alice)(cn=*))` is treated as all-groups
and `alice` is dropped. An OR filter only selects all groups if every child
does, so this should be `allMatch`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/roles/rest/RestApiLdapRolesReverseLookup.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.ldap.roles.rest;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.HttpEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.ldap.LdapMessages;
+import org.apache.knox.gateway.services.ldap.RoleAssignees;
+import org.apache.knox.gateway.services.ldap.RoleAssignment;
+import org.apache.knox.gateway.services.ldap.roles.LdapRolesReverseLookup;
+import org.apache.knox.gateway.services.ldap.roles.RoleException;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * REST API based implementation of LdapRolesReverseLookup.
+ */
+public class RestApiLdapRolesReverseLookup implements LdapRolesReverseLookup {
+ private static final LdapMessages LOG =
MessagesFactory.get(LdapMessages.class);
+
+ private final ObjectMapper mapper = new ObjectMapper();
+ private final String listEndpoint;
+ private final String getEndpoint;
+
+ public RestApiLdapRolesReverseLookup(String listEndpoint, String
getEndpoint) {
+ this.listEndpoint = listEndpoint;
+ this.getEndpoint = getEndpoint;
+ }
+
+ @Override
+ public Collection<String> listRoles() throws RoleException {
+ final List<Role> roles = listRoles(null);
+ Set<String> result = new HashSet<>();
+ for (Role role : roles) {
+ result.add(new RoleAssignment(role.getScope(),
role.getName()).getDisplayValue());
+ }
+ return result;
+ }
+
+ @Override
+ public RoleAssignees lookupRoleAssignees(String role) throws RoleException
{
+ if (role != null) {
+ RoleAssignment roleAssignment =
RoleAssignment.createFromDisplayValue(role);
+ List<Role> rolesList = listRoles(roleAssignment);
+ String roleId = findRoleId(roleAssignment, rolesList);
+ if (roleId != null) {
+ List<String> users = new ArrayList<>();
+ List<String> groups = new ArrayList<>();
+ List<RoleMember> members = getMembers(roleId);
+ if (members != null) {
+ for (RoleMember member : members) {
+ if (member.getMemberType().equalsIgnoreCase("user")) {
+ users.add(member.getMemberId());
+ } else if
(member.getMemberType().equalsIgnoreCase("group")) {
+ groups.add(member.getMemberId());
+ } else {
+ // ignore unknown member types
+ LOG.restRolesAssigneeTypeUnknown(role,
member.getMemberType());
+ }
+ }
+ }
+ return new RoleAssignees(users, groups);
+ }
+ }
+ return new RoleAssignees(List.of(), List.of());
+ }
+
+ private List<RoleMember> getMembers(String roleId) throws RoleException {
+ String encodedRoleId = URLEncoder.encode(roleId,
StandardCharsets.UTF_8)
+ .replace("+", "%20"); // URLEncoder converts spaces to '+',
path params need '%20'
+ String getUrl = getEndpoint + "/" + encodedRoleId;
+
+ try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
+ LOG.restRolesGetRequest(getUrl, roleId);
+ HttpGet getRequest = new HttpGet(getUrl);
+ getRequest.setHeader("Accept", "application/json");
+ try (CloseableHttpResponse response =
httpClient.execute(getRequest)) {
+ int statusCode = response.getStatusLine().getStatusCode();
+ HttpEntity entity = response.getEntity();
+ final String jsonResponse = entity == null ? null :
EntityUtils.toString(entity);
+ LOG.restRolesGetResponse(getUrl, statusCode, jsonResponse);
+
+ if (statusCode != 200) {
+ LOG.restRolesLookupHttpError(getUrl, statusCode,
jsonResponse);
Review Comment:
The new `restRolesGetHttpError` and `restRolesListHttpError` messages aren't
used, so get and list errors are logged as lookup errors
(`restRolesLookupHttpError`).
The same applies at line 162 (`listRoles`).
##########
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) {
Review Comment:
This loop always runs up to `recursiveGroupResolutionMaxDepth` (default 3),
even when `recursiveGroupResolution` is off, which is the default. Please do a
single pass when recursion is disabled.
##########
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);
+ }
+
+ return false;
+ }
+
+ private EntryFilteringCursor handleAllGroupsSearch(SearchOperationContext
ctx) throws LdapException {
+ // short-circuit the LDAP search to return all roles
+ Collection<String> roles = null;
+ try {
+ roles = rolesLookupService.listRoles();
+ } catch (Exception e) {
+ throw new LdapException("Failed to retrieve roles from roles
lookup service.", e);
+ }
+ List<Entry> entries = new ArrayList<>();
+ for (String role : roles) {
+ Dn roleDn = new Dn(schemaManager, createGroupDnFromName(role));
+ Entry entry = createRoleEntry(roleDn);
+ entries.add(entry);
+ }
+
+ return new EntryFilteringCursorImpl(new ListCursor<>(entries), ctx,
schemaManager);
+ }
+
+ private EntryFilteringCursor handleSearch(SearchOperationContext ctx)
throws LdapException {
+ // Replace the role dn with the member dns if the search filter
contains a "memberOf" node.
+ ExprNode filter = ctx.getFilter();
+ ReverseRoleLookupMappingVisitor reverseRoleLookupMappingVisitor =
+ new ReverseRoleLookupMappingVisitor(schemaManager,
rolesLookupService, this::createGroupDnFromName);
+ Object updatedFilter = filter.accept(reverseRoleLookupMappingVisitor);
Review Comment:
If the roles service fails, a DN can't be parsed, or
`RoleAssignment.createFromDisplayValue` throws on `a:b:c`, the visitor throws
an unchecked exception (`RuntimeException` from
`ReverseRoleLookupMappingVisitor.java:64`, or `IllegalArgumentException`). This
call isn't inside a try/catch, so the exception escapes, the client doesn't get
an `LdapException`, and `ldapRolesLookupFailed` is never logged.
Please catch the exception here and convert it to an `LdapException`. The
same applies to `MemberOfMappingVisitor.java:66` as called from
`LdapProxyBackend.mapMemberOfExpression`.
##########
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);
Review Comment:
This parses the raw filter before `convertProxyFilterToRemoteFilter`
(`RemoteSchemaConverter.java:151`) removes the `:[...]` annotations, and that
method's comment says those annotations occur in practice. Please apply the
same cleanup first, ideally through a shared helper. It's worth testing with a
real annotated filter.
##########
gateway-spi/src/main/java/org/apache/knox/gateway/services/ldap/RoleAssignment.java:
##########
@@ -20,6 +20,18 @@
import com.fasterxml.jackson.annotation.JsonProperty;
public class RoleAssignment {
+ public static RoleAssignment createFromDisplayValue(String displayValue) {
+ String[] tokens = displayValue.split(":");
Review Comment:
Please use `split(":", 2)` so a role name that contains `:` (for example
`platform:team:admin`) is parsed as scope `platform` and name `team:admin`.
Right now it throws `IllegalArgumentException` at line 31, and with the
unhandled-exception issue in `handleSearch` that error escapes the LDAP search
as an unchecked exception.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/roles/rest/RestApiLdapRolesReverseLookup.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.ldap.roles.rest;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.HttpEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.ldap.LdapMessages;
+import org.apache.knox.gateway.services.ldap.RoleAssignees;
+import org.apache.knox.gateway.services.ldap.RoleAssignment;
+import org.apache.knox.gateway.services.ldap.roles.LdapRolesReverseLookup;
+import org.apache.knox.gateway.services.ldap.roles.RoleException;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * REST API based implementation of LdapRolesReverseLookup.
+ */
+public class RestApiLdapRolesReverseLookup implements LdapRolesReverseLookup {
+ private static final LdapMessages LOG =
MessagesFactory.get(LdapMessages.class);
+
+ private final ObjectMapper mapper = new ObjectMapper();
+ private final String listEndpoint;
+ private final String getEndpoint;
+
+ public RestApiLdapRolesReverseLookup(String listEndpoint, String
getEndpoint) {
+ this.listEndpoint = listEndpoint;
+ this.getEndpoint = getEndpoint;
+ }
+
+ @Override
+ public Collection<String> listRoles() throws RoleException {
+ final List<Role> roles = listRoles(null);
+ Set<String> result = new HashSet<>();
+ for (Role role : roles) {
+ result.add(new RoleAssignment(role.getScope(),
role.getName()).getDisplayValue());
+ }
+ return result;
+ }
+
+ @Override
+ public RoleAssignees lookupRoleAssignees(String role) throws RoleException
{
+ if (role != null) {
+ RoleAssignment roleAssignment =
RoleAssignment.createFromDisplayValue(role);
+ List<Role> rolesList = listRoles(roleAssignment);
+ String roleId = findRoleId(roleAssignment, rolesList);
+ if (roleId != null) {
+ List<String> users = new ArrayList<>();
+ List<String> groups = new ArrayList<>();
+ List<RoleMember> members = getMembers(roleId);
+ if (members != null) {
+ for (RoleMember member : members) {
+ if (member.getMemberType().equalsIgnoreCase("user")) {
+ users.add(member.getMemberId());
+ } else if
(member.getMemberType().equalsIgnoreCase("group")) {
+ groups.add(member.getMemberId());
+ } else {
+ // ignore unknown member types
+ LOG.restRolesAssigneeTypeUnknown(role,
member.getMemberType());
+ }
+ }
+ }
+ return new RoleAssignees(users, groups);
+ }
+ }
+ return new RoleAssignees(List.of(), List.of());
+ }
+
+ private List<RoleMember> getMembers(String roleId) throws RoleException {
+ String encodedRoleId = URLEncoder.encode(roleId,
StandardCharsets.UTF_8)
+ .replace("+", "%20"); // URLEncoder converts spaces to '+',
path params need '%20'
+ String getUrl = getEndpoint + "/" + encodedRoleId;
+
+ try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
+ LOG.restRolesGetRequest(getUrl, roleId);
+ HttpGet getRequest = new HttpGet(getUrl);
+ getRequest.setHeader("Accept", "application/json");
+ try (CloseableHttpResponse response =
httpClient.execute(getRequest)) {
+ int statusCode = response.getStatusLine().getStatusCode();
+ HttpEntity entity = response.getEntity();
+ final String jsonResponse = entity == null ? null :
EntityUtils.toString(entity);
+ LOG.restRolesGetResponse(getUrl, statusCode, jsonResponse);
+
+ if (statusCode != 200) {
+ LOG.restRolesLookupHttpError(getUrl, statusCode,
jsonResponse);
+ throw new RoleException("Failed to get roles: HTTP " +
statusCode);
+ }
+
+ if (jsonResponse == null) {
+ throw new RoleException("Empty response from role get
API");
+ }
+
+ return mapper.readValue(jsonResponse, Role.class).getMembers();
+ }
+ } catch (IOException e) {
+ throw new RoleException("Error while executing role get", e);
+ }
+ }
+
+ private List<Role> listRoles(RoleAssignment roleAssignment) throws
RoleException {
+ String listUrl = null;
+ int numRolesRetrieved = 0;
+ int totalExpectedRoles = 0;
+ List<Role> roles = new ArrayList<>();
+
+ try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
+ do {
+ URIBuilder uriBuilder = new URIBuilder(listEndpoint);
+ if (roleAssignment != null) {
+ if (roleAssignment.getName() != null) {
+ uriBuilder.addParameter("name",
roleAssignment.getName());
+ }
+ if (roleAssignment.getScope() != null) {
+ uriBuilder.addParameter("scope",
roleAssignment.getScope());
+ }
+ }
+ uriBuilder.addParameter("offset",
Integer.toString(numRolesRetrieved));
+ URI listURI = uriBuilder.build();
+ listUrl = listURI.toString();
+ LOG.restRolesListRequest(listUrl);
+
+ HttpGet listRequest = new HttpGet(listUrl);
+ listRequest.setHeader("Accept", "application/json");
+ try (CloseableHttpResponse response =
httpClient.execute(listRequest)) {
+ int statusCode = response.getStatusLine().getStatusCode();
+ HttpEntity entity = response.getEntity();
+ final String jsonResponse = entity == null ? null :
EntityUtils.toString(entity);
+ LOG.restRolesListResponse(listUrl, statusCode,
jsonResponse);
+
+ if (statusCode != 200) {
+ LOG.restRolesLookupHttpError(listUrl, statusCode,
jsonResponse);
+ throw new RoleException("Failed to list roles: HTTP "
+ statusCode);
+ }
+
+ if (jsonResponse == null) {
+ throw new RoleException("Empty response from role list
API");
+ }
+
+ ListRolesResponse listRolesResponse =
mapper.readValue(jsonResponse, ListRolesResponse.class);
+ numRolesRetrieved += listRolesResponse.getData().size();
Review Comment:
These throw an NPE when the response has no `data`, `pagination` or
`member_type` (`getData().size()`, `getPagination().getTotal()`,
`member.getMemberType().equalsIgnoreCase(...)`). Please null-check them and
raise a `RoleException` with a clear message.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -861,13 +919,49 @@ private List<Entry> getUserGroupsInternal(LdapConnection
connection, Dn... dns)
return groups;
}
- String filter = buildMultipleGroupMemberFilter(dns);
-
- groups.addAll(performPagedSearch(connection, remoteGroupSearchBase,
filter, SearchScope.SUBTREE, "cn"));
+ groups.addAll(performSplitSearch(connection, remoteGroupSearchBase,
buildMultipleGroupMemberFilters(dns), SearchScope.SUBTREE, "cn"));
return groups;
}
+ /**
+ * Splits a filter that exceeds maxFilterLength into several filters whose
combined results are
+ * equivalent to the original filter. A filter that can't be split is
returned as is.
+ */
+ private List<String> splitFilter(String filter) throws LdapException {
+ if (maxFilterLength <= 0 || filter.length() <= maxFilterLength) {
+ return List.of(filter);
+ }
+ try {
+ // Remote attributes may be unknown to the local schema so parse
without a schema manager
+ List<ExprNode> pieces =
FilterSplitter.split(FilterParser.parse(filter), maxFilterLength);
+ LOG.ldapFilterSplit(filter.length(), maxFilterLength,
pieces.size());
+ return
pieces.stream().map(ExprNode::toString).collect(Collectors.toList());
+ } catch (java.text.ParseException e) {
+ throw new LdapException("Unable to split oversized filter: " +
e.getMessage(), e);
+ }
+ }
+
+ /**
+ * Executes each filter as a separate paged search and combines the
results, removing entries
+ * returned by more than one query. Stops early once maxResultSetSize
entries have been collected.
+ */
+ protected List<Entry> performSplitSearch(LdapConnection connection, String
baseDn, List<String> filters, SearchScope scope, String... attributes) throws
LdapException, CursorException, IOException {
+ if (filters.size() == 1) {
+ return performPagedSearch(connection, baseDn, filters.get(0),
scope, attributes);
+ }
+ Map<String, Entry> combined = new LinkedHashMap<>();
+ for (String filter : filters) {
+ for (Entry entry : performPagedSearch(connection, baseDn, filter,
scope, attributes)) {
+ combined.putIfAbsent(entry.getDn().getNormName(), entry);
+ }
+ if (maxResultSetSize != 0 && combined.size() >= maxResultSetSize) {
+ break;
+ }
+ }
+ return new ArrayList<>(combined.values());
Review Comment:
The `maxResultSetSize` check runs only after each filter piece finishes, so
the combined result can go past the limit. Please truncate it to
`maxResultSetSize`.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/roles/rest/RestApiLdapRolesReverseLookup.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.ldap.roles.rest;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.http.HttpEntity;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.knox.gateway.i18n.messages.MessagesFactory;
+import org.apache.knox.gateway.services.ldap.LdapMessages;
+import org.apache.knox.gateway.services.ldap.RoleAssignees;
+import org.apache.knox.gateway.services.ldap.RoleAssignment;
+import org.apache.knox.gateway.services.ldap.roles.LdapRolesReverseLookup;
+import org.apache.knox.gateway.services.ldap.roles.RoleException;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * REST API based implementation of LdapRolesReverseLookup.
+ */
+public class RestApiLdapRolesReverseLookup implements LdapRolesReverseLookup {
+ private static final LdapMessages LOG =
MessagesFactory.get(LdapMessages.class);
+
+ private final ObjectMapper mapper = new ObjectMapper();
+ private final String listEndpoint;
+ private final String getEndpoint;
+
+ public RestApiLdapRolesReverseLookup(String listEndpoint, String
getEndpoint) {
+ this.listEndpoint = listEndpoint;
+ this.getEndpoint = getEndpoint;
+ }
+
+ @Override
+ public Collection<String> listRoles() throws RoleException {
+ final List<Role> roles = listRoles(null);
+ Set<String> result = new HashSet<>();
+ for (Role role : roles) {
+ result.add(new RoleAssignment(role.getScope(),
role.getName()).getDisplayValue());
+ }
+ return result;
+ }
+
+ @Override
+ public RoleAssignees lookupRoleAssignees(String role) throws RoleException
{
+ if (role != null) {
+ RoleAssignment roleAssignment =
RoleAssignment.createFromDisplayValue(role);
+ List<Role> rolesList = listRoles(roleAssignment);
+ String roleId = findRoleId(roleAssignment, rolesList);
+ if (roleId != null) {
+ List<String> users = new ArrayList<>();
+ List<String> groups = new ArrayList<>();
+ List<RoleMember> members = getMembers(roleId);
+ if (members != null) {
+ for (RoleMember member : members) {
+ if (member.getMemberType().equalsIgnoreCase("user")) {
+ users.add(member.getMemberId());
+ } else if
(member.getMemberType().equalsIgnoreCase("group")) {
+ groups.add(member.getMemberId());
+ } else {
+ // ignore unknown member types
+ LOG.restRolesAssigneeTypeUnknown(role,
member.getMemberType());
+ }
+ }
+ }
+ return new RoleAssignees(users, groups);
+ }
+ }
+ return new RoleAssignees(List.of(), List.of());
+ }
+
+ private List<RoleMember> getMembers(String roleId) throws RoleException {
+ String encodedRoleId = URLEncoder.encode(roleId,
StandardCharsets.UTF_8)
+ .replace("+", "%20"); // URLEncoder converts spaces to '+',
path params need '%20'
+ String getUrl = getEndpoint + "/" + encodedRoleId;
Review Comment:
If the endpoint is configured with a trailing slash
(`http://host/auth/roles/`), this builds `//<id>`. Please strip trailing
slashes from the endpoint, or build the URL with `URIBuilder`.
##########
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:
This check is unreliable:
- `contains(remoteUserObjectClass)`: an objectClass is never part of a DN.
The default `inetOrgPerson` is mixed-case and is compared against a lowercased
string, so it can never match. For AD, `user` matches substrings such as
`cn=users` or `cn=superusers`.
- `proxyUserSearchBase` isn't lowercased, so a base DN configured as
`DC=proxy,DC=com` never matches.
- With `dnMappingEnabled=false`, member DNs stay remote DNs, so they never
contain the proxy base. Every user is then treated as a group, and the filter
silently returns nothing.
Please use `Dn.isDescendantOf` against the user base that matches the
DN-mapping mode, or check the member entry's objectClass. The current tests
don't catch this because `LdapProxyBackendTest` sets `baseDn == remoteBaseDn`
and the visitor test stubs `isUser`.
--
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]
