diff --git a/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py b/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py index c1248cafac5..7d85f3c4908 100644 --- a/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py +++ b/litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py @@ -39,6 +39,7 @@ from litellm.proxy._types import ( SpecialMCPServerName, SpecialMCPServerNames, UserAPIKeyAuth, + user_api_key_has_admin_view, ) from litellm.proxy.auth.ip_address_utils import IPAddressUtils from litellm.proxy.auth.user_api_key_auth import ( @@ -1785,11 +1786,14 @@ class MCPRequestHandler: global_mcp_server_manager, ) - # An OPEN channel (allow_all_keys, the user's own BYOM) makes the server REACHABLE through the - # user, though no grant source names it — without this the union returns [], listable but - # uninvokable. Reachability is ALL it confers, NOT a ceiling waiver: the user's own - # mcp_tool_permissions and org tool ceiling still bind, exactly as a key's do on an allow_all server. - reachable_via_open_channel: Final = server_id in await global_mcp_server_manager.operator_open_server_ids(auth) + # An OPEN channel (allow_all_keys, the user's own BYOM, an unscoped admin-view role) makes the + # server REACHABLE through the user, though no grant source names it — without this the union + # returns [], listable but uninvokable. Reachability is ALL it confers, NOT a ceiling waiver: + # the user's own mcp_tool_permissions and org tool ceiling still bind, exactly as a key's do + # on an allow_all server or an admin key's do on any server. + reachable_via_open_channel: Final = server_id in await global_mcp_server_manager.operator_open_server_ids( + auth + ) or await MCPRequestHandler.admin_view_unscoped(auth) allowed: Final[set[str]] = set() for source, granted in await MCPRequestHandler.admitted_source_grants(auth): @@ -2723,6 +2727,32 @@ class MCPRequestHandler: entitled_servers: Final = await MCPRequestHandler._get_allowed_mcp_servers_for_user(user_api_key_auth) return entitled_servers is None or len(entitled_servers) > 0 + @staticmethod + async def admin_view_unscoped(user_api_key_auth: UserAPIKeyAuth | None = None) -> bool: + """Whether this principal's admin-view role grants the unscoped MCP resolution, whatever + credential carries it (admin key, dashboard session, or OAuth-admitted session subject). + + Two bounds disqualify, one per ownership of the row. A CREDENTIAL's explicit + ``object_permission.mcp_servers`` scope wins even for admins, including the empty list. An + admitted subject's object_permission is the user's own row, whose ``mcp_servers`` column is + [] by DB default, so for that shape the row binds through the entitlement ceiling instead + (any non-empty entitlement, or an unresolved one, disqualifies), exactly as + ``operator_open_server_ids`` reads the same row. The one owner of this predicate: the + server-axis registry resolution in ``get_allowed_mcp_servers`` and the tools-axis open + channel in ``_resolve_admitted_subject_tools`` both consult it, so the two axes cannot + disagree.""" + if user_api_key_auth is None or not user_api_key_has_admin_view(user_api_key_auth): + return False + object_permission: Final = user_api_key_auth.object_permission + credential_scoped: Final = ( + not _is_mcp_admitted_user_subject(user_api_key_auth) + and object_permission is not None + and object_permission.mcp_servers is not None + ) + if credential_scoped: + return False + return not await MCPRequestHandler._user_places_mcp_ceiling(user_api_key_auth) + @staticmethod async def _apply_user_tool_ceiling( allowed_tools: Sequence[str] | None, diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index dbe97dd5bce..7ab26db0f3e 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -2943,17 +2943,14 @@ class MCPServerManager: 2. If admin and no object_permission, return all servers 3. Otherwise, use standard permission checks """ - from litellm.proxy.management_endpoints.common_utils import _user_has_admin_view - allow_all_server_ids: Final = self.get_allow_all_keys_server_ids() # A keyless admitted subject is resolved per grant source, and channel decisions that are # absolute for a scoped KEY credential are not absolute for it: its own opt-out silences its - # own source (handled per source in the resolver), never its teams' grants, and its admin - # role does not swallow the grant model — a session bearer is a third-party client - # credential, not the dashboard, so an admin signing in through the connect flow gets their - # grants like anyone else rather than handing the client the full registry ahead of every - # per-team org ceiling. + # own source (handled per source in the resolver), never its teams' grants. Its admin role + # rides the HUMAN, not the credential: an admin's session resolves the same registry their + # dashboard shows (connect-page parity), bounded like an admin key by explicit + # object_permission scope, the entitlement ceiling, and the session resource scope below. is_admitted_subject: Final = _is_mcp_admitted_user_subject(user_api_key_auth) # The key explicitly opted out of every MCP server. Return zero before @@ -2982,26 +2979,16 @@ class MCPServerManager: ) try: - # If admin but NO explicit object permission, get all servers (never for an admitted - # subject — see is_admitted_subject above) - if ( - user_api_key_auth - and not is_admitted_subject - and _user_has_admin_view(user_api_key_auth) - and not has_explicit_object_permission - # An entitlement attached to the HUMAN binds them whatever their role: it is the - # person's scope, not the credential's, so an admin role is not a waiver of it. An - # UNRESOLVED entitlement also skips the shortcut, so the resolver denies rather than - # handing over the whole registry on a transient fault. - and not await MCPRequestHandler._user_places_mcp_ceiling(user_api_key_auth) - ): - verbose_logger.debug("Admin user without explicit object_permission - returning all servers") - return list(self.get_registry().keys()) - - # Get allowed servers from object permissions (respects object_permission even for admins) - allowed_mcp_servers: Final = await MCPRequestHandler.get_allowed_mcp_servers(user_api_key_auth) - verbose_logger.debug("Allowed MCP Servers for user api key auth: %s", allowed_mcp_servers) - combined_servers: Final = set(allowed_mcp_servers) + # Admin view with no explicit object permission and no entitlement ceiling resolves the + # whole registry, for keys AND admitted session subjects alike (one predicate owns the + # question). Seeded into the union rather than returned early so the session resource + # scope below still bounds a per-server envelope held by an admin. + combined_servers: Final = ( + set(self.get_registry().keys()) + if await MCPRequestHandler.admin_view_unscoped(user_api_key_auth) + else set(await MCPRequestHandler.get_allowed_mcp_servers(user_api_key_auth)) + ) + verbose_logger.debug("Allowed MCP Servers for user api key auth: %s", combined_servers) combined_servers.update( await self.operator_open_server_ids( user_api_key_auth, diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py b/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py index 3bd615a6a33..17c4d773981 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py @@ -6430,9 +6430,7 @@ class TestAggregateGatewayDcrChallenge: assert _gateway_dcr_challenge_target("/mcp/srv", None, None) == expected, resolved assert _gateway_dcr_challenge_target("/mcp/a,b", None, None) is None assert _gateway_dcr_challenge_target("/mcp", None, None) is None - with patch( - "litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager" - ) as mock_mgr: + with patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager") as mock_mgr: mock_mgr.get_mcp_server_by_name.return_value = _server(MCPAuth.oauth2) assert _gateway_dcr_challenge_target("/mcp/srv", ["other"], None) is None @@ -7073,25 +7071,109 @@ class TestUserSubjectTeamUnion: assert await manager.operator_open_server_ids(admitted) == {"srv-byom"} assert await manager.operator_open_server_ids(scoped_key) == set(), "explicit key scope still suppresses BYOM" - async def test_admitted_admin_is_scoped_to_grants_not_full_registry(self): - """The wrapper's admin short-circuit hands the FULL registry to any admin-role auth before - the grant union or the per-team org ceilings run. A session bearer is a third-party client - credential, not the dashboard: an admin signing in through the connect flow gets their - grants like anyone else. A real admin key keeps the dashboard behavior unchanged.""" + @pytest.mark.parametrize( + "role", ["PROXY_ADMIN", "PROXY_ADMIN_VIEW_ONLY"], ids=["proxy_admin", "proxy_admin_view_only"] + ) + async def test_admitted_admin_gets_registry_like_an_admin_key(self, role): + """Connect-page parity: admin view rides the HUMAN, not the credential. An admitted session + subject with an admin-view role resolves the same full registry an admin KEY does, so the + servers the dashboard shows an admin are the servers their OAuth session serves. Regression + pin for the customer report where an admin's Claude Code session showed zero tools.""" + from litellm.proxy._types import LitellmUserRoles + + manager = self._manager_with(["srv-granted", "srv-secret"]) + admitted = _make_admitted_subject("admin-user") + admitted.user_role = LitellmUserRoles[role] + key_admin = UserAPIKeyAuth(user_id="admin-user", api_key="sk-hash", user_role=LitellmUserRoles[role]) + with patch.object(MCPRequestHandler, "get_allowed_mcp_servers", AsyncMock(return_value=["srv-granted"])): + admitted_view = set(await manager.get_allowed_mcp_servers(admitted)) + key_admin_view = set(await manager.get_allowed_mcp_servers(key_admin)) + assert admitted_view == {"srv-granted", "srv-secret"}, "an admitted admin resolves the registry" + assert key_admin_view == admitted_view, "session and key admin views must be identical" + + async def test_admitted_admin_explicit_scope_still_wins(self): + """An admin whose own user row names servers is entitlement-bound whatever their role: the + row binds through the ceiling for an admitted subject (a user row's mcp_servers is the + human's grant list, not a credential scope), so the registry seed must not fire. A KEY + carrying an explicit scope disqualifies directly, empty list included.""" + from litellm.proxy._types import LiteLLM_ObjectPermissionTable, LitellmUserRoles + + manager = self._manager_with(["srv-granted", "srv-secret"]) + admitted = _make_admitted_subject("admin-user", own_servers=["srv-granted"]) + admitted.user_role = LitellmUserRoles.PROXY_ADMIN + with ( + patch.object( + MCPRequestHandler, "_get_allowed_mcp_servers_for_user", AsyncMock(return_value=["srv-granted"]) + ), + patch.object(MCPRequestHandler, "get_allowed_mcp_servers", AsyncMock(return_value=["srv-granted"])), + ): + assert set(await manager.get_allowed_mcp_servers(admitted)) == {"srv-granted"} + + scoped_key = UserAPIKeyAuth( + user_id="admin-user", + api_key="sk-hash", + user_role=LitellmUserRoles.PROXY_ADMIN, + object_permission=LiteLLM_ObjectPermissionTable(object_permission_id="op-k", mcp_servers=[]), + ) + with patch.object(MCPRequestHandler, "get_allowed_mcp_servers", AsyncMock(return_value=[])): + assert await manager.get_allowed_mcp_servers(scoped_key) == [] + + async def test_admitted_admin_db_default_empty_scope_still_gets_registry(self): + """The admitted subject's object_permission is the user's own row, whose mcp_servers column + is [] by DB default whenever the row exists for any other field: default noise, never an + explicit scope. The registry seed must fire through it, or every admin with a shared + permission row keeps resolving zero servers while their dashboard shows all of them.""" + from litellm.proxy._types import LiteLLM_ObjectPermissionTable, LitellmUserRoles + + manager = self._manager_with(["srv-granted", "srv-secret"]) + admitted = _make_admitted_subject("admin-user") + admitted.user_role = LitellmUserRoles.PROXY_ADMIN + admitted.object_permission = LiteLLM_ObjectPermissionTable(object_permission_id="op-u", mcp_servers=[]) + with patch.object(MCPRequestHandler, "get_allowed_mcp_servers", AsyncMock(return_value=[])): + assert set(await manager.get_allowed_mcp_servers(admitted)) == {"srv-granted", "srv-secret"} + + async def test_non_admin_admitted_subject_never_gets_registry(self): + """The negative control for the registry seed: a plain admitted subject with no admin-view + role resolves only their grant union, however many servers the registry holds.""" + manager = self._manager_with(["srv-granted", "srv-secret"]) + plain = _make_admitted_subject("plain-user") + with patch.object(MCPRequestHandler, "get_allowed_mcp_servers", AsyncMock(return_value=["srv-granted"])): + assert set(await manager.get_allowed_mcp_servers(plain)) == {"srv-granted"} + + async def test_admitted_admin_entitlement_ceiling_disables_registry(self): + """An entitlement ceiling, including an UNRESOLVED one, binds the human whatever their role: + the registry seed must not fire on a transient fault, and the grant union answers instead.""" from litellm.proxy._types import LitellmUserRoles manager = self._manager_with(["srv-granted", "srv-secret"]) admitted = _make_admitted_subject("admin-user") admitted.user_role = LitellmUserRoles.PROXY_ADMIN - with patch.object(MCPRequestHandler, "get_allowed_mcp_servers", AsyncMock(return_value=["srv-granted"])): - admitted_view = set(await manager.get_allowed_mcp_servers(admitted)) - key_admin_view = set( - await manager.get_allowed_mcp_servers( - UserAPIKeyAuth(user_id="admin-user", api_key="sk-hash", user_role=LitellmUserRoles.PROXY_ADMIN) - ) - ) - assert admitted_view == {"srv-granted"}, "an admitted admin gets their grants, not the registry" - assert key_admin_view == {"srv-granted", "srv-secret"}, "admin KEY behavior must be unchanged" + with ( + patch.object(MCPRequestHandler, "_get_allowed_mcp_servers_for_user", AsyncMock(return_value=None)), + patch.object(MCPRequestHandler, "get_allowed_mcp_servers", AsyncMock(return_value=["srv-granted"])), + ): + assert set(await manager.get_allowed_mcp_servers(admitted)) == {"srv-granted"} + + async def test_admitted_admin_tools_ride_own_source_on_ungranted_server(self): + """Admin view is an open channel on the tools axis too: the user's OWN source resolves the + tools for a server no grant names, so an admin session's registry-wide servers are invokable + rather than listable-but-uninvokable. A non-admin subject on the same server stays denied. + An admin whose row carries any entitlement never reaches this channel: the ceiling clause + disqualifies the predicate first, so their own tool permissions keep binding on the grants path.""" + from litellm.proxy._types import LitellmUserRoles + + admin = _make_admitted_subject("admin-user") + admin.user_role = LitellmUserRoles.PROXY_ADMIN + plain = _make_admitted_subject("plain-user") + with self._patch(teams_by_id={}, user_teams=[]): + with patch( + "litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager.operator_open_server_ids", + AsyncMock(return_value=set()), + ): + admin_tools = await MCPRequestHandler.get_allowed_tools_for_server("srv-any", admin) + plain_tools = await MCPRequestHandler.get_allowed_tools_for_server("srv-any", plain) + assert admin_tools is None, "admin channel resolves allow-all through the user's own source" + assert plain_tools == [], "a non-admin subject with no granting source stays denied" async def test_admitted_opt_out_via_wrapper_keeps_team_servers(self): """The wrapper's no_mcp_servers early-return is a KEY rule (a scoped credential's opt-out is diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index 5ee8143fb8e..09e8c78a3f8 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -4846,7 +4846,9 @@ class TestMCPServerManager: @staticmethod def _manager_with_deepwiki_and_huggingface() -> MCPServerManager: manager = MCPServerManager() - deepwiki = MCPServer(server_id="deepwiki-id", name="deepwiki", server_name="deepwiki", transport=MCPTransport.http) + deepwiki = MCPServer( + server_id="deepwiki-id", name="deepwiki", server_name="deepwiki", transport=MCPTransport.http + ) huggingface = MCPServer( server_id="huggingface-id", name="huggingface", server_name="huggingface", transport=MCPTransport.http ) @@ -4867,8 +4869,14 @@ class TestMCPServerManager: with pytest.raises(ValueError, match="Tool hub_repo_search not found"): manager._resolve_mcp_server_for_tool_call("deepwiki", "hub_repo_search") - assert manager._resolve_mcp_server_for_tool_call("deepwiki", "read_wiki_structure") is manager.registry["deepwiki-id"] - assert manager._resolve_mcp_server_for_tool_call("huggingface", "hub_repo_search") is manager.registry["huggingface-id"] + assert ( + manager._resolve_mcp_server_for_tool_call("deepwiki", "read_wiki_structure") + is manager.registry["deepwiki-id"] + ) + assert ( + manager._resolve_mcp_server_for_tool_call("huggingface", "hub_repo_search") + is manager.registry["huggingface-id"] + ) def test_get_mcp_server_from_tool_name_rejects_other_servers_prefix(self): manager = self._manager_with_deepwiki_and_huggingface() @@ -4876,7 +4884,9 @@ class TestMCPServerManager: assert manager._get_mcp_server_from_tool_name("huggingface-read_wiki_structure") is None assert manager._get_mcp_server_from_tool_name("deepwiki-hub_repo_search") is None assert manager._get_mcp_server_from_tool_name("deepwiki-read_wiki_structure") is manager.registry["deepwiki-id"] - assert manager._get_mcp_server_from_tool_name("huggingface-hub_repo_search") is manager.registry["huggingface-id"] + assert ( + manager._get_mcp_server_from_tool_name("huggingface-hub_repo_search") is manager.registry["huggingface-id"] + ) def test_resolve_mcp_server_for_tool_call_shared_bare_name_resolves_via_own_prefixed_spelling(self): manager = MCPServerManager() @@ -10500,6 +10510,39 @@ class TestSessionResourceScopeIntersect: assert MCPServerManager._admitted_session_resource_scope(self._admitted_auth("b")) == "b" + @pytest.mark.asyncio + async def test_admin_registry_seed_still_bounded_by_session_resource_scope(self): + """The admin-view registry seed flows through the same scoped exit as every union: a + session envelope sealed to one server never widens past it, even held by an admin whose + role resolves the whole registry. Pin for the connect-page-parity change; without the + single-exit shape, the old early return would hand a per-server bearer the registry.""" + from unittest.mock import AsyncMock, patch + + from litellm.proxy._experimental.mcp_server.mcp_server_manager import MCPServerManager + from litellm.proxy._types import LitellmUserRoles + from litellm.types.mcp import MCPTransport + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + manager = MCPServerManager() + for sid in ("granted-id", "other-id"): + manager.registry[sid] = MCPServer( + server_id=sid, name=sid, server_name=sid, url="https://example.com/mcp", transport=MCPTransport.http + ) + auth = self._admitted_auth("granted-id") + auth.user_role = LitellmUserRoles.PROXY_ADMIN + with ( + patch.object(MCPServerManager, "get_allow_all_keys_server_ids", return_value=[]), + patch.object( + MCPServerManager, + "_get_active_submitted_mcp_server_ids_for_user", + new_callable=AsyncMock, + return_value=[], + ), + ): + assert await manager.get_allowed_mcp_servers(auth) == ["granted-id"] + auth.mcp_session_resource_server_id = None + assert set(await manager.get_allowed_mcp_servers(auth)) == {"granted-id", "other-id"} + @pytest.mark.asyncio async def test_get_allowed_mcp_servers_scopes_past_operator_open_union(self): """The intersect applies AFTER the operator-open (allow_all_keys) union, so a scoped @@ -10519,7 +10562,12 @@ class TestSessionResourceScopeIntersect: new_callable=AsyncMock, return_value=["granted-id", "other-id"], ), - patch.object(MCPServerManager, "_get_active_submitted_mcp_server_ids_for_user", new_callable=AsyncMock, return_value=[]), + patch.object( + MCPServerManager, + "_get_active_submitted_mcp_server_ids_for_user", + new_callable=AsyncMock, + return_value=[], + ), ): allowed = await manager.get_allowed_mcp_servers(auth) assert allowed == ["granted-id"] @@ -10531,7 +10579,12 @@ class TestSessionResourceScopeIntersect: new_callable=AsyncMock, side_effect=RuntimeError("resolver down"), ), - patch.object(MCPServerManager, "_get_active_submitted_mcp_server_ids_for_user", new_callable=AsyncMock, return_value=[]), + patch.object( + MCPServerManager, + "_get_active_submitted_mcp_server_ids_for_user", + new_callable=AsyncMock, + return_value=[], + ), ): fallback = await manager.get_allowed_mcp_servers(auth) assert fallback == ["granted-id"] @@ -10645,9 +10698,7 @@ class TestClientForwardedDiscoveryFailureIsNotFatal: @pytest.mark.parametrize("auth_type", [MCPAuth.true_passthrough, MCPAuth.oauth_delegate]) @pytest.mark.asyncio - async def test_client_forwarded_servers_keep_discovering_their_front_door_endpoints( - self, auth_type: MCPAuthType - ): + async def test_client_forwarded_servers_keep_discovering_their_front_door_endpoints(self, auth_type: MCPAuthType): """Exempting these modes from the FAILURE must not exempt them from discovery itself. ``/authorize``, ``/token`` and ``/register`` read the discovered endpoints for these servers