smolnar82 commented on code in PR #1331:
URL: https://github.com/apache/knox/pull/1331#discussion_r3686909146
##########
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:
Previously `searchUsers` had no `try/catch` around its cursor, so an
`LdapException` propagated to the caller. Now it routes through
`performPagedSearch`, which catches and logs, returning partial/empty results.
So a backend failure during user search now looks like "no users found" rather
than an error. `search()` already behaved this way, so this may be intentional.
But it's a behavior change for `searchUsers`. Please confirm silent degradation
is desired there (auth/lookup failures being indistinguishable from empty
results can be a real operational issue).
##########
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:
nit: this should be removed.
##########
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:
Here we log the failure but do not clear cookie or break the loop. As a
result, it might end up in an infinite loop: `page 1` succeeds and returns a
non-empty cookie. So the loop continues; `page 2`'s `connection.search(...)`
(or cursor iteration) throws `LdapException` (server restart, timeout,
connection drop mid-paging). The exception is caught, `cookie` still holds page
1's value, `pageNumber++` is skipped, and the loop condition is still `true`.
It retries the identical failing page forever.
The old code (`search(...)`) caught `LdapException` around a single cursor
and simply stopped with partial results. To preserve that, the catch should
terminate the loop, e.g.:
```
} catch (LdapException e) {
LOG.ldapSearchFailed(baseDn, filter, e);
break; // or: cookie = null;
}
```
What do you think?
##########
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:
Why are these changes needed here? Shouldn't this remain on `ERROR` level
instead of `DEBUG`? We might hide real conversion problems.
--
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]
