spring-projects
Support Authorities Without Role Prefix in `RoleHierarchyImpl` Builder
@marcusdacoregio here is my take on https://github.com/spring-projects/spring-security/issues/15264
This is 3 changes (hence 3 commits):
- The proposed way of being able to add the mapping from any authority into 1 or more roles.
- I found that if you add to a hierarchy builder the same role with some extra roles the existing roles are discarded. This is different from what you get with the text format.
- The tests to makes sure this actually works became unreadable (hence I wasn't sure anymore) so I added a few helper functions to make it a lot better to understand what the test does.
Are the documentation changes I created enough?
Hi @nielsbasjes, thanks for the report.
I found that if you add to a hierarchy builder the same role with some extra roles the existing roles are discarded. This is different from what you get with the text format.
Would you please move this to another PR? From my perspective it isn't exactly a bug but a support that should be added to the builder. A new PR is better so we can track exactly when/what changed.
Hi @nielsbasjes, thanks for the report.
I found that if you add to a hierarchy builder the same role with some extra roles the existing roles are discarded. This is different from what you get with the text format.
Would you please move this to another PR? From my perspective it isn't exactly a bug but a support that should be added to the builder. A new PR is better so we can track exactly when/what changed.
I have removed that part from this PR. After you have approved this I'll put that up for separate approval (to avoid needless merge complexity).
@marcusdacoregio is this how you want this merge request? If not then just tell me what I need to change.
Hi @nielsbasjes. I plan to review this PR before 6.4.0-M1 on July 15th. I'll leave my feedback if there is anything else to be changed.
@marcusdacoregio How shall I submit the other change? I'm asking because it is partially overlapping code.
Hi @nielsbasjes. I was talking to @jzheaux about this feature and we come to a conclusion that we should refrain from adding the .authority(...) method to the builder. The main reason is because that we can cause more confusion with methods with a not so predictable behavior, like we discussed here, which can easily lead to misconfiguration.
Our suggestion is to add a new public static Builder withNoRolePrefix() method. This is nice because the application is either always supplying fully-qualified authorities or always supplying roles, making it easier to infer at a glance when the code is read later on.
What do you think?
Hi @nielsbasjes. I was talking to @jzheaux about this feature and we come to a conclusion that we should refrain from adding the
.authority(...)method to the builder.
Unexpected but you're the committers here.
The main reason is because that we can cause more confusion with methods with a not so predictable behavior, like we discussed here, which can easily lead to misconfiguration.
Where do you see "not so predictable behavior"?
- In
.role("X")X is always a role (i.e. with prefix). - In
.implies("X")X is always a role (i.e. with prefix). - In
.authority("X")X is always an authority (i.e. without prefix).
From where I'm standing the confusion (which is there) stems mainly from the difference between authority and role being quite unclear (at least to me when reading the documentation) and being stored in the same structures with a hidden prefix to determine the difference.
My code and explanation (which as I said I'm willing to put into the documentation if so desired) make it clearer and reliably usable ... at least to me it does.
Our suggestion is to add a new
public static Builder withNoRolePrefix()method. This is nice because the application is either always supplying fully-qualified authorities or always supplying roles, making it easier to infer at a glance when the code is read later on.
That would be a completely silly change because it is 100% the same as simply doing .withRolePrefix(""). I do not see that as a valid code improvement because that makes the distinction between authorities and roles even harder to understand.
In my applications I do not want an external authority to accidentally match a role ever; I have even created this last week to make that even stronger to ensure no problems if someone creates an externally provided authority "ROLE_X":
@Bean
fun grantedAuthorityDefaults(): GrantedAuthorityDefaults = GrantedAuthorityDefaults(UUID.randomUUID().toString())
What do you think?
I think it would really help if you document what you see as the distinction between authority and role because from this discussion it seems you have a totally different idea than I have.
I don't think I clearly follow the following statement:
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.
In my opinion, this is probably mostly related to your business needs than how the framework sees it. Both authorities and roles are the same thing, sometimes it makes sense to group users into roles and sometimes you need a more granular control so you can opt-in for authorities. But that is just a matter of naming, when performing any authorization logic the framework does not differ whether it is an authority or a role.
That would be a completely silly change because it is 100% the same as simply doing .withRolePrefix(""). I do not see that as a valid code improvement because that makes the distinction between authorities and roles even harder to understand.
That's the point, there is no difference between those two in general. The example that you gave earlier from my point of view should have a different arrangement. Roles are often used to group authorities and simplify access control logic:
RoleHierarchyImpl.withDefaultRolePrefix()
.role("USER_WRITE") .implies("USER_READ")
.role("USER_DELETE") .implies("USER_WRITE")
.authority("User") .implies("USER_READ")
.authority("Manager") .implies("USER_WRITE")
.authority("Administrator").implies("USER_DELETE", "SHOW_DASHBOARD")
should be:
RoleHierarchyImpl.withNoRolePrefix()
.role("ROLE_USER") .implies("USER_READ")
.role("ROLE_MANAGER") .implies("USER_WRITE")
.role("ROLE_ADMINISTRATOR") .implies("USER_DELETE", "SHOW_DASHBOARD")
.role("USER_WRITE") .implies("USER_READ")
.role("USER_DELETE") .implies("USER_WRITE")
In the example above, there is three roles and based on the role that the user has authorities are given to them. It sounds like it's the other way around than your understanding, but authorities/roles can be given to users directly, with no hard rule on whether it should be external/internal. This is also (briefly) mentioned in the documentation.
I think it would really help if you document what you see as the distinction between authority and role because from this discussion it seems you have a totally different idea than I have.
Absolutely, we can include in the reference docs some guidance on the difference between those two.
I apologize for the back and forth on this, but since this is aimed for 6.4 we have a bit of time and we should take advantage of it to try to refine as much as possible.
In my opinion, this is probably mostly related to your business needs than how the framework sees it.
Yes, so that seems to be the big problem for me here; The framework has two flags that are technically identical and also do not have any logical distinction between them. So duplicate APIs that do the same, have the same implementation and only differ because one prefixes everything with a prefix.
That makes the current situation very confusing for me.
I'm confronted with authorities which are of external origin (i.e. usergroups from the IAM/IGA solution) which I need to handle in an as clean as possible way within the application.
My need is to map these external values (without any prefix; stored as authorities) into Roles in a clean way, that is what I'm trying to achieve here.
Key question: What would you like to do with this? How do you see this? I'm willing to change it as long as I understand how you want it.
For now: I have taken the code I put here as a starting point and created a company internal variant that meets our business needs: Mapping authorities and roles into new roles.
Note: I had to copy some of the code (instead of inherit) because parts of it were private instead of protected (just making the constructor with parameters protected would have fixed it for me).
Hi @nielsbasjes. The reference docs have a good explanation about authorities and where the ROLE_ prefix comes from.
With that in mind, I still think that adding that new method to the builder will cause some confusion, and therefore, I don't think we should merge it as is without more feedback from the community. For such scenarios, users can always use the constructor that accepts a role hierarchy as a String.
I'd still love a PR to fix https://github.com/spring-projects/spring-security/issues/15264#issuecomment-2175382499.
With that said, I'll close this as declined for now, but we can always reopen it as soon as we get more community feedback.