[
https://issues.apache.org/jira/browse/TOMEE-4648?focusedWorklogId=1036155&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1036155
]
ASF GitHub Bot logged work on TOMEE-4648:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 17/Aug/26 18:50
Start Date: 17/Aug/26 18:50
Worklog Time Spent: 10m
Work Description: rzo1 commented on PR #2851:
URL: https://github.com/apache/tomee/pull/2851#issuecomment-5318833942
Currently short on time, so AI review only: I ran an adversarial review pass
over this PR, followed by a second pass whose job was to *refute* the first
one's findings against the actual code. Everything below survived that second
pass. Take it as input, not as a verdict — I have not run the build.
**Overall: the core fix is correct and well-diagnosed.** Confirmed against
the bytecode of `jakarta.security.enterprise-api` 4.0.0 that
`IdentityStore.validate(Credential)` binds via
`MethodHandles.lookup().bind(this, "validate",
methodType(CredentialValidationResult.class, credential.getClass()))` — an
*exact runtime-class* match — so a store declaring only
`validate(UsernamePasswordCredential)` was genuinely never invoked when handed
a `BasicAuthenticationCredential`, and the `NoSuchMethodException` catch turned
that into `NOT_VALIDATED_RESULT`. Passing a plain `UsernamePasswordCredential`
matches what `FormAuthenticationMechanism` in this same module already does,
and what Soteria does, so the change is consistent rather than a one-off.
`BasicAuthenticationCredential` is referenced nowhere else in the repo, and all
four bundled stores branch on `instanceof UsernamePasswordCredential`, so none
regress. The empty/malformed-header path is unchanged, and the new test's
qualified `@BasicAuthenticationMechanismDefinition` resolves cleanly and
asserts real group propagation rather than just a 200.
The findings are all about the *diagnostics* half of the PR.
### 1. The new debug message is silent in exactly the case it was written
for (minor)
`TomEEIdentityStoreHandler` — the block sits inside the `else` of `if
(validationHappened)`, but `validationHappened` is set as soon as *any* store
returns INVALID, and `TomEEDefaultIdentityStore.validate` returns
`INVALID_RESULT` for any caller absent from `tomcat-users.xml` (it overrides
`validate(Credential)` itself, so it never goes through the exact-overload
dispatch).
This is not hypothetical for this PR's own reproducer:
`TomEESecurityExtension` registers `TomEEDefaultIdentityStore` app-wide as soon
as any bean in the archive carries `@TomcatUserIdentityStoreDefinition` (five
test classes do), `AbstractTomEESecurityTest` deploys the whole test-classes
tree as one webapp, and `reza` is not in
`src/test/resources/conf/tomcat-users.xml`. So running
`Tomee4648BasicGroupsTest` *without* the `BasicAuthenticationMechanism` fix
takes the `return INVALID_RESULT` path and the new message is never emitted —
the operator is left with exactly the undiagnosable 401 the block was added to
prevent.
The log would need to sit before the `if (validationHappened)` split, or
track "no store had a matching overload" separately from "a store said INVALID".
### 2. …and it fires on ordinary wrong-password logins, misdiagnosing them
(minor)
`TomEEDefaultIdentityStore.validate` returns `NOT_VALIDATED_RESULT` (not
`INVALID_RESULT`) when the user exists but the password does not match. So for
the single most common production failure — a known caller typing a wrong
password with only the Tomcat user store deployed — `validationHappened` stays
false and the new block logs *"No IdentityStore validated a credential of type
…; Check that a store declares validate(UsernamePasswordCredential) or
validate(Credential)"*. That is backwards: a store did declare and did run the
matching method.
Combined with finding 1: the message is silent where it is needed and
misleading where it is not. If the block stays where it is, the wording should
not assert an overload mismatch as the cause.
### 3. Stores declaring `validate(BasicAuthenticationCredential)` silently
stop authenticating (minor)
The exact-match dispatch cuts both ways. `BasicAuthenticationCredential
extends UsernamePasswordCredential`, so pre-PR a store declaring
`validate(BasicAuthenticationCredential)` bound successfully; post-PR it does
not match `credential.getClass()` and falls through to `NOT_VALIDATED_RESULT`.
Such an app was already non-portable (Soteria and this module's own
`FormAuthenticationMechanism` have always passed a plain
`UsernamePasswordCredential`), so the direction of the fix is right — but it is
a silent runtime break with no compile error and, per finding 1, no log. Worth
a note in the JIRA/changelog.
To be explicit about one thing the review pass initially suggested and the
refutation pass rejected: do *not* add a "retry with
`BasicAuthenticationCredential` when the `UsernamePasswordCredential` attempt
yields NOT_VALIDATED" fallback. That re-introduces double dispatch and can
invoke a store twice with two different credential objects.
### 4. A throwing IdentityStore is swallowed into a bare 401 (nit,
pre-existing)
`BasicAuthenticationMechanism`'s `catch (IllegalArgumentException |
IllegalStateException e)` has an empty body and a comment claiming the header
was invalid. Per the 4.0.0 bytecode, `IdentityStore.validate(Credential)`'s
default implementation catches `Throwable` from the dispatched overload and
rethrows it as `new IllegalStateException(t)`, which propagates through the
handler untouched and lands here. So a JDBC pool exhaustion or LDAP timeout
inside an application store surfaces as a silent 401.
Entirely pre-existing and untouched by this diff, so only a nit — but it is
the same "undiagnosable 401" problem the PR is about, so it may be worth
folding in. If you do add logging there, it must be debug-level:
`parseAuthenticationHeader`'s `new BasicAuthenticationCredential("")` fallback
always throws `IllegalArgumentException` from `decodeHeader`, so that catch
fires on every anonymous request.
Issue Time Tracking
-------------------
Worklog Id: (was: 1036155)
Time Spent: 1h (was: 50m)
> Jakarta Security: BASIC mechanism rejects valid credentials with an
> application IdentityStore
> ---------------------------------------------------------------------------------------------
>
> Key: TOMEE-4648
> URL: https://issues.apache.org/jira/browse/TOMEE-4648
> Project: TomEE
> Issue Type: Bug
> Reporter: Markus Jung
> Assignee: Markus Jung
> Priority: Major
> Time Spent: 1h
> Remaining Estimate: 0h
>
> When the BASIC authentication mechanism validates credentials against an
> application-supplied {{IdentityStore}}, TomEE answers 401 even when the
> caller sends valid credentials. This shows up in the plain BASIC test and in
> the decorated and custom-handler variants
> ({{AppCustomAuthenticationMechanismHandler2IT}}), so the problem is not about
> wrapping the mechanism.
> h2. Root cause
> The BASIC mechanism passed a {{BasicAuthenticationCredential}} to
> {{IdentityStoreHandler.validate(Credential)}}. The spec's default
> {{IdentityStore.validate(Credential)}} dispatches to a {{validate(...)}}
> overload only on an _exact_ parameter-type match (its javadoc explicitly
> states it does not look for the most specific overload). Because
> {{BasicAuthenticationCredential extends UsernamePasswordCredential}}, a store
> declaring the idiomatic {{validate(UsernamePasswordCredential)}} overload —
> exactly what the TCK's {{TestIdentityStore}} does — was never invoked and
> returned {{NOT_VALIDATED}}, producing a 401 for correct credentials. Built-in
> stores (e.g. Tomcat users) were unaffected, which is why it only surfaced
> with an application store.
> Fix: the BASIC mechanism now hands the identity store a plain
> {{UsernamePasswordCredential}} while still parsing the header via
> {{BasicAuthenticationCredential}}. This matches the Soteria RI, which
> constructs {{new UsernamePasswordCredential(...)}} for the same reason.
> h2. Behaviour change (release note)
> This is _not_ behaviour-preserving in one direction. An application
> {{IdentityStore}} that declares {{validate(BasicAuthenticationCredential)}}
> was previously dispatched to by the exact-match rule and will no longer be
> invoked on the BASIC path; the default {{validate(Credential)}} then applies
> and the store returns {{NOT_VALIDATED}}, so the caller gets a 401. Such
> stores must declare {{validate(UsernamePasswordCredential)}} (or
> {{validate(Credential)}}) instead. This aligns TomEE with the Jakarta
> Security RI, but it is a breaking change for any application relying on the
> previous TomEE-specific behaviour.
> To make this class of failure diagnosable,
> {{TomEEIdentityStoreHandler.validate}} now logs at debug when no store
> validated the credential — previously the caller simply received a 401 with
> nothing logged anywhere, which is what made this issue hard to pin down.
> h2. Steps to reproduce / TCK reference
> Run the Jakarta Security 4.0 TCK reactor against TomEE Plus (Java 21) through
> the {{security}} runner in {{runner-standalone}}. The following tests fail
> and are excluded in {{runner-standalone/exclusions/security.txt}} in the
> apache/tomee-tck harness repo:
> * {{AppCustomAuthenticationMechanismHandler2IT}} (3 failing methods)
> * {{AppMemBasicDecorateIT#testAuthenticated}}
> * {{AppMemBasicIT#testAuthenticated}}
> Remove the matching lines from {{security.txt}} once fixed, then re-run the
> {{security}} runner to confirm all three test classes pass.
> h2. Note on the OpenID modules (not a TomEE bug)
> {{OpenId2DefaultIT}} and {{OpenId3DefaultIT}} were previously listed here as
> token-validation failures. Investigation showed this was a test-harness
> environment problem, not a TomEE defect: these modules start their bundled
> OpenID provider through Tomcat's {{startup.sh}}, which requires
> {{JAVA_HOME}}/{{JRE_HOME}} and ignores {{PATH}}. With those unset the
> provider never started, the client's {{.well-known}} discovery fetch was
> refused, and the tests failed downstream in a way that looked like a token
> check. With {{JAVA_HOME}} set, both OpenID modules pass unchanged. The runner
> has been fixed in apache/tomee-tck to derive {{JAVA_HOME}} when unset; the
> two OpenID entries can be dropped from {{security.txt}} once a corrected CI
> run confirms them.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
