-
Notifications
You must be signed in to change notification settings - Fork 6k
Configure permissionEvaluator and roleHierarchy by default #4115
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
Conversation
Thanks for the PR @candrews! Any chance you can add some tests for this? |
@rwinch I've added tests. If there's anything else I can do, please let me know. Thanks! |
Thanks that is all I need. It will be merged into our next feature release (5.0.0.M1) once we create a new branch for it (next few weeks). |
@@ -102,22 +106,47 @@ protected abstract SecurityExpressionOperations createSecurityExpressionRoot( | |||
Authentication authentication, T invocation); | |||
|
|||
protected RoleHierarchy getRoleHierarchy() { | |||
if(! roleHierarchySet && context != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe extract this if sentence to a conditional method so that the code would be easier to read
e.g. private boolean roleHerarchyNotSetForValidContext(){
! roleHierarchySet && context != null
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much of an improvement it is, but I've made the change you requested and updated this PR accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's making the code more easily to read
Implementations of AbstractSecurityExpressionHandler (such as the very commonly used DefaultWebSecurityExpressionHandler) get PermissionEvaluator and RoleHierarchy from the application context (if the application context is provided, and exactly one of such a bean exists in it). This approach matches that used in GlobalMethodSecurityConfiguration, making everything in Spring Security work the same way (including WebSecurity). Issue spring-projectsgh-4077
@rwinch Looks like 5.0 is in progress in master... can this be merged? Thanks! |
Thanks for the PR @candrews! This is merged into master |
Configure WebSecurity's default WebSecurityExpressionHandler to use the ApplicationContext's PermissionEvaluator and RoleHierarchy by default, the same way that GlobalMethodSecurityConfiguration does for its default MethodSecurityExpressionHandler.
Issue gh-4077