From ffd8dff5cfd9c20bb777c9d4e7b0b440e3a58658 Mon Sep 17 00:00:00 2001 From: yassin Date: Sat, 5 Sep 2026 20:41:08 +0000 Subject: [PATCH] fix(ui): keep standing MCP grants on team save Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../src/components/team/TeamInfo.test.tsx | 55 ++++++++++++++--- .../src/components/team/TeamInfo.tsx | 61 +++++++++++++++++-- 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/ui/litellm-dashboard/src/components/team/TeamInfo.test.tsx b/ui/litellm-dashboard/src/components/team/TeamInfo.test.tsx index d77391c5eaf..f5afb636245 100644 --- a/ui/litellm-dashboard/src/components/team/TeamInfo.test.tsx +++ b/ui/litellm-dashboard/src/components/team/TeamInfo.test.tsx @@ -7,7 +7,12 @@ import { chooseSelectOption, renderWithProviders, testQueryClient } from "../../ import { toast } from "@/lib/toast"; import type { EffectiveMcpServer } from "../mcp_server_management/effectiveMcpServers"; import type { MCPServer } from "../mcp_tools/types"; -import TeamInfoView, { grantedMcpServerIds, retainedMcpToolPermissions, type TeamData } from "./TeamInfo"; +import TeamInfoView, { + grantedMcpServerIds, + retainedMcpToolPermissions, + standingToolPermissionServerIds, + type TeamData, +} from "./TeamInfo"; const authState = vi.hoisted(() => ({ userRole: "Admin" })); @@ -2286,6 +2291,7 @@ describe("TeamInfo MCP permission retention", () => { [effective("direct", "direct"), effective("inherited", "toolPermission")], ["ag-1"], [{ access_group_id: "ag-1", access_mcp_server_ids: ["inherited"] }], + new Set(), ); expect(resolution.kind).toBe("resolved"); if (resolution.kind !== "resolved") return; @@ -2318,7 +2324,7 @@ describe("TeamInfo MCP permission retention", () => { }); it("refuses an unresolved selected access group", () => { - expect(grantedMcpServerIds([effective("server-1", "direct")], ["missing"], [])).toEqual({ + expect(grantedMcpServerIds([effective("server-1", "direct")], ["missing"], [], new Set())).toEqual({ kind: "unresolvable", reason: expect.stringMatching(/access groups could not be loaded/), }); @@ -2330,35 +2336,66 @@ describe("TeamInfo MCP permission retention", () => { }); }); - it("retains an ambiguous name-keyed permission when one matching server remains granted", () => { + it("splits an ambiguous permission across the matching granted server", () => { const catalog = [server("server-1", "shared"), server("server-2", "shared")]; - expect(retainedMcpToolPermissions({ shared: ["read"] }, new Set(["server-1"]), catalog)).toEqual({ + expect( + retainedMcpToolPermissions({ shared: ["read"], "server-1": ["write"] }, new Set(["server-1"]), catalog), + ).toEqual({ + "server-1": ["write", "read"], + }); + }); + + it("keeps an ambiguous permission when all matching servers remain granted", () => { + const catalog = [server("server-1", "shared"), server("server-2", "shared")]; + + expect(retainedMcpToolPermissions({ shared: ["read"] }, new Set(["server-1", "server-2"]), catalog)).toEqual({ shared: ["read"], }); }); + it("identifies standing tool-permission grants not covered by loaded access groups", () => { + expect( + standingToolPermissionServerIds( + [effective("a", "toolPermission"), effective("b", "toolPermission"), effective("c", "direct")], + ["ag-1"], + [{ access_group_id: "ag-1", access_mcp_server_ids: ["b"] }], + ), + ).toEqual(new Set(["a"])); + }); + + it("includes standing tool-permission grants in the resolved server ids", () => { + expect(grantedMcpServerIds([effective("x", "toolPermission")], [], [], new Set(["x"]))).toEqual({ + kind: "resolved", + serverIds: new Set(["x"]), + }); + expect(grantedMcpServerIds([effective("x", "toolPermission")], [], [], new Set())).toEqual({ + kind: "resolved", + serverIds: new Set(), + }); + }); + it("retains an indirectly granted server on an unrelated team save", async () => { const user = userEvent.setup({ delay: null }); - const catalog = [server("direct-server", "deploy_tracker"), server("group-server", "issue_tracker")]; + const catalog = [server("direct-server", "deploy_tracker"), server("perm-only-server", "issue_tracker")]; mockUseMCPServers.mockReturnValue({ data: catalog, isLoading: false, isError: false } as any); mockUseMCPToolsets.mockReturnValue({ data: [], isLoading: false, isError: false } as any); mockUseAccessGroups.mockReturnValue({ - data: [{ access_group_id: "ag-1", access_group_name: "Ops", access_mcp_server_ids: ["group-server"] }], + data: [], isLoading: false, isError: false, } as any); vi.mocked(networking.teamInfoCall).mockResolvedValue( createMockTeamData({ models: ["gpt-4"], - access_group_ids: ["ag-1"], + access_group_ids: [], object_permission: { mcp_servers: ["direct-server"], mcp_access_groups: [], mcp_toolsets: [], mcp_tool_permissions: { "direct-server": ["create_issue"], - "group-server": ["list_issues"], + "perm-only-server": ["list_issues"], }, }, }), @@ -2388,7 +2425,7 @@ describe("TeamInfo MCP permission retention", () => { const [, payload] = vi.mocked(networking.teamUpdateCall).mock.calls[0]; expect(payload.object_permission.mcp_tool_permissions).toEqual({ "direct-server": ["create_issue"], - "group-server": ["list_issues"], + "perm-only-server": ["list_issues"], }); }); diff --git a/ui/litellm-dashboard/src/components/team/TeamInfo.tsx b/ui/litellm-dashboard/src/components/team/TeamInfo.tsx index 6331072ec34..f4e0b4928c0 100644 --- a/ui/litellm-dashboard/src/components/team/TeamInfo.tsx +++ b/ui/litellm-dashboard/src/components/team/TeamInfo.tsx @@ -131,10 +131,28 @@ export type McpGrantResolution = | { readonly kind: "resolved"; readonly serverIds: ReadonlySet } | { readonly kind: "unresolvable"; readonly reason: string }; +export const standingToolPermissionServerIds = ( + loadedEffectiveServers: readonly EffectiveMcpServer[], + loadedAccessGroupIds: readonly string[], + accessGroups: readonly Pick[], +): ReadonlySet => { + const loadedUnifiedServerIds = new Set( + accessGroups + .filter((group) => loadedAccessGroupIds.includes(group.access_group_id)) + .flatMap((group) => group.access_mcp_server_ids), + ); + return new Set( + loadedEffectiveServers + .filter(({ source, server }) => source.kind === "toolPermission" && !loadedUnifiedServerIds.has(server.server_id)) + .map(({ server }) => server.server_id), + ); +}; + export const grantedMcpServerIds = ( effectiveServers: readonly EffectiveMcpServer[], selectedAccessGroupIds: readonly string[], accessGroups: readonly Pick[], + standingServerIds: ReadonlySet, ): McpGrantResolution => { const selectedGroups = accessGroups.filter((group) => selectedAccessGroupIds.includes(group.access_group_id)); if (selectedAccessGroupIds.some((id) => !selectedGroups.some((group) => group.access_group_id === id))) { @@ -144,20 +162,36 @@ export const grantedMcpServerIds = ( .filter(({ source }) => source.kind !== "toolPermission") .map(({ server }) => server.server_id); const viaUnifiedGroups = selectedGroups.flatMap((group) => group.access_mcp_server_ids); - return { kind: "resolved", serverIds: new Set([...direct, ...viaUnifiedGroups]) }; + return { kind: "resolved", serverIds: new Set([...direct, ...viaUnifiedGroups, ...standingServerIds]) }; }; export const retainedMcpToolPermissions = ( toolPermissions: Record, grantedServerIds: ReadonlySet, knownServers: readonly MCPServer[], -): Record => - Object.fromEntries( - Object.entries(toolPermissions).filter(([key]) => { - const named = mcpServersForIdentifier(knownServers, key); - return named.length === 0 || named.some((server) => grantedServerIds.has(server.server_id)); +): Record => { + const entries = Object.entries(toolPermissions).flatMap(([key, tools]) => { + const named = mcpServersForIdentifier(knownServers, key); + const granted = named.filter((server) => grantedServerIds.has(server.server_id)); + if (named.length === 0 || granted.length === named.length) { + return [[key, tools] as const]; + } + if (granted.length === 0) { + return []; + } + return granted.map(({ server_id }) => [ + server_id, + [...(toolPermissions[server_id] ?? []), ...tools], + ] as const); + }); + return entries.reduce>( + (retained, [key, tools]) => ({ + ...retained, + [key]: [...new Set([...(retained[key] ?? []), ...tools])], }), + {}, ); +}; export const mcpUnresolvableSaveError = (reason: string): string => `Cannot save MCP tool permissions because ${reason}. Retry once the page has finished loading`; @@ -902,6 +936,20 @@ const TeamInfoView: React.FC = ({ toolsets: allMcpToolsets, toolPermissions: submittedToolPermissions, }; + const loadedObjectPermission = info.object_permission ?? {}; + const loadedEffectiveMcpServers = resolveEffectiveMcpServers({ + allServers: allMcpServers, + selectedServers: loadedObjectPermission.mcp_servers ?? [], + selectedAccessGroups: loadedObjectPermission.mcp_access_groups ?? [], + selectedToolsets: loadedObjectPermission.mcp_toolsets ?? [], + toolsets: allMcpToolsets, + toolPermissions: loadedObjectPermission.mcp_tool_permissions ?? {}, + }); + const standingServerIds = standingToolPermissionServerIds( + loadedEffectiveMcpServers, + info.access_group_ids ?? [], + allAccessGroups, + ); const mcpResolution: McpGrantResolution = mcpLookupFailure !== null ? { kind: "unresolvable", reason: mcpLookupFailure } @@ -909,6 +957,7 @@ const TeamInfoView: React.FC = ({ resolveEffectiveMcpServers(effectiveMcpInput), values.access_group_ids || [], allAccessGroups, + standingServerIds, ); if (mcpResolution.kind === "unresolvable" && Object.keys(submittedToolPermissions).length > 0) { toast.fromError(mcpUnresolvableSaveError(mcpResolution.reason));