Skip to content

Fixing Re-authentication with passkeys#10

Open
zaibkhan wants to merge 2 commits into
improve-auth-user-experiencefrom
enhance-passkey-authentication-flow
Open

Fixing Re-authentication with passkeys#10
zaibkhan wants to merge 2 commits into
improve-auth-user-experiencefrom
enhance-passkey-authentication-flow

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

mposolda and others added 2 commits July 22, 2025 10:00
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.
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Fix passkeys check and gating, prevent compile error
What’s good: Strong test coverage added for passkeys re-auth flows and centralization of re-auth form setup via AuthenticatorUtils improves maintainability.
Review Status: ❌ Requires changes
Overall Priority: Critical

Issues (Critical & High only)

Severity Issue Why it matters
Critical Correctness — Compile-time error: wrong method signature for isConditio... …/browser/UsernameForm.java
This calls isConditionalPasskeysEnabled() with no arguments, but the available method signature is isConditionalPasskeysEnabled(UserModel). This will not compile and blocks the build. Pass the current user to the method.
High Correctness — Conditional UI not enabled before user selection in Usern... …/browser/UsernamePasswordForm.java
Requiring a non-null user to enable conditional UI prevents WebAuthn UI from being shown on the initial login page (no user selected), which contradicts the tests asserting the presence of the webauth form before user selection. Restore gating to realm-level passkeys enabled so fillContextForm is invoked even when user is not identified.

Showing top 2 issues. Critical: 1, High: 1. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Address a compile-time error in UsernameForm and restore conditional UI enablement on the initial login page when no user is selected to match tests and expected behavior.
  • Testing: New tests comprehensively cover re-authentication with passkeys and mixed credentials. Consider adding a focused assertion that the conditional UI is enabled on the initial UsernamePasswordForm (before user selection), to guard against regressions in gating conditions.
  • Documentation: Add brief Javadoc to isConditionalPasskeysEnabled(UserModel) to clarify that it returns true only when a user is identified, and document the intended scenarios for showing the WebAuthn authenticators vs. only enabling conditional UI.
  • Compatibility: Changing USER_SET_BEFORE_USERNAME_PASSWORD_AUTH to public maintains backwards-compatibility and appears safe given downstream usage through AuthenticatorUtils.
  • Performance: No performance concerns observed; changes are control-flow and form-attribute adjustments.
  • Security: No new security risks identified; credential checks and brute-force guards remain unchanged.
  • Open questions: Should conditional passkeys UI be enabled on the initial username/password page when no user is identified? Tests suggest yes; if so, UsernamePasswordForm should not require a non-null user for fillContextForm.

Confidence: 2/5 — Not ready to merge (1 critical · 1 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

@Override
public void authenticate(AuthenticationFlowContext context) {
if (context.getUser() != null) {
if (context.getUser() != null && !isConditionalPasskeysEnabled()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Critical: This calls isConditionalPasskeysEnabled() with no arguments, but the available method signature is isConditionalPasskeysEnabled(UserModel). This will not compile and blocks the build. Pass the current user to the method.

Suggested change
if (context.getUser() != null && !isConditionalPasskeysEnabled()) {
```suggestion
if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) {

}
}
// setup webauthn data when passkeys enabled
if (isConditionalPasskeysEnabled(context.getUser())) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: Requiring a non-null user to enable conditional UI prevents WebAuthn UI from being shown on the initial login page (no user selected), which contradicts the tests asserting the presence of the webauth form before user selection. Restore gating to realm-level passkeys enabled so fillContextForm is invoked even when user is not identified.

Suggested change
if (isConditionalPasskeysEnabled(context.getUser())) {
```suggestion
if (webauthnAuth != null && webauthnAuth.isPasskeysEnabled()) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants