Integration v2 per block network isolation - #14285
Open
nimsara66 wants to merge 1 commit into
Open
Conversation
nimsara66
requested review from
AnuGayan,
Arshardh,
chamilaadhi and
tharindu1st
as code owners
August 10, 2026 02:29
Contributor
📝 WalkthroughWalkthroughThe change adds platform-gateway lifecycle tests, external-IdP console SSO tests, per-block container networks, dynamic Identity Server containers, gateway Compose resources, browser automation, provisioning utilities, and related Maven configuration. ChangesIntegration-v2 platform gateway and SSO
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
nimsara66
force-pushed
the
integration-v2-per-block-network-isolation_PR
branch
from
August 10, 2026 02:31
fd25156 to
10935cb
Compare
![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: 13
🧹 Nitpick comments (11)
all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/pom.xml (2)
133-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the Playwright version to a property.
Every other dependency in this POM uses a version property (for example
${cucumber.version}). A hardcoded1.49.0here is inconsistent and harder to bump together with any future Playwright-related plugin usage.♻️ Proposed change
<dependency> <groupId>com.microsoft.playwright</groupId> <artifactId>playwright</artifactId> - <version>1.49.0</version> + <version>${playwright.version}</version> <scope>test</scope> </dependency>Add the property in the
<properties>block:<playwright.version>1.49.0</playwright.version>🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/pom.xml` around lines 133 - 138, Add a playwright.version property with value 1.49.0 in the POM’s properties block, then update the com.microsoft.playwright dependency to reference ${playwright.version} instead of the hardcoded version.
159-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating the Chromium install behind the profile that runs the SSO tests.
The execution binds to
process-test-classesfor every build of this module. On a cold machine without the~/.cache/ms-playwrightcache, the step downloads a browser even when the external-IdP SSO suite is excluded bysurefire.excludedGroups/surefire.groups. Binding the execution inside the profile that enables the SSO block keeps unrelated builds offline-safe.🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/pom.xml` around lines 159 - 189, Move the install-playwright-chromium execution from the global build plugins section into the profile that enables the external-IdP SSO tests, so it runs only when that SSO profile is active. Preserve its process-test-classes phase and existing exec configuration.all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/SsoSteps.java (2)
145-151: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
baseUrlbefore dereferencing it.
TestContext.get("baseUrl").toString()throws aNullPointerExceptionif the block did not publishbaseUrl.SsoProvisioner.apimBase()handles the same key with an explicitIllegalStateExceptionthat names the cause. Align this step with that behavior so a boot-ordering fault is diagnosable.🛡️ Proposed fix
public void aRealBrowserSessionForTheConsoleSsoJourney() { + Object apimBase = TestContext.resolve("baseUrl"); Object gatewayBase = TestContext.get("baseGatewayUrl"); - this.browser = new PlaywrightSsoClient(TestContext.get("baseUrl").toString(), + this.browser = new PlaywrightSsoClient(apimBase.toString(), IntegrationActors.baseUrl(IntegrationActors.IS), gatewayBase == null ? null : gatewayBase.toString()); }🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/SsoSteps.java` around lines 145 - 151, Update aRealBrowserSessionForTheConsoleSsoJourney to validate TestContext.get("baseUrl") before converting it to a string, and throw an IllegalStateException with a clear missing-baseUrl message when absent. Preserve the existing PlaywrightSsoClient construction for valid baseUrl values.
189-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
TestContext.resolveforssoUserPasswordas well.Line 189 uses
TestContext.resolve("ssoUser")and line 190 usesTestContext.get("ssoUserPassword"). Both keys are written by the same background step. If the background step is skipped or renamed,getreturns null and the step fails with an opaqueNullPointerException.resolvefails fast with the missing key name.♻️ Proposed change
browser.authenticateAtExternalIs(TestContext.resolve("ssoUser").toString(), - TestContext.get("ssoUserPassword").toString()); + TestContext.resolve("ssoUserPassword").toString());Based on learnings: prefer
TestContext.resolve(key)overTestContext.get(key)for any key that must exist, so a missing key fails fast.🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/SsoSteps.java` around lines 189 - 190, Update the authentication call in SsoSteps to use TestContext.resolve("ssoUserPassword") instead of TestContext.get("ssoUserPassword"), matching the existing required-key handling for ssoUser and ensuring missing context values fail fast.Source: Learnings
all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java (1)
480-480: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the
updateApplicationresult.
appSoap("urn:updateApplication", updatePayload)discards the response. An Axis2 fault here leaves the console service provider without the multi-option step. The failure then surfaces much later as a browser assertion on the login page, which is harder to diagnose.updateConsoleCallbackToRegexalready applies a fault check at lines 541-545; apply the same check here.♻️ Proposed change
- appSoap("urn:updateApplication", updatePayload); + HttpResponse updateResp = appSoap("urn:updateApplication", updatePayload); + Assert.assertTrue(updateResp != null && updateResp.getData() != null + && !updateResp.getData().toLowerCase().contains("faultstring"), + "updateApplication failed for console SP '" + spName + "': " + + (updateResp == null ? "null" : updateResp.getResponseCode() + " " + updateResp.getData()));🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java` at line 480, Update the updateApplication call in SsoProvisioner to capture and assert the SOAP response for faults, matching the existing validation pattern in updateConsoleCallbackToRegex. Ensure any Axis2 fault fails immediately instead of discarding the result.all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/PlaywrightSsoClient.java (1)
888-895: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite debug screenshots under the build directory instead of
/tmp.
snapwrites to/tmp/snap-<label>.png. CI artifact collection normally archives the Maven build directory, so these screenshots are lost. The path also does not exist on Windows agents. A path undertarget/keeps the failure evidence with the rest of the build output.♻️ Proposed change
private void snap(String label) { try { page.screenshot(new Page.ScreenshotOptions() - .setPath(java.nio.file.Paths.get("/tmp", "snap-" + label + ".png")).setFullPage(true)); + .setPath(java.nio.file.Paths.get("target", "sso-snapshots", "snap-" + label + ".png")) + .setFullPage(true)); } catch (RuntimeException ignored) { } }🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/PlaywrightSsoClient.java` around lines 888 - 895, Update the PlaywrightSsoClient.snap method to write screenshots under the Maven build directory, such as target/, instead of the hard-coded /tmp path. Preserve the existing snap-<label>.png naming, full-page capture, and best-effort no-throw behavior.all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/docker-compose.yaml (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe header comment contradicts the file contents.
The comment states the file keeps only the two gateway services. The file also defines
echo-backend. Update the comment so a reader knows the third service is intentional.📝 Proposed comment fix
# Trimmed platform-gateway compose for the integration-v2 harness — derived from the wso2apip-api-gateway -# distribution's docker-compose.yaml, keeping ONLY the two gateway services (the distribution's observability -# services are all `profiles:`-gated and never start on a plain `up`, so they are simply omitted here). +# distribution's docker-compose.yaml, keeping the two gateway services plus a harness-only `echo-backend` +# (the distribution's observability services are all `profiles:`-gated and never start on a plain `up`, so +# they are simply omitted here).🤖 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 `@all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/docker-compose.yaml` around lines 1 - 3, Update the header comment in the compose file to state that the trimmed distribution intentionally includes the two gateway services and the echo-backend service defined as echo-backend.all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/listeners/BlockLifecycleListener.java (2)
504-513: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the derived control-plane port is always explicit.
bootPlatformGatewaybuildscontrolPlaneHostfromURI.create(apimBaseUrl).getPort().getServletHttpsUrlalways formats an explicit port, so the value is correct today. If any caller ever passes a URL without a port,getPort()returns-1and the gateway receiveshost.docker.internal:-1, which fails at connect time with an opaque error. Consider validating the port before use.🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/listeners/BlockLifecycleListener.java` around lines 504 - 513, Validate the port returned from URI.create(apimBaseUrl).getPort() in bootPlatformGateway before constructing controlPlaneHost, and fail immediately with a clear error when it is absent or invalid instead of passing host.docker.internal:-1. Preserve the existing explicit-port control-plane host behavior for valid URLs.
308-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe boot-failure path leaves stale teardown state in shared scope.
The catch block calls
teardownBlockNetworkAndIswith the local references and closes the network. It does not clearBLOCK_NETWORK_KEY,IS_CONTAINER_KEY, orBACKEND_ATTACHED_KEY. TestNG still callsonFinishfor the failed block, so lines 353-359 run the same teardown a second time on an already-closed network. Every guard tolerates this, so the outcome is only a misleadingNodeAppServer detach ... failedwarning and anetwork close() failedwarning on each boot failure. Clear the keys after teardown so the second pass no-ops.♻️ Proposed cleanup
teardownBlockNetworkAndIs(label, isContainer, blockNetwork, Boolean.TRUE.equals(TestContext.get(BACKEND_ATTACHED_KEY))); + // Already torn down here; drop the handles so onFinish does not repeat the teardown and log + // misleading detach/close warnings for an already-closed network. + TestContext.setShared(BLOCK_NETWORK_KEY, null); + TestContext.setShared(IS_CONTAINER_KEY, null); + TestContext.setShared(BACKEND_ATTACHED_KEY, Boolean.FALSE);🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/listeners/BlockLifecycleListener.java` around lines 308 - 311, Update the boot-failure catch path around teardownBlockNetworkAndIs to clear BLOCK_NETWORK_KEY, IS_CONTAINER_KEY, and BACKEND_ATTACHED_KEY after teardown completes, ensuring onFinish finds no stale shared state and does not repeat cleanup.all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicPlatformGatewayContainer.java (1)
126-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
distDirlifetime is split across the constructor andstop. The constructor creates the directory andstopdeletes it. Neither boundary matches the object's usable lifetime, which produces two separate effects: a restart afterstopcannot find its compose file, and an abnormal exit leaves the directory behind.
all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicPlatformGatewayContainer.java#L126-L131: separate the compose shutdown from the directory deletion, sostopdoes not make the instance unusable for a laterstart.all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicPlatformGatewayContainer.java#L184-L204: register a JVM shutdown hook for the created directory, so an abnormal exit does not leave it under$HOME/.wso2-pgw-testcontainers.🤖 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 `@all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicPlatformGatewayContainer.java` around lines 126 - 131, Update DynamicPlatformGatewayContainer.stop() so it only stops the compose environment and does not delete distDir, allowing the same instance to be started again. In the directory-creation logic around lines 184-204, register a JVM shutdown hook that deletes the created distDir during abnormal process termination; apply the requested changes at both specified locations in DynamicPlatformGatewayContainer.java.all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java (1)
169-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared gateway invocation helper.
Route this asserted gateway request through
APIInvocationSteps.execute(...). Extend that helper for the platform-gateway data-plane URL if needed. The directSimpleHTTPClientcall bypasses the sharedhttpResponseclear-and-publish flow.Based on learnings: asserted gateway calls must use
APIInvocationSteps.execute(...).🤖 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 `@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java` around lines 169 - 183, Update invokeAndAssert to route the asserted request through the shared APIInvocationSteps.execute(...) helper instead of calling SimpleHTTPClient directly, preserving the existing URL, headers, expected-status assertion, and timeout behavior. Extend APIInvocationSteps.execute(...) to support the platform-gateway data-plane URL when necessary, and rely on its shared httpResponse clear-and-publish flow rather than setting the response directly.Source: Learnings
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/listener-certs/default-listener.key`:
- Around line 1-52: Document the listener-certs key and certificate as
disposable, self-signed localhost test fixtures in a new short README,
explicitly prohibiting reuse outside the integration TestContainer harness. Add
the repository’s scanner allow-list entry for
listener-certs/default-listener.key, following the existing allow-list format
and conventions.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/runners/block/ExternalIdpConsoleSsoRunner.java`:
- Around line 22-29: Update the class Javadoc for ExternalIdpConsoleSsoRunner to
describe the federated login as running in a real headless Chromium browser
through PlaywrightSsoClient and ConnectProxy, rather than using a
browser-equivalent HTTP client. Preserve the existing scope and regression
context.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java`:
- Around line 69-75: The JSON parsing in PlatformGatewaySteps must guard
response bodies at both affected sites: in lines 69-75, assert that the response
status is 201 and the body is non-blank before constructing JSONObject,
including status and body in the assertion failure; in lines 189-203, return
false for blank bodies or invalid JSON so polling continues and the final
assertion reports the last response. Ensure every JSON response parse handles
whitespace-only bodies.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/SsoSteps.java`:
- Around line 34-43: Update the class Javadoc for SsoSteps to remove the claim
that the federated-login journey is a later addition or stub. Describe it as
fully implemented through PlaywrightSsoClient, including API creation,
deployment, subscription, and gateway invocation.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/ConnectProxy.java`:
- Around line 99-129: Update handle so the upstream socket is always closed when
tunnel setup or status writing fails, while remaining open during successful
bidirectional piping; track it outside the try block and close it in the
IOException path alongside client. Set a finite read timeout on client before
readRequestLineAndDrainHeaders to prevent idle peers from blocking pool threads
indefinitely.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/PlaywrightSsoClient.java`:
- Around line 177-183: Update pageOffersBothAuthenticators so the
federated-authenticator check only accepts idpName when it appears in either
OpenIDConnectAuthenticator%3A or OpenIDConnectAuthenticator: qualified form.
Remove the unqualified html.contains(idpName) alternative while preserving the
BasicAuthenticator requirement.
- Around line 838-853: Update the PlaywrightSsoClient constructor to set the
gatewayRouted flag when the localhost:8243 route is added, then make invokeApi
fail fast with an AssertionError naming the missing gateway base URL when that
flag is false. Before parsing the token response body, capture it, reject blank
or whitespace-only content with an assertion that includes the HTTP status and
response body, and parse only validated content; apply the same blank-body guard
to any intermediate response reads in this flow.
- Around line 255-291: Update assertLandedInConsole and waitForAccessTokenCookie
to derive the expected cookie path from consoleContext and pass it to
hasAccessTokenCookie instead of null. Make hasAccessTokenCookie require both the
AM_ACC_TOKEN name prefix and the expected path, using the paths associated with
consoleTokenPart1 for admin, publisher, and devportal.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/ResourceCleanup.java`:
- Around line 224-227: Update deleteRegisteredResources() so its finally block
removes CREATED_PLATFORM_GATEWAY_IDS from TestContext alongside the other
top-level CREATED_* registry removals, ensuring the platform-gateway list is
cleared after teardown.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java`:
- Around line 558-562: Make firstMatch return null immediately when its text
argument is null before invoking Pattern.matcher, preserving the existing regex
behavior for non-null text. In SsoProvisioner.java lines 435-441 and 499-507,
retain the existing call sites and assertions; they are corrected by this helper
change so null response bodies produce the existing assertion messages rather
than an exception.
- Around line 539-546: Update the assertion message in the consumer application
update flow to stop appending the full getOAuthApplicationData response from
body, which may contain oauthConsumerSecret. Retain the update failure context
and use only non-sensitive diagnostic fields or a redacted response in the
message, while preserving the existing assertion conditions.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/features/admin/external_idp_console_sso.feature`:
- Line 64: After the “change the API provider” step, add a follow-up that
re-reads the API and verifies its provider is “publisherUser.” Ensure the
scenario validates the persisted provider value rather than relying only on the
successful HTTP status.
- Line 1: Register the UI-created ssoApiId and ssoAppId with ResourceCleanup
after their creation, adding them to Constants.CREATED_API_IDS and
Constants.CREATED_APPLICATION_IDS respectively so the `@cleanup` hook removes both
resources.
---
Nitpick comments:
In
`@all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicPlatformGatewayContainer.java`:
- Around line 126-131: Update DynamicPlatformGatewayContainer.stop() so it only
stops the compose environment and does not delete distDir, allowing the same
instance to be started again. In the directory-creation logic around lines
184-204, register a JVM shutdown hook that deletes the created distDir during
abnormal process termination; apply the requested changes at both specified
locations in DynamicPlatformGatewayContainer.java.
In
`@all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/docker-compose.yaml`:
- Around line 1-3: Update the header comment in the compose file to state that
the trimmed distribution intentionally includes the two gateway services and the
echo-backend service defined as echo-backend.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/pom.xml`:
- Around line 133-138: Add a playwright.version property with value 1.49.0 in
the POM’s properties block, then update the com.microsoft.playwright dependency
to reference ${playwright.version} instead of the hardcoded version.
- Around line 159-189: Move the install-playwright-chromium execution from the
global build plugins section into the profile that enables the external-IdP SSO
tests, so it runs only when that SSO profile is active. Preserve its
process-test-classes phase and existing exec configuration.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java`:
- Around line 169-183: Update invokeAndAssert to route the asserted request
through the shared APIInvocationSteps.execute(...) helper instead of calling
SimpleHTTPClient directly, preserving the existing URL, headers, expected-status
assertion, and timeout behavior. Extend APIInvocationSteps.execute(...) to
support the platform-gateway data-plane URL when necessary, and rely on its
shared httpResponse clear-and-publish flow rather than setting the response
directly.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/SsoSteps.java`:
- Around line 145-151: Update aRealBrowserSessionForTheConsoleSsoJourney to
validate TestContext.get("baseUrl") before converting it to a string, and throw
an IllegalStateException with a clear missing-baseUrl message when absent.
Preserve the existing PlaywrightSsoClient construction for valid baseUrl values.
- Around line 189-190: Update the authentication call in SsoSteps to use
TestContext.resolve("ssoUserPassword") instead of
TestContext.get("ssoUserPassword"), matching the existing required-key handling
for ssoUser and ensuring missing context values fail fast.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/listeners/BlockLifecycleListener.java`:
- Around line 504-513: Validate the port returned from
URI.create(apimBaseUrl).getPort() in bootPlatformGateway before constructing
controlPlaneHost, and fail immediately with a clear error when it is absent or
invalid instead of passing host.docker.internal:-1. Preserve the existing
explicit-port control-plane host behavior for valid URLs.
- Around line 308-311: Update the boot-failure catch path around
teardownBlockNetworkAndIs to clear BLOCK_NETWORK_KEY, IS_CONTAINER_KEY, and
BACKEND_ATTACHED_KEY after teardown completes, ensuring onFinish finds no stale
shared state and does not repeat cleanup.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/PlaywrightSsoClient.java`:
- Around line 888-895: Update the PlaywrightSsoClient.snap method to write
screenshots under the Maven build directory, such as target/, instead of the
hard-coded /tmp path. Preserve the existing snap-<label>.png naming, full-page
capture, and best-effort no-throw behavior.
In
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java`:
- Line 480: Update the updateApplication call in SsoProvisioner to capture and
assert the SOAP response for faults, matching the existing validation pattern in
updateConsoleCallbackToRegex. Ensure any Axis2 fault fails immediately instead
of discarding the result.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bae962b-a272-4e7b-9c1b-ca8629ee2db9
📒 Files selected for processing (31)
all-in-one-apim/modules/integration-v2/docs/devs/capability-map.ymlall-in-one-apim/modules/integration-v2/docs/devs/is7-key-manager-integration-plan.mdall-in-one-apim/modules/integration-v2/docs/devs/platform-gateway-integration-plan.mdall-in-one-apim/modules/integration-v2/pom.xmlall-in-one-apim/modules/integration-v2/tests-common/testcontainers/pom.xmlall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/ContainerNetwork.javaall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicApimContainer.javaall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicISContainer.javaall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/DynamicPlatformGatewayContainer.javaall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/java/org/wso2/am/testcontainers/NodeAppServer.javaall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/config.tomlall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/docker-compose.yamlall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/listener-certs/default-listener.crtall-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/listener-certs/default-listener.keyall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/pom.xmlall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/runners/block/ExternalIdpConsoleSsoRunner.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/runners/block/PlatformGatewayRunner.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/SsoSteps.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/ConnectProxy.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/PlaywrightSsoClient.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/ResourceCleanup.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/ServerReadiness.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/TokenExchangeProvisioner.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/Utils.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/listeners/BlockLifecycleListener.javaall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/artifacts/payloads/create_platform_gateway_api.jsonall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/features/admin/external_idp_console_sso.featureall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/features/gateway/platform_gateway_lifecycle.featureall-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/testng-v2.xml
Comment on lines
+1
to
+52
| -----BEGIN PRIVATE KEY----- | ||
| MIIJRAIBADANBgkqhkiG9w0BAQEFAASCCS4wggkqAgEAAoICAQDCiHRl5eo6+Glx | ||
| j7WRBodNzD+WtvkUn4qFlxLvotzzYSgA9aY4mhHIWBmA8/o9MVcrE59mq22e+7G8 | ||
| ReKUKq9UyfAA/3D1LVnRj0wAONumZoAcQgH78vbzwUzML993WIZhhX/XHbKwD/hO | ||
| yIT5JCmiKGr9cFQNxS4IfNLP8VFvOPzLu+m5xNVTGXqilpCNJQ2L8PNRFtIjhIvg | ||
| 9uiayKHVdfn/0GpY8bZiROrQTWGUzlsTY2qnNFxAp/pmqYrwY6xBYTPwn+nEgZP9 | ||
| k5UYoV1DA8aoKYB9BQMM0qXa4XnEb1f/YL8NckiRb+eIyUKj2YHgpFw+DZSfTeH7 | ||
| yM81xNIcOp4RrKG3WamvsxJk204+JZJfjOSXJe+PzgSzI6WzGFyJDFqMBv1HKvUW | ||
| sKcbgIxN9Xa7NUvF7p/FJfKLJ3Opng0KhVbue+vCFacNw8PRrvfRk3UBWdXwywxL | ||
| fjIEKfPEAbP+5x2I/7IihxOnhPo5t/obbtH2Sf/8RK3GWA3U/aELE6ScOoNTZAz1 | ||
| 6dQO0D7Cr4OFKNbmZgmmzOFpChmBZZ1odVe7zjzRK6UirnuhRKxqBP8IAJj9X4OA | ||
| 0oPP7aGIJkaIGa7Vd5OXZ4nglwmY6pvci4mo+LD7r0yEZWDgdJW0HUpY3C14AwdZ | ||
| Rq/SZig6U5x1kB4FLNtFUWAh1bHTEQIDAQABAoICAA3BfOucsa64VKp3VUk7SzOM | ||
| 4x069IuJCZBPQcNIwrOCFgRVWorrmCDQ3ALSncoYdFfDNDUcH60LksKjtXUxPIM0 | ||
| IWDMr1Xi/lQzfBYS7zNJ0brNzxqcEGrlqxLQOs7TZ3M7b7IMM0/AXVhi1QZSEJCb | ||
| J4+i0wrDO/FVNEoIgC/reXLFp64fdU9CQLoBCBk2+sl/wRDlmnfw/jwq8YFKeSkh | ||
| RcuZVv/Rnco+odUXd7RAUE/jao4GxZQ73JjIWd8MSUi5QSU9hZgD6q1Wc4dRl8ju | ||
| mixXfPK6OUA9oAVFhukFT5VrJ69herCwF1EEnE3+SjESpkQsDSujo/x8M2zTrWC6 | ||
| WBjdkMNJ+OV5t+96FzFWKoRgCezmTByCE3GAPR+tHuVPHV/VJVgroWGYlPC68ixQ | ||
| S/bqNajGR0HiojmXSuWZYYndODtPMQuCQEF0LzUbFvw5fz40kfrkKsFt9W5uSJa1 | ||
| tkSHp+GOlbfPLvbIoF7oO0STcZRg7NgLiotMKuklqzJvVeoAbiLZP5nf2gsixb++ | ||
| c+5afjKtb5ejVwGiXRGCm2F1JAIMSiBWdOZmGoBxV2/nxv9ZainmMb2/Z3Xdb9+z | ||
| aoQNTl49jADH/YUruc0BFiVPtv+Xyu8Mw+r6IgybzjyGceYOPIqkttX2Ui9szBYy | ||
| PN+OGl2M1AH+2jQUz6ERAoIBAQD8AVvbaEj88Qtba4FSGEZPF7Hc7ePUugcqQQV0 | ||
| B7sLKCMN+ds5zdAlNUucLOBxLalZjFQPQiQrVWCzFZcz9KkjYLWcShMjMC4HYbOd | ||
| F8L5KpIMDILr9W98jucd3EtbvJiND59siaHdhfXpLJ5Tb8jA8Gp9TkKWevcLK6er | ||
| mcLGAEfwJ4EszjrS13vLSIySAa5QAqHEWHp+6RS4ieIM785qSHNZEcsLTcbsssKw | ||
| AhDgjV/eASUlXYm9vdexMyheR9atGYB0YFHVHaTg1RwveTenE2RQ5XO1Im4z6dKo | ||
| +nARTW3cPyiaUNSVihgJL2/40LkNk6sdgtvDVpTkI28qpSopAoIBAQDFnd9c/Y7b | ||
| w9hNy9ydmWDKGXrZTwUsAPgQYmiqkcd7Oglv362xbk/C6y+wgNelhQx2qv0QDDOT | ||
| F1c9As7iMUw6CKOyexMWwWw9BIeMbkEoAdkERKUp0uPuWUlvPmcNDQj/ECmDQWcW | ||
| R9QEEbYXqmyDMCZDag1pgx3CB/oz9ijqujyDy7HepfxqIPZKInD6CUpVDaONV3QV | ||
| vFFzjiz+sszWr4HrEb3B+KGPUqr0qUoiJIXBduJgvCG7OvtOhKmqLyn6pgXfwYXr | ||
| AZQCaksqNO4GFUjFY678IQFBfrAhkoWNl3GmSmXY5hriwMNkk4aRxcQf7ZiboHhh | ||
| 0qTOZRGgXM6pAoIBAQCDsg4YDlq+XRXLU9ZEWc08fiyEQYnj3MfrzAkWhwe3n+UZ | ||
| 464ueiFBoKV/22/7lZo/4vb29mDCiJ69WBYpn71YcKzYHVn89KvJTcS43vcUkau7 | ||
| QjqVJzF5DZE4aKy1J00twrFU3mRJyT0m+xtOjUeDlOCB1Yk07NP38XtxUSyZnwH5 | ||
| Phvil2/kSZo3NRXJI7m9tHJnkrmJYzNh8STCD/x2TKCDNqo+oQnJHu5hXuMFBrzH | ||
| 3x9TdJuMHg7/WrXI94/37DDWIJQDZLeKi7RsFtP/q0GsiQy7519iH9roVeCFGkrE | ||
| Y/lLE9zUvVMsUa8Zf7KFptsw0/cXEUNEKkRpK+FpAoIBAQCAobG50uZBkKpkgVVa | ||
| VHMXOhyoBPKl4P+Xkqb0mC2TvYRVCPEDtH1AsFv7dPJpsEAbolNVNcwAa8oCUaie | ||
| vV4TzHnZfJlBT6gH08Hzm/TXeNRtHc2werCY/g951NG5QgngFQIgiNr0Os770xeH | ||
| MgyuHBJcAVU3n/wnx4+KcSnTGGS5mUZvWfYCkvAd82s8pf13fYg/QXSzjUAripkp | ||
| NPN2rwlPHN46u39WToYu+V7i+IkEFEFonC0MtWjNhDb4KvkVSSo7qrlj7kQMimng | ||
| 0FE7LnGO4sMNJOKvYip9v+Uk9DYq+vRuieFoRfcpqYmHq4ZtDfgvHPYfHsDeBBXE | ||
| H4T5AoIBAQDkoaVC2Y6pxL9enJboXdXOit+OLxyiM+xjOUPBgKeHyDf/lVIE64w5 | ||
| QF4UKvp5ZbLCIZDOoKfhkyr3icyQ3u58IE+Ann07mE2iFATHblHx3oEm8dzZjYCn | ||
| W4huJg2GelF9hex9m6X9uQnVn92WNu3PpPZtLv5QzShMbXR7p1cvgaQItuZrQeET | ||
| fiSZFj/od8EdPrKLO44MNBN6tr9njb9Y3G4XC24LPjOxkjgR8uZCUrtrk6XRz5LJ | ||
| QK7Je+HOHsP2bqmvMcmlo2gBs6u47KMZGlp+MvijKAd1OhjoAszwLvQcueZIawNI | ||
| 3VTz5QSJqjO/p4nB5E/ME72lkrOfwnM7 | ||
| -----END PRIVATE KEY----- |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether this test key pair is referenced or duplicated outside the harness.
fd -H -t f 'default-listener' .
rg -n --iglob '!**/target/**' 'default-listener' -C2
# Confirm no other copy of the same private key material exists in the tree.
rg -n --iglob '!**/target/**' 'MIIJRAIBADANBgkqhkiG9w0BAQEFAASCCS4' -lRepository: wso2/product-apim
Length of output: 437
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
fd -H -t f 'default-listener|listener-certs|README' . | sed -n '1,120p'
echo
echo "== references to default-listener (no target) =="
rg -n --iglob '!**/target/**' 'default-listener' -C2 . | sed -n '1,200p'
echo
echo "== private key header occurrences (no target) =="
rg -n --iglob '!**/target/**' 'BEGIN (RSA )?PRIVATE KEY|BEGIN PRIVATE KEY' . | sed -n '1,240p'
echo
echo "== listener-certs contents =="
for f in all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/listener-certs/*; do
echo "--- $f"
wc -l "$f"
case "$f" in
*.key) sed -n '1,80p' "$f" ;;
*.crt|*.pem|*.txt) sed -n '1,80p' "$f" ;;
*) sed -n '1,80p' "$f" ;;
esac
done
echo
echo "== README/cleanup config near tests-common/testcontainers =="
fd -t f 'README|.*ignore|\.gitignore|trivy|bandit|pre-commit|gitleaks|secretlint|semgrep|spotbugs|checkstyle|sonar|detekt|osv-scanner' all-in-one-apim/modules/integration-v2/tests-common/testcontainers all-in-one-apim/modules/integration-v2 2>/dev/null | sed -n '1,160p'Repository: wso2/product-apim
Length of output: 10469
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
import re
root = Path('.')
key_pattern = re.compile(r'(-----BEGIN PRIVATE KEY-----[ \t\r\n]*[\r\n].*?-----END PRIVATE KEY-----)', re.S)
target_re = re.compile(r'.*(?:target/.*/.*)')
matches = []
seen = {}
for path in root.rglob('*'):
if not path.is_file():
continue
if path.relative_to(root).as_posix().startswith('all-in-one-apim/modules/integration-v2/tests-integration'):
continue
try:
text = path.read_text(errors='ignore')
except Exception:
continue
for i, key_block in enumerate(key_pattern.finditer(text), start=1):
body = key_block.group(1).strip()
if body not in seen:
seen[body] = (path, i)
matches.append((path.as_posix(), i, body in seen and seen[body][0] != path))
if i > 10:
break
key_path = Path('all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/listener-certs/default-listener.key')
key_body = key_path.read_text(errors='ignore').strip() if key_path.exists() else ''
print(f"default-listener.key exists: {key_path.exists()}")
print(f"default-listener.key occurrences of BEGIN PRIVATE KEY block: {sum(1 for m in re.finditer(key_pattern, key_path.read_text(errors='ignore')) if key_path.exists())}")
print(f"default-listener.key duplicate matches elsewhere: {sum(1 for _, _, dup in matches if dup)}")
print("Other files with same default-listener.key key material:")
for path, line, dup in matches:
if path == key_path.as_posix():
continue
if not dup:
continue
print(f"{path}:{line}")
print("Files with any private key block matching default-listener key material:")
for path, line, dup in matches:
if not key_path.exists():
break
if path == key_path.as_posix() or (not dup):
continue
print(f"{path}:{line}")
print("References to default-listener:")
for path in root.rglob('*'):
if not path.is_file():
continue
rel = path.relative_to(root).as_posix()
if 'target/' in rel:
continue
try:
text = path.read_text(errors='ignore')
except Exception:
continue
if 'default-listener' in text:
for n, line in enumerate(text.splitlines(), 1):
if 'default-listener' in line:
print(f"{rel}:{n}:{line.strip()}")
print("README/test-only marker candidates for listener-certs:")
for p in Path('all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/listener-certs').rglob('*'):
print(p)
PYRepository: wso2/product-apim
Length of output: 1404
Mark the listener key as a test-only fixture.
listener-certs/default-listener.key is an unencrypted private key used only by the integration TestContainer. Add a short listener-certs/README saying the self-signed localhost key/cert pair is a disposable test fixture and must not be reused outside the harness. Also add a scanner allow-list entry that covers listener-certs/default-listener.key.
🧰 Tools
🪛 Betterleaks (1.7.3)
[high] 1-52: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-common/testcontainers/src/main/resources/platform-gateway/listener-certs/default-listener.key`
around lines 1 - 52, Document the listener-certs key and certificate as
disposable, self-signed localhost test fixtures in a new short README,
explicitly prohibiting reuse outside the integration TestContainer harness. Add
the repository’s scanner allow-list entry for
listener-certs/default-listener.key, following the existing allow-list format
and conventions.
Source: Linters/SAST tools
Comment on lines
+22
to
+29
| /** | ||
| * Runner for the external-IdP console-SSO regression (#17744). Federates the Publisher/Admin/DevPortal consoles | ||
| * to an external WSO2 IS via a multi-option (BasicAuthenticator + OIDC IdP) login step, drives a federated login | ||
| * headlessly with a browser-equivalent HTTP client, then asserts cross-console single sign-on. The multi-option | ||
| * federated {@code /commonauth} request carries the OAuth2 scope list, so on a build whose Tomcat | ||
| * {@code maxHttpHeaderSize} is too small the login is rejected with 400 and never lands - failing this test. | ||
| * Requires the external Identity Server (block param {@code bootExternalIdentityServer=true}). | ||
| */ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the Javadoc: the login now runs in a real browser.
The Javadoc states the runner "drives a federated login headlessly with a browser-equivalent HTTP client". The shipped implementation uses PlaywrightSsoClient, a real headless Chromium reached through ConnectProxy. The feature file and PlaywrightSsoClient both state that a real browser is load-bearing for reproducing #17744. Correct the description so maintainers do not assume an HTTP-only walk.
📝 Proposed doc fix
- * to an external WSO2 IS via a multi-option (BasicAuthenticator + OIDC IdP) login step, drives a federated login
- * headlessly with a browser-equivalent HTTP client, then asserts cross-console single sign-on. The multi-option
+ * to an external WSO2 IS via a multi-option (BasicAuthenticator + OIDC IdP) login step, drives a federated login
+ * in a real headless browser (Playwright Chromium), then asserts cross-console single sign-on. The multi-option📝 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
| /** | |
| * Runner for the external-IdP console-SSO regression (#17744). Federates the Publisher/Admin/DevPortal consoles | |
| * to an external WSO2 IS via a multi-option (BasicAuthenticator + OIDC IdP) login step, drives a federated login | |
| * headlessly with a browser-equivalent HTTP client, then asserts cross-console single sign-on. The multi-option | |
| * federated {@code /commonauth} request carries the OAuth2 scope list, so on a build whose Tomcat | |
| * {@code maxHttpHeaderSize} is too small the login is rejected with 400 and never lands - failing this test. | |
| * Requires the external Identity Server (block param {@code bootExternalIdentityServer=true}). | |
| */ | |
| /** | |
| * Runner for the external-IdP console-SSO regression (`#17744`). Federates the Publisher/Admin/DevPortal consoles | |
| * to an external WSO2 IS via a multi-option (BasicAuthenticator + OIDC IdP) login step, drives a federated login | |
| * in a real headless browser (Playwright Chromium), then asserts cross-console single sign-on. The multi-option | |
| * federated {`@code` /commonauth} request carries the OAuth2 scope list, so on a build whose Tomcat | |
| * {`@code` maxHttpHeaderSize} is too small the login is rejected with 400 and never lands - failing this test. | |
| * Requires the external Identity Server (block param {`@code` bootExternalIdentityServer=true}). | |
| */ |
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/runners/block/ExternalIdpConsoleSsoRunner.java`
around lines 22 - 29, Update the class Javadoc for ExternalIdpConsoleSsoRunner
to describe the federated login as running in a real headless Chromium browser
through PlaywrightSsoClient and ConnectProxy, rather than using a
browser-equivalent HTTP client. Preserve the existing scope and regression
context.
Comment on lines
+69
to
+75
| if (resp != null && resp.getResponseCode() == 201 && resp.getData() != null) { | ||
| JSONObject body = new JSONObject(resp.getData()); | ||
| String id = body.getString("id"); | ||
| ResourceCleanup.register(ResourceCleanup.CREATED_PLATFORM_GATEWAY_IDS, id); | ||
| TestContext.set(GATEWAY_ID_KEY, id); | ||
| TestContext.set(GATEWAY_NAME_KEY, body.getString("name")); | ||
| TestContext.set(GATEWAY_TOKEN_KEY, body.getString("registrationToken")); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard response bodies before JSON parsing. Both paths can pass a blank or malformed body to JSONObject, which produces an opaque parsing failure instead of an assertion result.
all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java#L69-L75: before parsing, assert status 201 and a non-blank body. Include the status and body in the failure message.all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java#L189-L203: returnfalsefor blank or invalid JSON so polling continues and the final assertion reports the last response.
Based on learnings: guard every JSON response parse, including whitespace-only bodies.
📍 Affects 1 file
all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java#L69-L75(this comment)all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java#L189-L203
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/PlatformGatewaySteps.java`
around lines 69 - 75, The JSON parsing in PlatformGatewaySteps must guard
response bodies at both affected sites: in lines 69-75, assert that the response
status is 201 and the body is non-blank before constructing JSONObject,
including status and body in the assertion failure; in lines 189-203, return
false for blank bodies or invalid JSON so polling continues and the final
assertion reports the last response. Ensure every JSON response parse handles
whitespace-only bodies.
Source: Learnings
Comment on lines
+34
to
+43
| /** | ||
| * Provisioning steps for the external-IdP console-SSO block: registers the APIM-side OIDC identity provider | ||
| * against an IS OIDC app, wires the multi-option (local + federated) authentication step onto the Publisher, | ||
| * Admin and DevPortal console service providers, and creates the JIT-provisioned federated user on IS. These | ||
| * are the {@code Background} prerequisites; the federated-login journey itself is the browser-client layer | ||
| * (a later addition), stubbed here so the feature has no undefined step. | ||
| * | ||
| * <p>Per CLAUDE.md §14 these prerequisites drive SOAP admin services with no REST equivalent (IdP registration, | ||
| * console SP authentication-step editing) via {@link SsoProvisioner}, run super-tenant admin/admin. | ||
| */ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the class Javadoc: the browser steps are implemented, not stubbed.
The Javadoc states the federated-login journey is "a later addition, stubbed here so the feature has no undefined step". This class now implements the full journey through PlaywrightSsoClient, including API creation, deployment, subscription and gateway invocation. Correct the description so it matches the shipped steps.
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/stepdefinitions/SsoSteps.java`
around lines 34 - 43, Update the class Javadoc for SsoSteps to remove the claim
that the federated-login journey is a later addition or stub. Describe it as
fully implemented through PlaywrightSsoClient, including API creation,
deployment, subscription, and gateway invocation.
Comment on lines
+99
to
+129
| private void handle(Socket client) { | ||
| try { | ||
| client.setTcpNoDelay(true); | ||
| InputStream in = client.getInputStream(); | ||
| String requestLine = readRequestLineAndDrainHeaders(in); | ||
| if (requestLine == null || !requestLine.toUpperCase().startsWith("CONNECT ")) { | ||
| writeStatus(client, "400 Bad Request"); | ||
| client.close(); | ||
| return; | ||
| } | ||
| // "CONNECT host:port HTTP/1.1" | ||
| String target = requestLine.split("\\s+")[1]; | ||
| InetSocketAddress mapped = routes.get(target); | ||
| if (mapped == null) { | ||
| logger.warn("CONNECT proxy: no route for '" + target + "' — refusing (502)"); | ||
| writeStatus(client, "502 Bad Gateway"); | ||
| client.close(); | ||
| return; | ||
| } | ||
| Socket upstream = new Socket(); | ||
| upstream.setTcpNoDelay(true); | ||
| upstream.connect(mapped, 15000); | ||
| writeStatus(client, "200 Connection Established"); | ||
| // Raw bidirectional tunnel: TLS flows end-to-end, the proxy only shuffles bytes. | ||
| pool.submit(() -> pipe(client, upstream)); | ||
| pipe(upstream, client); | ||
| } catch (IOException e) { | ||
| logger.warn("CONNECT proxy tunnel failed: " + e.getMessage()); | ||
| closeQuietly(client); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close upstream on failure and bound the request-line read.
Two socket-lifecycle gaps exist in handle:
- The
catch (IOException)block closes onlyclient. Ifupstream.connect(mapped, 15000)or the followingwriteStatusthrows, theupstreamsocket is never closed. Each failed tunnel then leaks a socket for the lifetime of the test JVM. - No read timeout is set on
clientbeforereadRequestLineAndDrainHeaders. A peer that connects and sends nothing blocks a pool thread forever. The pool is an unbounded cached pool, so such peers accumulate threads.
🔒 Proposed fix
private void handle(Socket client) {
+ Socket upstream = null;
try {
client.setTcpNoDelay(true);
+ client.setSoTimeout(20000);
InputStream in = client.getInputStream();
String requestLine = readRequestLineAndDrainHeaders(in);
if (requestLine == null || !requestLine.toUpperCase().startsWith("CONNECT ")) {
writeStatus(client, "400 Bad Request");
client.close();
return;
}
// "CONNECT host:port HTTP/1.1"
String target = requestLine.split("\\s+")[1];
InetSocketAddress mapped = routes.get(target);
if (mapped == null) {
logger.warn("CONNECT proxy: no route for '" + target + "' — refusing (502)");
writeStatus(client, "502 Bad Gateway");
client.close();
return;
}
- Socket upstream = new Socket();
+ upstream = new Socket();
upstream.setTcpNoDelay(true);
upstream.connect(mapped, 15000);
writeStatus(client, "200 Connection Established");
+ // The tunnel reads must not inherit the handshake read timeout.
+ client.setSoTimeout(0);
// Raw bidirectional tunnel: TLS flows end-to-end, the proxy only shuffles bytes.
- pool.submit(() -> pipe(client, upstream));
+ Socket tunnelUpstream = upstream;
+ pool.submit(() -> pipe(client, tunnelUpstream));
pipe(upstream, client);
} catch (IOException e) {
logger.warn("CONNECT proxy tunnel failed: " + e.getMessage());
closeQuietly(client);
+ if (upstream != null) {
+ closeQuietly(upstream);
+ }
}
}📝 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
| private void handle(Socket client) { | |
| try { | |
| client.setTcpNoDelay(true); | |
| InputStream in = client.getInputStream(); | |
| String requestLine = readRequestLineAndDrainHeaders(in); | |
| if (requestLine == null || !requestLine.toUpperCase().startsWith("CONNECT ")) { | |
| writeStatus(client, "400 Bad Request"); | |
| client.close(); | |
| return; | |
| } | |
| // "CONNECT host:port HTTP/1.1" | |
| String target = requestLine.split("\\s+")[1]; | |
| InetSocketAddress mapped = routes.get(target); | |
| if (mapped == null) { | |
| logger.warn("CONNECT proxy: no route for '" + target + "' — refusing (502)"); | |
| writeStatus(client, "502 Bad Gateway"); | |
| client.close(); | |
| return; | |
| } | |
| Socket upstream = new Socket(); | |
| upstream.setTcpNoDelay(true); | |
| upstream.connect(mapped, 15000); | |
| writeStatus(client, "200 Connection Established"); | |
| // Raw bidirectional tunnel: TLS flows end-to-end, the proxy only shuffles bytes. | |
| pool.submit(() -> pipe(client, upstream)); | |
| pipe(upstream, client); | |
| } catch (IOException e) { | |
| logger.warn("CONNECT proxy tunnel failed: " + e.getMessage()); | |
| closeQuietly(client); | |
| } | |
| } | |
| private void handle(Socket client) { | |
| Socket upstream = null; | |
| try { | |
| client.setTcpNoDelay(true); | |
| client.setSoTimeout(20000); | |
| InputStream in = client.getInputStream(); | |
| String requestLine = readRequestLineAndDrainHeaders(in); | |
| if (requestLine == null || !requestLine.toUpperCase().startsWith("CONNECT ")) { | |
| writeStatus(client, "400 Bad Request"); | |
| client.close(); | |
| return; | |
| } | |
| // "CONNECT host:port HTTP/1.1" | |
| String target = requestLine.split("\\s+")[1]; | |
| InetSocketAddress mapped = routes.get(target); | |
| if (mapped == null) { | |
| logger.warn("CONNECT proxy: no route for '" + target + "' — refusing (502)"); | |
| writeStatus(client, "502 Bad Gateway"); | |
| client.close(); | |
| return; | |
| } | |
| upstream = new Socket(); | |
| upstream.setTcpNoDelay(true); | |
| upstream.connect(mapped, 15000); | |
| writeStatus(client, "200 Connection Established"); | |
| // The tunnel reads must not inherit the handshake read timeout. | |
| client.setSoTimeout(0); | |
| // Raw bidirectional tunnel: TLS flows end-to-end, the proxy only shuffles bytes. | |
| Socket tunnelUpstream = upstream; | |
| pool.submit(() -> pipe(client, tunnelUpstream)); | |
| pipe(upstream, client); | |
| } catch (IOException e) { | |
| logger.warn("CONNECT proxy tunnel failed: " + e.getMessage()); | |
| closeQuietly(client); | |
| if (upstream != null) { | |
| closeQuietly(upstream); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 117-117: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information
(unencrypted-socket-java)
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/ConnectProxy.java`
around lines 99 - 129, Update handle so the upstream socket is always closed
when tunnel setup or status writing fails, while remaining open during
successful bidirectional piping; track it outside the try block and close it in
the IOException path alongside client. Set a finite read timeout on client
before readRequestLineAndDrainHeaders to prevent idle peers from blocking pool
threads indefinitely.
Comment on lines
+224
to
+227
| // Platform gateway environments (admin) AFTER the APIs — deleting an API undeploys it from the | ||
| // gateway, so the gateway delete is unblocked. Deleted with the admin token. | ||
| deleteResources(CREATED_PLATFORM_GATEWAY_IDS, Identity::adminTokenKey, | ||
| id -> Utils.getPlatformGatewayByIdURL(baseUrl, id)); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clear the platform-gateway registry in finally.
deleteRegisteredResources() deletes CREATED_PLATFORM_GATEWAY_IDS, but the finally block does not remove that list from TestContext. Add TestContext.remove(CREATED_PLATFORM_GATEWAY_IDS) with the other registry removals.
Based on learnings: the teardown pattern must clear each new top-level CREATED_* list in finally.
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/ResourceCleanup.java`
around lines 224 - 227, Update deleteRegisteredResources() so its finally block
removes CREATED_PLATFORM_GATEWAY_IDS from TestContext alongside the other
top-level CREATED_* registry removals, ensuring the platform-gateway list is
cleared after teardown.
Source: Learnings
Comment on lines
+539
to
+546
| HttpResponse updResp = oauthSoap("urn:updateConsumerApplication", updatePayload); | ||
| String updBody = updResp == null ? "null" : updResp.getResponseCode() + " " + updResp.getData(); | ||
| Assert.assertTrue(updResp != null && updResp.getData() != null | ||
| && !updResp.getData().toLowerCase().contains("faultstring") | ||
| && !updResp.getData().toLowerCase().contains("exception"), | ||
| "updateConsumerApplication failed for consumerKey '" + consumerKey + "': " + updBody | ||
| + "\n---- getOAuthApplicationData response was ----\n" + body); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
The failure message prints the console OAuth client secret.
body holds the full getOAuthApplicationData response, which contains oauthConsumerSecret. The assertion message appends that body, so a failing run writes the console client secret into the test report and CI logs. The container is disposable, but the log output is not. Print only the fields needed for diagnosis.
🔒 Proposed change
HttpResponse updResp = oauthSoap("urn:updateConsumerApplication", updatePayload);
String updBody = updResp == null ? "null" : updResp.getResponseCode() + " " + updResp.getData();
Assert.assertTrue(updResp != null && updResp.getData() != null
&& !updResp.getData().toLowerCase().contains("faultstring")
&& !updResp.getData().toLowerCase().contains("exception"),
"updateConsumerApplication failed for consumerKey '" + consumerKey + "': " + updBody
- + "\n---- getOAuthApplicationData response was ----\n" + body);
+ + "\n---- read OAuth app: name=" + appName + " version=" + version
+ + " grants=" + grants + " ----");📝 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
| HttpResponse updResp = oauthSoap("urn:updateConsumerApplication", updatePayload); | |
| String updBody = updResp == null ? "null" : updResp.getResponseCode() + " " + updResp.getData(); | |
| Assert.assertTrue(updResp != null && updResp.getData() != null | |
| && !updResp.getData().toLowerCase().contains("faultstring") | |
| && !updResp.getData().toLowerCase().contains("exception"), | |
| "updateConsumerApplication failed for consumerKey '" + consumerKey + "': " + updBody | |
| + "\n---- getOAuthApplicationData response was ----\n" + body); | |
| } | |
| HttpResponse updResp = oauthSoap("urn:updateConsumerApplication", updatePayload); | |
| String updBody = updResp == null ? "null" : updResp.getResponseCode() + " " + updResp.getData(); | |
| Assert.assertTrue(updResp != null && updResp.getData() != null | |
| && !updResp.getData().toLowerCase().contains("faultstring") | |
| && !updResp.getData().toLowerCase().contains("exception"), | |
| "updateConsumerApplication failed for consumerKey '" + consumerKey + "': " + updBody | |
| "\n---- read OAuth app: name=" + appName + " version=" + version | |
| " grants=" + grants + " ----"); | |
| } |
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java`
around lines 539 - 546, Update the assertion message in the consumer application
update flow to stop appending the full getOAuthApplicationData response from
body, which may contain oauthConsumerSecret. Retain the update failure context
and use only non-sensitive diagnostic fields or a redacted response in the
message, while preserving the existing assertion conditions.
Comment on lines
+558
to
+562
| /** First regex-group-1 match in the text, or null. */ | ||
| private static String firstMatch(String text, String regex) { | ||
| java.util.regex.Matcher m = java.util.regex.Pattern.compile(regex).matcher(text); | ||
| return m.find() ? m.group(1) : null; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
firstMatch throws NullPointerException on a null response body. Pattern.matcher(text) rejects a null argument. HttpResponse.getData() can return null, so both call sites can pass null and fail with an opaque NullPointerException instead of the assertion message that follows.
.../utils/SsoProvisioner.java#L558-L562: makefirstMatchnull-safe by returningnullwhentextis null..../utils/SsoProvisioner.java#L435-L441:Utils.retryUntilreturns the last result on timeout, and that result can carry a null body even though the accept predicate required a non-null body. With the helper fixed,appIdandinboundAuthKeystay null and the existingAssert.assertNotNullmessages report the real cause..../utils/SsoProvisioner.java#L499-L507:bodyis set togetResp.getData()whengetRespis non-null, so it can be null. With the helper fixed, theAssert.assertNotNull(appName, ...)message reports the real cause.
🛡️ Proposed fix
/** First regex-group-1 match in the text, or null. */
private static String firstMatch(String text, String regex) {
+ if (text == null) {
+ return null;
+ }
java.util.regex.Matcher m = java.util.regex.Pattern.compile(regex).matcher(text);
return m.find() ? m.group(1) : null;
}Based on learnings: HttpResponse.getData() may legitimately return null, so any code that reads or parses the response body must keep a null guard.
📝 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
| /** First regex-group-1 match in the text, or null. */ | |
| private static String firstMatch(String text, String regex) { | |
| java.util.regex.Matcher m = java.util.regex.Pattern.compile(regex).matcher(text); | |
| return m.find() ? m.group(1) : null; | |
| } | |
| /** First regex-group-1 match in the text, or null. */ | |
| private static String firstMatch(String text, String regex) { | |
| if (text == null) { | |
| return null; | |
| } | |
| java.util.regex.Matcher m = java.util.regex.Pattern.compile(regex).matcher(text); | |
| return m.find() ? m.group(1) : null; | |
| } |
📍 Affects 1 file
all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java#L558-L562(this comment)all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java#L435-L441all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java#L499-L507
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java/org/wso2/am/integration/cucumbertests/utils/SsoProvisioner.java`
around lines 558 - 562, Make firstMatch return null immediately when its text
argument is null before invoking Pattern.matcher, preserving the existing regex
behavior for non-null text. In SsoProvisioner.java lines 435-441 and 499-507,
retain the existing call sites and assertions; they are corrected by this helper
change so null response bodies produce the existing assertion messages rather
than an exception.
Source: Learnings
| @@ -0,0 +1,75 @@ | |||
| @cleanup | |||
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether ResourceCleanup sweeps the UI-created API and application ids.
set -euo pipefail
fd -t f 'ResourceCleanup.java' | while IFS= read -r f; do
rg -n -C 6 'CREATED_.*_IDS|register\(|deleteRegisteredResources' "$f"
done
# Confirm whether the SSO steps register anything for cleanup.
rg -n -C 4 'ssoApiId|ssoAppId|ResourceCleanup' --glob '*SsoSteps.java' --glob '*PlaywrightSsoClient.java'Repository: wso2/product-apim
Length of output: 23221
🏁 Script executed:
#!/bin/bash
# Inspect the relevant test resources and step definitions.
set -euo pipefail
printf '--- Candidate files ---\n'
fd -t f 'external_idp_console_sso.feature|PlaywrightSsoClient.java|SsoSteps.java|ResourceCleanup.java|ISResourceCleanup.java' .
printf '\n--- Feature header/outline ---\n'
f="$(fd -t f 'external_idp_console_sso.feature' . | head -n 1 || true)"
if [ -n "$f" ]; then
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
fi
printf '\n--- SsoSteps outline ---\n'
sf="$(fd -t f 'SsoSteps.java' . | head -n 1 || true)"
if [ -n "$sf" ]; then
ast-grep outline "$sf" --view expanded || true
sed -n '1,240p' "$sf" | cat -n
fi
printf '\n--- PlaywrightSsoClient outline ---\n'
pcf="$(fd -t f 'PlaywrightSsoClient.java' . | head -n 1 || true)"
if [ -n "$pcf" ]; then
ast-grep outline "$pcf" --view expanded || true
sed -n '1,240p' "$pcf" | cat -n
fi
printf '\n--- ResourceCleanup constants/signature slice ---\n'
rf="$(fd -t f 'ResourceCleanup.java' . | head -n 1 || true)"
if [ -n "$rf" ]; then
sed -n '30,160p' "$rf" | cat -n
fiRepository: wso2/product-apim
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Candidate files ---'
fd -t f 'external_idp_console_sso.feature|PlaywrightSsoClient.java|SsoSteps.java|ResourceCleanup.java|ISResourceCleanup.java' .
printf '%s\n' '--- Feature header/outline ---'
f="$(fd -t f 'external_idp_console_sso.feature' . | head -n 1 || true)"
if [ -n "$f" ]; then
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
fi
printf '%s\n' '--- SsoSteps outline ---'
sf="$(fd -t f 'SsoSteps.java' . | head -n 1 || true)"
if [ -n "$sf" ]; then
ast-grep outline "$sf" --view expanded || true
sed -n '1,240p' "$sf" | cat -n
fi
printf '%s\n' '--- PlaywrightSsoClient outline ---'
pcf="$(fd -t f 'PlaywrightSsoClient.java' . | head -n 1 || true)"
if [ -n "$pcf" ]; then
ast-grep outline "$pcf" --view expanded || true
sed -n '1,260p' "$pcf" | cat -n
fi
printf '%s\n' '--- ResourceCleanup constants/signature slice ---'
rf="$(fd -t f 'ResourceCleanup.java' . | head -n 1 || true)"
if [ -n "$rf" ]; then
sed -n '30,160p' "$rf" | cat -n
fi
printf '%s\n' '--- Cleanup sweep check ---'
pf="$(fd -t f 'PlaywrightSsoClient.java|SsoSteps.java' . | tr '\n' ' ')"
if [ -n "$pf" ]; then
rg -n -C 4 'ssoApiId|ssoAppId|ResourceCleanup|register\(' $pf || true
else
echo 'no PlaywrightSsoClient.java or SsoSteps.java found'
fiRepository: wso2/product-apim
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Read-only cleanup-registration invariant for file under review ---'
python3 - <<'PY'
from pathlib import Path
import re
f = Path('all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/features/admin/external_idp_console_sso.feature')
text = f.read_text()
needles = ['`@cleanup`', 'ssoApiId', 'ssoAppId']
found = {n: text.find(n) for n in needles}
rgx = re.compile(r'\bResourceCleanup\s*\.\s*register\s*\(')
call_count = len(rgx.findall(text))
print('feature_has_cleanup=' + str(found['`@cleanup`'] >= 0))
print('feature_mentions_ssoApiId=' + str(found['ssoApiId'] >= 0))
print('feature_mentions_ssoAppId=' + str(found['ssoAppId'] >= 0))
print('feature_resourceCleanup_register_call_count=' + str(call_count))
PY
printf '%s\n' '--- References to cleanup hook and SSO ids under cucumber code ---'
rg -n '(`@cleanup`|deleteRegisteredResources|ssoApiId|ssoAppId|CREATED_API_IDS|CREATED_APPLICATION_IDS)' \
all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/java \
all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/features/admin/external_idp_console_sso.feature || trueRepository: wso2/product-apim
Length of output: 21814
Register the UI-created API and application for @cleanup teardown.
external_idp_console_sso.feature stores the UI-created ids in ssoApiId and ssoAppId, but only ResourceCleanup.register(...) entries in Constants.CREATED_API_IDS and Constants.CREATED_APPLICATION_IDS are swept by the per-scenario @cleanup hook. Add registration for these ids after creation so the @cleanup block does not leak them onto the shared APIM container.
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/features/admin/external_idp_console_sso.feature`
at line 1, Register the UI-created ssoApiId and ssoAppId with ResourceCleanup
after their creation, adding them to Constants.CREATED_API_IDS and
Constants.CREATED_APPLICATION_IDS respectively so the `@cleanup` hook removes both
resources.
| When I open the "admin" console | ||
| Then I should NOT be prompted to log in again | ||
| And I should land authenticated in the "admin" console | ||
| And I change the API provider to actor "publisherUser" using the admin SSO session |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the observable effect of the provider change.
The step asserts only HTTP 200 from change-provider. Add a follow-up step that re-reads the API and asserts the provider now equals the target actor. Without it, a silent no-op that still answers 200 passes the scenario.
Based on learnings: after an update step, assert the response contains the new value or otherwise reflects the updated state; do not rely only on a 200 status.
🤖 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
`@all-in-one-apim/modules/integration-v2/tests-integration/cucumber-tests/src/test/resources/features/admin/external_idp_console_sso.feature`
at line 64, After the “change the API provider” step, add a follow-up that
re-reads the API and verifies its provider is “publisherUser.” Ensure the
scenario validates the persisted provider value rather than relying only on the
successful HTTP status.
Source: Learnings
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #14285 +/- ##
=============================================
- Coverage 52.64% 11.77% -40.87%
- Complexity 131 895 +764
=============================================
Files 25 365 +340
Lines 454 17875 +17421
Branches 11 1913 +1902
=============================================
+ Hits 239 2105 +1866
- Misses 209 15736 +15527
- Partials 6 34 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Aug 10, 2026
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
1 participant
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.
This pull request introduces integration test support for the WSO2 API Platform Gateway (external data-plane gateway), expands the capability map, and improves concurrency isolation for external system integration blocks. The main changes include the addition of new documentation and configuration for the platform gateway, enhancements to the IS7 key manager integration plan, and updates to the test infrastructure to support per-block network isolation and image pre-pulling.
Platform Gateway Integration Support:
platform-gateway-integration-plan.md).pom.xmlfiles to define properties for the platform gateway controller/runtime images and backend echo service, and to ensure Docker images are pulled before integration tests run. [1] [2]Capability Map Expansion:
platform-gatewayandsso(console SSO) entries to the capability map to reflect new and upcoming features. [1] [2]IS7 Key Manager Integration Improvements:
DynamicISContainerand private Docker networks for better concurrency and isolation, removing alias collisions and the need for JVM-wide serialization. Updated documentation to reflect these changes and the impact on SSO test lanes. [1] [2] [3]Test Infrastructure Enhancements:
Other Configuration Updates:
Let me know if you'd like a walkthrough of any of these areas or details on how the new platform gateway tests are structured.