Add CredentialManagementService to user credential API v2#299
Merged
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
82
to
84
| } catch (PushDeviceHandlerClientException e) { | ||
| if (ERROR_CODE_PUSH_AUTH_DEVICE_NOT_FOUND.equals(e.getErrorCode())) { | ||
| if (ERROR_CODE_DEVICE_NOT_FOUND_FOR_USER_ID.getCode().equals(e.getErrorCode())) { | ||
| if (LOG.isDebugEnabled()) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| } catch (PushDeviceHandlerClientException e) { | |
| if (ERROR_CODE_PUSH_AUTH_DEVICE_NOT_FOUND.equals(e.getErrorCode())) { | |
| if (ERROR_CODE_DEVICE_NOT_FOUND_FOR_USER_ID.getCode().equals(e.getErrorCode())) { | |
| if (LOG.isDebugEnabled()) { | |
| } catch (PushDeviceHandlerClientException e) { | |
| if (ERROR_CODE_DEVICE_NOT_FOUND_FOR_USER_ID.getCode().equals(e.getErrorCode())) { | |
| LOG.info("No push authentication devices found for user ID: " + userId); |
| * @return true if the user exists, false otherwise. | ||
| * @throws CredentialMgtServerException if an error occurs while validating the user ID. | ||
| */ | ||
| public static boolean validateUserId(String userId) throws CredentialMgtServerException { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| public static boolean validateUserId(String userId) throws CredentialMgtServerException { | |
| public static boolean validateUserId(String userId) throws CredentialMgtServerException { | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Validating user ID: " + userId); | |
| } |
Comment on lines
+118
to
+120
| return false; | ||
| } | ||
| return ((UniqueIDUserStoreManager) userStoreManager).isExistingUserWithID(userId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| return false; | |
| } | |
| return ((UniqueIDUserStoreManager) userStoreManager).isExistingUserWithID(userId); | |
| return false; | |
| } | |
| boolean userExists = ((UniqueIDUserStoreManager) userStoreManager).isExistingUserWithID(userId); | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("User validation result for user ID " + userId + ": " + userExists); | |
| } | |
| return userExists; |
Comment on lines
+73
to
+74
| */ | ||
| public CredentialCreationResponseDTO createUserCredential(String userId, String type) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| */ | |
| public CredentialCreationResponseDTO createUserCredential(String userId, String type) { | |
| validateUserId(userId); | |
| LOG.info("Creating credential for user: " + userId + ", type: " + type); | |
| try { |
Comment on lines
+171
to
+173
| break; | ||
| default: | ||
| LOG.warn("No response mapping for credential type: " + entry.getKey() + ". Skipping."); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| break; | |
| default: | |
| LOG.warn("No response mapping for credential type: " + entry.getKey() + ". Skipping."); | |
| throw CredentialMgtEndpointUtils.handleCredentialMgtException(e); | |
| } catch (Exception e) { | |
| LOG.error("Unexpected error while retrieving credentials for user: " + userId + ", type: " + entry.getKey() + ". Error: " + e.getMessage()); | |
| throw CredentialMgtEndpointUtils.handleCredentialMgtException(new CredentialMgtServerException("Unexpected error occurred", e)); | |
| } |
| import org.wso2.carbon.identity.api.user.credential.common.exception.CredentialMgtClientException; | ||
| import org.wso2.carbon.identity.api.user.credential.common.exception.CredentialMgtException; | ||
| import org.wso2.carbon.identity.api.user.credential.common.exception.CredentialMgtServerException; | ||
| import org.wso2.carbon.identity.rest.api.user.credential.v2.UsersApiService; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| import org.wso2.carbon.identity.rest.api.user.credential.v2.UsersApiService; | |
| import org.apache.commons.logging.Log; | |
| import org.apache.commons.logging.LogFactory; | |
| import org.wso2.carbon.identity.rest.api.user.credential.v2.UsersApiService; |
Comment on lines
30
to
32
| */ | ||
| public class UsersCredentialApiServiceImpl implements UsersApiService { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| */ | |
| public class UsersCredentialApiServiceImpl implements UsersApiService { | |
| */ | |
| public class UsersCredentialApiServiceImpl implements UsersApiService { | |
| private static final Log LOG = LogFactory.getLog(UsersCredentialApiServiceImpl.class); | |
| private final CredentialManagementService credentialManagementService; |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughOverviewThis PR introduces a new Key ChangesNew Core Service:
Improved Error Handling:
Refactored REST Layer:
Simplified Utilities:
Test CoverageThe PR includes test plan items covering:
WalkthroughThis pull request refactors the credential management API by introducing a new CredentialManagementService core layer that centralizes credential operation orchestration. The changes standardize user ID validation, consolidate error handling, and simplify the API endpoint implementation through delegation. Five files are modified: error constants are added and standardized in handlers, user validation logic is centralized in utilities, a new service class coordinates all credential operations, the API implementation is simplified to delegate to the service, and endpoint utilities are cleaned up to remove now-redundant methods. Sequence DiagramsequenceDiagram
participant APIEndpoint as UsersCredentialApiServiceImpl
participant Service as CredentialManagementService
participant Handler as CredentialHandler
participant UserStore as UserStoreManager
participant DTO as DTO Layer
APIEndpoint->>Service: createUserCredential(userId, type)
Service->>Service: validateUserId(userId)
Service->>UserStore: isExistingUserWithID(userId)
UserStore-->>Service: boolean
Service->>Service: resolveCredentialHandler(type)
Service->>Handler: createCredential(userId)
Handler-->>Service: CredentialDTO
Service->>DTO: toEntryDTOs(credentials)
DTO-->>Service: List<CredentialEntryDTO>
Service-->>APIEndpoint: CredentialCreationResponseDTO
APIEndpoint-->>Client: 201 Created
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline 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/utils/CredentialManagementUtils.java`:
- Around line 113-119: Replace the current false return in the UniqueID support
check in CredentialManagementUtils (the branch that tests "if
(!(userStoreManager instanceof UniqueIDUserStoreManager))") with throwing a
server-side exception instead of returning false; specifically, throw an
appropriate runtime/server exception (e.g., IdentityServerException or
CarbonException used in this project) with a clear message that unique-ID
validation is unsupported for the current user store, include the userId and log
the condition (use LOG.debug/info/error as appropriate) so downstream code does
not interpret the result as USER_NOT_FOUND.
In
`@components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.v2/src/main/java/org/wso2/carbon/identity/rest/api/user/credential/v2/core/CredentialManagementService.java`:
- Around line 84-85: The response currently echoes the raw input `type` (the
.type(type) call in CredentialManagementService) which can preserve
case/whitespace variants; instead normalize to the canonical API enum value
before setting the response. Locate the .type(type) assignment in
CredentialManagementService and replace the raw value with a normalized form
derived from the API enum (e.g., trim the input, map/parse it to the canonical
enum constant using the credential enum/parsing helper, then use the enum's
canonical string/value) so the response always returns the enum's canonical API
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8d1e861-ea38-43a3-9529-6addb79cc901
⛔ Files ignored due to path filters (2)
components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.v2/src/gen/java/org/wso2/carbon/identity/rest/api/user/credential/v2/dto/ErrorDTO.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.v2/src/gen/java/org/wso2/carbon/identity/rest/api/user/credential/v2/factories/UsersApiServiceFactory.javais excluded by!**/gen/**
📒 Files selected for processing (6)
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/CredentialManagementConstants.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/core/PushCredentialHandler.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.javacomponents/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.v2/src/main/java/org/wso2/carbon/identity/rest/api/user/credential/v2/core/CredentialManagementService.javacomponents/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.v2/src/main/java/org/wso2/carbon/identity/rest/api/user/credential/v2/impl/UsersCredentialApiServiceImpl.javacomponents/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.v2/src/main/java/org/wso2/carbon/identity/rest/api/user/credential/v2/utils/CredentialMgtEndpointUtils.java
💤 Files with no reviewable changes (1)
- components/org.wso2.carbon.identity.api.user.credential/org.wso2.carbon.identity.api.user.credential.v2/src/main/java/org/wso2/carbon/identity/rest/api/user/credential/v2/utils/CredentialMgtEndpointUtils.java
Comment on lines
+113
to
+119
| if (!(userStoreManager instanceof UniqueIDUserStoreManager)) { | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("User store manager does not support unique user IDs. " + | ||
| "Cannot validate user ID: " + userId); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Return a server error when unique-ID validation is unsupported.
At Line 113-119, returning false causes downstream code to treat this as USER_NOT_FOUND, which can misreport valid users when the store cannot validate unique IDs. Throw a server exception on this branch instead.
Suggested fix
if (!(userStoreManager instanceof UniqueIDUserStoreManager)) {
if (LOG.isDebugEnabled()) {
LOG.debug("User store manager does not support unique user IDs. " +
"Cannot validate user ID: " + userId);
}
- return false;
+ throw handleServerException(ErrorMessages.ERROR_CODE_VALIDATE_USER_ID_FAILURE,
+ new IllegalStateException("Unique ID user store is not supported for tenant: " +
+ tenantDomain), userId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!(userStoreManager instanceof UniqueIDUserStoreManager)) { | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("User store manager does not support unique user IDs. " + | |
| "Cannot validate user ID: " + userId); | |
| } | |
| return false; | |
| } | |
| if (!(userStoreManager instanceof UniqueIDUserStoreManager)) { | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("User store manager does not support unique user IDs. " + | |
| "Cannot validate user ID: " + userId); | |
| } | |
| throw handleServerException(ErrorMessages.ERROR_CODE_VALIDATE_USER_ID_FAILURE, | |
| new IllegalStateException("Unique ID user store is not supported for tenant: " + | |
| tenantDomain), userId); | |
| } |
🤖 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/utils/CredentialManagementUtils.java`
around lines 113 - 119, Replace the current false return in the UniqueID support
check in CredentialManagementUtils (the branch that tests "if
(!(userStoreManager instanceof UniqueIDUserStoreManager))") with throwing a
server-side exception instead of returning false; specifically, throw an
appropriate runtime/server exception (e.g., IdentityServerException or
CarbonException used in this project) with a clear message that unique-ID
validation is unsupported for the current user store, include the userId and log
the condition (use LOG.debug/info/error as appropriate) so downstream code does
not interpret the result as USER_NOT_FOUND.
Comment on lines
+84
to
+85
| .type(type) | ||
| .credentials(dto.getCredentials()); |
There was a problem hiding this comment.
Normalize the response type to canonical API value.
At Line 84, the response echoes raw input type. If input includes case/whitespace variants, output becomes inconsistent despite successful normalized resolution. Return the canonical enum API value instead.
Suggested fix
return new CredentialCreationResponseDTO()
- .type(type)
+ .type(CredentialTypes.fromString(type).apiValue())
.credentials(dto.getCredentials());🤖 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.v2/src/main/java/org/wso2/carbon/identity/rest/api/user/credential/v2/core/CredentialManagementService.java`
around lines 84 - 85, The response currently echoes the raw input `type` (the
.type(type) call in CredentialManagementService) which can preserve
case/whitespace variants; instead normalize to the canonical API enum value
before setting the response. Locate the .type(type) assignment in
CredentialManagementService and replace the raw value with a normalized form
derived from the API enum (e.g., trim the input, map/parse it to the canonical
enum constant using the credential enum/parsing helper, then use the enum's
canonical string/value) so the response always returns the enum's canonical API
value.
pamodaaw
force-pushed
the
feature/user-credential-api-v2
branch
from
81d27e5 to
1d3044a
Compare
ZiyamSanthosh
approved these changes
Jun 11, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CredentialManagementServiceas a dedicated core service layer (mirroring theSessionManagementServicepattern) responsible for user ID validation, credential handler resolution, and translating handler results to API response DTOsUsersCredentialApiServiceImplto a thin delegation layer that only buildsResponseobjectsCredentialManagementUtils.validateUserId: handleUserStoreExceptionfromisExistingUserWithID, add a proper debug log and earlyreturn falsewhen the user store manager does not support unique IDs (instead of falling through to an unsafe cast), and replaceRuntimeExceptionwith a typedCredentialMgtServerExceptionCredentialManagementServiceand clean up now-unused methods fromCredentialMgtEndpointUtils