Register credential management v2 REST API in product-IS#28019
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds integration test support for Credential Management REST API v2: bumps framework and user-api versions, registers the v2 Users API in servlet wiring, adds managed and WAR dependencies, introduces Jackson model classes (CredentialCreationResponse, CredentialEntry, CredentialsByType), implements CredentialManagementV2RestClient, adds a test base class (Swagger loading, RestAssured hooks, org base path conversion), and adds positive and negative TestNG suites with org-scoped setup/teardown and comprehensive scenarios. Sequence DiagramsequenceDiagram
participant TestSuite as Test Suite
participant Client as CredentialManagementV2RestClient
participant API as Credential API v2
participant Storage as Backend Storage
TestSuite->>Client: new CredentialManagementV2RestClient(serverUrl, tenant)
Client->>Client: buildBasePath(tenant)
TestSuite->>Client: getUserCredentials(userId)
Client->>API: GET /api/users/v2/{userId}/credentials
API->>Storage: Query credentials by type
Storage-->>API: CredentialsByType
API-->>Client: HTTP 200 + JSON
Client->>Client: deserialize to CredentialsByType
Client-->>TestSuite: CredentialsByType
TestSuite->>Client: createBackupCode(userId)
Client->>API: POST /api/users/v2/{userId}/credentials/backup-code
API->>Storage: Generate backup codes
Storage-->>API: CredentialCreationResponse
API-->>Client: HTTP 201 + JSON
Client->>Client: deserialize to CredentialCreationResponse
Client-->>TestSuite: CredentialCreationResponse
TestSuite->>Client: deleteCredentialById(userId, type, credentialId)
Client->>API: DELETE /api/users/v2/{userId}/credentials/{type}/{credentialId}
API->>Storage: Delete credential
Storage-->>API: Deletion result
API-->>Client: HTTP 204/400/404
Client-->>TestSuite: HTTP status code
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/CredentialManagementV2RestClient.java (1)
66-78: 💤 Low valueConsider reusing ObjectMapper instance.
Creating a new
ObjectMapper(new JsonFactory())on each call is not idiomatic. TheJsonFactoryconstructor argument is redundant sinceObjectMappercreates one internally. For better performance and idiomatic Jackson usage, declare a static finalObjectMapperinstance and reuse it across calls.♻️ Suggested refactor
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); + public CredentialsByType getUserCredentials(String userId) throws Exception { String url = credentialManagementBasePath + PATH_SEPARATOR + userId + CREDENTIALS_PATH; try (CloseableHttpResponse response = getResponseOfHttpGet(url, getHeadersWithBasicAuth())) { int statusCode = response.getStatusLine().getStatusCode(); if (statusCode != HttpStatus.SC_OK) { throw new Exception("Unexpected status " + statusCode + " while getting credentials for user: " + userId); } String body = EntityUtils.toString(response.getEntity()); - return new ObjectMapper(new JsonFactory()).readValue(body, CredentialsByType.class); + return OBJECT_MAPPER.readValue(body, CredentialsByType.class); } }Apply the same pattern to
createBackupCodeon line 98.Also applies to: 87-100
🤖 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 `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/CredentialManagementV2RestClient.java` around lines 66 - 78, The code creates a new ObjectMapper(new JsonFactory()) inside getUserCredentials (and similarly in createBackupCode) each call; instead define a private static final ObjectMapper OBJECT_MAPPER (or similar) at class level in CredentialManagementV2RestClient and use OBJECT_MAPPER.readValue(...) in getUserCredentials and createBackupCode to reuse the instance and remove the redundant JsonFactory constructor call.
🤖 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
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.java`:
- Around line 210-218: In testCrossOrganizationGetCredentialsIsolation, add an
explicit assertion so the test fails if
credentialManagementV2RestClient.getUserCredentials(SECONDARY_ORG_USER_ID)
unexpectedly succeeds: either wrap the call to getUserCredentials and call
Assert.fail() immediately after a successful return, or capture the response and
assert it indicates non-success (e.g., response status is 404 or the returned
credential list is empty) using the test framework's Assert methods; update the
catch block to assert the exception type/message if you expect a specific error.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2PositiveTest.java`:
- Around line 85-96: The subclass method testCleanup (in
CredentialManagementV2PositiveTest) calls super.testConclude(), which causes the
base-class `@AfterClass` teardown to run twice; remove the super.testConclude()
invocation from testCleanup (or alternatively remove the `@AfterClass` from the
base class and keep a single teardown) so teardown is executed only once; ensure
credentialManagementV2RestClient.closeHttpClient() and
orgMgtRestClient.closeHttpClient() still run in the subclass cleanup after
removing the duplicate super call.
- Around line 197-204: The test currently treats 400 as an acceptable outcome
which masks failures; update testDeletePasskeyById to provision deterministic
test data (create a known passkey credential for PARENT_ORG_USER_ID using
TYPE_PASSKEY and capture its credential ID) then call
credentialManagementV2RestClient.deleteCredentialById with that known ID and
assert the returned status equals HttpStatus.SC_NO_CONTENT only; apply the same
change pattern to the other positive tests that currently accept 400 (the other
delete/positive test methods in this class) so they create deterministic
credentials and assert 204 strictly.
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/CredentialManagementV2RestClient.java`:
- Around line 66-78: The code creates a new ObjectMapper(new JsonFactory())
inside getUserCredentials (and similarly in createBackupCode) each call; instead
define a private static final ObjectMapper OBJECT_MAPPER (or similar) at class
level in CredentialManagementV2RestClient and use OBJECT_MAPPER.readValue(...)
in getUserCredentials and createBackupCode to reuse the instance and remove the
redundant JsonFactory constructor call.
🪄 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: aa32ddca-8c0d-4959-8a27-4b9ad04ba4e4
📒 Files selected for processing (12)
modules/api-resources/api-resources-full/pom.xmlmodules/api-resources/api-resources-full/src/main/webapp/WEB-INF/web.xmlmodules/api-resources/pom.xmlmodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2PositiveTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialCreationResponse.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialEntry.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialsByType.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/CredentialManagementV2RestClient.javamodules/integration/tests-integration/tests-backend/src/test/resources/testng.xmlpom.xml
d85fb50 to
b3a2cff
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.java (1)
109-116: Make the non-super-tenant replacement boundary-aware (but this likely doesn’t break current tests).
convertToOrgBasePathusesbasePath.replace(tenant, tenant + ORGANIZATION_PATH_SPECIFIER), which replaces all occurrences. In current test flow,basePathis built byISIntegrationTest.getTenantedRelativePath(...)as"/t/" + tenantDomain + endpointURLWithHostname(whereendpointURLWithHostnamedoesn’t include the tenant), sotenantshould appear only once and the replacement should be correct. Still, replacing only the/t/<tenant>segment (e.g., viareplaceFirst/segment-aware logic) would make this conversion more robust.🤖 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 `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.java` around lines 109 - 116, convertToOrgBasePath currently uses basePath.replace(tenant, tenant + ORGANIZATION_PATH_SPECIFIER) which replaces all occurrences of the tenant string; change this to only replace the /t/<tenant> segment so replacement is boundary-aware (e.g., target TENANTED_URL_PATH_SPECIFIER + tenant and append ORGANIZATION_PATH_SPECIFIER) to avoid accidental rewrites elsewhere in basePath — update the logic inside convertToOrgBasePath to search for the specific TENANTED_URL_PATH_SPECIFIER + tenant pattern (or use replaceFirst with an anchored regex for that segment) and only append ORGANIZATION_PATH_SPECIFIER to that match.
🤖 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.
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.java`:
- Around line 109-116: convertToOrgBasePath currently uses
basePath.replace(tenant, tenant + ORGANIZATION_PATH_SPECIFIER) which replaces
all occurrences of the tenant string; change this to only replace the
/t/<tenant> segment so replacement is boundary-aware (e.g., target
TENANTED_URL_PATH_SPECIFIER + tenant and append ORGANIZATION_PATH_SPECIFIER) to
avoid accidental rewrites elsewhere in basePath — update the logic inside
convertToOrgBasePath to search for the specific TENANTED_URL_PATH_SPECIFIER +
tenant pattern (or use replaceFirst with an anchored regex for that segment) and
only append ORGANIZATION_PATH_SPECIFIER to that match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7796d4c-311a-4b20-95bb-99e6b06fead1
⛔ Files ignored due to path filters (1)
modules/integration/tests-integration/tests-backend/logs/automation.logis excluded by!**/*.log
📒 Files selected for processing (12)
modules/api-resources/api-resources-full/pom.xmlmodules/api-resources/api-resources-full/src/main/webapp/WEB-INF/web.xmlmodules/api-resources/pom.xmlmodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2PositiveTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialCreationResponse.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialEntry.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialsByType.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/CredentialManagementV2RestClient.javamodules/integration/tests-integration/tests-backend/src/test/resources/testng.xmlpom.xml
✅ Files skipped from review due to trivial changes (1)
- modules/api-resources/api-resources-full/pom.xml
🚧 Files skipped from review as they are similar to previous changes (9)
- modules/api-resources/pom.xml
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialEntry.java
- modules/integration/tests-integration/tests-backend/src/test/resources/testng.xml
- modules/api-resources/api-resources-full/src/main/webapp/WEB-INF/web.xml
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialsByType.java
- pom.xml
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2PositiveTest.java
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/CredentialManagementV2RestClient.java
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.java
b3a2cff to
055780c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.java (1)
249-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit assertion for cross-org GET isolation test.
testCrossOrganizationGetCredentialsIsolationlacks an assertion. The test can pass even if the request unexpectedly succeeds and returns credentials from another organization. AddAssert.fail("Expected exception for cross-org access")after line 253, or verify the response indicates an error.🤖 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 `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.java` around lines 249 - 257, The test method testCrossOrganizationGetCredentialsIsolation currently catches any exception but has no assertion, so it can silently succeed; update the method in CredentialManagementV2NegativeTest to explicitly fail when no exception/error is thrown by adding an assertion (e.g., Assert.fail("Expected exception for cross-org access")) immediately after calling credentialManagementV2RestClient.getUserCredentials(SECONDARY_ORG_USER_ID), or instead inspect the returned response and assert it indicates a 404/empty result to ensure cross-org credentials are not returned.
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.java (1)
106-113: 💤 Low valueConsider more robust path construction for non-super tenant case.
Line 111 uses
basePath.replace(tenant, tenant + "/o")which performs a simple string replacement. If the tenant domain appears elsewhere in the basePath (e.g., as part of a user ID or resource name), it could be inadvertently replaced.For test code this is likely acceptable given controlled inputs, but a more robust approach would use path parsing or regex with word boundaries.
🤖 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 `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.java` around lines 106 - 113, The convertToOrgBasePath method uses basePath.replace(tenant, tenant + ORGANIZATION_PATH_SPECIFIER) which can accidentally replace tenant substrings elsewhere; update convertToOrgBasePath to build the path more robustly by parsing the path segments and inserting ORGANIZATION_PATH_SPECIFIER immediately after the tenant segment (e.g., split basePath on '/' or use a regex that matches the tenant as a full path segment) so only the tenant path segment is modified; ensure this logic is applied in the non-super-tenant branch and keeps existing behavior for SUPER_TENANT_DOMAIN_NAME.
🤖 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
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.java`:
- Around line 203-211: Update the assertion in
testDeletePasskeyWithNonExistentCredentialId to accept 404 in addition to 400
(or assert against the same allowed set used in testDeletePasskeyById) so the
negative test aligns with the other test's expectations; locate the call to
credentialManagementV2RestClient.deleteCredentialById(testUserId, TYPE_PASSKEY,
NON_EXISTENT_CREDENTIAL_ID) and change the Assert on response.getStatusCode() to
allow HttpStatus.SC_NOT_FOUND (404) as an acceptable result (or mirror the exact
status-set logic used by testDeletePasskeyById).
---
Duplicate comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.java`:
- Around line 249-257: The test method
testCrossOrganizationGetCredentialsIsolation currently catches any exception but
has no assertion, so it can silently succeed; update the method in
CredentialManagementV2NegativeTest to explicitly fail when no exception/error is
thrown by adding an assertion (e.g., Assert.fail("Expected exception for
cross-org access")) immediately after calling
credentialManagementV2RestClient.getUserCredentials(SECONDARY_ORG_USER_ID), or
instead inspect the returned response and assert it indicates a 404/empty result
to ensure cross-org credentials are not returned.
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.java`:
- Around line 106-113: The convertToOrgBasePath method uses
basePath.replace(tenant, tenant + ORGANIZATION_PATH_SPECIFIER) which can
accidentally replace tenant substrings elsewhere; update convertToOrgBasePath to
build the path more robustly by parsing the path segments and inserting
ORGANIZATION_PATH_SPECIFIER immediately after the tenant segment (e.g., split
basePath on '/' or use a regex that matches the tenant as a full path segment)
so only the tenant path segment is modified; ensure this logic is applied in the
non-super-tenant branch and keeps existing behavior for
SUPER_TENANT_DOMAIN_NAME.
🪄 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: 9578192e-7727-4a84-aa1b-6425033fcd88
📒 Files selected for processing (12)
modules/api-resources/api-resources-full/pom.xmlmodules/api-resources/api-resources-full/src/main/webapp/WEB-INF/web.xmlmodules/api-resources/pom.xmlmodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementTestBase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2NegativeTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2PositiveTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialCreationResponse.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialEntry.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialsByType.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/CredentialManagementV2RestClient.javamodules/integration/tests-integration/tests-backend/src/test/resources/testng.xmlpom.xml
✅ Files skipped from review due to trivial changes (1)
- modules/api-resources/api-resources-full/pom.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/api-resources/pom.xml
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialsByType.java
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialEntry.java
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/model/CredentialCreationResponse.java
- modules/api-resources/api-resources-full/src/main/webapp/WEB-INF/web.xml
- modules/integration/tests-integration/tests-backend/src/test/resources/testng.xml
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/user/credential/management/v2/CredentialManagementV2PositiveTest.java
a399d61 to
a0beccd
Compare
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
a0beccd to
d928049
Compare
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
d928049 to
2a378a6
Compare
|
|
PR builder started |
|
PR builder completed |



Summary
org.wso2.carbon.identity.rest.api.user.credential.v2.UsersApiin theUsersV2Servlet(/api/users/v2/*) inweb.xmlorg.wso2.carbon.identity.rest.api.user.credential.v2andorg.wso2.carbon.identity.api.user.credential.commonas dependencies inapi-resources-full/pom.xmldependencyManagemententries inapi-resources/pom.xmlunder${identity.user.api.version}identity.user.api.versionto1.5.7-SNAPSHOTto pull in the new credential management v2 modulesCredentialManagementV2PositiveTest,CredentialManagementV2NegativeTest) totestng.xml, along with the test base class, model classes, andCredentialManagementV2RestClientRelated PRs
Test plan
/api/users/v2/{user-id}/credentialsendpoints are accessibleCredentialManagementV2PositiveTestandCredentialManagementV2NegativeTestintegration testsRelated to