From d56adb8ffb72f4ca509015645275b1d907c6157a Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Thu, 10 Jun 2010 22:27:50 +0100 Subject: [PATCH] SEC-1495: Convert User class equals and hashcode methods to only use the "username" property. This prevents situations where other data may have changed when a User object is reloaded (during a subsequent authentication attempt, in which case and Set.contains()/Map.containsKey() will return false even though the collection in question contains a principal representing the same user. --- .../security/core/userdetails/User.java | 68 ++++++------------- .../security/core/userdetails/UserTests.java | 34 +++++----- 2 files changed, 38 insertions(+), 64 deletions(-) diff --git a/core/src/main/java/org/springframework/security/core/userdetails/User.java b/core/src/main/java/org/springframework/security/core/userdetails/User.java index 32d20c0266..fcc5f7f285 100644 --- a/core/src/main/java/org/springframework/security/core/userdetails/User.java +++ b/core/src/main/java/org/springframework/security/core/userdetails/User.java @@ -32,8 +32,14 @@ import org.springframework.util.Assert; * Implemented with value object semantics (immutable after construction, like a String). * Developers may use this class directly, subclass it, or write their own {@link UserDetails} implementation from * scratch. + *

+ * {@code equals} and {@code hashcode} implementations are based on the {@code username} property only, as the + * intention is that lookups of the same user principal object (in a user registry, for example) will match + * where the objects represent the same user, not just when all the properties (authorities, password for + * example) are the same. * * @author Ben Alex + * @author Luke Taylor */ public class User implements UserDetails { //~ Instance fields ================================================================================================ @@ -153,61 +159,27 @@ public class User implements UserDetails { } } + /** + * Returns {@code true} if the supplied object is a {@code User} instance with the + * same {@code username} value. + *

+ * In other words, the objects are equal if they have the same username, representing the + * same principal. + */ @Override public boolean equals(Object rhs) { - if (!(rhs instanceof User) || (rhs == null)) { - return false; + if (rhs instanceof User) { + return username.equals(((User) rhs).username); } - - User user = (User) rhs; - - // We rely on constructor to guarantee any User has non-null - // authorities - if (!authorities.equals(user.authorities)) { - return false; - } - - // We rely on constructor to guarantee non-null username and password - return (this.getPassword().equals(user.getPassword()) && this.getUsername().equals(user.getUsername()) - && (this.isAccountNonExpired() == user.isAccountNonExpired()) - && (this.isAccountNonLocked() == user.isAccountNonLocked()) - && (this.isCredentialsNonExpired() == user.isCredentialsNonExpired()) - && (this.isEnabled() == user.isEnabled())); + return false; } + /** + * Returns the hashcode of the {@code username}. + */ @Override public int hashCode() { - int code = 9792; - - for (GrantedAuthority authority : getAuthorities()) { - code = code * (authority.hashCode() % 7); - } - - if (this.getPassword() != null) { - code = code * (this.getPassword().hashCode() % 7); - } - - if (this.getUsername() != null) { - code = code * (this.getUsername().hashCode() % 7); - } - - if (this.isAccountNonExpired()) { - code = code * -2; - } - - if (this.isAccountNonLocked()) { - code = code * -3; - } - - if (this.isCredentialsNonExpired()) { - code = code * -5; - } - - if (this.isEnabled()) { - code = code * -7; - } - - return code; + return username.hashCode(); } @Override diff --git a/core/src/test/java/org/springframework/security/core/userdetails/UserTests.java b/core/src/test/java/org/springframework/security/core/userdetails/UserTests.java index e1621f6645..5c3165e342 100644 --- a/core/src/test/java/org/springframework/security/core/userdetails/UserTests.java +++ b/core/src/test/java/org/springframework/security/core/userdetails/UserTests.java @@ -19,7 +19,9 @@ import static org.junit.Assert.*; import java.io.ByteArrayOutputStream; import java.io.ObjectOutputStream; +import java.util.HashSet; import java.util.List; +import java.util.Set; import org.junit.Test; import org.springframework.security.core.GrantedAuthority; @@ -37,24 +39,24 @@ public class UserTests { //~ Methods ======================================================================================================== @Test - public void testEquals() { - User user1 = new User("rod", "koala", true, true, true, true,ROLE_12); + public void equalsReturnsTrueIfUsernamesAreTheSame() { + User user1 = new User("rod", "koala", true, true, true, true, ROLE_12); assertFalse(user1.equals(null)); assertFalse(user1.equals("A STRING")); assertTrue(user1.equals(user1)); - assertTrue(user1.equals(new User("rod", "koala", true, true, true, true,ROLE_12))); - // Equal as the new User will internally sort the GrantedAuthorities in the correct order, before running equals() - assertTrue(user1.equals(new User("rod", "koala", true, true, true, true, - AuthorityUtils.createAuthorityList("ROLE_TWO","ROLE_ONE")))); - assertFalse(user1.equals(new User("DIFFERENT_USERNAME", "koala", true, true, true, true, ROLE_12))); - assertFalse(user1.equals(new User("rod", "DIFFERENT_PASSWORD", true, true, true, true, ROLE_12))); - assertFalse(user1.equals(new User("rod", "koala", false, true, true, true, ROLE_12))); - assertFalse(user1.equals(new User("rod", "koala", true, false, true, true, ROLE_12))); - assertFalse(user1.equals(new User("rod", "koala", true, true, false, true, ROLE_12))); - assertFalse(user1.equals(new User("rod", "koala", true, true, true, false, ROLE_12))); - assertFalse(user1.equals(new User("rod", "koala", true, true, true, true, - AuthorityUtils.createAuthorityList("ROLE_ONE")))); + assertTrue(user1.equals(new User("rod", "notthesame", true, true, true, true, ROLE_12))); + } + + @Test + public void hashLookupOnlyDependsOnUsername() throws Exception { + User user1 = new User("rod", "koala", true, true, true, true, ROLE_12); + Set users = new HashSet(); + users.add(user1); + + assertTrue(users.contains(new User("rod", "koala", true, true, true, true, ROLE_12))); + assertTrue(users.contains(new User("rod", "anotherpass", false, false, false, false, AuthorityUtils.createAuthorityList("ROLE_X")))); + assertFalse(users.contains(new User("bod", "koala", true, true, true, true, ROLE_12))); } @Test @@ -116,9 +118,9 @@ public class UserTests { } @Test - public void testUserIsEnabled() throws Exception { + public void enabledFlagIsFalseForDisabledAccount() throws Exception { UserDetails user = new User("rod", "koala", false, true, true, true, ROLE_12); - assertTrue(!user.isEnabled()); + assertFalse(user.isEnabled()); } @Test