Fixing Re-authentication with passkeys#10
Conversation
closes #41242 closes #41008 Signed-off-by: mposolda <mposolda@gmail.com>
This change modifies the method signature to require a UserModel parameter for proper user context validation during conditional passkey checks.
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR refactors WebAuthn passwordless authentication by centralizing reauthentication setup into a utility method, introducing conditional passkey checking across authenticators, exposing a flag for broader access, and adding comprehensive test coverage for passwordless re-authentication flows with various credential scenarios. Changes
Sequence DiagramssequenceDiagram
actor User
participant LoginFlow as Login Flow
participant UsernameForm as UsernameForm
participant WebAuthn as WebAuthn Authenticator
participant AuthUtils as AuthenticatorUtils
participant Form as Form Provider
User->>LoginFlow: Initiate Authentication
LoginFlow->>UsernameForm: authenticate()
rect rgb(220, 240, 255)
Note over UsernameForm: Check: user set AND<br/>conditional passkeys disabled?
alt User set + Passkeys disabled
UsernameForm->>LoginFlow: success() + return early
else Continue normal flow
UsernameForm->>WebAuthn: super.authenticate()
end
end
rect rgb(240, 220, 255)
Note over WebAuthn: Check:<br/>shouldShowWebAuthnAuthenticators()?
alt Show WebAuthn
WebAuthn->>Form: Display WebAuthn authenticators
else Skip WebAuthn
WebAuthn->>LoginFlow: Continue to next handler
end
end
sequenceDiagram
participant Handler as Bad Password Handler
participant Context as Auth Context
participant AuthUtils as AuthenticatorUtils
participant Form as LoginFormsProvider
Handler->>Context: Bad password detected
rect rgb(255, 240, 220)
Note over Handler: New: Call centralized<br/>reauthentication setup
Handler->>AuthUtils: setupReauthenticationInUsernamePasswordFormError(context)
end
rect rgb(200, 255, 200)
AuthUtils->>Context: getAuthNote(USER_SET_BEFORE_...)
alt Reauthentication triggered
AuthUtils->>Form: setHideUsername(true)
AuthUtils->>Form: setDisableRegistration(true)
end
end
Handler->>Form: challenge() with reauthentication state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (1)
269-321: Consider extracting common setup to reduce duplication.The test correctly validates the re-authentication flow with passkeys. However, lines 271-302 duplicate the setup and initial login from
webauthnLoginWithDiscoverableKey()(lines 88-119). Consider extracting the common setup (WebAuthn configuration, user registration, and initial login verification) into a helper method to improve maintainability.Example refactor to reduce duplication:
// Add helper method to the class private UserRepresentation setupAndLoginWithDiscoverableKey() throws IOException { try (Closeable c = getWebAuthnRealmUpdater() .setWebAuthnPolicyRpEntityName("localhost") .setWebAuthnPolicyRequireResidentKey(Constants.WEBAUTHN_POLICY_OPTION_YES) .setWebAuthnPolicyUserVerificationRequirement(Constants.WEBAUTHN_POLICY_OPTION_REQUIRED) .setWebAuthnPolicyPasskeysEnabled(Boolean.TRUE) .update()) { checkWebAuthnConfiguration(Constants.WEBAUTHN_POLICY_OPTION_YES, Constants.WEBAUTHN_POLICY_OPTION_REQUIRED); registerDefaultUser(); UserRepresentation user = userResource().toRepresentation(); MatcherAssert.assertThat(user, Matchers.notNullValue()); logout(); events.clear(); // Auto-login with discoverable key oauth.openLoginForm(); WaitUtils.waitForPageToLoad(); appPage.assertCurrent(); events.expectLogin() .user(user.getId()) .detail(Details.USERNAME, user.getUsername()) .detail(Details.CREDENTIAL_TYPE, WebAuthnCredentialModel.TYPE_PASSWORDLESS) .detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, "true") .assertEvent(); return user; } }Then simplify both tests to use this helper.
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (5)
26-26: Unify JUnit assertion style for readabilityYou import
org.junit.Assert(line 26) and also statically importassertEqualsandfail(lines 51-53), then use both styles (Assert.assertEqualsand bareassertEquals). For consistency and easier scanning, consider standardizing on either the static imports or the class-qualified style across this test class.Also applies to: 51-53
152-152: Simplify “element should not exist” assertions and narrow exception handlingIn several places you rely on:
assertThat(..., nullValue())arounddriver.findElement(...)inside atry { ...; fail(...) } catch (Exception e) { /* expected */ }.Functionally it works (tests fail when the element exists and pass when a “not found” exception is thrown), but:
catch (Exception e)is very broad and can hide unrelated WebDriver or test infrastructure issues.- The
nullValue()matcher is redundant becausefindElementwill never returnnull; it either returns an element or throws.Consider a more explicit and narrow pattern, for example (conceptually):
// Pseudocode-style refactor: List<WebElement> elems = driver.findElements(By.xpath("//form[@id='webauth']")); assertTrue("Not expected to have webauthn button", elems.isEmpty());or, if you prefer exception-based checks, catch only the specific “element not found” exception type instead of generic
Exception.Also applies to: 291-296, 312-317
173-210:passwordLoginWithExternalKeytest behavior no longer matches its nameThe
passwordLoginWithExternalKeytest now:
- Configures an external resident key via
DefaultVirtualAuthOptions.DEFAULT_RESIDENT_KEY.- Sets the realm policy using
setPasswordlessPolicyForExternalKey().- Forces login using the WebAuthn link and asserts a successful passkey-based login.
There is no password-based login path exercised here anymore; the test exclusively validates passkey authentication with an external key. Consider renaming the test (e.g., to something like
passkeyLoginWithExternalKey) or adding an explicit password-login branch if the original intent was to cover both paths.
279-333: Re-authentication without passkey is well-specified; consider tightening exception scopeThis test nicely covers the case where the PASSKEYS feature is enabled but the realm has passkeys disabled:
- Realm is explicitly configured with
setWebAuthnPolicyPasskeysEnabled(FALSE).- Both initial login and subsequent
prompt=loginre-auth flow assert that the WebAuthn form is not present.- Final login uses password-only, and the event assertion checks that
USER_VERIFICATION_CHECKEDis null.The only concern is the use of
catch (Exception nsee)arounddriver.findElement(...): this will treat any exception as “expected”, not just “element not found”, which can mask unrelated issues. Narrowing this to the specific “element not found” exception type (while letting other exceptions fail the test) would make the test more reliable.
336-422: Re-authentication with either passkey or password is robustly exercisedThe
webauthnLoginWithExternalKey_reauthenticationWithPasswordOrPasskeytest does a good job of covering mixed-credential re-auth:
- Initial authentication via external passkey.
- Re-auth with
prompt=login, showing both password and passkey options.- Failed password attempt (using another user’s password), ensuring the error is shown and passkey option remains.
- Successful re-auth via passkey, with proper event details.
- Second re-auth with another failed password attempt, followed by successful re-auth via the correct password, and event assertion that
USER_VERIFICATION_CHECKEDis null.Two minor improvement opportunities:
- You might additionally assert the credential type for the final password-based login (e.g.,
Details.CREDENTIAL_TYPEwith the password credential type) for symmetry with the passkey assertion above.- As noted in another comment, fix the
.detail(..., nullValue())usage onUSER_VERIFICATION_CHECKEDto pass a plainnull.Functionally, though, the coverage and flow look solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java(2 hunks)services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java(1 hunks)services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java(3 hunks)services/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnAuthenticator.java(2 hunks)services/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnConditionalUIAuthenticator.java(3 hunks)services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java(3 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java(4 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.java(2 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java(8 hunks)
🔇 Additional comments (16)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.java (2)
37-37: LGTM! Import is required for the new re-authentication test.The
OIDCLoginProtocolimport is used in the new test method to access thePROMPT_VALUE_LOGINconstant for triggering re-authentication.
259-321: Well-structured test for passkey re-authentication flow.The test thoroughly validates that passkey authentication works during re-authentication when
prompt=loginis used. The implementation correctly:
- Configures passkey-only authentication by removing the password credential
- Verifies initial automatic login via discoverable key
- Tests re-authentication with
prompt=loginparameter (line 306)- Confirms passkey credential type in login events
The test follows the established pattern from
webauthnLoginWithDiscoverableKey()and properly handles the re-authentication scenario where no explicit logout is needed before triggeringprompt=login.Note: The AI summary mentions "the new test method appears twice in the file (duplicate insertion)," but only one instance is visible in the provided code. Please verify whether a duplicate exists elsewhere in the file.
services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java (2)
160-162: LGTM! Well-structured helper method.The new
isConditionalPasskeysEnabledhelper centralizes the conditional passkey logic that guards WebAuthn UI setup. The method correctly checks all three necessary conditions: WebAuthn authenticator instance exists, passkeys feature is enabled, and user is non-null.
113-117: Improved readability with centralized conditional check.The restructured code now uses the helper method to guard WebAuthn form setup, making the conditional logic more explicit and maintainable compared to inline checks.
services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java (1)
122-134: Well-designed utility for centralized reauthentication setup.This method effectively consolidates the logic for configuring the login form during re-authentication error scenarios. By reading the
USER_SET_BEFORE_USERNAME_PASSWORD_AUTHflag and conditionally hiding the username field while disabling registration, it ensures consistent UI behavior across different authenticator error paths.services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java (2)
58-58: Appropriate visibility change for cross-package access.Making
USER_SET_BEFORE_USERNAME_PASSWORD_AUTHpublic is necessary to support the centralized reauthentication setup inAuthenticatorUtils(which resides in a different package). This aligns with the broader refactoring to consolidate reauthentication UI logic.
222-222: Excellent refactor: eliminates duplication.Delegating to
AuthenticatorUtils.setupReauthenticationInUsernamePasswordFormError(context)consolidates the reauthentication form configuration logic, removing inline duplication and making the code more maintainable. This aligns with the DRY principle.services/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnAuthenticator.java (2)
124-130: Good polymorphic design for UI visibility control.Extracting the condition into a named
shouldShowWebAuthnAuthenticatorsmethod improves code clarity and enables subclasses likeWebAuthnConditionalUIAuthenticatorto override this behavior. The Javadoc clearly documents the typical use case (2-factor authentication scenarios).
101-101: Improved readability with intention-revealing method.Replacing the inline
user != nullcheck withshouldShowWebAuthnAuthenticators(context)makes the code more self-documenting and aligns with the broader refactoring to centralize WebAuthn UI logic.services/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnConditionalUIAuthenticator.java (2)
52-53: Consistent error handling with centralized setup.Invoking
AuthenticatorUtils.setupReauthenticationInUsernamePasswordFormError(context)in the error response path ensures the form is properly configured for re-authentication scenarios, maintaining consistency with other authenticators in the flow.
63-67: Correct polymorphic override for conditional UI behavior.Overriding
shouldShowWebAuthnAuthenticatorsto returnfalseis appropriate for the conditional passkeys flow (e.g., username/password with inline passkey support), where displaying the authenticators list would be redundant. The comment clearly documents this intent.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (2)
35-35: LGTM!The import is necessary for the new re-authentication test method and is correctly used at line 306.
197-211: Behavior change confirmed and correctly implemented.The webauth form element (
//form[@id='webauth']) is now conditionally rendered viapasskeys.ftlwhen theENABLE_WEBAUTHN_CONDITIONAL_UIattribute is set. Both the normal and error code paths inWebAuthnConditionalUIAuthenticatorset this attribute, ensuring the webauth form remains visible throughout the non-discoverable key reauthentication flow, allowing users to retry with passkeys after a password error.The implementation is correct and the test expectations at lines 200 and 211 align with the intended behavior where passkeys remain available during the username/password authentication flow.
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (3)
67-75: configureTestRealm hook in addTestRealms looks appropriateCalling
configureTestRealm(realmRepresentation);before adding the realm ensures any shared/passkey-specific realm tweaks from the base test infrastructure are applied uniformly. This makes the new re-authentication scenarios more robust against future realm configuration changes.
213-275: Re-auth with external passkey flow looks correct and well-coveredThe
webauthnLoginWithExternalKey_reauthenticationtest:
- Uses the external resident key setup and shared helper.
- Verifies initial login via passkey, then performs a re-authentication with
prompt=login.- Asserts the presence of the WebAuthn form, the re-auth info message, and that re-authentication via passkey succeeds with the expected event details.
The sequence and assertions match the intended behavior for passkey-based re-auth with external keys.
424-431: HelpersetPasswordlessPolicyForExternalKeynicely centralizes shared setupExtracting the common WebAuthn realm configuration into
setPasswordlessPolicyForExternalKey():
- Removes duplication across multiple tests.
- Makes the external-key policy (resident key required, UV required, passkeys enabled) explicit in one place.
- Preserves the try-with-resources pattern so realm changes are automatically reverted.
This is a clean and maintainable refactor.
| @Override | ||
| public void authenticate(AuthenticationFlowContext context) { | ||
| if (context.getUser() != null) { | ||
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { |
There was a problem hiding this comment.
Critical: Method call missing required argument.
The method isConditionalPasskeysEnabled() is defined in the parent class UsernamePasswordForm with a required parameter UserModel user (line 160 in UsernamePasswordForm.java), but it's being called here without any arguments. This will cause a compilation error.
Apply this diff to fix the method call:
- if (context.getUser() != null && !isConditionalPasskeysEnabled()) {
+ if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { | |
| if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) { |
🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java
around line 47, the call to isConditionalPasskeysEnabled() is missing the
required UserModel parameter; change the call to pass the current user (e.g.,
isConditionalPasskeysEnabled(context.getUser())) so the method signature matches
the parent class and compilation succeeds.
| } | ||
| } | ||
|
|
||
| // Test users is able to authenticate with passkey during re-authentication (for example when OIDC parameter prompt=login is used) |
There was a problem hiding this comment.
Fix grammatical error in comment.
The comment has a grammatical issue: "Test users is able" should be either "Test user is able" or "Tests that users are able".
Apply this diff to correct the grammar:
- // Test users is able to authenticate with passkey during re-authentication (for example when OIDC parameter prompt=login is used)
+ // Tests that users are able to authenticate with passkey during re-authentication (for example when OIDC parameter prompt=login is used)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Test users is able to authenticate with passkey during re-authentication (for example when OIDC parameter prompt=login is used) | |
| // Tests that users are able to authenticate with passkey during re-authentication (for example when OIDC parameter prompt=login is used) |
🤖 Prompt for AI Agents
In
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java
around line 268, the comment "Test users is able to authenticate with passkey
during re-authentication (for example when OIDC parameter prompt=login is used)"
is grammatically incorrect; update it to read "Tests that users are able to
authenticate with passkey during re-authentication (for example when OIDC
parameter prompt=login is used)" to correct subject-verb agreement and maintain
the original meaning.
| .user(user.getId()) | ||
| .detail(Details.USERNAME, USERNAME) | ||
| .detail(Details.CREDENTIAL_TYPE, Matchers.nullValue()) | ||
| .detail(Details.CREDENTIAL_TYPE, nullValue()) |
There was a problem hiding this comment.
🧩 Analysis chain
Do not pass Hamcrest nullValue() into events.detail(...)
In these expectations:
- Line 165:
.detail(Details.CREDENTIAL_TYPE, nullValue()) - Line 328:
.detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, nullValue()) - Line 417:
.detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, nullValue())
nullValue() is a Hamcrest Matcher, but detail(...) on the events expectation API is designed to take a String (or null) value, not a matcher. This is likely to be a compile-time type error and, even if it compiled via some overload, it would not perform the intended “no value present” check.
To express “this detail should be absent / null”, pass null explicitly and reserve nullValue() for assertThat(...) calls. For example:
- .detail(Details.CREDENTIAL_TYPE, nullValue())
+ .detail(Details.CREDENTIAL_TYPE, (String) null)
...
- .detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, nullValue())
+ .detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, (String) null)Apply the same pattern at all three call sites.
Please recompile the tests after this change to confirm there are no remaining type errors in the event expectation API.
Also applies to: 325-329, 413-418
I'll now verify this review comment by examining the file and understanding the detail() method signature.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the test file with context around the problematic lines
fd -t f "PasskeysUsernamePasswordFormTest.java" --exec cat {} \;
Length of output: 18421
🏁 Script executed:
#!/bin/bash
# Search for the detail() method definition in the codebase to understand its signature
rg -n "detail\s*\(" --type=java -B2 -A2 | head -100Length of output: 14036
🏁 Script executed:
#!/bin/bash
# Check if nullValue() is used and where it's defined/imported
rg -n "nullValue" --type=java | grep -E "(import|nullValue\(\))"Length of output: 50644
I found the file and confirmed the problematic code. Now I need to verify the detail() method signature to understand what it actually accepts.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the detail() method definition - likely in AssertEvents or EventExpectation class
rg -n "detail\s*\(\s*String" --type=java -B2 -A5 | head -80
Length of output: 6693
🏁 Script executed:
#!/bin/bash
# Search for the events class/interface definition that has the detail method
fd -t f "AssertEvents.java" --exec cat {} \; | head -200Length of output: 8404
🏁 Script executed:
#!/bin/bash
# Look for event expectation API classes
fd -t f "*Event*" --type=java | grep -i "expect\|assert" | head -20Length of output: 171
Fix all three locations to explicitly pass (String) null instead of nullValue()
The review comment is correct. While nullValue() is a Hamcrest Matcher that will compile via the detail(String key, Matcher<? super String> matcher) overload, this is semantically wrong. The intended behavior is to express "this detail should be absent," which is achieved by passing null explicitly to the detail(String key, String value) overload. Passing a Matcher attempts to match the detail value, which has different semantics.
Lines 165, 328, and 417 all need the cast applied:
- .detail(Details.CREDENTIAL_TYPE, nullValue())
+ .detail(Details.CREDENTIAL_TYPE, (String) null)- .detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, nullValue())
+ .detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, (String) null)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .detail(Details.CREDENTIAL_TYPE, nullValue()) | |
| .detail(Details.CREDENTIAL_TYPE, (String) null) |
🤖 Prompt for AI Agents
In
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java
around lines 165, 328 and 417 the test is incorrectly passing the Hamcrest
matcher nullValue() to detail(...) which selects the detail(String, Matcher)
overload; change each nullValue() to an explicit casted null by passing (String)
null so the detail(String, String) overload is used and the test expresses an
absent detail.
2 participants
Test 1
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#1