From e11ba5930e29a370f07685eac888bdb95aefc54f Mon Sep 17 00:00:00 2001 From: lmccay Date: Mon, 11 Jul 2022 01:03:44 -0400 Subject: [PATCH] =?UTF-8?q?HADOOP-18074=20-=20Partial/Incomplete=20groups?= =?UTF-8?q?=20list=20can=20be=20returned=20in=20LDAP=E2=80=A6=20(#4503)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * HADOOP-18074 - Partial/Incomplete groups list can be returned in LDAP groups lookup --- .../hadoop/security/LdapGroupsMapping.java | 6 +- .../TestLdapGroupsMappingWithOneQuery.java | 100 +++++++++++++++--- 2 files changed, 93 insertions(+), 13 deletions(-) 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 4eb3d865ec7..2c388107b62 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 @@ -59,6 +59,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; @@ -428,7 +429,8 @@ public class LdapGroupsMapping * @return a list of strings representing group names of the user. * @throws NamingException if unable to find group names */ - private Set lookupGroup(SearchResult result, DirContext c, + @VisibleForTesting + Set lookupGroup(SearchResult result, DirContext c, int goUpHierarchy) throws NamingException { Set groups = new LinkedHashSet<>(); @@ -510,6 +512,8 @@ 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); } 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..8686d5c6e3b 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,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 @@ 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; + } + Set lookupGroup(SearchResult result, DirContext c, + int goUpHierarchy) throws NamingException { + secondaryQueryCalled = true; + return super.lookupGroup(result, c, goUpHierarchy); + } + } +}