smolnar82 commented on code in PR #1240: URL: https://github.com/apache/knox/pull/1240#discussion_r3372159627
########## gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/InterceptorFactory.java: ########## @@ -0,0 +1,59 @@ +/* + * 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 org.apache.knox.gateway.i18n.messages.MessagesFactory; +import org.apache.knox.gateway.services.ldap.LdapMessages; + +import java.util.Map; +import java.util.ServiceLoader; + +/** + * Factory for creating LDAP Interceptor implementations using ServiceLoader for full extensibility. + * Backends are discovered via META-INF/services/org.apache.knox.gateway.services.ldap.interceptor.KnoxLdapInterceptorFactory + * Built-in interceptors are registered via ServiceLoader along with any external plugins. + */ +public class InterceptorFactory { + private static final LdapMessages LOG = MessagesFactory.get(LdapMessages.class); + + public static Interceptor createInterceptor(String interceptorName, Map<String, String> config) throws Exception { + String interceptorType = config.get("interceptorType"); + if (interceptorType == null) { + // No backend type configured found + LOG.ldapInterceptorTypeNotFound(interceptorName); + throw new IllegalArgumentException("No LDAP interceptor type configured for : " + interceptorName); + } + + // Use ServiceLoader to discover all available interceptors (built-in and external plugins) + // Indirect instantiation through a factory is used to allow configuration of multiple instances + // of the same class of interceptor. e.g., if multiple backends are configured + ServiceLoader<KnoxLdapInterceptorFactory> loader = ServiceLoader.load(KnoxLdapInterceptorFactory.class); + for (KnoxLdapInterceptorFactory interceptorFactory : loader) { + if (interceptorFactory.getType().equalsIgnoreCase(interceptorType)) { + LOG.ldapInterceptorCreating(interceptorType, "ServiceLoader"); + Interceptor interceptor = interceptorFactory.create(interceptorName, config); + return interceptor; Review Comment: nit: you can return directly ########## gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/KnoxLDAPServerManager.java: ########## @@ -99,6 +98,34 @@ public void initialize(GatewayConfig config) throws Exception { workDir.mkdirs(); } + private List<Interceptor> createInterceptors(GatewayConfig config) throws Exception { + List<String> interceptorNames = config.getLDAPInterceptorNames(); + List<Interceptor> interceptors = new ArrayList<>(interceptorNames.size()); + for (String interceptorName : interceptorNames) { + // Get backend-specific configuration using prefixed properties + Map<String, String> interceptorConfig = config.getLDAPInterceptorConfig(interceptorName); + + // Add common configuration + interceptorConfig.put("baseDn", baseDn); + + // Add common LDAP Proxy configurations to backends + String interceptorType = interceptorConfig.get("interceptorType"); + String backendType = interceptorConfig.get("backendType"); + if ("backend".equalsIgnoreCase(interceptorType)) { + interceptorConfig.put("recursiveGroupResolution", String.valueOf(config.isLDAPRecursiveGroupResolutionEnabled())); + interceptorConfig.put("recursiveGroupResolutionMaxDepth", String.valueOf(config.getLDAPRecursiveGroupResolutionMaxDepth())); + if ("file".equalsIgnoreCase(backendType) && + !interceptorConfig.containsKey("dataFile")) { + // Add legacy dataFile property for backwards compatibility with file backend + interceptorConfig.put("dataFile", config.getLDAPBackendDataFile()); + } + } + + interceptors.add(InterceptorFactory.createInterceptor(interceptorName, interceptorConfig)); + } + return interceptors; Review Comment: nit: since `interceptors` is a class member and this method is `private` (i.e. not used in tests), this might be `void` and set `interceptors directly. It really is just a nitpick comment tough. ########## gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DuplicateUserFilteringInterceptor.java: ########## @@ -0,0 +1,87 @@ +/* + * 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 com.google.common.annotations.VisibleForTesting; +import org.apache.directory.api.ldap.model.cursor.CursorException; +import org.apache.directory.api.ldap.model.cursor.ListCursor; +import org.apache.directory.api.ldap.model.entry.Attribute; +import org.apache.directory.api.ldap.model.entry.Entry; +import org.apache.directory.api.ldap.model.entry.Value; +import org.apache.directory.api.ldap.model.exception.LdapException; +import org.apache.directory.server.core.api.filtering.EntryFilteringCursor; +import org.apache.directory.server.core.api.filtering.EntryFilteringCursorImpl; +import org.apache.directory.server.core.api.interceptor.BaseInterceptor; +import org.apache.directory.server.core.api.interceptor.context.SearchOperationContext; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +public class DuplicateUserFilteringInterceptor extends BaseInterceptor { + + public DuplicateUserFilteringInterceptor(String name) { + super(name); + } + + @Override + public EntryFilteringCursor search(SearchOperationContext ctx) throws LdapException { + // First execute the interceptor chain to get the results + List<Entry> filteredEntries = List.of(); + try (EntryFilteringCursor originalResults = next(ctx)) { + List<Entry> originalEntries = new ArrayList<>(); + try { + while (originalResults.next()) { + originalEntries.add(originalResults.get()); + } + originalResults.close(); Review Comment: Since `originalResults` is now opened using `try-with-resources`, we don't need this call. ########## 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: @handavid - I merged #1227, we can add the sample configs there. IMO, we only need to keep the one above (`ldapproxy`) which is "active" (i.e. not commented out), the rest should be added in the documentation below. We can do that in a follow-up JIRA though. -- 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]
