Implemented validation to ensure each OTP device has a unique label #38657

SaikrishnaPavuluri · 2025-04-03T14:25:09Z

### Summary of Changes
This PR enhances the OTP credential creation process by improving duplicate device validation and error handling.

Changes in JpaUserCredentialStore and JpaUserFederatedStorageProvider

Added duplicate credential validation for both create and update methods.
Ensured that users cannot register an OTP device with the same name multiple times.
Introduced validateDuplicateUserCredential to handle duplicate detection.

### Enhancements in createOTPCredential (CredentialHelper.java)

Changed return type from boolean to Map<String, Boolean> for more detailed status tracking:
isDeviceDuplicate: true if a duplicate OTP device exists.
isOTPCreationSuccess: true if OTP credential creation was successful.

Handled ModelDuplicateException to prevent duplicate device registrations.

### Updates in processAction() (UpdateTotp class)

Now checks isDeviceDuplicate before proceeding with OTP creation.
If a duplicate device is detected, it shows an error message:

"Device already exists with the same name"

If OTP creation fails for other reasons, it shows:

"Invalid TOTP"

Additional Update:

Added duplicate and empty device name validation in setCredentialUserLabel method (UserResource class).
This ensures users cannot update a userlabel (i.e device name) to a blank value or a label that already exists.

### Feedback & Review Request

This is my first contribution to Keycloak, and I would appreciate any feedback on the implementation, approach, or any best practices I might have missed.

Please find the validated screenshot after implementation

Let me know if any improvements or additional changes are needed. Thanks! 🙌

SaikrishnaPavuluri · 2025-04-07T09:30:56Z

Additional Update:

Added duplicate and empty device name validation in setCredentialUserLabel method (UserResource class).
This ensures users cannot update a userlabel (i.e device name) to a blank value or a label that already exists.

Please find validated screenshots:

Thanks!

SaikrishnaPavuluri · 2025-04-08T16:09:40Z

Hi @ahus1,

I've made two commits to this draft PR and would really appreciate it if you could take a look whenever you get a chance. Please let me know if you have any feedback, suggestions, or improvements you'd like to see. I'd be happy to incorporate them.

Thank you for your time

ahus1

Thank you for the pull request. I pushed one more commit to suggest a change that doesn't make as many assumptions about the type of exceptions thrown, as there might be other exceptions appearing in other places.

I also see that you reformatted parts of the code, so the indentation has changed. Please revert those changes as they make it hard to review the changes on GitHub.

ahus1 · 2025-04-08T18:57:58Z

services/src/main/java/org/keycloak/services/resources/admin/UserResource.java

+
+        if (userLabel == null || userLabel.trim().isEmpty()) {
+            throw new ErrorResponseException("missingDeviceName", "Please specify the device name", Status.BAD_REQUEST);
+        }
+


As this is also used for all credentials including passwords, having an error message that is specific for a device name isn't a good fit here.

Hi @ahus1, I have updated the error to:

throw new ErrorResponseException("missingCredentialLabel", "Credential label must not be empty", Status.BAD_REQUEST);

Please let me know if this looks good or if you’d prefer a different message/key. Thanks!

ahus1 · 2025-04-08T19:00:12Z

Another hint: Please avoid to merge the main branch into the PR, instead rebase the pull request when needed. This makes it again simpler to review.

ahus1 · 2025-04-09T17:52:45Z

@SaikrishnaPavuluri - please don't merge the main branch into your PR, as this makes it difficult to review.

Rebasing should only be done when there are conflicts or a maintainer asks you to rebase. This avoid unnecessary CI runs and unnecessary notifications to maintainers.

ahus1

In a previous review I commented the following, which hasn't been addressed yet:

I also see that you reformatted parts of the code, so the indentation has changed. Please revert those changes as they make it hard to review the changes on GitHub.

The functional changes look good, please now add a test case that validates the new behavior. Maybe you can extend an existing test, maybe you need to write a new test. Let me know if you need pointers.

SaikrishnaPavuluri · 2025-04-09T18:02:43Z

@ahus1
Thanks! I've addressed the review changes.
Could you also please share some pointers on where I can add or extend the test case to validate the new behavior? That would help me get started in the right direction.

SaikrishnaPavuluri · 2025-04-09T18:56:10Z

@SaikrishnaPavuluri - please don't merge the main branch into your PR, as this makes it difficult to review.

Rebasing should only be done when there are conflicts or a maintainer asks you to rebase. This avoid unnecessary CI runs and unnecessary notifications to maintainers.

I have fixed the indentation issues and pushed the updated commit. Thanks a lot for your guidance I will be careful with formatting in future contributions to avoid this again.

SaikrishnaPavuluri · 2025-04-10T19:32:08Z

The functional changes look good, please now add a test case that validates the new behavior. Maybe you can extend an existing test, maybe you need to write a new test. Let me know if you need pointers.

Hi @ahus1
Could you please share the pointers you mentioned yesterday for adding the test case? Appreciate your help. Thanks!

ahus1 · 2025-04-10T19:33:22Z

@SaikrishnaPavuluri - I've read your comment, but allow for some more time to get back to you.

ahus1 · 2025-04-15T20:59:32Z

Ok, KC 26.2 has been released, and as the dust settles, here's my reply.

Sebastian and me recorded a session in Keycloak Hour of Code. See https://www.youtube.com/watch?v=gX9BWk-AiqE&list=PLFoHXRSurvbHNojMgLsn76-tdm13carIh&index=4&t=14s&ab_channel=AlexanderSchwartz for the video. And you find links including the script in the description of the video.

I hope this is helpful. Please let me know if you have questions.

SaikrishnaPavuluri · 2025-04-17T03:51:20Z

Ok, KC 26.2 has been released, and as the dust settles, here's my reply.

Sebastian and me recorded a session in Keycloak Hour of Code. See https://www.youtube.com/watch?v=gX9BWk-AiqE&list=PLFoHXRSurvbHNojMgLsn76-tdm13carIh&index=4&t=14s&ab_channel=AlexanderSchwartz for the video. And you find links including the script in the description of the video.

I hope this is helpful. Please let me know if you have questions.

I will try to complete this by the end of the week. Thanks

SaikrishnaPavuluri · 2025-04-19T13:57:24Z

Hi @ahus1, I have added test cases in the latest commit for setCredentialUserLabel, createCredential, and updateCredential to validate behavior with empty and duplicate device labels.

I also looked for a test class covering the processAction() method in UpdateTotp, but couldn’t find one. Please let me know if it exists elsewhere.

Thanks!

SaikrishnaPavuluri · 2025-04-24T03:31:39Z

Hi @ahus1
Could you please review the code for the integration tests and let me know if any changes are needed?
Thanks for your time!

… unique name Signed-off-by: Saikrishna <saikrishnap@optimeyes.ai>

…tCredentialUserLabel (UserResource) Signed-off-by: Saikrishna <saikrishnap@optimeyes.ai>

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

…Credential and updateCredential (duplicate label) Signed-off-by: Saikrishna <saikrishnap@optimeyes.ai> Fix indentation issues Completed review changes and reverted formatting

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

ahus1 · 2025-05-03T09:53:25Z

I found a place in AppInitiatedActionTotpSetupTest where to add a test and pushed it to this PR. As there were several merge commits, I also rebased it for better reviewability.

Let's look at the test results once the tests complete.

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

ahus1 · 2025-05-05T06:13:58Z

Thank you for the code and the test, the build is now green and the tests are stable. Unfortunately the changes broke some existing tests which I didn't foresee, and it also touched some browser-only tests that are difficult to reproduce locally.

I'm now marking it ready for review and have someone else from the team look at it as well.

When you look at my commits, please take a moment to review them. Given the documentation above, I hope it is sufficient to run a single test locally, and possibly even debug it. While running all tests locally is very time consuming and better handled by the GitHub actions, running a single test locally is a great skill that allows you to iterate faster when developing tests. If the documentation isn't sufficient, please let me know and we'll look into enhancing it.

ahus1 · 2025-05-05T06:15:54Z

@mposolda - this PR looks good to me, and the error messages shown to the users when they try to have a credential with the same label are IMHO descriptive.

As this touches the core-clients area, please give it a review as well. As this is a first-time contribution, I'm willing to give a helping hand where needed.

Thanks!

SaikrishnaPavuluri · 2025-05-05T07:40:08Z

Thank you for the code and the test, the build is now green and the tests are stable. Unfortunately the changes broke some existing tests which I didn't foresee, and it also touched some browser-only tests that are difficult to reproduce locally.

I'm now marking it ready for review and have someone else from the team look at it as well.

When you look at my commits, please take a moment to review them. Given the documentation above, I hope it is sufficient to run a single test locally, and possibly even debug it. While running all tests locally is very time consuming and better handled by the GitHub actions, running a single test locally is a great skill that allows you to iterate faster when developing tests. If the documentation isn't sufficient, please let me know and we'll look into enhancing it.

@ahus1 I’m truly grateful for all the detailed feedback and support you’ve given me throughout my first contribution. Your guidance has been invaluable not only in understanding development and debugging, but also in helping me navigate the codebase with more confidence. The video you and Sebastian shared was fantastic and really helped clarify many concepts.

I’m happy to have a mentor like you and excited to keep learning, contributing, and growing. Your encouragement motivates me to take on more challenges. I’ll definitely take your advice and spend time understanding the tests better so it helps me in future contributions.

I’d really appreciate a review of this PR when time permits. As this is my first contribution, I’m eager to learn and grow with your guidance. It’s a privilege to collaborate with such experienced contributors, and I’m open to any suggestions or improvements you may have.

ahus1 · 2025-05-06T19:57:09Z

@SaikrishnaPavuluri - thank you for sharing this, and kudos so @srose for preparing the recordings!

SaikrishnaPavuluri · 2025-06-12T03:54:42Z

Hi @mposolda and @ahus1 just checking in is this PR ready to be merged, or is there anything else needed from my side?

mabartos · 2025-06-12T09:05:05Z

@rmartinc Could you or someone from your team check this PR as @mposolda is currently out? Thanks!

rmartinc

Thanks @SaikrishnaPavuluri and @ahus1! Changes LGTM. I was thinking about adding some note in the upgrading guide. But I understand that this is more a bug than something else. Approved.

ahus1 · 2025-06-12T10:07:39Z

@rmartinc - thank you for the review. I'll create a follow-up issue with a "notable change" to the upgrade guide.

UPDATE: #40445

SaikrishnaPavuluri mentioned this pull request Apr 3, 2025

Name for OTP device should be unique #38465

Closed

2 tasks

SaikrishnaPavuluri force-pushed the feature/38465-unique-otp-device-name branch 2 times, most recently from b98ce83 to e11ef12 Compare April 5, 2025 09:10

ahus1 changed the title ~~#38465 Implemented validation to ensure each OTP device has a unique …~~ Implemented validation to ensure each OTP device has a unique label Apr 8, 2025

ahus1 force-pushed the feature/38465-unique-otp-device-name branch from e0e429d to a1b5f99 Compare April 8, 2025 18:55

ahus1 requested changes Apr 8, 2025

View reviewed changes

ahus1 requested changes Apr 9, 2025

View reviewed changes

ahus1 self-assigned this Apr 15, 2025

Saikrishna and others added 5 commits May 3, 2025 11:05

keycloak#38465 Implemented validation to ensure each OTP device has a…

f5c424c

… unique name Signed-off-by: Saikrishna <saikrishnap@optimeyes.ai>

Fixes keycloak#38465: Validate duplicate and empty device names in se…

5410be4

…tCredentialUserLabel (UserResource) Signed-off-by: Saikrishna <saikrishnap@optimeyes.ai>

Review

e7f0a1f

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

test: add tests for setCredentialUserLabel (empty, duplicate), create…

13d67da

…Credential and updateCredential (duplicate label) Signed-off-by: Saikrishna <saikrishnap@optimeyes.ai> Fix indentation issues Completed review changes and reverted formatting

Adding a test for the AIA

3abafb0

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

ahus1 force-pushed the feature/38465-unique-otp-device-name branch from beada4b to 3abafb0 Compare May 3, 2025 09:49

ahus1 added 3 commits May 3, 2025 13:18

Fixing test

929daf3

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

Fixing existing tests

dc4e09d

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

Fixing existing tests

0d9144b

Closes keycloak#38465 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

ahus1 added 2 commits May 3, 2025 16:43

Fixing failing test

6f6bb82

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

Cleanup users at the end of the test to avoid distinct user names

95b0cda

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>

keycloak deleted a comment from keycloak-github-bot bot May 3, 2025

This comment was marked as outdated.

Sign in to view

keycloak deleted a comment from keycloak-github-bot bot May 3, 2025

ahus1 marked this pull request as ready for review May 5, 2025 06:14

ahus1 requested review from a team as code owners May 5, 2025 06:14

keycloak-github-bot bot added team/ui team/cloud-native team/core-clients team/core-iam labels May 5, 2025

ahus1 approved these changes May 5, 2025

View reviewed changes

ahus1 requested a review from mposolda May 5, 2025 06:16

rmartinc approved these changes Jun 12, 2025

View reviewed changes

ahus1 merged commit 76ab8bd into keycloak:main Jun 12, 2025
76 checks passed

ahus1 mentioned this pull request Jun 16, 2025

Extend upgrading guide for OTP device has a unique label #40482

Merged

Implemented validation to ensure each OTP device has a unique label #38657

Implemented validation to ensure each OTP device has a unique label #38657

Uh oh!

Conversation

SaikrishnaPavuluri commented Apr 3, 2025 • edited by ahus1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Update:

Uh oh!

SaikrishnaPavuluri commented Apr 7, 2025

Uh oh!

SaikrishnaPavuluri commented Apr 8, 2025

Uh oh!

ahus1 left a comment

Choose a reason for hiding this comment

Uh oh!

ahus1 Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

SaikrishnaPavuluri Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

ahus1 commented Apr 8, 2025

Uh oh!

ahus1 commented Apr 9, 2025

Uh oh!

ahus1 left a comment

Choose a reason for hiding this comment

Uh oh!

SaikrishnaPavuluri commented Apr 9, 2025

Uh oh!

SaikrishnaPavuluri commented Apr 9, 2025

Uh oh!

SaikrishnaPavuluri commented Apr 10, 2025

Uh oh!

ahus1 commented Apr 10, 2025

Uh oh!

ahus1 commented Apr 15, 2025

Uh oh!

SaikrishnaPavuluri commented Apr 17, 2025

Uh oh!

SaikrishnaPavuluri commented Apr 19, 2025

Uh oh!

SaikrishnaPavuluri commented Apr 24, 2025

Uh oh!

ahus1 commented May 3, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

ahus1 commented May 5, 2025

Uh oh!

ahus1 commented May 5, 2025

Uh oh!

SaikrishnaPavuluri commented May 5, 2025

Uh oh!

ahus1 commented May 6, 2025

Uh oh!

SaikrishnaPavuluri commented Jun 12, 2025

Uh oh!

mabartos commented Jun 12, 2025

Uh oh!

rmartinc left a comment

Choose a reason for hiding this comment

Uh oh!

ahus1 commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SaikrishnaPavuluri commented Apr 3, 2025 •

edited by ahus1

Loading

ahus1 commented Jun 12, 2025 •

edited

Loading