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
qualifie
d `@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
