[GitHub] [hadoop] lmccay commented on a diff in pull request #4798: HADOOP-18388. Allow dynamic groupSearchFilter in LdapGroupsMapping.

[GitBox <gi...@apache.org>]

lmccay commented on code in PR #4798:
URL: https://github.com/apache/hadoop/pull/4798#discussion_r960739138


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java:
##########
@@ -437,8 +443,14 @@ Set<String> lookupGroup(SearchResult result, DirContext c,
     Set<String> groupDNs = new HashSet<>();
 
     NamingEnumeration<SearchResult> groupResults;
-    // perform the second LDAP query
-    if (isPosix) {
+
+    String[] resolved = resolveCustomGroupFilterArgs(result);
+    // If custom group filter argument is supplied, use that!!!
+    if (resolved != null) {

Review Comment:
   Is it not the case that this condition is a subset of the non-posix condition check? I think that we should only be looking for customGroupFilterArgs if it is non-posix not before the check of isPosix. Am I missing something that makes this the appropriate place for this?



##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMapping.java:
##########
@@ -120,6 +124,50 @@ public void testGetGroupsWithDefaultBaseDN() throws Exception {
     doTestGetGroupsWithBaseDN(conf, baseDN.trim(), baseDN.trim());
   }
 
+  @Test
+  public void testGetGroupsWithDynamicGroupFilter() throws Exception {
+    // Set basic mock stuff.
+    Configuration conf = getBaseConf(TEST_LDAP_URL);
+    String baseDN = "dc=xxx,dc=com";
+    conf.set(LdapGroupsMapping.BASE_DN_KEY, baseDN);
+    Attributes attributes = getAttributes();
+
+    // Set the groupFilter attributed to take the csv.
+    Attribute groupFilterAttr = mock(Attribute.class);
+    when(groupFilterAttr.get()).thenReturn("userDN,userName");
+    when(attributes.get(eq("groupfilter"))).thenReturn(groupFilterAttr);
+
+    // Set the value for userName attribute that is to be used as part of the
+    // group filter at argument 1.
+    final String userName = "some_user";
+    Attribute userNameAttr = mock(Attribute.class);
+    when(userNameAttr.get()).thenReturn(userName);
+    when(attributes.get(eq("userName"))).thenReturn(userNameAttr);
+
+    // Set the dynamic group search filter.
+    final String groupSearchFilter =
+        "(|(memberUid={0})(uname={1}))" + "(objectClass=group)";
+    conf.set(LdapGroupsMapping.GROUP_SEARCH_FILTER_KEY, groupSearchFilter);
+
+    final LdapGroupsMapping groupsMapping = getGroupsMapping();
+    groupsMapping.setConf(conf);
+
+    // The group search filter should be resolved and should be passed as the
+    // bellow.

Review Comment:
   sic. 'below'



-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org