Add integration tests for sub organization application's token exchange grant type#27440
Draft
ShanChathusanda93 wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 a new TestNG integration test class Sequence Diagram(s)sequenceDiagram
participant TestClient as TestClient
participant SecondaryIS as SecondaryIS
participant SubOrgIS as SubOrgIS
participant JWKS as JWKS
TestClient->>SecondaryIS: Request access token (password grant + Basic auth)
SecondaryIS-->>TestClient: Return access_token (JWT)
TestClient->>SubOrgIS: POST /o/<org>/oauth2/token (grant_type=token_exchange, subject_token=secondary JWT)
SubOrgIS->>JWKS: Fetch issuer JWKS for validation
JWKS-->>SubOrgIS: Return JWKS keys
SubOrgIS-->>TestClient: Return exchanged access_token (JWT)
TestClient->>TestClient: Parse exchanged JWTs and compare `sub` claim
Changes
Estimated code review effortHigh | ~45–60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java (1)
77-78: Compute the secondary token/JWKS URLs from the secondary server context.Pinning both URLs to
https://localhost:9854makes the test depend on one host/port layout. The rest of the class already uses port offsets and automation contexts, so these should be derived fromPORT_OFFSET_1instead of hardcoding them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 77 - 78, SECONDARY_IS_TOKEN_ENDPOINT and SECONDARY_IS_JWKS_URI are hardcoded to https://localhost:9854; instead compute them from PORT_OFFSET_1 so the test follows the same port-offset logic as the rest of the class. Replace the fixed strings by constructing the base HTTPS URL using the standard HTTPS port plus PORT_OFFSET_1 (or by using the test automation/context helper used elsewhere in this class), then append "/oauth2/token" for SECONDARY_IS_TOKEN_ENDPOINT and "/oauth2/jwks" for SECONDARY_IS_JWKS_URI so the constants derive from PORT_OFFSET_1 rather than hardcoded 9854.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 73-76: Replace the static identifiers with runtime-unique values
so parallel runs or leftover resources don't collide: generate unique suffixes
(timestamp/UUID/test-run-id) and append them to SUB_ORG_NAME, SUB_ORG_APP_NAME,
TRUSTED_TOKEN_ISSUER_NAME, SECONDARY_IS_SP_NAME and the other static names
referenced around the createOIDCConfiguration() usage; ensure all code that
creates or looks up resources (e.g., createOIDCConfiguration(), SP/IDP/user
creation helpers) uses the generated variables instead of the original constants
so the test creates and queries resources by the unique names.
- Around line 194-198: The teardown in the `@AfterClass`(alwaysRun = true) method
unconditionally calls close on httpClient, orgMgtRestClient, oAuth2RestClient,
and idpMgtRestClient which can be null if initTest() failed and thereby mask the
original setup error; update the teardown to guard each resource (httpClient,
orgMgtRestClient, oAuth2RestClient, idpMgtRestClient) with null checks or wrap
each close call in its own try/catch so you only attempt close when the object
is non-null and swallow/log close-time exceptions without throwing, ensuring the
original initialization exception is not overwritten.
- Around line 463-480: The sendPOSTMessage method currently parses the response
body without checking HTTP status; update sendPOSTMessage to inspect the
HttpResponse status code from the HttpResponse returned by
httpClient.execute(httpPost) and if it's not a 2xx success, throw an Exception
(or fail the test) that includes the numeric status code and the raw
responseString to surface OAuth error details, otherwise proceed to parse the
body as JSON; also switch to reading the entity using StandardCharsets.UTF_8
(import java.nio.charset.StandardCharsets) to avoid platform-encoding issues and
ensure you still call EntityUtils.consume on the entity after reading.
- Around line 181-184: The cleanup is calling deleteApplication(subOrgAppId)
(root-tenant API) while the app was created with
createOrganizationApplication(..., switchedM2MToken) against
subOrgApplicationManagementApiBasePath; add a new org-scoped deletion to
OAuth2RestClient such as deleteOrganizationApplication(String appId, String
accessToken) that issues a DELETE to
subOrgApplicationManagementApiBasePath/{appId} using the switchedM2MToken
(switchedM2MToken -> accessToken) and replace the call in
OAuth2TokenExchangeSubOrganizationTestCase to use
deleteOrganizationApplication(subOrgAppId, switchedM2MToken); if you add the new
API ensure the internal_org_application_mgt_delete permission is granted in
organization-service-apis.json as well.
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 77-78: SECONDARY_IS_TOKEN_ENDPOINT and SECONDARY_IS_JWKS_URI are
hardcoded to https://localhost:9854; instead compute them from PORT_OFFSET_1 so
the test follows the same port-offset logic as the rest of the class. Replace
the fixed strings by constructing the base HTTPS URL using the standard HTTPS
port plus PORT_OFFSET_1 (or by using the test automation/context helper used
elsewhere in this class), then append "/oauth2/token" for
SECONDARY_IS_TOKEN_ENDPOINT and "/oauth2/jwks" for SECONDARY_IS_JWKS_URI so the
constants derive from PORT_OFFSET_1 rather than hardcoded 9854.
🪄 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: 0b0f92da-3a92-4fe4-9701-682c0250b3ad
📒 Files selected for processing (2)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.javamodules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
Comment on lines
+73
to
+76
| private static final String SUB_ORG_NAME = "TokenExchangeTestOrg"; | ||
| private static final String SUB_ORG_APP_NAME = "subOrgApp"; | ||
| private static final String TRUSTED_TOKEN_ISSUER_NAME = "trustedTokenIssuer"; | ||
| private static final String SECONDARY_IS_SP_NAME = "secondarySP"; |
Contributor
There was a problem hiding this comment.
Make the created org/app/SP/IDP/user names unique per test run.
These identifiers are all static, so a failed cleanup or a parallel retry can make setup fail on duplicates. It also makes createOIDCConfiguration() vulnerable to picking up a stale OAuth app with the same applicationName.
Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 73 - 76, Replace the static identifiers with runtime-unique values
so parallel runs or leftover resources don't collide: generate unique suffixes
(timestamp/UUID/test-run-id) and append them to SUB_ORG_NAME, SUB_ORG_APP_NAME,
TRUSTED_TOKEN_ISSUER_NAME, SECONDARY_IS_SP_NAME and the other static names
referenced around the createOIDCConfiguration() usage; ensure all code that
creates or looks up resources (e.g., createOIDCConfiguration(), SP/IDP/user
creation helpers) uses the generated variables instead of the original constants
so the test creates and queries resources by the unique names.
Comment on lines
+194
to
+198
| // Close HTTP clients | ||
| httpClient.close(); | ||
| orgMgtRestClient.closeHttpClient(); | ||
| oAuth2RestClient.closeHttpClient(); | ||
| idpMgtRestClient.closeHttpClient(); |
Contributor
There was a problem hiding this comment.
Avoid masking setup failures during teardown.
@AfterClass(alwaysRun = true) can run after a partial initTest() failure, but these closes are unconditional. A null client here throws again and hides the original setup error.
Proposed fix
- httpClient.close();
- orgMgtRestClient.closeHttpClient();
- oAuth2RestClient.closeHttpClient();
- idpMgtRestClient.closeHttpClient();
+ if (httpClient != null) {
+ httpClient.close();
+ }
+ if (orgMgtRestClient != null) {
+ orgMgtRestClient.closeHttpClient();
+ }
+ if (oAuth2RestClient != null) {
+ oAuth2RestClient.closeHttpClient();
+ }
+ if (idpMgtRestClient != null) {
+ idpMgtRestClient.closeHttpClient();
+ }📝 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
| // Close HTTP clients | |
| httpClient.close(); | |
| orgMgtRestClient.closeHttpClient(); | |
| oAuth2RestClient.closeHttpClient(); | |
| idpMgtRestClient.closeHttpClient(); | |
| // Close HTTP clients | |
| if (httpClient != null) { | |
| httpClient.close(); | |
| } | |
| if (orgMgtRestClient != null) { | |
| orgMgtRestClient.closeHttpClient(); | |
| } | |
| if (oAuth2RestClient != null) { | |
| oAuth2RestClient.closeHttpClient(); | |
| } | |
| if (idpMgtRestClient != null) { | |
| idpMgtRestClient.closeHttpClient(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 194 - 198, The teardown in the `@AfterClass`(alwaysRun = true) method
unconditionally calls close on httpClient, orgMgtRestClient, oAuth2RestClient,
and idpMgtRestClient which can be null if initTest() failed and thereby mask the
original setup error; update the teardown to guard each resource (httpClient,
orgMgtRestClient, oAuth2RestClient, idpMgtRestClient) with null checks or wrap
each close call in its own try/catch so you only attempt close when the object
is non-null and swallow/log close-time exceptions without throwing, ensuring the
original initialization exception is not overwritten.
Comment on lines
+463
to
+480
| private JSONObject sendPOSTMessage(String endpoint, String clientID, String clientSecret, | ||
| List<NameValuePair> postParameters) throws Exception { | ||
|
|
||
| HttpPost httpPost = new HttpPost(endpoint); | ||
| httpPost.setHeader("Authorization", "Basic " + getBase64EncodedString(clientID, clientSecret)); | ||
| httpPost.setHeader("Content-Type", "application/x-www-form-urlencoded"); | ||
| httpPost.setEntity(new UrlEncodedFormEntity(postParameters)); | ||
|
|
||
| HttpResponse response = httpClient.execute(httpPost); | ||
| String responseString = EntityUtils.toString(response.getEntity(), "UTF-8"); | ||
| EntityUtils.consume(response.getEntity()); | ||
|
|
||
| JSONParser parser = new JSONParser(); | ||
| JSONObject json = (JSONObject) parser.parse(responseString); | ||
| if (json == null) { | ||
| throw new Exception("Error occurred while getting the response"); | ||
| } | ||
| return json; |
Contributor
There was a problem hiding this comment.
Assert the token endpoint result before parsing the happy-path body.
sendPOSTMessage() never checks the HTTP status and assumes an access_token will be present. When either token call returns an OAuth error, the class fails later with responseObject.get("access_token").toString() instead of surfacing the real response.
Proposed fix
- HttpResponse response = httpClient.execute(httpPost);
- String responseString = EntityUtils.toString(response.getEntity(), "UTF-8");
+ HttpResponse response = httpClient.execute(httpPost);
+ int statusCode = response.getStatusLine().getStatusCode();
+ String responseString = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8);
EntityUtils.consume(response.getEntity());
+ if (statusCode < 200 || statusCode >= 300) {
+ throw new AssertionError("Token endpoint returned " + statusCode + ": " + responseString);
+ }
JSONParser parser = new JSONParser();
JSONObject json = (JSONObject) parser.parse(responseString);
- if (json == null) {
+ if (json == null || !json.containsKey("access_token")) {
throw new Exception("Error occurred while getting the response");
}You'd also need import java.nio.charset.StandardCharsets;.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 463 - 480, The sendPOSTMessage method currently parses the response
body without checking HTTP status; update sendPOSTMessage to inspect the
HttpResponse status code from the HttpResponse returned by
httpClient.execute(httpPost) and if it's not a 2xx success, throw an Exception
(or fail the test) that includes the numeric status code and the raw
responseString to surface OAuth error details, otherwise proceed to parse the
body as JSON; also switch to reading the entity using StandardCharsets.UTF_8
(import java.nio.charset.StandardCharsets) to avoid platform-encoding issues and
ensure you still call EntityUtils.consume on the entity after reading.
ShanChathusanda93
force-pushed
the
sub-org-token-exchange-integration-tests-branch
branch
from
April 21, 2026 09:18
ea0cd36 to
dcb36a5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java (3)
471-479:⚠️ Potential issue | 🟡 MinorCheck the token endpoint result before parsing the happy path.
Surface non-2xx responses and missing
access_tokendirectly from this helper instead of failing later at.toString().Proposed fix
HttpResponse response = httpClient.execute(httpPost); + int statusCode = response.getStatusLine().getStatusCode(); String responseString = EntityUtils.toString(response.getEntity(), "UTF-8"); EntityUtils.consume(response.getEntity()); + + if (statusCode < 200 || statusCode >= 300) { + throw new Exception("Token endpoint returned " + statusCode + ": " + responseString); + } JSONParser parser = new JSONParser(); JSONObject json = (JSONObject) parser.parse(responseString); - if (json == null) { + if (json == null || !json.containsKey("access_token")) { throw new Exception("Error occurred while getting the response"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 471 - 479, Before parsing the response in the token helper, check the HTTP status and presence of the access token: after executing httpClient.execute(httpPost) read the response entity once into responseString, then verify response.getStatusLine().getStatusCode() is 2xx and throw a clear Exception including the status and responseString if not; after parsing with JSONParser (parser) and creating JSONObject json, check json contains "access_token" and throw a descriptive Exception if missing rather than letting a later .toString() NPE surface; ensure you don't consume the entity twice (use EntityUtils.toString(response.getEntity(), "UTF-8") once and remove the redundant EntityUtils.consume call).
73-76:⚠️ Potential issue | 🟠 MajorUse run-unique resource identifiers.
These static org/app/SP/IDP/user values can collide after partial cleanup or parallel execution, and the OAuth app lookup by application name can pick up stale state. Add a UUID/test-run suffix and use the generated values consistently for create, lookup, and cleanup.
Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 73 - 76, The hard-coded constants SUB_ORG_NAME, SUB_ORG_APP_NAME, TRUSTED_TOKEN_ISSUER_NAME, SECONDARY_IS_SP_NAME (and the other static names around lines 79–81) must be made unique per test run to avoid collisions: generate a short UUID/test-run suffix in the test setup (e.g., in a `@BeforeClass` or setup method), append it to each base name when constructing the actual values used for create, lookup and cleanup, and replace usages of the static final constants with the runtime-generated names so creation, lookup and teardown all use the same unique identifiers.
172-202:⚠️ Potential issue | 🟠 MajorMake teardown best-effort, org-scoped, and null-safe.
A failure in one cleanup step currently skips the remaining steps. Also, the sub-org app should be deleted through the organization-scoped API using
switchedM2MToken, and client closes should be guarded for partial setup failures. Handle each cleanup independently and rethrow after all cleanup attempts complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 172 - 202, Make the teardown best-effort and null-safe by running each cleanup step independently, capturing any exceptions and continuing, then rethrowing a combined/first exception after all attempts; specifically, wrap calls to super.deleteUser(PORT_OFFSET_1, NEW_USER_USERNAME), idpMgtRestClient.deleteIdpInOrganization(subOrgIdpId, switchedM2MToken), oAuth2RestClient.deleteApplicationInOrganization(subOrgAppId, switchedM2MToken) (use the org-scoped delete with switchedM2MToken instead of oAuth2RestClient.deleteApplication), super.deleteServiceProvider(PORT_OFFSET_1, SECONDARY_IS_SP_NAME), and orgMgtRestClient.deleteOrganization(subOrgId) each in their own try/catch so one failure doesn’t stop the rest, and guard httpClient.close(), orgMgtRestClient.closeHttpClient(), oAuth2RestClient.closeHttpClient(), idpMgtRestClient.closeHttpClient() with null checks and individual try/catch blocks; after all attempts, if any exception occurred, rethrow or wrap it to signal teardown failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 77-78: Replace the hard-coded SECONDARY_IS_TOKEN_ENDPOINT and
SECONDARY_IS_JWKS_URI with values derived from the automation context for
PORT_OFFSET_1: use the same pattern as in AbstractIdentityFederationTestCase
(retrieve the AutomationContext/ContextUrls for PORT_OFFSET_1 from
automationContextMap, then build the secure base URL and append "/oauth2/token"
and "/oauth2/jwks"). Update the constants SECONDARY_IS_TOKEN_ENDPOINT and
SECONDARY_IS_JWKS_URI to be initialized from that constructed URL so they follow
the automationContextMap / ContextUrls usage rather than
"https://localhost:9854".
- Around line 140-147: The catch block in
OAuth2TokenExchangeSubOrganizationTestCase that wraps RESTTestBase.readResource
hides the original failure; modify the catch to preserve the cause by including
the caught exception when rethrowing (e.g., throw new RuntimeException("Could
not load authorized APIs JSON.", e)) or by appending e.getMessage() to the
message so authorizedAPIs/RESTTestBase.readResource failures are visible in logs
and stacktraces.
- Around line 224-229: Test currently compares exchangedSubject and
originalSubject but doesn't verify they are non-null; add preconditions to
assert both getTokenSubject(accessTokenFromSecondaryIS) and
getTokenSubject(exchangedToken) are not null before calling Assert.assertEquals.
Specifically, update the block around originalSubject/exchangedSubject to call
assertions that originalSubject and exchangedSubject are present (e.g.,
Assert.assertNotNull on originalSubject and exchangedSubject with clear
messages) and then perform the Assert.assertEquals comparing exchangedSubject to
originalSubject.
---
Duplicate comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 471-479: Before parsing the response in the token helper, check
the HTTP status and presence of the access token: after executing
httpClient.execute(httpPost) read the response entity once into responseString,
then verify response.getStatusLine().getStatusCode() is 2xx and throw a clear
Exception including the status and responseString if not; after parsing with
JSONParser (parser) and creating JSONObject json, check json contains
"access_token" and throw a descriptive Exception if missing rather than letting
a later .toString() NPE surface; ensure you don't consume the entity twice (use
EntityUtils.toString(response.getEntity(), "UTF-8") once and remove the
redundant EntityUtils.consume call).
- Around line 73-76: The hard-coded constants SUB_ORG_NAME, SUB_ORG_APP_NAME,
TRUSTED_TOKEN_ISSUER_NAME, SECONDARY_IS_SP_NAME (and the other static names
around lines 79–81) must be made unique per test run to avoid collisions:
generate a short UUID/test-run suffix in the test setup (e.g., in a `@BeforeClass`
or setup method), append it to each base name when constructing the actual
values used for create, lookup and cleanup, and replace usages of the static
final constants with the runtime-generated names so creation, lookup and
teardown all use the same unique identifiers.
- Around line 172-202: Make the teardown best-effort and null-safe by running
each cleanup step independently, capturing any exceptions and continuing, then
rethrowing a combined/first exception after all attempts; specifically, wrap
calls to super.deleteUser(PORT_OFFSET_1, NEW_USER_USERNAME),
idpMgtRestClient.deleteIdpInOrganization(subOrgIdpId, switchedM2MToken),
oAuth2RestClient.deleteApplicationInOrganization(subOrgAppId, switchedM2MToken)
(use the org-scoped delete with switchedM2MToken instead of
oAuth2RestClient.deleteApplication), super.deleteServiceProvider(PORT_OFFSET_1,
SECONDARY_IS_SP_NAME), and orgMgtRestClient.deleteOrganization(subOrgId) each in
their own try/catch so one failure doesn’t stop the rest, and guard
httpClient.close(), orgMgtRestClient.closeHttpClient(),
oAuth2RestClient.closeHttpClient(), idpMgtRestClient.closeHttpClient() with null
checks and individual try/catch blocks; after all attempts, if any exception
occurred, rethrow or wrap it to signal teardown failure.
🪄 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: 9977d4f0-d542-4a19-be4b-7c35124122fa
📒 Files selected for processing (2)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.javamodules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
✅ Files skipped from review due to trivial changes (1)
- modules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
Comment on lines
+77
to
+78
| private static final String SECONDARY_IS_TOKEN_ENDPOINT = "https://localhost:9854/oauth2/token"; | ||
| private static final String SECONDARY_IS_JWKS_URI = "https://localhost:9854/oauth2/jwks"; |
Contributor
There was a problem hiding this comment.
Derive the secondary IS endpoints from the automation context.
Hard-coding localhost:9854 ties the test to one runtime layout. Build the token and JWKS URLs from automationContextMap/ContextUrls for PORT_OFFSET_1, matching the client initialization pattern in AbstractIdentityFederationTestCase.java:117-159.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 77 - 78, Replace the hard-coded SECONDARY_IS_TOKEN_ENDPOINT and
SECONDARY_IS_JWKS_URI with values derived from the automation context for
PORT_OFFSET_1: use the same pattern as in AbstractIdentityFederationTestCase
(retrieve the AutomationContext/ContextUrls for PORT_OFFSET_1 from
automationContextMap, then build the secure base URL and append "/oauth2/token"
and "/oauth2/jwks"). Update the constants SECONDARY_IS_TOKEN_ENDPOINT and
SECONDARY_IS_JWKS_URI to be initialized from that constructed URL so they follow
the automationContextMap / ContextUrls usage rather than
"https://localhost:9854".
Comment on lines
+140
to
+147
| // Read authorized APIs for organization management | ||
| org.json.JSONObject authorizedAPIs = new org.json.JSONObject(); | ||
| try { | ||
| String apisJson = RESTTestBase.readResource("organization-service-apis.json", this.getClass()); | ||
| authorizedAPIs = new org.json.JSONObject(apisJson); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Could not load authorized APIs JSON."); | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve the resource-load failure cause.
The current exception hides whether the JSON was missing, unreadable, or malformed.
Proposed fix
try {
String apisJson = RESTTestBase.readResource("organization-service-apis.json", this.getClass());
authorizedAPIs = new org.json.JSONObject(apisJson);
} catch (Exception e) {
- throw new RuntimeException("Could not load authorized APIs JSON.");
+ throw new RuntimeException("Could not load authorized APIs JSON.", e);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 140 - 147, The catch block in
OAuth2TokenExchangeSubOrganizationTestCase that wraps RESTTestBase.readResource
hides the original failure; modify the catch to preserve the cause by including
the caught exception when rethrowing (e.g., throw new RuntimeException("Could
not load authorized APIs JSON.", e)) or by appending e.getMessage() to the
message so authorizedAPIs/RESTTestBase.readResource failures are visible in logs
and stacktraces.
Comment on lines
+224
to
+229
| // Verify subject in exchanged token matches original token | ||
| String originalSubject = getTokenSubject(accessTokenFromSecondaryIS); | ||
| String exchangedSubject = getTokenSubject(exchangedToken); | ||
|
|
||
| Assert.assertEquals(exchangedSubject, originalSubject, | ||
| "Subject of the exchanged token should be the same as the subject token"); |
Contributor
There was a problem hiding this comment.
Assert both JWT subjects are present before comparing.
If both subjects are unexpectedly null, this test can pass without validating the token-exchange subject propagation.
Proposed fix
String originalSubject = getTokenSubject(accessTokenFromSecondaryIS);
String exchangedSubject = getTokenSubject(exchangedToken);
+
+ Assert.assertNotNull(originalSubject, "Original access token subject is null.");
+ Assert.assertNotNull(exchangedSubject, "Exchanged access token subject is null.");
Assert.assertEquals(exchangedSubject, originalSubject,
"Subject of the exchanged token should be the same as the subject token");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 224 - 229, Test currently compares exchangedSubject and
originalSubject but doesn't verify they are non-null; add preconditions to
assert both getTokenSubject(accessTokenFromSecondaryIS) and
getTokenSubject(exchangedToken) are not null before calling Assert.assertEquals.
Specifically, update the block around originalSubject/exchangedSubject to call
assertions that originalSubject and exchangedSubject are present (e.g.,
Assert.assertNotNull on originalSubject and exchangedSubject with clear
messages) and then perform the Assert.assertEquals comparing exchangedSubject to
originalSubject.
ShanChathusanda93
force-pushed
the
sub-org-token-exchange-integration-tests-branch
branch
from
April 23, 2026 08:23
dcb36a5 to
2cde44c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.java (2)
282-300: Consolidate duplicated header construction.
getHeadersWithBearerTokenandgetHeadersdiffer only in theAuthorizationvalue. Consider a small helper that accepts the authorization attribute to reduce duplication.♻️ Proposed refactor
private Header[] getHeadersWithBearerToken(String accessToken) { - - Header[] headerList = new Header[3]; - headerList[0] = new BasicHeader(USER_AGENT_ATTRIBUTE, OAuth2Constant.USER_AGENT); - headerList[1] = new BasicHeader(AUTHORIZATION_ATTRIBUTE, BEARER_TOKEN_AUTHORIZATION_ATTRIBUTE + accessToken); - headerList[2] = new BasicHeader(CONTENT_TYPE_ATTRIBUTE, String.valueOf(ContentType.JSON)); - return headerList; + return buildHeaders(BEARER_TOKEN_AUTHORIZATION_ATTRIBUTE + accessToken); } private Header[] getHeaders() { - - Header[] headerList = new Header[3]; - headerList[0] = new BasicHeader(USER_AGENT_ATTRIBUTE, OAuth2Constant.USER_AGENT); - headerList[1] = new BasicHeader(AUTHORIZATION_ATTRIBUTE, BASIC_AUTHORIZATION_ATTRIBUTE + - Base64.encodeBase64String((username + ":" + password).getBytes()).trim()); - headerList[2] = new BasicHeader(CONTENT_TYPE_ATTRIBUTE, String.valueOf(ContentType.JSON)); - - return headerList; + return buildHeaders(BASIC_AUTHORIZATION_ATTRIBUTE + + Base64.encodeBase64String((username + ":" + password).getBytes()).trim()); + } + + private Header[] buildHeaders(String authorizationValue) { + + return new Header[] { + new BasicHeader(USER_AGENT_ATTRIBUTE, OAuth2Constant.USER_AGENT), + new BasicHeader(AUTHORIZATION_ATTRIBUTE, authorizationValue), + new BasicHeader(CONTENT_TYPE_ATTRIBUTE, String.valueOf(ContentType.JSON)) + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.java` around lines 282 - 300, Consolidate duplicated header creation by extracting a helper (e.g., buildHeaders or createHeaders) that takes the Authorization header value as a parameter and returns the Header[]; replace getHeadersWithBearerToken and getHeaders so they each call this helper with BEARER_TOKEN_AUTHORIZATION_ATTRIBUTE + accessToken and with BASIC_AUTHORIZATION_ATTRIBUTE + Base64.encodeBase64String((username + ":" + password).getBytes()).trim() respectively, keeping the USER_AGENT_ATTRIBUTE and CONTENT_TYPE_ATTRIBUTE construction inside the helper.
273-280: Reuse the existing base path constant to avoid drift.The hard-coded literal
"api/server/v1/identity-providers"duplicates the existingIDENTITY_PROVIDER_BASE_PATHfield. If the base path ever changes, the org-scoped variant can silently diverge.♻️ Proposed refactor
private String getSubOrgIdentityProviderPath() { if (MultitenantConstants.SUPER_TENANT_DOMAIN_NAME.equals(tenantDomain)) { - return serverUrl + ORGANIZATION_PATH + "api/server/v1/identity-providers"; + return serverUrl + ORGANIZATION_PATH + IDENTITY_PROVIDER_BASE_PATH.replaceFirst("^/", ""); } - return serverUrl + TENANT_PATH + tenantDomain + PATH_SEPARATOR + ORGANIZATION_PATH + - "api/server/v1/identity-providers"; + return serverUrl + TENANT_PATH + tenantDomain + PATH_SEPARATOR + ORGANIZATION_PATH + + IDENTITY_PROVIDER_BASE_PATH.replaceFirst("^/", ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.java` around lines 273 - 280, The getSubOrgIdentityProviderPath method hardcodes "api/server/v1/identity-providers" which duplicates the IDENTITY_PROVIDER_BASE_PATH constant; change the method to reuse IDENTITY_PROVIDER_BASE_PATH instead of the literal so the org-scoped path stays in sync (compose serverUrl + ORGANIZATION_PATH [+ TENANT_PATH + tenantDomain + PATH_SEPARATOR when not super tenant] + IDENTITY_PROVIDER_BASE_PATH), keeping the same condition on MultitenantConstants.SUPER_TENANT_DOMAIN_NAME and existing symbols tenantDomain, serverUrl, ORGANIZATION_PATH, TENANT_PATH, PATH_SEPARATOR.modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java (4)
504-518: Remove trailing blank lines at end of file.There are ~14 empty lines after the closing brace of the class. Trim them to a single trailing newline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 504 - 518, Remove the excessive blank lines after the class closing brace in OAuth2TokenExchangeSubOrganizationTestCase: trim the ~14 empty lines following the final '}' to leave exactly one trailing newline so the file ends with a single newline only.
485-488: Specify charset explicitly when encoding credentials.
(consumerKey + ":" + consumerSecret).getBytes()and the outernew String(...)both rely on the platform default charset, which is non-portable. UseStandardCharsets.UTF_8(orUS_ASCII) to make the Basic-auth encoding deterministic across environments.♻️ Proposed refactor
private String getBase64EncodedString(String consumerKey, String consumerSecret) { - return new String(Base64.encodeBase64((consumerKey + ":" + consumerSecret).getBytes())); + return new String(Base64.encodeBase64( + (consumerKey + ":" + consumerSecret).getBytes(java.nio.charset.StandardCharsets.UTF_8)), + java.nio.charset.StandardCharsets.US_ASCII); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 485 - 488, The method getBase64EncodedString uses platform-default charset twice; change it to use an explicit charset (e.g., StandardCharsets.UTF_8) when converting the credential string to bytes and when creating the resulting String (or use Base64.encodeBase64String) so the Basic auth encoding is deterministic across environments—update the getBase64EncodedString implementation accordingly.
92-114: Unusedcontextfield.
contextis assigned in the constructor but never read elsewhere in the class. Either remove it or use it (e.g., as the source of the secondary-IS endpoints instead of the hard-coded values already flagged at lines 76–77).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 92 - 114, The private field context is assigned in the OAuth2TokenExchangeSubOrganizationTestCase constructor but never used; either remove the unused field declaration or replace the hard-coded secondary IS endpoint values with values derived from context (e.g., use AutomationContext.getConfiguration() or relevant context methods to obtain the secondary IS host/port and client config), and then update any references that currently use hard-coded secondary IS endpoints or credentials (such as secondaryISClientID/secondaryISClientSecret and accessTokenFromSecondaryIS initialization) to read from context instead; ensure you also remove the constructor assignment if you choose deletion to avoid dead code.
168-202: Cleanup aborts on the first failure, leaving orphaned resources.The teardown sequence (user → IDP → app → SP → org → client closes) rethrows on the first exception, so a failure early in cleanup leaves later artifacts (e.g., sub-organization) undeleted and can break subsequent runs. Consider isolating each cleanup step in its own try/catch that logs and continues, then rethrow at the end if any step failed. This also complements the null-guard concern already raised on this block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 168 - 202, The current endTest cleanup rethrows on first exception and aborts remaining cleanup; modify endTest so each cleanup action (super.deleteUser, idpMgtRestClient.deleteIdpInOrganization using subOrgIdpId, oAuth2RestClient.deleteApplication using subOrgAppId, super.deleteServiceProvider, orgMgtRestClient.deleteOrganization using subOrgId, and closing httpClient / orgMgtRestClient.closeHttpClient / oAuth2RestClient.closeHttpClient / idpMgtRestClient.closeHttpClient) is executed inside its own try/catch that logs failures and continues; aggregate any thrown exceptions (e.g., in a List<Exception> or suppressed on one primary exception) and after all steps, if any failures occurred rethrow a composed exception so callers still see overall failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 220-221: The test calls .toString() on
responseObject.get("access_token") before asserting it's non-null, which can
throw NPE and makes Assert.assertNotNull unreachable; change
OAuth2TokenExchangeSubOrganizationTestCase to first assert that
responseObject.get("access_token") is not null (use Assert.assertNotNull on the
raw value) and only then assign exchangedToken =
responseObject.get("access_token").toString(); apply the same fix for
accessTokenFromSecondaryIS by asserting the raw responseObject.get(...) is not
null before calling .toString().
- Around line 400-417: The if/else in createOIDCConfiguration is redundant
because both branches construct OauthAdminClient the same way; simplify by
removing the conditional and instantiate adminClient once using the
automationContextMap lookup for the given portOffset (keeping the same arguments
used in both branches) before calling registerOAuthApplicationData(appDTO) and
getAllOAuthApplicationData(); reference OauthAdminClient and adminClient to
locate and replace the duplicated logic.
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 504-518: Remove the excessive blank lines after the class closing
brace in OAuth2TokenExchangeSubOrganizationTestCase: trim the ~14 empty lines
following the final '}' to leave exactly one trailing newline so the file ends
with a single newline only.
- Around line 485-488: The method getBase64EncodedString uses platform-default
charset twice; change it to use an explicit charset (e.g.,
StandardCharsets.UTF_8) when converting the credential string to bytes and when
creating the resulting String (or use Base64.encodeBase64String) so the Basic
auth encoding is deterministic across environments—update the
getBase64EncodedString implementation accordingly.
- Around line 92-114: The private field context is assigned in the
OAuth2TokenExchangeSubOrganizationTestCase constructor but never used; either
remove the unused field declaration or replace the hard-coded secondary IS
endpoint values with values derived from context (e.g., use
AutomationContext.getConfiguration() or relevant context methods to obtain the
secondary IS host/port and client config), and then update any references that
currently use hard-coded secondary IS endpoints or credentials (such as
secondaryISClientID/secondaryISClientSecret and accessTokenFromSecondaryIS
initialization) to read from context instead; ensure you also remove the
constructor assignment if you choose deletion to avoid dead code.
- Around line 168-202: The current endTest cleanup rethrows on first exception
and aborts remaining cleanup; modify endTest so each cleanup action
(super.deleteUser, idpMgtRestClient.deleteIdpInOrganization using subOrgIdpId,
oAuth2RestClient.deleteApplication using subOrgAppId,
super.deleteServiceProvider, orgMgtRestClient.deleteOrganization using subOrgId,
and closing httpClient / orgMgtRestClient.closeHttpClient /
oAuth2RestClient.closeHttpClient / idpMgtRestClient.closeHttpClient) is executed
inside its own try/catch that logs failures and continues; aggregate any thrown
exceptions (e.g., in a List<Exception> or suppressed on one primary exception)
and after all steps, if any failures occurred rethrow a composed exception so
callers still see overall failure.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.java`:
- Around line 282-300: Consolidate duplicated header creation by extracting a
helper (e.g., buildHeaders or createHeaders) that takes the Authorization header
value as a parameter and returns the Header[]; replace getHeadersWithBearerToken
and getHeaders so they each call this helper with
BEARER_TOKEN_AUTHORIZATION_ATTRIBUTE + accessToken and with
BASIC_AUTHORIZATION_ATTRIBUTE + Base64.encodeBase64String((username + ":" +
password).getBytes()).trim() respectively, keeping the USER_AGENT_ATTRIBUTE and
CONTENT_TYPE_ATTRIBUTE construction inside the helper.
- Around line 273-280: The getSubOrgIdentityProviderPath method hardcodes
"api/server/v1/identity-providers" which duplicates the
IDENTITY_PROVIDER_BASE_PATH constant; change the method to reuse
IDENTITY_PROVIDER_BASE_PATH instead of the literal so the org-scoped path stays
in sync (compose serverUrl + ORGANIZATION_PATH [+ TENANT_PATH + tenantDomain +
PATH_SEPARATOR when not super tenant] + IDENTITY_PROVIDER_BASE_PATH), keeping
the same condition on MultitenantConstants.SUPER_TENANT_DOMAIN_NAME and existing
symbols tenantDomain, serverUrl, ORGANIZATION_PATH, TENANT_PATH, PATH_SEPARATOR.
🪄 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: 506b8091-57eb-4c02-ae47-6e3c3e0939fe
📒 Files selected for processing (3)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.javamodules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
✅ Files skipped from review due to trivial changes (1)
- modules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
Comment on lines
+220
to
+221
| String exchangedToken = responseObject.get("access_token").toString(); | ||
| Assert.assertNotNull(exchangedToken, "Exchanged access token is null."); |
Contributor
There was a problem hiding this comment.
assertNotNull after toString() is unreachable.
If responseObject.get("access_token") is null, the .toString() call on line 220 throws NullPointerException before the assertion on line 221 can run. Assert presence first, then extract.
The same pattern occurs at lines 454–455 for accessTokenFromSecondaryIS.
🐛 Proposed fix
- String exchangedToken = responseObject.get("access_token").toString();
- Assert.assertNotNull(exchangedToken, "Exchanged access token is null.");
+ Object exchangedTokenObj = responseObject.get("access_token");
+ Assert.assertNotNull(exchangedTokenObj, "Exchanged access token is null.");
+ String exchangedToken = exchangedTokenObj.toString();📝 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
| String exchangedToken = responseObject.get("access_token").toString(); | |
| Assert.assertNotNull(exchangedToken, "Exchanged access token is null."); | |
| Object exchangedTokenObj = responseObject.get("access_token"); | |
| Assert.assertNotNull(exchangedTokenObj, "Exchanged access token is null."); | |
| String exchangedToken = exchangedTokenObj.toString(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 220 - 221, The test calls .toString() on
responseObject.get("access_token") before asserting it's non-null, which can
throw NPE and makes Assert.assertNotNull unreachable; change
OAuth2TokenExchangeSubOrganizationTestCase to first assert that
responseObject.get("access_token") is not null (use Assert.assertNotNull on the
raw value) and only then assign exchangedToken =
responseObject.get("access_token").toString(); apply the same fix for
accessTokenFromSecondaryIS by asserting the raw responseObject.get(...) is not
null before calling .toString().
Comment on lines
+400
to
+417
| public OAuthConsumerAppDTO[] createOIDCConfiguration(int portOffset, OAuthConsumerAppDTO appDTO) | ||
| throws Exception { | ||
|
|
||
| if (portOffset == PORT_OFFSET_0) { | ||
| adminClient = new OauthAdminClient(automationContextMap.get(portOffset).getContextUrls() | ||
| .getBackEndUrl(), automationContextMap.get(portOffset).getContextTenant() | ||
| .getTenantAdmin().getUserName(), automationContextMap.get(portOffset).getContextTenant() | ||
| .getTenantAdmin().getPassword()); | ||
| } else { | ||
| adminClient = new OauthAdminClient(automationContextMap.get(portOffset).getContextUrls() | ||
| .getBackEndUrl(), automationContextMap.get(portOffset).getContextTenant() | ||
| .getTenantAdmin().getUserName(), automationContextMap.get(portOffset).getContextTenant() | ||
| .getTenantAdmin().getPassword()); | ||
| } | ||
|
|
||
| adminClient.registerOAuthApplicationData(appDTO); | ||
| return adminClient.getAllOAuthApplicationData(); | ||
| } |
Contributor
There was a problem hiding this comment.
Both branches of the if/else are identical — collapse them.
The portOffset == PORT_OFFSET_0 branch and the else branch construct OauthAdminClient with exactly the same arguments. The conditional is dead code.
♻️ Proposed refactor
public OAuthConsumerAppDTO[] createOIDCConfiguration(int portOffset, OAuthConsumerAppDTO appDTO)
throws Exception {
- if (portOffset == PORT_OFFSET_0) {
- adminClient = new OauthAdminClient(automationContextMap.get(portOffset).getContextUrls()
- .getBackEndUrl(), automationContextMap.get(portOffset).getContextTenant()
- .getTenantAdmin().getUserName(), automationContextMap.get(portOffset).getContextTenant()
- .getTenantAdmin().getPassword());
- } else {
- adminClient = new OauthAdminClient(automationContextMap.get(portOffset).getContextUrls()
- .getBackEndUrl(), automationContextMap.get(portOffset).getContextTenant()
- .getTenantAdmin().getUserName(), automationContextMap.get(portOffset).getContextTenant()
- .getTenantAdmin().getPassword());
- }
-
+ AutomationContext ctx = automationContextMap.get(portOffset);
+ adminClient = new OauthAdminClient(
+ ctx.getContextUrls().getBackEndUrl(),
+ ctx.getContextTenant().getTenantAdmin().getUserName(),
+ ctx.getContextTenant().getTenantAdmin().getPassword());
+
adminClient.registerOAuthApplicationData(appDTO);
return adminClient.getAllOAuthApplicationData();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 400 - 417, The if/else in createOIDCConfiguration is redundant
because both branches construct OauthAdminClient the same way; simplify by
removing the conditional and instantiate adminClient once using the
automationContextMap lookup for the given portOffset (keeping the same arguments
used in both branches) before calling registerOAuthApplicationData(appDTO) and
getAllOAuthApplicationData(); reference OauthAdminClient and adminClient to
locate and replace the duplicated logic.
ShanChathusanda93
force-pushed
the
sub-org-token-exchange-integration-tests-branch
branch
from
April 23, 2026 09:00
2cde44c to
8b374a9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java (1)
485-488: Specify an explicit charset when encoding Basic auth credentials.
getBytes()uses the platform default charset. UseStandardCharsets.UTF_8for deterministic behavior across environments.♻️ Proposed refactor
private String getBase64EncodedString(String consumerKey, String consumerSecret) { - return new String(Base64.encodeBase64((consumerKey + ":" + consumerSecret).getBytes())); + return new String(Base64.encodeBase64( + (consumerKey + ":" + consumerSecret).getBytes(java.nio.charset.StandardCharsets.UTF_8)), + java.nio.charset.StandardCharsets.UTF_8); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 485 - 488, The method getBase64EncodedString should use an explicit charset instead of platform default; update the call that builds the bytes for Base64 encoding in getBase64EncodedString(String consumerKey, String consumerSecret) to use StandardCharsets.UTF_8 when converting (consumerKey + ":" + consumerSecret) to bytes so the Basic auth credential encoding is deterministic across environments.modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.java (1)
273-280: ReuseIDENTITY_PROVIDER_BASE_PATHinstead of duplicating the path literal.The string
"api/server/v1/identity-providers"duplicates the existingIDENTITY_PROVIDER_BASE_PATHconstant (line 50). Reusing it keeps the base path in one place.♻️ Proposed refactor
private String getSubOrgIdentityProviderPath() { if (MultitenantConstants.SUPER_TENANT_DOMAIN_NAME.equals(tenantDomain)) { - return serverUrl + ORGANIZATION_PATH + "api/server/v1/identity-providers"; + return serverUrl + ORGANIZATION_PATH + IDENTITY_PROVIDER_BASE_PATH.substring(1); } - return serverUrl + TENANT_PATH + tenantDomain + PATH_SEPARATOR + ORGANIZATION_PATH + - "api/server/v1/identity-providers"; + return serverUrl + TENANT_PATH + tenantDomain + PATH_SEPARATOR + ORGANIZATION_PATH + + IDENTITY_PROVIDER_BASE_PATH.substring(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.java` around lines 273 - 280, The getSubOrgIdentityProviderPath method currently duplicates the literal "api/server/v1/identity-providers"; replace that literal with the existing constant IDENTITY_PROVIDER_BASE_PATH to avoid duplication. Update both return expressions in getSubOrgIdentityProviderPath to concatenate serverUrl + ORGANIZATION_PATH + IDENTITY_PROVIDER_BASE_PATH for the super tenant branch and serverUrl + TENANT_PATH + tenantDomain + PATH_SEPARATOR + ORGANIZATION_PATH + IDENTITY_PROVIDER_BASE_PATH for the tenant branch, ensuring the method uses the constant instead of the hard-coded string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 168-202: endTest() currently reuses the earlier obtained
switchedM2MToken which can expire before teardown; refresh the org-scoped
machine-to-machine token at the start of endTest() (before calling
idpMgtRestClient.deleteIdpInOrganization and
oAuth2RestClient.deleteOrganizationApplication) by invoking the same
token-acquisition routine used in `@BeforeClass` (or a helper like
obtainSwitchedM2MToken()) and replace switchedM2MToken, then proceed with
deletions and handle failures; ensure the refresh is guarded (null/exception
handling) so cleanup still attempts other steps if token refresh fails.
---
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 485-488: The method getBase64EncodedString should use an explicit
charset instead of platform default; update the call that builds the bytes for
Base64 encoding in getBase64EncodedString(String consumerKey, String
consumerSecret) to use StandardCharsets.UTF_8 when converting (consumerKey + ":"
+ consumerSecret) to bytes so the Basic auth credential encoding is
deterministic across environments.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.java`:
- Around line 273-280: The getSubOrgIdentityProviderPath method currently
duplicates the literal "api/server/v1/identity-providers"; replace that literal
with the existing constant IDENTITY_PROVIDER_BASE_PATH to avoid duplication.
Update both return expressions in getSubOrgIdentityProviderPath to concatenate
serverUrl + ORGANIZATION_PATH + IDENTITY_PROVIDER_BASE_PATH for the super tenant
branch and serverUrl + TENANT_PATH + tenantDomain + PATH_SEPARATOR +
ORGANIZATION_PATH + IDENTITY_PROVIDER_BASE_PATH for the tenant branch, ensuring
the method uses the constant instead of the hard-coded string.
🪄 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: 16ae13fa-3c7a-4688-8b03-f1f39cdc80e4
📒 Files selected for processing (4)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/OAuth2RestClient.javamodules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
✅ Files skipped from review due to trivial changes (1)
- modules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
Comment on lines
+168
to
+202
| @AfterClass(alwaysRun = true) | ||
| public void endTest() throws Exception { | ||
|
|
||
| try { | ||
| // Delete user from secondary IS | ||
| super.deleteUser(PORT_OFFSET_1, NEW_USER_USERNAME); | ||
|
|
||
| // Delete IDP from sub organization | ||
| if (subOrgIdpId != null) { | ||
| idpMgtRestClient.deleteIdpInOrganization(subOrgIdpId, switchedM2MToken); | ||
| } | ||
|
|
||
| // Delete application from sub organization | ||
| if (subOrgAppId != null) { | ||
| oAuth2RestClient.deleteOrganizationApplication(subOrgAppId, switchedM2MToken); | ||
| } | ||
|
|
||
| // Delete service provider from secondary IS | ||
| super.deleteServiceProvider(PORT_OFFSET_1, SECONDARY_IS_SP_NAME); | ||
|
|
||
| // Clean up sub organization | ||
| if (subOrgId != null) { | ||
| orgMgtRestClient.deleteOrganization(subOrgId); | ||
| } | ||
|
|
||
| // Close HTTP clients | ||
| httpClient.close(); | ||
| orgMgtRestClient.closeHttpClient(); | ||
| oAuth2RestClient.closeHttpClient(); | ||
| idpMgtRestClient.closeHttpClient(); | ||
| } catch (Exception e) { | ||
| log.error("Failure occurred during cleanup: " + e.getMessage(), e); | ||
| throw e; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Teardown depends on switchedM2MToken remaining valid.
The org-scoped IdP and application deletions reuse the switchedM2MToken obtained in @BeforeClass. If the token expires before @AfterClass runs (long-running suites, retries, or slow secondary IS setup), cleanup will fail and leak sub-org resources. Consider refreshing the switched M2M token at the start of endTest() before invoking the org-scoped deletions.
Note: the unconditional close() calls and lack of null guards on the rest clients are already covered by a prior review comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`
around lines 168 - 202, endTest() currently reuses the earlier obtained
switchedM2MToken which can expire before teardown; refresh the org-scoped
machine-to-machine token at the start of endTest() (before calling
idpMgtRestClient.deleteIdpInOrganization and
oAuth2RestClient.deleteOrganizationApplication) by invoking the same
token-acquisition routine used in `@BeforeClass` (or a helper like
obtainSwitchedM2MToken()) and replace switchedM2MToken, then proceed with
deletions and handle failures; ensure the refresh is guarded (null/exception
handling) so cleanup still attempts other steps if token refresh fails.
Contributor
|
PR builder started |
Contributor
|
PR builder completed |
ShanChathusanda93
force-pushed
the
sub-org-token-exchange-integration-tests-branch
branch
from
April 23, 2026 09:39
8b374a9 to
d30b300
Compare
Contributor
|
PR builder started |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java (4)
217-228:⚠️ Potential issue | 🟡 MinorValidate token responses before reading fields.
access_tokenis dereferenced before null checks, andsendPOSTMessage()parses the body without asserting a successful status. Assert the status/body first, then extract the token and assert both JWT subjects are present before comparing.Also applies to: 451-479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 217 - 228, The test reads responseObject.get("access_token") before asserting the POST succeeded and the body contains the token; update the test around sendPOSTMessage and subsequent checks so you first assert the HTTP response is successful and responseObject contains a non-null "access_token" entry, then extract exchangedToken, assert it is non-empty, and only then call getTokenSubject(exchangedToken) and getTokenSubject(accessTokenFromSecondaryIS) while asserting both subjects are non-null before comparing them (refer to sendPOSTMessage, responseObject, "access_token", getTokenSubject, and accessTokenFromSecondaryIS).
140-146:⚠️ Potential issue | 🟡 MinorPreserve the resource-load failure cause.
The current wrapper hides whether the JSON was missing, unreadable, or malformed.
Proposed fix
- } catch (Exception e) { - throw new RuntimeException("Could not load authorized APIs JSON."); + } catch (Exception e) { + throw new RuntimeException("Could not load authorized APIs JSON.", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 140 - 146, The catch block around RESTTestBase.readResource hides the underlying failure; update the exception handling in the OAuth2TokenExchangeSubOrganizationTestCase block that builds authorizedAPIs so that the caught Exception e is preserved when throwing the RuntimeException (e.g., pass e as the cause or include e.getMessage()), keeping the descriptive message "Could not load authorized APIs JSON." while attaching the original exception so callers can see whether the file was missing, unreadable, or malformed.
72-80:⚠️ Potential issue | 🟠 MajorAvoid static test resources and hard-coded secondary IS URLs.
Use per-run unique names for created org/app/SP/IdP/user resources, and derive the secondary token/JWKS URLs from the automation context for
PORT_OFFSET_1. This prevents collisions and keeps the test portable across runtime layouts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 72 - 80, The test uses static hard-coded constants (SUB_ORG_NAME, SUB_ORG_APP_NAME, TRUSTED_TOKEN_ISSUER_NAME, SECONDARY_IS_TOKEN_ENDPOINT, SECONDARY_IS_JWKS_URI, NEW_USER_USERNAME, NEW_USER_PASSWORD, NEW_USER_EMAIL) which cause collisions and non-portability; change these from fixed static finals to values created at runtime (e.g., append a UUID/timestamp in the test setup for SUB_ORG_NAME, SUB_ORG_APP_NAME, TRUSTED_TOKEN_ISSUER_NAME, NEW_USER_USERNAME) and compute SECONDARY_IS_TOKEN_ENDPOINT and SECONDARY_IS_JWKS_URI from the automation context/PORT_OFFSET_1 (derive host/port dynamically) instead of hard-coding https://localhost:9854/..., and ensure NEW_USER_PASSWORD/EMAIL are generated or parameterized per run so the test resources are unique and portable.
168-201:⚠️ Potential issue | 🟠 MajorMake teardown best-effort and null-safe.
A failure in an early cleanup step currently skips later deletions, and partial setup failures can still hit null clients. Isolate each cleanup action so the test attempts to remove all created resources and logs individual failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java` around lines 168 - 201, Make the teardown best-effort and null-safe by isolating each cleanup action into its own try/catch and checking for null clients/resources before calling them: wrap calls to super.deleteUser(PORT_OFFSET_1, NEW_USER_USERNAME), idpMgtRestClient.deleteIdpInOrganization(subOrgIdpId, switchedM2MToken), oAuth2RestClient.deleteOrganizationApplication(subOrgAppId, switchedM2MToken), super.deleteServiceProvider(PORT_OFFSET_1, SECONDARY_IS_SP_NAME) and orgMgtRestClient.deleteOrganization(subOrgId) each in their own try/catch (log failures with log.error including exception) and guard idpMgtRestClient, oAuth2RestClient, orgMgtRestClient, httpClient and idpMgtRestClient before calling closeHttpClient()/close() so null clients won’t be dereferenced; ensure the method continues attempting all cleanup steps even if earlier ones fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.java`:
- Around line 217-228: The test reads responseObject.get("access_token") before
asserting the POST succeeded and the body contains the token; update the test
around sendPOSTMessage and subsequent checks so you first assert the HTTP
response is successful and responseObject contains a non-null "access_token"
entry, then extract exchangedToken, assert it is non-empty, and only then call
getTokenSubject(exchangedToken) and getTokenSubject(accessTokenFromSecondaryIS)
while asserting both subjects are non-null before comparing them (refer to
sendPOSTMessage, responseObject, "access_token", getTokenSubject, and
accessTokenFromSecondaryIS).
- Around line 140-146: The catch block around RESTTestBase.readResource hides
the underlying failure; update the exception handling in the
OAuth2TokenExchangeSubOrganizationTestCase block that builds authorizedAPIs so
that the caught Exception e is preserved when throwing the RuntimeException
(e.g., pass e as the cause or include e.getMessage()), keeping the descriptive
message "Could not load authorized APIs JSON." while attaching the original
exception so callers can see whether the file was missing, unreadable, or
malformed.
- Around line 72-80: The test uses static hard-coded constants (SUB_ORG_NAME,
SUB_ORG_APP_NAME, TRUSTED_TOKEN_ISSUER_NAME, SECONDARY_IS_TOKEN_ENDPOINT,
SECONDARY_IS_JWKS_URI, NEW_USER_USERNAME, NEW_USER_PASSWORD, NEW_USER_EMAIL)
which cause collisions and non-portability; change these from fixed static
finals to values created at runtime (e.g., append a UUID/timestamp in the test
setup for SUB_ORG_NAME, SUB_ORG_APP_NAME, TRUSTED_TOKEN_ISSUER_NAME,
NEW_USER_USERNAME) and compute SECONDARY_IS_TOKEN_ENDPOINT and
SECONDARY_IS_JWKS_URI from the automation context/PORT_OFFSET_1 (derive
host/port dynamically) instead of hard-coding https://localhost:9854/..., and
ensure NEW_USER_PASSWORD/EMAIL are generated or parameterized per run so the
test resources are unique and portable.
- Around line 168-201: Make the teardown best-effort and null-safe by isolating
each cleanup action into its own try/catch and checking for null
clients/resources before calling them: wrap calls to
super.deleteUser(PORT_OFFSET_1, NEW_USER_USERNAME),
idpMgtRestClient.deleteIdpInOrganization(subOrgIdpId, switchedM2MToken),
oAuth2RestClient.deleteOrganizationApplication(subOrgAppId, switchedM2MToken),
super.deleteServiceProvider(PORT_OFFSET_1, SECONDARY_IS_SP_NAME) and
orgMgtRestClient.deleteOrganization(subOrgId) each in their own try/catch (log
failures with log.error including exception) and guard idpMgtRestClient,
oAuth2RestClient, orgMgtRestClient, httpClient and idpMgtRestClient before
calling closeHttpClient()/close() so null clients won’t be dereferenced; ensure
the method continues attempting all cleanup steps even if earlier ones fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 890f1e1a-35ed-4ab0-a5a5-055de2105575
📒 Files selected for processing (5)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/oauth2/OAuth2TokenExchangeSubOrganizationTestCase.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/IdpMgtRestClient.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/restclients/OAuth2RestClient.javamodules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.jsonmodules/integration/tests-integration/tests-backend/src/test/resources/testng.xml
✅ Files skipped from review due to trivial changes (1)
- modules/integration/tests-integration/tests-backend/src/test/resources/org/wso2/identity/integration/test/oauth2/organization-service-apis.json
Contributor
|
PR builder completed |
ShanChathusanda93
force-pushed
the
sub-org-token-exchange-integration-tests-branch
branch
from
April 23, 2026 11:14
d30b300 to
15936e0
Compare
Contributor
|
PR builder started |
Contributor
|
PR builder completed |
Contributor
|
PR builder started |
Contributor
|
PR builder completed |
jenkins-is-staging
approved these changes
Apr 24, 2026
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
Contributor
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/24876258511
Yasasr1
reviewed
Apr 24, 2026
| @DataProvider(name = "configProvider") | ||
| public static Object[][] configProvider() { | ||
|
|
||
| return new Object[][]{{TestUserMode.SUPER_TENANT_ADMIN}}; |
Contributor
There was a problem hiding this comment.
since this is for sub-orgs shall we test for another root tenant as well?
Yasasr1
reviewed
Apr 24, 2026
| } | ||
|
|
||
| @Test(groups = "wso2.is", description = "Exchange access token in sub organization") | ||
| public void testTokenExchangeInSubOrganization() throws Exception { |
Contributor
There was a problem hiding this comment.
shall we add a test for exchanging tokens for two apps of the same org as well?
ShanChathusanda93
force-pushed
the
sub-org-token-exchange-integration-tests-branch
branch
from
May 5, 2026 09:54
15936e0 to
c5396ba
Compare
|
ShanChathusanda93
marked this pull request as draft
June 24, 2026 06:49
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
3 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.
Changes in this PR