Fixing Re-authentication with passkeys

[ShashankFC]

Test 1nn---nReplicated from ai-code-review-evaluation/keycloak-greptile#1

[GitHoobar]

@claude

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 3s —— View job

---

Code Review Complete

• Read repository guidelines (CLAUDE.md)
• Review authentication changes
• Review test coverage
• Provide comprehensive feedback

---

Summary

This PR fixes re-authentication with passkeys (issues #41242 and #41008) by ensuring that passkey authentication remains available during re-authentication flows (e.g., when prompt=login is used). The changes are well-structured with good test coverage.

Positive Observations

✅ Good refactoring: Extracted setupReauthenticationInUsernamePasswordFormError() into AuthenticatorUtils for reusability
✅ Comprehensive test coverage: Added 3 new test methods covering various re-authentication scenarios
✅ Consistent error handling: Both AbstractUsernameFormAuthenticator and WebAuthnConditionalUIAuthenticator now use the same helper method
✅ Method extraction: New isConditionalPasskeysEnabled() method improves code readability and maintainability
✅ Proper override pattern: WebAuthnConditionalUIAuthenticator correctly overrides shouldShowWebAuthnAuthenticators() to return false

Issues & Suggestions

1. ⚠️ Method Signature Inconsistency (Minor)

Location: UsernameForm.java:47

The isConditionalPasskeysEnabled() method is called without parameters in UsernameForm, but it doesn't exist in that class. Looking at the parent UsernamePasswordForm, this method requires a UserModel parameter.

// UsernameForm.java:47
if (context.getUser() != null && !isConditionalPasskeysEnabled()) {

This should be:

if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) {

However, since context.getUser() != null is already checked, the call would always pass a non-null user to the method. This might work if there's method overloading, but it's not clear from the diff.

Recommendation: Ensure the method signature matches throughout the class hierarchy.

2. 📝 Missing JavaDoc (Minor)

Location: UsernamePasswordForm.java:160-162

The new isConditionalPasskeysEnabled() method lacks JavaDoc documentation.

Recommendation: Add JavaDoc explaining when conditional passkeys should be enabled:

/**
 * Checks if conditional passkeys UI should be enabled for the given user.
 * Conditional passkeys are enabled when passkeys feature is active and a user is identified.
 *
 * @param user the user model to check, must not be null
 * @return true if conditional passkeys should be enabled
 */
protected boolean isConditionalPasskeysEnabled(UserModel user) {

3. 💡 Code Duplication (Minor)

Locations:

• UsernamePasswordForm.java:115-117
• UsernamePasswordForm.java:137-140

The same conditional passkeys setup code appears in two places:

if (isConditionalPasskeysEnabled(context.getUser())) {
    webauthnAuth.fillContextForm(context);
}

Recommendation: Consider extracting this into a helper method like setupConditionalPasskeysIfEnabled(context) to reduce duplication.

4. 🔍 Visibility Change Justification

Location: AbstractUsernameFormAuthenticator.java:58

The USER_SET_BEFORE_USERNAME_PASSWORD_AUTH constant was changed from protected to public. While this enables access from AuthenticatorUtils, it broadens the API surface.

Question: Is this constant intended for external use, or could AuthenticatorUtils be moved into the same package to avoid needing public visibility?

5. ✅ Test Improvements Noted

The test changes are excellent:

• Added logout() call in webauthnLoginWithDiscoverableKey() test to clean up state
• Added comprehensive re-authentication tests for both UsernameForm and UsernamePasswordForm
• Added test for passkeys disabled scenario during re-authentication
• Proper test assertions with meaningful error messages

Security Considerations

🔒 No security issues identified. The changes maintain proper authentication flow and don't introduce vulnerabilities.

Minor Code Quality Notes

• Import optimization: Some unused imports were added (e.g., static org.hamcrest.Matchers.nullValue) - these could be cleaned up
• Comment clarity: Line 138 in UsernamePasswordForm.java has comment "setup webauthn data when possible" - could be more specific: "setup webauthn data during re-authentication"

Conclusion

This is a solid fix with good test coverage. The main concern is the potential method signature mismatch in UsernameForm.java:47. Please verify that the parameterless call to isConditionalPasskeysEnabled() is correct, or add the required context.getUser() parameter.

Overall Assessment: ✅ Approve with minor suggestions

---