From cb1ac0281462a4a86490094fbf3975e4eb2a4cf0 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 25 Sep 2026 03:01:35 +0000 Subject: [PATCH] fix(ui): outer deny precedence and prune overrides for deselected servers Co-Authored-By: bot_apk --- .../mcp_tool_classification_cases.json | 156 +++--------------- ui/litellm-dashboard/src/components/Teams.tsx | 17 +- .../effectiveMcpServers.test.ts | 27 +++ .../effectiveMcpServers.ts | 22 ++- .../src/components/team/TeamInfo.tsx | 10 +- .../src/utils/mcpToolCrudClassification.ts | 1 + 6 files changed, 98 insertions(+), 135 deletions(-) diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/fixtures/mcp_tool_classification_cases.json b/tests/test_litellm/proxy/_experimental/mcp_server/fixtures/mcp_tool_classification_cases.json index 68b679dc9a7..ebfecfe3c19 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/fixtures/mcp_tool_classification_cases.json +++ b/tests/test_litellm/proxy/_experimental/mcp_server/fixtures/mcp_tool_classification_cases.json @@ -1,132 +1,28 @@ [ - { - "name": "delete_link", - "description": null, - "expected": "delete" - }, - { - "name": "delete-link", - "description": null, - "expected": "delete" - }, - { - "name": "deleteLink", - "description": null, - "expected": "delete" - }, - { - "name": "DeleteLink", - "description": null, - "expected": "delete" - }, - { - "name": "DELETE_LINK", - "description": null, - "expected": "delete" - }, - { - "name": "remove_user", - "description": null, - "expected": "delete" - }, - { - "name": "purge_cache", - "description": null, - "expected": "delete" - }, - { - "name": "get_removed_entries", - "description": null, - "expected": "read" - }, - { - "name": "list_deleted_items", - "description": null, - "expected": "read" - }, - { - "name": "find_deleted", - "description": null, - "expected": "read" - }, - { - "name": "describe_purge_job", - "description": null, - "expected": "delete" - }, - { - "name": "updateItem", - "description": null, - "expected": "update" - }, - { - "name": "create-record", - "description": null, - "expected": "create" - }, - { - "name": "foo", - "description": "Deletes the file", - "expected": "delete" - }, - { - "name": "foo", - "description": "Lists files", - "expected": "read" - }, - { - "name": "delete_link", - "description": "Reads a link", - "expected": "delete" - }, - { - "name": "get_removed_entries", - "description": "Permanently deletes", - "expected": "read" - }, - { - "name": "unlinkNode", - "description": null, - "expected": "delete" - }, - { - "name": "checkDeletion", - "description": null, - "expected": "read" - }, - { - "name": "rm_file", - "description": null, - "expected": "delete" - }, - { - "name": "x", - "description": null, - "expected": "unknown" - }, - { - "name": "info", - "description": null, - "expected": "read" - }, - { - "name": "settings", - "description": null, - "expected": "unknown" - }, - { - "name": "get_and_delete_item", - "description": null, - "expected": "delete" - }, - { - "name": "fetchAndDelete", - "description": null, - "expected": "delete" - }, - { - "name": "read_then_remove", - "description": null, - "expected": "delete" - } + {"name": "delete_link", "description": null, "expected": "delete"}, + {"name": "delete-link", "description": null, "expected": "delete"}, + {"name": "deleteLink", "description": null, "expected": "delete"}, + {"name": "DeleteLink", "description": null, "expected": "delete"}, + {"name": "DELETE_LINK", "description": null, "expected": "delete"}, + {"name": "remove_user", "description": null, "expected": "delete"}, + {"name": "purge_cache", "description": null, "expected": "delete"}, + {"name": "get_removed_entries", "description": null, "expected": "read"}, + {"name": "list_deleted_items", "description": null, "expected": "read"}, + {"name": "find_deleted", "description": null, "expected": "read"}, + {"name": "describe_purge_job", "description": null, "expected": "delete"}, + {"name": "updateItem", "description": null, "expected": "update"}, + {"name": "create-record", "description": null, "expected": "create"}, + {"name": "foo", "description": "Deletes the file", "expected": "delete"}, + {"name": "foo", "description": "Lists files", "expected": "read"}, + {"name": "delete_link", "description": "Reads a link", "expected": "delete"}, + {"name": "get_removed_entries", "description": "Permanently deletes", "expected": "read"}, + {"name": "unlinkNode", "description": null, "expected": "delete"}, + {"name": "checkDeletion", "description": null, "expected": "read"}, + {"name": "rm_file", "description": null, "expected": "delete"}, + {"name": "x", "description": null, "expected": "unknown"}, + {"name": "info", "description": null, "expected": "read"}, + {"name": "settings", "description": null, "expected": "unknown"}, + {"name": "get_and_delete_item", "description": null, "expected": "delete"}, + {"name": "fetchAndDelete", "description": null, "expected": "delete"}, + {"name": "read_then_remove", "description": null, "expected": "delete"} ] diff --git a/ui/litellm-dashboard/src/components/Teams.tsx b/ui/litellm-dashboard/src/components/Teams.tsx index 2842446d30d..e48571c9c8f 100644 --- a/ui/litellm-dashboard/src/components/Teams.tsx +++ b/ui/litellm-dashboard/src/components/Teams.tsx @@ -40,6 +40,9 @@ import { fetchAvailableModelsForTeamOrKey } from "./key_team_helpers/fetch_avail import type { Team } from "./key_team_helpers/key_list"; import MCPServerSelector from "./mcp_server_management/MCPServerSelector"; import MCPToolPermissions from "./mcp_server_management/MCPToolPermissions"; +import { extractMcpEntitlement } from "./mcp_server_management/mcpEntitlement"; +import { useMCPServers } from "@/app/(dashboard)/hooks/mcpServers/useMCPServers"; +import { useMCPToolsets } from "@/app/(dashboard)/hooks/mcpServers/useMCPToolsets"; import { toast } from "@/lib/toast"; import { extractProxyErrorMessage } from "@/lib/http/client"; import BudgetDurationDropdown, { @@ -259,6 +262,8 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser const watchedMcpSelection = form.watch("allowed_mcp_servers_and_groups"); const watchedToolPermissions = form.watch("mcp_tool_permissions"); const watchedToolOverrides = form.watch("mcp_tool_overrides"); + const { data: allMcpServers = [] } = useMCPServers(); + const { data: allMcpToolsets = [] } = useMCPToolsets(); const [selectedTeam, setSelectedTeam] = useState(null); const [selectedTeamId, setSelectedTeamId] = useQueryState("team", parseAsString.withOptions({ history: "push" })); @@ -485,7 +490,17 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser delete formValues.mcp_tool_permissions; } if (formValues.mcp_tool_overrides && Object.keys(formValues.mcp_tool_overrides).length > 0) { - formValues.object_permission.mcp_tool_overrides = formValues.mcp_tool_overrides; + const entitlement = extractMcpEntitlement( + { + mcp_servers_and_groups: formValues.allowed_mcp_servers_and_groups, + mcp_tool_overrides: formValues.mcp_tool_overrides, + }, + allMcpServers, + allMcpToolsets, + ); + if (entitlement && Object.keys(entitlement.mcp_tool_overrides).length > 0) { + formValues.object_permission.mcp_tool_overrides = entitlement.mcp_tool_overrides; + } delete formValues.mcp_tool_overrides; } } diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.test.ts b/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.test.ts index ff25a984b9f..3e6b97dab8e 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.test.ts +++ b/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.test.ts @@ -12,6 +12,7 @@ import { mcpToolPermissionKeyFor, mcpToolState, resolveEffectiveMcpServers, + retainedMcpToolOverrides, } from "./effectiveMcpServers"; const server = (overrides: Partial & { server_id: string }): MCPServer => @@ -715,3 +716,29 @@ describe("convention servers and tool overrides", () => { expect(applyToolOverrideWrites(deselectAll)).toEqual({ "srv-1": { allow: [], deny: ["list_pages"] } }); }); }); + +describe("retainedMcpToolOverrides", () => { + const catalog = [server({ server_id: "srv-1" }), server({ server_id: "srv-2", alias: "shared" })]; + + it("drops the override for a server the save no longer grants", () => { + expect( + retainedMcpToolOverrides( + { "srv-1": { allow: [], deny: ["list_pages"] }, "srv-2": { allow: ["delete_page"], deny: [] } }, + new Set(["srv-2"]), + catalog, + ), + ).toEqual({ "srv-2": { allow: ["delete_page"], deny: [] } }); + }); + + it("keeps an override while any server the key names stays granted", () => { + expect( + retainedMcpToolOverrides({ shared: { allow: [], deny: ["t"] } }, new Set(["srv-2"]), catalog), + ).toEqual({ shared: { allow: [], deny: ["t"] } }); + }); + + it("keeps an override whose key resolves to nothing, so an unloaded catalog prunes nothing", () => { + expect(retainedMcpToolOverrides({ "not-yet-loaded": { allow: [], deny: ["t"] } }, new Set(), catalog)).toEqual( + { "not-yet-loaded": { allow: [], deny: ["t"] } }, + ); + }); +}); diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts b/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts index fc50b06b46c..461ad7e4c25 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts +++ b/ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts @@ -271,8 +271,14 @@ export const mcpToolState = ( isDeleteTool: boolean, ): McpToolState => { const denied = (entry.overrides?.deny ?? []).includes(toolName); + if (denied) { + if (entry.keyedTools !== undefined) { + return { checked: entry.keyedTools.includes(toolName), locked: false }; + } + return { checked: false, locked: entry.toolsetTools !== undefined }; + } if ((entry.toolsetTools ?? []).includes(toolName)) { - return { checked: !denied, locked: true }; + return { checked: true, locked: true }; } if (entry.keyedTools !== undefined) { return { checked: entry.keyedTools.includes(toolName), locked: false }; @@ -281,12 +287,24 @@ export const mcpToolState = ( return { checked: false, locked: true }; } const allowed = (entry.overrides?.allow ?? []).includes(toolName); - return { checked: (isDeleteTool ? false : !denied) || (allowed && !denied), locked: false }; + return { checked: !isDeleteTool || allowed, locked: false }; }; export const isConventionServer = (entry: EffectiveMcpServer): boolean => entry.keyedTools === undefined && entry.toolsetTools === undefined; +export const retainedMcpToolOverrides = ( + toolOverrides: Readonly>, + grantedServerIds: ReadonlySet, + knownServers: readonly MCPServer[], +): Record => + Object.fromEntries( + Object.entries(toolOverrides).filter(([permissionKey]) => { + const named = mcpServersForIdentifier(knownServers, permissionKey); + return named.length === 0 || named.some((server) => grantedServerIds.has(server.server_id)); + }), + ); + export const applyToolOverrideWrite = ({ toolOverrides, permissionKey, diff --git a/ui/litellm-dashboard/src/components/team/TeamInfo.tsx b/ui/litellm-dashboard/src/components/team/TeamInfo.tsx index 738e9d225e0..313fd15e1d6 100644 --- a/ui/litellm-dashboard/src/components/team/TeamInfo.tsx +++ b/ui/litellm-dashboard/src/components/team/TeamInfo.tsx @@ -94,6 +94,7 @@ import { mcpServersForIdentifier, normalizeMcpToolOverrides, resolveEffectiveMcpServers, + retainedMcpToolOverrides, type EffectiveMcpServer, } from "../mcp_server_management/effectiveMcpServers"; import type { MCPServer } from "../mcp_tools/types"; @@ -1109,6 +1110,11 @@ const TeamInfoView: React.FC = ({ mcpResolution.kind === "resolved" ? retainedMcpToolPermissions(submittedToolPermissions, mcpResolution.serverIds, allMcpServers) : submittedToolPermissions; + const submittedToolOverrides = values.mcp_tool_overrides || {}; + const mcpToolOverrides = + mcpResolution.kind === "resolved" + ? retainedMcpToolOverrides(submittedToolOverrides, mcpResolution.serverIds, allMcpServers) + : submittedToolOverrides; updateData.object_permission = {}; if (servers) { @@ -1120,8 +1126,8 @@ const TeamInfoView: React.FC = ({ if (mcpToolPermissions) { updateData.object_permission.mcp_tool_permissions = mcpToolPermissions; } - if (values.mcp_tool_overrides && Object.keys(values.mcp_tool_overrides).length > 0) { - updateData.object_permission.mcp_tool_overrides = values.mcp_tool_overrides; + if (Object.keys(mcpToolOverrides).length > 0) { + updateData.object_permission.mcp_tool_overrides = mcpToolOverrides; } if (toolsets) { updateData.object_permission.mcp_toolsets = toolsets; diff --git a/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts b/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts index cfeacae03c9..56a4de86959 100644 --- a/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts +++ b/ui/litellm-dashboard/src/utils/mcpToolCrudClassification.ts @@ -100,6 +100,7 @@ const tokenVariants = (token: string): string[] => // Matches litellm/proxy/_experimental/mcp_server/tool_classification.py, fixture-pinned. const classifyTokens = (tokens: string[]): CrudOp => { + if (tokens.some((token) => DELETE_TOKENS.has(token))) return "delete"; const variants = new Set(tokens.flatMap((token) => tokenVariants(token))); if ([...variants].some((variant) => READ_TOKENS.has(variant))) return "read"; if ([...variants].some((variant) => DELETE_TOKENS.has(variant))) return "delete";