Infinite loop in role hierarchy resolving

[karelmaxa]

Summary

spring-security allows to define hierarchical roles to offer a convenient means of simplifying the access-control configuration data. When you provide hierarchy definition with cycle that does not include currently resolved role, resolving process will fall into infinite loop.

Actual Behavior

Resolving of role hierarchy is not able to correctly detect cycle when currently resolved role is not included in the cycle. Resolving process falls into infinite loop.

Expected Behavior

CycleInRoleHierarchyException should be thrown.

Version

5.2.0.M3, 5.1.5.RELEASE

Sample

Add provided test fragment into RoleHierarchyImplTests#testCyclesInRoleHierarchy [1].

try {
    roleHierarchyImpl.setHierarchy("ROLE_C > ROLE_B\nROLE_B > ROLE_A\nROLE_A > ROLE_B");
    fail("Cycle in role hierarchy was not detected!");
} catch (CycleInRoleHierarchyException e) {
}

I am probably able to resolve the issue and submit pull request. But i have to say that current implementation of role hierarchy resolving looks little strange. For example this condition [2] is completely useless?.

[1] https://github.com/spring-projects/spring-security/blob/5.1.5.RELEASE/core/src/test/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java#L139
[2] https://github.com/spring-projects/spring-security/blob/5.1.5.RELEASE/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java#L220

[pavelhoral]

I would add that there are strange methods marked with SEC-863 comment in RoleHierarchyImpl. I understand that there was a need to check authorities based on their string representation, but more obvious implementation would be to use HashMap. Such implementation would not require additional methods such as getRolesReachableInOneOrMoreSteps and addReachableRoles And those methods are not even used consistently. You can be sure that when building the hierarchy you are working exclusively with SimpleGrantedAuthority with proper equals implementation. So those two methods are AFAIK useful only in #getReachableGrantedAuthorities method call and are just making the class unnecessarily more complex.

[rwinch]

@pavelhoral Thank you for the Pull Request. A PR would be most welcome. I'd suggest trying to split up the changes into distinct goals as large changes take longer to get merged.

[pavelhoral]

@rwinch I have one question - what is the required java version compatibility? I can see gradle defines 1.8, but official documentation is mentioning 1.5. Can we use lambdas, new collection methods and other goodies from Java 8 (not that we will necessarily use them, but want to know the limits)?

[rwinch]

As of Spring Security 5 the minimum is Java 8. Would you be interested in sending a PR to update the appendix as well?

[ankurpathak]

@rwinch Can i take PR of updating appendix?

[karelmaxa]

We have created pull request #7106 that resolves the original issue. The pull request to resolve appendix issue has not been created yet. Maybe it should be processed in dedicated issue?.

[rwinch]

Yes let's do that as a dedicated issue.