Introduce end user credential management admin API #1008
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...n/identity/api/server/credential/management/common/impl/CredentialManagementServiceImpl.java
Outdated
Show resolved
Hide resolved
...n/identity/api/server/credential/management/common/impl/CredentialManagementServiceImpl.java
Outdated
Show resolved
Hide resolved
...n/identity/api/server/credential/management/common/impl/CredentialManagementServiceImpl.java
Outdated
Show resolved
Hide resolved
...2/carbon/identity/api/server/credential/management/common/core/PasskeyCredentialHandler.java
Show resolved
Hide resolved
...2/carbon/identity/api/server/credential/management/common/impl/PasskeyCredentialHandler.java
Outdated
Show resolved
Hide resolved
...2/carbon/identity/api/server/credential/management/common/core/PasskeyCredentialHandler.java
Show resolved
Hide resolved
...2/carbon/identity/api/server/credential/management/common/impl/PasskeyCredentialHandler.java
Outdated
Show resolved
Hide resolved
...2/carbon/identity/api/server/credential/management/common/impl/PasskeyCredentialHandler.java
Outdated
Show resolved
Hide resolved
...2/carbon/identity/api/server/credential/management/common/impl/PasskeyCredentialHandler.java
Show resolved
Hide resolved
...wso2/carbon/identity/api/server/credential/management/common/core/PushCredentialHandler.java
Show resolved
Hide resolved
...wso2/carbon/identity/api/server/credential/management/common/core/PushCredentialHandler.java
Show resolved
Hide resolved
...wso2/carbon/identity/api/server/credential/management/common/core/PushCredentialHandler.java
Show resolved
Hide resolved
...wso2/carbon/identity/api/server/credential/management/common/core/PushCredentialHandler.java
Show resolved
Hide resolved
...wso2/carbon/identity/api/server/credential/management/common/core/PushCredentialHandler.java
Show resolved
Hide resolved
...carbon/identity/api/server/credential/management/common/utils/CredentialManagementUtils.java
Show resolved
Hide resolved
...y/api/server/user/credential/management/v1/core/UserCredentialManagementServiceImplTest.java
Outdated
Show resolved
Hide resolved
...bon/identity/api/server/credential/management/v1/core/ServerCredentialManagementService.java
Outdated
Show resolved
Hide resolved
| CredentialMgtEndpointUtils.validateUserId(userId); | ||
| CredentialMgtEndpointUtils.validateCredentialId(credentialId); | ||
| CredentialMgtEndpointUtils.validateCredentialType(type); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
| CredentialMgtEndpointUtils.validateUserId(userId); | |
| CredentialMgtEndpointUtils.validateCredentialId(credentialId); | |
| CredentialMgtEndpointUtils.validateCredentialType(type); | |
| CredentialMgtEndpointUtils.validateCredentialId(credentialId); | |
| CredentialMgtEndpointUtils.validateCredentialType(type); | |
| log.info("Deleting credential of type: {} for user: {}", type, userId); | |
| adminCredentialManagementService.deleteCredentialForUser(userId, type, credentialId); |
.../wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialApiServiceImpl.java
Outdated
Show resolved
Hide resolved
.../wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialApiServiceImpl.java
Show resolved
Hide resolved
.../wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialApiServiceImpl.java
Outdated
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Outdated
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Outdated
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Show resolved
Hide resolved
...o2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
Show resolved
Hide resolved
NipuniBhagya
force-pushed
the
credential-mgt
branch
2 times, most recently
from
42747b1 to
f0f9713
Compare
NipuniBhagya
force-pushed
the
credential-mgt
branch
from
f0f9713 to
55c3c53
Compare
NipuniBhagya
force-pushed
the
credential-mgt
branch
from
541396e to
7ec8883
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementServiceDataHolder.java`:
- Around line 46-50: The SERVICE field in the inner class RealmServiceHolder is
declared package-private; change its declaration to private static final to
match the visibility of WebAuthnServiceHolder and PushDeviceHandlerHolder.
Locate the RealmServiceHolder inner class and update the SERVICE field modifier
accordingly so it becomes private static final RealmService SERVICE (keeping the
existing initialization via PrivilegedCarbonContext unchanged).
In
`@components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtServerException.java`:
- Around line 26-36: Update the Javadoc for the
CredentialMgtServerException(String errorCode, String message, String
description, Throwable cause) constructor to match the actual parameter list and
include the missing cause: reorder the `@param` tags to errorCode, message,
description, cause; add a `@param` description for cause (e.g., throwable cause of
the exception); and update the main description to mention that a cause can be
provided. Ensure the comment text and tag order exactly reflect the constructor
signature.
In
`@components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yaml`:
- Around line 110-136: The DELETE endpoint responses block in
credential-management.yaml is missing a 404 response for "credential or user not
found"; update the responses for the DELETE operation (the responses block
shown) to include a '404' entry with a description like "Not Found - Credential
or User Not Found" and the same application/json schema ref
('#/components/schemas/Error') used by the other error responses so the API
returns 404 when the credential or user does not exist.
In
`@components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImplTest.java`:
- Around line 55-64: The test currently mocks the system under test
(userCredentialManagementService) which is wrong; remove the `@Mock` on
CredentialManagementServiceImpl, instantiate a real
CredentialManagementServiceImpl in setUp() and inject the mocked
CredentialHandler instances (passkeyHandler, pushAuthHandler) into its handler
map (e.g., via the existing handlerMap field or reflection), then update tests
to call real methods on userCredentialManagementService and remove
doCallRealMethod() usages so the real implementation is exercised with mocked
dependencies.
In
`@components/org.wso2.carbon.identity.api.server.credential.management/pom.xml`:
- Around line 23-28: The parent POM version in the <parent> block (artifactId
identity-api-server, groupId org.wso2.carbon.identity.server.api) is incorrect
(1.3.223-SNAPSHOT) and must match the root POM; update the <version> value to
1.3.231-SNAPSHOT so Maven can resolve the parent successfully.
♻️ Duplicate comments (8)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementService.java (1)
29-40: Interface structure looks good; documentation concern already raised.The interface design is clean with appropriate method signatures and DTO usage. The previous review comment comprehensively addresses the need for detailed JavaDoc including parameter descriptions, return value semantics, and exception documentation. Please ensure that feedback is addressed.
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.java (1)
26-28: Declare fields asfinalfor true immutability.The fields should be
finalsince the class uses a private constructor, no setters, and the builder pattern—all indicating immutability intent. This ensures thread safety and prevents accidental reassignment.- private String entityId; - private String credentialId; - private String type; + private final String entityId; + private final String credentialId; + private final String type;components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementServiceDataHolder.java (1)
35-38: Direct instantiation of OSGi service is inconsistent with other holders.This issue was already flagged in a previous review. The
WebAuthnServiceis instantiated directly withnew WebAuthnService(), whilePushDeviceHandlerHolderandRealmServiceHolderretrieve services viaPrivilegedCarbonContext. This inconsistency may lead to runtime failures ifWebAuthnServicedepends on OSGi lifecycle management.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImplTest.java (1)
251-258: Unused DataProvideredgeCaseInputs.This DataProvider is defined but not consumed by any test method. Either add a test using
@Test(dataProvider = "edgeCaseInputs")to validate trimming and mixed-case handling, or remove this dead code.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yaml (1)
157-160: Verify error code example validity.The example error code
'CM-60401'was previously flagged as potentially invalid sinceCredentialManagementConstants.ErrorMessagesdefines codes likeCM-60001throughCM-60006(client errors) andCM-65001throughCM-65004(server errors). The previous review was marked as addressed, but the value appears unchanged.Please confirm if
CM-60401is intentional (perhaps a new error code was added) or if it should be updated to match an existing constant likeCM-60001.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialHandler.java (1)
27-29: Correct the interface Javadoc to match the interface name.The Javadoc describes this as "Credential Management Service interface" but the interface is actually named
CredentialHandler. This inconsistency can cause confusion for developers.📝 Suggested fix
/** - * Credential Management Service interface. + * Interface for handling credential operations for a specific credential type. */ public interface CredentialHandler {components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImpl.java (1)
78-86: Potential NPE if handler is not registered for a valid credential type.After
validateCredentialTypepasses,CredentialTypes.fromStringwill return a valid enum value. However,handlerMap.get(credentialType)could still returnnullif no handler is registered for that type, causing an NPE on line 83.🐛 Proposed fix
try { CredentialMgtEndpointUtils.validateCredentialId(credentialDeletionRequest.getCredentialId()); CredentialMgtEndpointUtils.validateCredentialType(credentialDeletionRequest.getType()); CredentialTypes credentialType = CredentialTypes.fromString(credentialDeletionRequest.getType()); CredentialHandler handler = handlerMap.get(credentialType); + if (handler == null) { + throw new CredentialMgtException( + "No handler registered for credential type: " + credentialType, + CredentialManagementConstants.ErrorMessages.ERROR_CODE_INVALID_CREDENTIAL_TYPE.getCode(), + "Handler not available for the specified credential type."); + } handler.deleteCredential(credentialDeletionRequest); } catch (CredentialMgtException e) {Note: You'll need to add the import for
CredentialMgtExceptionandCredentialManagementConstantsif not already present.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/core/PasskeyCredentialHandler.java (1)
141-156: Potential null username and duplicate exception handling.Two issues remain from previous review:
Null username risk (line 148):
getUserNameFromUserID(userId)could returnnull, resulting in"null@tenantDomain"being returned, which could cause downstream issues.Duplicate catch blocks (lines 149-155): The two
UserStoreExceptioncatch blocks have identical handling and can be combined using multi-catch syntax.🐛 Proposed fix
private String resolveUsernameFromUserId(String userId) throws CredentialMgtException { try { RealmService userRealm = CredentialManagementServiceDataHolder.getRealmService(); String tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); AbstractUserStoreManager userStoreManager = (AbstractUserStoreManager) userRealm .getTenantUserRealm(IdentityTenantUtil.getTenantId(tenantDomain)).getUserStoreManager(); - return userStoreManager.getUserNameFromUserID(userId) + "@" + tenantDomain; - } catch (UserStoreException e) { - throw CredentialManagementUtils.handleServerException( - CredentialManagementConstants.ErrorMessages.ERROR_CODE_GET_USERNAME_FROM_USERID, e, userId); - } catch (org.wso2.carbon.user.api.UserStoreException e) { + String username = userStoreManager.getUserNameFromUserID(userId); + if (username == null) { + throw CredentialManagementUtils.handleClientException( + CredentialManagementConstants.ErrorMessages.ERROR_CODE_ENTITY_NOT_FOUND, null, userId); + } + return username + "@" + tenantDomain; + } catch (UserStoreException | org.wso2.carbon.user.api.UserStoreException e) { throw CredentialManagementUtils.handleServerException( CredentialManagementConstants.ErrorMessages.ERROR_CODE_GET_USERNAME_FROM_USERID, e, userId); } }
🧹 Nitpick comments (8)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.java (1)
79-82: Add validation for required fields inbuild()method.The
build()method creates the DTO without validating that essential fields (entityId,credentialId,type) are non-null. This could allow invalid deletion requests to propagate, causing runtime failures downstream when the service attempts to delete a credential.♻️ Suggested validation
public CredentialDeletionRequestDTO build() { + if (entityId == null || entityId.isEmpty()) { + throw new IllegalArgumentException("entityId is required"); + } + if (credentialId == null || credentialId.isEmpty()) { + throw new IllegalArgumentException("credentialId is required"); + } + if (type == null || type.isEmpty()) { + throw new IllegalArgumentException("type is required"); + } return new CredentialDeletionRequestDTO(entityId, credentialId, type); }components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtClientException.java (1)
58-61: Consider supporting formatted descriptions in theErrorMessagesconstructor.The
ErrorMessagesenum has description templates with%splaceholders (e.g.,"Entity with ID %s not found..."), but this constructor useserror.getDescription()directly without formatting. Callers using this constructor won't be able to substitute placeholders.If template substitution is needed, consider adding an overload:
💡 Suggested enhancement
/** * Constructor with {`@code` error} parameter and format arguments. * * `@param` error Error message. * `@param` args Arguments to format the description. */ public CredentialMgtClientException(ErrorMessages error, Object... args) { super(error.getMessage(), error.getCode(), String.format(error.getDescription(), args)); }components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementConstants.java (2)
73-80: Consider caching theCollatorinstance for better performance.A new
Collatorinstance is created on every call tofromString(). While not critical for low-volume usage, this could be optimized by caching it as a static field.♻️ Suggested optimization
public enum CredentialTypes { PASSKEY("passkey"), PUSH_AUTH("push-auth"); private final String apiValue; + private static final Collator COLLATOR; + + static { + COLLATOR = Collator.getInstance(Locale.ROOT); + COLLATOR.setStrength(Collator.PRIMARY); + } // ... constructor and getApiValue ... public static CredentialTypes fromString(String value) { if (value == null) { return null; } String candidate = value.trim(); if (candidate.isEmpty()) { return null; } - Collator collator = Collator.getInstance(Locale.ROOT); - collator.setStrength(Collator.PRIMARY); - return Arrays.stream(values()) - .filter(type -> collator.compare(type.name(), candidate) == 0 - || collator.compare(type.getApiValue(), candidate) == 0) + .filter(type -> COLLATOR.compare(type.name(), candidate) == 0 + || COLLATOR.compare(type.getApiValue(), candidate) == 0) .findFirst() .orElse(null); } }
117-117: RelocateERROR_CODE_PUSH_AUTH_DEVICE_NOT_FOUNDoutside theErrorMessagesenum.This constant uses a different prefix (
PDH-vsCM-) and isn't an enum member—it appears to be an external error code from the Push Device Handler used for comparison. Placing it inside theErrorMessagesenum is misleading.♻️ Suggested relocation
public class CredentialManagementConstants { private CredentialManagementConstants() { } + + /** + * Error code from Push Device Handler indicating device not found. + */ + public static final String ERROR_CODE_PUSH_AUTH_DEVICE_NOT_FOUND = "PDH-15010"; // ... CredentialTypes enum ... public enum ErrorMessages { // ... enum values ... private static final String ERROR_PREFIX = "CM"; - public static final String ERROR_CODE_PUSH_AUTH_DEVICE_NOT_FOUND = "PDH-15010"; private final String code;components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtException.java (1)
26-27: Consider making exception fields immutable.The
errorCodeanddescriptionfields are non-final with protected setters, allowing mutation after construction. Exceptions are conventionally immutable, and mutable exception state can lead to subtle bugs if the exception is caught, modified, and re-thrown or logged at different points.If post-construction modification isn't required by subclasses, consider making the fields
finaland removing the setters.♻️ Suggested change for immutability
public class CredentialMgtException extends Exception { - private String errorCode; - private String description; + private final String errorCode; + private final String description; // ... constructors unchanged ... - /** - * Set the {`@code` errorCode}. - * - * `@param` errorCode The value to be set as the {`@code` errorCode}. - */ - protected void setErrorCode(String errorCode) { - - this.errorCode = errorCode; - } - - /** - * Set the {`@code` description}. - * - * `@param` description The value to be set as the {`@code` description}. - */ - protected void setDescription(String description) { - - this.description = description; - } }Also applies to: 83-96
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImplTest.java (1)
174-181: Test assumes handler invocation order.Line 179 asserts
pushAuthHandleris never called, assumingpasskeyHandlerthrows first. This relies onEnumMapiteration order (which follows enum declaration order). IfCredentialTypesenum is reordered, this test could fail or produce false positives.Consider documenting this assumption or making the test order-agnostic by verifying that at least one handler was called before the exception.
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yaml (2)
48-51: Consider addingmaxItemsto the array response.Static analysis flagged that the array response lacks a maximum bound. Adding
maxItemsimproves API documentation and sets clear expectations for consumers about response size limits.📝 Suggested improvement
schema: type: array + maxItems: 100 items: $ref: '#/components/schemas/Credential'
104-109: Minor: Description refers to "device" instead of "credential".Line 106 describes
credential-idas "The unique identifier of the device to be deleted" but this API manages credentials, not specifically devices. Consider updating for consistency.📝 Suggested fix
- name: credential-id in: path - description: The unique identifier of the device to be deleted. + description: The unique identifier of the credential to be deleted. required: true
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/credential/management/v1/Credential.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/credential/management/v1/Error.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/credential/management/v1/UsersApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/credential/management/v1/UsersApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/gen/java/org/wso2/carbon/identity/api/server/credential/management/v1/factories/UsersApiServiceFactory.javais excluded by!**/gen/**
📒 Files selected for processing (24)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/pom.xmlcomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialHandler.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementConstants.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementService.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementServiceDataHolder.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/core/PasskeyCredentialHandler.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/core/PushCredentialHandler.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDTO.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtClientException.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtException.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtServerException.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/utils/CredentialManagementUtils.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/pom.xmlcomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/constants/CredentialMgtEndpointConstants.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/factories/CredentialManagementServiceFactory.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImpl.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/UsersCredentialApiServiceImpl.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yamlcomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImplTest.javacomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/resources/testng.xmlcomponents/org.wso2.carbon.identity.api.server.credential.management/pom.xmlpom.xml
🚧 Files skipped from review as they are similar to previous changes (9)
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/pom.xml
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/factories/CredentialManagementServiceFactory.java
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/resources/testng.xml
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/pom.xml
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDTO.java
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/core/PushCredentialHandler.java
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/UsersCredentialApiServiceImpl.java
- components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/utils/CredentialManagementUtils.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T02:37:06.312Z
Learnt from: mpmadhavig
Repo: wso2/identity-api-server PR: 1042
File: components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java:515-534
Timestamp: 2025-12-10T02:37:06.312Z
Learning: In the file `components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java`, the `createIDPImportRequest` method intentionally supports only JSON format for the new certificate export/import structure when the `SupportMultipleCertificateExport` feature flag is enabled. XML and YAML format support for the new certificate structure is deferred as future work.
Applied to files:
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yaml
🧬 Code graph analysis (6)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/core/PasskeyCredentialHandler.java (6)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementConstants.java (1)
CredentialManagementConstants(28-150)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementServiceDataHolder.java (1)
CredentialManagementServiceDataHolder(29-81)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDTO.java (1)
CredentialDTO(24-84)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.java (1)
CredentialDeletionRequestDTO(24-84)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtException.java (1)
CredentialMgtException(24-97)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/utils/CredentialManagementUtils.java (1)
CredentialManagementUtils(29-70)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtClientException.java (1)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementConstants.java (1)
CredentialManagementConstants(28-150)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementService.java (2)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDTO.java (1)
CredentialDTO(24-84)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.java (1)
CredentialDeletionRequestDTO(24-84)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.java (1)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDTO.java (1)
Builder(55-83)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImpl.java (5)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementConstants.java (1)
CredentialManagementConstants(28-150)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDTO.java (1)
CredentialDTO(24-84)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.java (1)
CredentialDeletionRequestDTO(24-84)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtException.java (1)
CredentialMgtException(24-97)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/utils/CredentialMgtEndpointUtils.java (1)
CredentialMgtEndpointUtils(43-185)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialHandler.java (3)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDTO.java (1)
CredentialDTO(24-84)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/dto/CredentialDeletionRequestDTO.java (1)
CredentialDeletionRequestDTO(24-84)components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtException.java (1)
CredentialMgtException(24-97)
🪛 Checkov (3.2.334)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yaml
[medium] 49-53: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (27)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtServerException.java (1)
1-24: LGTM!License header, package declaration, and class design are appropriate for a server-side exception in the credential management hierarchy.
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/constants/CredentialMgtEndpointConstants.java (2)
36-67: LGTM!The
ErrorMessagesenum follows a clean pattern for managing error codes with the prefix applied ingetCode(). The structure is extensible for adding additional error codes as the module evolves.
30-31: > Likely an incorrect or invalid review comment.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtException.java (1)
36-56: LGTM!The constructors are well-structured with proper delegation to the parent
Exceptionclass and consistent field initialization. The Javadoc accurately reflects the parameter order.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialManagementServiceDataHolder.java (1)
1-33: LGTM!License header, imports, and class structure with private constructor follow standard patterns for a non-instantiable service holder class.
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImplTest.java (6)
66-78: Reflection injection is acceptable but ensure the SUT is a real instance.The reflection-based handler injection is a valid approach for testing when constructors don't expose dependencies. However, this becomes meaningful only when
userCredentialManagementServiceis a real instance rather than a mock.
83-103: Test logic is correct.The test properly verifies that credentials from both handlers are aggregated. Once the SUT is changed to a real instance, remove the
doCallRealMethod()call on line 92.
108-143: Good coverage of empty and partial result scenarios.These tests effectively cover edge cases where handlers return empty lists or only one handler has credentials.
183-215: Well-structured data-driven test for credential type handling.Good coverage of case-insensitive type matching and hyphen/underscore variations.
220-249: Good negative test coverage.Tests properly validate error handling for invalid credential types and handler exceptions.
263-283: Helper methods are well-structured.Clean factoring of mock DTO creation improves test readability.
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yaml (2)
1-14: LGTM!The API metadata is well-structured with clear description of purpose, required admin scopes (
internal_user_mgt_viewandinternal_user_mgt_delete), and proper licensing information.
16-18: Security schemes are referenced but not defined.The
OAuth2andBasicAuthsecurity schemes are referenced in the global security section but are not defined incomponents/securitySchemes. This may cause OpenAPI validation warnings and confusion for API consumers.Please verify whether these security schemes are injected during build/generation or if they should be explicitly defined:
components: securitySchemes: OAuth2: type: oauth2 flows: authorizationCode: authorizationUrl: https://localhost:9443/oauth2/authorize tokenUrl: https://localhost:9443/oauth2/token scopes: internal_user_mgt_view: View user details internal_user_mgt_delete: Delete user details BasicAuth: type: http scheme: basiccomponents/org.wso2.carbon.identity.api.server.credential.management/pom.xml (1)
33-36: LGTM!The multi-module structure follows the project's established pattern for API modules (common + versioned implementation).
pom.xml (6)
842-847: LGTM!The mockito-testng dependency is properly added with test scope and version managed via property.
893-898: LGTM!The credential management common dependency is correctly added with
providedscope, consistent with other common module dependencies in this POM.
1042-1043: LGTM!Version properties are properly defined for the new dependencies.
1064-1064: LGTM!Version property correctly defined for mockito-testng.
1112-1112: LGTM!The credential management module is correctly added to the modules list.
899-908: Verify fido2 dependency version; 5.4.31 does not appear published.Regarding the
providedscope recommendation: While the suggestion to align with similar backend service dependencies is reasonable, I cannot verify the current scope values in other dependencies without direct repository access.However, the version claim requires attention: Search results indicate that org.wso2.carbon.identity.application.authenticator.fido2 version 5.4.31 is not published in Maven repositories. Available versions include 5.4.30, 5.4.18, and earlier 5.3.x releases. If the pom.xml declares version 5.4.31, this will fail to resolve. Update to an available version (e.g., 5.4.30) before merging.
Regarding the
providedscope: Please verify the scope declarations in similar backend service dependencies (identity.governance, identity.event.handler) to confirm whether addingprovidedscope is consistent with the project's dependency management strategy.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/CredentialHandler.java (1)
30-41: LGTM!The interface methods are well-defined with appropriate signatures. The contract clearly defines operations for retrieving and deleting credentials with proper exception handling.
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImpl.java (2)
48-67: LGTM!The
getCredentialsmethod correctly aggregates credentials from all registered handlers with proper exception handling and debug logging.
87-92: LGTM!The logging has been correctly updated to use
LOG.debugwithin theLOG.isDebugEnabled()check.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/core/PasskeyCredentialHandler.java (4)
46-58: LGTM!The class structure is clean with appropriate constructor injection pattern for the WebAuthnService dependency.
60-88: LGTM!The
getCredentialsmethod has proper exception handling that distinguishes between server errors (FIDO2AuthenticatorServerException) and client errors (IdentityRuntimeExceptionfor entity not found). Debug logging is correctly implemented.
90-117: LGTM!The
deleteCredentialmethod has comprehensive exception handling covering client errors, server errors, and entity-not-found scenarios. Debug logging is correctly implemented.
125-132: Consider adding null safety for credential field access.The method chain
credential.getCredential().getCredentialId().getBase64Url()could throw NPE if any intermediate value is null. Consider adding null checks or defensive handling if the upstreamFIDO2CredentialRegistrationcontract doesn't guarantee non-null values.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
.../identity/api/server/credential/management/common/CredentialManagementServiceDataHolder.java
Show resolved
Hide resolved
| /** | ||
| * Constructor with {@code message}, {@code errorCode} and {@code description} parameters. | ||
| * | ||
| * @param message Message to be included in the exception. | ||
| * @param errorCode Error code of the exception. | ||
| * @param description Description of the exception. | ||
| */ | ||
| public CredentialMgtServerException(String errorCode, String message, String description, Throwable cause) { | ||
|
|
||
| super(message, errorCode, description, cause); | ||
| } |
There was a problem hiding this comment.
Javadoc is inconsistent with the constructor signature.
The Javadoc has the following issues:
- The description omits the
causeparameter. - The
@paramtag order doesn't match the actual parameter order (errorCode,message,description,cause). - Missing
@paramforcause.
📝 Proposed fix for the Javadoc
/**
- * Constructor with {`@code` message}, {`@code` errorCode} and {`@code` description} parameters.
+ * Constructor with {`@code` errorCode}, {`@code` message}, {`@code` description} and {`@code` cause} parameters.
*
- * `@param` message Message to be included in the exception.
* `@param` errorCode Error code of the exception.
+ * `@param` message Message to be included in the exception.
* `@param` description Description of the exception.
+ * `@param` cause Throwable cause of the exception.
*/
public CredentialMgtServerException(String errorCode, String message, String description, Throwable cause) {
super(message, errorCode, description, cause);
}📝 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.
| /** | |
| * Constructor with {@code message}, {@code errorCode} and {@code description} parameters. | |
| * | |
| * @param message Message to be included in the exception. | |
| * @param errorCode Error code of the exception. | |
| * @param description Description of the exception. | |
| */ | |
| public CredentialMgtServerException(String errorCode, String message, String description, Throwable cause) { | |
| super(message, errorCode, description, cause); | |
| } | |
| /** | |
| * Constructor with {`@code` errorCode}, {`@code` message}, {`@code` description} and {`@code` cause} parameters. | |
| * | |
| * `@param` errorCode Error code of the exception. | |
| * `@param` message Message to be included in the exception. | |
| * `@param` description Description of the exception. | |
| * `@param` cause Throwable cause of the exception. | |
| */ | |
| public CredentialMgtServerException(String errorCode, String message, String description, Throwable cause) { | |
| super(message, errorCode, description, cause); | |
| } |
🤖 Prompt for AI Agents
In
`@components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/src/main/java/org/wso2/carbon/identity/api/server/credential/management/common/exception/CredentialMgtServerException.java`
around lines 26 - 36, Update the Javadoc for the
CredentialMgtServerException(String errorCode, String message, String
description, Throwable cause) constructor to match the actual parameter list and
include the missing cause: reorder the `@param` tags to errorCode, message,
description, cause; add a `@param` description for cause (e.g., throwable cause of
the exception); and update the main description to mention that a cause can be
provided. Ensure the comment text and tag order exactly reflect the constructor
signature.
| responses: | ||
| '204': | ||
| description: User Credential Deleted. | ||
| '400': | ||
| description: Bad Request. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Error' | ||
| '401': | ||
| description: Unauthorized. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Error' | ||
| '403': | ||
| description: Forbidden. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Error' | ||
| '500': | ||
| description: Internal Server Error. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Error' |
There was a problem hiding this comment.
Add 404 response for credential or user not found.
The DELETE endpoint is missing a 404 response, which is inconsistent with the GET endpoint that includes 404 for "User Not Found". When deleting a non-existent credential or for a non-existent user, the API should return 404.
📝 Suggested addition
'403':
description: Forbidden.
content:
application/json:
schema:
$ref: '#/components/schemas/Error'
+ '404':
+ description: User or Credential Not Found.
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/Error'
'500':
description: Internal Server Error.🤖 Prompt for AI Agents
In
`@components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/main/resources/credential-management.yaml`
around lines 110 - 136, The DELETE endpoint responses block in
credential-management.yaml is missing a 404 response for "credential or user not
found"; update the responses for the DELETE operation (the responses block
shown) to include a '404' entry with a description like "Not Found - Credential
or User Not Found" and the same application/json schema ref
('#/components/schemas/Error') used by the other error responses so the API
returns 404 when the credential or user does not exist.
| @Mock | ||
| private CredentialHandler passkeyHandler; | ||
|
|
||
| @Mock | ||
| private CredentialHandler pushAuthHandler; | ||
|
|
||
| @Mock | ||
| private CredentialManagementServiceImpl userCredentialManagementService; | ||
|
|
||
| private static final String TEST_USER_ID = "test-user-123"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid mocking the class under test.
userCredentialManagementService is the system under test (SUT), but it's declared as a @Mock. This anti-pattern leads to fragile tests that rely on doCallRealMethod() and may not accurately validate real implementation behavior.
Instantiate a real CredentialManagementServiceImpl and inject the mocked handlers directly.
♻️ Suggested refactor
`@Mock`
private CredentialHandler passkeyHandler;
`@Mock`
private CredentialHandler pushAuthHandler;
- `@Mock`
- private CredentialManagementServiceImpl userCredentialManagementService;
+ private CredentialManagementServiceImpl userCredentialManagementService;
private static final String TEST_USER_ID = "test-user-123";Then in setUp(), instantiate the real service:
userCredentialManagementService = new CredentialManagementServiceImpl();
// Inject the handlerMap via reflection into the real instance🤖 Prompt for AI Agents
In
`@components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/src/test/java/org/wso2/carbon/identity/api/server/credential/management/v1/impl/CredentialManagementServiceImplTest.java`
around lines 55 - 64, The test currently mocks the system under test
(userCredentialManagementService) which is wrong; remove the `@Mock` on
CredentialManagementServiceImpl, instantiate a real
CredentialManagementServiceImpl in setUp() and inject the mocked
CredentialHandler instances (passkeyHandler, pushAuthHandler) into its handler
map (e.g., via the existing handlerMap field or reflection), then update tests
to call real methods on userCredentialManagementService and remove
doCallRealMethod() usages so the real implementation is exercised with mocked
dependencies.
components/org.wso2.carbon.identity.api.server.credential.management/pom.xml
Show resolved
Hide resolved
|
PR builder started |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pom.xml`:
- Around line 1042-1043: The pom property identity.notification.push.version is
set to the non-existent 1.0.9 and will break the build; change the property
value to the latest published release 1.0.6 (or point to a repository that
actually hosts 1.0.9) by updating the identity.notification.push.version
property in the POM so it matches an available artifact, then re-run the build
to confirm resolution.
🧹 Nitpick comments (3)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/pom.xml (2)
162-180: Consider updating build-helper-maven-plugin version.Version 1.8 of
build-helper-maven-pluginis quite dated. Consider updating to a more recent version (e.g., 3.4.0 or later) for better compatibility and bug fixes, or align with the version used elsewhere in the project.🔧 Suggested update
<plugin> <groupId>org.codehaus.mojo</groupId> <artifactId>build-helper-maven-plugin</artifactId> - <version>1.8</version> + <version>3.4.0</version> <executions>
145-145: Inconsistent dateLibrary in commented OpenAPI generator config.The commented
dateLibraryis set tojava17, but the project's compiler targets Java 1.8 (line 119-120). If the OpenAPI generator is run, this mismatch could cause compatibility issues. Consider updating tojava8if this generator config is intended to be used.pom.xml (1)
899-908: Consider adding explicit scope to external dependencies.The
push.device.handlerandfido2dependencies lack explicit scope declarations, defaulting tocompile. Most other external dependencies in this POM useprovidedscope. Verify whether these should also beprovidedto match the pattern used for similar dependencies (e.g., lines 112-117, 148-159).🔧 Suggested fix
<dependency> <groupId>org.wso2.carbon.identity.notification.push</groupId> <artifactId>org.wso2.carbon.identity.notification.push.device.handler</artifactId> <version>${identity.notification.push.version}</version> + <scope>provided</scope> </dependency> <dependency> <groupId>org.wso2.carbon.identity.local.auth.fido</groupId> <artifactId>org.wso2.carbon.identity.application.authenticator.fido2</artifactId> <version>${identity.local.auth.fido.version}</version> + <scope>provided</scope> </dependency>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/pom.xmlcomponents/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/pom.xmlcomponents/org.wso2.carbon.identity.api.server.credential.management/pom.xmlpom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- components/org.wso2.carbon.identity.api.server.credential.management/pom.xml
🔇 Additional comments (4)
components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.v1/pom.xml (1)
36-111: Dependencies are properly configured.The dependency declarations follow the project conventions with appropriate scopes. The exclusions on
swagger-jaxrsare correctly configured to avoid transitive dependency conflicts. Test dependencies are properly test-scoped.components/org.wso2.carbon.identity.api.server.credential.management/org.wso2.carbon.identity.api.server.credential.management.common/pom.xml (1)
35-52: LGTM!The common module POM is well-structured with appropriate dependencies for credential management functionality. Version management is correctly delegated to the parent POM's dependencyManagement section.
pom.xml (2)
842-847: LGTM - Test dependency addition.The
mockito-testngdependency is properly declared with test scope and a dedicated version property.
1112-1112: LGTM - Module registration.The new credential management module is correctly registered in the modules section.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <identity.local.auth.fido.version>5.4.31</identity.local.auth.fido.version> | ||
| <identity.notification.push.version>1.0.9</identity.notification.push.version> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
org.wso2.carbon.identity.application.authenticator.fido2 5.4.31 Maven Central
💡 Result:
Yes — version 5.4.31 is available on Maven Central.
Maven coordinates:
- groupId: org.wso2.carbon.identity.local.auth.fido
- artifactId: org.wso2.carbon.identity.application.authenticator.fido2
- version: 5.4.31. [1]
[1] Maven Central package page (org.wso2.carbon.identity.application.authenticator.fido2, v5.4.31).
🌐 Web query:
org.wso2.carbon.identity.notification.push 1.0.9 WSO2 repository
💡 Result:
I couldn't find org.wso2.carbon.identity.notification.push version 1.0.9 in the WSO2 public/release repositories. The latest published release I can find is 1.0.6 (published Feb 20, 2025) — see the WSO2/Maven listings. [1][2]
Sources:
- mvnrepository listings for the group (shows available artifacts/versions). [1]
- specific artifact release (example: push feature 1.0.6). [2]
🌐 Web query:
WSO2 Maven repository FIDO2 authenticator 5.4.31 release
💡 Result:
Do you want the Maven coordinates (groupId/artifactId/version) and repository URL for the WSO2 FIDO2 Authenticator 5.4.31 artifact, or do you want me to check whether version 5.4.31 exists and provide download links?
Update identity.notification.push.version to an available release.
The FIDO2 authenticator version 5.4.31 is confirmed available on Maven Central. However, identity.notification.push.version is set to 1.0.9, which does not exist in WSO2 repositories—the latest published release is 1.0.6. This will cause a build failure. Update to version 1.0.6 or verify if 1.0.9 is available in a different repository.
🤖 Prompt for AI Agents
In `@pom.xml` around lines 1042 - 1043, The pom property
identity.notification.push.version is set to the non-existent 1.0.9 and will
break the build; change the property value to the latest published release 1.0.6
(or point to a repository that actually hosts 1.0.9) by updating the
identity.notification.push.version property in the POM so it matches an
available artifact, then re-run the build to confirm resolution.
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/21033768213
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pom.xml`:
- Around line 941-948: The Maven pom's compilerArgs section currently injects
JVM --add-opens flags (the <compilerArgs> block and its <arg> entries) which are
only valid on JDK 9+, so remove those unguarded --add-opens lines or relocate
them into a JDK9+ specific Maven profile; alternatively adjust the build matrix
to drop OpenJDK8. Update the <compilerArgs> block by either deleting the
<arg>-J--add-opens=...</arg> entries or wrap them in a profile activated for JDK
9+ (e.g., a profile keyed to a property or toolchain) so that functions/builds
that run on openjdk8 won't receive these flags.
♻️ Duplicate comments (1)
pom.xml (1)
1050-1051: Ensureidentity.notification.push.versionpoints to a published artifact.Please verify that
1.0.9is actually available in your repositories; otherwise the build will fail. If it’s not published, update to a known released version or add the correct repo.🔧 Possible adjustment (if 1.0.9 isn’t published)
- <identity.notification.push.version>1.0.9</identity.notification.push.version> + <identity.notification.push.version>1.0.6</identity.notification.push.version>
3 participants
This pull request introduces a new common module for credential management in the WSO2 Identity Server API, providing foundational interfaces, DTOs, constants, and exception handling for credential management operations. The changes establish a clear contract for handling different credential types (such as passkeys and push authentication), standardize error handling, and facilitate integration with relevant backend services.
Key additions and changes include:
Core Interfaces and DTOs
CredentialManagementServiceandCredentialHandlerinterfaces, defining methods for retrieving and deleting user credentials, and introduced theCredentialDTOclass for transferring credential data. [1] [2] [3]Constants and Credential Types
CredentialManagementConstants, which defines supported credential types (e.g., passkey, push-auth) and standardized error messages for both server and client errors.Exception Handling Framework
CredentialMgtExceptionand specialized client and server exceptions (CredentialMgtClientException,CredentialMgtServerException). [1] [2] [3]Service Integration
CredentialManagementServiceDataHolderto provide access to backend services (e.g., WebAuthn and Push Device Handler) required for credential operations.Build Configuration
pom.xmlfor the common module, specifying dependencies on required WSO2 and test libraries.## PurposeGoals
Approach
User stories
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Related Issues
wso2/product-is#25666
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.