From d208cf3824e46651fb8ffa8f0051e11d9047bee8 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Thu, 13 Sep 2007 20:42:50 +0000 Subject: [PATCH] SEC-449: Make LdapUserDetailsMapper a pure ContextMapper so it can be used with LdapTemplate. --- .../search/FilterBasedLdapUserSearch.java | 15 +++--- .../ldap/authenticator/BindAuthenticator.java | 5 +- .../PasswordComparisonAuthenticator.java | 6 +-- .../ldap/LdapUserDetailsMapper.java | 48 +++++++++---------- .../FilterBasedLdapUserSearchTests.java | 2 +- ...swordComparisonAuthenticatorMockTests.java | 8 ++-- .../PasswordComparisonAuthenticatorTests.java | 20 ++++---- .../ldap/LdapUserDetailsMapperTests.java | 16 ++++--- 8 files changed, 62 insertions(+), 58 deletions(-) diff --git a/core/src/main/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearch.java b/core/src/main/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearch.java index fc91e87d96..0caf20312d 100644 --- a/core/src/main/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearch.java +++ b/core/src/main/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearch.java @@ -117,15 +117,14 @@ public class FilterBasedLdapUserSearch implements LdapUserSearch { try { - LdapUserDetailsImpl.Essence user = (LdapUserDetailsImpl.Essence) template.searchForSingleEntry( + LdapUserDetailsImpl user = (LdapUserDetailsImpl) template.searchForSingleEntry( searchBase, searchFilter, new String[] {username}, userDetailsMapper); - user.setUsername(username); -// if (!username.equals(user.getUsername())) { -// logger.debug("Search returned user object with different username: " + user.getUsername()); -// } + if (!username.equals(user.getUsername())) { + logger.debug("Search returned user object with different username: " + user.getUsername()); + } - return user.createUserDetails(); + return user; } catch (IncorrectResultSizeDataAccessException notFound) { if (notFound.getActualSize() == 0) { throw new UsernameNotFoundException("User " + username + " not found in directory."); @@ -164,6 +163,10 @@ public class FilterBasedLdapUserSearch implements LdapUserSearch { searchControls.setTimeLimit(searchTimeLimit); } + public void setUserDetailsMapper(LdapUserDetailsMapper userDetailsMapper) { + this.userDetailsMapper = userDetailsMapper; + } + public String toString() { StringBuffer sb = new StringBuffer(); diff --git a/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticator.java b/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticator.java index 4a9cdf393f..e661e4b075 100644 --- a/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticator.java +++ b/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticator.java @@ -88,11 +88,10 @@ public class BindAuthenticator extends AbstractLdapAuthenticator { new BindWithSpecificDnContextSource(getInitialDirContextFactory(), userDn, password)); try { - LdapUserDetailsImpl.Essence user = (LdapUserDetailsImpl.Essence) template.retrieveEntry(userDn, + LdapUserDetailsImpl user = (LdapUserDetailsImpl) template.retrieveEntry(userDn, getUserDetailsMapper(), getUserAttributes()); - user.setUsername(username); - return user.createUserDetails(); + return user; } catch (BadCredentialsException e) { // This will be thrown if an invalid user name is used and the method may // be called multiple times to try different names, so we trap the exception diff --git a/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticator.java b/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticator.java index 77218e4521..f751647cfc 100644 --- a/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticator.java +++ b/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticator.java @@ -82,14 +82,12 @@ public final class PasswordComparisonAuthenticator extends AbstractLdapAuthentic final String userDn = (String) dns.next(); if (ldapTemplate.nameExists(userDn)) { - LdapUserDetailsImpl.Essence userEssence = (LdapUserDetailsImpl.Essence) + user = (LdapUserDetailsImpl) ldapTemplate.retrieveEntry(userDn, getUserDetailsMapper(), getUserAttributes()); - userEssence.setUsername(username); - user = userEssence.createUserDetails(); } } - if ((user == null) && (getUserSearch() != null)) { + if (user == null && getUserSearch() != null) { user = getUserSearch().searchForUser(username); } diff --git a/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapper.java b/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapper.java index 98819858ad..51824acee2 100644 --- a/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapper.java +++ b/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapper.java @@ -40,7 +40,7 @@ public class LdapUserDetailsMapper implements ContextMapper { //~ Instance fields ================================================================================================ private final Log logger = LogFactory.getLog(LdapUserDetailsMapper.class); -// private String usernameAttributeName = "uid"; + private String usernameAttributeName = "uid"; private String passwordAttributeName = "userPassword"; private String rolePrefix = "ROLE_"; private String[] roleAttributes = null; @@ -66,7 +66,7 @@ public class LdapUserDetailsMapper implements ContextMapper { essence.setPassword(mapPassword(passwordAttribute)); } -// essence.setUsername(mapUsername(ctx)); + essence.setUsername(mapUsername(ctx)); // Map the roles for (int i = 0; (roleAttributes != null) && (i < roleAttributes.length); i++) { @@ -86,8 +86,8 @@ public class LdapUserDetailsMapper implements ContextMapper { } } - //return essence.createUserDetails(); - return essence; + return essence.createUserDetails(); + //return essence; } /** @@ -115,23 +115,23 @@ public class LdapUserDetailsMapper implements ContextMapper { } -// protected String mapUsername(DirContextAdapter ctx) { -// Attribute usernameAttribute = ctx.getAttributes().get(usernameAttributeName); -// String username; -// -// if (usernameAttribute == null) { -// throw new AttributesIntegrityViolationException( -// "Failed to get attribute " + usernameAttributeName + " from context"); -// } -// -// try { -// username = (String) usernameAttribute.get(); -// } catch (NamingException e) { -// throw new UncategorizedLdapException("Failed to get username from attribute " + usernameAttributeName, e); -// } -// -// return username; -// } + protected String mapUsername(DirContextAdapter ctx) { + Attribute usernameAttribute = ctx.getAttributes().get(usernameAttributeName); + String username; + + if (usernameAttribute == null) { + throw new UncategorizedLdapException( + "Failed to get attribute " + usernameAttributeName + " from context"); + } + + try { + username = (String) usernameAttribute.get(); + } catch (NamingException e) { + throw new UncategorizedLdapException("Failed to get username from attribute " + usernameAttributeName, e); + } + + return username; + } /** * Creates a GrantedAuthority from a role attribute. Override to customize @@ -176,9 +176,9 @@ public class LdapUserDetailsMapper implements ContextMapper { } -// public void setUsernameAttributeName(String usernameAttributeName) { -// this.usernameAttributeName = usernameAttributeName; -// } + public void setUsernameAttributeName(String usernameAttributeName) { + this.usernameAttributeName = usernameAttributeName; + } /** * The names of any attributes in the user's entry which represent application diff --git a/core/src/test/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearchTests.java b/core/src/test/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearchTests.java index b2114af662..36c07f2d5f 100644 --- a/core/src/test/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearchTests.java +++ b/core/src/test/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearchTests.java @@ -92,7 +92,7 @@ public class FilterBasedLdapUserSearchTests extends AbstractLdapIntegrationTests locator.setSearchSubtree(true); LdapUserDetails ben = locator.searchForUser("Ben Alex"); - assertEquals("Ben Alex", ben.getUsername()); + assertEquals("ben", ben.getUsername()); // assertEquals("uid=ben,ou=people,dc=acegisecurity,dc=org", ben.getDn()); } diff --git a/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorMockTests.java b/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorMockTests.java index c78c8747b6..d7b9c6b217 100644 --- a/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorMockTests.java +++ b/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorMockTests.java @@ -23,6 +23,7 @@ import org.jmock.MockObjectTestCase; import javax.naming.directory.Attributes; import javax.naming.directory.BasicAttributes; import javax.naming.directory.DirContext; +import javax.naming.directory.BasicAttribute; /** @@ -33,9 +34,10 @@ import javax.naming.directory.DirContext; public class PasswordComparisonAuthenticatorMockTests extends MockObjectTestCase { //~ Methods ======================================================================================================== - public void testLdapCompareIsUsedWhenPasswordIsNotRetrieved() - throws Exception { + public void testLdapCompareIsUsedWhenPasswordIsNotRetrieved() throws Exception { Mock mockCtx = mock(DirContext.class); + BasicAttributes attrs = new BasicAttributes(); + attrs.put(new BasicAttribute("uid", "bob")); PasswordComparisonAuthenticator authenticator = new PasswordComparisonAuthenticator(new MockInitialDirContextFactory( (DirContext) mockCtx.proxy(), "dc=acegisecurity,dc=org")); @@ -46,7 +48,7 @@ public class PasswordComparisonAuthenticatorMockTests extends MockObjectTestCase mockCtx.expects(atLeastOnce()).method("getNameInNamespace").will(returnValue("dc=acegisecurity,dc=org")); mockCtx.expects(once()).method("lookup").with(eq("cn=Bob, ou=people")).will(returnValue(true)); mockCtx.expects(once()).method("getAttributes").with(eq("cn=Bob, ou=people"), NULL) - .will(returnValue(new BasicAttributes())); + .will(returnValue(attrs)); // Setup a single return value (i.e. success) Attributes searchResults = new BasicAttributes("", null); diff --git a/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java b/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java index 216b32554b..c5bdddb3ca 100644 --- a/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java +++ b/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java @@ -86,7 +86,7 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio public void testLdapPasswordCompareFailsWithWrongPassword() { // Don't retrieve the password - authenticator.setUserAttributes(new String[] {"cn", "sn"}); + authenticator.setUserAttributes(new String[] {"uid", "cn", "sn"}); try { authenticator.authenticate("Bob", "wrongpassword"); fail("Authentication should fail with wrong password."); @@ -95,9 +95,9 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio } public void testLocalPasswordComparisonSucceedsWithCorrectPassword() { - LdapUserDetails user = authenticator.authenticate("Bob", "bobspassword"); + LdapUserDetails user = authenticator.authenticate("bob", "bobspassword"); // check username is retrieved. - assertEquals("Bob", user.getUsername()); + assertEquals("bob", user.getUsername()); assertEquals("bobspassword", user.getPassword()); } @@ -107,16 +107,16 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio } public void testOnlySpecifiedAttributesAreRetrieved() throws Exception { - authenticator.setUserAttributes(new String[] {"userPassword"}); + authenticator.setUserAttributes(new String[] {"uid", "userPassword"}); authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); LdapUserDetails user = authenticator.authenticate("Bob", "bobspassword"); - assertEquals("Should have retrieved 1 attribute (userPassword)", 1, user.getAttributes().size()); + assertEquals("Should have retrieved 2 attribute (uid, userPassword)", 2, user.getAttributes().size()); } public void testLdapCompareSucceedsWithCorrectPassword() { // Don't retrieve the password - authenticator.setUserAttributes(new String[] {"cn"}); + authenticator.setUserAttributes(new String[] {"uid"}); // Bob has a plaintext password. authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); authenticator.authenticate("bob", "bobspassword"); @@ -124,7 +124,7 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio public void testLdapCompareSucceedsWithShaEncodedPassword() { // Don't retrieve the password - authenticator.setUserAttributes(new String[] {"cn"}); + authenticator.setUserAttributes(new String[] {"uid"}); authenticator.authenticate("ben", "benspassword"); } @@ -145,10 +145,10 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio } public void testLdapCompareWithDifferentPasswordAttributeSucceeds() { - authenticator.setUserAttributes(new String[] {"cn"}); + authenticator.setUserAttributes(new String[] {"uid"}); authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); - authenticator.setPasswordAttributeName("uid"); - authenticator.authenticate("bob", "bob"); + authenticator.setPasswordAttributeName("cn"); + authenticator.authenticate("bob", "Bob Hamilton"); } public void testWithUserSearch() { diff --git a/core/src/test/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapperTests.java b/core/src/test/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapperTests.java index aee18cfab7..b0a2c5385f 100644 --- a/core/src/test/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapperTests.java +++ b/core/src/test/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapperTests.java @@ -42,10 +42,11 @@ public class LdapUserDetailsMapperTests extends TestCase { DirContextAdapter ctx = new DirContextAdapter(); ctx.setAttributeValues("userRole", new String[] {"X", "Y", "Z"}); + ctx.setAttributeValue("uid", "ani"); - LdapUserDetailsImpl.Essence user = (LdapUserDetailsImpl.Essence) mapper.mapFromContext(ctx); + LdapUserDetailsImpl user = (LdapUserDetailsImpl) mapper.mapFromContext(ctx); - assertEquals(3, user.getGrantedAuthorities().length); + assertEquals(3, user.getAuthorities().length); } /** @@ -60,11 +61,12 @@ public class LdapUserDetailsMapperTests extends TestCase { attrs.put(new BasicAttribute("userRole", "x")); DirContextAdapter ctx = new DirContextAdapter(attrs, new DistinguishedName("cn=someName")); + ctx.setAttributeValue("uid", "ani"); - LdapUserDetailsImpl.Essence user = (LdapUserDetailsImpl.Essence) mapper.mapFromContext(ctx); + LdapUserDetailsImpl user = (LdapUserDetailsImpl) mapper.mapFromContext(ctx); - assertEquals(1, user.getGrantedAuthorities().length); - assertEquals("ROLE_X", user.getGrantedAuthorities()[0].getAuthority()); + assertEquals(1, user.getAuthorities().length); + assertEquals("ROLE_X", user.getAuthorities()[0].getAuthority()); } // public void testNonStringRoleAttributeIsIgnoredByDefault() throws Exception { @@ -90,9 +92,9 @@ public class LdapUserDetailsMapperTests extends TestCase { attrs.put(new BasicAttribute("myappsPassword", "mypassword".getBytes())); DirContextAdapter ctx = new DirContextAdapter(attrs, new DistinguishedName("cn=someName")); + ctx.setAttributeValue("uid", "ani"); - LdapUserDetails user = - ((LdapUserDetailsImpl.Essence) mapper.mapFromContext(ctx)).createUserDetails(); + LdapUserDetails user = (LdapUserDetailsImpl) mapper.mapFromContext(ctx); assertEquals("mypassword", user.getPassword()); }