Keycloak / PR #1

Fixing Re-authentication with passkeys

keycloak/keycloak#41249
Golden recall: — Extra findings: 5

Golden Comments (0/0 valid, 2 N/A)

# Comment Severity Status FriendlyReviewer Detail
#1 ConditionalPasskeysEnabled() called without UserModel parameter N/A N/A Golden bug does not apply — the method isConditionalPasskeysEnabled() in the actual PR code takes no UserModel parameter. It is a simple feature flag check.
#2 With UserModel parameter requiring user != null, authenticate(...) would not call webauthnAuth.fillContextForm(context) when context.getUser() is null N/A N/A Golden bug does not apply — in the actual PR code, isConditionalPasskeysEnabled() has no parameter and fillContextForm() is called regardless of context.getUser() being null or not.

Supplementary Findings (8 findings)

Finding File Severity Legitimate?
Import of AbstractUsernameFormAuthenticator.USER_SET_BEFORE_USERNAME_PASSWORD_AUTH in AuthenticatorUtils creates a bidirectional package dependency between util and browser AuthenticatorUtils.java:35 MEDIUM ✓ Real architectural concern — layer mixing
Return value of webauthnAuth.fillContextForm(context) ignored in passkeys-enabled block UsernamePasswordForm.java:115 MEDIUM ✓ Return value potentially null, silently discarded
Missing events.clear() after passkey login assertion — may cause interference with intermediate events PasskeysUsernamePasswordFormTest.java:396 MEDIUM ✓ Real test reliability issue
Missing events.clear() before re-authentication with prompt=login — residual events in queue PasskeysOrganizationAuthenticationTest.java:263 MEDIUM ✓ Same pattern, real fragility
Missing events.clear() after first login assertion before re-authentication PasskeysUsernameFormTest.java:313 MEDIUM ✓ Test fragility, same pattern
USER_SET_BEFORE_USERNAME_PASSWORD_AUTH changed from protected to public without documentation — API surface widened AbstractUsernameFormAuthenticator.java:58 LOW ✓ Should be documented
Unused local variable user in WebAuthnConditionalUIAuthenticator path after refactoring WebAuthnAuthenticator.java:99 LOW ✓ Cleanup justified
Try-catch for NoSuchElementException fragile — assertThrows would be more readable PasskeysUsernamePasswordFormTest.java:278 LOW ✓ Readability improvement

Honest Analysis

FriendlyReviewer detected 0 of 2 golden bugs in this PR, but both golden comments are not applicable. They describe a hypothetical bug where isConditionalPasskeysEnabled() takes a UserModel parameter — but in the actual PR code, the method takes no parameter and is called regardless of the user state. The golden data does not match the actual commit.

Despite the non-applicable golden set, FriendlyReviewer produced 8 supplementary findings. The most significant is the bidirectional package dependency: importing from AbstractUsernameFormAuthenticator into AuthenticatorUtils creates a circular dependency between the util and browser packages — a genuine architectural concern.

The three events.clear() findings (affecting three different test files) follow the same pattern: missing event queue cleanup between login and re-authentication assertions. All three are legitimate test reliability issues.

The ignored return value of webauthnAuth.fillContextForm(context) is a valid defensive coding concern, though it would only become a bug if a future subclass returns null. The three low-severity findings (widened API surface, unused variable, fragile try-catch) are minor but genuine.

Overall, FriendlyReviewer correctly identified the real problems in this PR — architecture, test reliability, and defensive coding — even though the golden benchmark could not score it.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

⛅ Tech
5 medium, 3 low

This merge request implements passkey re-authentication support across the username/password authentication flow. The changes are functionally correct and maintain existing security measures (brute force protection, timing attack mitigation, user enumeration prevention). Key modifications include:

  • Extracting re-authentication form setup into a shared utility method (AuthenticatorUtils.setupReauthenticationInUsernamePasswordFormError()).
  • Making the USER_SET_BEFORE_USERNAME_PASSWORD_AUTH constant publicly accessible.
  • Moving WebAuthn conditional UI setup outside the user-set check to enable passkeys during both initial login and re-authentication.
  • Updating UsernameForm to no longer skip the form during re-authentication when passkeys are enabled.

While the implementation is solid, there are several areas for improvement, particularly around code cleanliness, test robustness, and an architectural concern about a new bidirectional package dependency.

Worth checking

  • 🟡 services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java (L35)
    Architectural concern: importing AbstractUsernameFormAuthenticator.USER_SET_BEFORE_USERNAME_PASSWORD_AUTH creates a bidirectional dependency between the util and browser packages. Consider moving the constant to a shared neutral location (e.g., an authentication session note key constants class) to avoid future refactoring difficulties.
  • 🟡 services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java (L115)
    The return value of webauthnAuth.fillContextForm(context) is ignored. Although currently safe (the method never returns null in the conditional UI path), this could silently discard a null value if a future subclass changes behavior. Consider handling the return value or adding logging.
  • 🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (L396)
    Missing events.clear() after the initial passkey login assertion. This could cause interference from unexpected intermediate events when re-authentication begins. Add events.clear() immediately after the initial login assertion to ensure a clean event queue.
  • 🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (L263)
    Missing events.clear() before re-authentication with prompt=login. Residual events from the initial login could be in the queue. Add events.clear() before the re-authentication attempt.
  • 🟡 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.java (L313)
    Missing events.clear() after the first login assertion before re-authentication. Adding it would make the test more robust against unexpected intermediate events.

Small things (take or leave)

  • 🔵 services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java (L58)
    Access modifier changed from protected to public for USER_SET_BEFORE_USERNAME_PASSWORD_AUTH. While necessary for cross-package access, this widens the public API surface and should be documented.
  • 🔵 services/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnAuthenticator.java (L99)
    The user local variable is now only used inside the if (shouldShowWebAuthnAuthenticators(context)) block, leaving it unused in the WebAuthnConditionalUIAuthenticator subclass path. Consider moving the variable declaration inside the if block for clarity.
  • 🔵 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (L278)
    The try-catch pattern to assert absence of the webauthn form is fragile and hard to read. Consider using Assert.assertThrows(NoSuchElementException.class, ...) for clearer, more maintainable assertions.