Markus Jung created TOMEE-4708:
----------------------------------
Summary: OpenID definition EL expressions are evaluated on a
shared ELProcessor and fail under concurrent requests (regression of TOMEE-4595)
Key: TOMEE-4708
URL: https://issues.apache.org/jira/browse/TOMEE-4708
Project: TomEE
Issue Type: Bug
Components: TomEE Core Server
Affects Versions: 11.0.0-M1
Reporter: Markus Jung
Assignee: Markus Jung
An {{@OpenIdAuthenticationMechanismDefinition}} with method-call expressions,
e.g. {{tokenAutoRefreshExpression = "#
{cfg.isTrue('...', false)}
"}}, fails as soon as several requests with an expired access token hit the
mechanism at once (typically the burst of parallel
{{/jakarta.faces.resource/*}} requests after a page load):
{noformat}
jakarta.el.PropertyNotFoundException: ELResolver did not handle type: [false]
with property of [isTrue]
at org.apache.el.parser.AstValue.getValue (AstValue.java:156)
at jakarta.el.ELProcessor.getValue (ELProcessor.java:62)
at org.apache.tomee.security.TomEEELInvocationHandler.eval
(TomEEELInvocationHandler.java:149)
at
org.apache.tomee.security.cdi.OpenIdAuthenticationMechanism.handleExpiredTokens
(OpenIdAuthenticationMechanism.java:215)
{noformat}
The message varies between identical requests ({{{}[false]/[isTrue]{}}},
{{{}[null]/[isTrue]{}}}, {{{}[null]/[null]{}}}) and some evaluations silently
return wrong values (e.g. {{tokenMinValidity()}} = 0). Affected requests get a
500 and the token refresh never runs.
h3.
{{TomEEELInvocationHandler.of(Class, Annotation, BeanManager)}} creates one
{{ELProcessor}} per definition proxy and uses it from every request thread.
{{ELContext}} keeps its "property resolved" flag as a plain field;
{{CompositeELResolver.invoke}} clears and re-reads it per evaluation, so
concurrent threads corrupt each other's state.
{{{}ELProcessor{}}}/{{{}ELContext{}}} are not thread-safe by contract.
TOMEE-4595 (59da9e49d1) made the {{OpenIdAuthenticationMechanismDefinition}}
bean {{{}@RequestScoped{}}}, giving each request its own proxy and processor.
The Security 4.0 rework (9aaa5454d6, before 11.0.0-M1) introduced
{{{}createOpenIdAuthenticationMechanismDefinitionSupplier{}}}, which caches a
single proxy in an {{AtomicReference}} for the whole application, and
{{OpenIdAuthenticationMechanism.getDefinition()}} prefers that supplier over
the injected bean. The request-scoped bean is bypassed on the request path, so
the shared processor is back. This shipped in 11.0.0-M1.
h3. Why the scope change was the wrong fix
* It patched one consumer instead of the shared processor in
{{{}TomEEELInvocationHandler{}}}. Any other code holding the proxy across
threads reintroduces the bug, which is what happened. The
Basic/LDAP/database/in-memory/{{{}LoginToContinue{}}} proxies were never
covered.
* It made the definition request-bound. {{AutoResolvingProviderMetadata}}
caches the discovered provider metadata on the proxy, so a request-scoped proxy
re-fetches {{/.well-known/openid-configuration}} from the IdP on every request
that reaches it (which {{OpenIdIdentityStore}} does today).
* It requires an active request context the definition does not otherwise need.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
