From d4e68a72804835ad067305d5d9c19c662e8eb1f5 Mon Sep 17 00:00:00 2001 From: Toshiaki Maki Date: Sat, 9 Sep 2023 12:10:47 +0900 Subject: [PATCH 1/2] Add RoleHierarchyImpl#of Closes gh-13788 --- .../hierarchicalroles/RoleHierarchyImpl.java | 11 +++++ .../RoleHierarchyImplTests.java | 42 ++++++++++++------- 2 files changed, 39 insertions(+), 14 deletions(-) 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..095aa06936c 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 @@ -125,6 +125,17 @@ public static Builder withRolePrefix(String rolePrefix) { 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 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..9323f16b337 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,8 +40,7 @@ public class RoleHierarchyImplTests { public void testRoleHierarchyWithNullOrEmptyAuthorities() { List authorities0 = null; List authorities1 = new ArrayList<>(); - RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); - roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B"); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of("ROLE_A > ROLE_B"); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(authorities0)).isNotNull(); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(authorities0)).isEmpty(); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(authorities1)).isNotNull(); @@ -53,8 +52,7 @@ public void testSimpleRoleHierarchy() { List authorities0 = AuthorityUtils.createAuthorityList("ROLE_0"); List authorities1 = AuthorityUtils.createAuthorityList("ROLE_A"); List authorities2 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B"); - RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); - roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B"); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of("ROLE_A > ROLE_B"); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authorities0), authorities0)) .isTrue(); @@ -72,8 +70,10 @@ public void testTransitiveRoleHierarchies() { List authorities2 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C"); List authorities3 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C", "ROLE_D"); - RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); - roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_B > ROLE_C"); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of(""" + ROLE_A > ROLE_B + ROLE_B > ROLE_C + """); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authorities1), authorities2)) .isTrue(); @@ -94,8 +94,13 @@ public void testComplexRoleHierarchy() { List authoritiesOutput3 = AuthorityUtils.createAuthorityList("ROLE_C", "ROLE_D"); List authoritiesInput4 = AuthorityUtils.createAuthorityList("ROLE_D"); List authoritiesOutput4 = AuthorityUtils.createAuthorityList("ROLE_D"); - RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); - roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B\nROLE_A > ROLE_C\nROLE_C > ROLE_D\nROLE_B > ROLE_D"); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl + .of(""" + ROLE_A > ROLE_B + ROLE_A > ROLE_C + ROLE_C > ROLE_D + ROLE_B > ROLE_D + """); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthorities( roleHierarchyImpl.getReachableGrantedAuthorities(authoritiesInput1), authoritiesOutput1)) .isTrue(); @@ -138,8 +143,7 @@ public void testSimpleRoleHierarchyWithCustomGrantedAuthorityImplementation() { List authorities0 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_0"); List authorities1 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A"); List authorities2 = HierarchicalRolesTestHelper.createAuthorityList("ROLE_A", "ROLE_B"); - RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); - roleHierarchyImpl.setHierarchy("ROLE_A > ROLE_B"); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of("ROLE_A > ROLE_B"); assertThat(HierarchicalRolesTestHelper.containTheSameGrantedAuthoritiesCompareByAuthorityString( roleHierarchyImpl.getReachableGrantedAuthorities(authorities0), authorities0)) .isTrue(); @@ -157,12 +161,22 @@ public void testWhitespaceRoleHierarchies() { List authorities2 = AuthorityUtils.createAuthorityList("ROLE A", "ROLE B", "ROLE>C"); List authorities3 = AuthorityUtils.createAuthorityList("ROLE A", "ROLE B", "ROLE>C", "ROLE D"); - RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); - roleHierarchyImpl.setHierarchy("ROLE A > ROLE B\nROLE B > ROLE>C"); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.of(""" + ROLE A > ROLE B + ROLE 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(); @@ -200,8 +214,8 @@ public void singleLineLargeHierarchy() { List flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_HIGHEST"); List allAuthorities = AuthorityUtils.createAuthorityList("ROLE_HIGHEST", "ROLE_HIGHER", "ROLE_LOW", "ROLE_LOWER"); - RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl(); - roleHierarchyImpl.setHierarchy("ROLE_HIGHEST > ROLE_HIGHER > ROLE_LOW > ROLE_LOWER"); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl + .of("ROLE_HIGHEST > ROLE_HIGHER > ROLE_LOW > ROLE_LOWER"); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) .containsExactlyInAnyOrderElementsOf(allAuthorities); } From 9ffa54d20819724aae916fd51fee576784074ecc Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Fri, 8 Dec 2023 10:45:26 -0700 Subject: [PATCH 2/2] Polish RoleHierachyImpl#of - Change to #fromHierarchy to match naming convention - Keep existing test methods the same - Deprecate setHierarchy and default constructor - Add private Map constructor - Change Adjust RoleHierarchyBuilder to use Map constructor Issue gh-13788 --- .../hierarchicalroles/RoleHierarchyImpl.java | 101 +++++++++--------- .../RoleHierarchyImplTests.java | 78 +++++++++----- .../servlet/authorization/architecture.adoc | 6 +- .../authorization/method-security.adoc | 7 +- 4 files changed, 109 insertions(+), 83 deletions(-) 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 095aa06936c..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 @@ -125,17 +139,6 @@ public static Builder withRolePrefix(String rolePrefix) { 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 static RoleHierarchyImpl of(String roleHierarchyStringRepresentation) { * 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 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( @@ -215,6 +220,7 @@ private void buildRolesReachableInOneStepMap() { higherRole, lowerRole)); } } + return rolesReachableInOneStepMap; } /** @@ -223,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; } /** @@ -261,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; @@ -285,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 9323f16b337..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 @@ -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 void testSimpleRoleHierarchy() { 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 void testTransitiveRoleHierarchies() { 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 void testComplexRoleHierarchy() { 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 void testSimpleRoleHierarchyWithCustomGrantedAuthorityImplementation() { 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 void testWhitespaceRoleHierarchies() { 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 void singleLineLargeHierarchy() { 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 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"] ---- - + ----