Skip to content

Fix infinite loop in role hierarchy resolving #7106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 23, 2019
Merged

Fix infinite loop in role hierarchy resolving #7106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d36062
Remove unnecessary authority comparison.
karelmaxa Jul 11, 2019
d3eaef6
Fix infinite loop in role hierarchy resolving
karelmaxa Jul 11, 2019
be0ad67
Make RoleHierarchyImpl internals a bit simpler.
pavelhoral Jul 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 95 additions & 121 deletions ...rc/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2019 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,9 +15,6 @@
*/
package org.springframework.security.access.hierarchicalroles;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
Expand All @@ -34,61 +31,68 @@

/**
* <p>
* This class defines a role hierarchy for use with the UserDetailsServiceWrapper.
* This class defines a role hierarchy for use with various access checking components.
*
* <p>
* Here is an example configuration of a role hierarchy (hint: read the "&gt;" sign as
* "includes"):
* Here is an example configuration of a role hierarchy (hint: read the "&gt;" sign as "includes"):
*
* <pre>
* &lt;property name="hierarchy"&gt;
* &lt;value&gt;
* ROLE_A &gt; ROLE_B
* ROLE_B &gt; ROLE_AUTHENTICATED
* ROLE_AUTHENTICATED &gt; ROLE_UNAUTHENTICATED
* &lt;/value&gt;
* &lt;/property&gt;
* &lt;property name="hierarchy"&gt;
* &lt;value&gt;
* ROLE_A &gt; ROLE_B
* ROLE_B &gt; ROLE_AUTHENTICATED
* ROLE_AUTHENTICATED &gt; ROLE_UNAUTHENTICATED
* &lt;/value&gt;
* &lt;/property&gt;
* </pre>
*
* <p>
* Explanation of the above:<br>
* In effect every user with ROLE_A also has ROLE_B, ROLE_AUTHENTICATED and
* ROLE_UNAUTHENTICATED;<br>
* every user with ROLE_B also has ROLE_AUTHENTICATED and ROLE_UNAUTHENTICATED;<br>
* every user with ROLE_AUTHENTICATED also has ROLE_UNAUTHENTICATED.
* Explanation of the above:
* <ul>
* <li>In effect every user with ROLE_A also has ROLE_B, ROLE_AUTHENTICATED and ROLE_UNAUTHENTICATED;</li>
* <li>every user with ROLE_B also has ROLE_AUTHENTICATED and ROLE_UNAUTHENTICATED;</li>
* <li>every user with ROLE_AUTHENTICATED also has ROLE_UNAUTHENTICATED.</li>
* </ul>
*
* <p>
* Hierarchical Roles will dramatically shorten your access rules (and also make the
* access rules much more elegant).
* Hierarchical Roles will dramatically shorten your access rules (and also make the access rules
* much more elegant).
*
* <p>
* Consider this access rule for Spring Security's RoleVoter (background: every user that
* is authenticated should be able to log out):<br>
* /logout.html=ROLE_A,ROLE_B,ROLE_AUTHENTICATED<br>
* With hierarchical roles this can now be shortened to:<br>
* /logout.html=ROLE_AUTHENTICATED<br>
* In addition to shorter rules this will also make your access rules more readable and
* your intentions clearer.
* Consider this access rule for Spring Security's RoleVoter (background: every user that is
* authenticated should be able to log out):
* <pre>/logout.html=ROLE_A,ROLE_B,ROLE_AUTHENTICATED</pre>
*
* With hierarchical roles this can now be shortened to:
* <pre>/logout.html=ROLE_AUTHENTICATED</pre>
*
* In addition to shorter rules this will also make your access rules more readable and your
* intentions clearer.
*
* @author Michael Mayr
*/
public class RoleHierarchyImpl implements RoleHierarchy {

private static final Log logger = LogFactory.getLog(RoleHierarchyImpl.class);

/**
* Raw hierarchy configuration where each line represents single or multiple level role chain.
*/
private String roleHierarchyStringRepresentation = null;

/**
* rolesReachableInOneStepMap is a Map that under the key of a specific role name
* {@code rolesReachableInOneStepMap} is a Map that under the key of a specific role name
* contains a set of all roles reachable from this role in 1 step
* (i.e. parsed {@link #roleHierarchyStringRepresentation} grouped by the higher role)
*/
private Map<GrantedAuthority, Set<GrantedAuthority>> rolesReachableInOneStepMap = null;
private Map<String, Set<GrantedAuthority>> rolesReachableInOneStepMap = null;

/**
* rolesReachableInOneOrMoreStepsMap is a Map that under the key of a specific role
* {@code rolesReachableInOneOrMoreStepsMap} is a Map that under the key of a specific role
* name contains a set of all roles reachable from this role in 1 or more steps
* (i.e. fully resolved hierarchy from {@link #rolesReachableInOneStepMap})
*/
private Map<GrantedAuthority, Set<GrantedAuthority>> rolesReachableInOneOrMoreStepsMap = null;
private Map<String, Set<GrantedAuthority>> rolesReachableInOneOrMoreStepsMap = null;

/**
* Set the role hierarchy and pre-calculate for every role the set of all reachable
Expand All @@ -102,27 +106,46 @@ public class RoleHierarchyImpl implements RoleHierarchy {
public void setHierarchy(String roleHierarchyStringRepresentation) {
this.roleHierarchyStringRepresentation = roleHierarchyStringRepresentation;

logger.debug("setHierarchy() - The following role hierarchy was set: "
+ roleHierarchyStringRepresentation);
if (logger.isDebugEnabled()) {
logger.debug("setHierarchy() - The following role hierarchy was set: "
+ roleHierarchyStringRepresentation);
}

buildRolesReachableInOneStepMap();
buildRolesReachableInOneOrMoreStepsMap();
}

@Override
public Collection<GrantedAuthority> getReachableGrantedAuthorities(
Collection<? extends GrantedAuthority> authorities) {
if (authorities == null || authorities.isEmpty()) {
return AuthorityUtils.NO_AUTHORITIES;
}

Set<GrantedAuthority> reachableRoles = new HashSet<>();
Set<String> processedNames = new HashSet<>();

for (GrantedAuthority authority : authorities) {
addReachableRoles(reachableRoles, authority);
Set<GrantedAuthority> additionalReachableRoles = getRolesReachableInOneOrMoreSteps(
authority);
if (additionalReachableRoles != null) {
reachableRoles.addAll(additionalReachableRoles);
// Do not process authorities without string representation
if (authority.getAuthority() == null) {
reachableRoles.add(authority);
continue;
}
// Do not process already processed roles
if (!processedNames.add(authority.getAuthority())) {
continue;
}
// Add original authority
reachableRoles.add(authority);
// Add roles reachable in one or more steps
Set<GrantedAuthority> lowerRoles = this.rolesReachableInOneOrMoreStepsMap.get(authority.getAuthority());
if (lowerRoles == null) {
continue; // No hierarchy for the role
}
for (GrantedAuthority role : lowerRoles) {
if (processedNames.add(role.getAuthority())) {
reachableRoles.add(role);
}
}
}

Expand All @@ -132,75 +155,40 @@ public Collection<GrantedAuthority> getReachableGrantedAuthorities(
+ " in zero or more steps.");
}

List<GrantedAuthority> reachableRoleList = new ArrayList<>(
reachableRoles.size());
List<GrantedAuthority> reachableRoleList = new ArrayList<>(reachableRoles.size());
reachableRoleList.addAll(reachableRoles);

return reachableRoleList;
}

// SEC-863
private void addReachableRoles(Set<GrantedAuthority> reachableRoles,
GrantedAuthority authority) {

for (GrantedAuthority testAuthority : reachableRoles) {
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;
}

for (GrantedAuthority testAuthority : this.rolesReachableInOneOrMoreStepsMap
.keySet()) {
String testKey = testAuthority.getAuthority();
if ((testKey != null) && (testKey.equals(authority.getAuthority()))) {
return this.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.
*/
private void buildRolesReachableInOneStepMap() {
this.rolesReachableInOneStepMap = new HashMap<>();
try (BufferedReader bufferedReader = new BufferedReader(
new StringReader(this.roleHierarchyStringRepresentation))) {
for (String readLine; (readLine = bufferedReader.readLine()) != null;) {
String[] roles = readLine.split(" > ");
for (int i = 1; i < roles.length; i++) {
GrantedAuthority higherRole = new SimpleGrantedAuthority(
roles[i - 1].replaceAll("^\\s+|\\s+$", ""));
GrantedAuthority lowerRole = new SimpleGrantedAuthority(roles[i].replaceAll("^\\s+|\\s+$", ""));
Set<GrantedAuthority> rolesReachableInOneStepSet;
if (!this.rolesReachableInOneStepMap.containsKey(higherRole)) {
rolesReachableInOneStepSet = new HashSet<>();
this.rolesReachableInOneStepMap.put(higherRole, rolesReachableInOneStepSet);
} else {
rolesReachableInOneStepSet = this.rolesReachableInOneStepMap.get(higherRole);
}
addReachableRoles(rolesReachableInOneStepSet, lowerRole);
if (logger.isDebugEnabled()) {
logger.debug("buildRolesReachableInOneStepMap() - From role " + higherRole
+ " one can reach role " + lowerRole + " in one step.");
}
for (String line : this.roleHierarchyStringRepresentation.split("\n")) {
// Split on > and trim excessive whitespace
String[] roles = line.trim().split("\\s+>\\s+");

for (int i = 1; i < roles.length; i++) {
String higherRole = roles[i - 1];
GrantedAuthority lowerRole = new SimpleGrantedAuthority(roles[i]);

Set<GrantedAuthority> rolesReachableInOneStepSet;
if (!this.rolesReachableInOneStepMap.containsKey(higherRole)) {
rolesReachableInOneStepSet = new HashSet<>();
this.rolesReachableInOneStepMap.put(higherRole, rolesReachableInOneStepSet);
} else {
rolesReachableInOneStepSet = this.rolesReachableInOneStepMap.get(higherRole);
}
rolesReachableInOneStepSet.add(lowerRole);

if (logger.isDebugEnabled()) {
logger.debug("buildRolesReachableInOneStepMap() - From role " + higherRole
+ " one can reach role " + lowerRole + " in one step.");
}
}
} catch (IOException e) {
throw new IllegalStateException(e);
}
}

Expand All @@ -213,39 +201,25 @@ private void buildRolesReachableInOneStepMap() {
private void buildRolesReachableInOneOrMoreStepsMap() {
this.rolesReachableInOneOrMoreStepsMap = new HashMap<>();
// iterate over all higher roles from rolesReachableInOneStepMap

for (GrantedAuthority role : this.rolesReachableInOneStepMap.keySet()) {
Set<GrantedAuthority> rolesToVisitSet = new HashSet<>();

if (this.rolesReachableInOneStepMap.containsKey(role)) {
rolesToVisitSet.addAll(this.rolesReachableInOneStepMap.get(role));
}

for (String roleName : this.rolesReachableInOneStepMap.keySet()) {
Set<GrantedAuthority> rolesToVisitSet = new HashSet<>(this.rolesReachableInOneStepMap.get(roleName));
Set<GrantedAuthority> visitedRolesSet = new HashSet<>();

while (!rolesToVisitSet.isEmpty()) {
// take a role from the rolesToVisit set
GrantedAuthority aRole = rolesToVisitSet.iterator().next();
rolesToVisitSet.remove(aRole);
addReachableRoles(visitedRolesSet, aRole);
if (this.rolesReachableInOneStepMap.containsKey(aRole)) {
Set<GrantedAuthority> newReachableRoles = this.rolesReachableInOneStepMap
.get(aRole);

// 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
rolesToVisitSet.addAll(newReachableRoles);
}
GrantedAuthority lowerRole = rolesToVisitSet.iterator().next();
rolesToVisitSet.remove(lowerRole);
if (!visitedRolesSet.add(lowerRole) ||
!this.rolesReachableInOneStepMap.containsKey(lowerRole.getAuthority())) {
continue; // Already visited role or role with missing hierarchy
} else if (roleName.equals(lowerRole.getAuthority())) {
throw new CycleInRoleHierarchyException();
}
rolesToVisitSet.addAll(this.rolesReachableInOneStepMap.get(lowerRole.getAuthority()));
}
this.rolesReachableInOneOrMoreStepsMap.put(role, visitedRolesSet);
this.rolesReachableInOneOrMoreStepsMap.put(roleName, visitedRolesSet);

logger.debug("buildRolesReachableInOneOrMoreStepsMap() - From role " + role
logger.debug("buildRolesReachableInOneOrMoreStepsMap() - From role " + roleName
+ " one can reach " + visitedRolesSet + " in one or more steps.");
}

Expand Down
6 changes: 6 additions & 0 deletions ...st/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@ public void testCyclesInRoleHierarchy() {
}
catch (CycleInRoleHierarchyException e) {
}

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) {
}
}

@Test
Expand Down