Skip to content

Easier way to define Role Hierarchy #13300

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.

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
bwgjoseph opened this issue Jun 9, 2023 · 7 comments · Fixed by #14196
Closed

Easier way to define Role Hierarchy #13300

bwgjoseph opened this issue Jun 9, 2023 · 7 comments · Fixed by #14196
Labels
in: core An issue in spring-security-core status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement

Comments

@bwgjoseph
Copy link

Disclaimer: I'm hesitant to create this issue for sometime as this is quite subjective, but I thought that I still want to make the suggestion to see if there's any other ways to achieve it (that I've perhaps missed out on)


Currently, if I want to define a RoleHierarchy, I can do it like

@Bean
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();
    String roleHierarchyFromMap = "ROLE_ADMIN > ROLE_STAFF \n ROLE_STAFF > ROLE_USER \n ROLE_STAFF > ROLE_GUEST";
    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

The result would be something like

2023-06-09 21:17:39.881 DEBUG 20580 --- [  restartedMain] o.s.s.a.h.RoleHierarchyImpl              : setHierarchy() - The following role hierarchy was set: ROLE_ADMIN > ROLE_STAFF
 ROLE_STAFF > ROLE_USER
 ROLE_STAFF > ROLE_GUEST
2023-06-09 21:17:39.884 DEBUG 20580 --- [  restartedMain] o.s.s.a.h.RoleHierarchyImpl              : buildRolesReachableInOneStepMap() - From role ROLE_ADMIN one 
can reach role ROLE_STAFF in one step.
2023-06-09 21:17:39.885 DEBUG 20580 --- [  restartedMain] o.s.s.a.h.RoleHierarchyImpl              : buildRolesReachableInOneStepMap() - From role ROLE_STAFF one 
can reach role ROLE_USER in one step.
2023-06-09 21:17:39.885 DEBUG 20580 --- [  restartedMain] o.s.s.a.h.RoleHierarchyImpl              : buildRolesReachableInOneStepMap() - From role ROLE_STAFF one 
can reach role ROLE_GUEST in one step.
2023-06-09 21:17:39.886 DEBUG 20580 --- [  restartedMain] o.s.s.a.h.RoleHierarchyImpl              : buildRolesReachableInOneOrMoreStepsMap() - From role ROLE_STAFF one can reach [ROLE_USER, ROLE_GUEST] in one or more steps.
2023-06-09 21:17:39.888 DEBUG 20580 --- [  restartedMain] o.s.s.a.h.RoleHierarchyImpl              : buildRolesReachableInOneOrMoreStepsMap() - From role ROLE_ADMIN one can reach [ROLE_USER, ROLE_STAFF, ROLE_GUEST] in one or more steps.

Which is correct, but it is more prone to mistake (with more complex setup), and "uglier" to declare this way via \n

Luckily, for most use case, I can replace it with RoleHierarchyUtils.roleHierarchyFromMap but not for this case where there's a repeated key. And since we are using Map, key has to be unique.

@Bean
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();
    Map<String, List<String>> roleHierarchyMap = new HashMap<>();

    roleHierarchyMap.put("ROLE_ADMIN", List.of("ROLE_STAFF"));
    roleHierarchyMap.put("ROLE_STAFF", List.of("ROLE_USER"));
    // Because it is Map, I can't quite define this
    // roleHierarchyMap.put("ROLE_STAFF", List.of("ROLE_GUEST"));
    String roleHierarchyFromMap = RoleHierarchyUtils.roleHierarchyFromMap(roleHierarchyMap);

    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

Note that for this use case, ROLE_STAFF inherit ROLE_USER AND inherit ROLE_GUEST but ROLE_USER does not inherit ROLE_GUEST. Hence, I can't simply do the following

@Bean
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();
    Map<String, List<String>> roleHierarchyMap = new HashMap<>();

    roleHierarchyMap.put("ROLE_ADMIN", List.of("ROLE_STAFF"));
    roleHierarchyMap.put("ROLE_STAFF", List.of("ROLE_USER", "ROLE_GUEST"));
    String roleHierarchyFromMap = RoleHierarchyUtils.roleHierarchyFromMap(roleHierarchyMap);

    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

Which is a limitation for this use case.

I have another way to declare. Since Java now supports TextBlock, I can still define as

@Bean
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();
    Map<String, List<String>> roleHierarchyMap = new HashMap<>();

    String roleHierarchyFromMap = """
            ROLE_ADMIN > ROLE_STAFF
            ROLE_STAFF > ROLE_USER
            ROLE_STAFF > ROLE_GUEST
            """;

    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

Which still works. But I do still like the way that I use Map to define my hierarchy, which is still clearer, better and straightforward IMO. And I think it's easier to support the declaration via application.yaml and construct through @ConfigurationProperties and set it.

I wonder if there's any way to support my use case which make the declaration easier.

Thanks!

@bwgjoseph bwgjoseph added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jun 9, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Jun 12, 2023

Nice idea, @bwgjoseph. There may be a way with regular expressions to make this work. For example, if it meets this regex, then process it the new way; otherwise, process it the old way. Are you able to provide a PR?

@jzheaux jzheaux added in: core An issue in spring-security-core status: ideal-for-contribution An issue that we actively are looking for someone to help us with and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 12, 2023
@bwgjoseph
Copy link
Author

bwgjoseph commented Jun 14, 2023

Hello! I'm not quite sure what you mean by using regular expression to make it work, can you expand on that?

I don't mind working on a PR if there's a way to provide an easier way to declare the hierarchy.

@AugustoRavazoli
Copy link

AugustoRavazoli commented Jun 21, 2023

What about this way?

String roles = RoleHierarchyUtils.roleHierarchyBuilder("ROLE") // sets the prefix, optional
  .role("ADMIN").inherits("STAFF")
  .role("STAFF").inherits("USER", "GUEST")
  .build();

Then the output will be the same as:

String roleHierarchyFromMap = """
  ROLE_ADMIN > ROLE_STAFF
  ROLE_STAFF > ROLE_USER
  ROLE_STAFF > ROLE_GUEST
  """;

It's more readable and less boilerplate, I can send a PR if needed.

@bwgjoseph
Copy link
Author

Hi @AugustoRavazoli,

I'm not sure if that's straight-forward or satisfy all use case as well.

String roles = RoleHierarchyUtils.roleHierarchyBuilder("ROLE") // sets the prefix, optional
  .role("ADMIN").inherits("STAFF")
  .role("STAFF").inherits("USER", "GUEST")
  .build();

Based on your example, does USER inherits GUEST?

Based on the textblock, we know it's a no, but based on your proposed solution, we can't quite tell easily

@FdHerrera
Copy link

Hello! Just created this PR to address this issue. While implementing found out that you can do something like the following:

@Bean
public RoleHierarchy roleHierarchy() {
    RoleHierarchyImpl roleHierarchy = new RoleHierarchyImpl();
    Map<String, List<String>> roleHierarchyMap = new HashMap<>();

    roleHierarchyMap.put("ROLE_ADMIN", List.of("ROLE_STAFF"));
    roleHierarchyMap.put("ROLE_STAFF", List.of("ROLE_USER", "ROLE_GUEST"));
    String roleHierarchyFromMap = RoleHierarchyUtils.roleHierarchyFromMap(roleHierarchyMap);

    roleHierarchy.setHierarchy(roleHierarchyFromMap);
    return roleHierarchy;
}

And the ROLE_STAFF will imply the roles ROLE_USER and ROLE_GUEST, and ROLE_USER won't imply ROLE_GUEST as stated in a previous comment in this issue.

So, just went ahead and created a class to just provide a fluent API to build the map and use the same existing utility method RoleHierarchyUtils#roleHierarchyFromMap, hopefully, this way is more readable and easy to create a RoleHierarchy.

Let me here your thoughts!

@bwgjoseph
Copy link
Author

Hi @FdHerrera,

That's interesting, let me try it out and update the result here.

@bwgjoseph
Copy link
Author

Hello, I have finally gotten the chance to try it out.

So I have tested it, and while it works, it is still more involved / explicit than using textblock

The definition is have is slightly more complicated than above, which is something alone the line of

SUPER_ADMIN > ADMIN > ROLE_A > ROLE_B
ROLE_A > ROLE_AA
ROLE_B > ROLE_BB
ROLE_C > ROLE_CC
ROLE_C > ROLE_A

So what this translates to your sample would be

roleHierarchyMap.put("SUPER_ADMIN", List.of("ADMIN", "ROLE_A", "ROLE_B"));
roleHierarchyMap.put("ADMIN", List.of("ROLE_A", "ROLE_B"));
roleHierarchyMap.put("ROLE_A", List.of("ROLE_AA", "ROLE_B"));
roleHierarchyMap.put("ROLE_B", List.of("ROLE_BB"));
roleHierarchyMap.put("ROLE_C", List.of("ROLE_CC", "ROLE_A"));

I guess it does works, and did actually fulfill what I had initially asked, to be created via a Map. So I can't argue against that 🤣

Initially, I didn't quite get you, and this is what I did.

roleHierarchyMap.put("SUPER_ADMIN", List.of("ADMIN", "ROLE_A", "ROLE_B"));
roleHierarchyMap.put("ROLE_A", List.of("ROLE_AA"));
roleHierarchyMap.put("ROLE_B", List.of("ROLE_BB"));
roleHierarchyMap.put("ROLE_C", List.of("ROLE_CC", "ROLE_A"));

And the output was different from what I had expected.

Still appreciate you putting up the PR!

jzheaux pushed a commit that referenced this issue Dec 7, 2023
jzheaux added a commit that referenced this issue Dec 7, 2023
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants