lmccay commented on code in PR #1236:
URL: https://github.com/apache/knox/pull/1236#discussion_r3277120139
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -131,13 +133,16 @@ public void initialize(Map<String, String> config) throws
Exception {
userIdentifierAttribute =
config.getOrDefault("userIdentifierAttribute", "uid");
groupMemberAttribute = config.getOrDefault("groupMemberAttribute",
"memberUid");
useMemberOf = Boolean.parseBoolean(config.getOrDefault("useMemberOf",
"false"));
+ recursiveGroupResolution =
Boolean.parseBoolean(config.getOrDefault("recursiveGroupResolution", "false"));
+ groupMaxDepth = Integer.parseInt(config.getOrDefault("groupMaxDepth",
"10"));
Review Comment:
That seems like a pretty deep default.
Performance would be terrible, I would think.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -269,6 +274,18 @@ public Entry getUser(String username, SchemaManager
schemaManager) throws Except
return entry;
}
cursor.close();
Review Comment:
If the above 'return entry;' line is executed, this line does not.
Will it be properly cleaned up un the finally block's releaseConnection()?
If there a clearer way to denote this?
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -269,6 +274,18 @@ public Entry getUser(String username, SchemaManager
schemaManager) throws Except
return entry;
}
cursor.close();
+
+ // 2. Try search in group base if not found in user base
+ String groupFilter = "(cn=" + username + ")";
Review Comment:
Do we know for sure that this will always be common name?
Also, I think a little more description of what this groupFilter's affect on
the search will be in the comment would go a long way for future readers.
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -269,6 +274,18 @@ public Entry getUser(String username, SchemaManager
schemaManager) throws Except
return entry;
}
cursor.close();
+
+ // 2. Try search in group base if not found in user base
+ String groupFilter = "(cn=" + username + ")";
+ cursor = connection.search(groupSearchBase, groupFilter,
SearchScope.SUBTREE, "*", "+");
+ if (cursor.next()) {
+ Entry sourceEntry = cursor.get();
+ Entry entry = createProxyEntry(sourceEntry, username,
connection, schemaManager);
+ cursor.close();
+ return entry;
+ }
+ cursor.close();
+
return null;
Review Comment:
do we not need any logging here?
What affect does not getting an Entry for the getUser method have?
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -280,191 +297,218 @@ public List<String> getUserGroups(String username)
throws Exception {
LdapConnection connection = null;
try {
connection = getConnection();
+ Set<String> allGroupDns = new HashSet<>();
if (useMemberOf) {
- // Use memberOf attribute for efficient AD lookups
- return getUserGroupsViaMemberOf(connection, username);
+ allGroupDns.addAll(getUserGroupDnsViaMemberOf(connection,
username));
+ if (recursiveGroupResolution && !allGroupDns.isEmpty()) {
+ resolveRecursiveGroupDnsViaMemberOf(connection,
allGroupDns, 0);
+ }
} else {
- // Use traditional group search approach
String filter = userSearchFilter.replace("{username}",
username);
EntryCursor cursor = connection.search(userSearchBase, filter,
SearchScope.SUBTREE, "dn");
-
if (cursor.next()) {
String userDn = cursor.get().getDn().toString();
cursor.close();
- return getCnsFromEntries(getUserGroupsInternal(connection,
userDn, username));
+ Set<String> visited = new HashSet<>();
+ visited.add(userDn);
+ List<Entry> initialGroups =
getUserGroupsInternal(connection, userDn, username);
+ for (Entry group : initialGroups) {
+ allGroupDns.add(group.getDn().toString());
+ }
+ if (recursiveGroupResolution && !allGroupDns.isEmpty()) {
+ resolveRecursiveGroupDnsInternal(connection,
allGroupDns, visited, 0);
+ }
+ } else {
+ cursor.close();
Review Comment:
Why is this conditional?
cursor doesn't seem to be used below and should be closed even when the
cursor.next() is true when it is complete, no?
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/ldap/backend/LdapProxyBackend.java:
##########
@@ -280,191 +297,218 @@ public List<String> getUserGroups(String username)
throws Exception {
LdapConnection connection = null;
try {
connection = getConnection();
+ Set<String> allGroupDns = new HashSet<>();
if (useMemberOf) {
- // Use memberOf attribute for efficient AD lookups
- return getUserGroupsViaMemberOf(connection, username);
+ allGroupDns.addAll(getUserGroupDnsViaMemberOf(connection,
username));
+ if (recursiveGroupResolution && !allGroupDns.isEmpty()) {
+ resolveRecursiveGroupDnsViaMemberOf(connection,
allGroupDns, 0);
+ }
} else {
- // Use traditional group search approach
String filter = userSearchFilter.replace("{username}",
username);
EntryCursor cursor = connection.search(userSearchBase, filter,
SearchScope.SUBTREE, "dn");
-
if (cursor.next()) {
String userDn = cursor.get().getDn().toString();
cursor.close();
- return getCnsFromEntries(getUserGroupsInternal(connection,
userDn, username));
+ Set<String> visited = new HashSet<>();
+ visited.add(userDn);
+ List<Entry> initialGroups =
getUserGroupsInternal(connection, userDn, username);
+ for (Entry group : initialGroups) {
+ allGroupDns.add(group.getDn().toString());
+ }
+ if (recursiveGroupResolution && !allGroupDns.isEmpty()) {
+ resolveRecursiveGroupDnsInternal(connection,
allGroupDns, visited, 0);
+ }
+ } else {
+ cursor.close();
}
+ }
- cursor.close();
+ List<String> groupNames = new ArrayList<>();
+ for (String dn : allGroupDns) {
+ String name = extractGroupNameFromDn(dn);
+ if (name != null) {
+ groupNames.add(name);
+ }
}
- return Collections.emptyList();
+ return groupNames;
} finally {
releaseConnection(connection);
}
}
- private List<String> getUserGroupsViaMemberOf(LdapConnection connection,
String username) throws LdapException, CursorException, IOException {
- List<String> groups = new ArrayList<>();
-
- // Search for user and retrieve memberOf attribute
+ private List<String> getUserGroupDnsViaMemberOf(LdapConnection connection,
String username) throws LdapException, CursorException, IOException {
+ List<String> dns = new ArrayList<>();
String filter = userSearchFilter.replace("{username}", username);
- EntryCursor cursor = connection.search(userSearchBase, filter,
SearchScope.SUBTREE, "memberOf");
-
+ EntryCursor cursor = connection.search(userSearchBase, filter,
SearchScope.SUBTREE, "memberOf", "+");
if (cursor.next()) {
Entry userEntry = cursor.get();
Attribute memberOfAttr = userEntry.get("memberOf");
-
if (memberOfAttr != null) {
- // Extract group names from DNs
for (org.apache.directory.api.ldap.model.entry.Value value :
memberOfAttr) {
- String groupDn = value.getString();
- String groupName = extractGroupNameFromDn(groupDn);
- if (groupName != null) {
- groups.add(groupName);
+ dns.add(value.getString());
+ }
+ }
+ }
+ cursor.close();
Review Comment:
In any of these methods that throw exceptions, do we know that these cursors
are going to be cleaned up properly or do we need to try-finally and rethrow or
try with resources or something?
--
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]
