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 095aa06936..d64ad57e9d 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.Collection; 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,31 +76,46 @@ import org.springframework.util.Assert; * 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. - */ - private String roleHierarchyStringRepresentation = 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) - */ - private Map> rolesReachableInOneStepMap = null; - /** * {@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> rolesReachableInOneOrMoreStepsMap = null; + /** + * @deprecated Use {@link RoleHierarchyImpl#fromHierarchy} instead + */ + @Deprecated + public RoleHierarchyImpl() { + + } + + private RoleHierarchyImpl(Map> hierarchy) { + this.rolesReachableInOneOrMoreStepsMap = buildRolesReachableInOneOrMoreStepsMap(hierarchy); + } + + /** + * 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} + */ + public static RoleHierarchyImpl fromHierarchy(String hierarchy) { + return new RoleHierarchyImpl(buildRolesReachableInOneStepMap(hierarchy)); + } + /** * Factory method that creates a {@link Builder} instance with the default role prefix * "ROLE_" @@ -125,17 +139,6 @@ public class RoleHierarchyImpl implements RoleHierarchy { return new Builder(rolePrefix); } - /** - * Create a role hierarchy instance with the given definition - * @param roleHierarchyStringRepresentation String definition of the role hierarchy. - * @return role hierarchy instance - */ - public static RoleHierarchyImpl of(String roleHierarchyStringRepresentation) { - RoleHierarchyImpl hierarchy = new RoleHierarchyImpl(); - hierarchy.setHierarchy(roleHierarchyStringRepresentation); - return hierarchy; - } - /** * Set the role hierarchy and pre-calculate for every role the set of all reachable * roles, i.e. all roles lower in the hierarchy of every given role. Pre-calculation @@ -143,13 +146,15 @@ public class RoleHierarchyImpl implements RoleHierarchy { * 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 @@ -193,21 +198,21 @@ public class RoleHierarchyImpl implements RoleHierarchy { * 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( @@ -215,6 +220,7 @@ public class RoleHierarchyImpl implements RoleHierarchy { higherRole, lowerRole)); } } + return rolesReachableInOneStepMap; } /** @@ -223,31 +229,31 @@ public class RoleHierarchyImpl implements RoleHierarchy { * 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; } /** @@ -261,7 +267,7 @@ public class RoleHierarchyImpl implements RoleHierarchy { private final String rolePrefix; - private final Map> hierarchy; + private final Map> hierarchy; private Builder(String rolePrefix) { this.rolePrefix = rolePrefix; @@ -285,16 +291,13 @@ public class RoleHierarchyImpl implements RoleHierarchy { * @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 9323f16b33..f6a48ea7a7 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 @@ -40,7 +40,8 @@ public class RoleHierarchyImplTests { public void testRoleHierarchyWithNullOrEmptyAuthorities() { List authorities0 = null; List authorities1 = new ArrayList<>(); - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of("ROLE_A > ROLE_B"); + RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); + roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B"); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(authorities0)).isNotNull(); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(authorities0)).isEmpty(); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(authorities1)).isNotNull(); @@ -52,7 +53,8 @@ public class RoleHierarchyImplTests { List authorities0 = AuthorityUtils.createAuthorityList("ROLE_0"); List authorities1 = AuthorityUtils.createAuthorityList("ROLE_A"); List authorities2 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B"); - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of("ROLE_A > ROLE_B"); + RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); + roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B"); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authorities0), authorities0)) .isTrue(); @@ -70,10 +72,8 @@ public class RoleHierarchyImplTests { List authorities2 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C"); List authorities3 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C", "ROLE_D"); - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of(""" - ROLE_A > ROLE_B - ROLE_B > ROLE_C - """); + RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); + roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_B > ROLE_C"); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authorities1), authorities2)) .isTrue(); @@ -94,13 +94,8 @@ public class RoleHierarchyImplTests { List authoritiesOutput3 = AuthorityUtils.createAuthorityList("ROLE_C", "ROLE_D"); List authoritiesInput4 = AuthorityUtils.createAuthorityList("ROLE_D"); List authoritiesOutput4 = AuthorityUtils.createAuthorityList("ROLE_D"); - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl - .of(""" - ROLE_A > ROLE_B - ROLE_A > ROLE_C - ROLE_C > ROLE_D - ROLE_B > ROLE_D - """); + RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); + roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_A > ROLE_C\nROLE_C > ROLE_D\nROLE_B > ROLE_D"); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authoritiesInput1), authoritiesOutput1)) .isTrue(); @@ -143,7 +138,8 @@ public class RoleHierarchyImplTests { List authorities0 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_0"); List authorities1 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A"); List authorities2 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A", "ROLE_B"); - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of("ROLE_A > ROLE_B"); + RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); + roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B"); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthoritiesCompareByAuthorityString( roleHierarchyImpl.getReachableGrantedAuthorities(authorities0), authorities0)) .isTrue(); @@ -161,22 +157,12 @@ public class RoleHierarchyImplTests { List authorities2 = AuthorityUtils.createAuthorityList("ROLE A", "ROLE B", "ROLE>C"); List authorities3 = AuthorityUtils.createAuthorityList("ROLE A", "ROLE B", "ROLE>C", "ROLE D"); - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of(""" - ROLE A > ROLE B - ROLE B > ROLE>C - """); + RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); + roleHierarchyImpl.setHierarchy("ROLE A > ROLE B\nROLE B > ROLE>C"); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authorities1), authorities2)) .isTrue(); roleHierarchyImpl.setHierarchy("ROLE A > ROLE B\nROLE B > ROLE>C\nROLE>C > ROLE D"); - assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( - roleHierarchyImpl.getReachableGrantedAuthorities(authorities1), authorities2)) - .isTrue(); - roleHierarchyImpl.setHierarchy(""" - ROLE A > ROLE B - ROLE B > ROLE>C - ROLE>C > ROLE D - """); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authorities1), authorities3)) .isTrue(); @@ -214,12 +200,48 @@ public class RoleHierarchyImplTests { List flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_HIGHEST"); List allAuthorities = AuthorityUtils.createAuthorityList("ROLE_HIGHEST", "ROLE_HIGHER", "ROLE_LOW", "ROLE_LOWER"); - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl - .of("ROLE_HIGHEST > ROLE_HIGHER > ROLE_LOW > ROLE_LOWER"); + RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); + roleHierarchyImpl.setHierarchy("ROLE_HIGHEST > ROLE_HIGHER > ROLE_LOW > ROLE_LOWER"); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) .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 edbcbc7e72..be8d5ac809 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 600ef376e8..a6fe865652 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"] ---- - + ----