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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
