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/