[FIX][RBAC]: Session tokens denied tools.execute on /rpc and /mcp despite having team-scoped role#3516
Conversation
6652846 to
efea4c2
Compare
MohanLaksh
left a comment
There was a problem hiding this comment.
Followed the below steps to test the fix:
- Set
DEFAULT_TEAM_MEMBER_ROLE=developerin my.envfile - Create a new user (user1@example.com) and a new team (Test Team) via the admin UI
- Logged in as
user1@example.com - Added Github MCP server scoped to Test team
- Went to the Tools screen and run a tool (github-tools-get-me)
Successfully able to execute the tool
Works as intended.
APPROVED
…sure_rpc_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…sion tokens Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…_streamable_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…ss check_any_team for session tokens Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…llback Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…_any_team Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…eny-path tests - streamablehttp_transport.py: drop redundant ternary guard; user_context is guaranteed non-None at that point by _should_enforce_streamable_rbac check - test_rpc_permission_team_fallback.py: replace vacuous admin-bypass smoke test with a meaningful assertion that has_permission is called with check_any_team=True for admin session tokens (bypass lives inside PermissionService, not _ensure_rpc_permission) - test_streamable_rpc_permission_fallback.py: add deny-path integration test verifying call_tool raises PermissionError when _check_streamable_permission returns False Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…ool execution Three tests in TestRPCToolExecutionRBAC: - test_developer_rpc_tools_call_not_denied: developer with team-scoped role sends tools/call via /rpc as a session token and must NOT receive -32003 - test_viewer_rpc_tools_call_denied: viewer (no tools.execute) must still receive -32003 (deny-path regression guard) - test_developer_can_list_team_tool: developer can see their team-scoped tool in GET /tools (Layer 1 visibility check) Signed-off-by: Madhav Kandukuri <madhav165@gmail.com>
…_permission Add two tests covering the db_session and fresh_db_session paths of PermissionChecker.has_permission to assert check_any_team=True is forwarded to PermissionService.check_permission. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
…e in _normalize_jwt_payload Address Codex review findings: - servers.use check in handle_streamable_http now detects session tokens and passes check_any_team=True, matching the call_tool fix. - _normalize_jwt_payload includes token_use in the returned context dict so the re-auth fallback path preserves session-token detection. - Update existing _normalize_jwt_payload test assertions for the new key. - Add servers.use session-token regression test. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
aa8cd48 to
9eb09ed
Compare
Four new E2E tests in TestSessionTokenCookieRBAC: - test_developer_cookie_rpc_tools_call: session cookie + page.evaluate fetch to /rpc — matches the actual Admin UI Tools screen flow - test_viewer_cookie_rpc_tools_call_denied: viewer deny-path via cookie - test_developer_cookie_rpc_tools_list: tools.read permission via /rpc tools/list (verifies check_any_team applies to all RPC permissions) - test_cross_team_tool_not_visible: Layer 1 isolation — developer in Team A cannot see Team B's tool even with check_any_team=True Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
crivetimihai
left a comment
There was a problem hiding this comment.
Review: Approved
Rebased onto main, reviewed for correctness/security/consistency, addressed findings from Codex cross-review, and added additional test coverage. All unit and Playwright E2E tests pass.
Changes made during review
Codex-identified fixes (commit 9eb09ed):
-
servers.usemissingcheck_any_team(High) — Theservers.usecheck inhandle_streamable_httpat line 2353 gates access to/servers/{id}/mcp(the primary MCP client URL). Bothdeveloperandteam_adminroles grantservers.useas a team-scoped permission, so session tokens would be denied at the transport level before even reachingcall_tool. Fixed by adding the same session-token detection pattern. -
_normalize_jwt_payloaddropstoken_use(Low) — The re-auth fallback path in_get_request_context_or_defaultuses_normalize_jwt_payload, which didn't includetoken_usein its output. If that fallback were triggered,check_any_teamwould silently default toFalse. Fixed by adding"token_use": token_useto the returned dict. Updated 4 existing test assertions accordingly.
Test coverage additions:
test_permission_checker_has_permission_forwards_check_any_team— verifiescheck_any_team=Trueforwarding throughPermissionChecker.has_permission(db_session path) (commit9267f28)test_permission_checker_has_permission_forwards_check_any_team_fresh_db— same for the fresh_db_session path (commit9267f28)test_servers_use_check_passes_check_any_team_for_session_token— unit test for theservers.usefix (commit9eb09ed)- 4 new Playwright E2E cookie-based browser tests in
TestSessionTokenCookieRBAC(commita7331f3):test_developer_cookie_rpc_tools_call— session cookie +page.evaluate(fetch('/rpc')), matching actual Admin UI flowtest_viewer_cookie_rpc_tools_call_denied— viewer deny-path via cookie authtest_developer_cookie_rpc_tools_list—tools/listRPC verifying fix covers all RPC permissionstest_cross_team_tool_not_visible— Layer 1 cross-team isolation: developer in Team A cannot see Team B's tool
Security verification
check_any_team=Truedoes NOT enable cross-team access — Layer 1 (_has_tool_access_by_visibility) checkstool.team_id in token_teams- API tokens unaffected (scoped to
token_use == "session"only) - Personal team exclusion intact (
_get_user_rolesexcludes personal teams wheninclude_all_teams=True) - Fail-closed: missing
token_usedefaults tocheck_any_team=False
Test results
- 724 unit tests passed (0 failed)
- 7 Playwright E2E tests passed against live server (3 original + 4 new)
rbac.pyat 100% coverage
…pite having team-scoped role (#3516) * test(rbac): add failing tests for session-token check_any_team in _ensure_rpc_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): harden assertions in rpc permission team fallback tests Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * feat(rbac): add check_any_team param to PermissionChecker.has_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * fix(rbac): pass check_any_team=True in _ensure_rpc_permission for session tokens Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): add contract tests for check_any_team threading in _check_streamable_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * fix(rbac): propagate token_use in streamablehttp auth_user_ctx and pass check_any_team for session tokens Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): add deny-path regression tests for rpc team-permission fallback Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): accept **kwargs in _has_permission mocks to forward check_any_team Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * fix(rbac): remove dead null-guard in call_tool and strengthen admin+deny-path tests - streamablehttp_transport.py: drop redundant ternary guard; user_context is guaranteed non-None at that point by _should_enforce_streamable_rbac check - test_rpc_permission_team_fallback.py: replace vacuous admin-bypass smoke test with a meaningful assertion that has_permission is called with check_any_team=True for admin session tokens (bypass lives inside PermissionService, not _ensure_rpc_permission) - test_streamable_rpc_permission_fallback.py: add deny-path integration test verifying call_tool raises PermissionError when _check_streamable_permission returns False Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): add #3515 regression Playwright tests for session-token tool execution Three tests in TestRPCToolExecutionRBAC: - test_developer_rpc_tools_call_not_denied: developer with team-scoped role sends tools/call via /rpc as a session token and must NOT receive -32003 - test_viewer_rpc_tools_call_denied: viewer (no tools.execute) must still receive -32003 (deny-path regression guard) - test_developer_can_list_team_tool: developer can see their team-scoped tool in GET /tools (Layer 1 visibility check) Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): verify check_any_team forwarding in PermissionChecker.has_permission Add two tests covering the db_session and fresh_db_session paths of PermissionChecker.has_permission to assert check_any_team=True is forwarded to PermissionService.check_permission. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(rbac): pass check_any_team for servers.use and propagate token_use in _normalize_jwt_payload Address Codex review findings: - servers.use check in handle_streamable_http now detects session tokens and passes check_any_team=True, matching the call_tool fix. - _normalize_jwt_payload includes token_use in the returned context dict so the re-auth fallback path preserves session-token detection. - Update existing _normalize_jwt_payload test assertions for the new key. - Add servers.use session-token regression test. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(rbac): add browser cookie session-token Playwright tests for #3515 Four new E2E tests in TestSessionTokenCookieRBAC: - test_developer_cookie_rpc_tools_call: session cookie + page.evaluate fetch to /rpc — matches the actual Admin UI Tools screen flow - test_viewer_cookie_rpc_tools_call_denied: viewer deny-path via cookie - test_developer_cookie_rpc_tools_list: tools.read permission via /rpc tools/list (verifies check_any_team applies to all RPC permissions) - test_cross_team_tool_not_visible: Layer 1 isolation — developer in Team A cannot see Team B's tool even with check_any_team=True Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
…pite having team-scoped role (#3516) * test(rbac): add failing tests for session-token check_any_team in _ensure_rpc_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): harden assertions in rpc permission team fallback tests Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * feat(rbac): add check_any_team param to PermissionChecker.has_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * fix(rbac): pass check_any_team=True in _ensure_rpc_permission for session tokens Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): add contract tests for check_any_team threading in _check_streamable_permission Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * fix(rbac): propagate token_use in streamablehttp auth_user_ctx and pass check_any_team for session tokens Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): add deny-path regression tests for rpc team-permission fallback Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): accept **kwargs in _has_permission mocks to forward check_any_team Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * fix(rbac): remove dead null-guard in call_tool and strengthen admin+deny-path tests - streamablehttp_transport.py: drop redundant ternary guard; user_context is guaranteed non-None at that point by _should_enforce_streamable_rbac check - test_rpc_permission_team_fallback.py: replace vacuous admin-bypass smoke test with a meaningful assertion that has_permission is called with check_any_team=True for admin session tokens (bypass lives inside PermissionService, not _ensure_rpc_permission) - test_streamable_rpc_permission_fallback.py: add deny-path integration test verifying call_tool raises PermissionError when _check_streamable_permission returns False Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): add #3515 regression Playwright tests for session-token tool execution Three tests in TestRPCToolExecutionRBAC: - test_developer_rpc_tools_call_not_denied: developer with team-scoped role sends tools/call via /rpc as a session token and must NOT receive -32003 - test_viewer_rpc_tools_call_denied: viewer (no tools.execute) must still receive -32003 (deny-path regression guard) - test_developer_can_list_team_tool: developer can see their team-scoped tool in GET /tools (Layer 1 visibility check) Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> * test(rbac): verify check_any_team forwarding in PermissionChecker.has_permission Add two tests covering the db_session and fresh_db_session paths of PermissionChecker.has_permission to assert check_any_team=True is forwarded to PermissionService.check_permission. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(rbac): pass check_any_team for servers.use and propagate token_use in _normalize_jwt_payload Address Codex review findings: - servers.use check in handle_streamable_http now detects session tokens and passes check_any_team=True, matching the call_tool fix. - _normalize_jwt_payload includes token_use in the returned context dict so the re-auth fallback path preserves session-token detection. - Update existing _normalize_jwt_payload test assertions for the new key. - Add servers.use session-token regression test. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(rbac): add browser cookie session-token Playwright tests for #3515 Four new E2E tests in TestSessionTokenCookieRBAC: - test_developer_cookie_rpc_tools_call: session cookie + page.evaluate fetch to /rpc — matches the actual Admin UI Tools screen flow - test_viewer_cookie_rpc_tools_call_denied: viewer deny-path via cookie - test_developer_cookie_rpc_tools_list: tools.read permission via /rpc tools/list (verifies check_any_team applies to all RPC permissions) - test_cross_team_tool_not_visible: Layer 1 isolation — developer in Team A cannot see Team B's tool even with check_any_team=True Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Madhav Kandukuri <madhav165@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
📌 Summary
Session tokens whose
tools.executepermission exists only in a team-scoped RBAC role were incorrectly denied on/rpc(tools/call) and/mcp(Streamable HTTP). The same user succeeded on REST endpoints because those use the@require_permissiondecorator which already applies acheck_any_teamfallback for session tokens.Closes #3515
🔁 Reproduction Steps
DEFAULT_TEAM_MEMBER_ROLE=developerin your environment-32003 Insufficient permissions. Required: tools.execute🐞 Root Cause
Two permission-check helpers were missing the
check_any_teamfallback that the@require_permissiondecorator applies for session tokens (rbac.py:562–576):_ensure_rpc_permission(main.py) — used byPOST /rpc. Cannot use@require_permissionat the endpoint level because/rpcis a multi-method dispatcher; permission is resolved after parsing the JSON-RPC body. CalledPermissionChecker.has_permissionwithoutcheck_any_team, so_get_user_rolesonly returned global roles and team roles withscope_id=NULL, missing the user's actual team assignments._check_streamable_permissioncall site (streamablehttp_transport.py) — used byPOST /mcp. The function already acceptedcheck_any_teambut it was always called with the defaultFalse. Additionally,token_usewas not stored inauth_user_ctx, so the transport had no way to detect session tokens.💡 Fix Description
Three targeted changes:
PermissionChecker.has_permission(mcpgateway/middleware/rbac.py) — addedcheck_any_team: bool = Falseparameter and threaded it through to bothPermissionService.check_permissioncall sites. Default isFalse, preserving all existing callers._ensure_rpc_permission(mcpgateway/main.py) — detects session tokens viauser.get("token_use") == "session"and passescheck_any_team=TruetoPermissionChecker.has_permission.Streamable HTTP transport (
mcpgateway/transports/streamablehttp_transport.py) — storestoken_useinauth_user_ctxso it propagates into the MCP handler context;call_toolnow detects session tokens and passescheck_any_team=Trueto_check_streamable_permission.🔐 Security Rationale for
check_any_team=TrueA reviewer might ask: does
check_any_team=Trueallow a user withtools.executein Team A to execute a team-scoped tool owned by Team B?The answer is no, and here is why the two paths differ:
REST path (
@require_permission): Tier 1 team derivation (rbac.py:566) looks up the target resource'steam_idfrom the DB before the RBAC check. For a Team B tool,team_id = Team Bis derived,check_any_teamstaysFalse, and the check "does user havetools.executein Team B?" fails → denied at RBAC (Layer 2).RPC/MCP path (this fix):
check_any_team=Truemeans the RBAC check passes (user hastools.executein Team A). However,tool_service.invoke_toolthen calls_has_tool_access_by_visibility(tool_service.py:796–807), which explicitly evaluatestool.team_id in token_teams. For a session token user in Team A only,token_teams = ["team-a-id"](derived from server-side membership), so"team-b-id" NOT IN ["team-a-id"]→ToolNotFoundError→ denied at tool service (Layer 1).Both paths deny cross-team execution. The REST path catches it at RBAC; the RPC/MCP path catches it at the tool service's visibility check. Public tools (visibility =
"public") are correctly accessible to any authenticated user with the permission on both paths — that is the intended behaviour of public visibility.API tokens carry explicit
token_teamsJWT claims and never usecheck_any_team(it staysFalse). Their scope is fully determined by the token claims, not server-side membership.🧪 Verification
make lintuv run pytest tests/unit/uv run pytest tests/unit/mcpgateway/test_rpc_permission_team_fallback.py tests/unit/mcpgateway/transports/test_streamable_rpc_permission_fallback.py -v📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)