Methods annotated with @PostFilter are processed twice by PostFilterAuthorizationMethodInterceptor

RADickinson commented 2 months ago

Bug description

Found in version: 5.8.13

Fix required in: 5.8.x (required to enable migration to Spring Security 6).

While migrating from @EnableGlobalMethodSecurity(prePostEnabled = true) to @EnableMethodSecurity we have noticed the methods annotated with @PostFilter are processed twice by the PostFilterAuthorizationMethodInterceptor.

We use a custom PermissionEvaluator and custom MethodSecurityExpressionHandler to evaluate hasPermission expressions used with various prePost security annotations, for example:

  @PreAuthorize("hasPermission(#id, 'SecuredVO', 'SecuredVO:read')")
  @PostFilter("hasPermission(filterObject, 'SecuredVO:read')")
  List<SecuredVO> findByScopingId(final long scopingId);

The implementation of the PermissionEvaluator and MethodSecurityExpressionHandler is relatively operationally expensive and filtered objects are modified in some cases (from custom types) as well as removed from standard array / collection / stream types (using the DefaultMethodSecurityExpressionHandler). Running the filter twice for each operation annotated with @PostFilter leads to application errors and also significantly reduces performance.

We need this issue to be fixed in the 5.8.x version to enable us to complete migration steps towards upgrade to Spring Security 6.

Sample

A minimal sample of the issue is provided in this repository.

Steps to reproduce

The sample repository contains sample code and configuration used to reproduce the issue in a much simplified state from the original application. The issue can be reproduced simply by running the SecuredServiceTest which results in a RuntimeException to be thrown stating java.lang.RuntimeException: Collection already filtered.. The components of the test are:

The configuration classes in org.radickins.ssa to component scan and @EnableMethodSecurity
The minimal custom classes in org.radickins.ssa.security implementing the PermissionEvaluator and MethodSecurityExpressionHandler
An implementation of a secured Service in org.radickins.ssa.service with a simple API annotated with @PostFilter

Given the SecuredService API that is annotated with @PostFilter is entirely self-contained in a simple implementation, we expect the result to be filtered only once by the Spring Security framework. The permission evaluator marks each object that should be filtered as having been filtered, and the expression handler raises an exception if it detects the filterTarget has already been filtered. If working correctly, the expectation is the test should pass as filtering only occurs once.

Investigation

I have not attempted to fix the issue, but I believe the cause to be due to the bean registration of the PostFilterAuthorizationMethodInterceptor bean. The PrePostMethodSecurityConfiguration configuration declares the postFilterAuthorizationMethodInterceptor bean as an Advisor type, and as you can see the other interceptors for PreFilter, PreAuthorize, and PostAuthorize are each declared as MethodInterceptor types.

All of these interceptors are registered again as Advisor beans using the MethodSecurityAdvisorRegistrar

When the secured proxy is built, methods annotated with @PostFilter are assigned any Advisor beans required to process the AOP security proxy invocation, and the PostFilterAuthorizationMethodInterceptor is added twice (as shown in the image below taken from debugging a breakpoint in the CustomExpressionHandler).

AdvisorDebug

I believe the fix is to simply register the PostFilterAuthorizationMethodInterceptor as the MethodInterceptor type in the PrePostMethodSecurityConfiguration config.

marcusdacoregio commented 2 months ago

Hi @kse-music, thanks for the report. I believe this is a duplicate of https://github.com/spring-projects/spring-security/issues/15608.

Would you please wait for https://github.com/spring-projects/spring-security/issues/15608 to be fixed and check if it fixes the problem?

RADickinson commented 2 months ago

Hi @marcusdacoregio Thanks for looking at this. I think @kse-music has provided a suggested fix to this issue. I did notice #15608 just after I raised this issue but unfortunately I don't think I can verify the fix to 15608 will fix my issue. It looks like the target version for that fix is 6.4.0, and as I mentioned in the bug description, I am trying to apply the pre-migration changes suggested for 5.8 in order to allow us to migrate to Spring Security 6. I think we need this fix in a 5.8.x version in order to allow us to complete the pre-migration steps, and once that is working we can look at migrating. Hope that makes sense. Thanks Rob

jzheaux commented 2 months ago

@RADickinson, thanks for this report. I had hoped to get a fix out in time for the release this morning; however it will need to wait until the next release.

In the meantime, it is permissible to continue using @EnableGlobalMethodSecurity while upgrading as it is not removed in Spring Security 6, only deprecated.

jzheaux commented 2 months ago

Closed in 5c604b95fb620e1da99f8612e42935d6110cd60c

RADickinson commented 1 month ago

Thanks @jzheaux for fixing this issue.

spring-projects / spring-security