SEC-863: Hierarchical roles should use the interface GrantedAuthority. Applied submitted patch.

core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java

@@ -106,8 +106,8 @@ public class RoleHierarchyImpl implements RoleHierarchy {
         Set<GrantedAuthority> reachableRoles = new HashSet<GrantedAuthority>();
 
         for (GrantedAuthority authority : authorities) {
-            reachableRoles.add(authority);
-            Set<GrantedAuthority> additionalReachableRoles = rolesReachableInOneOrMoreStepsMap.get(authority);
+            addReachableRoles(reachableRoles, authority);
+            Set<GrantedAuthority> additionalReachableRoles = getRolesReachableInOneOrMoreSteps(authority);
             if (additionalReachableRoles != null) {
                 reachableRoles.addAll(additionalReachableRoles);
             }
@@ -124,6 +124,41 @@ public class RoleHierarchyImpl implements RoleHierarchy {
         return reachableRoleList;
     }
 
+    // SEC-863
+	private void addReachableRoles(Set<GrantedAuthority> reachableRoles,
+			GrantedAuthority authority) {
+		
+		Iterator<GrantedAuthority> iterator = reachableRoles.iterator();		
+		while (iterator.hasNext()) {
+			GrantedAuthority testAuthority = iterator.next(); 
+			String testKey = testAuthority.getAuthority();
+			if ((testKey != null) && (testKey.equals(authority.getAuthority()))) {
+				return;
+			}
+		}
+		reachableRoles.add(authority);
+	}
+
+    // SEC-863
+	private Set<GrantedAuthority> getRolesReachableInOneOrMoreSteps(
+			GrantedAuthority authority) {
+		
+		if (authority.getAuthority() == null) {
+			return null;
+		}
+		
+		Iterator<GrantedAuthority> iterator = rolesReachableInOneOrMoreStepsMap.keySet().iterator();		
+		while (iterator.hasNext()) {
+			GrantedAuthority testAuthority = iterator.next(); 
+			String testKey = testAuthority.getAuthority();
+			if ((testKey != null) && (testKey.equals(authority.getAuthority()))) {
+				return rolesReachableInOneOrMoreStepsMap.get(testAuthority);
+			}
+		}
+		
+		return null;
+	}
+    
     /**
      * Parse input and build the map for the roles reachable in one step: the higher role will become a key that
      * references a set of the reachable lower roles.
@@ -145,7 +180,7 @@ public class RoleHierarchyImpl implements RoleHierarchy {
             } else {
                 rolesReachableInOneStepSet = rolesReachableInOneStepMap.get(higherRole);
             }
-            rolesReachableInOneStepSet.add(lowerRole);
+            addReachableRoles(rolesReachableInOneStepSet, lowerRole);
 
             logger.debug("buildRolesReachableInOneStepMap() - From role "
                     + higherRole + " one can reach role " + lowerRole + " in one step.");
@@ -174,7 +209,7 @@ public class RoleHierarchyImpl implements RoleHierarchy {
                 // take a role from the rolesToVisit set
                 GrantedAuthority aRole = (GrantedAuthority) rolesToVisitSet.iterator().next();
                 rolesToVisitSet.remove(aRole);
-                visitedRolesSet.add(aRole);
+                addReachableRoles(visitedRolesSet, aRole);
                 if (rolesReachableInOneStepMap.containsKey(aRole)) {
                     Set<GrantedAuthority> newReachableRoles = rolesReachableInOneStepMap.get(aRole);
 

core/src/test/java/org/springframework/security/access/hierarchicalroles/HierarchicalRolesTestHelper.java

@@ -14,6 +14,7 @@
 
 package org.springframework.security.access.hierarchicalroles;
 
+import java.util.ArrayList;
 import java.util.List;
 
 import org.springframework.security.core.GrantedAuthority;
@@ -37,4 +38,55 @@ public abstract class HierarchicalRolesTestHelper {
         return CollectionUtils.isEqualCollection(authorities1, authorities2);
     }
 
+    public static boolean containTheSameGrantedAuthoritiesCompareByAuthorityString(List<GrantedAuthority> authorities1, List<GrantedAuthority> authorities2) {
+        if (authorities1 == null && authorities2 == null) {
+            return true;
+        }
+
+        if (authorities1 == null || authorities2 == null) {
+            return false;
+        }
+        return CollectionUtils.isEqualCollection(toListOfAuthorityStrings(authorities1), toListOfAuthorityStrings(authorities2));
+    }
+
+    public static List<String> toListOfAuthorityStrings(List<GrantedAuthority> authorities) {
+        if (authorities == null) {
+            return null;
+        }
+
+        List<String> result = new ArrayList<String>(authorities.size());
+        for (GrantedAuthority authority : authorities) {
+            result.add(authority.getAuthority());
+        }
+        return result;
+    }
+
+    public static List<GrantedAuthority> createAuthorityList(final String... roles) {
+        List<GrantedAuthority> authorities = new ArrayList<GrantedAuthority>(roles.length);
+
+        for (final String role : roles) {
+            // Use non GrantedAuthorityImpl (SEC-863)
+            authorities.add(new GrantedAuthority() {
+                public String getAuthority() {
+                    return role;
+                }
+
+                public int compareTo(GrantedAuthority ga) {
+                    if (ga != null) {
+                        String rhsRole = ga.getAuthority();
+
+                        if (rhsRole == null) {
+                            return -1;
+                        }
+
+                        return role.compareTo(rhsRole);
+                    }
+                    return -1;
+                }
+            });
+        }
+
+        return authorities;
+    }
+
 }

core/src/test/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java

@@ -111,4 +111,18 @@ public class RoleHierarchyImplTests extends TestCase {
         }
     }
     
+    // SEC-863
+    public void testSimpleRoleHierarchyWithCustomGrantedAuthorityImplementation() {
+
+        List<GrantedAuthority> authorities0 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_0");
+        List<GrantedAuthority> authorities1 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A");
+        List<GrantedAuthority> authorities2 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A","ROLE_B");
+
+        RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl();
+        roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B");
+
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthoritiesCompareByAuthorityString(roleHierarchyImpl.getReachableGrantedAuthorities(authorities0), authorities0));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthoritiesCompareByAuthorityString(roleHierarchyImpl.getReachableGrantedAuthorities(authorities1), authorities2));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthoritiesCompareByAuthorityString(roleHierarchyImpl.getReachableGrantedAuthorities(authorities2), authorities2));
+    }
 }

core/src/test/java/org/springframework/security/access/hierarchicalroles/TestHelperTests.java

@@ -14,11 +14,12 @@
 
 package org.springframework.security.access.hierarchicalroles;
 
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 
+import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.commons.collections.CollectionUtils;
 import org.junit.Test;
 import org.springframework.security.core.GrantedAuthority;
 import org.springframework.security.core.authority.AuthorityUtils;
@@ -52,4 +53,91 @@ public class TestHelperTests {
         assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities4, authorities5));
     }
 
+    // SEC-863
+    @Test
+    public void testToListOfAuthorityStrings() {
+        List<GrantedAuthority> authorities1 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B");
+        List<GrantedAuthority> authorities2 = AuthorityUtils.createAuthorityList("ROLE_B", "ROLE_A");
+        List<GrantedAuthority> authorities3 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_C");
+        List<GrantedAuthority> authorities4 = AuthorityUtils.createAuthorityList("ROLE_A");
+        List<GrantedAuthority> authorities5 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_A");
+
+        List<String> authoritiesStrings1 = new ArrayList<String>();
+        authoritiesStrings1.add("ROLE_A");
+        authoritiesStrings1.add("ROLE_B");
+
+        List<String> authoritiesStrings2 = new ArrayList<String>();
+        authoritiesStrings2.add("ROLE_B");
+        authoritiesStrings2.add("ROLE_A");
+
+        List<String> authoritiesStrings3 = new ArrayList<String>();
+        authoritiesStrings3.add("ROLE_A");
+        authoritiesStrings3.add("ROLE_C");
+
+        List<String> authoritiesStrings4 = new ArrayList<String>();
+        authoritiesStrings4.add("ROLE_A");
+
+        List<String> authoritiesStrings5 = new ArrayList<String>();
+        authoritiesStrings5.add("ROLE_A");
+        authoritiesStrings5.add("ROLE_A");
+
+        assertTrue(CollectionUtils.isEqualCollection(
+                HierarchicalRolesTestHelper.toListOfAuthorityStrings(authorities1),	authoritiesStrings1));
+
+        assertTrue(CollectionUtils.isEqualCollection(
+                HierarchicalRolesTestHelper.toListOfAuthorityStrings(authorities2),	authoritiesStrings2));
+
+        assertTrue(CollectionUtils.isEqualCollection(
+                HierarchicalRolesTestHelper.toListOfAuthorityStrings(authorities3),	authoritiesStrings3));
+
+        assertTrue(CollectionUtils.isEqualCollection(
+                HierarchicalRolesTestHelper.toListOfAuthorityStrings(authorities4),	authoritiesStrings4));
+
+        assertTrue(CollectionUtils.isEqualCollection(
+                HierarchicalRolesTestHelper.toListOfAuthorityStrings(authorities5),	authoritiesStrings5));
+    }
+
+    // SEC-863
+    @Test
+    public void testContainTheSameGrantedAuthoritiesCompareByAuthorityString() {
+        List<GrantedAuthority> authorities1 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B");
+        List<GrantedAuthority> authorities2 = AuthorityUtils.createAuthorityList("ROLE_B", "ROLE_A");
+        List<GrantedAuthority> authorities3 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_C");
+        List<GrantedAuthority> authorities4 = AuthorityUtils.createAuthorityList("ROLE_A");
+        List<GrantedAuthority> authorities5 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_A");
+
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(null, null));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities1, authorities1));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities1, authorities2));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities2, authorities1));
+
+        assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(null, authorities1));
+        assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities1, null));
+        assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities1, authorities3));
+        assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities3, authorities1));
+        assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities1, authorities4));
+        assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities4, authorities1));
+        assertFalse(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(authorities4, authorities5));
+    }
+
+    // SEC-863
+    @Test
+    public void testContainTheSameGrantedAuthoritiesCompareByAuthorityStringWithAuthorityLists() {
+        List<GrantedAuthority> authorities1 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A", "ROLE_B");
+        List<GrantedAuthority> authorities2 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B");
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthoritiesCompareByAuthorityString(authorities1, authorities2));
+    }
+
+    // SEC-863
+    @Test
+    public void testCreateAuthorityList() {
+        List<GrantedAuthority> authorities1 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A");
+        assertEquals(authorities1.size(), 1);
+        assertEquals("ROLE_A", authorities1.get(0).getAuthority());
+
+        List<GrantedAuthority> authorities2 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A", "ROLE_C");
+        assertEquals(authorities2.size(), 2);
+        assertEquals("ROLE_A", authorities2.get(0).getAuthority());
+        assertEquals("ROLE_C", authorities2.get(1).getAuthority());
+    }
 }