Skip to content
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

Fix adding more implied roles in the RoleHierarchy Builder. #15717

Merged

Conversation

nielsbasjes
Copy link
Contributor

[ As requested by @marcusdacoregio in this comment ]

When defining a RoleHierarchy you can use the text format and the Builder model.

In the text format you can specify

   ROLE_A > ROLE_B
   ROLE_A > ROLE_C

which gives 'A' both 'B' and 'C'

The same specification in the Builder

RoleHierarchyImpl.withDefaultRolePrefix()
    .role("A").implies("B")
    .role("A").implies("C")
    .build();

which gives 'A' only 'C'

Rootcause: doing implies wipes any previous specifications for the same source role.

This fixes that and makes the Builder behave the same as the text format.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 31, 2024
@marcusdacoregio marcusdacoregio self-assigned this Sep 3, 2024
@marcusdacoregio marcusdacoregio added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 3, 2024
@marcusdacoregio marcusdacoregio added this to the 6.4.0-M4 milestone Sep 3, 2024
nielsbasjes added a commit to nielsbasjes/spring-security that referenced this pull request Sep 3, 2024
Closes spring-projectsgh-15717

Signed-off-by: Niels Basjes <niels@basjes.nl>
@nielsbasjes
Copy link
Contributor Author

Apologies for putting in merge request with a checkstyle problem.
I tried to check the format locally but that failed.
Now I know this is a problem others have already reported #14575

Closes spring-projectsgh-15717

Signed-off-by: Niels Basjes <niels@basjes.nl>
@marcusdacoregio marcusdacoregio merged commit 2dc787a into spring-projects:main Sep 4, 2024
6 checks passed
@marcusdacoregio
Copy link
Contributor

Thanks @nielsbasjes, this is now merged into main.

I am considering this as an enhancement for now, but we can consider backporting it if we receive feedback about users needing it in older versions.

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 type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants