SEC-232: Additional updates to hierachical roles code from contributor.

core/src/main/java/org/acegisecurity/userdetails/hierarchicalroles/RoleHierarchyImpl.java

@@ -14,21 +14,16 @@
 
 package org.acegisecurity.userdetails.hierarchicalroles;
 
+
 import org.acegisecurity.GrantedAuthority;
 import org.acegisecurity.GrantedAuthorityImpl;
-
-import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.Map;
-import java.util.Set;
-import java.util.Arrays;
-import java.util.regex.Pattern;
 import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import java.util.*;
 
 /**
  * <p>
@@ -183,8 +178,8 @@ public class RoleHierarchyImpl implements RoleHierarchy {
                 if (rolesReachableInOneStepMap.containsKey(aRole)) {
                     Set newReachableRoles = (Set) rolesReachableInOneStepMap.get(aRole);
 
-                    if (CollectionUtils.containsAny(rolesToVisitSet, newReachableRoles)
-                            || CollectionUtils.containsAny(visitedRolesSet, newReachableRoles)) {
+                    // definition of a cycle: you can reach the role you are starting from
+                    if (rolesToVisitSet.contains(role) || visitedRolesSet.contains(role)) {
                         throw new CycleInRoleHierarchyException();
                     } else {
                          // no cycle
@@ -200,4 +195,4 @@ public class RoleHierarchyImpl implements RoleHierarchy {
 
     }
 
-}
+}

core/src/test/java/org/acegisecurity/userdetails/hierarchicalroles/RoleHierarchyImplTests.java

@@ -34,10 +34,6 @@ public class RoleHierarchyImplTests extends TestCase {
         super(testCaseName);
     }
 
-    public static void main(String[] args) {
-        TestRunner.run(RoleHierarchyImplTests.class);
-    }
-
     public void testSimpleRoleHierarchy() {
         GrantedAuthority[] authorities0 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_0") };
         GrantedAuthority[] authorities1 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_A") };
@@ -66,6 +62,26 @@ public class RoleHierarchyImplTests extends TestCase {
         assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(roleHierarchyImpl.getReachableGrantedAuthorities(authorities1), authorities3));
     }
 
+    public void testComplexRoleHierarchy() {
+        GrantedAuthority[] authoritiesInput1 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_A") };
+        GrantedAuthority[] authoritiesOutput1 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_A"), new GrantedAuthorityImpl("ROLE_B"), new GrantedAuthorityImpl("ROLE_C"),
+                                                                         new GrantedAuthorityImpl("ROLE_D") };
+        GrantedAuthority[] authoritiesInput2 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_B") };
+        GrantedAuthority[] authoritiesOutput2 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_B"), new GrantedAuthorityImpl("ROLE_D") };
+        GrantedAuthority[] authoritiesInput3 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_C") };
+        GrantedAuthority[] authoritiesOutput3 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_C"), new GrantedAuthorityImpl("ROLE_D") };
+        GrantedAuthority[] authoritiesInput4 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_D") };
+        GrantedAuthority[] authoritiesOutput4 = new GrantedAuthority[] { new GrantedAuthorityImpl("ROLE_D") };
+
+        RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl();
+        roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_A > ROLE_C\nROLE_C > ROLE_D\nROLE_B > ROLE_D");
+
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(roleHierarchyImpl.getReachableGrantedAuthorities(authoritiesInput1), authoritiesOutput1));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(roleHierarchyImpl.getReachableGrantedAuthorities(authoritiesInput2), authoritiesOutput2));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(roleHierarchyImpl.getReachableGrantedAuthorities(authoritiesInput3), authoritiesOutput3));
+        assertTrue(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities(roleHierarchyImpl.getReachableGrantedAuthorities(authoritiesInput4), authoritiesOutput4));
+    }
+
     public void testCyclesInRoleHierarchy() {
         RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl();
 
@@ -83,6 +99,21 @@ public class RoleHierarchyImplTests extends TestCase {
             roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_B > ROLE_C\nROLE_C > ROLE_A");
             fail("Cycle in role hierarchy was not detected!");
         } catch (CycleInRoleHierarchyException e) {}
+
+        try {
+            roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_B > ROLE_C\nROLE_C > ROLE_E\nROLE_E > ROLE_D\nROLE_D > ROLE_B");
+            fail("Cycle in role hierarchy was not detected!");
+        } catch (CycleInRoleHierarchyException e) {}
+    }
+
+    public void testNoCyclesInRoleHierarchy() {
+        RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl();
+
+        try {
+            roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_A > ROLE_C\nROLE_C > ROLE_D\nROLE_B > ROLE_D");
+        } catch (CycleInRoleHierarchyException e) {
+            fail("A cycle in role hierarchy was incorrectly detected!");
+        }
     }
 
 }