OPEN - issue SEC-899: GrantedAuthorityImpl.compareTo should handle null roles

http://jira.springframework.org/browse/SEC-899. Changed to return -1 when compared to custom auhority which returns null from getAuthority()

core/src/main/java/org/springframework/security/GrantedAuthorityImpl.java

@@ -21,8 +21,13 @@ import org.springframework.util.Assert;
 
 
 /**
- * Basic concrete implementation of a {@link GrantedAuthority}.<p>Stores a <code>String</code> representation of an
- * authority granted to  the {@link Authentication} object.</p>
+ * Basic concrete implementation of a {@link GrantedAuthority}.
+ * 
+ * <p>
+ * Stores a <code>String</code> representation of an authority granted to  the {@link Authentication} object.
+ * <p>
+ * If compared to a custom authority which returns null from {@link #getAuthority}, the <tt>compareTo</tt> 
+ * method will return -1, so the custom authority will take precedence.
  *
  * @author Ben Alex
  * @version $Id$
@@ -36,7 +41,6 @@ public class GrantedAuthorityImpl implements GrantedAuthority, Serializable {
     //~ Constructors ===================================================================================================
 
     public GrantedAuthorityImpl(String role) {
-        super();
         Assert.hasText(role, "A granted authority textual representation is required");
         this.role = role;
     }
@@ -71,8 +75,13 @@ public class GrantedAuthorityImpl implements GrantedAuthority, Serializable {
 
 	public int compareTo(Object o) {
 		if (o != null && o instanceof GrantedAuthority) {
-			GrantedAuthority rhs = (GrantedAuthority) o;
-			return this.role.compareTo(rhs.getAuthority());
+			String rhsRole = ((GrantedAuthority) o).getAuthority();
+			
+			if (rhsRole == null) {
+				return -1;
+			}
+			
+			return role.compareTo(rhsRole);
 		}
 		return -1;
 	}

core/src/test/java/org/springframework/security/GrantedAuthorityImplTests.java

@@ -15,7 +15,9 @@
 
 package org.springframework.security;
 
-import junit.framework.TestCase;
+import static org.junit.Assert.*;
+
+import org.junit.Test;
 
 
 /**
@@ -24,28 +26,10 @@ import junit.framework.TestCase;
  * @author Ben Alex
  * @version $Id$
  */
-public class GrantedAuthorityImplTests extends TestCase {
-    //~ Constructors ===================================================================================================
+public class GrantedAuthorityImplTests {
 	
-    public GrantedAuthorityImplTests() {
-        super();
-    }
-
-    public GrantedAuthorityImplTests(String arg0) {
-        super(arg0);
-    }
-
-    //~ Methods ========================================================================================================
-
-    public static void main(String[] args) {
-        junit.textui.TestRunner.run(GrantedAuthorityImplTests.class);
-    }
-
-    public final void setUp() throws Exception {
-        super.setUp();
-    }
-
-    public void testObjectEquals() throws Exception {
+	@Test
+    public void equalsBehavesAsExpected() throws Exception {
         GrantedAuthorityImpl auth1 = new GrantedAuthorityImpl("TEST");
         GrantedAuthorityImpl auth2 = new GrantedAuthorityImpl("TEST");
         assertEquals(auth1, auth2);
@@ -59,32 +43,52 @@ public class GrantedAuthorityImplTests extends TestCase {
         GrantedAuthorityImpl auth3 = new GrantedAuthorityImpl("NOT_EQUAL");
         assertTrue(!auth1.equals(auth3));
 
-        MockGrantedAuthorityImpl mock1 = new MockGrantedAuthorityImpl("TEST");
+        MockGrantedAuthority mock1 = new MockGrantedAuthority("TEST");
         assertEquals(auth1, mock1);
 
-        MockGrantedAuthorityImpl mock2 = new MockGrantedAuthorityImpl("NOT_EQUAL");
+        MockGrantedAuthority mock2 = new MockGrantedAuthority("NOT_EQUAL");
         assertTrue(!auth1.equals(mock2));
 
         Integer int1 = new Integer(222);
         assertTrue(!auth1.equals(int1));
     }
 
-    public void testToString() {
+	@Test
+    public void toStringReturnsAuthorityValue() {
         GrantedAuthorityImpl auth = new GrantedAuthorityImpl("TEST");
         assertEquals("TEST", auth.toString());
     }
 
+	@Test
+	public void compareToGrantedAuthorityWithSameValueReturns0() {
+		assertEquals(0, new GrantedAuthorityImpl("TEST").compareTo(new MockGrantedAuthority("TEST")));
+	}		
+
+	@Test
+	public void compareToNullReturnsNegativeOne() {
+		assertEquals(-1, new GrantedAuthorityImpl("TEST").compareTo(null));
+	}	
+	
+	/* SEC-899 */
+	@Test
+	public void compareToHandlesCustomAuthorityWhichReturnsNullFromGetAuthority() {
+		assertEquals(-1, new GrantedAuthorityImpl("TEST").compareTo(new MockGrantedAuthority()));
+	}	
+	
     //~ Inner Classes ==================================================================================================
 
-    private class MockGrantedAuthorityImpl implements GrantedAuthority, Comparable {
+    private class MockGrantedAuthority implements GrantedAuthority {
         private String role;
 
-        public MockGrantedAuthorityImpl(String role) {
+        public MockGrantedAuthority() {
+        }        
+        
+        public MockGrantedAuthority(String role) {
             this.role = role;
         }
 
         public int compareTo(Object o) {
-			return this.role.compareTo(((GrantedAuthority)o).getAuthority());
+			throw new UnsupportedOperationException();
 		}
 
         public String getAuthority() {