[
https://issues.apache.org/jira/browse/KNOX-3330?focusedWorklogId=1023391&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1023391
]
ASF GitHub Bot logged work on KNOX-3330:
----------------------------------------
Author: ASF GitHub Bot
Created on: 02/Jun/26 20:00
Start Date: 02/Jun/26 20:00
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #1240:
URL: https://github.com/apache/knox/pull/1240#discussion_r3343894688
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/UserSearchInterceptor.java:
##########
@@ -98,47 +107,55 @@ public EntryFilteringCursor search(SearchOperationContext
ctx) throws LdapExcept
}
originalResults.close();
} catch (Exception e) {
- // If we get an error or no results, try the backend
+ // If we get an error or no results, try the backends
}
- // If no local results, try backend
- if (entries.isEmpty() && username != null) {
+ if (username != null) {
try {
- SchemaManager schemaManager =
directoryService.getSchemaManager();
-
if (username.contains("*")) {
// Wildcard search - use searchUsers
LOG.ldapSearch(baseDn, "wildcard user search: " +
username);
- List<Entry> backendEntries =
backend.searchUsers(username, schemaManager);
-
// Return backend results directly without caching to
avoid deadlock
// (caching during an active search can cause ApacheDS
locking issues)
- entries.addAll(backendEntries);
+ entries.addAll(searchUsers(username));
} else {
- // Specific user lookup
- LOG.ldapUserLoaded(username);
- Entry backendEntry = backend.getUser(username,
schemaManager);
-
- if (backendEntry != null) {
- // Return backend result directly without caching
- entries.add(backendEntry);
- LOG.ldapUserEntry(backendEntry.toString());
- } else {
- LOG.ldapUserNull(username);
+ // if no results, perform single-user search
+ if (entries.isEmpty()) {
+ // Specific user lookup
+ LOG.ldapUserLoaded(username);
+ Entry backendEntry = getUser(username);
+
+ if (backendEntry != null) {
+ // Return backend result directly without
caching
+ entries.add(backendEntry);
+ LOG.ldapUserEntry(backendEntry.toString());
+ } else {
+ LOG.ldapUserNull(username);
+ }
}
}
} catch (Exception e) {
- LOG.ldapServiceStopFailed(e);
+ LOG.ldapSearchFailed(baseDn, filter, e);
}
}
// Return cursor with our results - use a simple approach
- return new EntryFilteringCursorImpl(new ListCursor<>(entries),
ctx, directoryService.getSchemaManager());
+ return new EntryFilteringCursorImpl(new ListCursor<>(entries),
ctx, schemaManager);
}
return originalResults;
}
+ private List<Entry> searchUsers(String userSearchString) throws Exception {
+ List<Entry> entries = new ArrayList<>();
+ entries.addAll(backend.searchUsers(userSearchString, schemaManager));
+ return entries;
Review Comment:
Why not simply: `return backend.searchUsers(userSearchString,
schemaManager)`?
In fact, if this is a simple inline backend invocation, why we have this
private method at all? No any particular check, logic, etc...in here.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/UserSearchInterceptor.java:
##########
@@ -98,47 +107,55 @@ public EntryFilteringCursor search(SearchOperationContext
ctx) throws LdapExcept
}
originalResults.close();
} catch (Exception e) {
- // If we get an error or no results, try the backend
+ // If we get an error or no results, try the backends
}
- // If no local results, try backend
- if (entries.isEmpty() && username != null) {
+ if (username != null) {
try {
- SchemaManager schemaManager =
directoryService.getSchemaManager();
-
if (username.contains("*")) {
// Wildcard search - use searchUsers
LOG.ldapSearch(baseDn, "wildcard user search: " +
username);
- List<Entry> backendEntries =
backend.searchUsers(username, schemaManager);
-
// Return backend results directly without caching to
avoid deadlock
// (caching during an active search can cause ApacheDS
locking issues)
- entries.addAll(backendEntries);
+ entries.addAll(searchUsers(username));
} else {
- // Specific user lookup
- LOG.ldapUserLoaded(username);
- Entry backendEntry = backend.getUser(username,
schemaManager);
-
- if (backendEntry != null) {
- // Return backend result directly without caching
- entries.add(backendEntry);
- LOG.ldapUserEntry(backendEntry.toString());
- } else {
- LOG.ldapUserNull(username);
+ // if no results, perform single-user search
+ if (entries.isEmpty()) {
+ // Specific user lookup
+ LOG.ldapUserLoaded(username);
+ Entry backendEntry = getUser(username);
+
+ if (backendEntry != null) {
+ // Return backend result directly without
caching
+ entries.add(backendEntry);
+ LOG.ldapUserEntry(backendEntry.toString());
+ } else {
+ LOG.ldapUserNull(username);
+ }
}
}
} catch (Exception e) {
- LOG.ldapServiceStopFailed(e);
+ LOG.ldapSearchFailed(baseDn, filter, e);
}
}
// Return cursor with our results - use a simple approach
- return new EntryFilteringCursorImpl(new ListCursor<>(entries),
ctx, directoryService.getSchemaManager());
+ return new EntryFilteringCursorImpl(new ListCursor<>(entries),
ctx, schemaManager);
}
return originalResults;
}
+ private List<Entry> searchUsers(String userSearchString) throws Exception {
+ List<Entry> entries = new ArrayList<>();
+ entries.addAll(backend.searchUsers(userSearchString, schemaManager));
+ return entries;
+ }
+
+ private Entry getUser(String username) throws Exception {
+ return backend.getUser(username, schemaManager);
Review Comment:
The same as above: I don't think this private method is necessary.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/UserSearchInterceptorFactory.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.interceptor;
+
+import org.apache.directory.server.core.api.interceptor.Interceptor;
+
+import java.util.Map;
+
+public class UserSearchInterceptorFactory implements
KnoxLdapInterceptorFactory {
+ @Override
+ public Interceptor create(String name, Map<String, String> config) throws
Exception {
+ return new UserSearchInterceptor(name, config);
+ }
+
+ @Override
+ public String getType() {
+ return "backend";
Review Comment:
nit: Should be a constant (we could reuse it in KnoxLdapManager, see my
comment about not using the `instanceof` operator below).
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/KnoxLDAPServerManager.java:
##########
@@ -174,11 +183,30 @@ private void addGroupLookupInterceptor() {
}
}
- final GroupLookupInterceptor interceptor = new
GroupLookupInterceptor(directoryService, backend);
- if (authIdx != -1) {
- interceptors.add(authIdx, interceptor);
- } else {
- interceptors.add(interceptor);
+ // Add our configured interceptors
+ SchemaManager schemaManager = directoryService.getSchemaManager();
+ for (Interceptor interceptor : interceptors) {
+ if (interceptor instanceof UserSearchInterceptor) {
+ // Create partition for remote base DN if different from proxy
base DN
+ // This allows backend entries with remote DNs to be returned
in search results
+ LdapBackend backend = ((UserSearchInterceptor)
interceptor).getBackend();
+ String remoteBaseDn = backend.getBaseDn();
+ if (!baseDns.contains(remoteBaseDn)) {
+ //create partition
+ String id = backend.getName().replaceAll("\\s+", "");
+ JdbmPartition remotePartition = new
JdbmPartition(schemaManager, directoryService.getDnFactory());
+ remotePartition.setId(id);
+ remotePartition.setSuffixDn(new Dn(schemaManager,
remoteBaseDn));
+ remotePartition.setPartitionPath(new File(workDir,
id).toURI());
+ directoryService.addPartition(remotePartition);
+ baseDns.add(remoteBaseDn);
+ }
Review Comment:
nit: a new private method (`addRemotePartition`, for instance) would
increase the readability of this part of the code.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/KnoxLDAPServerManager.java:
##########
@@ -174,11 +183,30 @@ private void addGroupLookupInterceptor() {
}
}
- final GroupLookupInterceptor interceptor = new
GroupLookupInterceptor(directoryService, backend);
- if (authIdx != -1) {
- interceptors.add(authIdx, interceptor);
- } else {
- interceptors.add(interceptor);
+ // Add our configured interceptors
+ SchemaManager schemaManager = directoryService.getSchemaManager();
+ for (Interceptor interceptor : interceptors) {
+ if (interceptor instanceof UserSearchInterceptor) {
Review Comment:
I personally am not a big fan of the `instanceof` operator, so this feel
free to ignore this comment :)
It might be an option to store the interceptors above in a `Map<String,
Interceptor>`, where the key is the interceptor type. If that was the case, we
could simply check if the Map.Entry.key.equals("backend") (this is where my
comment about creating the `backend` interceptor type constant would be useful).
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/KnoxLDAPServerManager.java:
##########
@@ -32,27 +34,34 @@
import org.apache.directory.server.protocol.shared.transport.TcpTransport;
import org.apache.knox.gateway.config.GatewayConfig;
import org.apache.knox.gateway.i18n.messages.MessagesFactory;
-import org.apache.knox.gateway.services.ldap.backend.BackendFactory;
import org.apache.knox.gateway.services.ldap.backend.LdapBackend;
+import org.apache.knox.gateway.services.ldap.interceptor.InterceptorFactory;
+import org.apache.knox.gateway.services.ldap.interceptor.UserSearchInterceptor;
import java.io.File;
import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
/**
* Manages the ApacheDS LDAP server instance with pluggable backends
*/
public class KnoxLDAPServerManager {
private static final LdapMessages LOG =
MessagesFactory.get(LdapMessages.class);
- private DirectoryService directoryService;
+ @VisibleForTesting
+ DirectoryService directoryService;
private LdapServer ldapServer;
- private LdapBackend backend;
+ private List<Interceptor> interceptors = new ArrayList<>();
Review Comment:
This is redundant, as you initialize interceptors anyway (i.e. not simply
add entries into it).
See new line 80: `this.interceptors = createInterceptors(config)`
##########
gateway-server/src/main/resources/conf/gateway-site.xml:
##########
@@ -68,61 +68,76 @@ limitations under the License.
<description>Path to JSON data file for file-based backend. Supports
${GATEWAY_DATA_HOME} variable.</description>
</property>
- <!
Issue Time Tracking
-------------------
Worklog Id: (was: 1023391)
Time Spent: 1h (was: 50m)
> Improve LDAP Proxy configurability for multiple LDAP backends
> -------------------------------------------------------------
>
> Key: KNOX-3330
> URL: https://issues.apache.org/jira/browse/KNOX-3330
> Project: Apache Knox
> Issue Type: Improvement
> Components: Server
> Reporter: David Han
> Assignee: David Han
> Priority: Major
> Time Spent: 1h
> Remaining Estimate: 0h
>
> The Knox ldap proxy uses an interceptor to proxy search requests to a
> configured backend. This configuration is somewhat limiting in how the proxy
> can transform search results. Refactor to configure on the interceptor level
> instead of the backend level so that new types of interceptors can be easily
> added and multiple backends can be included.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
