Fix passkey credential resolution to use fully-qualified username#301
Conversation
📝 WalkthroughSummaryThis pull request addresses username resolution in passkey credential management by ensuring fully-qualified usernames (in the format ChangesCredentialManagementUtils - The PasskeyCredentialHandler - Updated to call CredentialManagementService - A new core service was added to handle credential management operations. It validates user IDs, resolves credential handlers based on credential type, and translates handler results to API response DTOs. The service provides methods for creating, deleting, and retrieving credentials by type or ID. Walkthrough
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.common/src/main/java/org/wso2/carbon/identity/api/user/credential/common/core/PasskeyCredentialHandler.java (1)
63-84:⚠️ Potential issue | 🔴 CriticalWrap username resolution to handle exceptions consistently.
The
resolveUsernameFromUserId()call at line 64 can throw runtime exceptions (e.g., when the user ID cannot be resolved), which are not caught by the try-catch block that only handlesFIDO2AuthenticatorServerException. This causes resolution failures to propagate as unhandled exceptions instead of being wrapped asCredentialMgtExceptionfor consistent error handling.Wrap the username resolution call in a separate try-catch block or modify the existing catch block to handle exceptions from
UniqueIdToUser.apply().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.common/src/main/java/org/wso2/carbon/identity/api/user/credential/common/core/PasskeyCredentialHandler.java` around lines 63 - 84, The username resolution call to `CredentialManagementUtils.resolveUsernameFromUserId(userId)` at the beginning of the try block can throw runtime exceptions that are not caught by the existing catch block, which only handles `FIDO2AuthenticatorServerException`. Wrap the `resolveUsernameFromUserId()` call and its `toFullQualifiedUsername()` invocation in a separate try-catch block, or extend the existing catch block to also catch exceptions from username resolution and wrap them using `CredentialManagementUtils.handleServerException` with an appropriate error code to ensure consistent error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.common/src/main/java/org/wso2/carbon/identity/api/user/credential/common/core/PasskeyCredentialHandler.java`:
- Around line 63-84: The username resolution call to
`CredentialManagementUtils.resolveUsernameFromUserId(userId)` at the beginning
of the try block can throw runtime exceptions that are not caught by the
existing catch block, which only handles `FIDO2AuthenticatorServerException`.
Wrap the `resolveUsernameFromUserId()` call and its `toFullQualifiedUsername()`
invocation in a separate try-catch block, or extend the existing catch block to
also catch exceptions from username resolution and wrap them using
`CredentialManagementUtils.handleServerException` with an appropriate error code
to ensure consistent error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40bcc98d-b4b7-490e-9bf2-49f85a4e064c
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.common/src/main/java/org/wso2/carbon/identity/api/user/credential/common/core/PasskeyCredentialHandler.javacomponents/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.common/src/main/java/org/wso2/carbon/identity/api/user/credential/common/utils/CredentialManagementUtils.java
Summary
CredentialManagementServiceand fixed user ID validationresolveUsernameFromUserIdinCredentialManagementUtilsto return the fullUserobject instead of a manually concatenated username stringPasskeyCredentialHandlerto call.toFullQualifiedUsername()at the call sites for get and delete passkey operations, ensuring the tenant domain is correctly included when invoking the WebAuthn service