handavid commented on code in PR #1331:
URL: https://github.com/apache/knox/pull/1331#discussion_r3687377781
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -839,15 +841,67 @@ private List<Entry> getUserGroupsInternal(LdapConnection
connection, Dn... dns)
String filter = buildMultipleGroupMemberFilter(dns);
- try (EntryCursor cursor = connection.search(remoteGroupSearchBase,
filter, SearchScope.SUBTREE, "cn")) {
- while (cursor.next()) {
- groups.add(cursor.get());
- }
- }
+ groups.addAll(performPagedSearch(connection, remoteGroupSearchBase,
filter, SearchScope.SUBTREE, "cn"));
return groups;
}
+ protected List<Entry> performPagedSearch(LdapConnection connection, String
baseDn, String filter, SearchScope scope, String... attributes ) throws
LdapException, CursorException, IOException {
+ List<Entry> results = new ArrayList<>();
+
+ // 1. Setup basic search parameters
+ SearchRequest searchRequest = new SearchRequestImpl();
+ searchRequest.setBase(new Dn(baseDn));
+ searchRequest.setFilter(filter);
+ searchRequest.setScope(scope);
+ searchRequest.addAttributes(attributes);
+
+ // 2. Initialize the PagedResults control
+ PagedResults pagedControl = new PagedResultsImpl();
+ pagedControl.setSize(pageSize);
+ searchRequest.addControl(pagedControl);
+
+ byte[] cookie = null;
+
+ // 3. Loop until no more pages remain
+ int pageNumber = 1;
+ do {
+ // Update cookie for the subsequent pages
+ if (cookie != null) {
+ pagedControl.setCookie(cookie);
+ }
+
+ try (SearchCursor cursor = connection.search(searchRequest)) {
+ LOG.ldapPagedSearch(baseDn, filter, pageSize, pageNumber);
+ while (cursor.next()) {
+ Response response = cursor.get();
+
+ // Process matching entries
+ if (response instanceof SearchResultEntry) {
+ Entry entry = ((SearchResultEntry)
response).getEntry();
+ results.add(entry);
+ }
+ }
+ if (cursor.isDone()) {
+ SearchResultDone done = cursor.getSearchResultDone();
+ PagedResults responseControl = (PagedResults)
done.getControl(PagedResults.OID);
+
+ if (responseControl != null) {
+ cookie = responseControl.getCookie();
+ } else {
+ cookie = null;
+ }
+ }
+ pageNumber++;
+ } catch (LdapException e) {
+ LOG.ldapSearchFailed(baseDn, filter, e);
Review Comment:
I removed the catch to let the calling methods handle or propagate the
exception. This breaks the loop.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/LdapMessages.java:
##########
@@ -133,9 +141,9 @@ public interface LdapMessages {
text = "Backend user not found: {0}")
void ldapUserNull(String username);
- @Message(level = MessageLevel.ERROR,
+ @Message(level = MessageLevel.DEBUG,
text = "Failed to copy attribute: {0}")
- void ldapAttributeCopyError(@StackTrace(level = MessageLevel.DEBUG)
Exception e);
+ void ldapAttributeCopyError(@StackTrace(level = MessageLevel.TRACE)
Exception e);
Review Comment:
we get these messages because the entry returned from the backend holds
attributes that are not in the proxy's schema. This isn't an "error", but
rather an artifact of the design. At error level, the logs get flooded with 10s
of lines per entry, which would make the log unusable.
##########
gateway-server/src/test/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackendTest.java:
##########
@@ -809,4 +874,28 @@ public Set<String> get(Object key) {
// For the second user, many groups should have been found in the
cache.
assertEquals("Expected " + expectedCacheHits + " cache hits for shared
groups, but got " + cacheHits.get(), expectedCacheHits, cacheHits.get());
}
+
+ private static class CapturingSearchRequestHandler extends
LdapRequestHandler<SearchRequest> {
+ private final LdapRequestHandler<SearchRequest> delegate;
+ private final List<SearchRequest> requests =
Collections.synchronizedList(new ArrayList<>());
+
+ CapturingSearchRequestHandler(LdapRequestHandler<SearchRequest>
delegate) {
+ this.delegate = delegate;
+ }
+
+ public void reset() {
+ requests.clear();
+ }
+
+ public List<SearchRequest> getRequests() {
+ return List.copyOf(requests);
+ }
+
+ @Override
+ public void handle(LdapSession session, SearchRequest message) throws
Exception {
+ requests.add(message);
+ System.out.println(message.toString());
Review Comment:
done
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -511,12 +521,10 @@ public List<Entry> searchUsers(String filter,
SchemaManager schemaManager) throw
try {
connection = getConnection();
String ldapFilter = "(" + remoteUserIdentifierAttribute + "=" +
filter.trim() + ")";
- try (EntryCursor cursor = connection.search(remoteUserSearchBase,
ldapFilter, SearchScope.SUBTREE, "*")) {
- while (cursor.next()) {
- Entry sourceEntry = cursor.get();
- addGroupMemberships(sourceEntry, connection, entryCache,
resolvedParentsCache);
-
results.add(remoteSchemaConverter.convertRemoteEntryToProxyEntry(sourceEntry,
schemaManager));
- }
+ List<Entry> searchResults = performPagedSearch(connection,
remoteUserSearchBase, ldapFilter, SearchScope.SUBTREE, "*");
Review Comment:
good catch. I think that I'll have search propagate the exception also. I
don't think it makes sense to return empty if there is an exception in all
cases.
--
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]
