Skip to content

Support Authorities Without Role Prefix in RoleHierarchyImpl Builder #15272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Support Authorities Without Role Prefix in RoleHierarchyImpl Builder #15272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87f42d4
RoleHierarchyImpl builder supports authority to get the roles for.
nielsbasjes Jun 18, 2024
35ded39
RoleHierarchyImpl improve test readability.
nielsbasjes Jun 18, 2024
aa8d0b5
Add Since 6.4. annotation
nielsbasjes Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions ...rc/main/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -282,7 +282,19 @@ 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
* <code>authority</code>
* @since 6.4
*/
public ImpliedRoles authority(String authority) {
Assert.hasText(authority, "authority must not be empty");
return new ImpliedRoles(authority);
}

/**
Expand All @@ -299,7 +311,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;
}

Expand Down
35 changes: 35 additions & 0 deletions ...va/org/springframework/security/access/hierarchicalroles/HierarchicalRolesTestHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -74,4 +76,37 @@ public static List<GrantedAuthority> 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<GrantedAuthority> authorities;
public GivenAuthorities(Collection<GrantedAuthority> authorities) {
this.authorities = authorities;
}
public void theseAuthorities(String... expectedAuthorities) {
List<GrantedAuthority> expectedGrantedAuthorities = createAuthorityList(expectedAuthorities);
assertThat(
containTheSameGrantedAuthoritiesCompareByAuthorityString(authorities, expectedGrantedAuthorities))
.isTrue();
}
}

}
32 changes: 19 additions & 13 deletions ...st/java/org/springframework/security/access/hierarchicalroles/RoleHierarchyImplTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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}.
Expand Down Expand Up @@ -249,29 +250,34 @@ public void testBuilderWithDefaultRolePrefix() {
.implies("B")
.role("B")
.implies("C", "D")
.authority("C")
.implies("E", "F", "B")
.build();
List<GrantedAuthority> flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_A");
List<GrantedAuthority> allAuthorities = AuthorityUtils.createAuthorityList("ROLE_A", "ROLE_B", "ROLE_C",
"ROLE_D");

assertThat(roleHierarchyImpl).isNotNull();
assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities))
.containsExactlyInAnyOrderElementsOf(allAuthorities);
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of now, using .authority("C").implies("E", "F", "B"), means that C will have ROLE_E, ROLE_F and ROLE_B. What if I want to say that authority user:write implies in user:read, user:block, for example?

I'm thinking that when using authority(...) the role prefix should be ignored for the implied authorities too. It should be possible to do .authority("user:write").implies("user:read", "user:delete"). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mindset when writing this was that

  • the authority is a "label" that is provided externally provided (via authentication).
  • the role is a "label" that is internal to use to actually give access to things.

So from your example assuming the 'user:...' are externally provided authorities I see this as a possible way of doing this:

.authority("user:read")  .implies("USER_READ") 
.authority("user:write") .implies("USER_WRITE")
.authority("user:delete").implies("USER_DELETE")

.authority("TeamXYZMembers").implies("USER_READ", "USER_WRITE")

.role("USER_WRITE")      .implies("USER_READ", "USER_DELETE")

This would make all members of "Team XYZ" have the roles USER_READ, USER_WRITE and USER_DELETE.

Primary question: Does this reasoning make sense? It is correct?
If this is correct; Perhaps it should be documented as such?
(if you want I'll put up a separate merge request with such documentation)

The way I normally structure this is that in an application I define roles being groups of users with the same set of tasks like "Administrator", "Manager", "User", etc. Those are then assigned to the user in an external IGA/IAM system (often based on department/job title/...).

Then via the authentication of the specific user; the application receives one or more of these "external roles" as the authorities via the authentication.

From there my application has something like this

.authority("User")         .implies("USER_READ") 
.authority("Manager")      .implies("USER_WRITE")
.authority("Administrator").implies("USER_DELETE", "SHOW_DASHBOARD")

.role("USER_WRITE")       .implies("USER_READ")
.role("USER_DELETE")      .implies("USER_WRITE")

As said; I you want I'm willing to make a first draft on documenting this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to be able to imply an authority then something like this is needed:
.authority("user:write").impliesAuthority("user:read", "user:delete")

A separate function is needed because there is no way to make any distinction between authority or role because they are both just a string.

Also

  • I currently disagree with this approach given how I look at the authority (external) and role (internal).
  • It is not needed to get the desired effect as shown in the example I posted yesterday.

}

@Test
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<GrantedAuthority> flatAuthorities = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A");
List<GrantedAuthority> allAuthorities = AuthorityUtils.createAuthorityList("CUSTOM_PREFIX_A",
"CUSTOM_PREFIX_B");

assertThat(roleHierarchyImpl).isNotNull();
assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities))
.containsExactlyInAnyOrderElementsOf(allAuthorities);
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
Expand Down
2 changes: 2 additions & 0 deletions docs/modules/ROOT/pages/servlet/authorization/architecture.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand All @@ -280,6 +281,7 @@ Xml::
ROLE_ADMIN > ROLE_STAFF
ROLE_STAFF > ROLE_USER
ROLE_USER > ROLE_GUEST
TEAM_ABC > ROLE_STAFF
</value>
</constructor-arg>
</bean>
Expand Down