Add per-proxy resource-server scopes for MCP proxies#1304
Conversation
📝 WalkthroughWalkthroughThis PR replaces organization-global scopes with MCP proxy-scoped scopes, adding persistence, nested APIs, per-proxy Thunder resource servers, role permission reconciliation, gateway policy generation, cleanup flows, and updated application wiring. ChangesMCP proxy scope foundation
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 |
jhivandb
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent-manager-service/clients/thundersvc/identity_client.go (1)
916-982: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftQualify Thunder resource-server identifiers by org
agent-manager-service/clients/thundersvc/identity_client.go:916-982,1006-1061—EnsureProxyResourceServerand the delete paths key Thunder resource servers only byproxyHandleand place them in the shared default OU. Since MCP proxy handles are only unique per org in this codebase, identical handles from different orgs can collide on the same Thunder resource server/actions. Use an org-qualified Thunder identifier, or enforce global uniqueness upstream.🤖 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 `@agent-manager-service/clients/thundersvc/identity_client.go` around lines 916 - 982, EnsureProxyResourceServer and its delete paths must prevent cross-org collisions for identical proxy handles. Use an org-qualified value for the Thunder resource-server identifier and handle (and corresponding action/resource references), or enforce global handle uniqueness before these methods; do not rely on the shared default OU for isolation. Update find/create/delete lookups consistently so all lifecycle operations use the same qualified identity.Source: Path instructions
agent-manager-service/controllers/agent_identity_controller.go (1)
387-454: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftScope resolution/grouping business logic lives in the controller, not a service.
resolveScopeGroups,proxyScopeGroup, and the RS-ensure/permission-diff orchestration inCreateRole/UpdateRolequeryproxyRepo/scopeRepoand orchestrate Thunder calls directly from the controller. Per the controller guideline, this logic (proxy/action validation, grouping, RS reconciliation) belongs in a service that the controller merely calls into.As per coding guidelines: "Controllers must be HTTP-only: parse and validate requests, map results to status codes, and translate sentinel errors to HTTP errors; they must not contain business logic."
Also applies to: 483-590, 759-834
🤖 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 `@agent-manager-service/controllers/agent_identity_controller.go` around lines 387 - 454, Move scope resolution/grouping, proxy/action validation, resource-server reconciliation, and permission orchestration out of agentIdentityController methods CreateRole and UpdateRole into a dedicated service with methods such as ResolveScopeGroups and CreateRole/UpdateRole orchestration; relocate proxyScopeGroup and repository/Thunder client calls there. Keep the controller HTTP-only by decoding and validating requests, invoking the service, and mapping returned results or sentinel errors to HTTP responses.Source: Coding guidelines
🧹 Nitpick comments (6)
deployments/setup/setup-openchoreo.sh (1)
105-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwallowed apply output hides the failure reason.
On failure,
kubectl apply ... >/dev/null 2>&1discards stderr, so operators only see the generic "Failed to migrate" message with no diagnostic detail, making a real setup failure harder to triage.♻️ Proposed fix
- if echo "$patched_binding_yaml" | kubectl apply --server-side --field-manager=helm --force-conflicts -f - >/dev/null 2>&1; then + apply_output=$(echo "$patched_binding_yaml" | kubectl apply --server-side --field-manager=helm --force-conflicts -f - 2>&1) + if [ $? -eq 0 ]; then echo " ✓ migrated ${binding}" else echo "❌ Failed to migrate ClusterAuthzRoleBinding '${binding}' to client_id" + echo "$apply_output" return 1 fi🤖 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 `@deployments/setup/setup-openchoreo.sh` at line 105, Preserve kubectl diagnostics in the apply command within the migration logic by removing the redirection that discards stdout and stderr, or otherwise capture and print the command output when it fails. Keep the existing success check and ensure the failure path reports the actual kubectl error alongside the generic migration message.agent-manager-service/docs/api_v1_openapi.yaml (2)
6156-6161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTag naming inconsistent with existing convention.
The new scope endpoints use
tags: [MCPProxyScopes]andtags: [AgentIdentities], but every existing related group uses spaced tags like- MCP Proxiesand- Agent Identity(e.g. lines 5844, 5926, 3295, 9890). This splits the generated docs/CLI command grouping into a separate, differently-named tag rather than joining the existing group, which is likely to affect theamCLI command grouping mentioned in the PR objectives.🎨 Proposed fix
- tags: [MCPProxyScopes] + tags: + - MCP Proxies- tags: [AgentIdentities] + tags: + - Agent IdentityAlso applies to: 6182-6185, 6217-6221, 6253-6257, 10432-10437
🤖 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 `@agent-manager-service/docs/api_v1_openapi.yaml` around lines 6156 - 6161, Update the tags for all affected MCP proxy scope and agent identity operations, including listMCPProxyScopes and the operations around the referenced sections, to use the existing spaced tag names “MCP Proxies” and “Agent Identity” instead of MCPProxyScopes and AgentIdentities. Ensure every related endpoint consistently uses the established grouping.
6156-6279: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNew MCP proxy scope endpoints omit error content schemas and 401 responses.
Every other error response in this file (400/404/409) includes a
content/application/jsonblock referencing#/components/schemas/ErrorResponse, and comparable proxy-scoped endpoints (e.g.getMCPProxy,updateMCPProxy) document a401response. The newlistMCPProxyScopes/createMCPProxyScope/updateMCPProxyScope/deleteMCPProxyScope/listAgentIdentityScopesoperations only give baredescriptionstrings for their error responses and omit401entirely, even though these routes are org-scoped and require authorization per the API guidelines.🐛 Proposed fix (example for one operation)
responses: "201": description: Scope created content: application/json: schema: $ref: "`#/components/schemas/MCPProxyScopeResponse`" "400": description: Invalid action, tools, or unknown tool name + content: + application/json: + schema: + $ref: "`#/components/schemas/ErrorResponse`" + "401": + description: Unauthorized + content: + application/json: + schema: + $ref: "`#/components/schemas/ErrorResponse`" "404": description: MCP proxy not found + content: + application/json: + schema: + $ref: "`#/components/schemas/ErrorResponse`" "409": description: A scope with this action already exists on the proxy + content: + application/json: + schema: + $ref: "`#/components/schemas/ErrorResponse`"Also applies to: 10432-10458
🤖 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 `@agent-manager-service/docs/api_v1_openapi.yaml` around lines 6156 - 6279, Update the response definitions for listMCPProxyScopes, createMCPProxyScope, updateMCPProxyScope, deleteMCPProxyScope, and listAgentIdentityScopes to document authorization failures with a 401 response. Add application/json content referencing components/schemas/ErrorResponse to each existing 400, 404, and 409 error response where applicable, matching the conventions used by getMCPProxy and updateMCPProxy.agent-manager-service/services/mcp_proxy_scope_service_unit_test.go (1)
86-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
discardLogger()instead ofslog.Default()in this test file
agent-manager-service/services/mcp_proxy_scope_service_unit_test.go:86, 105, 321, 359, 424 —discardLogger()already exists in this package, so use it at these constructor call sites to keep unit-test logs quiet.🤖 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 `@agent-manager-service/services/mcp_proxy_scope_service_unit_test.go` at line 86, Replace slog.Default() with discardLogger() at all MCPProxyScopeService constructor call sites in mcp_proxy_scope_service_unit_test.go, including the locations around lines 86, 105, 321, 359, and 424, and remove any now-unused slog import.Source: Coding guidelines
agent-manager-service/services/mcp_proxy_service.go (1)
495-552: 🚀 Performance & Scalability | 🔵 TrivialLGTM on correctness — scopes are captured before the cascading delete, and Thunder resource-server/role-permission cleanup is properly best-effort (never fails the caller, per the accompanying unit tests).
One architectural note:
cleanupProxyResourceServersmakes sequential network calls to each environment's Thunder identity client synchronously within theDeleterequest path. This mirrors the existingcleanupDeletedScopepattern elsewhere, so it isn't a new defect, but for orgs with many environments this could noticeably slow down proxy deletion. Worth considering backgrounding this cleanup (with its own context/timeout) if delete latency becomes a concern in practice.Also applies to: 554-599
🤖 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 `@agent-manager-service/services/mcp_proxy_service.go` around lines 495 - 552, Consider moving cleanupProxyResourceServers out of the synchronous Delete request path, using a background task with its own bounded context and timeout. Preserve best-effort behavior and ensure cleanup failures are logged without affecting successful proxy deletion; follow the existing cleanupDeletedScope pattern where appropriate.agent-manager-service/services/mcp_proxy_deployment.go (1)
147-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProxy-handle-from-Artifact fallback logic is duplicated across this file (and again in
mcp_proxy_service.go).
buildMCPProxyEnvArtifact,deployMCPProxyEndpoints, andbuildMCPProxyDeploymentYAMLeach independently re-deriveproxyHandlevia the sameif proxy.Artifact != nil && proxy.Artifact.Handle != "" { ... }pattern, whilemcp_proxy_service.go'scleanupProxyResourceServersalready calls a sharedproxyHandleOf(proxy)helper for the identical rule. Consolidating all four call sites onto the one helper removes the risk of the fallback rule drifting between gateway emission and Thunder cleanup.Also applies to: 228-240, 428-472
🤖 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 `@agent-manager-service/services/mcp_proxy_deployment.go` around lines 147 - 190, Centralize proxy handle resolution by replacing the duplicated Artifact-handle fallback logic in buildMCPProxyEnvArtifact, deployMCPProxyEndpoints, and buildMCPProxyDeploymentYAML with the existing proxyHandleOf helper, and update cleanupProxyResourceServers to remain consistent with that shared helper. Ensure all gateway emission and cleanup paths use proxyHandleOf rather than independently deriving proxyHandle.
🤖 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 `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Around line 935-941: The resource-server mutex currently spans multiple
Thunder I/O operations, is global per client, and is not shared by deletion
paths. Refactor EnsureProxyResourceServer and
DeleteProxyResourceServerAction/DeleteProxyResourceServer to use the existing
keyed-mutex pattern keyed by proxyHandle, acquiring it only around the minimal
check-then-create or delete decision while releasing it before unrelated I/O
where safe. Replace read-then-write races with atomic ON CONFLICT upserts where
applicable, and ensure all ensure/delete operations for the same handle use the
same lock without serializing different handles.
In `@agent-manager-service/controllers/agent_controller.go`:
- Around line 1033-1038: All affected controller handlers must reject an empty
tenant identity before invoking the service layer. After each
middleware.OUIDFromRequest call in the relevant handlers, add a fail-fast
validation for an empty ouID that returns the controller’s standard
internal-error response, and ensure no agentService call occurs until validation
passes.
In `@agent-manager-service/controllers/agent_identity_controller.go`:
- Around line 529-586: Update UpdateRole to distinguish an omitted scopes field
from an explicitly empty list by using a nullable/pointer field or equivalent
HasScopes indicator in the request model; when scopes is omitted, skip
resolveScopeGroups and all permission reconciliation while still applying
metadata updates, while an explicit empty list must retain full-replacement
behavior and clear permissions. If omission is not supported, mark scopes as
required in the API schema.
In `@agent-manager-service/repositories/mcp_proxy_scope_repository.go`:
- Around line 73-80: Map gorm.ErrRecordNotFound to utils.ErrScopeNotFound in the
mcpProxyScopeRepository methods Get, Update, and Delete, while preserving all
other errors unchanged; add the necessary utils reference and apply the mapping
at each database error return.
In `@agent-manager-service/services/mcp_proxy_scope_service_unit_test.go`:
- Around line 311-318: Update the ResolveIdentityFunc mock in
TestMCPProxyScopeDelete_CleansThunderBestEffort so it does not call t.Fatalf for
identity-disabled environments. Continue returning envAClient for env-a, but
return an error such as assert.AnError for env-b or other environments, allowing
cleanupDeletedScope to exercise its best-effort warning-and-continue behavior.
In `@agent-manager-service/services/mcp_proxy_scope_service.go`:
- Around line 379-390: In the environment lookup within the relevant service
method, replace uuid.MustParse with uuid.Parse and handle the returned error
before assigning envUUID. Return a descriptive error that identifies the
malformed environment UUID and environment name, rather than allowing a panic;
preserve the existing ErrEnvironmentNotFound behavior when no environment
matches.
---
Outside diff comments:
In `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Around line 916-982: EnsureProxyResourceServer and its delete paths must
prevent cross-org collisions for identical proxy handles. Use an org-qualified
value for the Thunder resource-server identifier and handle (and corresponding
action/resource references), or enforce global handle uniqueness before these
methods; do not rely on the shared default OU for isolation. Update
find/create/delete lookups consistently so all lifecycle operations use the same
qualified identity.
In `@agent-manager-service/controllers/agent_identity_controller.go`:
- Around line 387-454: Move scope resolution/grouping, proxy/action validation,
resource-server reconciliation, and permission orchestration out of
agentIdentityController methods CreateRole and UpdateRole into a dedicated
service with methods such as ResolveScopeGroups and CreateRole/UpdateRole
orchestration; relocate proxyScopeGroup and repository/Thunder client calls
there. Keep the controller HTTP-only by decoding and validating requests,
invoking the service, and mapping returned results or sentinel errors to HTTP
responses.
---
Nitpick comments:
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 6156-6161: Update the tags for all affected MCP proxy scope and
agent identity operations, including listMCPProxyScopes and the operations
around the referenced sections, to use the existing spaced tag names “MCP
Proxies” and “Agent Identity” instead of MCPProxyScopes and AgentIdentities.
Ensure every related endpoint consistently uses the established grouping.
- Around line 6156-6279: Update the response definitions for listMCPProxyScopes,
createMCPProxyScope, updateMCPProxyScope, deleteMCPProxyScope, and
listAgentIdentityScopes to document authorization failures with a 401 response.
Add application/json content referencing components/schemas/ErrorResponse to
each existing 400, 404, and 409 error response where applicable, matching the
conventions used by getMCPProxy and updateMCPProxy.
In `@agent-manager-service/services/mcp_proxy_deployment.go`:
- Around line 147-190: Centralize proxy handle resolution by replacing the
duplicated Artifact-handle fallback logic in buildMCPProxyEnvArtifact,
deployMCPProxyEndpoints, and buildMCPProxyDeploymentYAML with the existing
proxyHandleOf helper, and update cleanupProxyResourceServers to remain
consistent with that shared helper. Ensure all gateway emission and cleanup
paths use proxyHandleOf rather than independently deriving proxyHandle.
In `@agent-manager-service/services/mcp_proxy_scope_service_unit_test.go`:
- Line 86: Replace slog.Default() with discardLogger() at all
MCPProxyScopeService constructor call sites in
mcp_proxy_scope_service_unit_test.go, including the locations around lines 86,
105, 321, 359, and 424, and remove any now-unused slog import.
In `@agent-manager-service/services/mcp_proxy_service.go`:
- Around line 495-552: Consider moving cleanupProxyResourceServers out of the
synchronous Delete request path, using a background task with its own bounded
context and timeout. Preserve best-effort behavior and ensure cleanup failures
are logged without affecting successful proxy deletion; follow the existing
cleanupDeletedScope pattern where appropriate.
In `@deployments/setup/setup-openchoreo.sh`:
- Line 105: Preserve kubectl diagnostics in the apply command within the
migration logic by removing the redirection that discards stdout and stderr, or
otherwise capture and print the command output when it fails. Keep the existing
success check and ensure the failure path reports the actual kubectl error
alongside the generic migration message.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a16eabb1-d537-49c7-b8b7-1f8c57da3610
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (55)
agent-manager-service/api/app.goagent-manager-service/api/mcp_proxy_scope_routes.goagent-manager-service/clients/clientmocks/env_identity_client_mock.goagent-manager-service/clients/thundersvc/client.goagent-manager-service/clients/thundersvc/identity_client.goagent-manager-service/clients/thundersvc/identity_client_test.goagent-manager-service/controllers/agent_controller.goagent-manager-service/controllers/agent_identity_controller.goagent-manager-service/controllers/agent_identity_controller_unit_test.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/controllers/mcp_proxy_scope_controller.goagent-manager-service/controllers/scope_controller.goagent-manager-service/db_migrations/033_create_mcp_proxy_scopes.goagent-manager-service/db_migrations/034_drop_scopes.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/mcp_proxy.goagent-manager-service/models/mcp_proxy_scope.goagent-manager-service/models/scope.goagent-manager-service/repositories/mcp_proxy_scope_repository.goagent-manager-service/repositories/repomocks/mcp_proxy_scope_repository_mock.goagent-manager-service/repositories/repomocks/scope_repository_mock.goagent-manager-service/repositories/scope_repository.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/mcp_proxy_deployment.goagent-manager-service/services/mcp_proxy_deployment_test.goagent-manager-service/services/mcp_proxy_scope_service.goagent-manager-service/services/mcp_proxy_scope_service_unit_test.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/mcp_proxy_service_unit_test.goagent-manager-service/services/scope_service.goagent-manager-service/services/scope_service_unit_test.goagent-manager-service/spec/api_agent_identities.goagent-manager-service/spec/api_mcp_proxy_scopes.goagent-manager-service/spec/client.goagent-manager-service/spec/model_agent_identity_role_request.goagent-manager-service/spec/model_agent_identity_scope_entry.goagent-manager-service/spec/model_agent_identity_scope_list_response.goagent-manager-service/spec/model_mcp_proxy_endpoint.goagent-manager-service/spec/model_mcp_proxy_scope_list_response.goagent-manager-service/spec/model_mcp_proxy_scope_request.goagent-manager-service/spec/model_mcp_proxy_scope_response.goagent-manager-service/spec/model_mcp_proxy_scope_update_request.goagent-manager-service/spec/model_mcp_tool_scope_binding.goagent-manager-service/spec/model_scope_list_response.goagent-manager-service/spec/model_scope_request.goagent-manager-service/spec/model_scope_response.goagent-manager-service/spec/model_scope_update_request.goagent-manager-service/utils/constants.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.godeployments/setup/setup-openchoreo.shdocs/superpowers/plans/2026-07-09-mcp-proxy-resource-server-scopes.mddocs/superpowers/specs/2026-07-09-mcp-proxy-resource-server-scopes-design.md
💤 Files with no reviewable changes (11)
- agent-manager-service/models/scope.go
- agent-manager-service/controllers/scope_controller.go
- agent-manager-service/spec/model_scope_request.go
- agent-manager-service/repositories/scope_repository.go
- agent-manager-service/spec/model_scope_update_request.go
- agent-manager-service/services/scope_service.go
- agent-manager-service/repositories/repomocks/scope_repository_mock.go
- agent-manager-service/spec/model_mcp_tool_scope_binding.go
- agent-manager-service/services/scope_service_unit_test.go
- agent-manager-service/spec/model_scope_list_response.go
- agent-manager-service/spec/model_scope_response.go
| func (c *thunderClient) EnsureProxyResourceServer(ctx context.Context, proxyHandle, displayName string, actions []string) (string, error) { | ||
| if len(proxyHandle) > thunderHandleMaxLen { | ||
| return "", fmt.Errorf("proxy handle %q exceeds the Thunder handle limit of %d characters", proxyHandle, thunderHandleMaxLen) | ||
| } | ||
| for _, action := range actions { | ||
| if len(action) > thunderHandleMaxLen { | ||
| return "", fmt.Errorf("action %q exceeds the Thunder handle limit of %d characters", action, thunderHandleMaxLen) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Global mutex held across external I/O; Delete paths bypass it entirely.
ensureResourceServerMu wraps the full RS lookup/create/list/create-action sequence — several sequential Thunder HTTP round-trips — rather than just the check-then-create decision. It's also a single mutex per client (per org/env), so unrelated proxies within the same org/env serialize behind one Thunder call chain; a slow/stuck Thunder request stalls every other proxy's role-creation/scope-write in that org/env. Separately, DeleteProxyResourceServerAction/DeleteProxyResourceServer never take this lock, so a concurrent EnsureProxyResourceServer and proxy delete for the same handle can interleave (e.g., Ensure lists actions right as Delete removes the RS, or vice versa).
As per path instructions: "Never hold a lock across I/O; use atomic ON CONFLICT upserts instead of read-then-write, and serialize expensive side effects per key rather than globally." Consider a keyed mutex scoped to proxyHandle (the codebase already has this pattern in agent_thunder_provisioning_service.go's keyedMutex) and have DeleteProxyResourceServer* take the same per-handle lock.
Also applies to: 950-1001, 1006-1061
🤖 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 `@agent-manager-service/clients/thundersvc/identity_client.go` around lines 935
- 941, The resource-server mutex currently spans multiple Thunder I/O
operations, is global per client, and is not shared by deletion paths. Refactor
EnsureProxyResourceServer and
DeleteProxyResourceServerAction/DeleteProxyResourceServer to use the existing
keyed-mutex pattern keyed by proxyHandle, acquiring it only around the minimal
check-then-create or delete decision while releasing it before unrelated I/O
where safe. Replace read-then-write races with atomic ON CONFLICT upserts where
applicable, and ensure all ensure/delete operations for the same handle use the
same lock without serializing different handles.
Source: Path instructions
There was a problem hiding this comment.
Skipping in this PR (tracking as follow-up). The lock is TOCTOU-safe for the create it guards and the code is functionally correct today; the real concern is contention when many scope/role writes hit the same org/env Thunder concurrently (low in practice) and Ensure/Delete interleaving on the same handle — both idempotent against Thunder, which is the source of truth. Migrating to a per-proxyHandle keyed mutex (the keyedMutex pattern in agent_thunder_provisioning_service.go) is the right fix but a heavy lift, so deferring rather than bundling it into this change.
There was a problem hiding this comment.
@jhivandb Understood, that's a reasonable call given the low real-world contention and idempotency guarantees on the Thunder side. Deferring the per-proxyHandle keyed mutex migration as a follow-up makes sense rather than bundling it into this PR.
Would you like me to open a follow-up issue to track this (per-proxy keyed mutex for EnsureProxyResourceServer/DeleteProxyResourceServer* in agent-manager-service/clients/thundersvc/identity_client.go)? I can link back to this PR and comment for context.
8e7fbbb to
74d1f6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@agent-manager-service/controllers/agent_identity_controller.go`:
- Around line 529-591: Update the role response path in CreateRole and
UpdateRole so the returned role reflects permissions written by
AddRolePermissions and RemoveRolePermissions. After reconciliation completes,
re-fetch the role or merge the final permission state into updated before
calling utils.WriteSuccessResponse, ensuring both 200 and 201 responses contain
current permissions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29a70989-cf70-452a-b6b8-2570d6c21f2f
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (53)
agent-manager-service/api/app.goagent-manager-service/api/mcp_proxy_scope_routes.goagent-manager-service/clients/clientmocks/env_identity_client_mock.goagent-manager-service/clients/thundersvc/client.goagent-manager-service/clients/thundersvc/identity_client.goagent-manager-service/clients/thundersvc/identity_client_test.goagent-manager-service/controllers/agent_controller.goagent-manager-service/controllers/agent_identity_controller.goagent-manager-service/controllers/agent_identity_controller_unit_test.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/controllers/mcp_proxy_scope_controller.goagent-manager-service/controllers/scope_controller.goagent-manager-service/db_migrations/033_create_mcp_proxy_scopes.goagent-manager-service/db_migrations/034_drop_scopes.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/mcp_proxy.goagent-manager-service/models/mcp_proxy_scope.goagent-manager-service/models/scope.goagent-manager-service/repositories/mcp_proxy_scope_repository.goagent-manager-service/repositories/repomocks/mcp_proxy_scope_repository_mock.goagent-manager-service/repositories/repomocks/scope_repository_mock.goagent-manager-service/repositories/scope_repository.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/mcp_proxy_deployment.goagent-manager-service/services/mcp_proxy_deployment_test.goagent-manager-service/services/mcp_proxy_scope_service.goagent-manager-service/services/mcp_proxy_scope_service_unit_test.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/mcp_proxy_service_unit_test.goagent-manager-service/services/scope_service.goagent-manager-service/services/scope_service_unit_test.goagent-manager-service/spec/api_agent_identities.goagent-manager-service/spec/api_mcp_proxy_scopes.goagent-manager-service/spec/client.goagent-manager-service/spec/model_agent_identity_role_request.goagent-manager-service/spec/model_agent_identity_scope_entry.goagent-manager-service/spec/model_agent_identity_scope_list_response.goagent-manager-service/spec/model_mcp_proxy_endpoint.goagent-manager-service/spec/model_mcp_proxy_scope_list_response.goagent-manager-service/spec/model_mcp_proxy_scope_request.goagent-manager-service/spec/model_mcp_proxy_scope_response.goagent-manager-service/spec/model_mcp_proxy_scope_update_request.goagent-manager-service/spec/model_mcp_tool_scope_binding.goagent-manager-service/spec/model_scope_list_response.goagent-manager-service/spec/model_scope_request.goagent-manager-service/spec/model_scope_response.goagent-manager-service/spec/model_scope_update_request.goagent-manager-service/utils/constants.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.godeployments/setup/setup-openchoreo.sh
💤 Files with no reviewable changes (11)
- agent-manager-service/models/scope.go
- agent-manager-service/spec/model_mcp_tool_scope_binding.go
- agent-manager-service/controllers/scope_controller.go
- agent-manager-service/repositories/scope_repository.go
- agent-manager-service/repositories/repomocks/scope_repository_mock.go
- agent-manager-service/spec/model_scope_update_request.go
- agent-manager-service/services/scope_service_unit_test.go
- agent-manager-service/spec/model_scope_list_response.go
- agent-manager-service/services/scope_service.go
- agent-manager-service/spec/model_scope_response.go
- agent-manager-service/spec/model_scope_request.go
✅ Files skipped from review due to trivial changes (9)
- agent-manager-service/spec/model_agent_identity_role_request.go
- agent-manager-service/spec/model_mcp_proxy_scope_list_response.go
- agent-manager-service/spec/api_agent_identities.go
- agent-manager-service/spec/model_agent_identity_scope_list_response.go
- agent-manager-service/spec/model_agent_identity_scope_entry.go
- agent-manager-service/clients/clientmocks/env_identity_client_mock.go
- agent-manager-service/repositories/repomocks/mcp_proxy_scope_repository_mock.go
- agent-manager-service/spec/model_mcp_proxy_scope_response.go
- agent-manager-service/wiring/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (31)
- agent-manager-service/db_migrations/migration_list.go
- agent-manager-service/api/app.go
- agent-manager-service/db_migrations/034_drop_scopes.go
- agent-manager-service/models/mcp_proxy_scope.go
- agent-manager-service/api/mcp_proxy_scope_routes.go
- agent-manager-service/clients/thundersvc/client.go
- agent-manager-service/wiring/params.go
- agent-manager-service/spec/model_mcp_proxy_scope_request.go
- agent-manager-service/controllers/mcp_proxy_controller.go
- agent-manager-service/utils/constants.go
- agent-manager-service/clients/thundersvc/identity_client_test.go
- agent-manager-service/db_migrations/033_create_mcp_proxy_scopes.go
- deployments/setup/setup-openchoreo.sh
- agent-manager-service/spec/client.go
- agent-manager-service/wiring/wire.go
- agent-manager-service/repositories/mcp_proxy_scope_repository.go
- agent-manager-service/controllers/agent_controller.go
- agent-manager-service/spec/model_mcp_proxy_endpoint.go
- agent-manager-service/models/mcp_proxy.go
- agent-manager-service/spec/model_mcp_proxy_scope_update_request.go
- agent-manager-service/services/agent_configuration_service.go
- agent-manager-service/services/mcp_proxy_scope_service.go
- agent-manager-service/services/mcp_proxy_deployment.go
- agent-manager-service/controllers/mcp_proxy_scope_controller.go
- agent-manager-service/services/mcp_proxy_scope_service_unit_test.go
- agent-manager-service/services/mcp_proxy_deployment_test.go
- agent-manager-service/spec/api_mcp_proxy_scopes.go
- agent-manager-service/clients/thundersvc/identity_client.go
- agent-manager-service/services/mcp_proxy_service_unit_test.go
- agent-manager-service/docs/api_v1_openapi.yaml
- agent-manager-service/services/mcp_proxy_service.go
2 participants
Purpose
For:
Introduces per-proxy resource-server scopes for MCP proxies, replacing the old org-global scope catalog. Each MCP proxy now owns its scopes (
<proxy-handle>:<action>), maps them to tool sets, and projects them onto a per-proxy Thunder OAuth resource server. Agent-identity roles reference these scopes, and the gateway enforces them per tool at request time.This lets an Agent Identity present a narrowly-scoped JWT (e.g.
railway-toolbox:read) and call only the tools that scope covers, while write tools are rejected.Goals
mcp-authfor authentication,mcp-authzfor per-tool authorization) directly from themcp_proxy_scopesrows.Approach
mcp_proxy_scopestable/model/repository; org-globalscopestable dropped (migrations 033 create, 034 drop).MCPProxyScopeServicevalidates kebab-case actions and tool membership, and drives Thunder resource-server ensure/cleanup plus gateway re-emission on every mutation./orgs/{org}/mcp-proxies/{proxyId}/scopes, plus an env-filtered agent-identity scope aggregate endpoint. OpenAPI spec and theamCLI client are regenerated to match.mcp-authzrule per tool (any-of scopes within a rule);mcp-authcarries only the pinnedThunderKeyManagerissuer.Late fixes from end-to-end verification against a live gateway + Thunder:
mcp-authno longer carriesrequiredScopes. It forwards its params verbatim tojwt-auth, which enforces every listed scope (all-of); emitting the proxy's scope union there 401'd any token holding only a subset. Per-tool authorization remains inmcp-authz.Update()now uses struct-basedUpdates+Selectso theToolsfield routes through itsserializer:jsontag instead of the postgres-array valuer (was 500ing).middleware.OUIDFromRequest) instead of the org name to the service layer.User stories
As a platform operator, I define read/write scopes on an MCP proxy, bind them to agent-identity roles, and have the gateway enforce per-tool access from the agent's JWT.
Release note
Add per-proxy resource-server scopes for MCP proxies with per-tool gateway authorization via Agent Identity JWTs.
Documentation
N/A for this PR — design spec and implementation plan are included under
docs/superpowers/plans/. Product-doc impact to be tracked separately.Training
N/A — no training-content impact.
Certification
N/A — no impact on certification exams.
Marketing
N/A — internal platform capability, no marketing content.
Automation tests
Security checks
Samples
N/A — no standalone sample; the scope CRUD API and gateway enforcement are exercised by the tests above.
Related PRs
N/A.
Migrations (if applicable)
033_create_mcp_proxy_scopes— creates themcp_proxy_scopestable.034_drop_scopes— drops the org-globalscopestable.Apply with
make dev-migrate(requires the docker-compose DB up). Tested locally against the docker-compose Postgres.Test environment
gateway-runtime:1.1.0).go.mod.Learning
Root-caused the gateway 401 by tracing the
mcp-auth→jwt-authdelegation inwso2/gateway-controllers:mcp-authforwards its full params map tojwt-auth, whoserequiredScopescheck is all-of, so advertising the scope union at the auth layer blocked partial-scope tokens before per-toolmcp-authzever ran.Summary by CodeRabbit
New Features
Breaking Changes
<proxy-handle>:<action>format.Bug Fixes