Fixing Re-authentication with passkeys#1
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 |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes refactor WebAuthn reauthentication handling by centralizing error form setup into a dedicated utility method, introducing conditional passkey enablement checks across authenticators, and adding new helper methods to determine WebAuthn visibility. Comprehensive tests validate passkey-based reauthentication flows with discoverable and external keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In
@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java:
- Line 47: The call in UsernameForm.java currently invokes
isConditionalPasskeysEnabled() without the required UserModel parameter; update
the call to pass the current user (context.getUser()) so it matches the method
signature isConditionalPasskeysEnabled(UserModel user) used in
UsernamePasswordForm.java and avoids the missing-parameter compile error.
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java:
- Line 413: Fix the typo in the inline comment inside the
PasskeysUsernamePasswordFormTest class: change the word "Succesful" to
"Successful" in the comment that reads "// Succesful event - password login" so
the comment correctly reads "// Successful event - password login".
🧹 Nitpick comments (2)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.java (1)
258-321: Comprehensive re-authentication test with passkeys.The test effectively validates the passkey-based re-authentication flow using
prompt=login, covering:
- Initial automatic login with discoverable keys
- Successful re-authentication with passkeys
- Proper event logging for both authentication attempts
Optional: Consider adding UI state assertions
While the functional behavior is well-tested, you might consider adding assertions to verify the UI state during re-authentication (e.g., username field hidden, registration disabled) to comprehensively validate all aspects of the PR changes. However, this may be deferred if UI state testing is handled elsewhere or if the test framework doesn't easily support such assertions.
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (1)
291-296: PreferfindElements()for element absence checks.Using try-catch with
fail()to verify element absence is fragile and verbose. Consider usingfindElements()which returns an empty list if no elements match.Suggested improvement
- try { - MatcherAssert.assertThat(driver.findElement(By.xpath("//form[@id='webauth']")), nullValue()); - fail("Not expected to have webauthn button"); - } catch (Exception nsee) { - // expected - } + MatcherAssert.assertThat( + "WebAuthn form should not be present", + driver.findElements(By.xpath("//form[@id='webauth']")), + Matchers.empty() + );Also applies to: 312-317
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.javaservices/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.javaservices/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.javaservices/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnAuthenticator.javaservices/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnConditionalUIAuthenticator.javaservices/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.javatestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.javatestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.javatestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java
🧰 Additional context used
🧬 Code graph analysis (5)
services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java (1)
services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java (1)
AuthenticatorUtils(40-135)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.java (2)
services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java (1)
OIDCLoginProtocol(78-570)server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-222)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (3)
services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java (1)
OIDCLoginProtocol(78-570)server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-222)testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/WaitUtils.java (1)
WaitUtils(53-192)
services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java (1)
services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java (1)
AbstractUsernameFormAuthenticator(49-261)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (2)
core/src/main/java/org/keycloak/representations/idm/RealmRepresentation.java (1)
RealmRepresentation(40-1504)server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-222)
🔇 Additional comments (22)
services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java (2)
113-117: Conditional WebAuthn setup looks correct.The conditional check properly gates WebAuthn context filling based on whether conditional passkeys are enabled. The null-safety is handled within the helper method, making this robust for both initial login and re-authentication scenarios.
160-162: Well-structured helper method.The
isConditionalPasskeysEnabledmethod appropriately centralizes the conditional passkeys check and includes proper null-safety for the user parameter. This improves maintainability and consistency across the authentication flow.services/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnConditionalUIAuthenticator.java (2)
52-53: Proper integration of reauthentication error handling.The call to
AuthenticatorUtils.setupReauthenticationInUsernamePasswordFormError(context)correctly maintains reauthentication state during passkey authentication errors by hiding the username field and disabling registration when appropriate.
63-67: Appropriate visibility control for WebAuthn UI.The
shouldShowWebAuthnAuthenticatorsmethod correctly returnsfalseto prevent WebAuthn authenticators from appearing during conditional passkey flows (e.g., username/password login), aligning with the intended UX for conditional UI.services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.java (2)
58-58: Good practice: Making the constant public.Changing the visibility from
protectedtopublicallows other classes (likeAuthenticatorUtils) to reference this constant instead of using magic strings, improving maintainability and reducing the risk of typos.
222-222: Excellent refactoring to centralize re-authentication form setup.Replacing inline form configuration with
AuthenticatorUtils.setupReauthenticationInUsernamePasswordFormError(context)centralizes the reauthentication UI state management, ensuring consistency across all authentication error paths. This follows the DRY principle and makes the code more maintainable.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (4)
35-35: LGTM!Import added for
OIDCLoginProtocol.PROMPT_VALUE_LOGINused in the new reauthentication test.
197-200: LGTM!The updated assertions correctly reflect the intended behavior change: passkeys should remain enabled on the username/password form after username entry, rather than being hidden. This aligns with the PR's objective of fixing passkey re-authentication flows.
210-211: LGTM!Consistent with the earlier assertion changes - passkeys remain available after failed login attempts.
267-321: LGTM!Well-structured test for passkey reauthentication with discoverable keys:
- Performs initial login with discoverable passkey
- Triggers re-authentication via
prompt=login- Validates automatic passkey login succeeds during re-authentication
- Event assertions correctly verify credential type and user verification
This directly validates the fix for issues #41242 and #41008 mentioned in the commit message.
services/src/main/java/org/keycloak/authentication/authenticators/util/AuthenticatorUtils.java (3)
26-26: LGTM!Import added for
LoginFormsProviderused in the new utility method to configure form attributes.
35-36: LGTM!Static import for the
USER_SET_BEFORE_USERNAME_PASSWORD_AUTHconstant, which is now publicly exposed fromAbstractUsernameFormAuthenticatorto enable centralized re-authentication checks.
122-133: LGTM!This utility method effectively centralizes the re-authentication form setup logic that was previously duplicated. Key points:
Boolean.parseBoolean()is null-safe (returnsfalsefor null input)- Correctly hides username field and disables registration during re-authentication to prevent user switching
- Aligns with the usage pattern in
AbstractUsernameFormAuthenticator.badPasswordHandler()per the relevant code snippetservices/src/main/java/org/keycloak/authentication/authenticators/browser/WebAuthnAuthenticator.java (2)
100-101: LGTM!Good refactoring - extracting the condition into a method call enables subclass customization of WebAuthn authenticator visibility.
124-130: LGTM!Well-designed protected helper method with clear documentation. This enables subclasses (e.g.,
WebAuthnConditionalUIAuthenticator) to override the visibility decision for WebAuthn authenticators based on flow-specific conditions like conditional passkeys.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (7)
26-53: LGTM!New imports support the added test functionality:
Assertfor assertions,PasswordCredentialModelandOIDCLoginProtocolfor credential/protocol handling,ApiUtilfor user lookup, and static imports for cleaner assertions.
73-73: LGTM!Added
configureTestRealmcall to set up the realm with the required configuration for passkeys testing.
152-168: LGTM!Updated assertions use
nullValue()matchers appropriately for checking null values in error messages and credential types.
177-210: LGTM!Refactored to use the new
setPasswordlessPolicyForExternalKey()helper method, reducing code duplication across tests.
212-274: LGTM!Comprehensive test for passkey re-authentication with external keys:
- Initial login via external passkey (click authenticate link)
- Re-authentication via
prompt=loginwith correct info message assertion- Validates passkey form availability and successful re-authentication event
336-422: LGTM!Thorough test covering the interplay between password and passkey re-authentication:
- Validates passkey elements remain available after failed password attempts
- Tests both passkey and password re-authentication paths
- Verifies correct credential types in login events
424-431: LGTM!Good extraction of the common policy setup into a reusable helper method, reducing duplication across the reauthentication tests.
| @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 parameter.
The call to isConditionalPasskeysEnabled() is missing the required UserModel parameter. According to the method signature in UsernamePasswordForm.java (line 160), this method requires a UserModel user parameter.
🐛 Proposed fix
- 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
at line 47, The call in UsernameForm.java currently invokes
isConditionalPasskeysEnabled() without the required UserModel parameter; update
the call to pass the current user (context.getUser()) so it matches the method
signature isConditionalPasskeysEnabled(UserModel user) used in
UsernamePasswordForm.java and avoids the missing-parameter compile error.
| loginPage.login(getPassword(USERNAME)); | ||
| appPage.assertCurrent(); | ||
|
|
||
| // Succesful event - password login |
There was a problem hiding this comment.
Minor typo in comment.
"Succesful" should be "Successful".
Suggested fix
- // Succesful event - password login
+ // Successful event - password login📝 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.
| // Succesful event - password login | |
| // Successful event - password login |
🤖 Prompt for AI Agents
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java
at line 413, Fix the typo in the inline comment inside the
PasskeysUsernamePasswordFormTest class: change the word "Succesful" to
"Successful" in the comment that reads "// Succesful event - password login" so
the comment correctly reads "// Successful event - password login".
2 participants
Test 1
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#1