Methods annotated with @PostFilter are processed twice by PostFilterAuthorizationMethodInterceptor

[RADickinson]

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).

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

[marcusdacoregio]

Hi @kse-music, thanks for the report. I believe this is a duplicate of #15608.

Would you please wait for #15608 to be fixed and check if it fixes the problem?

[RADickinson]

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]

@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.