diff --git a/core/src/main/java/org/springframework/security/GrantedAuthority.java b/core/src/main/java/org/springframework/security/GrantedAuthority.java index 9848089926..a0c8ed40f7 100644 --- a/core/src/main/java/org/springframework/security/GrantedAuthority.java +++ b/core/src/main/java/org/springframework/security/GrantedAuthority.java @@ -17,6 +17,8 @@ package org.springframework.security; import java.io.Serializable; +import org.springframework.security.userdetails.UserDetails; + /** * Represents an authority granted to an {@link Authentication} object. * @@ -25,11 +27,17 @@ import java.io.Serializable; * 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$ */ -public interface GrantedAuthority extends Serializable { +public interface GrantedAuthority extends Serializable, Comparable { //~ Methods ======================================================================================================== /** diff --git a/core/src/main/java/org/springframework/security/GrantedAuthorityImpl.java b/core/src/main/java/org/springframework/security/GrantedAuthorityImpl.java index 0acfddc7c4..daf6b9f1f0 100644 --- a/core/src/main/java/org/springframework/security/GrantedAuthorityImpl.java +++ b/core/src/main/java/org/springframework/security/GrantedAuthorityImpl.java @@ -17,6 +17,8 @@ package org.springframework.security; import java.io.Serializable; +import org.springframework.util.Assert; + /** * Basic concrete implementation of a {@link GrantedAuthority}.

Stores a String representation of an @@ -35,6 +37,7 @@ public class GrantedAuthorityImpl implements GrantedAuthority, Serializable { public GrantedAuthorityImpl(String role) { super(); + Assert.hasText(role, "A granted authority textual representation is required"); this.role = role; } @@ -65,4 +68,12 @@ public class GrantedAuthorityImpl implements GrantedAuthority, Serializable { public String toString() { return this.role; } + + public int compareTo(Object o) { + if (o != null && o instanceof GrantedAuthority) { + GrantedAuthority rhs = (GrantedAuthority) o; + return this.role.compareTo(rhs.getAuthority()); + } + return -1; + } } diff --git a/core/src/main/java/org/springframework/security/userdetails/User.java b/core/src/main/java/org/springframework/security/userdetails/User.java index 0e8d947fc2..0bc00326bd 100644 --- a/core/src/main/java/org/springframework/security/userdetails/User.java +++ b/core/src/main/java/org/springframework/security/userdetails/User.java @@ -15,8 +15,11 @@ package org.springframework.security.userdetails; -import org.springframework.security.GrantedAuthority; +import java.util.Iterator; +import java.util.SortedSet; +import java.util.TreeSet; +import org.springframework.security.GrantedAuthority; import org.springframework.util.Assert; @@ -231,13 +234,15 @@ public class User implements UserDetails { protected void setAuthorities(GrantedAuthority[] 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(); for (int i = 0; i < authorities.length; i++) { Assert.notNull(authorities[i], "Granted authority element " + i + " is null - GrantedAuthority[] cannot contain any null elements"); + sorter.add(authorities[i]); } - - this.authorities = authorities; + + this.authorities = (GrantedAuthority[]) sorter.toArray(new GrantedAuthority[sorter.size()]); } public String toString() { diff --git a/core/src/main/java/org/springframework/security/userdetails/UserDetails.java b/core/src/main/java/org/springframework/security/userdetails/UserDetails.java index 1c9f419725..8c4efa0b25 100644 --- a/core/src/main/java/org/springframework/security/userdetails/UserDetails.java +++ b/core/src/main/java/org/springframework/security/userdetails/UserDetails.java @@ -54,7 +54,7 @@ public interface UserDetails extends Serializable { /** * Returns the authorities granted to the user. Cannot return null. * - * @return the authorities (never null) + * @return the authorities, sorted by natural key (never null) */ GrantedAuthority[] getAuthorities(); diff --git a/core/src/test/java/org/springframework/security/GrantedAuthorityImplTests.java b/core/src/test/java/org/springframework/security/GrantedAuthorityImplTests.java index 75219162af..5563168bc2 100644 --- a/core/src/test/java/org/springframework/security/GrantedAuthorityImplTests.java +++ b/core/src/test/java/org/springframework/security/GrantedAuthorityImplTests.java @@ -76,14 +76,18 @@ public class GrantedAuthorityImplTests extends TestCase { //~ Inner Classes ================================================================================================== - private class MockGrantedAuthorityImpl implements GrantedAuthority { + private class MockGrantedAuthorityImpl implements GrantedAuthority, Comparable { private String role; public MockGrantedAuthorityImpl(String role) { this.role = role; } - private MockGrantedAuthorityImpl() { + public int compareTo(Object o) { + return this.role.compareTo(((GrantedAuthority)o).getAuthority()); + } + + private MockGrantedAuthorityImpl() { super(); } diff --git a/core/src/test/java/org/springframework/security/userdetails/UserTests.java b/core/src/test/java/org/springframework/security/userdetails/UserTests.java index 69872d304b..471ef26d45 100644 --- a/core/src/test/java/org/springframework/security/userdetails/UserTests.java +++ b/core/src/test/java/org/springframework/security/userdetails/UserTests.java @@ -64,6 +64,11 @@ public class UserTests extends TestCase { new User("rod", "koala", true, true, true, true, new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}))); + // 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, + new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_TWO"), new GrantedAuthorityImpl("ROLE_ONE")}))); + assertFalse(user1.equals( new User("DIFFERENT_USERNAME", "koala", true, true, true, true, new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}))); @@ -153,7 +158,7 @@ public class UserTests extends TestCase { public void testUserGettersSetter() throws Exception { UserDetails user = new User("rod", "koala", true, true, true, true, - new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_ONE"), new GrantedAuthorityImpl("ROLE_TWO")}); + new GrantedAuthority[] {new GrantedAuthorityImpl("ROLE_TWO"), new GrantedAuthorityImpl("ROLE_ONE")}); assertEquals("rod", user.getUsername()); assertEquals("koala", user.getPassword()); assertTrue(user.isEnabled());