Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the change aligning Admin API credential-label clearing with the Account API.
No open issues 🎉
What changed in this PR
Aligns the Admin API with the Account API’s acceptance of empty credential labels, addressing #51440 while preserving duplicate-label validation for named credentials.
Changes:
- Treats empty or blank Admin API labels as requests to clear the label, storing
null. - Replaces the rejection test with coverage that clears two OTP credential labels without a duplicate-label conflict.
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/admin/user/UserCredentialTest.java |
Verifies empty and whitespace-only labels clear two OTP credential labels to null. |
services/src/main/java/org/keycloak/services/resources/admin/UserResource.java |
Normalizes empty or blank labels to null and documents the clearing behavior. |
Review effort: Balanced
|
thanks @vaceksimon for your review, looking forward to get it merged. I see at the moment merging shows blocked as it requires a code owners review. |
|
@shuseng8, we have discussed this within the team and decided that the second option is more optimal: reject empty credential labels in the user API. Sorry for the trouble. Would you still be up for providing a fix? Here is my suggestion: The validation should be included in AccountCredentialResource, but not only there, because of #53544. The credential label is also set during credential setup as a required action in OTP:
Passkeys:
@shuseng8 wdyt? |
|
Hi @vaceksimon, no trouble at all, and thanks for coming back after the team discussion. I'm happy to take on the new approach and reject empty credential labels in the Account API and the setup flows. My plan:
Would you prefer that I rework this PR (rebase, update the title and description) or open a fresh one? |
|
@shuseng8 I agree with the plan; you can carry on in this PR. One thing I forgot: add a section about this to changes-27_0_0.adoc at the end of Notable changes. |
91093ed to
f1b1223
Compare
f1b1223 to
79ba50a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new server-side WebAuthn label rejection lacks regression coverage for missing, empty, and whitespace-only labels in regular and passwordless registration, including the expected error, no credential storage, and challenge reuse after correction.
1 open issue
2 resolved since last review
Review effort: Balanced
Closes keycloak#51440 Signed-off-by: Shubham <87247136+shuseng8@users.noreply.github.com>
79ba50a to
1a3d0cb
Compare
|
Hi @vaceksimon, thanks for the review and suggestions! The changes have been addressed as per your plan. Empty/blank labels are now rejected in the Account API, the OTP setup (count check dropped) and Passkey registration (re-prompt + server-side check), and the note is added at the end of Notable changes in |
Closes #51440
This reworks #54849 after the team discussion in #54849 (comment).
Instead of allowing empty credential labels in the Admin API, empty or blank labels are now rejected in the Account REST API and in the OTP and Passkey setup flows. The Admin API already rejected empty labels and is unchanged.
Changes:
PUT /credentials/{id}/labelreturns 400missingCredentialLabelfor null, empty, or whitespace-only labels.changes-27_0_0.adoc.AI assistance: An AI agent was used to help implement this change from a written plan. I reviewed the code and tests and take responsibility for the contribution.