Keycloak PR #21 — Live Benchmark

Golden Comments (2/2 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	`ConditionalPasskeysEnabled()` called without `UserModel` parameter	MEDIUM	✓ Found	"`UsernameForm.authenticate()` calls `isConditionalPasskeysEnabled()` with no arguments, but the only definition in the hierarchy is `protected boolean isConditionalPasskeysEnabled(UserModel user)` in `UsernamePasswordForm` (line 160). This is a compile error that breaks the entire `services` module and dependent tests" (authenticators/browser/UsernameForm.java L47)
#2	With `isConditionalPasskeysEnabled(UserModel user)` requiring `user != null`, `authenticate(...)` will not call `webauthnAuth.fillContextForm(context)` on the initial login page where `context.getUser()` is still null, so conditional passkey UI will not be set up for first-time passkey login. Consider whether this should also be enabled when no user has been selected yet so ID-less passkey authentication on the initial login form continues to work.	MEDIUM	✓ Found	"Conditional UI for WebAuthn is no longer set on the initial normal-login page. The block previously ran when `context.getUser() == null` but is now gated by `isConditionalPasskeysEnabled(context.getUser())`, which requires a non-null user. As a result, the `#webauth` form and `autocomplete="username webauthn"` are suppressed during normal login, contradicting the MR's own tests that assert the web form's presence (e.g. `passwordLoginWithNonDiscoverableKey`, `webauthnLoginWithExternalKey`). The same inversion applies to the `challenge()` override at line 137" (authenticators/browser/UsernamePasswordForm.java L115)

Supplementary Findings (7 findings)

Finding	File	Severity	Legitimate?
`shouldShowWebAuthnAuthenticators` unconditionally returns `false`, so `WebAuthnAuthenticator.fillContextForm` always takes the NOP branch and never populates `ALLOWED_AUTHENTICATORS` — silently changes behaviour for any future caller that passes an identified user	WebAuthnConditionalUIAuthenticator.java:65	MEDIUM	✓ Real latent behavior change, correctly labeled no-impact today
Re-authentication block asserts a second LOGIN event without a preceding `events.clear()` — the FIFO event queue may still contain post-login events, making the assertion order-dependent and flaky	PasskeysUsernameFormTest.java:312	MEDIUM	✓ Real flaky-test risk, also present in two sibling tests
The absence-check pattern for the webAuthn form catches all `Exception`, making the `fail(...)` line dead when the element is present and swallowing other `WebDriverException`s as "expected"	PasskeysUsernamePasswordFormTest.java:292	MEDIUM	✓ Real dead assertion and swallowed exceptions
Extraction of `USERNAME_HIDDEN`/`REGISTRATION_DISABLED` into `setupReauthenticationInUsernamePasswordFormError` is behavior-preserving and consistent with existing conventions	AuthenticatorUtils.java:123	LOW	✓ Real positive confirmation of the refactor; no change requested
Discoverable-key re-auth tests jump straight to `appPage.assertCurrent()` without asserting the re-authentication page or webauth form were presented — the event assertion would pass even if the passkey never surfaced	PasskeysOrganizationAuthenticationTest.java:310	LOW	✓ Real coverage gap in the re-auth path
`webauthnLoginWithDiscoverableKey_reauthentication` is essentially identical to the corresponding UsernameForm test and adds no org-specific coverage (the realm's org layer is bypassed)	PasskeysOrganizationAuthenticationTest.java:270	LOW	✓ Real duplicate test with no org-specific value
`configureTestRealm` newly added only to `PasskeysUsernamePasswordFormTest`, making `directAccessGrantsEnabled` inconsistent across the three test classes	PasskeysUsernamePasswordFormTest.java:73	LOW	✓ Real inconsistency; cosmetic unless the flag is genuinely needed

Honest Analysis

FriendlyReviewer found both golden bugs of this PR — recall 100%. G1 (the no-arg isConditionalPasskeysEnabled() call) is established with certainty rather than suspicion: the review checks the whole hierarchy, finds the only definition takes a UserModel, and names the blast radius — a compile error that breaks the entire services module and dependent tests. This is the strongest kind of finding, verifiable mechanically and blocking regardless of intent.

G2 is found with more force than the dataset itself: the golden merely asks whether conditional UI should also be set up on the initial page; the review asserts it is a regression, shows the block previously ran when context.getUser() == null, cites the MR's own tests (passwordLoginWithNonDiscoverableKey, webauthnLoginWithExternalKey) that the change now contradicts, and extends the same inversion to the challenge() override at L137. The finding is test-backed, which makes it hard to dismiss.

The seven supplementary findings are all test hygiene or latent-behavior items, none of them blockers. The most useful are the missing events.clear() (order-dependent LOGIN assertion, present in three tests) and the catch-all Exception absence-check that silently kills a fail() assertion. The hardcoded false in WebAuthnConditionalUIAuthenticator.shouldShowWebAuthnAuthenticators is honestly labeled no-impact today, and the L123 refactor confirmation is a legitimate positive rather than filler.

The main caveat is severity calibration: both goldens are ranked as blocking "close look" findings while the dataset rates them Medium. That is defensible for a broken build, but the review also asserts the compile error without running a build — the method-hierarchy argument makes it almost certainly right, yet a maintainer would still want the module compiled before trusting the verdict. A compile error and a UI regression are different orders of urgency; the review treats them identically.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌥️ Feat
2 high, 3 medium, 4 low	1 issue

The MR reworks passkey re-authentication across the browser-authenticator stack. The direction is sound: moving conditional-UI form setup to the user-identified case and centralizing re-auth form setup in AuthenticatorUtils.setupReauthenticationInUsernamePasswordFormError are appropriate, and the API changes are backward compatible. However, the review surfaced two blocking problems: a compile error in UsernameForm (calling a non-existent no-arg isConditionalPasskeysEnabled()) and a regression where conditional UI is no longer shown on the initial normal-login page of UsernamePasswordForm, contradicting existing/updated tests. Several integration-test robustness and coverage issues were also noted, including missing events.clear() calls, weak UI-state assertions in discoverable-key tests, a duplicate org re-auth test, inconsistent realm setup, and a fragile absence-check pattern. These do not block the functional intent but should be addressed for test reliability.

These might need a close look

🔴 services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java (L47)
UsernameForm.authenticate() calls isConditionalPasskeysEnabled() with no arguments, but the only definition in the hierarchy is protected boolean isConditionalPasskeysEnabled(UserModel user) in UsernamePasswordForm (line 160). This is a compile error that breaks the entire services module and dependent tests. Fix: pass context.getUser() or add a no-arg overload.
🔴 services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java (L115)
Conditional UI for WebAuthn is no longer set on the initial normal-login page. The block previously ran when context.getUser() == null but is now gated by isConditionalPasskeysEnabled(context.getUser()), which requires a non-null user. As a result, the #webauth form and autocomplete="username webauthn" are suppressed during normal login, contradicting the MR's own tests that assert the web form's presence (e.g. passwordLoginWithNonDiscoverableKey, webauthnLoginWithExternalKey). The same inversion applies to the challenge() override at line 137. The re-auth fix should not drop the initial-page conditional UI.

Worth checking

🟡 services/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnConditionalUIAuthenticator.java (L65)
shouldShowWebAuthnAuthenticators unconditionally returns false, so WebAuthnAuthenticator.fillContextForm always takes the NOP branch for this class, never populating ALLOWED_AUTHENTICATORS even during re-authentication. This is intentional for the concurrent username/password form, but it also affects the OrganizationAuthenticator consumer (blast radius 2). No functional change exists today because the org path only calls it with user == null, but the hardcoded false silently changes behaviour for any future caller that passes an identified user. Consider scoping the override to "conditional UI active" and documenting the org-path impact.
🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.java (L312)
The re-authentication block asserts a second LOGIN event without a preceding events.clear(). The FIFO event queue may still contain events emitted after the first login (e.g. CODE_TO_TOKEN/REFRESH_TOKEN), making the assertion order-dependent and flaky. The same omission exists in PasskeysOrganizationAuthenticationTest (line 312) and PasskeysUsernamePasswordFormTest (line 265). Recommend adding events.clear() before each re-auth expectLogin().
🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (L292)
The absence-check pattern for the webAuthn form relies on findElement throwing NoSuchElementException but catches all Exception, so the fail("Not expected to have webauthn button") line is dead when the element is present (an AssertionError is not caught) and can swallow other WebDriverExceptions (stale/obfuscated elements, timeouts) as "expected". Recommend a dedicated helper that catches only NoSuchElementException and rethrows others, applied at lines 290-296 and 310-317 (and similar patterns).

Small things (take or leave)

🔵 services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java (L123)
The extraction of USERNAME_HIDDEN/REGISTRATION_DISABLED logic into setupReauthenticationInUsernamePasswordFormError is behavior-preserving and consistent with existing auth-note conventions. No change requested; this is a positive confirmation of the refactor.
🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (L310)
The discoverable-key re-auth tests (also in PasskeysUsernameFormTest at line 310) jump straight to appPage.assertCurrent() without asserting that the "Please re-authenticate to continue" page or the webauth form were presented. The event assertion proves a passkey login occurred but would pass even if the re-auth page never surfaced the passkey. For parity with the UsernamePasswordForm test, assert the re-auth info message and webauth form presence before expecting auto-login.
🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (L270)
webauthnLoginWithDiscoverableKey_reauthentication is essentially identical to the corresponding UsernameForm test and adds no org-specific coverage (the realm's org layer is bypassed). The org-specific membership scenario is only exercised for initial login, not re-authentication. Consider adding a re-auth variant with REQUIRES_USER_MEMBERSHIP=true or dropping the duplicate to reduce redundant coverage.
🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (L73)
configureTestRealm was newly added only to PasskeysUsernamePasswordFormTest, enabling directAccessGrantsEnabled on the test-app client, while the other two test classes do not call it. This makes the direct-access-grant flag inconsistent across the three tests. If it's genuinely needed, enable it consistently; otherwise the unrelated setup change should be justified.

✅ Feature-level checklist

Not there yet

❌ Fix re-authentication with passkeys
The MR's intent is to fix re-authentication with passkeys, but a compile error in UsernameForm prevents the entire module from building, so the fix cannot be delivered as-is. Additionally, the change removes conditional UI from the normal-login page, which regresses existing login behavior and contradicts the MR's own tests, undermining the requested fix.

Fixing Re-authentication with passkeys