From 85127cf719af3819a6de62ca83c70ab69bd5414e Mon Sep 17 00:00:00 2001 From: Federico Herrera Date: Sun, 26 Nov 2023 16:02:43 -0300 Subject: [PATCH 1/2] Add RoleHierarchyImpl.Builder Closes gh-13300 --- .../hierarchicalroles/RoleHierarchyImpl.java | 109 ++++++++++++++++++ .../RoleHierarchyImplTests.java | 65 +++++++++++ 2 files changed, 174 insertions(+) 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 e0988445fef..30ea1db90e8 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 @@ -20,8 +20,11 @@ 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; +import java.util.stream.Stream; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -30,6 +33,9 @@ import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.util.Assert; + +import static org.springframework.security.access.hierarchicalroles.RoleHierarchyUtils.roleHierarchyFromMap; /** *

@@ -99,6 +105,35 @@ public class RoleHierarchyImpl implements RoleHierarchy { */ private Map> rolesReachableInOneOrMoreStepsMap = null; + /** + * Factory method that creates a {@link Builder} instance with the default role prefix + * "ROLE_" + * @return a {@link Builder} instance with the default role prefix "ROLE_" + */ + public static Builder withDefaultRolePrefix() { + return withRolePrefix("ROLE_"); + } + + /** + * Factory method that creates a {@link Builder} instance with the specified role + * prefix. + * @param rolePrefix the prefix to be used for the roles in the hierarchy. + * @return a new {@link Builder} instance with the specified role prefix + * @throws IllegalArgumentException if the provided role prefix is null + */ + public static Builder withRolePrefix(String rolePrefix) { + Assert.notNull(rolePrefix, "rolePrefix must not be null"); + return new Builder(rolePrefix); + } + + /** + * Factory method that creates a {@link Builder} instance with no role prefix. + * @return a new {@link Builder} instance with no role prefix. + */ + public static Builder withNoRolePrefix() { + return withRolePrefix(""); + } + /** * 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 @@ -213,4 +248,78 @@ else if (roleName.equals(lowerRole.getAuthority())) { } + /** + * Builder class for constructing a {@link RoleHierarchyImpl} based on a hierarchical + * role structure. + * + * @author Federico Herrera + * @since 6.3 + */ + public static final class Builder { + + private final String rolePrefix; + + private final Map> roleBranches; + + private Builder(String rolePrefix) { + this.rolePrefix = rolePrefix; + this.roleBranches = new LinkedHashMap<>(); + } + + /** + * Creates a new hierarchy branch to define a role and its child roles. + * @param role the highest role in this branch + * @return a {@link RoleBranchBuilder} to define the child roles for the + * role + */ + public RoleBranchBuilder role(String role) { + Assert.hasText(role, "role must not be empty"); + return new RoleBranchBuilder(this, rolePrefix.concat(role)); + } + + /** + * Builds and returns a {@link RoleHierarchyImpl} describing the defined role + * hierarchy. + * @return a {@link RoleHierarchyImpl} + */ + public RoleHierarchyImpl build() { + String roleHierarchyRepresentation = roleHierarchyFromMap(roleBranches); + RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl(); + roleHierarchy.setHierarchy(roleHierarchyRepresentation); + return roleHierarchy; + } + + /** + * Builder class for constructing child roles within a role hierarchy branch. + */ + public static final class RoleBranchBuilder { + + private final Builder parentBuilder; + + private final String role; + + private RoleBranchBuilder(Builder parentBuilder, String role) { + this.parentBuilder = parentBuilder; + this.role = role; + } + + /** + * Specifies implied role(s) for the current role in the hierarchy. + * @param impliedRoles role name(s) implied by the role. + * @return the same {@link Builder} instance + * @throws IllegalArgumentException if impliedRoles is null, + * empty or contains any null element. + */ + public Builder implies(String... impliedRoles) { + Assert.notEmpty(impliedRoles, "at least one implied role must be provided"); + Assert.noNullElements(impliedRoles, "implied role name(s) cannot be empty"); + parentBuilder.roleBranches.put(role, + Stream.of(impliedRoles).map(parentBuilder.rolePrefix::concat).toList()); + return parentBuilder; + } + + } + + } + } 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 c95024d6867..4e03b40d0f8 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 @@ -26,6 +26,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatNoException; /** @@ -205,4 +206,68 @@ public void singleLineLargeHierarchy() { .containsExactlyInAnyOrderElementsOf(allAuthorities); } + @Test + public void testBuilderWithDefaultRolePrefix() { + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withDefaultRolePrefix().role("A").implies("B").build(); + List flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_A"); + List allAuthorities = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B"); + + assertThat(roleHierarchyImpl).isNotNull(); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) + .containsExactlyInAnyOrderElementsOf(allAuthorities); + } + + @Test + public void testBuilderWithRolePrefix() { + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withRolePrefix("CUSTOM_PREFIX_") + .role("A") + .implies("B") + .build(); + List flatAuthorities = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A"); + List allAuthorities = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A", + "CUSTOM_PREFIX_B"); + + assertThat(roleHierarchyImpl).isNotNull(); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) + .containsExactlyInAnyOrderElementsOf(allAuthorities); + } + + @Test + public void testBuilderWithNoRolePrefix() { + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withNoRolePrefix().role("A").implies("B").build(); + List flatAuthorities = AuthorityUtils.createAuthorityList("A"); + List allAuthorities = AuthorityUtils.createAuthorityList("A", "B"); + + assertThat(roleHierarchyImpl).isNotNull(); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) + .containsExactlyInAnyOrderElementsOf(allAuthorities); + } + + @Test + public void testBuilderThrowIllegalArgumentExceptionWhenPrefixRoleNull() { + assertThatIllegalArgumentException().isThrownBy(() -> RoleHierarchyImpl.withRolePrefix(null)); + } + + @Test + public void testBuilderThrowIllegalArgumentExceptionWhenRoleEmpty() { + assertThatIllegalArgumentException().isThrownBy(() -> RoleHierarchyImpl.withDefaultRolePrefix().role("")); + } + + @Test + public void testBuilderThrowIllegalArgumentExceptionWhenRoleNull() { + assertThatIllegalArgumentException().isThrownBy(() -> RoleHierarchyImpl.withDefaultRolePrefix().role(null)); + } + + @Test + public void testBuilderThrowIllegalArgumentExceptionWhenImpliedRolesNull() { + assertThatIllegalArgumentException() + .isThrownBy(() -> RoleHierarchyImpl.withDefaultRolePrefix().role("A").implies((String) null)); + } + + @Test + public void testBuilderThrowIllegalArgumentExceptionWhenImpliedRolesEmpty() { + assertThatIllegalArgumentException() + .isThrownBy(() -> RoleHierarchyImpl.withDefaultRolePrefix().role("A").implies()); + } + } From be983789301f618eed9e00ba1491b27988eb3738 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Wed, 6 Dec 2023 14:33:59 -0700 Subject: [PATCH 2/2] Polish RoleHierarchyImpl#Builder - Added documentation - Removed withNoRolePrefix for now; let's see how folks use the minimal API first - Adjusted class hierarchy to match AuthorizeHttpRequests more closely - Adjusted to match Spring Security style guide - Added needed @since attributes Issue gh-13300 --- .../hierarchicalroles/RoleHierarchyImpl.java | 45 +++++++++---------- .../RoleHierarchyImplTests.java | 21 ++++----- .../servlet/authorization/architecture.adoc | 9 ++-- 3 files changed, 32 insertions(+), 43 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 30ea1db90e8..e4a5c77fe50 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 @@ -24,7 +24,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Stream; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -35,8 +34,6 @@ import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.util.Assert; -import static org.springframework.security.access.hierarchicalroles.RoleHierarchyUtils.roleHierarchyFromMap; - /** *

* This class defines a role hierarchy for use with various access checking components. @@ -109,6 +106,7 @@ public class RoleHierarchyImpl implements RoleHierarchy { * Factory method that creates a {@link Builder} instance with the default role prefix * "ROLE_" * @return a {@link Builder} instance with the default role prefix "ROLE_" + * @since 6.3 */ public static Builder withDefaultRolePrefix() { return withRolePrefix("ROLE_"); @@ -120,20 +118,13 @@ public static Builder withDefaultRolePrefix() { * @param rolePrefix the prefix to be used for the roles in the hierarchy. * @return a new {@link Builder} instance with the specified role prefix * @throws IllegalArgumentException if the provided role prefix is null + * @since 6.3 */ public static Builder withRolePrefix(String rolePrefix) { Assert.notNull(rolePrefix, "rolePrefix must not be null"); return new Builder(rolePrefix); } - /** - * Factory method that creates a {@link Builder} instance with no role prefix. - * @return a new {@link Builder} instance with no role prefix. - */ - public static Builder withNoRolePrefix() { - return withRolePrefix(""); - } - /** * 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 @@ -259,22 +250,22 @@ public static final class Builder { private final String rolePrefix; - private final Map> roleBranches; + private final Map> hierarchy; private Builder(String rolePrefix) { this.rolePrefix = rolePrefix; - this.roleBranches = new LinkedHashMap<>(); + this.hierarchy = new LinkedHashMap<>(); } /** * Creates a new hierarchy branch to define a role and its child roles. * @param role the highest role in this branch - * @return a {@link RoleBranchBuilder} to define the child roles for the + * @return a {@link ImpliedRoles} to define the child roles for the * role */ - public RoleBranchBuilder role(String role) { + public ImpliedRoles role(String role) { Assert.hasText(role, "role must not be empty"); - return new RoleBranchBuilder(this, rolePrefix.concat(role)); + return new ImpliedRoles(role); } /** @@ -283,23 +274,29 @@ public RoleBranchBuilder role(String role) { * @return a {@link RoleHierarchyImpl} */ public RoleHierarchyImpl build() { - String roleHierarchyRepresentation = roleHierarchyFromMap(roleBranches); + String roleHierarchyRepresentation = RoleHierarchyUtils.roleHierarchyFromMap(this.hierarchy); RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl(); roleHierarchy.setHierarchy(roleHierarchyRepresentation); return roleHierarchy; } + private Builder addHierarchy(String role, String... impliedRoles) { + List withPrefix = new ArrayList<>(); + for (String impliedRole : impliedRoles) { + withPrefix.add(this.rolePrefix.concat(impliedRole)); + } + this.hierarchy.put(this.rolePrefix.concat(role), withPrefix); + return this; + } + /** * Builder class for constructing child roles within a role hierarchy branch. */ - public static final class RoleBranchBuilder { - - private final Builder parentBuilder; + public final class ImpliedRoles { private final String role; - private RoleBranchBuilder(Builder parentBuilder, String role) { - this.parentBuilder = parentBuilder; + private ImpliedRoles(String role) { this.role = role; } @@ -313,9 +310,7 @@ private RoleBranchBuilder(Builder parentBuilder, String role) { public Builder implies(String... impliedRoles) { Assert.notEmpty(impliedRoles, "at least one implied role must be provided"); Assert.noNullElements(impliedRoles, "implied role name(s) cannot be empty"); - parentBuilder.roleBranches.put(role, - Stream.of(impliedRoles).map(parentBuilder.rolePrefix::concat).toList()); - return parentBuilder; + return Builder.this.addHierarchy(this.role, impliedRoles); } } 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 4e03b40d0f8..977781231ae 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 @@ -208,9 +208,15 @@ public void singleLineLargeHierarchy() { @Test public void testBuilderWithDefaultRolePrefix() { - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withDefaultRolePrefix().role("A").implies("B").build(); + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withDefaultRolePrefix() + .role("A") + .implies("B") + .role("B") + .implies("C", "D") + .build(); List flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_A"); - List allAuthorities = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B"); + List allAuthorities = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C", + "ROLE_D"); assertThat(roleHierarchyImpl).isNotNull(); assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) @@ -232,17 +238,6 @@ public void testBuilderWithRolePrefix() { .containsExactlyInAnyOrderElementsOf(allAuthorities); } - @Test - public void testBuilderWithNoRolePrefix() { - RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withNoRolePrefix().role("A").implies("B").build(); - List flatAuthorities = AuthorityUtils.createAuthorityList("A"); - List allAuthorities = AuthorityUtils.createAuthorityList("A", "B"); - - assertThat(roleHierarchyImpl).isNotNull(); - assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) - .containsExactlyInAnyOrderElementsOf(allAuthorities); - } - @Test public void testBuilderThrowIllegalArgumentExceptionWhenPrefixRoleNull() { assertThatIllegalArgumentException().isThrownBy(() -> RoleHierarchyImpl.withRolePrefix(null)); diff --git a/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc b/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc index adda11bb476..b287ecad833 100644 --- a/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc +++ b/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc @@ -253,11 +253,10 @@ Java:: ---- @Bean static RoleHierarchy roleHierarchy() { - RoleHierarchyImpl hierarchy = new RoleHierarchyImpl(); - hierarchy.setHierarchy("ROLE_ADMIN > ROLE_STAFF\n" + - "ROLE_STAFF > ROLE_USER\n" + - "ROLE_USER > ROLE_GUEST"); - return hierarchy; + return RoleHierarchyImpl.withDefaultRolePrefix() + .role("ADMIN").implies("STAFF") + .role("STAFF").implies("USER") + .role("USER").implies("GUEST"); } // and, if using method security also add