HADOOP-18074 - Partial/Incomplete groups list can be returned in LDAP…

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java

@@ -428,7 +428,7 @@ private NamingEnumeration<SearchResult> lookupPosixGroup(SearchResult result,
    * @return a list of strings representing group names of the user.
    * @throws NamingException if unable to find group names
    */
-  private Set<String> lookupGroup(SearchResult result, DirContext c,
+  Set<String> lookupGroup(SearchResult result, DirContext c,
       int goUpHierarchy)
       throws NamingException {
     Set<String> groups = new LinkedHashSet<>();
@@ -510,6 +510,8 @@ Set<String> doGetGroups(String user, int goUpHierarchy)
         }
       } catch (NamingException e) {
         // If the first lookup failed, fall back to the typical scenario.
+        // In order to force the fallback, we need to reset groups collection.
+        groups.clear();
         LOG.info("Failed to get groups from the first lookup. Initiating " +
                 "the second LDAP query using the user's DN.", e);
       }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java

@@ -20,10 +20,12 @@
 
 import java.util.Arrays;
 import java.util.List;
+import java.util.Set;
 
 import javax.naming.NamingEnumeration;
 import javax.naming.NamingException;
 import javax.naming.directory.Attribute;
+import javax.naming.directory.DirContext;
 import javax.naming.directory.SearchControls;
 import javax.naming.directory.SearchResult;
 
@@ -72,6 +74,9 @@ public void testGetGroups() throws NamingException {
     // properties, return an array of strings representing its groups.
     String[] testGroups = new String[] {"abc", "xyz", "sss"};
     doTestGetGroups(Arrays.asList(testGroups));
+
+    // test fallback triggered by NamingException
+    doTestGetGroupsWithFallback();
   }
 
   private void doTestGetGroups(List<String> expectedGroups)
@@ -81,16 +86,72 @@ private void doTestGetGroups(List<String> expectedGroups)
     // enable single-query lookup
     conf.set(LdapGroupsMapping.MEMBEROF_ATTR_KEY, "memberOf");
 
-    LdapGroupsMapping groupsMapping = getGroupsMapping();
+    TestLdapGroupsMapping groupsMapping = new TestLdapGroupsMapping();
     groupsMapping.setConf(conf);
     // Username is arbitrary, since the spy is mocked to respond the same,
     // regardless of input
     List<String> groups = groupsMapping.getGroups("some_user");
 
     Assert.assertEquals(expectedGroups, groups);
+    Assert.assertFalse(groupsMapping.isSecondaryQueryCalled());
 
     // We should have only made one query because single-query lookup is enabled
     verify(getContext(), times(1)).search(anyString(), anyString(),
         any(Object[].class), any(SearchControls.class));
   }
-}
+
+  private void doTestGetGroupsWithFallback()
+          throws NamingException {
+    Attribute groupDN = mock(Attribute.class);
+
+    NamingEnumeration<SearchResult> groupNames = getGroupNames();
+    doReturn(groupNames).when(groupDN).getAll();
+    String groupName1 = "CN=abc,DC=foo,DC=bar,DC=com";
+    String groupName2 = "CN=xyz,DC=foo,DC=bar,DC=com";
+    String groupName3 = "ipaUniqueID=e4a9a634-bb24-11ec-aec1-06ede52b5fe1," +
+            "CN=sudo,DC=foo,DC=bar,DC=com";
+    doReturn(groupName1).doReturn(groupName2).doReturn(groupName3).
+            when(groupNames).next();
+    when(groupNames.hasMore()).thenReturn(true).thenReturn(true).
+            thenReturn(true).thenReturn(false);
+
+    when(getAttributes().get(eq("memberOf"))).thenReturn(groupDN);
+
+    String ldapUrl = "ldap://test";
+    Configuration conf = getBaseConf(ldapUrl);
+    // enable single-query lookup
+    conf.set(LdapGroupsMapping.MEMBEROF_ATTR_KEY, "memberOf");
+    conf.set(LdapGroupsMapping.LDAP_NUM_ATTEMPTS_KEY, "1");
+
+    TestLdapGroupsMapping groupsMapping = new TestLdapGroupsMapping();
+    groupsMapping.setConf(conf);
+    // Username is arbitrary, since the spy is mocked to respond the same,
+    // regardless of input
+    List<String> groups = groupsMapping.getGroups("some_user");
+
+    // expected to be empty due to invalid memberOf
+    Assert.assertEquals(0, groups.size());
+
+    // expect secondary query to be called: getGroups()
+    Assert.assertTrue(groupsMapping.isSecondaryQueryCalled());
+
+    // We should have fallen back to the second query because first threw
+    // NamingException expected count is 3 since testGetGroups calls
+    // doTestGetGroups and doTestGetGroupsWithFallback in succession and
+    // the count is across both test scenarios.
+    verify(getContext(), times(3)).search(anyString(), anyString(),
+            any(Object[].class), any(SearchControls.class));
+  }
+
+  class TestLdapGroupsMapping extends LdapGroupsMapping {
+    private boolean secondaryQueryCalled = false;
+    public boolean isSecondaryQueryCalled() {
+      return secondaryQueryCalled;
+    }
+    Set<String> lookupGroup(SearchResult result, DirContext c,
+                                    int goUpHierarchy) throws NamingException {
+      secondaryQueryCalled = true;
+      return super.lookupGroup(result, c, goUpHierarchy);
+    }
+  }
+}