Sorting in AuthorizationAdvisorProxyFactory should be thread-safe

[wbxz987]

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 0cab7c8

[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?

[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?

[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: wbxz987/ConcurrentModificationException#1

Does this solution work better?

[wbxz987]

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