[
https://issues.apache.org/jira/browse/HADOOP-18074?focusedWorklogId=785017&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-785017
]
ASF GitHub Bot logged work on HADOOP-18074:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 27/Jun/22 10:02
Start Date: 27/Jun/22 10:02
Worklog Time Spent: 10m
Work Description: steveloughran commented on code in PR #4503:
URL: https://github.com/apache/hadoop/pull/4503#discussion_r907199594
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java:
##########
@@ -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());
Review Comment:
add an error message for the assertion failure
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java:
##########
@@ -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));
}
-}
\ No newline at end of file
+
+ 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());
Review Comment:
add an error message
##########
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,
Review Comment:
this has been made private so that you can subclass for testing, right?
tag it as @VisibleForTesting
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java:
##########
@@ -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));
}
-}
\ No newline at end of file
+
+ 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 {
Review Comment:
nit private final. probably static too, right
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestLdapGroupsMappingWithOneQuery.java:
##########
@@ -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));
}
-}
\ No newline at end of file
+
+ private void doTestGetGroupsWithFallback()
Review Comment:
mockito tests are always painful to maintain.
could the more complex mocking chains have comments explaining what they do
and put every thenReturn/doReturn on its own line, to make the sequence more
clear.
Issue Time Tracking
-------------------
Worklog Id: (was: 785017)
Time Spent: 50m (was: 40m)
> Partial/Incomplete groups list can be returned in LDAP groups lookup
> --------------------------------------------------------------------
>
> Key: HADOOP-18074
> URL: https://issues.apache.org/jira/browse/HADOOP-18074
> Project: Hadoop Common
> Issue Type: Bug
> Components: security
> Reporter: Philippe Lanoe
> Assignee: Larry McCay
> Priority: Major
> Labels: pull-request-available
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Hello,
> The
> {code:java}
> Set<String> doGetGroups(String user, int goUpHierarchy) {code}
> method in
> [https://github.com/apache/hadoop/blob/b27732c69b114f24358992a5a4d170bc94e2ceaf/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/LdapGroupsMapping.java#L476]
> Looks like having an issue if in the middle of the loop a *NamingException*
> is caught:
> The groups variable is not reset in the catch clause and therefore the
> fallback lookup cannot be executed (when goUpHierarchy==0 at least):
> ||
> {code:java}
> if (groups.isEmpty() || goUpHierarchy > 0) {
> groups = lookupGroup(result, c, goUpHierarchy);
> }
> {code}
>
> Consequence is that only a partial list of groups is returned, which is not
> correct.
> Following options could be used as solution:
> * Reset the group to an empty list in the catch clause, to trigger the
> fallback query.
> * Add an option flag to enable ignoring groups with Naming Exception (since
> they are not groups most probably)
> Independently, would any issue also occur (and therefore full list cannot be
> returned) in the first lookup as well as in the fallback query, the method
> should/could(with option flag) throw an Exception, because in some scenario
> accuracy is important.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
