mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-19 00:01:29 +00:00
fix(mcp): stable ordering for MCP servers list in Admin UI
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
30f33a949b
commit
d94b227906
4 changed files with 127 additions and 30 deletions
|
|
@ -1024,6 +1024,9 @@ if MCP_AVAILABLE:
|
|||
return {"servers": registry_servers}
|
||||
|
||||
## FastAPI Routes
|
||||
def _mcp_server_display_order(server: LiteLLM_MCPServerTable) -> tuple[str, str]:
|
||||
return ((server.server_name or server.alias or server.server_id).lower(), server.server_id)
|
||||
|
||||
def _get_user_mcp_management_mode() -> UserMCPManagementMode:
|
||||
from litellm.proxy.proxy_server import (
|
||||
general_settings as proxy_general_settings,
|
||||
|
|
@ -1174,10 +1177,12 @@ if MCP_AVAILABLE:
|
|||
detail="You do not have permission to view MCP servers for this team.",
|
||||
)
|
||||
|
||||
redacted_mcp_servers = await _get_team_scoped_mcp_server_list(sanitized_team_id)
|
||||
redacted_mcp_servers = sorted(
|
||||
await _get_team_scoped_mcp_server_list(sanitized_team_id), key=_mcp_server_display_order
|
||||
)
|
||||
else:
|
||||
servers: Final = await _resolve_accessible_mcp_servers(user_api_key_dict)
|
||||
redacted_mcp_servers = _redact_mcp_credentials_list(servers)
|
||||
redacted_mcp_servers = sorted(_redact_mcp_credentials_list(servers), key=_mcp_server_display_order)
|
||||
|
||||
if connected_app_view is True and is_ui_session_credential(user_api_key_dict):
|
||||
reachable_ids: Final = await _connected_app_reachable_server_ids(user_api_key_dict)
|
||||
|
|
|
|||
|
|
@ -1535,6 +1535,76 @@ class TestTeamScopedMCPServerAccess:
|
|||
result = await fetch_all_mcp_servers(user_api_key_dict=mock_user_auth, team_id="any-team-id")
|
||||
assert len(result) == 1
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_team_scoped_list_is_sorted_by_display_name(self):
|
||||
"""Set-derived resolution order must not leak to the client."""
|
||||
mock_user_auth = generate_mock_user_api_key_auth(
|
||||
user_role=LitellmUserRoles.PROXY_ADMIN,
|
||||
user_id="admin_user",
|
||||
)
|
||||
unsorted = [
|
||||
generate_mock_mcp_server_db_record(server_id="s-zeta", alias="zeta"),
|
||||
generate_mock_mcp_server_db_record(server_id="s-alpha", alias="Alpha"),
|
||||
generate_mock_mcp_server_db_record(server_id="s-mid", alias="mid"),
|
||||
]
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints._user_has_admin_view",
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints._get_team_scoped_mcp_server_list",
|
||||
AsyncMock(return_value=unsorted),
|
||||
),
|
||||
):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
fetch_all_mcp_servers,
|
||||
)
|
||||
|
||||
result = await fetch_all_mcp_servers(user_api_key_dict=mock_user_auth, team_id="any-team-id")
|
||||
assert [s.server_id for s in result] == ["s-alpha", "s-mid", "s-zeta"]
|
||||
|
||||
|
||||
class TestFetchAllMCPServersOrdering:
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_is_sorted_by_display_name_regardless_of_resolution_order(self):
|
||||
"""The registry resolves ids through a set, so the response must impose its own order."""
|
||||
mock_user_auth = generate_mock_user_api_key_auth(
|
||||
user_role=LitellmUserRoles.PROXY_ADMIN,
|
||||
user_id="admin_user",
|
||||
)
|
||||
first_order = [
|
||||
generate_mock_mcp_server_db_record(server_id="s-zeta", alias="zeta"),
|
||||
generate_mock_mcp_server_db_record(server_id="s-alpha", alias="Alpha"),
|
||||
generate_mock_mcp_server_db_record(server_id="s-mid", alias="mid"),
|
||||
]
|
||||
second_order = list(reversed(first_order))
|
||||
|
||||
for resolved in (first_order, second_order):
|
||||
mock_manager = MagicMock()
|
||||
mock_manager.get_all_allowed_mcp_servers = AsyncMock(return_value=resolved)
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager",
|
||||
mock_manager,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints._user_has_admin_view",
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.mcp_management_endpoints.build_effective_auth_contexts",
|
||||
AsyncMock(return_value=[mock_user_auth]),
|
||||
),
|
||||
):
|
||||
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
|
||||
fetch_all_mcp_servers,
|
||||
)
|
||||
|
||||
result = await fetch_all_mcp_servers(user_api_key_dict=mock_user_auth)
|
||||
assert [s.server_id for s in result] == ["s-alpha", "s-mid", "s-zeta"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_restricted_virtual_key_cannot_use_team_id_filter(self):
|
||||
"""Restricted virtual keys must not bypass access limits via team_id."""
|
||||
|
|
|
|||
|
|
@ -3,7 +3,8 @@ import { render, waitFor, screen, act, within } from "@testing-library/react";
|
|||
import userEvent from "@testing-library/user-event";
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import MCPServers from "./mcp_servers";
|
||||
import MCPServers, { compareServers } from "./mcp_servers";
|
||||
import type { MCPServer } from "@/components/mcp_tools/types";
|
||||
import * as networking from "@/components/networking";
|
||||
|
||||
// Mock the networking module
|
||||
|
|
@ -29,6 +30,32 @@ const createQueryClient = () =>
|
|||
},
|
||||
});
|
||||
|
||||
describe("compareServers", () => {
|
||||
const server = (server_id: string, name: string, created_at = ""): MCPServer =>
|
||||
({ server_id, server_name: name, created_at, updated_at: created_at }) as MCPServer;
|
||||
|
||||
const shuffled = [server("c", "github"), server("a", "slack"), server("b", "Jira")];
|
||||
|
||||
it("orders servers without timestamps by name so config.yaml servers render in a stable order", () => {
|
||||
const byCreated = [...shuffled].sort((a, b) => compareServers(a, b, "created_desc")).map((s) => s.server_id);
|
||||
const byUpdated = [...shuffled].sort((a, b) => compareServers(a, b, "updated_desc")).map((s) => s.server_id);
|
||||
const byHealth = [...shuffled].sort((a, b) => compareServers(a, b, "health")).map((s) => s.server_id);
|
||||
|
||||
expect(byCreated).toEqual(["c", "b", "a"]);
|
||||
expect(byUpdated).toEqual(["c", "b", "a"]);
|
||||
expect(byHealth).toEqual(["c", "b", "a"]);
|
||||
});
|
||||
|
||||
it("keeps newest-first when timestamps differ", () => {
|
||||
const newest = server("new", "zzz", "2026-02-01T00:00:00Z");
|
||||
const oldest = server("old", "aaa", "2026-01-01T00:00:00Z");
|
||||
expect([oldest, newest].sort((a, b) => compareServers(a, b, "created_desc")).map((s) => s.server_id)).toEqual([
|
||||
"new",
|
||||
"old",
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("MCPServers", () => {
|
||||
const defaultProps = {
|
||||
accessToken: "123",
|
||||
|
|
|
|||
|
|
@ -45,7 +45,7 @@ import { TOOLS_OAUTH_UI_STATE_KEY } from "@/hooks/mcpOAuthUtils";
|
|||
import UserEnvVarsModal from "./UserEnvVarsModal";
|
||||
import { listMCPUserEnvVarStatus } from "@/components/networking";
|
||||
|
||||
type SortKey = "created_desc" | "updated_desc" | "name_asc" | "health";
|
||||
export type SortKey = "created_desc" | "updated_desc" | "name_asc" | "health";
|
||||
|
||||
const SORT_OPTIONS: { value: SortKey; label: string }[] = [
|
||||
{ value: "created_desc", label: "Recently created" },
|
||||
|
|
@ -60,32 +60,33 @@ const HEALTH_RANK: Record<string, number> = {
|
|||
healthy: 2,
|
||||
};
|
||||
|
||||
const compareServers = (a: MCPServer, b: MCPServer, sort: SortKey): number => {
|
||||
const compareByName = (a: MCPServer, b: MCPServer): number => {
|
||||
const nameA = (a.server_name || a.alias || a.server_id).toLowerCase();
|
||||
const nameB = (b.server_name || b.alias || b.server_id).toLowerCase();
|
||||
return nameA.localeCompare(nameB) || a.server_id.localeCompare(b.server_id);
|
||||
};
|
||||
|
||||
const compareByTimestampDesc = (a: string | null | undefined, b: string | null | undefined): number => {
|
||||
const ta = a ? new Date(a).getTime() : 0;
|
||||
const tb = b ? new Date(b).getTime() : 0;
|
||||
return tb - ta;
|
||||
};
|
||||
|
||||
export const compareServers = (a: MCPServer, b: MCPServer, sort: SortKey): number => {
|
||||
switch (sort) {
|
||||
case "name_asc": {
|
||||
const nameA = (a.server_name || a.alias || a.server_id).toLowerCase();
|
||||
const nameB = (b.server_name || b.alias || b.server_id).toLowerCase();
|
||||
return nameA.localeCompare(nameB);
|
||||
}
|
||||
case "updated_desc": {
|
||||
const ta = a.updated_at ? new Date(a.updated_at).getTime() : 0;
|
||||
const tb = b.updated_at ? new Date(b.updated_at).getTime() : 0;
|
||||
return tb - ta;
|
||||
}
|
||||
case "name_asc":
|
||||
return compareByName(a, b);
|
||||
case "updated_desc":
|
||||
return compareByTimestampDesc(a.updated_at, b.updated_at) || compareByName(a, b);
|
||||
case "health": {
|
||||
const ra = HEALTH_RANK[a.status ?? "unknown"] ?? 1;
|
||||
const rb = HEALTH_RANK[b.status ?? "unknown"] ?? 1;
|
||||
if (ra !== rb) return ra - rb;
|
||||
const ta = a.created_at ? new Date(a.created_at).getTime() : 0;
|
||||
const tb = b.created_at ? new Date(b.created_at).getTime() : 0;
|
||||
return tb - ta;
|
||||
return compareByTimestampDesc(a.created_at, b.created_at) || compareByName(a, b);
|
||||
}
|
||||
case "created_desc":
|
||||
default: {
|
||||
const ta = a.created_at ? new Date(a.created_at).getTime() : 0;
|
||||
const tb = b.created_at ? new Date(b.created_at).getTime() : 0;
|
||||
return tb - ta;
|
||||
}
|
||||
default:
|
||||
return compareByTimestampDesc(a.created_at, b.created_at) || compareByName(a, b);
|
||||
}
|
||||
};
|
||||
|
||||
|
|
@ -297,13 +298,7 @@ const MCPServers: React.FC<MCPServerProps> = ({ accessToken, userRole, userID })
|
|||
server.mcp_access_groups?.some((g: any) => (typeof g === "string" ? g === group : g && g.name === group)),
|
||||
);
|
||||
}
|
||||
const sorted = [...filtered].sort((a, b) => {
|
||||
if (!a.created_at && !b.created_at) return 0;
|
||||
if (!a.created_at) return 1;
|
||||
if (!b.created_at) return -1;
|
||||
return new Date(b.created_at).getTime() - new Date(a.created_at).getTime();
|
||||
});
|
||||
setFilteredServers(sorted);
|
||||
setFilteredServers(filtered);
|
||||
},
|
||||
[serversWithHealth],
|
||||
);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue