Add missing scope validation test for DirectBackend MCP server subtype#13983
Open
Thisun-17 wants to merge 1 commit into
Open
Add missing scope validation test for DirectBackend MCP server subtype#13983Thisun-17 wants to merge 1 commit into
Thisun-17 wants to merge 1 commit into
Conversation
Thisun-17
requested review from
AnuGayan,
Arshardh,
chamilaadhi and
tharindu1st
as code owners
Contributor
WalkthroughAdds a third scope and application plus a new integration test to MCPServerTestCase that updates MCP scopes, deploys a revision, creates/subscribes a new application, generates a scoped access token, invokes direct-backend endpoints to validate scope enforcement, and extends cleanup to remove the new app/subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (MCPServerTestCase)
participant MCP as MCP Server (config)
participant Gateway as API Gateway / Revision
participant KM as KeyManager (Token generation)
participant Backend as Direct Backend
Test->>MCP: GET current MCP configuration
Test->>MCP: PATCH add new Scope (SCOPE_3) bound to role
Test->>Gateway: POST deploy new revision with updated scopes
Test->>KM: POST create application & subscribe (SCOPES_APP_NAME_3)
Test->>KM: POST generate access token scoped to SCOPE_3
Test->>Backend: GET direct-backend endpoint with token (SCOPE_3)
Backend-->>Test: 200 OK (authorized)
Test->>Backend: GET direct-backend endpoint without token or wrong scope
Backend-->>Test: 403 Forbidden (unauthorized)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java (1)
1087-1102: Avoid index-based operation mutation in the new scope test.Using a fixed list index here makes the test order-dependent and potentially flaky if operation ordering changes. Resolve and replace by target (
TARGET_GET_PETS) instead.💡 Suggested refactor
- final MCPServerOperationDTO updateOp = new MCPServerOperationDTO(); - copyOperation(mcpServer.getOperations().get(1), updateOp); + final MCPServerOperationDTO sourceOp = + getOperationByTarget(mcpServer.getOperations(), TARGET_GET_PETS); + Assert.assertNotNull(sourceOp, "MCP Server operation not found: " + TARGET_GET_PETS); + final MCPServerOperationDTO updateOp = new MCPServerOperationDTO(); + copyOperation(sourceOp, updateOp); @@ - mcpServer.getOperations().set(1, updateOp); + for (int i = 0; i < mcpServer.getOperations().size(); i++) { + if (TARGET_GET_PETS.equals(mcpServer.getOperations().get(i).getTarget())) { + mcpServer.getOperations().set(i, updateOp); + break; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java` around lines 1087 - 1102, The test mutates operations by hard-coded index (mcpServer.getOperations().get(1) and set(1)) which is order-dependent; instead locate the operation with target == TARGET_GET_PETS from mcpServer.getOperations(), copy it into updateOp via copyOperation, modify updateOp.setScopes(...), and replace the original entry by updating the found operation reference (or by replacing at its found index) so the test targets the operation by its TARGET_GET_PETS identifier rather than a fixed index; update references to MCPServerOperationDTO, copyOperation, mcpServer.getOperations(), updateOp, and TARGET_GET_PETS accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java`:
- Around line 1669-1677: The teardown calls handling applicationForScopesId_3
can run when that ID is unset and cause spurious failures; update the
MCPServerTestCase teardown to guard the subscription cleanup and deletion by
checking that applicationForScopesId_3 (and other application IDs like
applicationId, applicationForScopesId_1/2) are non-null/non-empty before calling
restAPIStore.getAllSubscriptionsOfApplication, restAPIStore.removeSubscription,
or restAPIStore.deleteApplication; only fetch subs4DTO and iterate/remove
subscriptions if applicationForScopesId_3 is present and ensure subs4DTO is
non-null before looping.
---
Nitpick comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java`:
- Around line 1087-1102: The test mutates operations by hard-coded index
(mcpServer.getOperations().get(1) and set(1)) which is order-dependent; instead
locate the operation with target == TARGET_GET_PETS from
mcpServer.getOperations(), copy it into updateOp via copyOperation, modify
updateOp.setScopes(...), and replace the original entry by updating the found
operation reference (or by replacing at its found index) so the test targets the
operation by its TARGET_GET_PETS identifier rather than a fixed index; update
references to MCPServerOperationDTO, copyOperation, mcpServer.getOperations(),
updateOp, and TARGET_GET_PETS accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java
Thisun-17
force-pushed
the
add-missing-mcp-scopes-test
branch
from
8320f2e to
42318da
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java (1)
1669-1673:⚠️ Potential issue | 🟠 MajorGuard teardown for partial setup to avoid masking failures.
Because
destroy()isalwaysRun = true, this block can execute with an unsetapplicationForScopesId_3and fail during subscription fetch/delete, hiding the original test failure.🛠️ Suggested hardening
- SubscriptionListDTO subs4DTO = - restAPIStore.getAllSubscriptionsOfApplication(applicationForScopesId_3, tenantDomain); - for (org.wso2.am.integration.clients.store.api.v1.dto.SubscriptionDTO subscriptionDTO : subs4DTO.getList()) { - restAPIStore.removeSubscription(subscriptionDTO); - } + if (StringUtils.isNotBlank(applicationForScopesId_3)) { + SubscriptionListDTO subs4DTO = + restAPIStore.getAllSubscriptionsOfApplication(applicationForScopesId_3, tenantDomain); + if (subs4DTO != null && subs4DTO.getList() != null) { + for (org.wso2.am.integration.clients.store.api.v1.dto.SubscriptionDTO subscriptionDTO + : subs4DTO.getList()) { + restAPIStore.removeSubscription(subscriptionDTO); + } + } + restAPIStore.deleteApplication(applicationForScopesId_3); + } @@ - restAPIStore.deleteApplication(applicationForScopesId_3);Also applies to: 1677-1677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java` around lines 1669 - 1673, Guard the teardown code so it only runs when the partial setup actually created the application id: check that applicationForScopesId_3 is non-null/not empty before calling restAPIStore.getAllSubscriptionsOfApplication(...) and iterating to call restAPIStore.removeSubscription(...); do the same defensive check for the similar block that uses applicationForScopesId_4 (the other teardown at 1677) to avoid masking the original test failure when destroy() is alwaysRun = true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java`:
- Around line 1669-1673: Guard the teardown code so it only runs when the
partial setup actually created the application id: check that
applicationForScopesId_3 is non-null/not empty before calling
restAPIStore.getAllSubscriptionsOfApplication(...) and iterating to call
restAPIStore.removeSubscription(...); do the same defensive check for the
similar block that uses applicationForScopesId_4 (the other teardown at 1677) to
avoid masking the original test failure when destroy() is alwaysRun = true.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java
- Add testScopesForDirectBackendSubtype to MCPServerTestCase - Validates scope enforcement on the DirectBackend (OpenAPI) subtype, completing the scopes test coverage across all three MCP server subtypes (ExistingApi, Proxy, and now DirectBackend) - Tests both positive path (token with scope -> 200) and negative path (token without scope -> 403) - Adds required constants, fields, and @afterclass cleanup
Thisun-17
force-pushed
the
add-missing-mcp-scopes-test
branch
from
42318da to
d5ab4ee
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java (1)
1676-1682:⚠️ Potential issue | 🟡 MinorHarden cleanup loop against null subscription payloads.
At Line 1679,
subs4DTO.getList()is dereferenced without a null check; teardown can still throw and mask the original failure if subscription fetch returns null/empty unexpectedly.🛠️ Suggested hardening
if (StringUtils.isNotBlank(applicationForScopesId_3)) { SubscriptionListDTO subs4DTO = restAPIStore.getAllSubscriptionsOfApplication(applicationForScopesId_3, tenantDomain); - for (org.wso2.am.integration.clients.store.api.v1.dto.SubscriptionDTO subscriptionDTO : subs4DTO.getList()) { - restAPIStore.removeSubscription(subscriptionDTO); + if (subs4DTO != null && subs4DTO.getList() != null) { + for (org.wso2.am.integration.clients.store.api.v1.dto.SubscriptionDTO subscriptionDTO + : subs4DTO.getList()) { + restAPIStore.removeSubscription(subscriptionDTO); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java` around lines 1676 - 1682, The teardown loop in MCPServerTestCase uses subs4DTO.getList() without null checks, which can throw during cleanup; update the block that obtains SubscriptionListDTO subs4DTO (from restAPIStore.getAllSubscriptionsOfApplication) to first verify subs4DTO != null and subs4DTO.getList() != null (or replace with an empty list) before iterating, and only call restAPIStore.removeSubscription(subscriptionDTO) for non-null subscriptionDTO entries to avoid masking original failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java`:
- Around line 1676-1682: The teardown loop in MCPServerTestCase uses
subs4DTO.getList() without null checks, which can throw during cleanup; update
the block that obtains SubscriptionListDTO subs4DTO (from
restAPIStore.getAllSubscriptionsOfApplication) to first verify subs4DTO != null
and subs4DTO.getList() != null (or replace with an empty list) before iterating,
and only call restAPIStore.removeSubscription(subscriptionDTO) for non-null
subscriptionDTO entries to avoid masking original failures.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/mcp/MCPServerTestCase.java
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.
Summary by CodeRabbit