HADOOP-18074 - Partial/Incomplete groups list can be returned in LDAP… (#4503)

Author: lmccay

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

@@ -58,6 +58,7 @@
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
 
+import org.apache.hadoop.classification.VisibleForTesting;
 import org.apache.hadoop.thirdparty.com.google.common.collect.Iterators;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
@@ -458,7 +459,8 @@ 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 List<String> lookupGroup(SearchResult result, DirContext c,
+  @VisibleForTesting
+  Set<String> lookupGroup(SearchResult result, DirContext c,
       int goUpHierarchy)
       throws NamingException {
     List<String> groups = new ArrayList<>();
@@ -544,6 +546,8 @@ List<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

@@ -18,19 +18,22 @@
 
 package org.apache.hadoop.security;
 
+import java.util.ArrayList;
 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;
 
 import org.apache.hadoop.conf.Configuration;
 import org.junit.Assert;
-import org.junit.Before;
 import org.junit.Test;
+import org.mockito.stubbing.Stubber;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
@@ -49,48 +52,121 @@
 public class TestLdapGroupsMappingWithOneQuery
     extends TestLdapGroupsMappingBase {
 
-  @Before
-  public void setupMocks() throws NamingException {
+  public void setupMocks(List<String> listOfDNs) 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 = "CN=sss,CN=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);
+    buildListOfGroupDNs(listOfDNs).when(groupNames).next();
+    when(groupNames.hasMore()).
+      thenReturn(true).thenReturn(true).
+      thenReturn(true).thenReturn(false);
 
     when(getAttributes().get(eq("memberOf"))).thenReturn(groupDN);
   }
 
+  /**
+   * Build and return a list of individually added group DNs such
+   * that calls to .next() will result in a single value each time.
+   *
+   * @param listOfDNs
+   * @return the stubber to use for the .when().next() call
+   */
+  private Stubber buildListOfGroupDNs(List<String> listOfDNs) {
+    Stubber stubber = null;
+    for (String s : listOfDNs) {
+      if (stubber != null) {
+        stubber.doReturn(s);
+      } else {
+        stubber = doReturn(s);
+      }
+    }
+    return stubber;
+  }
+
   @Test
   public void testGetGroups() throws NamingException {
     // given a user whose ldap query returns a user object with three "memberOf"
     // 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)
       throws NamingException {
+    List<String> groupDns = new ArrayList<>();
+    groupDns.add("CN=abc,DC=foo,DC=bar,DC=com");
+    groupDns.add("CN=xyz,DC=foo,DC=bar,DC=com");
+    groupDns.add("CN=sss,DC=foo,DC=bar,DC=com");
+
+    setupMocks(groupDns);
     String ldapUrl = "ldap://test";
     Configuration conf = getBaseConf(ldapUrl);
     // 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("Second LDAP query should NOT have been called.",
+            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 {
+    List<String> groupDns = new ArrayList<>();
+    groupDns.add("CN=abc,DC=foo,DC=bar,DC=com");
+    groupDns.add("CN=xyz,DC=foo,DC=bar,DC=com");
+    groupDns.add("ipaUniqueID=e4a9a634-bb24-11ec-aec1-06ede52b5fe1," +
+            "CN=sudo,DC=foo,DC=bar,DC=com");
+    setupMocks(groupDns);
+    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("Second LDAP query should have been called.",
+            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));
+  }
+
+  private static final 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);
+    }
+  }
+}