From 88e01624ebacb16b190b765c7365d1d0666044e2 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Wed, 28 Nov 2007 18:29:04 +0000 Subject: [PATCH] SEC-560: Removed local password comparison form PasswordComparisonAuthenticator. --- .../PasswordComparisonAuthenticator.java | 42 ++--------------- .../PasswordComparisonAuthenticatorTests.java | 47 ++++--------------- 2 files changed, 11 insertions(+), 78 deletions(-) diff --git a/core/src/main/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticator.java b/core/src/main/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticator.java index ea909b26f2..cf9d17ad70 100644 --- a/core/src/main/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticator.java +++ b/core/src/main/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticator.java @@ -35,7 +35,7 @@ import java.util.Iterator; /** * An {@link org.springframework.security.providers.ldap.LdapAuthenticator LdapAuthenticator} which compares the login - * password with the value stored in the directory. + * password with the value stored in the directory using an LDAP "compare" operation. * *

* This can be achieved either by retrieving the password attribute for the user and comparing it locally, @@ -98,20 +98,9 @@ public final class PasswordComparisonAuthenticator extends AbstractLdapAuthentic throw new UsernameNotFoundException(username); } - Object retrievedPassword = user.getObjectAttribute(passwordAttributeName); - - if (retrievedPassword != null) { - if (!verifyPassword(password, retrievedPassword)) { - throw new BadCredentialsException(messages.getMessage( - "PasswordComparisonAuthenticator.badCredentials", "Bad credentials")); - } - - return user; - } - if (logger.isDebugEnabled()) { - logger.debug("Password attribute wasn't retrieved for user '" + authentication - + "'. Performing LDAP compare of password attribute '" + passwordAttributeName + "'"); + logger.debug("Performing LDAP compare of password attribute '" + passwordAttributeName + "' for user '" + + user.getDn() +"'"); } String encodedPassword = passwordEncoder.encodePassword(password, null); @@ -134,29 +123,4 @@ public final class PasswordComparisonAuthenticator extends AbstractLdapAuthentic Assert.notNull(passwordEncoder, "passwordEncoder must not be null."); this.passwordEncoder = passwordEncoder; } - - /** - * Allows the use of both simple and hashed passwords in the directory. - * - * @param password the password supplied by the user - * @param ldapPassword the (possibly hashed) password (from the directory) - * - * @return true if they match - */ - protected boolean verifyPassword(String password, Object ldapPassword) { - if (!(ldapPassword instanceof String)) { - // Assume it's binary - ldapPassword = new String((byte[]) ldapPassword); - } - - if (ldapPassword.equals(password)) { - return true; - } - - if (passwordEncoder.isPasswordValid((String)ldapPassword, password, null)) { - return true; - } - - return false; - } } diff --git a/core/src/test/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java b/core/src/test/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java index 2fddb7d87b..c7b58caed1 100644 --- a/core/src/test/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java +++ b/core/src/test/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticatorTests.java @@ -49,6 +49,7 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio public void onSetUp() throws Exception { super.onSetUp(); authenticator = new PasswordComparisonAuthenticator(getContextSource()); + authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); authenticator.setUserDnPatterns(new String[] {"uid={0},ou=people"}); bob = new UsernamePasswordAuthenticationToken("bob", "bobspassword"); ben = new UsernamePasswordAuthenticationToken("ben", "benspassword"); @@ -74,38 +75,11 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio } catch (UsernameNotFoundException expected) {} } - @Test - public void testLocalComparisonSucceedsWithShaEncodedPassword() { - // Ben's password is SHA encoded - authenticator.authenticate(ben); - } - - @Test - public void testLocalPasswordComparisonFailsWithWrongPassword() { - try { - authenticator.authenticate(new UsernamePasswordAuthenticationToken("bob", "wrongpass")); - fail("Authentication should fail with wrong password."); - } catch (BadCredentialsException expected) {} - } - - @Test + @Test(expected = BadCredentialsException.class) public void testLdapPasswordCompareFailsWithWrongPassword() { // Don't retrieve the password authenticator.setUserAttributes(new String[] {"uid", "cn", "sn"}); - try { - authenticator.authenticate(new UsernamePasswordAuthenticationToken("bob", "wrongpass")); - fail("Authentication should fail with wrong password."); - } catch(BadCredentialsException expected) { - } - } - - @Test - public void testLocalPasswordComparisonSucceedsWithCorrectPassword() { - DirContextOperations user = authenticator.authenticate(bob); - // check username is retrieved. - assertEquals("bob", user.getStringAttribute("uid")); - String password = new String((byte[])user.getObjectAttribute("userPassword")); - assertEquals("bobspassword", password); + authenticator.authenticate(new UsernamePasswordAuthenticationToken("bob", "wrongpass")); } @Test @@ -117,7 +91,6 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio @Test public void testOnlySpecifiedAttributesAreRetrieved() throws Exception { authenticator.setUserAttributes(new String[] {"uid", "userPassword"}); - authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); DirContextAdapter user = (DirContextAdapter) authenticator.authenticate(bob); assertEquals("Should have retrieved 2 attribute (uid, userPassword)", 2, user.getAttributes().size()); @@ -127,8 +100,6 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio public void testLdapCompareSucceedsWithCorrectPassword() { // Don't retrieve the password authenticator.setUserAttributes(new String[] {"uid"}); - // Bob has a plaintext password. - authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); authenticator.authenticate(bob); } @@ -136,15 +107,13 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio public void testLdapCompareSucceedsWithShaEncodedPassword() { // Don't retrieve the password authenticator.setUserAttributes(new String[] {"uid"}); + authenticator.setPasswordEncoder(new LdapShaPasswordEncoder()); authenticator.authenticate(ben); } - @Test + @Test(expected = IllegalArgumentException.class) public void testPasswordEncoderCantBeNull() { - try { - authenticator.setPasswordEncoder(null); - fail("Password encoder can't be null"); - } catch (IllegalArgumentException expected) {} + authenticator.setPasswordEncoder(null); } @Test @@ -156,7 +125,6 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio @Test public void testLdapCompareWithDifferentPasswordAttributeSucceeds() { authenticator.setUserAttributes(new String[] {"uid"}); - authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); authenticator.setPasswordAttributeName("cn"); authenticator.authenticate(new UsernamePasswordAuthenticationToken("ben", "Ben Alex")); } @@ -164,9 +132,10 @@ public class PasswordComparisonAuthenticatorTests extends AbstractLdapIntegratio @Test public void testWithUserSearch() { authenticator = new PasswordComparisonAuthenticator(getContextSource()); + authenticator.setPasswordEncoder(new PlaintextPasswordEncoder()); assertTrue("User DN matches shouldn't be available", authenticator.getUserDns("Bob").isEmpty()); - DirContextAdapter ctx = new DirContextAdapter(new DistinguishedName("uid=Bob,ou=people,dc=springframework,dc=org")); + DirContextAdapter ctx = new DirContextAdapter(new DistinguishedName("uid=Bob,ou=people")); ctx.setAttributeValue("userPassword", "bobspassword"); authenticator.setUserSearch(new MockUserSearch(ctx));