diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java index e4d6225fe87..e751ed6a373 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java @@ -58,6 +58,7 @@ import javax.net.ssl.SSLSocketFactory; 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 @@ public class LdapGroupsMapping * @return a list of strings representing group names of the user. * @throws NamingException if unable to find group names */ - private List lookupGroup(SearchResult result, DirContext c, + @VisibleForTesting + List lookupGroup(SearchResult result, DirContext c, int goUpHierarchy) throws NamingException { List groups = new ArrayList<>(); @@ -510,6 +512,7 @@ public class LdapGroupsMapping List doGetGroups(String user, int goUpHierarchy) throws NamingException { DirContext c = getDirContext(); + List groups = new ArrayList<>(); // Search for the user. We'll only ever need to look at the first result NamingEnumeration results = c.search(userbaseDN, @@ -518,11 +521,10 @@ public class LdapGroupsMapping if (!results.hasMoreElements()) { LOG.debug("doGetGroups({}) returned no groups because the " + "user is not found.", user); - return new ArrayList<>(); + return groups; } SearchResult result = results.nextElement(); - List groups = null; if (useOneQuery) { try { /** @@ -536,7 +538,6 @@ public class LdapGroupsMapping memberOfAttr + "' attribute." + "Returned user object: " + result.toString()); } - groups = new ArrayList<>(); NamingEnumeration groupEnumeration = groupDNAttr.getAll(); while (groupEnumeration.hasMore()) { String groupDN = groupEnumeration.next().toString(); @@ -544,11 +545,13 @@ public class LdapGroupsMapping } } 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); } } - if (groups == null || groups.isEmpty() || goUpHierarchy > 0) { + if (groups.isEmpty() || goUpHierarchy > 0) { groups = lookupGroup(result, c, goUpHierarchy); } LOG.debug("doGetGroups({}) returned {}", user, groups); diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java index 7ae802e26d3..c86f1768b7f 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java @@ -18,19 +18,21 @@ package org.apache.hadoop.security; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; 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 +51,121 @@ import static org.mockito.Mockito.when; public class TestLdapGroupsMappingWithOneQuery extends TestLdapGroupsMappingBase { - @Before - public void setupMocks() throws NamingException { + public void setupMocks(List listOfDNs) throws NamingException { Attribute groupDN = mock(Attribute.class); NamingEnumeration 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 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 expectedGroups) throws NamingException { + List 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 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)); } -} \ No newline at end of file + + private void doTestGetGroupsWithFallback() + throws NamingException { + List 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 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; + } + List lookupGroup(SearchResult result, DirContext c, + int goUpHierarchy) throws NamingException { + secondaryQueryCalled = true; + return super.lookupGroup(result, c, goUpHierarchy); + } + } +}