From 4d24c88d1ecbf4296cc078781f27c92f559b7bc7 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Mon, 22 May 2006 23:40:29 +0000 Subject: [PATCH] Enforce the setting of a LdapUserDetailsMapper on authenticators (rather than a general mapper) to make sure the correct type is returned and that the username is set before it is returned. --- .../ldap/search/FilterBasedLdapUserSearch.java | 11 ++++++----- .../authenticator/AbstractLdapAuthenticator.java | 4 ++-- .../ldap/authenticator/BindAuthenticator.java | 15 ++++++++------- .../PasswordComparisonAuthenticator.java | 6 +++++- .../userdetails/ldap/LdapUserDetailsImpl.java | 13 +++++++++++-- .../userdetails/ldap/LdapUserDetailsMapper.java | 6 +++++- .../search/FilterBasedLdapUserSearchTests.java | 2 ++ .../authenticator/BindAuthenticatorTests.java | 1 + .../PasswordComparisonAuthenticatorTests.java | 9 ++++++--- 9 files changed, 46 insertions(+), 21 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 7b614a153f..85457a0979 100644 --- a/core/src/main/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearch.java +++ b/core/src/main/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearch.java @@ -18,10 +18,10 @@ package org.acegisecurity.ldap.search; import org.acegisecurity.userdetails.UsernameNotFoundException; import org.acegisecurity.userdetails.ldap.LdapUserDetailsMapper; import org.acegisecurity.userdetails.ldap.LdapUserDetails; +import org.acegisecurity.userdetails.ldap.LdapUserDetailsImpl; import org.acegisecurity.ldap.LdapUserSearch; import org.acegisecurity.ldap.InitialDirContextFactory; import org.acegisecurity.ldap.LdapTemplate; -import org.acegisecurity.ldap.LdapEntryMapper; import org.springframework.util.Assert; @@ -80,7 +80,7 @@ public class FilterBasedLdapUserSearch implements LdapUserSearch { private InitialDirContextFactory initialDirContextFactory; - private LdapEntryMapper userDetailsMapper = new LdapUserDetailsMapper(); + private LdapUserDetailsMapper userDetailsMapper = new LdapUserDetailsMapper(); //~ Methods ================================================================ @@ -122,10 +122,11 @@ public class FilterBasedLdapUserSearch implements LdapUserSearch { template.setSearchControls(searchControls); try { - Object user = template.searchForSingleEntry(searchBase, searchFilter, new String[] { username }, userDetailsMapper); - Assert.isInstanceOf(LdapUserDetails.class, user, "Entry mapper must return an LdapUserDetailsImpl instance"); + LdapUserDetailsImpl.Essence user = (LdapUserDetailsImpl.Essence) + template.searchForSingleEntry(searchBase, searchFilter, new String[] { username }, userDetailsMapper); + user.setUsername(username); - return (LdapUserDetails)user; + return user.createUserDetails(); } catch(EmptyResultDataAccessException notFound) { throw new UsernameNotFoundException("User " + username + " not found in directory."); diff --git a/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/AbstractLdapAuthenticator.java b/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/AbstractLdapAuthenticator.java index 3544dcc058..46645d5aa1 100644 --- a/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/AbstractLdapAuthenticator.java +++ b/core/src/main/java/org/acegisecurity/providers/ldap/authenticator/AbstractLdapAuthenticator.java @@ -57,7 +57,7 @@ public abstract class AbstractLdapAuthenticator implements LdapAuthenticator, /** The attributes which will be retrieved from the directory. Null means all attributes */ private String[] userAttributes = null; - private LdapEntryMapper userDetailsMapper = new LdapUserDetailsMapper(); + private LdapUserDetailsMapper userDetailsMapper = new LdapUserDetailsMapper(); /** * The suffix to be added to the DN patterns, worked out internally from the root DN of the @@ -141,7 +141,7 @@ public abstract class AbstractLdapAuthenticator implements LdapAuthenticator, this.userSearch = userSearch; } - public void setUserDetailsMapper(LdapEntryMapper userDetailsMapper) { + public void setUserDetailsMapper(LdapUserDetailsMapper userDetailsMapper) { Assert.notNull("userDetailsMapper must not be null"); this.userDetailsMapper = userDetailsMapper; } 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 24b9a6b1f7..55cf247334 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 @@ -19,10 +19,10 @@ import org.acegisecurity.ldap.InitialDirContextFactory; import org.acegisecurity.ldap.LdapTemplate; import org.acegisecurity.BadCredentialsException; import org.acegisecurity.userdetails.ldap.LdapUserDetails; +import org.acegisecurity.userdetails.ldap.LdapUserDetailsImpl; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.springframework.util.Assert; import java.util.Iterator; @@ -57,14 +57,14 @@ public class BindAuthenticator extends AbstractLdapAuthenticator { Iterator dns = getUserDns(username).iterator(); while(dns.hasNext() && user == null) { - user = bindWithDn((String)dns.next(), password); + user = bindWithDn((String)dns.next(), username, password); } // Otherwise use the configured locator to find the user // and authenticate with the returned DN. if (user == null && getUserSearch() != null) { LdapUserDetails userFromSearch = getUserSearch().searchForUser(username); - user = bindWithDn(userFromSearch.getDn(), password); + user = bindWithDn(userFromSearch.getDn(), username, password); } if(user == null) { @@ -77,7 +77,7 @@ public class BindAuthenticator extends AbstractLdapAuthenticator { } - LdapUserDetails bindWithDn(String userDn, String password) { + private LdapUserDetails bindWithDn(String userDn, String username, String password) { LdapTemplate template = new LdapTemplate(getInitialDirContextFactory(), userDn, password); if (logger.isDebugEnabled()) { @@ -86,10 +86,11 @@ public class BindAuthenticator extends AbstractLdapAuthenticator { try { - Object user = (LdapUserDetails)template.retrieveEntry(userDn, getUserDetailsMapper(), getUserAttributes()); - Assert.isInstanceOf(LdapUserDetails.class, user, "Entry mapper must return an LdapUserDetails instance"); + LdapUserDetailsImpl.Essence user = + (LdapUserDetailsImpl.Essence) template.retrieveEntry(userDn, getUserDetailsMapper(), getUserAttributes()); + user.setUsername(username); - return (LdapUserDetails) user; + return user.createUserDetails(); } catch(BadCredentialsException e) { // This will be thrown if an invalid user name is used and the method may 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 050e4183ab..9570b50cdd 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 @@ -19,6 +19,7 @@ import org.acegisecurity.ldap.LdapUtils; import org.acegisecurity.ldap.InitialDirContextFactory; import org.acegisecurity.ldap.LdapTemplate; import org.acegisecurity.userdetails.ldap.LdapUserDetails; +import org.acegisecurity.userdetails.ldap.LdapUserDetailsImpl; import org.acegisecurity.providers.encoding.PasswordEncoder; import org.acegisecurity.BadCredentialsException; import org.acegisecurity.userdetails.UsernameNotFoundException; @@ -81,7 +82,10 @@ public final class PasswordComparisonAuthenticator extends AbstractLdapAuthentic final String userDn = (String)dns.next(); if(ldapTemplate.nameExists(userDn)) { - user = (LdapUserDetails)ldapTemplate.retrieveEntry(userDn, getUserDetailsMapper(), getUserAttributes()); + LdapUserDetailsImpl.Essence userEssence = + (LdapUserDetailsImpl.Essence) ldapTemplate.retrieveEntry(userDn, getUserDetailsMapper(), getUserAttributes()); + userEssence.setUsername(username); + user = userEssence.createUserDetails(); } } diff --git a/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsImpl.java b/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsImpl.java index eef759a907..fd69e3814a 100644 --- a/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsImpl.java +++ b/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsImpl.java @@ -1,6 +1,7 @@ package org.acegisecurity.userdetails.ldap; import org.acegisecurity.GrantedAuthority; +import org.springframework.util.Assert; import javax.naming.directory.Attributes; import javax.naming.directory.BasicAttributes; @@ -93,7 +94,9 @@ public class LdapUserDetailsImpl implements LdapUserDetails { //~ Inner classes ========================================================== - /** Variation of essence pattern. Used to create mutable intermediate object */ + /** + * Variation of essence pattern. Used to create mutable intermediate object + */ public static class Essence { LdapUserDetailsImpl instance = new LdapUserDetailsImpl(); @@ -178,9 +181,15 @@ public class LdapUserDetailsImpl implements LdapUserDetails { public LdapUserDetails createUserDetails() { //TODO: Validation of properties + Assert.notNull(instance, "Essence can only be used to create a single instance"); + instance.authorities = getGrantedAuthorities(); - return instance; + LdapUserDetails newInstance = instance; + + instance = null; + + return newInstance; } } } 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 43307bf7b8..5ade7e94f7 100644 --- a/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapper.java +++ b/core/src/main/java/org/acegisecurity/userdetails/ldap/LdapUserDetailsMapper.java @@ -27,6 +27,10 @@ import javax.naming.NamingException; import javax.naming.NamingEnumeration; /** + * The entry mapper used by the authenticators to create an ldap user + * object. + * + * * @author Luke Taylor * @version $Id$ */ @@ -100,6 +104,6 @@ public class LdapUserDetailsMapper implements LdapEntryMapper { } } - return essence.createUserDetails(); + return essence; } } 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 019801b03f..4969842985 100644 --- a/core/src/test/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearchTests.java +++ b/core/src/test/java/org/acegisecurity/ldap/search/FilterBasedLdapUserSearchTests.java @@ -37,6 +37,7 @@ public class FilterBasedLdapUserSearchTests extends AbstractLdapServerTestCase { locator.setDerefLinkFlag(false); LdapUserDetails bob = locator.searchForUser("bob"); + assertEquals("bob", bob.getUsername()); // name is wrong with embedded apacheDS // assertEquals("uid=bob,ou=people,dc=acegisecurity,dc=org", bob.getDn()); } @@ -48,6 +49,7 @@ public class FilterBasedLdapUserSearchTests extends AbstractLdapServerTestCase { locator.setSearchSubtree(true); LdapUserDetails ben = locator.searchForUser("Ben Alex"); + assertEquals("Ben Alex", 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/BindAuthenticatorTests.java b/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticatorTests.java index dc43883da6..b311208dac 100644 --- a/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticatorTests.java +++ b/core/src/test/java/org/acegisecurity/providers/ldap/authenticator/BindAuthenticatorTests.java @@ -32,6 +32,7 @@ public class BindAuthenticatorTests extends AbstractLdapServerTestCase { public void testAuthenticationWithCorrectPasswordSucceeds() { authenticator.setUserDnPatterns(new String[] {"uid={0},ou=people"}); LdapUserDetails user = authenticator.authenticate("bob","bobspassword"); + assertEquals("bob", user.getUsername()); } public void testAuthenticationWithWrongPasswordFails() { 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 9948540546..3758c2bc8d 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 @@ -65,7 +65,10 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapServerTest } */ public void testLocalPasswordComparisonSucceedsWithCorrectPassword() { - authenticator.authenticate("Bob", "bobspassword"); + LdapUserDetails user = authenticator.authenticate("Bob", "bobspassword"); + // check username is retrieved. + assertEquals("Bob", user.getUsername()); + assertEquals("bobspassword", user.getPassword()); } public void testMultipleDnPatternsWorkOk() { @@ -88,7 +91,7 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapServerTest public void testAllAttributesAreRetrivedByDefault() { LdapUserDetails user = authenticator.authenticate("Bob", "bobspassword"); - System.out.println(user.getAttributes().toString()); + //System.out.println(user.getAttributes().toString()); assertEquals("User should have 5 attributes", 5, user.getAttributes().size()); } @@ -107,7 +110,7 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapServerTest mapper.setPasswordAttributeName("uid"); authenticator.setPasswordAttributeName("uid"); authenticator.setUserDetailsMapper(mapper); - authenticator.authenticate("bob", "bob"); + LdapUserDetails bob = authenticator.authenticate("bob", "bob"); } /* public void testLdapCompareWithDifferentPasswordAttributeSucceeds() {