TOMEE-4648 - BASIC mechanism rejects valid credentials with an application IdentityStore

[jungm]

What

Fixes TOMEE-4648: the BASIC authentication mechanism answered 401 for valid credentials when validation went against an application-supplied IdentityStore.

Why

BasicAuthenticationMechanism 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 states explicitly that it does not look for the most specific overload. Since BasicAuthenticationCredential extends UsernamePasswordCredential, a store declaring the idiomatic validate(UsernamePasswordCredential) overload (exactly what the Jakarta Security TCK's TestIdentityStore does) was never invoked and returned NOT_VALIDATED, producing a 401.

Built-in stores (e.g. Tomcat users) declare their own credential handling and were unaffected, which is why this only surfaced with an application store — and why the plain, decorated, and custom-handler TCK variants all failed the same testAuthenticated check.

Change

BasicAuthenticationMechanism.validateRequest still parses the Authorization header via BasicAuthenticationCredential (which does the base64 decoding), but now hands the identity store a plain UsernamePasswordCredential, reusing the already-parsed Password (no plaintext round-trip). The empty/malformed-header path is unchanged — the constructor still throws IllegalArgumentException, caught as before to fall through to the challenge.

Tests

• New Tomee4648BasicGroupsTest mirrors the TCK app-mem-basic module (an @ApplicationScoped IdentityStore with only the validate(UsernamePasswordCredential) overload). It fails with 401 before the change and passes after, asserting caller name and both groups.
• Full tomee-security module is green (108 tests), including the existing BASIC negative cases (wrong password, unknown user, missing role).

TCK impact

Removes the cause of these exclusions in the apache/tomee-tck security runner:

• AppMemBasicIT#testAuthenticated
• AppMemBasicDecorateIT#testAuthenticated
• AppCustomAuthenticationMechanismHandler2IT

Note for reviewers

The two OpenID TCK modules originally bundled into TOMEE-4648 were not a TomEE bug — they were a test-harness environment issue (the bundled OpenID provider's startup.sh needs JAVA_HOME, which was unset). Both pass unchanged once JAVA_HOME is set; that fix lives in apache/tomee-tck and is out of scope for this PR. The ticket has been updated accordingly.

🤖 Generated with Claude Code

[rzo1]

Diagnosis is right and matches the RI — Soteria's BasicAuthenticationMechanism
constructs new UsernamePasswordCredential(credentials[0], new Password(credentials[1]))
for exactly this reason. Verified the test fails without the production hunk
(expected:<200> but was:<401>) and the module is green with it.

Three things before merge:

1. This is not behaviour-preserving in one direction. An application store that
   declares validate(BasicAuthenticationCredential) was previously dispatched to
   by the exact-match rule and now never is — the default validate(Credential)
   throws NoSuchMethodException, it's swallowed into NOT_VALIDATED_RESULT, and
   the user gets a 401 with nothing logged anywhere. Matching Soteria is still the
   right call, but this needs a line in the JIRA / release notes.

   Separately, and maybe as a follow-up: TomEEIdentityStoreHandler.validate could
   log at debug when every store returns NOT_VALIDATED. Today this whole class of
   failure is completely silent, which is why TOMEE-4648 was hard to pin down.

2. TestIdentityStore is @ApplicationScoped and src/test/resources/META-INF/beans.xml
   is a bare <beans/>, while AbstractTomEESecurityTest deploys the whole
   test-classes tree as one webapp. So this store becomes an active authentication
   store for every test in tomee-security and is consulted on every BASIC/FORM login
   in the module. It's harmless today because it returns INVALID_RESULT and the
   handler falls through to TomEEDefaultIdentityStore, but it's easy to trip over
   for whoever adds the next test here. Please scope it (@Vetoed + explicit
   registration, or a caller check that can't collide).

3. The servlet writes the kaz role line and the test never asserts it — that's the
   negative case, worth asserting false.

Follow-up JIRA worth filing: OpenIdAuthenticationMechanism.handleTokenResponse has
the same problem. It passes TomEEOpenIdCredential, which lives in a TomEE-internal
package, so given the same exact-parameter-type dispatch an application store on the
OpenID path can only be invoked by declaring validate(Credential) or importing a
TomEE-internal class. Not a regression and not something this PR must fix.

[jungm]

Thanks — all three addressed in 6f0e1f9, plus the two follow-ups filed.

1. Behaviour change documented. Added a "Behaviour change (release note)" section to TOMEE-4648 spelling out that a store declaring validate(BasicAuthenticationCredential) is no longer dispatched to on the BASIC path, that such stores must move to validate(UsernamePasswordCredential)/validate(Credential), and that this aligns us with Soteria at the cost of being breaking for anyone relying on the old TomEE-specific behaviour.

Took the debug-logging suggestion in this PR rather than deferring it: TomEEIdentityStoreHandler.validate now logs at debug when no store validated, naming the credential type and the stores consulted, and pointing at the exact-match dispatch rule. That's the branch that was silently returning NOT_VALIDATED_RESULT.

2. Test store scoped. You're right that a bare <beans/> plus the whole-tree webapp deployment made it a module-wide authentication store. It now returns NOT_VALIDATED_RESULT for any caller other than reza — which appears in no other test and in no conf/tomcat-users.xml entry — so the handler falls through to the remaining stores exactly as if it weren't deployed. I went with the caller check rather than @Vetoed + explicit registration because AbstractTomEESecurityTest uses the embedded container rather than ApplicationComposer, so there's no per-test @Classes hook to register it back through; @Vetoed alone would remove the store the test needs. Happy to switch if you'd prefer a different shape.

3. kaz asserted as false.

Re-verified the test still fails without the production hunk (expected:<200> but was:<401>) after the strengthening, and the module is green at 108 tests.

Follow-up filed: TOMEE-4660 for the OpenID path — confirmed handleTokenResponse passes TomEEOpenIdCredential from the internal org.apache.tomee.security.http.openid.model package, so a portable application store can't name a type to overload on at all. Left out of this PR as you suggested.

🤖 Addressed by Claude Code

[rzo1]

can you run a full build for this one on CI ?

[jungm]

https://ci-builds.apache.org/job/Tomee/view/tomee-11.x/job/pull-request-manual-with-tck/55/

[rzo1]

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.