Follow up to improve webauthn error messages - #51463
Conversation
Signed-off-by: uzmamansoor09 <Uzma.Mansoor@ibm.com>
There was a problem hiding this comment.
Pull request overview
Adds localized, verifier-specific WebAuthn error reporting for registration and authentication.
Changes:
- Maps six webauthn4j verification exceptions to message keys.
- Adds English messages and event/error handling.
- Adds integration coverage and updates existing expectations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
themes/.../messages_en.properties |
Adds verifier-specific messages. |
services/.../Messages.java |
Defines new message keys. |
services/.../WebAuthnRegister.java |
Maps registration verifier failures. |
services/.../WebAuthnAuthenticator.java |
Reports mapped authentication failures. |
tests/.../WebAuthnErrorMessagesTest.java |
Tests mappings, events, and messages. |
tests/.../WebAuthnPolicyComplianceTest.java |
Updates expected specific errors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rmartinc
left a comment
There was a problem hiding this comment.
Thanks @uzmamansoor09 for the PR! LGTM in general.
Please link the PR and the commit to the issue #44963 (add the Closes #44963 in your commit). I'm going to close the other PR saying that is superseded by this one.
I just added two nit-picks for your consideration. Not needed, just suggestion if you think that they are useful. I just added them because you need to change the commit to link the issue, so you need to modify the commit anyway.
| String messageKey = getWebAuthnErrorMessageKey(wae); | ||
| String errorCase = messageKey != null ? messageKey : WEBAUTHN_ERROR_AUTH_VERIFICATION; |
There was a problem hiding this comment.
Nit-pick: You can just return WEBAUTHN_ERROR_AUTH_VERIFICATION or WEBAUTHN_ERROR_REGISTRATION as the default in the getWebAuthnErrorMessageKey and avoid this. You can pass a isRegistration parameter like in the other map method.
| String messageKey = getWebAuthnErrorMessageKey(wae); | ||
| String errorCase = messageKey != null ? messageKey : WEBAUTHN_ERROR_REGISTRATION; |
There was a problem hiding this comment.
Same here. You can return WEBAUTHN_ERROR_REGISTRATION as the default in the method when isRegistration parameter is true in getWebAuthnErrorMessageKey.
| /** | ||
| * Maps server side webauthn4j verifier exceptions to localizable Keycloak message keys | ||
| */ | ||
| public static String getWebAuthnErrorMessageKey(WebAuthnException exception) { |
There was a problem hiding this comment.
You can add a boolean isRegistration parameter and return WEBAUTHN_ERROR_REGISTRATION or WEBAUTHN_ERROR_AUTH_VERIFICATION instead of null as the default.
| EventRepresentation event = null; | ||
|
|
||
| for (int i = 0; i < 10 && event == null; i++) { | ||
| EventRepresentation polled = events.poll(); | ||
| if (polled == null) { | ||
| break; | ||
| } | ||
| if (polled.getDetails() != null && polled.getDetails().containsKey(errorLabel)) { | ||
| event = polled; | ||
| } | ||
| } | ||
|
|
||
| Assertions.assertNotNull(event, "No error event with detail '" + errorLabel + "' was found"); | ||
|
|
||
| EventAssertion.assertError(event) | ||
| .error(expectedError) | ||
| .details(errorLabel, expectedMessageKey); |
There was a problem hiding this comment.
| EventRepresentation event = null; | |
| for (int i = 0; i < 10 && event == null; i++) { | |
| EventRepresentation polled = events.poll(); | |
| if (polled == null) { | |
| break; | |
| } | |
| if (polled.getDetails() != null && polled.getDetails().containsKey(errorLabel)) { | |
| event = polled; | |
| } | |
| } | |
| Assertions.assertNotNull(event, "No error event with detail '" + errorLabel + "' was found"); | |
| EventAssertion.assertError(event) | |
| .error(expectedError) | |
| .details(errorLabel, expectedMessageKey); | |
| EventAssertion.assertError(events.poll()) | |
| .error(expectedError) | |
| .details(errorLabel, expectedMessageKey); |
You are doing correctly the clear of the events. So I think you can just get the next event and check it is the expected one. I have tested it and it is working for me. WDYT?
Closes #44963.
Mapping some webauthn4j verifier exceptions to specific, localized error messages