Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 107 additions & 11 deletions litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import asyncio
import re
from datetime import datetime, timezone
from typing import Dict, List, Optional, Set, Tuple, cast
Expand Down Expand Up @@ -126,6 +127,21 @@ def _has_client_supplied_mcp_auth(
return bool(mcp_auth_header) or bool(mcp_server_auth_headers)


MCP_ADMITTED_USER_SUBJECT_METADATA = "mcp_admitted_user_subject"
"""Marker key stamped into ``UserAPIKeyAuth.metadata`` by ``_reload_admitted_user`` for a
subject admitted keyless through the gateway session / bridge user path. It is what lets
``_team_ids_for_mcp_grant`` union the user's teams for exactly those admissions without also
broadening JWT auth, which produces a structurally identical keyless auth."""


def _is_mcp_admitted_user_subject(user_api_key_auth: UserAPIKeyAuth) -> bool:
"""True when this auth is a keyless subject admitted by the gateway session / bridge user
path (stamped at admission), as opposed to a JWT or other keyless auth that merely lacks a
``team_id``."""
metadata = user_api_key_auth.metadata
return isinstance(metadata, dict) and metadata.get(MCP_ADMITTED_USER_SUBJECT_METADATA) is True


def _is_aggregate_mcp_scope(route: str, mcp_servers: list[str] | None) -> bool:
"""True when a request targets the aggregate ``/mcp`` endpoint rather than any named
server. Named targets arrive either through ``x-mcp-servers`` (``mcp_servers``) or a
Expand Down Expand Up @@ -807,6 +823,7 @@ async def _reload_admitted_user(user_id: str) -> UserAPIKeyAuth:
org_id=user_object.organization_id,
object_permission=object_permission,
object_permission_id=user_object.object_permission_id,
metadata={MCP_ADMITTED_USER_SUBJECT_METADATA: True},
)

@staticmethod
Expand Down Expand Up @@ -1590,10 +1607,91 @@ async def _get_allowed_mcp_servers_for_key(

@staticmethod
async def _get_allowed_mcp_servers_for_team(
user_api_key_auth: Optional[UserAPIKeyAuth] = None,
) -> List[str]:
"""
Get allowed MCP servers for a team.
user_api_key_auth: UserAPIKeyAuth | None = None,
) -> list[str]:
"""
Get allowed MCP servers a caller inherits from team membership.

For a caller with a ``team_id`` (every key-based caller) the result is that one team's
grants, byte-identical to before this method learned about multiple teams. For a
subject admitted keyless through the gateway DCR session or bridge user path, a
``UserAPIKeyAuth`` can only pin one team while the user may belong to many, so the
inherited grant is the UNION across every team the user belongs to. Without this a
signed-in user would see only servers granted to them directly and none granted
through their teams, which is how servers are meant to be shared (assign teams, not
individuals). The union is gated on the admission marker
``_team_ids_for_mcp_grant`` checks, NOT on ``api_key is None``, so JWT auth (also
keyless, also possibly team-less) keeps its prior behavior and is not silently
broadened.
"""
team_ids = await MCPRequestHandler._team_ids_for_mcp_grant(user_api_key_auth)
if not team_ids:
return []
per_team = await asyncio.gather(
*(
MCPRequestHandler._allowed_mcp_servers_for_single_team(team_id, user_api_key_auth)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High: Team tool restrictions are not preserved

This grants servers from every team, but get_allowed_tools_for_server() still obtains team permissions through _get_team_object_permission(), which returns no team permission when this admitted auth has no team_id. A user can consequently call any tool on an inherited server even when the granting team limits mcp_tool_permissions; aggregate the corresponding per-team tool permissions alongside these server grants and enforce their union during tool listing and execution.

for team_id in team_ids
)
)
Comment on lines +1630 to +1635

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 asyncio.gather will propagate an uncaught exception from any team coroutine

_allowed_mcp_servers_for_single_team wraps all of its logic in a broad except Exception block, so in practice nothing escapes. However, if that guard ever develops a gap (e.g., a BaseException subclass like asyncio.CancelledError, or an error raised during the generator expression itself), the gather without return_exceptions=True will cancel remaining tasks and surface the exception to the caller, which has no error handling at this level — potentially returning a 500 to the MCP client. Using return_exceptions=True and filtering out Exception instances before the set union would make the fan-out robust regardless of what _allowed_mcp_servers_for_single_team throws.

return list({server for servers in per_team for server in servers})

@staticmethod
async def _team_ids_for_mcp_grant(user_api_key_auth: UserAPIKeyAuth | None) -> list[str]:
"""The team ids whose MCP grants a caller inherits.

A caller with an explicit ``team_id`` (every key-based caller, and any auth that pins
a team) uses that single team, so key auth is byte-identical. The fan-out to the
user's full team list happens ONLY for a subject admitted keyless through the gateway
session or bridge user path, which ``_reload_admitted_user`` stamps with
``MCP_ADMITTED_USER_SUBJECT_METADATA``. Gating on that positive marker rather than on
``api_key is None`` is deliberate: JWT auth also produces a keyless ``user_id`` auth
with no ``team_id``, and it must keep its prior behavior (no team-inherited grants)
rather than silently gaining the union across every team the user belongs to. The
``UI_TEAM_ID`` sentinel resolves to no teams exactly as before."""
if user_api_key_auth is None:
return []
if user_api_key_auth.team_id:
return [] if user_api_key_auth.team_id == UI_TEAM_ID else [user_api_key_auth.team_id]
if not user_api_key_auth.user_id or not _is_mcp_admitted_user_subject(user_api_key_auth):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High: Key metadata can forge MCP session admission

UserAPIKeyAuth.metadata contains the metadata supplied when a virtual key is created, and /key/generate permits personal keys with no team_id. A user can therefore create such a key with mcp_admitted_user_subject: true and use it to execute tools on servers inherited from every team, bypassing the key's intended team scope. Keep the positive marker check, but also require that this is actually a keyless credential.

Suggested change
if not user_api_key_auth.user_id or not _is_mcp_admitted_user_subject(user_api_key_auth):
if (
user_api_key_auth.api_key is not None
or not user_api_key_auth.user_id
or not _is_mcp_admitted_user_subject(user_api_key_auth)
):

return []
return await MCPRequestHandler._resolve_user_team_ids(user_api_key_auth.user_id, user_api_key_auth)

@staticmethod
async def _resolve_user_team_ids(user_id: str, user_api_key_auth: UserAPIKeyAuth) -> list[str]:
"""The distinct team ids a user belongs to, from the live user record. Returns [] on
no DB, a missing user, or any resolution failure so a lookup blip narrows access
rather than raising; the caller's direct grants still apply."""
from litellm.proxy.auth.auth_checks import get_user_object
from litellm.proxy.proxy_server import (
prisma_client,
proxy_logging_obj,
user_api_key_cache,
)

if prisma_client is None:
return []
try:
user_object = await get_user_object(
user_id=user_id,
prisma_client=prisma_client,
user_api_key_cache=user_api_key_cache,
user_id_upsert=False,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium: Team removal does not immediately revoke inherited access

get_user_object reads from user_api_key_cache by default. If a user's team membership is changed without evicting that cache entry, a user holding a valid gateway session can continue executing MCP tools granted by the removed team until the configurable management-object cache TTL expires. Resolve this authorization decision from the database or ensure every membership mutation invalidates the user cache.

Suggested change
user_id_upsert=False,
user_id_upsert=False,
check_db_only=True,

parent_otel_span=user_api_key_auth.parent_otel_span,
proxy_logging_obj=proxy_logging_obj,
)
except Exception as e: # noqa: BLE001 # a team-resolution blip narrows access, never raises
verbose_logger.warning(f"Failed to resolve user teams for MCP grant: {str(e)}")
return []
if user_object is None or not user_object.teams:
return []
return list(dict.fromkeys(t for t in user_object.teams if t and t != UI_TEAM_ID))
Comment on lines +1660 to +1687

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Extra get_user_object call on every keyless MCP request

For a keyless user-subject caller this path fires get_user_object to look up team membership. The user was already fetched and placed in user_api_key_cache during the upstream admission check, so a warm-cache hit is cheap. On a cold-cache miss, however, this adds a second DB round-trip per request specifically in the MCP auth path, on top of the N parallel get_team_object calls triggered afterward. If UserAPIKeyAuth is ever extended to carry the user's team list (it already carries user_id), this fetch could be eliminated; for now, worth noting the per-request cost for deployments with a large team count per user.

Rule Used: What: Avoid creating new database requests or Rout... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +1673 to +1687

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Exception fallback path is untested

The except Exception branch in _resolve_user_team_ids silently returns [] on any DB failure, narrowing the user's team-inherited access to nothing. This is the explicitly documented fail-safe behavior, but there is no test in TestUserSubjectTeamUnion that exercises this path (e.g., get_user_object raising or returning a network error). A test that patches get_user_object to raise and asserts the result is [] would pin this contract so future exception handling changes cannot accidentally widen access or surface unhandled errors.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


@staticmethod
async def _allowed_mcp_servers_for_single_team(
team_id: str,
user_api_key_auth: UserAPIKeyAuth | None,
) -> list[str]:
"""Allowed MCP servers granted by ONE team.

Unions two sources:
- Legacy team.object_permission (mcp_servers, mcp_access_groups,
Expand All @@ -1617,17 +1715,15 @@ async def _get_allowed_mcp_servers_for_team(
user_api_key_cache,
)

if user_api_key_auth is None or not user_api_key_auth.team_id or prisma_client is None:
if not team_id or team_id == UI_TEAM_ID or prisma_client is None:
return []

if user_api_key_auth.team_id == UI_TEAM_ID:
return []

team_obj: Optional[LiteLLM_TeamTable] = await get_team_object(
team_id=user_api_key_auth.team_id,
parent_otel_span = user_api_key_auth.parent_otel_span if user_api_key_auth is not None else None
team_obj: LiteLLM_TeamTable | None = await get_team_object(
team_id=team_id,
prisma_client=prisma_client,
user_api_key_cache=user_api_key_cache,
parent_otel_span=user_api_key_auth.parent_otel_span,
parent_otel_span=parent_otel_span,
proxy_logging_obj=proxy_logging_obj,
)
if team_obj is None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6218,3 +6218,119 @@ async def test_arm_does_not_fire_for_named_server(self):
with pytest.raises((HTTPException, ProxyException)):
await MCPRequestHandler.process_mcp_request(self._scope(token, path="/mcp/github"))
mock_auth.assert_called_once()


@pytest.mark.asyncio
class TestUserSubjectTeamUnion:
"""_get_allowed_mcp_servers_for_team unions across ALL a user's teams for a keyless
user-subject caller (the gateway DCR session bearer and bridge user-envelope), while a
key-based caller keeps its single-team behavior byte-identically."""

def _team(self, team_id, mcp_servers):
from litellm.proxy._types import LiteLLM_ObjectPermissionTable, LiteLLM_TeamTable

return LiteLLM_TeamTable(
team_id=team_id,
access_group_ids=[],
object_permission=LiteLLM_ObjectPermissionTable(
object_permission_id=f"op-{team_id}", mcp_servers=mcp_servers
),
)

@contextlib.contextmanager
def _patch(self, *, teams_by_id, user_teams=None):
async def _get_team_object(team_id, **kw):
return teams_by_id.get(team_id)

async def _get_user_object(user_id, **kw):
return MagicMock(user_id=user_id, teams=user_teams or [])

with (
patch("litellm.proxy.auth.auth_checks.get_team_object", _get_team_object),
patch("litellm.proxy.auth.auth_checks.get_user_object", _get_user_object),
patch("litellm.proxy.auth.auth_checks._get_mcp_server_ids_from_access_groups", AsyncMock(return_value=[])),
patch("litellm.proxy.proxy_server.prisma_client", MagicMock()),
patch("litellm.proxy.proxy_server.user_api_key_cache", MagicMock()),
patch("litellm.proxy.proxy_server.proxy_logging_obj", MagicMock()),
):
yield

@staticmethod
def _admitted_subject(user_id):
from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import (
MCP_ADMITTED_USER_SUBJECT_METADATA,
)

return UserAPIKeyAuth(user_id=user_id, api_key=None, metadata={MCP_ADMITTED_USER_SUBJECT_METADATA: True})

async def test_keyless_user_unions_servers_across_all_their_teams(self):
teams = {"team-a": self._team("team-a", ["srv1", "srv2"]), "team-b": self._team("team-b", ["srv2", "srv3"])}
auth = self._admitted_subject("sso-user")
with self._patch(teams_by_id=teams, user_teams=["team-a", "team-b"]):
result = await MCPRequestHandler._get_allowed_mcp_servers_for_team(auth)
assert set(result) == {"srv1", "srv2", "srv3"}

async def test_key_based_caller_uses_single_team_only(self):
"""A key-based caller (api_key set) with a team_id sees ONLY that team, even though the
same user belongs to other teams: key auth must be byte-identical to before."""
teams = {"team-a": self._team("team-a", ["srv1"]), "team-b": self._team("team-b", ["srv2", "srv3"])}
auth = UserAPIKeyAuth(user_id="sso-user", api_key="sk-hash", team_id="team-a")
with self._patch(teams_by_id=teams, user_teams=["team-a", "team-b"]):
result = await MCPRequestHandler._get_allowed_mcp_servers_for_team(auth)
assert set(result) == {"srv1"}

async def test_keyless_user_with_explicit_team_id_uses_that_team_only(self):
"""A keyless caller that already pins a team_id (not the user-subject fan-out shape)
resolves only that team; the union is strictly for the no-team-id user-subject case."""
teams = {"team-a": self._team("team-a", ["srv1"]), "team-b": self._team("team-b", ["srv2"])}
auth = UserAPIKeyAuth(user_id="sso-user", api_key=None, team_id="team-a")
with self._patch(teams_by_id=teams, user_teams=["team-a", "team-b"]):
result = await MCPRequestHandler._get_allowed_mcp_servers_for_team(auth)
assert set(result) == {"srv1"}

async def test_keyless_user_with_no_teams_gets_nothing_from_teams(self):
auth = self._admitted_subject("lonely-user")
with self._patch(teams_by_id={}, user_teams=[]):
result = await MCPRequestHandler._get_allowed_mcp_servers_for_team(auth)
assert result == []

async def test_ui_session_team_id_still_resolves_to_nothing(self):
from litellm.proxy._types import UI_TEAM_ID

auth = UserAPIKeyAuth(user_id="dash-user", api_key="sk-hash", team_id=UI_TEAM_ID)
with self._patch(teams_by_id={}, user_teams=["team-a"]):
result = await MCPRequestHandler._get_allowed_mcp_servers_for_team(auth)
assert result == []

async def test_team_ids_helper_gates_on_shape(self):
from litellm.proxy._types import UI_TEAM_ID

# key-based with team -> that team
assert await MCPRequestHandler._team_ids_for_mcp_grant(
UserAPIKeyAuth(api_key="sk", team_id="t1", user_id="u")
) == ["t1"]
# keyless subject admitted by the gateway/bridge path (marked), no team -> resolved from record
with self._patch(teams_by_id={}, user_teams=["t2", "t3"]):
assert await MCPRequestHandler._team_ids_for_mcp_grant(self._admitted_subject("u")) == ["t2", "t3"]
# keyless, no user_id -> nothing
assert await MCPRequestHandler._team_ids_for_mcp_grant(UserAPIKeyAuth(api_key=None)) == []
# keyless with a user_id but NOT admission-marked (JWT auth) -> nothing (unchanged behavior)
with self._patch(teams_by_id={}, user_teams=["t2", "t3"]):
assert await MCPRequestHandler._team_ids_for_mcp_grant(
UserAPIKeyAuth(api_key=None, user_id="jwt-user")
) == []
# UI sentinel -> nothing
assert await MCPRequestHandler._team_ids_for_mcp_grant(
UserAPIKeyAuth(api_key="sk", team_id=UI_TEAM_ID, user_id="u")
) == []

async def test_jwt_keyless_user_without_team_claim_does_not_union(self):
"""Regression for the review finding: a JWT-authenticated caller is also keyless with a
user_id and (with no team claim) no team_id, but it is NOT admission-marked, so it must
keep its prior behavior of inheriting no team grants rather than silently gaining the
union across every team the user belongs to."""
teams = {"team-a": self._team("team-a", ["srv1"]), "team-b": self._team("team-b", ["srv2"])}
jwt_auth = UserAPIKeyAuth(user_id="jwt-user", api_key=None) # no admission marker
with self._patch(teams_by_id=teams, user_teams=["team-a", "team-b"]):
result = await MCPRequestHandler._get_allowed_mcp_servers_for_team(jwt_auth)
assert result == []
Loading