diff --git a/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java b/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java index e4a5c77fe50..d64ad57e9d7 100755 --- a/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java +++ b/core/src/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2023 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. @@ -21,7 +21,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Set; @@ -77,30 +76,45 @@ * your intentions clearer. * * @author Michael Mayr + * @author Josh Cummings */ 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. + * {@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 */ - private String roleHierarchyStringRepresentation = null; + private Map> rolesReachableInOneOrMoreStepsMap = null; /** - * {@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) + * @deprecated Use {@link RoleHierarchyImpl#fromHierarchy} instead */ - private Map> rolesReachableInOneStepMap = null; + @Deprecated + public RoleHierarchyImpl() { + + } + + private RoleHierarchyImpl(Map> hierarchy) { + this.rolesReachableInOneOrMoreStepsMap = buildRolesReachableInOneOrMoreStepsMap(hierarchy); + } /** - * {@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}) + * Create a role hierarchy instance with the given definition, similar to the + * following: + * + *
+	 *     ROLE_A > ROLE_B
+	 *     ROLE_B > ROLE_AUTHENTICATED
+	 *     ROLE_AUTHENTICATED > ROLE_UNAUTHENTICATED
+	 * 
+ * @param hierarchy the role hierarchy to use + * @return a {@link RoleHierarchyImpl} that uses the given {@code hierarchy} */ - private Map> rolesReachableInOneOrMoreStepsMap = null; + public static RoleHierarchyImpl fromHierarchy(String hierarchy) { + return new RoleHierarchyImpl(buildRolesReachableInOneStepMap(hierarchy)); + } /** * Factory method that creates a {@link Builder} instance with the default role prefix @@ -132,13 +146,15 @@ public static Builder withRolePrefix(String rolePrefix) { * time). During pre-calculation, cycles in role hierarchy are detected and will cause * a CycleInRoleHierarchyException to be thrown. * @param roleHierarchyStringRepresentation - String definition of the role hierarchy. + * @deprecated Use {@link RoleHierarchyImpl#fromHierarchy} instead */ + @Deprecated public void setHierarchy(String roleHierarchyStringRepresentation) { - this.roleHierarchyStringRepresentation = roleHierarchyStringRepresentation; logger.debug(LogMessage.format("setHierarchy() - The following role hierarchy was set: %s", roleHierarchyStringRepresentation)); - buildRolesReachableInOneStepMap(); - buildRolesReachableInOneOrMoreStepsMap(); + Map> hierarchy = buildRolesReachableInOneStepMap( + roleHierarchyStringRepresentation); + this.rolesReachableInOneOrMoreStepsMap = buildRolesReachableInOneOrMoreStepsMap(hierarchy); } @Override @@ -182,21 +198,21 @@ public Collection getReachableGrantedAuthorities( * 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<>(); - for (String line : this.roleHierarchyStringRepresentation.split("\n")) { + private static Map> buildRolesReachableInOneStepMap(String hierarchy) { + Map> rolesReachableInOneStepMap = new HashMap<>(); + for (String line : hierarchy.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 rolesReachableInOneStepSet; - if (!this.rolesReachableInOneStepMap.containsKey(higherRole)) { + if (!rolesReachableInOneStepMap.containsKey(higherRole)) { rolesReachableInOneStepSet = new HashSet<>(); - this.rolesReachableInOneStepMap.put(higherRole, rolesReachableInOneStepSet); + rolesReachableInOneStepMap.put(higherRole, rolesReachableInOneStepSet); } else { - rolesReachableInOneStepSet = this.rolesReachableInOneStepMap.get(higherRole); + rolesReachableInOneStepSet = rolesReachableInOneStepMap.get(higherRole); } rolesReachableInOneStepSet.add(lowerRole); logger.debug(LogMessage.format( @@ -204,6 +220,7 @@ private void buildRolesReachableInOneStepMap() { higherRole, lowerRole)); } } + return rolesReachableInOneStepMap; } /** @@ -212,31 +229,31 @@ private void buildRolesReachableInOneStepMap() { * CycleInRoleHierarchyException if a cycle in the role hierarchy definition is * detected) */ - private void buildRolesReachableInOneOrMoreStepsMap() { - this.rolesReachableInOneOrMoreStepsMap = new HashMap<>(); + private static Map> buildRolesReachableInOneOrMoreStepsMap( + Map> hierarchy) { + Map> rolesReachableInOneOrMoreStepsMap = new HashMap<>(); // iterate over all higher roles from rolesReachableInOneStepMap - for (String roleName : this.rolesReachableInOneStepMap.keySet()) { - Set rolesToVisitSet = new HashSet<>(this.rolesReachableInOneStepMap.get(roleName)); + for (String roleName : hierarchy.keySet()) { + Set rolesToVisitSet = new HashSet<>(hierarchy.get(roleName)); Set visitedRolesSet = new HashSet<>(); while (!rolesToVisitSet.isEmpty()) { // take a role from the rolesToVisit set GrantedAuthority lowerRole = rolesToVisitSet.iterator().next(); rolesToVisitSet.remove(lowerRole); - if (!visitedRolesSet.add(lowerRole) - || !this.rolesReachableInOneStepMap.containsKey(lowerRole.getAuthority())) { + if (!visitedRolesSet.add(lowerRole) || !hierarchy.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())); + rolesToVisitSet.addAll(hierarchy.get(lowerRole.getAuthority())); } - this.rolesReachableInOneOrMoreStepsMap.put(roleName, visitedRolesSet); + rolesReachableInOneOrMoreStepsMap.put(roleName, visitedRolesSet); logger.debug(LogMessage.format( "buildRolesReachableInOneOrMoreStepsMap() - From role %s one can reach %s in one or more steps.", roleName, visitedRolesSet)); } - + return rolesReachableInOneOrMoreStepsMap; } /** @@ -250,7 +267,7 @@ public static final class Builder { private final String rolePrefix; - private final Map> hierarchy; + private final Map> hierarchy; private Builder(String rolePrefix) { this.rolePrefix = rolePrefix; @@ -274,16 +291,13 @@ public ImpliedRoles role(String role) { * @return a {@link RoleHierarchyImpl} */ public RoleHierarchyImpl build() { - String roleHierarchyRepresentation = RoleHierarchyUtils.roleHierarchyFromMap(this.hierarchy); - RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl(); - roleHierarchy.setHierarchy(roleHierarchyRepresentation); - return roleHierarchy; + return new RoleHierarchyImpl(this.hierarchy); } private Builder addHierarchy(String role, String... impliedRoles) { - List withPrefix = new ArrayList<>(); + Set withPrefix = new HashSet<>(); for (String impliedRole : impliedRoles) { - withPrefix.add(this.rolePrefix.concat(impliedRole)); + withPrefix.add(new SimpleGrantedAuthority(this.rolePrefix.concat(impliedRole))); } this.hierarchy.put(this.rolePrefix.concat(role), withPrefix); return this; diff --git a/core/src/test/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java b/core/src/test/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java index 977781231ae..f6a48ea7a7e 100644 --- a/core/src/test/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java +++ b/core/src/test/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java @@ -206,6 +206,42 @@ public void singleLineLargeHierarchy() { .containsExactlyInAnyOrderElementsOf(allAuthorities); } + @Test + public void testFromHierarchyWithTextBlock() { + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.fromHierarchy(""" + ROLE_A > ROLE_B + ROLE_B > ROLE_C + ROLE_B > ROLE_D + """); + List flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_A"); + List allAuthorities = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C", + "ROLE_D"); + + assertThat(roleHierarchyImpl).isNotNull(); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) + .containsExactlyInAnyOrderElementsOf(allAuthorities); + } + + @Test + public void testFromHierarchyNoCycles() { + assertThatNoException().isThrownBy(() -> RoleHierarchyImpl + .fromHierarchy("ROLE_A > ROLE_B\nROLE_A > ROLE_C\nROLE_C > ROLE_D\nROLE_B > ROLE_D")); + } + + @Test + public void testFromHierarchyCycles() { + assertThatExceptionOfType(CycleInRoleHierarchyException.class) + .isThrownBy(() -> RoleHierarchyImpl.fromHierarchy("ROLE_A > ROLE_A")); + assertThatExceptionOfType(CycleInRoleHierarchyException.class) + .isThrownBy(() -> RoleHierarchyImpl.fromHierarchy("ROLE_A > ROLE_B\nROLE_B > ROLE_A")); + assertThatExceptionOfType(CycleInRoleHierarchyException.class) + .isThrownBy(() -> RoleHierarchyImpl.fromHierarchy("ROLE_A > ROLE_B\nROLE_B > ROLE_C\nROLE_C > ROLE_A")); + assertThatExceptionOfType(CycleInRoleHierarchyException.class).isThrownBy(() -> RoleHierarchyImpl + .fromHierarchy("ROLE_A > ROLE_B\nROLE_B > ROLE_C\nROLE_C > ROLE_E\nROLE_E > ROLE_D\nROLE_D > ROLE_B")); + assertThatExceptionOfType(CycleInRoleHierarchyException.class) + .isThrownBy(() -> RoleHierarchyImpl.fromHierarchy("ROLE_C > ROLE_B\nROLE_B > ROLE_A\nROLE_A > ROLE_B")); + } + @Test public void testBuilderWithDefaultRolePrefix() { RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withDefaultRolePrefix() diff --git a/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc b/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc index edbcbc7e72d..be8d5ac8098 100644 --- a/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc +++ b/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc @@ -273,14 +273,14 @@ Xml:: [source,java,role="secondary"] ---- - + class="org.springframework.security.access.hierarchicalroles.RoleHierarchyImpl" factory-method="fromHierarchy"> + ROLE_ADMIN > ROLE_STAFF ROLE_STAFF > ROLE_USER ROLE_USER > ROLE_GUEST - + diff --git a/docs/modules/ROOT/pages/servlet/authorization/method-security.adoc b/docs/modules/ROOT/pages/servlet/authorization/method-security.adoc index 600ef376e80..a6fe865652f 100644 --- a/docs/modules/ROOT/pages/servlet/authorization/method-security.adoc +++ b/docs/modules/ROOT/pages/servlet/authorization/method-security.adoc @@ -233,7 +233,7 @@ Java:: ---- @Bean static RoleHierarchy roleHierarchy() { - return new RoleHierarchyImpl("ROLE_ADMIN > permission:read"); + return RoleHierarchyImpl.fromHierarchy("ROLE_ADMIN > permission:read"); } ---- @@ -244,7 +244,7 @@ Kotlin:: companion object { @Bean fun roleHierarchy(): RoleHierarchy { - return RoleHierarchyImpl("ROLE_ADMIN > permission:read") + return RoleHierarchyImpl.fromHierarchy("ROLE_ADMIN > permission:read") } } ---- @@ -253,7 +253,8 @@ Xml:: + [source,xml,role="secondary"] ---- - + ----