-
Notifications
You must be signed in to change notification settings - Fork 0
Fixing Re-authentication with passkeys #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: improve-auth-user-experience
Are you sure you want to change the base?
Fixing Re-authentication with passkeys #10
Conversation
closes #41242 closes #41008 Signed-off-by: mposolda <[email protected]>
This change modifies the method signature to require a UserModel parameter for proper user context validation during conditional passkey checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Greptile OverviewConfidence Score: 0/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant UsernameForm
participant UsernamePasswordForm
participant WebAuthnConditionalUI
participant AuthContext
Note over User,AuthContext: Re-authentication Flow (prompt=login)
User->>Browser: Request with prompt=login
Browser->>UsernameForm: authenticate()
UsernameForm->>AuthContext: getUser()
AuthContext-->>UsernameForm: user (not null)
UsernameForm->>UsernameForm: isConditionalPasskeysEnabled(user)
UsernameForm->>WebAuthnConditionalUI: isPasskeysEnabled()
WebAuthnConditionalUI-->>UsernameForm: true
UsernameForm->>UsernamePasswordForm: super.authenticate()
UsernamePasswordForm->>AuthContext: setAuthNote(USER_SET_BEFORE_USERNAME_PASSWORD_AUTH)
UsernamePasswordForm->>WebAuthnConditionalUI: fillContextForm()
WebAuthnConditionalUI->>AuthContext: form.setAttribute(ENABLE_WEBAUTHN_CONDITIONAL_UI)
UsernamePasswordForm-->>Browser: Display login form with passkeys enabled
alt User enters wrong password
Browser->>UsernamePasswordForm: action() with password
UsernamePasswordForm->>UsernamePasswordForm: badPasswordHandler()
UsernamePasswordForm->>AuthenticatorUtils: setupReauthenticationInUsernamePasswordFormError()
AuthenticatorUtils->>AuthContext: getAuthNote(USER_SET_BEFORE_USERNAME_PASSWORD_AUTH)
AuthenticatorUtils->>AuthContext: form.setAttribute(USERNAME_HIDDEN, true)
UsernamePasswordForm->>WebAuthnConditionalUI: fillContextForm()
UsernamePasswordForm-->>Browser: Display error with passkeys still enabled
else User selects passkey
Browser->>UsernamePasswordForm: action() with webauthn data
UsernamePasswordForm->>WebAuthnConditionalUI: action()
WebAuthnConditionalUI->>AuthContext: Validate WebAuthn credential
WebAuthnConditionalUI-->>Browser: Success or error with passkeys enabled
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9 files reviewed, 1 comment
| @Override | ||
| public void authenticate(AuthenticationFlowContext context) { | ||
| if (context.getUser() != null) { | ||
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: isConditionalPasskeysEnabled() is called without passing the required UserModel user parameter
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { | |
| if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java
Line: 47:47
Comment:
**syntax:** `isConditionalPasskeysEnabled()` is called without passing the required `UserModel user` parameter
```suggestion
if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) {
```
How can I resolve this? If you propose a fix, please make it concise.
Test 1
Replicated from ai-code-review-evaluation/keycloak-greptile#1