From 87f42d4f2fae2e6caaee41b41446525dc4f378d1 Mon Sep 17 00:00:00 2001 From: Niels Basjes Date: Tue, 18 Jun 2024 10:47:35 +0200 Subject: [PATCH 1/4] RoleHierarchyImpl builder supports authority to get the roles for. Closes gh-15264 Signed-off-by: Niels Basjes --- .../hierarchicalroles/RoleHierarchyImpl.java | 17 ++++++-- .../RoleHierarchyImplTests.java | 39 ++++++++++++++----- .../servlet/authorization/architecture.adoc | 2 + 3 files changed, 45 insertions(+), 13 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 d64ad57e9d7..78295b57eb6 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-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -282,7 +282,18 @@ private Builder(String rolePrefix) { */ public ImpliedRoles role(String role) { Assert.hasText(role, "role must not be empty"); - return new ImpliedRoles(role); + return new ImpliedRoles(this.rolePrefix.concat(role)); + } + + /** + * Creates a new hierarchy branch to define an authority and its child roles. + * @param authority the highest authority in this branch + * @return a {@link ImpliedRoles} to define the child roles for the + * authority + */ + public ImpliedRoles authority(String authority) { + Assert.hasText(authority, "authority must not be empty"); + return new ImpliedRoles(authority); } /** @@ -299,7 +310,7 @@ private Builder addHierarchy(String role, String... impliedRoles) { for (String impliedRole : impliedRoles) { withPrefix.add(new SimpleGrantedAuthority(this.rolePrefix.concat(impliedRole))); } - this.hierarchy.put(this.rolePrefix.concat(role), withPrefix); + this.hierarchy.put(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 f6a48ea7a7e..7c2d2a04f9f 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2024 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. @@ -249,14 +249,23 @@ public void testBuilderWithDefaultRolePrefix() { .implies("B") .role("B") .implies("C", "D") + .authority("C") + .implies("E", "F", "B") .build(); - List flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_A"); - List allAuthorities = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C", + List flatAuthorities1 = AuthorityUtils.createAuthorityList("ROLE_A"); + List allAuthorities1 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C", "ROLE_D"); assertThat(roleHierarchyImpl).isNotNull(); - assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) - .containsExactlyInAnyOrderElementsOf(allAuthorities); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities1)) + .containsExactlyInAnyOrderElementsOf(allAuthorities1); + + List flatAuthorities2 = AuthorityUtils.createAuthorityList("C"); + List allAuthorities2 = AuthorityUtils.createAuthorityList("C", "ROLE_B", "ROLE_C", "ROLE_D", + "ROLE_E", "ROLE_F"); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities2)) + .containsExactlyInAnyOrderElementsOf(allAuthorities2); + } @Test @@ -264,14 +273,24 @@ public void testBuilderWithRolePrefix() { RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withRolePrefix("CUSTOM_PREFIX_") .role("A") .implies("B") + .role("B") + .implies("C", "D") + .authority("C") + .implies("E", "F", "B") .build(); - List flatAuthorities = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A"); - List allAuthorities = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A", - "CUSTOM_PREFIX_B"); + List flatAuthorities1 = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A"); + List allAuthorities1 = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A", + "CUSTOM_PREFIX_B", "CUSTOM_PREFIX_C", "CUSTOM_PREFIX_D"); assertThat(roleHierarchyImpl).isNotNull(); - assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)) - .containsExactlyInAnyOrderElementsOf(allAuthorities); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities1)) + .containsExactlyInAnyOrderElementsOf(allAuthorities1); + + List flatAuthorities2 = AuthorityUtils.createAuthorityList("C"); + List allAuthorities2 = AuthorityUtils.createAuthorityList("C", "CUSTOM_PREFIX_B", + "CUSTOM_PREFIX_C", "CUSTOM_PREFIX_D", "CUSTOM_PREFIX_E", "CUSTOM_PREFIX_F"); + assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities2)) + .containsExactlyInAnyOrderElementsOf(allAuthorities2); } @Test diff --git a/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc b/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc index 490a4c0616f..8ae8da7c4ef 100644 --- a/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc +++ b/docs/modules/ROOT/pages/servlet/authorization/architecture.adoc @@ -257,6 +257,7 @@ static RoleHierarchy roleHierarchy() { .role("ADMIN").implies("STAFF") .role("STAFF").implies("USER") .role("USER").implies("GUEST") + .authority("TEAM_ABC").implies("STAFF") .build(); } @@ -280,6 +281,7 @@ Xml:: ROLE_ADMIN > ROLE_STAFF ROLE_STAFF > ROLE_USER ROLE_USER > ROLE_GUEST + TEAM_ABC > ROLE_STAFF From 35ded39f8b52796f517436ff5886ec8e0e7e8e7c Mon Sep 17 00:00:00 2001 From: Niels Basjes Date: Tue, 18 Jun 2024 10:53:43 +0200 Subject: [PATCH 2/4] RoleHierarchyImpl improve test readability. Closes gh-15264 Signed-off-by: Niels Basjes --- .../HierarchicalRolesTestHelper.java | 35 +++++++++++++++++++ .../RoleHierarchyImplTests.java | 35 ++++++------------- 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/core/src/test/java/org/springframework/security/access/hierarchicalroles/HierarchicalRolesTestHelper.java b/core/src/test/java/org/springframework/security/access/hierarchicalroles/HierarchicalRolesTestHelper.java index b8df1e837e2..193d84f1ee2 100755 --- a/core/src/test/java/org/springframework/security/access/hierarchicalroles/HierarchicalRolesTestHelper.java +++ b/core/src/test/java/org/springframework/security/access/hierarchicalroles/HierarchicalRolesTestHelper.java @@ -24,6 +24,8 @@ import org.springframework.security.core.GrantedAuthority; +import static org.assertj.core.api.Assertions.assertThat; + /** * Test helper class for the hierarchical roles tests. * @@ -74,4 +76,37 @@ public static List createAuthorityList(final String... roles) return authorities; } + // Usage example: + // assertHierarchy(roleHierarchyImpl) + // .givesToAuthorities("C") + // .theseAuthorities("C", "ROLE_B", "ROLE_C", "ROLE_D", "ROLE_E", "ROLE_F"); + + public static AssertingHierarchy assertHierarchy(RoleHierarchyImpl hierarchy) { + return new AssertingHierarchy(hierarchy); + } + + public static class AssertingHierarchy { + RoleHierarchyImpl hierarchy; + public AssertingHierarchy(RoleHierarchyImpl hierarchy) { + assertThat(hierarchy).isNotNull(); + this.hierarchy = hierarchy; + } + public GivenAuthorities givesToAuthorities(String... authorities) { + return new GivenAuthorities(hierarchy.getReachableGrantedAuthorities(createAuthorityList(authorities))); + } + } + + public static class GivenAuthorities { + Collection authorities; + public GivenAuthorities(Collection authorities) { + this.authorities = authorities; + } + public void theseAuthorities(String... expectedAuthorities) { + List expectedGrantedAuthorities = createAuthorityList(expectedAuthorities); + assertThat( + containTheSameGrantedAuthoritiesCompareByAuthorityString(authorities, expectedGrantedAuthorities)) + .isTrue(); + } + } + } 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 7c2d2a04f9f..494173f8fc2 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 @@ -28,6 +28,7 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.springframework.security.access.hierarchicalroles.HierarchicalRolesTestHelper.assertHierarchy; /** * Tests for {@link RoleHierarchyImpl}. @@ -252,20 +253,12 @@ public void testBuilderWithDefaultRolePrefix() { .authority("C") .implies("E", "F", "B") .build(); - List flatAuthorities1 = AuthorityUtils.createAuthorityList("ROLE_A"); - List allAuthorities1 = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C", - "ROLE_D"); - - assertThat(roleHierarchyImpl).isNotNull(); - assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities1)) - .containsExactlyInAnyOrderElementsOf(allAuthorities1); - List flatAuthorities2 = AuthorityUtils.createAuthorityList("C"); - List allAuthorities2 = AuthorityUtils.createAuthorityList("C", "ROLE_B", "ROLE_C", "ROLE_D", - "ROLE_E", "ROLE_F"); - assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities2)) - .containsExactlyInAnyOrderElementsOf(allAuthorities2); + assertHierarchy(roleHierarchyImpl).givesToAuthorities("ROLE_A") + .theseAuthorities("ROLE_A", "ROLE_B", "ROLE_C", "ROLE_D"); + assertHierarchy(roleHierarchyImpl).givesToAuthorities("C") + .theseAuthorities("C", "ROLE_B", "ROLE_C", "ROLE_D", "ROLE_E", "ROLE_F"); } @Test @@ -278,19 +271,13 @@ public void testBuilderWithRolePrefix() { .authority("C") .implies("E", "F", "B") .build(); - List flatAuthorities1 = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A"); - List allAuthorities1 = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A", - "CUSTOM_PREFIX_B", "CUSTOM_PREFIX_C", "CUSTOM_PREFIX_D"); - assertThat(roleHierarchyImpl).isNotNull(); - assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities1)) - .containsExactlyInAnyOrderElementsOf(allAuthorities1); - - List flatAuthorities2 = AuthorityUtils.createAuthorityList("C"); - List allAuthorities2 = AuthorityUtils.createAuthorityList("C", "CUSTOM_PREFIX_B", - "CUSTOM_PREFIX_C", "CUSTOM_PREFIX_D", "CUSTOM_PREFIX_E", "CUSTOM_PREFIX_F"); - assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities2)) - .containsExactlyInAnyOrderElementsOf(allAuthorities2); + assertHierarchy(roleHierarchyImpl).givesToAuthorities("CUSTOM_PREFIX_A") + .theseAuthorities("CUSTOM_PREFIX_A", "CUSTOM_PREFIX_B", "CUSTOM_PREFIX_C", "CUSTOM_PREFIX_D"); + + assertHierarchy(roleHierarchyImpl).givesToAuthorities("C") + .theseAuthorities("C", "CUSTOM_PREFIX_B", "CUSTOM_PREFIX_C", "CUSTOM_PREFIX_D", "CUSTOM_PREFIX_E", + "CUSTOM_PREFIX_F"); } @Test From aa8d0b5954139adf44cf7bd62fff747546071f6a Mon Sep 17 00:00:00 2001 From: Niels Basjes Date: Wed, 26 Jun 2024 11:18:42 +0200 Subject: [PATCH 3/4] Add Since 6.4. annotation Signed-off-by: Niels Basjes --- .../security/access/hierarchicalroles/RoleHierarchyImpl.java | 1 + 1 file changed, 1 insertion(+) 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 78295b57eb6..f7e748f84da 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 @@ -290,6 +290,7 @@ public ImpliedRoles role(String role) { * @param authority the highest authority in this branch * @return a {@link ImpliedRoles} to define the child roles for the * authority + * @since 6.4 */ public ImpliedRoles authority(String authority) { Assert.hasText(authority, "authority must not be empty"); From d20bef6e35ed123ca77ad0659f2057a8c01ba673 Mon Sep 17 00:00:00 2001 From: Niels Basjes Date: Tue, 18 Jun 2024 10:49:14 +0200 Subject: [PATCH 4/4] RoleHierarchyImpl builder supports augmenting the implied roles. Signed-off-by: Niels Basjes --- .../hierarchicalroles/RoleHierarchyImpl.java | 3 +-- .../RoleHierarchyImplTests.java | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 2 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 f7e748f84da..a4288468576 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 @@ -307,11 +307,10 @@ public RoleHierarchyImpl build() { } private Builder addHierarchy(String role, String... impliedRoles) { - Set withPrefix = new HashSet<>(); + Set withPrefix = this.hierarchy.computeIfAbsent(role, r -> new HashSet<>()); for (String impliedRole : impliedRoles) { withPrefix.add(new SimpleGrantedAuthority(this.rolePrefix.concat(impliedRole))); } - this.hierarchy.put(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 494173f8fc2..b2fa1b09a3d 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 @@ -280,6 +280,25 @@ public void testBuilderWithRolePrefix() { "CUSTOM_PREFIX_F"); } + @Test + public void testBuilderWithRepeatedRoleBuilder() { + RoleHierarchyImpl roleHierarchyImpl = RoleHierarchyImpl.withDefaultRolePrefix() + .role("A") + .implies("B") + .role("A") + .implies("C", "D") + .authority("A") + .implies("E") + .authority("A") + .implies("F", "G") + .build(); + + assertHierarchy(roleHierarchyImpl).givesToAuthorities("ROLE_A") + .theseAuthorities("ROLE_A", "ROLE_B", "ROLE_C", "ROLE_D"); + + assertHierarchy(roleHierarchyImpl).givesToAuthorities("A").theseAuthorities("A", "ROLE_E", "ROLE_F", "ROLE_G"); + } + @Test public void testBuilderThrowIllegalArgumentExceptionWhenPrefixRoleNull() { assertThatIllegalArgumentException().isThrownBy(() -> RoleHierarchyImpl.withRolePrefix(null));