fix(mcp): highlight MCP cards red when the logged-in user is missing per-user env vars (#29856)
* fix(mcp): flag missing per-user env vars on the card for every accessible server The dashboard MCP card grid lists servers via the registry-backed manager (get_all_mcp_servers_unfiltered for admins in view_all mode, the allowed-context aggregation otherwise), but the per-user env-var status endpoint that drives the red "user fields missing" highlight resolved servers through the much narrower get_all_mcp_servers_for_user, which only returns servers explicitly granted on the calling key. An admin's dashboard session key carries no per-server MCP grant, so the status feed came back empty and the card never turned red even when the logged-in user had not filled in their required variables. Both surfaces now share a single _resolve_accessible_mcp_servers helper, so the status feed is computed over exactly the cards the user sees. The helper returns servers unredacted; the status endpoint needs the raw env_vars and still only ever reports is_set booleans, never the stored secret values. * test(mcp): drop dead get_all_mcp_servers_for_user patch from view_all regression test The bulk status endpoint resolves servers through _resolve_accessible_mcp_servers now, so the old get_all_mcp_servers_for_user patch in the admin view_all regression test is never hit. Removing it keeps the test honest about which code path it exercises.
This commit is contained in:
Mateo Wang
GitHub
parent
d61f7747c0
commit
aeb55e7a11
@ -879,6 +879,32 @@ if MCP_AVAILABLE:
|
||||
|
||||
return _redact_mcp_credentials_list(servers)
|
||||
|
||||
async def _resolve_accessible_mcp_servers(
|
||||
user_api_key_dict: UserAPIKeyAuth,
|
||||
) -> List[LiteLLM_MCPServerTable]:
|
||||
"""The server set the dashboard grid shows (GET /v1/mcp/server, no team
|
||||
filter), returned unredacted. Callers that surface this to a client must
|
||||
apply their own redaction; the per-user env-var status endpoint relies on
|
||||
the raw env_vars and only ever returns is_set booleans, never secrets.
|
||||
|
||||
Sharing this resolution keeps the red "missing user fields" card status
|
||||
aligned with the cards actually rendered: an admin in view_all mode sees
|
||||
every server even when their key carries no per-server MCP grant.
|
||||
"""
|
||||
if (
|
||||
_get_user_mcp_management_mode() == "view_all"
|
||||
and not _is_restricted_virtual_key_request(user_api_key_dict)
|
||||
):
|
||||
return await global_mcp_server_manager.get_all_mcp_servers_unfiltered()
|
||||
|
||||
aggregated: Dict[str, LiteLLM_MCPServerTable] = {}
|
||||
for auth_context in await build_effective_auth_contexts(user_api_key_dict):
|
||||
for server in await global_mcp_server_manager.get_all_allowed_mcp_servers(
|
||||
user_api_key_auth=auth_context
|
||||
):
|
||||
aggregated.setdefault(server.server_id, server)
|
||||
return list(aggregated.values())
|
||||
|
||||
@router.get(
|
||||
"/server",
|
||||
description="Returns the mcp server list with associated teams",
|
||||
@ -950,30 +976,8 @@ if MCP_AVAILABLE:
|
||||
sanitized_team_id
|
||||
)
|
||||
else:
|
||||
user_mcp_management_mode = _get_user_mcp_management_mode()
|
||||
|
||||
if user_mcp_management_mode == "view_all" and not is_restricted_virtual_key:
|
||||
servers = (
|
||||
await global_mcp_server_manager.get_all_mcp_servers_unfiltered()
|
||||
)
|
||||
redacted_mcp_servers = _redact_mcp_credentials_list(servers)
|
||||
else:
|
||||
auth_contexts = await build_effective_auth_contexts(user_api_key_dict)
|
||||
|
||||
aggregated_servers: Dict[str, LiteLLM_MCPServerTable] = {}
|
||||
for auth_context in auth_contexts:
|
||||
servers = (
|
||||
await global_mcp_server_manager.get_all_allowed_mcp_servers(
|
||||
user_api_key_auth=auth_context
|
||||
)
|
||||
)
|
||||
for server in servers:
|
||||
if server.server_id not in aggregated_servers:
|
||||
aggregated_servers[server.server_id] = server
|
||||
|
||||
redacted_mcp_servers = _redact_mcp_credentials_list(
|
||||
aggregated_servers.values()
|
||||
)
|
||||
servers = await _resolve_accessible_mcp_servers(user_api_key_dict)
|
||||
redacted_mcp_servers = _redact_mcp_credentials_list(servers)
|
||||
|
||||
# augment the mcp servers with public status
|
||||
if litellm.public_mcp_servers is not None:
|
||||
@ -2391,9 +2395,7 @@ if MCP_AVAILABLE:
|
||||
user_id = user_api_key_dict.user_id or ""
|
||||
if not user_id:
|
||||
return []
|
||||
accessible = await get_all_mcp_servers_for_user(
|
||||
prisma_client, user_api_key_dict
|
||||
)
|
||||
accessible = await _resolve_accessible_mcp_servers(user_api_key_dict)
|
||||
if not accessible:
|
||||
return []
|
||||
server_ids = [s.server_id for s in accessible]
|
||||
|
||||
@ -4146,7 +4146,7 @@ class TestListMCPUserEnvVarStatus:
|
||||
),
|
||||
patch.object(
|
||||
mgmt_endpoints,
|
||||
"get_all_mcp_servers_for_user",
|
||||
"_resolve_accessible_mcp_servers",
|
||||
AsyncMock(return_value=[]),
|
||||
),
|
||||
):
|
||||
@ -4174,7 +4174,7 @@ class TestListMCPUserEnvVarStatus:
|
||||
),
|
||||
patch.object(
|
||||
mgmt_endpoints,
|
||||
"get_all_mcp_servers_for_user",
|
||||
"_resolve_accessible_mcp_servers",
|
||||
AsyncMock(return_value=[server_with, server_without]),
|
||||
),
|
||||
patch.object(
|
||||
@ -4204,7 +4204,7 @@ class TestListMCPUserEnvVarStatus:
|
||||
),
|
||||
patch.object(
|
||||
mgmt_endpoints,
|
||||
"get_all_mcp_servers_for_user",
|
||||
"_resolve_accessible_mcp_servers",
|
||||
AsyncMock(return_value=[server]),
|
||||
),
|
||||
patch.object(
|
||||
@ -4221,6 +4221,51 @@ class TestListMCPUserEnvVarStatus:
|
||||
assert by_name["CORP_PASSWORD"].is_set is False
|
||||
assert "alice" not in result[0].model_dump_json()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_admin_view_all_flags_missing_fields_without_key_grants(self):
|
||||
"""Regression: the red "user fields missing" card must light up for an
|
||||
admin in view_all mode even when their key carries no per-server MCP
|
||||
grant. The bulk status feed has to resolve the same server set the
|
||||
dashboard grid renders; the old narrow key-scoped listing returned
|
||||
nothing for such an admin, leaving every card un-highlighted."""
|
||||
server = _make_env_var_server(
|
||||
server_id="srv-with",
|
||||
env_vars=_ENV_VARS_MIXED,
|
||||
static_headers=_STATIC_HEADERS_MIXED,
|
||||
)
|
||||
with (
|
||||
patch.object(
|
||||
mgmt_endpoints, "get_prisma_client_or_throw", return_value=MagicMock()
|
||||
),
|
||||
patch.object(
|
||||
mgmt_endpoints,
|
||||
"_get_user_mcp_management_mode",
|
||||
return_value="view_all",
|
||||
),
|
||||
patch.object(
|
||||
mgmt_endpoints.global_mcp_server_manager,
|
||||
"get_all_mcp_servers_unfiltered",
|
||||
AsyncMock(return_value=[server]),
|
||||
),
|
||||
patch.object(
|
||||
mgmt_endpoints,
|
||||
"get_user_env_vars_bulk",
|
||||
AsyncMock(return_value={}),
|
||||
),
|
||||
):
|
||||
result = await mgmt_endpoints.list_mcp_user_env_var_status(
|
||||
user_api_key_dict=generate_mock_user_api_key_auth(
|
||||
user_id="admin",
|
||||
user_role=LitellmUserRoles.PROXY_ADMIN,
|
||||
)
|
||||
)
|
||||
assert [s.server_id for s in result] == ["srv-with"]
|
||||
assert result[0].missing_count == 2
|
||||
assert {f.name for f in result[0].required} == {
|
||||
"CORP_USERNAME",
|
||||
"CORP_PASSWORD",
|
||||
}
|
||||
|
||||
|
||||
class TestMCPUserEnvVarsAccessControl:
|
||||
"""Per-server env-var endpoints must enforce the same access gate as
|
||||
|
||||
Loading…
Reference in New Issue
Block a user