Using Spring Security 6.4.4 (via Spring Boot 3.4.4)

Describe the bug When a endpoint annotated with @AuthorizeReturnObject returns an object that has an object authorized with e.g. @PreAuthorize, ConcurrentModificationException is thrown when multiple requests come in parallel.

To Reproduce See the sample repository below for a reproducible test case.

Expected behavior Parallel requests should be handled without errors.

Sample https://github.com/wbxz987/ConcurrentModificationException

The sample repository contains a test, that simulates multiple requests coming in parallel. The test fails because a ConcurrentModificationException is thrown.

Caused by: java.util.ConcurrentModificationException
    at java.base/java.util.ArrayList.sort(ArrayList.java:1806)
    at org.springframework.core.annotation.AnnotationAwareOrderComparator.sort(AnnotationAwareOrderComparator.java:111)
    at org.springframework.security.authorization.method.AuthorizationAdvisorProxyFactory.proxy(AuthorizationAdvisorProxyFactory.java:168)
    at org.springframework.security.authorization.method.AuthorizeReturnObjectMethodInterceptor.invoke(AuthorizeReturnObjectMethodInterceptor.java:61)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:184)
    at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:727)
    at org.example.concurrentmodificationexception.controller.Controller$$SpringCGLIB$$0.getModel(<generated>)

The test works when downgrading Spring Security to version 6.3.2, and breaks after this commit https://github.com/spring-projects/spring-security/commit/0cab7c8f15ab98e97b10458d63ed3d85601b0903

Comment From: jzheaux

Thanks for the detailed report, @wbxz987! We'll target fixing this in the next maintenance release.

In the meantime, you can workaround this by providing your own proxy factory instance like so:

@Primary
@Bean
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
static AuthorizationProxyFactory proxyFactory() {
    AuthorizationAdvisorProxyFactory proxyFactory = new AuthorizationAdvisorProxyFactory(new ArrayList<>());
    List<AuthorizationAdvisor> advisors = new ArrayList<>();
    advisors.add(AuthorizationManagerBeforeMethodInterceptor.preAuthorize());
    advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize());
    advisors.add(new PreFilterAuthorizationMethodInterceptor());
    advisors.add(new PostFilterAuthorizationMethodInterceptor());
    advisors.add(new AuthorizeReturnObjectMethodInterceptor(proxyFactory));
    AnnotationAwareOrderComparator.sort(advisors);
    for (AuthorizationAdvisor advisor : advisors) {
        proxyFactory.addAdvisor(advisor);
    }
    return proxyFactory;
}

Will you please let me know if this addresses the issue for the time being?

Comment From: wbxz987

Thanks for the quick fix @jzheaux and @ngocnhan-tran1996, looking forward for the release.

I updated the sample repository with the proposed workaround: https://github.com/wbxz987/ConcurrentModificationException/blob/main/src/main/java/org/example/concurrentmodificationexception/config/SecurityConfig.java#L38

  1. I modified the return type to be AuthorizationAdvisorProxyFactory instead of AuthorizationProxyFactory, because AuthorizeReturnObjectMethodInterceptor was still using AuthorizationAdvisorProxyFactory from AuthorizationProxyConfiguration. Is this correct?

  2. After modifying the return type, the AuthorizationAdvisorProxyFactory seems to have duplicated entries of AuthorizationAdvisor. I guess this is because AuthorizationProxyConfiguration adds the same advisors when initializing authorizeReturnObjectMethodInterceptor bean.

  3. The test still fails, with the same exception at the same location

Am i missing something?

Comment From: jzheaux

I see, my mistake, @wbxz987. I incorrectly anticipated that List.sort would only throw ConcurrentModificationException when it needed to make modifications; however, it appears to check for this any time List.sort is called, regardless if there are any changes.

What you can do instead is publish a thread safe version of the proxy like so:

@Component
@Primary
public class ThreadsafeAuthorizationProxyFactory implements AuthorizationProxyFactory {
    private AuthorizationAdvisorProxyFactory delegate;

    private final ReentrantLock lock = new ReentrantLock();

    @Override
    public Object proxy(Object object) {
        try {
            this.lock.lock();
            return this.delegate.proxy(object);
        } finally {
            this.lock.unlock();
        }
    }

    public void setDelegate(AuthorizationAdvisorProxyFactory delegate) {
        this.delegate = delegate;
    }
}

And then provide that proxy to an authorizeReturnObjectAdvisor like so:

@Bean
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
static Advisor authorizeReturnObjectAdvisor(ObjectProvider<AuthorizationAdvisor> provider, 
    ThreadsafeAuthorizationProxyFactory proxyFactory) {
    AuthorizationAdvisorProxyFactory delegate = new AuthorizationAdvisorProxyFactory(new ArrayList<>());
    provider.forEach(delegate::addAdvisor);
    AuthorizeReturnObjectMethodInterceptor interceptor = new AuthorizeReturnObjectMethodInterceptor(proxyFactory);
    delegate.addAdvisor(interceptor);
    proxyFactory.setDelegate(delegate);
    return interceptor;
}

I updated the provided sample with this change: https://github.com/wbxz987/ConcurrentModificationException/pull/1

Does this solution work better?

Comment From: wbxz987

Yes, the workaround works perfectly. Thank you @jzheaux!