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>
- <!-- LDAP backend proxy configuration (active when
gateway.ldap.backend.type=ldap) -->
+ <!-- LDAP backend proxy configuration (active when
gateway.ldap.interceptor.names includes ldapproxy) -->
+ <property>
+ <name>gateway.ldap.interceptor.ldapproxy.backendType</name>
+ <value>ldap</value>
+ <description>Backend type for LDAP service. Currently supported: file,
ldap. Future: jdbc, knox.</description>
+ </property>
<property>
- <name>gateway.ldap.backend.ldap.url</name>
+ <name>gateway.ldap.interceptor.ldapproxy.url</name>
<value>ldap://localhost:33389</value>
<description>LDAP server URL for proxy backend</description>
</property>
<property>
- <name>gateway.ldap.backend.ldap.remoteBaseDn</name>
+ <name>gateway.ldap.interceptor.ldapproxy.remoteBaseDn</name>
<value>dc=hadoop,dc=apache,dc=org</value>
<description>Base DN of the remote LDAP server</description>
</property>
<property>
- <name>gateway.ldap.backend.ldap.systemUsername</name>
+ <name>gateway.ldap.interceptor.ldapproxy.systemUsername</name>
<value>uid=guest,ou=people,dc=hadoop,dc=apache,dc=org</value>
<description>LDAP bind DN for proxy backend
authentication</description>
</property>
<property>
- <name>gateway.ldap.backend.ldap.systemPassword</name>
+ <name>gateway.ldap.interceptor.ldapproxy.systemPassword</name>
<value>guest-password</value>
<description>LDAP bind password for proxy backend
authentication</description>
</property>
<!-- Backend-specific configuration using prefixed properties -->
- <!-- Uncomment and configure based on backend type specified in
gateway.ldap.backend.type -->
+ <!-- Uncomment and configure based on interceptor names specified in
gateway.ldap.interceptor.names -->
- <!-- File backend configuration (gateway.ldap.backend.type=file) -->
+ <!-- File backend configuration
(gateway.ldap.interceptor.<interceptorName>.backendType=file) -->
<!--
<property>
- <name>gateway.ldap.backend.file.dataFile</name>
+ <name>gateway.ldap.interceptor.ldapfile.backendType</name>
+ <value>ldap</value>
+ <description>Backend type for LDAP service. Currently supported: file,
ldap. Future: jdbc, knox.</description>
+ </property>
+ <property>
+ <name>gateway.ldap.interceptor.ldapfile.dataFile</name>
<value>${GATEWAY_DATA_HOME}/ldap-users.json</value>
<description>Path to JSON file containing user and group
data</description>
</property>
-->
- <!-- LDAP proxy backend configuration (gateway.ldap.backend.type=ldap) -->
+ <!-- LDAP proxy backend configuration
(gateway.ldap.interceptor.<interceptorName>.backendType=ldap) -->
<!-- This backend proxies to an external LDAP server (e.g., demo LDAP) -->
<!--
Example 1: Using Knox demo LDAP server (default port 33389)
Review Comment:
I'm not sure I like the idea of putting all these samples in the
gateway-site.xml here.
We should rather create the new section in our user guide (see the
`knox-site` module), and add all these sample configs there.
##########
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>
- <!-- LDAP backend proxy configuration (active when
gateway.ldap.backend.type=ldap) -->
+ <!-- LDAP backend proxy configuration (active when
gateway.ldap.interceptor.names includes ldapproxy) -->
+ <property>
+ <name>gateway.ldap.interceptor.ldapproxy.backendType</name>
+ <value>ldap</value>
+ <description>Backend type for LDAP service. Currently supported: file,
ldap. Future: jdbc, knox.</description>
+ </property>
<property>
- <name>gateway.ldap.backend.ldap.url</name>
+ <name>gateway.ldap.interceptor.ldapproxy.url</name>
<value>ldap://localhost:33389</value>
<description>LDAP server URL for proxy backend</description>
</property>
<property>
- <name>gateway.ldap.backend.ldap.remoteBaseDn</name>
+ <name>gateway.ldap.interceptor.ldapproxy.remoteBaseDn</name>
<value>dc=hadoop,dc=apache,dc=org</value>
<description>Base DN of the remote LDAP server</description>
</property>
<property>
- <name>gateway.ldap.backend.ldap.systemUsername</name>
+ <name>gateway.ldap.interceptor.ldapproxy.systemUsername</name>
<value>uid=guest,ou=people,dc=hadoop,dc=apache,dc=org</value>
<description>LDAP bind DN for proxy backend
authentication</description>
</property>
<property>
- <name>gateway.ldap.backend.ldap.systemPassword</name>
+ <name>gateway.ldap.interceptor.ldapproxy.systemPassword</name>
<value>guest-password</value>
<description>LDAP bind password for proxy backend
authentication</description>
</property>
<!-- Backend-specific configuration using prefixed properties -->
- <!-- Uncomment and configure based on backend type specified in
gateway.ldap.backend.type -->
+ <!-- Uncomment and configure based on interceptor names specified in
gateway.ldap.interceptor.names -->
- <!-- File backend configuration (gateway.ldap.backend.type=file) -->
+ <!-- File backend configuration
(gateway.ldap.interceptor.<interceptorName>.backendType=file) -->
<!--
<property>
- <name>gateway.ldap.backend.file.dataFile</name>
+ <name>gateway.ldap.interceptor.ldapfile.backendType</name>
+ <value>ldap</value>
Review Comment:
Should be `file`.
--
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]
