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

Add RoleHierarchyBuilder #14196

Merged
merged 2 commits into from Dec 7, 2023
Merged

Add RoleHierarchyBuilder #14196

merged 2 commits into from Dec 7, 2023

Conversation

FdHerrera
Copy link

Create builder class to declaratively create a RoleHierarchy with a fluent API, add test. Closes #13300
gh-13300

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @FdHerrera! Given that this is ultimately in the service of RoleHierarchyImpl, will you please move this into that class? In that way, I believe it will also better align with #13788

@jzheaux jzheaux self-assigned this Nov 29, 2023
@jzheaux jzheaux 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 Nov 29, 2023
@FdHerrera
Copy link
Author

Thank you very much for your feedback, @jzheaux !

I'll start implementing your suggestions.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @FdHerrera! Things are shaping up nicely. I've left a little bit more feedback inline.

Also, in preparation for merging, will you please squash your commits into a single commit that looks something like this:

Add RoleHierarchy.Builder

Closes gh-13300

@FdHerrera
Copy link
Author

Thank you for the thorough review and constructive feedback! I appreciate the collaborative back-and-forth @jzheaux .

I implemented your suggestions and squashed the commits, I think is looking pretty good let me hear your thoughts!

@FdHerrera
Copy link
Author

I believe it will also better align with #13788

Also, regarding this comment, should I change the commit and description to close that issue instead of gh-13300?

- 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
@jzheaux
Copy link
Contributor

jzheaux commented Dec 6, 2023

Thanks, @FdHerrera! I've added a polish to make it align more closely with the rest of the codebase stylistically.

For now, I think we should leave out withNoRolePrefix and wait to see how the community uses the class. In other places in Spring Security, ROLE_ is the default and you supply a string value to override the default. There isn't the notion elsewhere in the code base of "no prefix". I went ahead and adjusted that in my polish commit. Let me know if me removing that method causes a problem.

@FdHerrera
Copy link
Author

Looks good @jzheaux thank you for your help!

@jzheaux jzheaux added this to the 6.3.0-M1 milestone Dec 7, 2023
@jzheaux jzheaux merged commit ee8bc78 into spring-projects:main Dec 7, 2023
2 checks passed
@FdHerrera FdHerrera deleted the gh-13300 branch December 7, 2023 22:26
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: Done
Development

Successfully merging this pull request may close these issues.

Easier way to define Role Hierarchy
3 participants