diff --git a/core/src/main/java/org/springframework/security/core/GrantedAuthority.java b/core/src/main/java/org/springframework/security/core/GrantedAuthority.java
index f313345502..345805376f 100644
--- a/core/src/main/java/org/springframework/security/core/GrantedAuthority.java
+++ b/core/src/main/java/org/springframework/security/core/GrantedAuthority.java
@@ -18,7 +18,6 @@ package org.springframework.security.core;
import java.io.Serializable;
import org.springframework.security.access.AccessDecisionManager;
-import org.springframework.security.core.userdetails.UserDetails;
/**
* Represents an authority granted to an {@link Authentication} object.
@@ -27,10 +26,6 @@ import org.springframework.security.core.userdetails.UserDetails;
* A GrantedAuthority must either represent itself as a
* String or be specifically supported by an {@link
* AccessDecisionManager}.
- *
- * Implementations must implement {@link Comparable} in order to ensure that
- * array sorting logic guaranteed by {@link UserDetails#getAuthorities()} can
- * be reliably implemented.
*
* @author Ben Alex
* @version $Id$
@@ -41,11 +36,12 @@ public interface GrantedAuthority extends Serializable, ComparableGrantedAuthority can be represented as a String and that
* String is sufficient in precision to be relied upon for an access control decision by an {@link
- * AccessDecisionManager} (or delegate), this method should return such a String.If the
- * GrantedAuthority cannot be expressed with sufficient precision as a String,
+ * AccessDecisionManager} (or delegate), this method should return such a String.
+ *
+ * If the GrantedAuthority cannot be expressed with sufficient precision as a String,
* null should be returned. Returning null will require an
- * AccessDecisionManager (or delegate) to specifically support the GrantedAuthority
- * implementation, so returning null should be avoided unless actually required.
+ * AccessDecisionManager (or delegate) to specifically support the GrantedAuthority
+ * implementation, so returning null should be avoided unless actually required.
*
* @return a representation of the granted authority (or null if the granted authority cannot be
* expressed as a String with sufficient precision).
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 cf30ef95ea..c58e8aaacc 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
@@ -15,11 +15,11 @@
package org.springframework.security.core.userdetails;
-import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
-import java.util.List;
+import java.util.Comparator;
+import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
@@ -40,7 +40,7 @@ public class User implements UserDetails {
//~ Instance fields ================================================================================================
private final String password;
private final String username;
- private final List authorities;
+ private final Set authorities;
private final boolean accountNonExpired;
private final boolean accountNonLocked;
private final boolean credentialsNonExpired;
@@ -78,7 +78,7 @@ public class User implements UserDetails {
*
* @throws IllegalArgumentException if a null value was passed
* either as a parameter or as an element in the
- * GrantedAuthority[] array
+ * GrantedAuthority collection
*/
public User(String username, String password, boolean enabled, boolean accountNonExpired,
boolean credentialsNonExpired, boolean accountNonLocked, Collection authorities) {
@@ -93,7 +93,7 @@ public class User implements UserDetails {
this.accountNonExpired = accountNonExpired;
this.credentialsNonExpired = credentialsNonExpired;
this.accountNonLocked = accountNonLocked;
- this.authorities = Collections.unmodifiableList(sortAuthorities(authorities));
+ this.authorities = Collections.unmodifiableSet(sortAuthorities(authorities));
}
//~ Methods ========================================================================================================
@@ -105,7 +105,7 @@ public class User implements UserDetails {
User user = (User) rhs;
- // We rely on constructor to guarantee any User has non-null and >0
+ // We rely on constructor to guarantee any User has non-null
// authorities
if (!authorities.equals(user.authorities)) {
return false;
@@ -134,10 +134,8 @@ public class User implements UserDetails {
public int hashCode() {
int code = 9792;
- if (this.getAuthorities() != null) {
- for (int i = 0; i < this.getAuthorities().size(); i++) {
- code = code * (authorities.get(i).hashCode() % 7);
- }
+ for (GrantedAuthority authority : getAuthorities()) {
+ code = code * (authority.hashCode() % 7);
}
if (this.getPassword() != null) {
@@ -183,19 +181,31 @@ public class User implements UserDetails {
return enabled;
}
- private static List sortAuthorities(Collection authorities) {
- Assert.notNull(authorities, "Cannot pass a null GrantedAuthority array");
- // Ensure array iteration order is predictable (as per UserDetails.getAuthorities() contract and SEC-xxx)
- SortedSet sorter = new TreeSet();
+ private static SortedSet sortAuthorities(Collection authorities) {
+ Assert.notNull(authorities, "Cannot pass a null GrantedAuthority collection");
+ // Ensure array iteration order is predictable (as per UserDetails.getAuthorities() contract and SEC-717)
+ SortedSet sortedAuthorities =
+ new TreeSet(new Comparator() {
+ public int compare(GrantedAuthority g1, GrantedAuthority g2) {
+ // Neither should ever be null as each entry is checked before adding it to the set.
+ // If the authority is null, it is a custom authority and should precede others.
+ if (g2.getAuthority() == null) {
+ return -1;
+ }
+
+ if (g1.getAuthority() == null) {
+ return 1;
+ }
+
+ return g1.getAuthority().compareTo(g2.getAuthority());
+ }
+ });
for (GrantedAuthority grantedAuthority : authorities) {
Assert.notNull(grantedAuthority, "GrantedAuthority list cannot contain any null elements");
- sorter.add(grantedAuthority);
+ sortedAuthorities.add(grantedAuthority);
}
- List sortedAuthorities = new ArrayList(sorter.size());
- sortedAuthorities.addAll(sorter);
-
return sortedAuthorities;
}
@@ -212,12 +222,14 @@ public class User implements UserDetails {
if (this.getAuthorities() != null) {
sb.append("Granted Authorities: ");
- for (int i = 0; i < authorities.size(); i++) {
- if (i > 0) {
+ boolean first = true;
+ for (GrantedAuthority auth : authorities) {
+ if (!first) {
sb.append(", ");
+ first = false;
}
- sb.append(authorities.get(i));
+ sb.append(auth);
}
} else {
sb.append("Not granted any authorities");