From e72cfd58d4cb2ff49bd60e0493a71e497290b0f2 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Mon, 30 Nov 2009 21:22:11 +0000 Subject: [PATCH] SEC-1304: Remove Comparable interface from GrantedAuthority to enable it to be imlemented by an enum. --- .../security/core/GrantedAuthority.java | 14 ++--- .../security/core/userdetails/User.java | 54 +++++++++++-------- 2 files changed, 38 insertions(+), 30 deletions(-) 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");