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 a factory method for RoleHierarchyImpl #13788

Merged
merged 2 commits into from Dec 8, 2023

Conversation

making
Copy link
Member

@making making commented Sep 9, 2023

Currenctly Hierarchical Roles requires the setter method to define the hierarchy.

RoleHierarchyImpl hierarchy = new RoleHierarchyImpl();
hierarchy.setHierarchy("...");

This PR adds a factory method for this use case to make it easier

RoleHierarchyImpl hierarchy = RoleHierarchyImpl.of("...");

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 9, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Sep 12, 2023

@making, I like this idea; however, I'd prefer that it not be on the implementation.

Usually, this is addressed in Spring Security with a factory class like RoleHierarchies. The nice thing about that is that we could do the following:

public final class RoleHierarchies {
    public static RoleHierarchy of(String hierarchy) { ... }
    public static RoleHierarchy of(Map<String, List<String>> hierarchy) { ... }
}

Are you able to change the PR along these lines?

@jzheaux jzheaux self-assigned this Sep 12, 2023
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 12, 2023
@making
Copy link
Member Author

making commented Sep 12, 2023

I’ll do it

@making
Copy link
Member Author

making commented Sep 13, 2023

The reason I created this factory method in the impl class is because the format of the string representation is highly dependent on RoleHierarchyImpl.
If I don't have to care about it I'll move to RoleHierarchies.

@making
Copy link
Member Author

making commented Sep 13, 2023

Map<String, List<String>> hierarchy

Is this a map where directly assigned authority is the key and reachable authorities is the value?
And is this intended to be a factory method for a different implementation class than RoleHierarchyImpl?

@jzheaux
Copy link
Contributor

jzheaux commented Sep 26, 2023

@making Good points.

The reason I created this factory method in the impl class is because the format of the string representation is highly dependent on RoleHierarchyImpl.

Good point. Maybe instead what we do is deprecate the setHierarchy method as well as add the factory method. My concern is that it's confusing to have a setter and a constructor that do the same thing in the same class.

And is this intended to be a factory method for a different implementation class than RoleHierarchyImpl?

No, I was thinking for the same implementation. It could use RoleHierarchyUtils to adapt, for example.

That said, a builder is probably better than taking a map, so instead let's just add the factory method to RoleHierarchyImpl, as you originally proposed, and also deprecate setHierarchy. I'll create separate ticket to look at a more fluent way to describe a hierarchy. Will that work?

making and others added 2 commits December 8, 2023 11:14
- Change to #fromHierarchy to match naming convention
- Keep existing test methods the same
- Deprecate setHierarchy and default constructor
- Add private Map constructor
- Change Adjust RoleHierarchyBuilder to use Map constructor

Issue spring-projectsgh-13788
@jzheaux jzheaux merged commit 92be497 into spring-projects:main Dec 8, 2023
2 checks passed
jzheaux pushed a commit that referenced this pull request Dec 8, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Dec 8, 2023

Thanks again for your submission, @making! This is now merged into main. I added a polish in 9ffa54d.

@jzheaux jzheaux added this to the 6.3.0-M1 milestone Dec 8, 2023
@making making deleted the role-hierarchy-factory branch December 9, 2023 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants