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

Migration to EnableMethodSecurity break Transactional on custom PermissionEvaluator #13152

Closed
kris2kris opened this issue May 11, 2023 · 2 comments
Assignees
Labels
in: config An issue in spring-security-config type: bug A general bug

Comments

@kris2kris
Copy link
Contributor

kris2kris commented May 11, 2023

Hello,

I perform the migration from EnableGlobalMethodSecurity to EnableMethodSecurity.
So I :

  1. Change the annotation
  2. Stop to extend GlobalMethodSecurityConfiguration class
  3. Replace override of createExpressionHandler() by a Bean to put my custom permission evaluator

I have a method with two annotations Transactionnal and PreAuthorize.
PreAuthorize call my custom hasPermission method.

Before migration my custom hasPermission method is executed after Transaction creation.
After migration my custom hasPermission method is executed before Transaction creation, so I cannot use some operation like a findAll which return a stream.

The workaround that I found is to change the order of annotation EnableTransactionManagement to put HIGHEST_PRECEDENCE.

Are you aware about this problem ? Is there a better way to fix it ? I didn't find anything in migration guide about this.

I use spring boot 3.0.6

You can find a simple sample here https://github.com/kris2kris/migration-method-security
Once started you must perform a get on localhost:8080/entities to reproduce the problem.

If you switch comment in Config.class and rollback on old version it works, if you uncomment the order EnableTransactionManagement it also works

@kris2kris kris2kris added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels May 11, 2023
@Nowheresly
Copy link

Same issue for.
The old EnableGlobalMethodSecurity has an attribute order set by default to Ordered.LOWEST_PRECEDENCE while the new EnableMethodSecurity does have such attribute.
With EnableGlobalMethodSecurity, the generated security interceptors have lowest precedence,so they are executed after the other interceptors. So the PermissionEvaluator can performs its checks within a transaction.
With EnableMethodSecurity, the generated security interceptors have by default highest precedence,so there's no guarantee anymore that the PermissionEvaluator can perform its checks within a transaction, causing this kind of issue.

After reading carefully the migration guide, it seems this new behaviour is not mentioned while impacts are real under the hood (the security checks are not performed in the same transaction as the protected business method).

Any recommendation or advice?

@kris2kris kris2kris changed the title Migration to EnableMethodSecurity Migration to EnableMethodSecurity break Transactional on custom PermissionEvaluator May 17, 2023
@jzheaux jzheaux self-assigned this May 23, 2023
@jzheaux jzheaux added in: config An issue in spring-security-config and removed status: waiting-for-triage An issue we've not yet triaged labels May 23, 2023
@jzheaux
Copy link
Contributor

jzheaux commented May 23, 2023

Thanks for the report, @kris2kris.

@EnableTransactionManagement and @EnableGlobalMethodSecurity have the same order value. This means that their order relative to each other is undefined. In your case, it works, but there are other arrangements (like with @PostAuthorize) where the order of @EnableTransactionManagement and @EnableGlobalMethodSecurity need to be adjusted so that @Transactional wraps appropriately.

The reason your app is breaking with @EnableMethodSecurity is that it publishes more than one method interceptor. Since their order of execution matters, we can't pick the same value as @EnableTransactionManagement for all of them and try and achieve a backward-compatible behavior.

Instead what happens is the authorization interceptors are ordered from 0, going up by 100 for each interceptor, similar to the security filter chain. @PreFilter's interceptor has an order of 100, @PreAuthorize's interceptor has an order of 200, and so on.

Given that, you can define your @EnableTransactionManagement as follows to precede all of Spring Security's method interceptors:

@EnableTransactionManagement(order = 0)

Yes, I think folks would benefit from this being clearer in the migration guide and the reference documentation, so I'll use this ticket to add that.

Also, I think it would be helpful to add a value like order. I've added #13214 to address that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: bug A general bug
Projects
Status: Done
Development

No branches or pull requests

3 participants