Skip to content

Commit b9440d7

Browse files
madhav165crivetimihai
authored andcommitted
[FIX][RBAC]: Session tokens denied tools.execute on /rpc and /mcp despite 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>
1 parent f271ac5 commit b9440d7

File tree

9 files changed

+824
-10
lines changed

9 files changed

+824
-10
lines changed

mcpgateway/main.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -576,8 +576,11 @@ async def _ensure_rpc_permission(user, db: Session, permission: str, method: str
576576
raise JSONRPCError(-32003, f"Insufficient permissions. Required: {permission}", {"method": method})
577577

578578
# Layer 2: RBAC check
579+
# Session tokens have no explicit team_id, so check across all team-scoped roles.
580+
# Mirrors the @require_permission decorator's check_any_team fallback (rbac.py:562-576).
581+
check_any_team = isinstance(user, dict) and user.get("token_use") == "session"
579582
checker = PermissionChecker(_build_rpc_permission_user(user, db))
580-
if not await checker.has_permission(permission):
583+
if not await checker.has_permission(permission, check_any_team=check_any_team):
581584
raise JSONRPCError(-32003, f"Insufficient permissions. Required: {permission}", {"method": method})
582585

583586

mcpgateway/middleware/rbac.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -954,14 +954,15 @@ def __init__(self, user_context: dict):
954954
self.user_context = user_context
955955
self.db_session = user_context.get("db")
956956

957-
async def has_permission(self, permission: str, resource_type: Optional[str] = None, resource_id: Optional[str] = None, team_id: Optional[str] = None) -> bool:
957+
async def has_permission(self, permission: str, resource_type: Optional[str] = None, resource_id: Optional[str] = None, team_id: Optional[str] = None, check_any_team: bool = False) -> bool:
958958
"""Check if user has specific permission.
959959
960960
Args:
961961
permission: Permission to check
962962
resource_type: Optional resource type
963963
resource_id: Optional resource ID
964964
team_id: Optional team context
965+
check_any_team: If True, check across all teams the user belongs to
965966
966967
Returns:
967968
bool: True if user has permission
@@ -978,6 +979,7 @@ async def has_permission(self, permission: str, resource_type: Optional[str] = N
978979
token_teams=self.user_context.get("token_teams"),
979980
ip_address=self.user_context.get("ip_address"),
980981
user_agent=self.user_context.get("user_agent"),
982+
check_any_team=check_any_team,
981983
)
982984
# Create fresh db session
983985
with fresh_db_session() as db:
@@ -991,6 +993,7 @@ async def has_permission(self, permission: str, resource_type: Optional[str] = N
991993
token_teams=self.user_context.get("token_teams"),
992994
ip_address=self.user_context.get("ip_address"),
993995
user_agent=self.user_context.get("user_agent"),
996+
check_any_team=check_any_team,
994997
)
995998

996999
async def has_admin_permission(self) -> bool:

mcpgateway/transports/streamablehttp_transport.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -987,9 +987,13 @@ async def call_tool(name: str, arguments: dict) -> Union[
987987
if not _check_scoped_permission(user_context, "tools.execute"):
988988
raise PermissionError("Insufficient permissions. Required: tools.execute")
989989
# Layer 2: RBAC check
990+
# Session tokens have no explicit team_id; check across all team-scoped roles.
991+
# Mirrors the @require_permission decorator's check_any_team fallback (rbac.py:562-576).
992+
_is_session_token = user_context.get("token_use") == "session"
990993
has_execute_permission = await _check_streamable_permission(
991994
user_context=user_context,
992995
permission="tools.execute",
996+
check_any_team=_is_session_token,
993997
)
994998
if not has_execute_permission:
995999
raise PermissionError("Insufficient permissions. Required: tools.execute")
@@ -1360,7 +1364,7 @@ def _normalize_jwt_payload(payload: dict[str, Any]) -> dict[str, Any]:
13601364
"""Normalize a raw JWT payload to the canonical user context shape.
13611365
13621366
Converts raw JWT fields (sub, token_use, nested user.is_admin) into the
1363-
canonical ``{email, teams, is_admin, is_authenticated}`` dict that MCP
1367+
canonical ``{email, teams, is_admin, is_authenticated, token_use}`` dict that MCP
13641368
handlers expect. This mirrors the normalization performed by
13651369
``streamable_http_auth`` so that the stateful-session fallback path in
13661370
``_get_request_context_or_default`` returns an identical shape.
@@ -1369,7 +1373,7 @@ def _normalize_jwt_payload(payload: dict[str, Any]) -> dict[str, Any]:
13691373
payload: Raw JWT payload dict from ``require_auth_header_first``.
13701374
13711375
Returns:
1372-
Canonical user context dict with keys email, teams, is_admin, is_authenticated.
1376+
Canonical user context dict with keys email, teams, is_admin, is_authenticated, token_use.
13731377
"""
13741378
email = payload.get("sub") or payload.get("email")
13751379
is_admin = payload.get("is_admin", False)
@@ -1401,6 +1405,7 @@ def _normalize_jwt_payload(payload: dict[str, Any]) -> dict[str, Any]:
14011405
"teams": final_teams,
14021406
"is_admin": is_admin,
14031407
"is_authenticated": True,
1408+
"token_use": token_use,
14041409
}
14051410
# Extract scoped permissions from JWT for per-method enforcement
14061411
scopes = payload.get("scopes") or {}
@@ -2346,9 +2351,11 @@ async def handle_streamable_http(self, scope: Scope, receive: Receive, send: Sen
23462351
# This mirrors /servers/{id}/sse and /servers/{id}/message guards.
23472352
user_context = user_context_var.get()
23482353
if match and _should_enforce_streamable_rbac(user_context):
2354+
_is_session = user_context.get("token_use") == "session" if user_context else False
23492355
has_server_access = await _check_streamable_permission(
23502356
user_context=user_context,
23512357
permission="servers.use",
2358+
check_any_team=_is_session,
23522359
)
23532360
if not has_server_access:
23542361
response = ORJSONResponse(
@@ -3014,6 +3021,7 @@ async def _auth_jwt(self, *, token: str) -> bool:
30143021
"teams": final_teams,
30153022
"is_authenticated": True,
30163023
"is_admin": is_admin,
3024+
"token_use": token_use, # propagated for downstream RBAC (check_any_team)
30173025
}
30183026
# Extract scoped permissions from JWT for per-method enforcement
30193027
jwt_scopes = user_payload.get("scopes") or {}

0 commit comments

Comments
 (0)