Skip to content

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

Open
jungm wants to merge 1 commit into
mainfrom
claude/tomee-4648-fix-79009f
Open

TOMEE-4648 - BASIC mechanism rejects valid credentials with an application IdentityStore#2851
jungm wants to merge 1 commit into
mainfrom
claude/tomee-4648-fix-79009f

Conversation

@jungm

@jungm jungm commented Jul 23, 2026

Copy link
Copy Markdown
Member

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

@jungm
TOMEE-4648 - BASIC mechanism: validate with UsernamePasswordCredential
9ceece7 
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, and BasicAuthenticationCredential
is a subclass of UsernamePasswordCredential, so an application store
declaring the idiomatic validate(UsernamePasswordCredential) overload was
never invoked and returned NOT_VALIDATED - producing a 401 for valid
credentials. Built-in stores were unaffected, so it only surfaced with an
application-supplied IdentityStore.

Hand the identity store a plain UsernamePasswordCredential (reusing the
already-parsed Password) while still parsing the header via
BasicAuthenticationCredential. Adds a test mirroring the TCK app-mem-basic
identity store.
@rzo1

rzo1 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jungm @rzo1