Use available RoleHierachy Bean for MethodSecurity Config #14057
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
Thanks for the PR, @kandaguru17! Will you please add some tests to confirm that the feature works? |
jzheaux
added
in: config
done :) |
There was a problem hiding this comment.
Thanks, @kandaguru17! This is shaping up nicely.
In addition to my inline comments, will you please update the copyright years for each file that you edit?
| @@ -447,6 +449,24 @@ public void configureWhenBeanOverridingDisallowedThenWorks() { | |||
| .autowire(); | |||
| } | |||
|
|
|||
| @WithMockUser(roles = "ADMIN") | |||
| @Test | |||
| public void methodSecurityAdminWhenRoleHierarchyBeanAvailableThenUses() { | |||
There was a problem hiding this comment.
I don't think I'm understanding these tests yet. They seem to be testing that ADMIN can reach ADMIN resources. But with a role hierarchy in place, wouldn't we want to test that ADMIN can reach USER resources as well?
| @@ -48,8 +52,13 @@ final class SecuredMethodSecurityConfiguration { | |||
| @Role(BeanDefinition.ROLE_INFRASTRUCTURE) | |||
| static MethodInterceptor securedAuthorizationMethodInterceptor( | |||
| ObjectProvider<SecurityContextHolderStrategy> strategyProvider, | |||
| ObjectProvider<ObservationRegistry> registryProvider) { | |||
| ObjectProvider<ObservationRegistry> registryProvider, ApplicationContext context) { | |||
There was a problem hiding this comment.
Is there a reason to use ApplicationContext instead of ObjectProvider? I'd prefer to avoid future maintenance questions, if possible, by having this feature follow the same patterns other features.
It also makes this beans dependencies clearer.
There was a problem hiding this comment.
It also may ultimately be less code to maintain:
RoleHierarchy roleHierarchy = roleHierarchyProvider.getIfAvailable(NullRoleHierarchy::new);| static class RoleHierarchyConfig { | ||
|
|
||
| @Bean | ||
| RoleHierarchy roleHierarchy() { |
There was a problem hiding this comment.
Will you please make this method static? Such ensures that it is available at the time the static method interceptor beans are published. It also ensures that the interceptor beans are available for bean post-processing.
| @@ -50,7 +50,7 @@ public interface MethodSecurityService { | |||
| @PermitAll | |||
| String jsr250PermitAll(); | |||
|
|
|||
| @RolesAllowed("ADMIN") | |||
| @RolesAllowed({ "ADMIN", "USER" }) | |||
There was a problem hiding this comment.
Will you please separate this into a user method, like you did with the others? In this way, the test methods will read more consistently.
|
Dear @kandaguru17 , thank you for raising this PR. Will you be able to integrate the requested changes? Otherwise I will attempt to pick it up. |
|
Thanks again, @kandaguru17! This was merged via #14260. |
Apologies @jzheaux for the delay, Many thanks @DerChris173 , for amending the comments in the PR :) |
Closes gh-12783
PR includes