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

RoleHierarchy is ignored with GlobalMethodSecurityConfiguration and @Secured annotation #9158

Closed
mkrasuski opened this issue Oct 27, 2020 · 12 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@mkrasuski
Copy link

Describe the bug
Using @EnableGlobalMethodSecurity(securedEnabled = true) does not work with injected RoleHierarchy
For @Secured based version of interceptor the only Voters configured in AffirmativeBased are RoleVoter and AuthenticatedVoter. Injected RoleHierarchy is only used with Spel and @PreAuthorize

sample DEBUG logs are

2020-10-27 22:58:17.342 DEBUG 2441 --- [nio-8080-exec-1] o.s.s.a.i.a.MethodSecurityInterceptor    : Previously Authenticated: org.springframework.security.authentication.UsernamePasswordAuthenticationToken@6ed73ad3: Principal: mkr; Credentials: [PROTECTED]; Authenticated: true; Details: null; Granted Authorities: ROLE_ADMIN
2020-10-27 22:58:17.342 DEBUG 2441 --- [nio-8080-exec-1] o.s.s.access.vote.AffirmativeBased       : Voter: org.springframework.security.access.vote.RoleVoter@77795ee6, returned: 0
2020-10-27 22:58:17.343 DEBUG 2441 --- [nio-8080-exec-1] o.s.s.access.vote.AffirmativeBased       : Voter: org.springframework.security.access.vote.AuthenticatedVoter@54b6c513, returned: 0
2020-10-27 22:58:17.348 DEBUG 2441 --- [nio-8080-exec-1] o.s.s.w.a.ExceptionTranslationFilter     : Access is denied (user is not anonymous); delegating to AccessDeniedHandler
@mkrasuski mkrasuski added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Oct 27, 2020
@rwinch
Copy link
Member

rwinch commented Oct 28, 2020

Thanks for the report mkrasuki. Would you be interested in submitting a pull request? If not, could you at least put together a complete minimal example demonstrating the problem?

@rwinch rwinch added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 28, 2020
@mkrasuski
Copy link
Author

I've created demo app here
Hope this helps

@kcschan
Copy link

kcschan commented Dec 15, 2020

It seems when combining @secured with RoleHierarchy injection, the superclass RoleVoter is used during voting process instead of RoleHierarchyVoter, which contains inherited authorities.
The extractAuthorities(Authentication) of the superclass returns authorities from Authentication only, thus the inherited authorities described in the RoleHierarchy is not known to the voting process.

With @PreAuthorize, the RoleHierarchyVoter.extractAuthorities() is called correctly.

The issue is still reproducible in spring security 5.3.6(referred by springboot 2.3.7)

@jzheaux jzheaux self-assigned this Dec 16, 2020
@jzheaux jzheaux added type: enhancement A general enhancement and removed type: bug A general bug labels Dec 16, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Dec 16, 2020

I believe this is because GlobalMethodSecurityConfiguration does not look in the ApplicationContext for a RoleVoter bean.

That said, I think it would be best to see what happens with #9290 before enhancing GlobalMethodSecurityConfiguration to look for RoleVoter beans.

@rwinch rwinch added this to the 5.8.x milestone Jun 2, 2022
@howellevans
Copy link

Is this still an open issue? I am having an issue that sounds very similar to this. I have a custom permission evaluator I wrote so that I can make part of the authorization work off of the URI component, I used SPeL for that piece and do this.

@PreAuthorize("hasPermission(#eai, 'lead')")
@PostMapping(value="somepath/{eai}")
public ResponseEntitiy<SomeObject> someMethod(@PathVariable long eai) {
   //Do something meaningful
}

That works great, but now I have need on another method to do @secured (or some equivalent) to check for a claim in the JWT. That doesn't seem to work at all and always returns a 403. I confirmed that the role is in the JWT. I have tried a bunch of variations but all of them seem to lead to getting a 403. I tried all of the following.

@RolesAllowed("ROLE_somerole")``` @RolesAllowed("somerole") @secured("ROLE_somerole") @secured("somerole") @PreAuthorize("hasRole('ROLE_somerole')") @PreAuthorize("hasRole('somerole')")```

Any suggestions would be greatly appreciated.

I am using SpringBoot 2.7.2 and Java 11.

This is my GlobalMethodSecurityConfiguration

@Configuration
@EnableGlobalMethodSecurity(prePostEnabled = true,
                            securedEnabled = true,
                            jsr250Enabled = true
                           )
public class MethodSecurityConfig extends GlobalMethodSecurityConfiguration {

    @Autowired
    CustomPermissionEvaluator customPermissionEvaluator;

    @Override
    protected MethodSecurityExpressionHandler createExpressionHandler() {
        DefaultMethodSecurityExpressionHandler expressionHandler =
                new DefaultMethodSecurityExpressionHandler();
        expressionHandler.setPermissionEvaluator(customPermissionEvaluator);
        return expressionHandler;
    }
}

@sjohnr
Copy link
Member

sjohnr commented Aug 15, 2022

@howellevans, thanks for getting in touch, but it feels like this is a question that would be better suited to Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it).

Based on the provided details it doesn't sound related to this issue, which is focused on RoleHierarchy and RoleHierarchyVoter. I could be wrong, but wanted to mention that in case it helps point you in the right direction. Please open a new issue with a minimal sample that reproduces this issue if you feel this is a genuine bug.

@c-mansfield
Copy link

This still seems to be an issue. When using the @Secured annotation and having a RoleHierarchy setup the RoleHierarchyVoter is ignored and the authorities are not being pulled from the hierarchy in RoleVoter.

@sjerman
Copy link

sjerman commented Nov 18, 2022

This issue still seems to be present with the new @EnableMethodSecurity support. It looks to be that WebSecurityExpressionHandler is initialised with a RoleHierarchy bean correctly, however MethodSecurityExpressionHandler isn't.

You can work around by defining your own bean.

  @Bean
  MethodSecurityExpressionHandler MmthodSecurityExpressionHandler(RoleHierarchy roleHierarchy) {
    DefaultMethodSecurityExpressionHandler meh = new DefaultMethodSecurityExpressionHandler();
    meh.setRoleHierarchy(roleHierarchy);
    return meh;
  }

This seems to fix @PreAuthorize uses - doesn't fix @Secured use and you need to use @EnableMethodSecurity without securedEnabled=true:

@Configuration
@EnableWebSecurity
@EnableMethodSecurity
public class DefaultSecurityConfig {
...

@aburmeis
Copy link

aburmeis commented Sep 29, 2023

Same problem here with Spring Boot 3.1.4. I have @EnableMethodSecurity(securedEnabled = true) and a custom bean of type RoleHierarchy. The configuration from @sjerman fixed any @PreAuthorize("hasAuthority(...)") but @Secured(...) is still broken.

As fas as I understand, a SecuredAuthorizationManager is used (created at SecuredMethodSecurityConfiguration). There seems to be a lack of setting the role hierarchy in the AuthoritiesAuthorizationManager used internally, it is always a NullRoleHierarchy.

@jzheaux As this was working with Spring Boot 2.7.x but not with 3.x., I would consider this is a bug.

@DerChris173
Copy link
Contributor

I would be able to do a simple fix in org.springframework.security.config.annotation.method.configuration.SecuredMethodSecurityConfiguration by injecting the RoleHierarchy and setting it into the SecuredAuthorizationManager, with some reasonable default behavior.

Would a PR be accepeted?

@DerChris173
Copy link
Contributor

I just checked open PRs, and this would be resolved with this PR

#14057

@jzheaux jzheaux modified the milestones: 5.8.x, 6.3.0-M1 Dec 7, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Dec 7, 2023

Given that @EnableGlobalMethodSecurity is deprecated, this is superseded by #12783

@jzheaux jzheaux closed this as completed Dec 7, 2023
@jzheaux jzheaux added the status: declined A suggestion or change that we don't feel we should currently apply label Dec 7, 2023
@jzheaux jzheaux removed this from the 6.3.0-M1 milestone Dec 7, 2023
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) status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

10 participants