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.
--
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]
