mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-09 03:18:44 +00:00
fix(mcp): byom visibility, preview UX, and admin settings gating (#31809)
* fix(ui): show info message when MCP tool preview returns 403
Internal users submitting MCP servers hit an admin-only preview endpoint; replace the red connection error with a clear review notice while leaving other failures unchanged.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(mcp): let BYOM submitters see their approved servers
Approved user-submitted MCP servers defaulted to no access groups and allow_all_keys=false, so submitters could not see them after admin approval. Grant creator visibility for active submissions in get_allowed_mcp_servers.
Co-authored-by: Cursor <cursoragent@cursor.com>
* Improve dialogue box
* fix(security): restrict MCP semantic filter settings to proxy admins
Add an explicit PROXY_ADMIN check on PATCH /update/mcp_semantic_filter_settings
and hide Semantic Filter and Network Settings tabs from non-admin users in
the MCP Servers UI.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(lint): use list[str] instead of List[str] to satisfy UP006 budget
Co-authored-by: Cursor <cursoragent@cursor.com>
* perf(mcp): cache BYOM submitter server lookup with 60s TTL
Co-authored-by: Cursor <cursoragent@cursor.com>
* style: fix ruff format and prettier formatting
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix: preserve approved BYOM server visibility
* fix(mcp): keep no-mcp-servers opt-out absolute and gate BYOM union by key scope
The autofix in 94fd2bf made the no-mcp-servers sentinel return the caller's
submitted BYOM servers, which weakened an explicit key-level opt-out into a
soft preference. Restore the absolute opt-out and additionally skip the BYOM
union for keys with an explicit object_permission.mcp_servers list and for
toolset-scoped requests, mirroring how allow_all_keys servers are handled.
Add unit tests for the sentinel, explicit scoping, toolset scope, the cache
invalidation helper, the cache-miss DB path, and the db.py query helper.
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
parent
85db18e618
commit
b96f1aa686
17 changed files with 574 additions and 51 deletions
|
|
@ -1228,6 +1228,23 @@ def _remaining_token_seconds(expires_at: str | None) -> int | None:
|
|||
return remaining if remaining > 0 else None
|
||||
|
||||
|
||||
async def get_active_submitted_mcp_server_ids_for_user(
|
||||
prisma_client: PrismaClient,
|
||||
user_id: str,
|
||||
) -> list[str]:
|
||||
"""Return active BYOM servers submitted by this user (creator visibility)."""
|
||||
if not user_id:
|
||||
return []
|
||||
|
||||
rows = await MCPServerRepository(prisma_client).table.find_many(
|
||||
where={
|
||||
"submitted_by": user_id,
|
||||
"approval_status": MCPApprovalStatus.active,
|
||||
},
|
||||
)
|
||||
return [row.server_id for row in rows]
|
||||
|
||||
|
||||
async def approve_mcp_server(
|
||||
prisma_client: PrismaClient,
|
||||
server_id: str,
|
||||
|
|
|
|||
|
|
@ -1246,6 +1246,67 @@ class MCPServerManager:
|
|||
"""Return server IDs that bypass per-key restrictions."""
|
||||
return [server.server_id for server in self.get_registry().values() if server.allow_all_keys is True]
|
||||
|
||||
@staticmethod
|
||||
def get_byom_submitted_servers_cache_key(user_id: str) -> str:
|
||||
return f"byom_submitted_servers:{user_id}"
|
||||
|
||||
async def invalidate_byom_submitted_servers_cache(self, user_id: str | None) -> None:
|
||||
if not user_id:
|
||||
return
|
||||
try:
|
||||
from litellm.proxy.proxy_server import user_api_key_cache
|
||||
|
||||
await user_api_key_cache.async_delete_cache(key=self.get_byom_submitted_servers_cache_key(user_id))
|
||||
except Exception as e: # noqa: BLE001
|
||||
verbose_logger.warning(f"Failed to invalidate BYOM submitted MCP server cache: {str(e)}")
|
||||
|
||||
async def _get_active_submitted_mcp_server_ids_for_user(
|
||||
self, user_api_key_auth: UserAPIKeyAuth | None
|
||||
) -> list[str]:
|
||||
submitter_user_id = getattr(user_api_key_auth, "user_id", None) if user_api_key_auth else None
|
||||
if not submitter_user_id:
|
||||
return []
|
||||
|
||||
try:
|
||||
from litellm.proxy._experimental.mcp_server.db import ( # noqa: PLC0415
|
||||
get_active_submitted_mcp_server_ids_for_user,
|
||||
)
|
||||
from litellm.proxy.proxy_server import prisma_client, user_api_key_cache
|
||||
except Exception as e: # noqa: BLE001
|
||||
verbose_logger.warning(f"Failed to load BYOM submitted MCP server cache dependencies: {str(e)}")
|
||||
return []
|
||||
|
||||
byom_cache_key = self.get_byom_submitted_servers_cache_key(submitter_user_id)
|
||||
submitted_server_ids: list[str] | None = None
|
||||
try:
|
||||
cached_submitted_server_ids = await user_api_key_cache.async_get_cache(key=byom_cache_key)
|
||||
if cached_submitted_server_ids is not None:
|
||||
submitted_server_ids = cast(list[str], cached_submitted_server_ids)
|
||||
except Exception as e: # noqa: BLE001
|
||||
verbose_logger.warning(f"Failed to read BYOM submitted MCP server cache: {str(e)}")
|
||||
|
||||
if submitted_server_ids is None:
|
||||
if prisma_client is None:
|
||||
submitted_server_ids = []
|
||||
else:
|
||||
try:
|
||||
submitted_server_ids = await get_active_submitted_mcp_server_ids_for_user(
|
||||
prisma_client, submitter_user_id
|
||||
)
|
||||
except Exception as e: # noqa: BLE001
|
||||
verbose_logger.warning(f"Failed to read BYOM submitted MCP servers from database: {str(e)}")
|
||||
submitted_server_ids = []
|
||||
try:
|
||||
await user_api_key_cache.async_set_cache(
|
||||
key=byom_cache_key,
|
||||
value=submitted_server_ids,
|
||||
ttl=60,
|
||||
)
|
||||
except Exception as e: # noqa: BLE001
|
||||
verbose_logger.warning(f"Failed to write BYOM submitted MCP server cache: {str(e)}")
|
||||
|
||||
return [server_id for server_id in submitted_server_ids if self.get_mcp_server_by_id(server_id) is not None]
|
||||
|
||||
async def get_allowed_mcp_servers(self, user_api_key_auth: Optional[UserAPIKeyAuth] = None) -> List[str]:
|
||||
"""
|
||||
Get the allowed MCP Servers for the user.
|
||||
|
|
@ -1259,25 +1320,30 @@ class MCPServerManager:
|
|||
|
||||
allow_all_server_ids = self.get_allow_all_keys_server_ids()
|
||||
|
||||
# The key explicitly opted out of every MCP server. Return zero before
|
||||
# layering on allow_all_keys or submitted servers so the opt-out is absolute.
|
||||
key_object_permission = user_api_key_auth.object_permission if user_api_key_auth else None
|
||||
if key_object_permission is not None and (
|
||||
SpecialMCPServerNames.no_mcp_servers.value in (key_object_permission.mcp_servers or [])
|
||||
):
|
||||
return []
|
||||
|
||||
# Check if object_permission.mcp_servers is explicitly set (not None, empty list is valid)
|
||||
has_explicit_object_permission = key_object_permission is not None and (
|
||||
key_object_permission.mcp_servers is not None
|
||||
)
|
||||
if has_explicit_object_permission:
|
||||
verbose_logger.debug(f"Object permission mcp_servers explicitly set: {key_object_permission.mcp_servers}")
|
||||
|
||||
# BYOM creator visibility never widens a key that was explicitly scoped:
|
||||
# only keys without their own mcp_servers list get submitted servers unioned in.
|
||||
submitted_server_ids = (
|
||||
[]
|
||||
if has_explicit_object_permission
|
||||
else await self._get_active_submitted_mcp_server_ids_for_user(user_api_key_auth)
|
||||
)
|
||||
|
||||
try:
|
||||
# The key explicitly opted out of every MCP server. Return zero before
|
||||
# layering on allow_all_keys servers so the opt-out is absolute.
|
||||
key_object_permission = user_api_key_auth.object_permission if user_api_key_auth else None
|
||||
if key_object_permission is not None and (
|
||||
SpecialMCPServerNames.no_mcp_servers.value in (key_object_permission.mcp_servers or [])
|
||||
):
|
||||
return []
|
||||
|
||||
# Check if object_permission.mcp_servers is explicitly set
|
||||
has_explicit_object_permission = False
|
||||
if user_api_key_auth and user_api_key_auth.object_permission:
|
||||
# Check if mcp_servers is explicitly set (not None, empty list is valid)
|
||||
if user_api_key_auth.object_permission.mcp_servers is not None:
|
||||
has_explicit_object_permission = True
|
||||
verbose_logger.debug(
|
||||
f"Object permission mcp_servers explicitly set: {user_api_key_auth.object_permission.mcp_servers}"
|
||||
)
|
||||
|
||||
# If admin but NO explicit object permission, get all servers
|
||||
if user_api_key_auth and _user_has_admin_view(user_api_key_auth) and not has_explicit_object_permission:
|
||||
verbose_logger.debug("Admin user without explicit object_permission - returning all servers")
|
||||
|
|
@ -1299,6 +1365,7 @@ class MCPServerManager:
|
|||
in_toolset_scope = _mcp_active_toolset_id.get() is not None
|
||||
if not in_toolset_scope:
|
||||
combined_servers.update(allow_all_server_ids)
|
||||
combined_servers.update(submitted_server_ids)
|
||||
|
||||
# For anonymous callers (no user_id, no role), also surface any
|
||||
# servers the operator has opted into upstream-delegated auth.
|
||||
|
|
@ -1331,9 +1398,9 @@ class MCPServerManager:
|
|||
except Exception: # noqa: BLE001
|
||||
verbose_logger.exception(
|
||||
"Failed to get allowed MCP servers; team-level object_permission "
|
||||
"grants may be dropped. Falling back to global servers only."
|
||||
"grants may be dropped. Falling back to global and submitted servers."
|
||||
)
|
||||
return allow_all_server_ids
|
||||
return list(dict.fromkeys(allow_all_server_ids + submitted_server_ids))
|
||||
|
||||
async def resolve_toolset_tool_permissions(
|
||||
self,
|
||||
|
|
|
|||
|
|
@ -1158,6 +1158,7 @@ if MCP_AVAILABLE:
|
|||
server_id,
|
||||
touched_by=user_api_key_dict.user_id or LITELLM_PROXY_ADMIN_NAME,
|
||||
)
|
||||
await global_mcp_server_manager.invalidate_byom_submitted_servers_cache(approved.submitted_by)
|
||||
await global_mcp_server_manager.reload_servers_from_database()
|
||||
|
||||
return _redact_mcp_credentials(approved)
|
||||
|
|
|
|||
|
|
@ -1160,6 +1160,12 @@ async def update_mcp_semantic_filter_settings(
|
|||
Update MCP semantic filter settings in database.
|
||||
Settings will be picked up by all pods within approximately 10 seconds via background polling.
|
||||
"""
|
||||
if user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN:
|
||||
raise HTTPException(
|
||||
status_code=403,
|
||||
detail="Only proxy admins can update MCP semantic filter settings.",
|
||||
)
|
||||
|
||||
result = await _update_litellm_setting(
|
||||
settings=settings,
|
||||
settings_key="mcp_semantic_tool_filter",
|
||||
|
|
|
|||
|
|
@ -6498,3 +6498,86 @@ class TestMCPMetaTraceCarrier:
|
|||
assert _mcp_meta_trace_carrier(SimpleNamespace(meta=None)) is None
|
||||
only_progress = RequestParams.Meta.model_validate({"progressToken": "p1"})
|
||||
assert _mcp_meta_trace_carrier(SimpleNamespace(meta=only_progress)) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_allowed_mcp_servers_includes_active_servers_submitted_by_user():
|
||||
"""BYOM submitters can see approved servers they submitted without allow_all_keys."""
|
||||
from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
|
||||
global_mcp_server_manager,
|
||||
)
|
||||
from litellm.proxy._types import LitellmUserRoles, UserAPIKeyAuth
|
||||
|
||||
submitted_server = _make_mcp_server_for_scope_filter("submitted-1", "user_mcp")
|
||||
submitter = UserAPIKeyAuth(
|
||||
user_id="submitter-user",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
api_key="sk-submitter",
|
||||
)
|
||||
other_user = UserAPIKeyAuth(
|
||||
user_id="other-user",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
api_key="sk-other",
|
||||
)
|
||||
|
||||
async def _submitted_ids(prisma_client, user_id):
|
||||
return ["submitted-1"] if user_id == "submitter-user" else []
|
||||
|
||||
with (
|
||||
patch.object(
|
||||
global_mcp_server_manager,
|
||||
"get_registry",
|
||||
return_value={"submitted-1": submitted_server},
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp."
|
||||
"MCPRequestHandler.get_allowed_mcp_servers",
|
||||
AsyncMock(return_value=[]),
|
||||
),
|
||||
patch("litellm.proxy.proxy_server.prisma_client", MagicMock()),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.db.get_active_submitted_mcp_server_ids_for_user",
|
||||
side_effect=_submitted_ids,
|
||||
),
|
||||
):
|
||||
submitter_allowed = await global_mcp_server_manager.get_allowed_mcp_servers(submitter)
|
||||
other_allowed = await global_mcp_server_manager.get_allowed_mcp_servers(other_user)
|
||||
|
||||
assert "submitted-1" in submitter_allowed
|
||||
assert "submitted-1" not in other_allowed
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_active_submitted_mcp_server_ids_for_user_queries_active_rows():
|
||||
from litellm.proxy._experimental.mcp_server.db import (
|
||||
get_active_submitted_mcp_server_ids_for_user,
|
||||
)
|
||||
from litellm.proxy._types import MCPApprovalStatus
|
||||
|
||||
row = MagicMock()
|
||||
row.server_id = "submitted-1"
|
||||
prisma_client = MagicMock()
|
||||
prisma_client.db.litellm_mcpservertable.find_many = AsyncMock(return_value=[row])
|
||||
|
||||
result = await get_active_submitted_mcp_server_ids_for_user(prisma_client, "submitter-user")
|
||||
|
||||
assert result == ["submitted-1"]
|
||||
prisma_client.db.litellm_mcpservertable.find_many.assert_awaited_once_with(
|
||||
where={
|
||||
"submitted_by": "submitter-user",
|
||||
"approval_status": MCPApprovalStatus.active,
|
||||
},
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_active_submitted_mcp_server_ids_for_user_empty_user_id_skips_db():
|
||||
from litellm.proxy._experimental.mcp_server.db import (
|
||||
get_active_submitted_mcp_server_ids_for_user,
|
||||
)
|
||||
|
||||
prisma_client = MagicMock()
|
||||
prisma_client.db.litellm_mcpservertable.find_many = AsyncMock()
|
||||
|
||||
assert await get_active_submitted_mcp_server_ids_for_user(prisma_client, "") == []
|
||||
prisma_client.db.litellm_mcpservertable.find_many.assert_not_awaited()
|
||||
|
|
|
|||
|
|
@ -3198,6 +3198,250 @@ class TestMCPServerManager:
|
|||
assert result == []
|
||||
mock_inner.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_mcp_servers_sentinel_excludes_submitted_byom_servers(self):
|
||||
from litellm.proxy import proxy_server as proxy_server_module
|
||||
from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import (
|
||||
MCPRequestHandler,
|
||||
)
|
||||
from litellm.proxy._types import LiteLLM_ObjectPermissionTable, UserAPIKeyAuth
|
||||
|
||||
class _Cache:
|
||||
async def async_get_cache(self, key: str):
|
||||
return ["submitted-server"]
|
||||
|
||||
manager = MCPServerManager()
|
||||
manager.registry = {
|
||||
"submitted-server": MCPServer(
|
||||
server_id="submitted-server",
|
||||
name="submitted",
|
||||
transport=MCPTransport.http,
|
||||
)
|
||||
}
|
||||
object_permission = LiteLLM_ObjectPermissionTable(
|
||||
object_permission_id="perm_no_mcp",
|
||||
mcp_servers=["no-mcp-servers"],
|
||||
mcp_access_groups=[],
|
||||
)
|
||||
user_api_key_auth = UserAPIKeyAuth(
|
||||
api_key="sk-test",
|
||||
user_id="user-123",
|
||||
object_permission=object_permission,
|
||||
object_permission_id="perm_no_mcp",
|
||||
)
|
||||
|
||||
with (
|
||||
patch.object(proxy_server_module, "user_api_key_cache", _Cache()),
|
||||
patch.object(proxy_server_module, "prisma_client", None),
|
||||
patch.object(
|
||||
manager, "get_allow_all_keys_server_ids", return_value=["global-server"]
|
||||
),
|
||||
patch.object(
|
||||
MCPRequestHandler,
|
||||
"get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=["leaked-server"],
|
||||
) as mock_inner,
|
||||
):
|
||||
result = await manager.get_allowed_mcp_servers(user_api_key_auth)
|
||||
|
||||
assert result == []
|
||||
mock_inner.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_explicitly_scoped_key_excludes_submitted_byom_servers(self):
|
||||
from litellm.proxy import proxy_server as proxy_server_module
|
||||
from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import (
|
||||
MCPRequestHandler,
|
||||
)
|
||||
from litellm.proxy._types import LiteLLM_ObjectPermissionTable, UserAPIKeyAuth
|
||||
|
||||
cache = MagicMock()
|
||||
cache.async_get_cache = AsyncMock(return_value=["submitted-server"])
|
||||
|
||||
manager = MCPServerManager()
|
||||
manager.registry = {
|
||||
"submitted-server": MCPServer(
|
||||
server_id="submitted-server",
|
||||
name="submitted",
|
||||
transport=MCPTransport.http,
|
||||
),
|
||||
"scoped-server": MCPServer(
|
||||
server_id="scoped-server",
|
||||
name="scoped",
|
||||
transport=MCPTransport.http,
|
||||
),
|
||||
}
|
||||
object_permission = LiteLLM_ObjectPermissionTable(
|
||||
object_permission_id="perm_scoped",
|
||||
mcp_servers=["scoped-server"],
|
||||
mcp_access_groups=[],
|
||||
)
|
||||
user_api_key_auth = UserAPIKeyAuth(
|
||||
api_key="sk-test",
|
||||
user_id="user-123",
|
||||
object_permission=object_permission,
|
||||
object_permission_id="perm_scoped",
|
||||
)
|
||||
|
||||
with (
|
||||
patch.object(proxy_server_module, "user_api_key_cache", cache),
|
||||
patch.object(proxy_server_module, "prisma_client", None),
|
||||
patch.object(manager, "get_allow_all_keys_server_ids", return_value=[]),
|
||||
patch.object(
|
||||
MCPRequestHandler,
|
||||
"get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=["scoped-server"],
|
||||
),
|
||||
):
|
||||
result = await manager.get_allowed_mcp_servers(user_api_key_auth)
|
||||
|
||||
assert result == ["scoped-server"]
|
||||
cache.async_get_cache.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_toolset_scope_excludes_submitted_byom_servers(self):
|
||||
from litellm.proxy import proxy_server as proxy_server_module
|
||||
from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import (
|
||||
MCPRequestHandler,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.mcp_context import (
|
||||
_mcp_active_toolset_id,
|
||||
)
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
cache = MagicMock()
|
||||
cache.async_get_cache = AsyncMock(return_value=["submitted-server"])
|
||||
|
||||
manager = MCPServerManager()
|
||||
manager.registry = {
|
||||
"submitted-server": MCPServer(
|
||||
server_id="submitted-server",
|
||||
name="submitted",
|
||||
transport=MCPTransport.http,
|
||||
),
|
||||
"toolset-server": MCPServer(
|
||||
server_id="toolset-server",
|
||||
name="toolset",
|
||||
transport=MCPTransport.http,
|
||||
),
|
||||
}
|
||||
user_api_key_auth = UserAPIKeyAuth(api_key="sk-test", user_id="user-123")
|
||||
|
||||
token = _mcp_active_toolset_id.set("toolset-abc")
|
||||
try:
|
||||
with (
|
||||
patch.object(proxy_server_module, "user_api_key_cache", cache),
|
||||
patch.object(proxy_server_module, "prisma_client", None),
|
||||
patch.object(
|
||||
manager, "get_allow_all_keys_server_ids", return_value=["global-server"]
|
||||
),
|
||||
patch.object(
|
||||
MCPRequestHandler,
|
||||
"get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
return_value=["toolset-server"],
|
||||
),
|
||||
):
|
||||
result = await manager.get_allowed_mcp_servers(user_api_key_auth)
|
||||
finally:
|
||||
_mcp_active_toolset_id.reset(token)
|
||||
|
||||
assert result == ["toolset-server"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_invalidate_byom_submitted_servers_cache_deletes_key(self):
|
||||
from litellm.proxy import proxy_server as proxy_server_module
|
||||
|
||||
cache = MagicMock()
|
||||
cache.async_delete_cache = AsyncMock()
|
||||
manager = MCPServerManager()
|
||||
|
||||
with patch.object(proxy_server_module, "user_api_key_cache", cache):
|
||||
await manager.invalidate_byom_submitted_servers_cache("user-123")
|
||||
await manager.invalidate_byom_submitted_servers_cache(None)
|
||||
|
||||
cache.async_delete_cache.assert_awaited_once_with(key="byom_submitted_servers:user-123")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_active_submitted_ids_cache_miss_queries_db_and_caches(self):
|
||||
from litellm.proxy import proxy_server as proxy_server_module
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
cache = MagicMock()
|
||||
cache.async_get_cache = AsyncMock(return_value=None)
|
||||
cache.async_set_cache = AsyncMock()
|
||||
manager = MCPServerManager()
|
||||
manager.registry = {
|
||||
"submitted-server": MCPServer(
|
||||
server_id="submitted-server",
|
||||
name="submitted",
|
||||
transport=MCPTransport.http,
|
||||
)
|
||||
}
|
||||
user_api_key_auth = UserAPIKeyAuth(api_key="sk-test", user_id="user-123")
|
||||
|
||||
with (
|
||||
patch.object(proxy_server_module, "user_api_key_cache", cache),
|
||||
patch.object(proxy_server_module, "prisma_client", MagicMock()),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.db.get_active_submitted_mcp_server_ids_for_user",
|
||||
AsyncMock(return_value=["submitted-server", "unknown-server"]),
|
||||
),
|
||||
):
|
||||
result = await manager._get_active_submitted_mcp_server_ids_for_user(user_api_key_auth)
|
||||
|
||||
assert result == ["submitted-server"]
|
||||
cache.async_set_cache.assert_awaited_once_with(
|
||||
key="byom_submitted_servers:user-123",
|
||||
value=["submitted-server", "unknown-server"],
|
||||
ttl=60,
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_allowed_mcp_servers_fallback_keeps_submitted_byom_servers(self):
|
||||
from litellm.proxy import proxy_server as proxy_server_module
|
||||
from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import (
|
||||
MCPRequestHandler,
|
||||
)
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
class _Cache:
|
||||
async def async_get_cache(self, key: str):
|
||||
assert key == "byom_submitted_servers:user-123"
|
||||
return ["submitted-server"]
|
||||
|
||||
manager = MCPServerManager()
|
||||
manager.registry = {
|
||||
"submitted-server": MCPServer(
|
||||
server_id="submitted-server",
|
||||
name="submitted",
|
||||
transport=MCPTransport.http,
|
||||
)
|
||||
}
|
||||
user_api_key_auth = UserAPIKeyAuth(
|
||||
api_key="sk-test",
|
||||
user_id="user-123",
|
||||
)
|
||||
|
||||
with (
|
||||
patch.object(proxy_server_module, "user_api_key_cache", _Cache()),
|
||||
patch.object(proxy_server_module, "prisma_client", None),
|
||||
patch.object(
|
||||
manager, "get_allow_all_keys_server_ids", return_value=["global-server"]
|
||||
),
|
||||
patch.object(
|
||||
MCPRequestHandler,
|
||||
"get_allowed_mcp_servers",
|
||||
new_callable=AsyncMock,
|
||||
side_effect=RuntimeError("permission resolver failed"),
|
||||
),
|
||||
):
|
||||
result = await manager.get_allowed_mcp_servers(user_api_key_auth)
|
||||
|
||||
assert set(result) == {"global-server", "submitted-server"}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_allowed_mcp_servers_anonymous_delegate_requires_oauth2(self):
|
||||
"""Anonymous delegated auth listing should only include oauth2 servers."""
|
||||
|
|
|
|||
|
|
@ -3223,8 +3223,10 @@ class TestMCPApprovalWorkflow:
|
|||
pending_server.approval_status = MCPApprovalStatus.pending_review
|
||||
approved_server = generate_mock_mcp_server_db_record()
|
||||
approved_server.approval_status = MCPApprovalStatus.active
|
||||
approved_server.submitted_by = "submitter-user"
|
||||
|
||||
mock_manager = MagicMock()
|
||||
mock_manager.invalidate_byom_submitted_servers_cache = AsyncMock()
|
||||
mock_manager.reload_servers_from_database = AsyncMock()
|
||||
|
||||
with (
|
||||
|
|
@ -3250,6 +3252,9 @@ class TestMCPApprovalWorkflow:
|
|||
)
|
||||
|
||||
mock_manager.reload_servers_from_database.assert_awaited_once()
|
||||
mock_manager.invalidate_byom_submitted_servers_cache.assert_awaited_once_with(
|
||||
"submitter-user"
|
||||
)
|
||||
assert result is not None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -2367,3 +2367,29 @@ def test_update_ui_settings_writes_audit_log(monkeypatch):
|
|||
assert after["disable_custom_api_keys"] is True
|
||||
finally:
|
||||
app.dependency_overrides.pop(user_api_key_auth, None)
|
||||
|
||||
|
||||
def test_update_mcp_semantic_filter_settings_requires_proxy_admin(monkeypatch):
|
||||
"""Non-admin callers must not mutate global MCP semantic filter settings."""
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
||||
|
||||
monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
|
||||
|
||||
async def _internal_user_auth():
|
||||
return UserAPIKeyAuth(
|
||||
user_id="internal-user-1",
|
||||
api_key="hashed-internal-key",
|
||||
user_role=LitellmUserRoles.INTERNAL_USER,
|
||||
)
|
||||
|
||||
app.dependency_overrides[user_api_key_auth] = _internal_user_auth
|
||||
try:
|
||||
resp = client.patch(
|
||||
"/update/mcp_semantic_filter_settings",
|
||||
json={"enabled": True, "top_k": 99, "similarity_threshold": 0.01},
|
||||
)
|
||||
assert resp.status_code == 403
|
||||
assert "proxy admin" in resp.json()["detail"].lower()
|
||||
finally:
|
||||
app.dependency_overrides.pop(user_api_key_auth, None)
|
||||
|
|
|
|||
|
|
@ -97,7 +97,9 @@ function ConfirmDialog({ action, serverName, isCurrentlyActive, onConfirm, onCan
|
|||
</h3>
|
||||
<p className="text-sm text-gray-500 mb-4">
|
||||
Are you sure you want to {action} <span className="font-medium text-gray-700">"{serverName}"</span>?{" "}
|
||||
{isApprove ? "This will make it active and available for use." : rejectBody}
|
||||
{isApprove
|
||||
? "This will activate the server. The submitting user will see it in their MCP Servers list once approved."
|
||||
: rejectBody}
|
||||
</p>
|
||||
{!isApprove && (
|
||||
<textarea
|
||||
|
|
|
|||
|
|
@ -1,2 +1,5 @@
|
|||
// Must match the backend SpecialMCPServerNames.no_mcp_servers enum value.
|
||||
export const NO_MCP_SERVERS_SENTINEL = "no-mcp-servers";
|
||||
|
||||
export const MCP_TOOLS_PREVIEW_FORBIDDEN_MESSAGE =
|
||||
"Tool preview is not available for submissions. Tools will be verified by an admin during review.";
|
||||
|
|
|
|||
|
|
@ -91,13 +91,21 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
const [oauthDocsUrl, setOauthDocsUrl] = useState<string | null>(null);
|
||||
|
||||
// Single hook call shared by MCPConnectionStatus and MCPToolConfiguration to avoid duplicate requests.
|
||||
const { tools, isLoadingTools, toolsError, toolsErrorStackTrace, canFetchTools, fetchTools, clearTools } =
|
||||
useTestMCPConnection({
|
||||
accessToken,
|
||||
oauthAccessToken,
|
||||
formValues,
|
||||
enabled: true,
|
||||
});
|
||||
const {
|
||||
tools,
|
||||
isLoadingTools,
|
||||
toolsError,
|
||||
toolsErrorStatus,
|
||||
toolsErrorStackTrace,
|
||||
canFetchTools,
|
||||
fetchTools,
|
||||
clearTools,
|
||||
} = useTestMCPConnection({
|
||||
accessToken,
|
||||
oauthAccessToken,
|
||||
formValues,
|
||||
enabled: true,
|
||||
});
|
||||
|
||||
const authType = formValues.auth_type as string | undefined;
|
||||
const shouldShowAuthValueField = authType ? AUTH_TYPES_REQUIRING_AUTH_VALUE.includes(authType) : false;
|
||||
|
|
@ -469,7 +477,12 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
}
|
||||
|
||||
NotificationsManager.success(
|
||||
isAdmin ? "MCP Server created successfully" : "MCP Server submitted for admin review",
|
||||
isAdmin
|
||||
? "MCP Server created successfully"
|
||||
: {
|
||||
message: "MCP Server submitted for admin review",
|
||||
description: "Once an admin approves it, the server will appear in your MCP Servers list.",
|
||||
},
|
||||
);
|
||||
form.resetFields();
|
||||
setCostConfig({});
|
||||
|
|
@ -622,8 +635,8 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
>
|
||||
{!isAdmin && (
|
||||
<div className="rounded-md bg-blue-50 border border-blue-200 px-4 py-3 text-sm text-blue-800">
|
||||
Your submission will be sent for admin review before it becomes active. Note: the request must be made
|
||||
with a team-scoped API key.
|
||||
Your submission will be sent for admin review. Once approved, the server will appear in your MCP Servers
|
||||
list. The request must be made with a team-scoped API key.
|
||||
</div>
|
||||
)}
|
||||
<div className="grid grid-cols-1 gap-6">
|
||||
|
|
@ -1088,6 +1101,7 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
tools={tools}
|
||||
isLoadingTools={isLoadingTools}
|
||||
toolsError={toolsError}
|
||||
toolsErrorStatus={toolsErrorStatus}
|
||||
toolsErrorStackTrace={toolsErrorStackTrace}
|
||||
canFetchTools={canFetchTools}
|
||||
fetchTools={fetchTools}
|
||||
|
|
@ -1112,6 +1126,7 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
externalTools={tools}
|
||||
externalIsLoading={isLoadingTools}
|
||||
externalError={toolsError}
|
||||
externalErrorStatus={toolsErrorStatus}
|
||||
externalCanFetch={canFetchTools}
|
||||
/>
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -41,6 +41,21 @@ describe("MCPConnectionStatus", () => {
|
|||
expect(screen.getByText("Connecting...")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should show info message without retry when tool preview returns 403", () => {
|
||||
render(
|
||||
<MCPConnectionStatus
|
||||
{...defaultProps}
|
||||
canFetchTools={true}
|
||||
toolsError="Tool preview is not available for submissions. Tools will be verified by an admin during review."
|
||||
toolsErrorStatus={403}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(screen.getByRole("alert")).toHaveTextContent(/Tools will be verified by an admin during review/i);
|
||||
expect(screen.queryByText("Connection Failed")).not.toBeInTheDocument();
|
||||
expect(screen.queryByRole("button", { name: /retry/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should show error state with retry button when toolsError is set", async () => {
|
||||
const fetchTools = vi.fn();
|
||||
const user = userEvent.setup();
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ interface MCPConnectionStatusProps {
|
|||
tools: any[];
|
||||
isLoadingTools: boolean;
|
||||
toolsError: string | null;
|
||||
toolsErrorStatus?: number | null;
|
||||
toolsErrorStackTrace: string | null;
|
||||
canFetchTools: boolean;
|
||||
fetchTools: () => Promise<void>;
|
||||
|
|
@ -18,10 +19,12 @@ const MCPConnectionStatus: React.FC<MCPConnectionStatusProps> = ({
|
|||
tools,
|
||||
isLoadingTools,
|
||||
toolsError,
|
||||
toolsErrorStatus = null,
|
||||
toolsErrorStackTrace,
|
||||
canFetchTools,
|
||||
fetchTools,
|
||||
}) => {
|
||||
const isPreviewForbidden = toolsErrorStatus === 403;
|
||||
// Don't show anything if required fields aren't filled
|
||||
if (!canFetchTools && !formValues.url && !formValues.spec_path) {
|
||||
return null;
|
||||
|
|
@ -54,7 +57,9 @@ const MCPConnectionStatus: React.FC<MCPConnectionStatusProps> = ({
|
|||
: tools.length > 0
|
||||
? "Connection successful"
|
||||
: toolsError
|
||||
? "Connection failed"
|
||||
? isPreviewForbidden
|
||||
? "Ready to submit"
|
||||
: "Connection failed"
|
||||
: "Ready to test connection"}
|
||||
</Text>
|
||||
<br />
|
||||
|
|
@ -75,7 +80,7 @@ const MCPConnectionStatus: React.FC<MCPConnectionStatusProps> = ({
|
|||
</div>
|
||||
)}
|
||||
|
||||
{toolsError && (
|
||||
{toolsError && !isPreviewForbidden && (
|
||||
<div className="flex items-center text-red-600">
|
||||
<ExclamationCircleOutlined className="mr-1" />
|
||||
<Text className="text-red-600 font-medium">Failed</Text>
|
||||
|
|
@ -90,7 +95,11 @@ const MCPConnectionStatus: React.FC<MCPConnectionStatusProps> = ({
|
|||
</div>
|
||||
)}
|
||||
|
||||
{toolsError && (
|
||||
{toolsError && isPreviewForbidden && (
|
||||
<Alert message="Tool preview unavailable" description={toolsError} type="info" showIcon />
|
||||
)}
|
||||
|
||||
{toolsError && !isPreviewForbidden && (
|
||||
<Alert
|
||||
message="Connection Failed"
|
||||
description={
|
||||
|
|
|
|||
|
|
@ -497,8 +497,8 @@ const MCPServers: React.FC<MCPServerProps> = ({ accessToken, userRole, userID })
|
|||
<Tab>All Servers</Tab>
|
||||
<Tab>Toolsets</Tab>
|
||||
<Tab>Connect</Tab>
|
||||
<Tab>Semantic Filter</Tab>
|
||||
<Tab>Network Settings</Tab>
|
||||
{isAdminRole(userRole) && <Tab>Semantic Filter</Tab>}
|
||||
{isAdminRole(userRole) && <Tab>Network Settings</Tab>}
|
||||
{isAdminRole(userRole) && (
|
||||
<Tab>
|
||||
<span className="flex items-center gap-2">
|
||||
|
|
@ -652,12 +652,16 @@ const MCPServers: React.FC<MCPServerProps> = ({ accessToken, userRole, userID })
|
|||
<TabPanel>
|
||||
<MCPConnect />
|
||||
</TabPanel>
|
||||
<TabPanel>
|
||||
<MCPSemanticFilterSettings accessToken={accessToken} />
|
||||
</TabPanel>
|
||||
<TabPanel>
|
||||
<MCPNetworkSettings accessToken={accessToken} />
|
||||
</TabPanel>
|
||||
{isAdminRole(userRole) && (
|
||||
<TabPanel>
|
||||
<MCPSemanticFilterSettings accessToken={accessToken} />
|
||||
</TabPanel>
|
||||
)}
|
||||
{isAdminRole(userRole) && (
|
||||
<TabPanel>
|
||||
<MCPNetworkSettings accessToken={accessToken} />
|
||||
</TabPanel>
|
||||
)}
|
||||
{isAdminRole(userRole) && (
|
||||
<TabPanel>
|
||||
<MCPSubmissionsTab accessToken={accessToken} />
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ interface MCPToolConfigurationProps {
|
|||
externalTools?: any[];
|
||||
externalIsLoading?: boolean;
|
||||
externalError?: string | null;
|
||||
externalErrorStatus?: number | null;
|
||||
externalCanFetch?: boolean;
|
||||
/** When true, do not auto-select all tools for servers with no stored allowlist. */
|
||||
isEditMode?: boolean;
|
||||
|
|
@ -156,6 +157,7 @@ const MCPToolConfiguration: React.FC<MCPToolConfigurationProps> = ({
|
|||
externalTools,
|
||||
externalIsLoading,
|
||||
externalError,
|
||||
externalErrorStatus = null,
|
||||
externalCanFetch,
|
||||
isEditMode = false,
|
||||
}) => {
|
||||
|
|
@ -165,6 +167,7 @@ const MCPToolConfiguration: React.FC<MCPToolConfigurationProps> = ({
|
|||
const hasInitializedRef = useRef(false);
|
||||
const previousSuggestedToolNamesRef = useRef<string>("");
|
||||
const [expandedTools, setExpandedTools] = useState<Set<string>>(new Set());
|
||||
const isPreviewForbidden = externalErrorStatus === 403;
|
||||
|
||||
// Tool list is fetched by the parent (create/edit flow) and passed in. This
|
||||
// component renders that state; it never fetches on its own, so there is a
|
||||
|
|
@ -429,7 +432,13 @@ const MCPToolConfiguration: React.FC<MCPToolConfigurationProps> = ({
|
|||
)}
|
||||
|
||||
{/* Error state */}
|
||||
{toolsError && !isLoadingTools && (
|
||||
{toolsError && !isLoadingTools && isPreviewForbidden && (
|
||||
<div className="rounded-lg border border-blue-200 bg-blue-50 p-4">
|
||||
<Text className="text-sm text-blue-800">{toolsError}</Text>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{toolsError && !isLoadingTools && !isPreviewForbidden && (
|
||||
<div className="text-center py-6 text-red-500 border rounded-lg border-dashed border-red-300 bg-red-50">
|
||||
<ToolOutlined className="text-2xl mb-2" />
|
||||
<Text className="text-red-600 font-medium">Unable to load tools</Text>
|
||||
|
|
|
|||
|
|
@ -30,6 +30,7 @@ import type { SkillRegisterRequest } from "./claude_code_plugins/types";
|
|||
import { jsonFields } from "./common_components/check_openapi_schema";
|
||||
import NotificationsManager from "./molecules/notifications_manager";
|
||||
import type { MCPUserEnvVarsStatus } from "./mcp_tools/types";
|
||||
import { MCP_TOOLS_PREVIEW_FORBIDDEN_MESSAGE } from "./mcp_tools/constants";
|
||||
import { createApiClient, deriveErrorMessage } from "@/lib/http/client";
|
||||
import { resolveApiBase } from "@/lib/http/resolveApiBase";
|
||||
import { serverRootPath, setServerRootPath } from "@/lib/serverRootPath";
|
||||
|
|
@ -6796,17 +6797,25 @@ export const testMCPToolsListRequest = async (
|
|||
const data = await response.json();
|
||||
|
||||
if (!response.ok || data.error) {
|
||||
if (response.status === 403) {
|
||||
return {
|
||||
tools: [],
|
||||
error: true,
|
||||
status: 403,
|
||||
message: MCP_TOOLS_PREVIEW_FORBIDDEN_MESSAGE,
|
||||
};
|
||||
}
|
||||
// Return the error response instead of throwing an error
|
||||
// This allows the caller to handle the error format properly
|
||||
if (data.error) {
|
||||
return data; // Return the full error response
|
||||
} else {
|
||||
return {
|
||||
tools: [],
|
||||
error: "request_failed",
|
||||
message: data.message || `MCP tools list failed: ${response.status} ${response.statusText}`,
|
||||
};
|
||||
return { ...data, status: response.status };
|
||||
}
|
||||
return {
|
||||
tools: [],
|
||||
error: "request_failed",
|
||||
status: response.status,
|
||||
message: data.message || `MCP tools list failed: ${response.status} ${response.statusText}`,
|
||||
};
|
||||
}
|
||||
|
||||
return data;
|
||||
|
|
|
|||
|
|
@ -33,6 +33,7 @@ interface UseTestMCPConnectionReturn {
|
|||
tools: any[];
|
||||
isLoadingTools: boolean;
|
||||
toolsError: string | null;
|
||||
toolsErrorStatus: number | null;
|
||||
toolsErrorStackTrace: string | null;
|
||||
hasShownSuccessMessage: boolean;
|
||||
canFetchTools: boolean;
|
||||
|
|
@ -49,6 +50,7 @@ export const useTestMCPConnection = ({
|
|||
const [tools, setTools] = useState<any[]>([]);
|
||||
const [isLoadingTools, setIsLoadingTools] = useState(false);
|
||||
const [toolsError, setToolsError] = useState<string | null>(null);
|
||||
const [toolsErrorStatus, setToolsErrorStatus] = useState<number | null>(null);
|
||||
const [toolsErrorStackTrace, setToolsErrorStackTrace] = useState<string | null>(null);
|
||||
const [hasShownSuccessMessage, setHasShownSuccessMessage] = useState(false);
|
||||
|
||||
|
|
@ -85,6 +87,7 @@ export const useTestMCPConnection = ({
|
|||
|
||||
setIsLoadingTools(true);
|
||||
setToolsError(null);
|
||||
setToolsErrorStatus(null);
|
||||
|
||||
try {
|
||||
// Prepare the MCP server config from form values
|
||||
|
|
@ -155,6 +158,7 @@ export const useTestMCPConnection = ({
|
|||
if (toolsResponse.tools && !toolsResponse.error) {
|
||||
setTools(toolsResponse.tools);
|
||||
setToolsError(null);
|
||||
setToolsErrorStatus(null);
|
||||
setToolsErrorStackTrace(null);
|
||||
if (toolsResponse.tools.length > 0 && !hasShownSuccessMessage) {
|
||||
setHasShownSuccessMessage(true);
|
||||
|
|
@ -162,13 +166,15 @@ export const useTestMCPConnection = ({
|
|||
} else {
|
||||
const errorMessage = toolsResponse.message || "Failed to retrieve tools list";
|
||||
setToolsError(errorMessage);
|
||||
setToolsErrorStackTrace(toolsResponse.stack_trace || null);
|
||||
setToolsErrorStatus(typeof toolsResponse.status === "number" ? toolsResponse.status : null);
|
||||
setToolsErrorStackTrace(toolsResponse.status === 403 ? null : toolsResponse.stack_trace || null);
|
||||
setTools([]);
|
||||
setHasShownSuccessMessage(false);
|
||||
}
|
||||
} catch (error) {
|
||||
console.error("Tools fetch error:", error);
|
||||
setToolsError(error instanceof Error ? error.message : String(error));
|
||||
setToolsErrorStatus(null);
|
||||
setToolsErrorStackTrace(null);
|
||||
setTools([]);
|
||||
setHasShownSuccessMessage(false);
|
||||
|
|
@ -180,6 +186,7 @@ export const useTestMCPConnection = ({
|
|||
const clearTools = useCallback(() => {
|
||||
setTools([]);
|
||||
setToolsError(null);
|
||||
setToolsErrorStatus(null);
|
||||
setToolsErrorStackTrace(null);
|
||||
setHasShownSuccessMessage(false);
|
||||
}, []);
|
||||
|
|
@ -213,6 +220,7 @@ export const useTestMCPConnection = ({
|
|||
tools,
|
||||
isLoadingTools,
|
||||
toolsError,
|
||||
toolsErrorStatus,
|
||||
toolsErrorStackTrace,
|
||||
hasShownSuccessMessage,
|
||||
canFetchTools,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue