[
https://issues.apache.org/jira/browse/KNOX-3330?focusedWorklogId=1023335&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1023335
]
ASF GitHub Bot logged work on KNOX-3330:
----------------------------------------
Author: ASF GitHub Bot
Created on: 02/Jun/26 15:09
Start Date: 02/Jun/26 15:09
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #1240:
URL: https://github.com/apache/knox/pull/1240#discussion_r3341618563
##########
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java:
##########
@@ -1748,8 +1749,16 @@ public String getLDAPBaseDN() {
}
@Override
- public String getLDAPBackendType() {
- return get(LDAP_BACKEND_TYPE, "file");
+ public List<String> getLDAPInterceptorNames() {
+ String names = get(LDAP_INTERCEPTOR_NAMES, "");
+ String[] namesArray = names.split(",");
+ List<String> namesList = new ArrayList<>();
+ for (String name : namesArray) {
+ if (!Strings.isNullOrEmpty(name)) {
Review Comment:
We use `org.apache.commons.lang3.StringUtils#isNotBlank` in the project for
the same purpose.
This is already imported -> the new import will be eliminated too.
Even better, we already have a method to get comma separated config values
as list:
`org.apache.knox.gateway.config.impl.GatewayConfigImpl#splitConfigValueToList`
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DuplicateUserFilteringInterceptor.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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 {
Review Comment:
Please add unit test coverage for this class.
##########
gateway-server/data/security/keystores/__gateway-credentials.jceks:
##########
Review Comment:
Please remove this file from the commit (and the other .jceks files too).
##########
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);
Review Comment:
This log line should be under the actual `getUser(username);` line
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DuplicateUserFilteringInterceptorFactory.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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 DuplicateUserFilteringInterceptorFactory implements
KnoxLdapInterceptorFactory {
+ @Override
+ public Interceptor create(String name, Map<String, String> config) {
+ // NOTE: no further configuration expected for this Interceptor
+ return new DuplicateUserFilteringInterceptor(name);
+ }
+
+ @Override
+ public String getType() {
+ return "duplicateuserfilter";
Review Comment:
nit: using a constant would be better
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DuplicateUserFilteringInterceptor.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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
+ EntryFilteringCursor originalResults;
+ originalResults = next(ctx);
+
+ List<Entry> originalEntries = new ArrayList<>();
+ try {
+ while (originalResults.next()) {
+ originalEntries.add(originalResults.get());
+ }
+ originalResults.close();
Review Comment:
Please ensure that `originalResults.close()` is always invoked to avoid
cursor leaks (using try-with-resources is a good option here).
E.g.
```
try (EntryFilteringCursor originalResults = next(ctx)) {
...
}
```
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/interceptor/DuplicateUserFilteringInterceptorFactory.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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 DuplicateUserFilteringInterceptorFactory implements
KnoxLdapInterceptorFactory {
+ @Override
+ public Interceptor create(String name, Map<String, String> config) {
+ // NOTE: no further configuration expected for this Interceptor
Review Comment:
nit: I think this JavaDoc comment is redundant and can be obsolete quickly.
Issue Time Tracking
-------------------
Worklog Id: (was: 1023335)
Time Spent: 50m (was: 40m)
> 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: 50m
> 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)
