This is an automated email from the ASF dual-hosted git repository. jungm pushed a commit to branch issue/TOMEE-4708 in repository https://gitbox.apache.org/repos/asf/tomee.git
commit 4c07dd6b5f47cb60a6782d737d22cd6debbe05ba Author: Markus Jung <[email protected]> AuthorDate: Mon Sep 21 10:53:01 2026 +0200 TOMEE-4708 - fix race condition in shared EL processor --- .../tomee/security/TomEEELInvocationHandler.java | 28 ++++++-- .../tomee/security/cdi/TomEESecurityExtension.java | 3 +- ...dAuthenticationMechanismDefinitionDelegate.java | 2 +- .../security/TomEEELInvocationHandlerTest.java | 75 ++++++++++++++++++++++ 4 files changed, 98 insertions(+), 10 deletions(-) diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java index 31fb01de5c..048856702c 100644 --- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java +++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/TomEEELInvocationHandler.java @@ -26,6 +26,7 @@ import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.HashSet; import java.util.Set; +import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -34,15 +35,22 @@ public class TomEEELInvocationHandler implements InvocationHandler { private static final Pattern EL_EXPRESSION_PATTERN = Pattern.compile("[#$]\\{([^{}]+)}"); private final Annotation annotation; - private final ELProcessor processor; + + // ElProcessor isn't threadsafe, so use a Supplier + private final Supplier<ELProcessor> processors; public TomEEELInvocationHandler(final Annotation annotation, final ELProcessor processor) { + this(annotation, () -> processor); + } + + private TomEEELInvocationHandler(final Annotation annotation, final Supplier<ELProcessor> processors) { this.annotation = annotation; - this.processor = processor; + this.processors = processors; } @Override public Object invoke(final Object proxy, final Method method, final Object[] args) throws Throwable { + final ELProcessor processor = processors.get(); // todo optimize and cache methods @@ -56,7 +64,7 @@ public class TomEEELInvocationHandler implements InvocationHandler { // Nested annotation with possible EL attributes (e.g. OpenIdAuthenticationMechanismDefinition -> LogoutDefinition) if (method.getReturnType().isAnnotation()) { - return of(((Class<Annotation>) method.getReturnType()), (Annotation) method.invoke(annotation, args), processor); + return of(((Class<Annotation>) method.getReturnType()), (Annotation) method.invoke(annotation, args), processors); } // If return value is not a String or an array of string, there is another method with "Expression" at the end and a return type String @@ -200,15 +208,21 @@ public class TomEEELInvocationHandler implements InvocationHandler { } public static <T extends Annotation> T of(final Class<T> annotationClass, final T annotation, final BeanManager beanManager) { - final ELProcessor elProcessor = new ELProcessor(); - elProcessor.getELManager().addELResolver(beanManager.getELResolver()); - return of(annotationClass, annotation, elProcessor); + return of(annotationClass, annotation, () -> { + final ELProcessor elProcessor = new ELProcessor(); + elProcessor.getELManager().addELResolver(beanManager.getELResolver()); + return elProcessor; + }); } public static <T extends Annotation> T of(final Class<T> annotationClass, final T annotation, final ELProcessor elProcessor) { + return of(annotationClass, annotation, () -> elProcessor); + } + + private static <T extends Annotation> T of(final Class<T> annotationClass, final T annotation, final Supplier<ELProcessor> processors) { return (T) Proxy.newProxyInstance(annotation.getClass().getClassLoader(), new Class[]{annotationClass}, - new TomEEELInvocationHandler(annotation, elProcessor)); + new TomEEELInvocationHandler(annotation, processors)); } } diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/TomEESecurityExtension.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/TomEESecurityExtension.java index ab63f96e90..fca2f660b8 100644 --- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/TomEESecurityExtension.java +++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/cdi/TomEESecurityExtension.java @@ -16,7 +16,6 @@ */ package org.apache.tomee.security.cdi; -import jakarta.enterprise.context.RequestScoped; import jakarta.enterprise.inject.spi.Bean; import jakarta.security.enterprise.authentication.mechanism.http.OpenIdAuthenticationMechanismDefinition; import org.apache.tomee.security.TomEEELInvocationHandler; @@ -403,7 +402,7 @@ public class TomEESecurityExtension implements Extension { .beanClass(OpenIdAuthenticationMechanismDefinition.class) .types(Object.class, OpenIdAuthenticationMechanismDefinition.class) .qualifiers(Default.Literal.INSTANCE, Any.Literal.INSTANCE) - .scope(RequestScoped.class) + .scope(ApplicationScoped.class) .createWith(creationalContext -> createOpenIdAuthenticationMechanismDefinition(defaultOidcDefinition, beanManager)); afterBeanDiscovery.addBean() diff --git a/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/OpenIdAuthenticationMechanismDefinitionDelegate.java b/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/OpenIdAuthenticationMechanismDefinitionDelegate.java index c4d9bc7203..5fabe7f164 100644 --- a/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/OpenIdAuthenticationMechanismDefinitionDelegate.java +++ b/tomee/tomee-security/src/main/java/org/apache/tomee/security/http/openid/OpenIdAuthenticationMechanismDefinitionDelegate.java @@ -210,7 +210,7 @@ public class OpenIdAuthenticationMechanismDefinitionDelegate implements OpenIdAu private static final String WELL_KNOWN_CONFIGURATION_PATH = "/.well-known/openid-configuration"; - private OpenIdProviderMetadata cached = null; + private volatile OpenIdProviderMetadata cached = null; public AutoResolvingProviderMetadata(OpenIdAuthenticationMechanismDefinition delegate) { super(delegate); diff --git a/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java b/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java index f4948a0819..cc5f3c10eb 100644 --- a/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java +++ b/tomee/tomee-security/src/test/java/org/apache/tomee/security/TomEEELInvocationHandlerTest.java @@ -18,6 +18,7 @@ package org.apache.tomee.security; import jakarta.el.ELProcessor; import jakarta.el.ELResolver; +import jakarta.enterprise.context.ApplicationScoped; import jakarta.enterprise.inject.Vetoed; import jakarta.enterprise.inject.spi.BeanManager; import jakarta.enterprise.inject.spi.CDI; @@ -30,8 +31,15 @@ import jakarta.security.enterprise.identitystore.PasswordHash; import org.junit.Assert; import org.junit.Test; +import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import static java.util.Arrays.stream; import static java.util.stream.Collectors.toMap; @@ -150,6 +158,51 @@ public class TomEEELInvocationHandlerTest extends AbstractTomEESecurityTest { Assert.assertThrows(IllegalArgumentException.class, () -> proxiedAnnotation.clientSecret()); } + // See TOMEE-4708 + @Test + public void sharedProxyIsSafeUnderConcurrentEvaluation() throws Exception { + final OpenIdAuthenticationMechanismDefinition annotation = + ConfigDrivenDefinition.class.getAnnotation(OpenIdAuthenticationMechanismDefinition.class); + + // the BeanManager variant is what TomEESecurityExtension uses for application-wide definitions + final OpenIdAuthenticationMechanismDefinition proxiedAnnotation = TomEEELInvocationHandler.of( + OpenIdAuthenticationMechanismDefinition.class, annotation, bm()); + + final int threads = 16; + final int iterations = 2000; + final CyclicBarrier barrier = new CyclicBarrier(threads); + final List<Throwable> failures = new CopyOnWriteArrayList<>(); + final ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + final List<Future<?>> futures = new java.util.ArrayList<>(); + for (int t = 0; t < threads; t++) { + futures.add(executor.submit(() -> { + try { + barrier.await(); + for (int i = 0; i < iterations; i++) { + Assert.assertFalse(proxiedAnnotation.tokenAutoRefresh()); + Assert.assertEquals(10000, proxiedAnnotation.tokenMinValidity()); + } + } catch (final Throwable e) { + failures.add(e); + } + })); + } + for (final Future<?> future : futures) { + future.get(2, TimeUnit.MINUTES); + } + } finally { + executor.shutdownNow(); + } + + if (!failures.isEmpty()) { + final AssertionError error = new AssertionError(failures.size() + " of " + threads + + " threads failed, first: " + failures.get(0)); + failures.forEach(error::addSuppressed); + throw error; + } + } + private BeanManager bm() { return CDI.current().getBeanManager(); } @@ -227,4 +280,26 @@ public class TomEEELInvocationHandlerTest extends AbstractTomEESecurityTest { } } + + @OpenIdAuthenticationMechanismDefinition( + providerURI = "https://server.example.com";, + clientId = "client", + tokenAutoRefreshExpression = "#{elHandlerTestConfig.isTrue('tokenAutoRefresh', false)}", + tokenMinValidityExpression = "#{elHandlerTestConfig.getInt('tokenMinValidity', 10000)}") + @Vetoed // keep the extension from registering this as a real OpenID mechanism + public static class ConfigDrivenDefinition { + } + + // discovered by CDI so the BeanManager's ELResolver can resolve it, like an application's config bean + @Named("elHandlerTestConfig") + @ApplicationScoped + public static class Config { + public boolean isTrue(final String key, final boolean defaultValue) { + return defaultValue; + } + + public int getInt(final String key, final int defaultValue) { + return defaultValue; + } + } }
