[ 
https://issues.apache.org/jira/browse/KNOX-3330?focusedWorklogId=1024045&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1024045
 ]

ASF GitHub Bot logged work on KNOX-3330:
----------------------------------------

                Author: ASF GitHub Bot
            Created on: 08/Jun/26 09:55
            Start Date: 08/Jun/26 09:55
    Worklog Time Spent: 10m 
      Work Description: 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>
 
-    <!

Issue Time Tracking
-------------------

    Worklog Id:     (was: 1024045)
    Time Spent: 2.5h  (was: 2h 20m)

> 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: 2.5h
>  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)

Reply via email to