From 2bb1c4bf2ddb3ef1f92780378ecf1c5c2fccce3a Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:05:56 +0000 Subject: [PATCH] feat(ui): write mcp tool permissions as overrides over convention defaults Co-Authored-By: bot_apk --- .../mcp_server/tool_permission_backfill.py | 15 +- litellm/types/agents.py | 2 + .../test_tool_permission_backfill.py | 16 ++ ui/litellm-dashboard/eslint-suppressions.json | 10 +- .../agents/_components/AgentFormKit.tsx | 1 + .../agents/_components/add_agent_form.tsx | 6 + .../agents/_components/agent_config.ts | 2 + .../agents/_components/agent_info.tsx | 3 + .../users/_components/user_edit_view.tsx | 7 + .../_components/view_users/user_info_view.tsx | 3 +- ui/litellm-dashboard/src/components/Teams.tsx | 9 + .../MCPToolPermissions.test.tsx | 178 ++++++++++++++++-- .../MCPToolPermissions.tsx | 130 ++++++++----- .../effectiveMcpServers.test.ts | 169 +++++++++++++++++ .../effectiveMcpServers.ts | 116 +++++++++++- .../mcp_server_management/mcpEntitlement.ts | 17 ++ .../components/organisms/createKeyPayload.ts | 10 + .../organisms/create_key_button.tsx | 10 + .../src/components/team/TeamInfo.test.tsx | 1 + .../src/components/team/TeamInfo.tsx | 11 ++ .../components/templates/keyEditFormValues.ts | 5 + .../components/templates/key_edit_view.tsx | 2 + .../components/templates/key_info_view.tsx | 1 + ui/litellm-dashboard/src/lib/http/schema.d.ts | 26 +++ .../utils/mcpToolCrudClassification.test.ts | 24 +++ .../src/utils/mcpToolCrudClassification.ts | 140 +++++++++++--- 26 files changed, 812 insertions(+), 102 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py b/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py index fbdfd62562e..c0c459512c0 100644 --- a/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py +++ b/litellm/proxy/_experimental/mcp_server/tool_permission_backfill.py @@ -247,14 +247,21 @@ async def _convert_one_row( conversion: Final = convert_row(row, await gather_inventories(granted, manager, inventory_cache)) if isinstance(conversion, Unavailable): return conversion.server_ids + stored_fields: Final = ( + ("mcp_servers", raw_row.mcp_servers), + ("mcp_access_groups", raw_row.mcp_access_groups), + ("mcp_toolsets", raw_row.mcp_toolsets), + ("mcp_tool_permissions", raw_row.mcp_tool_permissions), + ) updated: Final = await ObjectPermissionRepository(prisma_client).table.update_many( where={ "object_permission_id": row.object_permission_id, "mcp_permission_version": {"in": [0, None]}, - "mcp_servers": {"equals": raw_row.mcp_servers}, - "mcp_access_groups": {"equals": raw_row.mcp_access_groups}, - "mcp_toolsets": {"equals": raw_row.mcp_toolsets}, - "mcp_tool_permissions": {"equals": raw_row.mcp_tool_permissions}, + # prisma-client-py has no DbNull/JsonNull sentinel for `equals` on a + # Json? column, so a stored NULL field is left unguarded rather than + # filtered with a wrong null literal; the id and version still bound + # the CAS. + **{field: {"equals": value} for field, value in stored_fields if value is not None}, }, data={ "mcp_tool_overrides": json.dumps(dict(conversion.mcp_tool_overrides)), diff --git a/litellm/types/agents.py b/litellm/types/agents.py index 7f8d8c6af66..004cd03ecf2 100644 --- a/litellm/types/agents.py +++ b/litellm/types/agents.py @@ -6,6 +6,7 @@ from pydantic import BaseModel, ConfigDict, PrivateAttr, StrictInt from typing_extensions import ReadOnly, Required, TypedDict from litellm.types.llms.base import LiteLLMPydanticObjectBase +from litellm.types.mcp import MCPToolOverrideEntry if TYPE_CHECKING: from a2a.types import SendMessageResponse @@ -174,6 +175,7 @@ class AgentObjectPermission(TypedDict, total=False): mcp_access_groups: list[str] | None mcp_toolsets: ReadOnly[Sequence[str] | None] mcp_tool_permissions: dict[str, list[str]] | None + mcp_tool_overrides: dict[str, MCPToolOverrideEntry] | None models: list[str] | None agents: list[str] | None diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_tool_permission_backfill.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_tool_permission_backfill.py index c8355a0e925..776c5ed3929 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_tool_permission_backfill.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_tool_permission_backfill.py @@ -143,6 +143,22 @@ async def test_runner_converts_row_with_cas_update(): assert where["mcp_permission_version"] == {"in": [0, None]} +@pytest.mark.asyncio +async def test_runner_omits_cas_equals_filter_for_null_fields(): + row = _row(mcp_tool_permissions=None) + prisma = _prisma([row]) + manager = _manager(inventories={"server-a": INVENTORY}) + with patch( + "litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp.MCPRequestHandler._get_mcp_servers_from_access_groups", + AsyncMock(return_value=[]), + ): + report = await run_mcp_tool_permission_backfill(prisma, manager) + assert report.converted == {"perm-1"} + where = prisma.db.litellm_objectpermissiontable.update_many.await_args.kwargs["where"] + assert "mcp_tool_permissions" not in where + assert where["mcp_permission_version"] == {"in": [0, None]} + + @pytest.mark.asyncio async def test_runner_unavailable_server_skips_row_no_write(): prisma = _prisma([_row()]) diff --git a/ui/litellm-dashboard/eslint-suppressions.json b/ui/litellm-dashboard/eslint-suppressions.json index daf12d11743..4f93fb01047 100644 --- a/ui/litellm-dashboard/eslint-suppressions.json +++ b/ui/litellm-dashboard/eslint-suppressions.json @@ -1719,11 +1719,6 @@ "count": 1 } }, - "src/components/mcp_server_management/MCPToolPermissions.tsx": { - "local/no-complex-jsx-arrow": { - "count": 1 - } - }, "src/components/mcp_tools/MCPToolArgumentsForm.tsx": { "no-nested-ternary": { "count": 1 @@ -2070,6 +2065,9 @@ "src/components/templates/key_edit_view.tsx": { "local/filename-pascal-case": { "count": 1 + }, + "max-lines": { + "count": 1 } }, "src/components/templates/key_info_view.tsx": { @@ -2432,4 +2430,4 @@ "count": 1 } } -} \ No newline at end of file +} diff --git a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/AgentFormKit.tsx b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/AgentFormKit.tsx index 8e100d0c3ed..f09b13ba3aa 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/AgentFormKit.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/AgentFormKit.tsx @@ -93,6 +93,7 @@ export interface AgentFormValues { access_group_ids?: string[]; allowed_mcp_servers_and_groups?: McpServerSelection; mcp_tool_permissions?: Record; + mcp_tool_overrides?: Record; defaultInputModes?: string[]; defaultOutputModes?: string[]; enable_tracing?: boolean; diff --git a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.tsx b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.tsx index 5bd6ea9b83a..74c1b7fe7df 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.tsx @@ -112,6 +112,7 @@ const StepProgress: React.FC<{ current: number }> = ({ current }) => ( const SHARED_INITIAL_VALUES: AgentFormValues = { allowed_mcp_servers_and_groups: { servers: [], accessGroups: [] }, mcp_tool_permissions: {}, + mcp_tool_overrides: {}, entitlement_models: [], entitlement_agents: [], access_group_ids: [], @@ -253,6 +254,7 @@ const AddAgentForm: React.FC = ({ visible, onClose, accessTok const watchedFormValues = useWatch({ control: form.control }); const mcpSelection = useWatch({ control: form.control, name: "allowed_mcp_servers_and_groups" }); const mcpToolPermissions = useWatch({ control: form.control, name: "mcp_tool_permissions" }); + const mcpToolOverrides = useWatch({ control: form.control, name: "mcp_tool_overrides" }); // Build the discovery plan for the proxy. Different agent runtimes publish // their cards at different URL shapes: @@ -363,6 +365,7 @@ const AddAgentForm: React.FC = ({ visible, onClose, accessTok // Build object_permission from MCP Tools step (allowed_mcp_servers_and_groups, mcp_tool_permissions) const mcpServersAndGroups = values.allowed_mcp_servers_and_groups ?? {}; const toolPermissions = values.mcp_tool_permissions ?? {}; + const toolOverrides = values.mcp_tool_overrides ?? {}; const entitlementModels = values.entitlement_models ?? []; const entitlementAgents = values.entitlement_agents ?? []; const objectPermission: Record = { @@ -370,6 +373,7 @@ const AddAgentForm: React.FC = ({ visible, onClose, accessTok ...(mcpServersAndGroups.accessGroups?.length ? { mcp_access_groups: mcpServersAndGroups.accessGroups } : {}), ...(mcpServersAndGroups.toolsets?.length ? { mcp_toolsets: mcpServersAndGroups.toolsets } : {}), ...(Object.keys(toolPermissions).length ? { mcp_tool_permissions: toolPermissions } : {}), + ...(Object.keys(toolOverrides).length ? { mcp_tool_overrides: toolOverrides } : {}), ...(entitlementModels.length ? { models: entitlementModels } : {}), ...(entitlementAgents.length ? { agents: entitlementAgents } : {}), }; @@ -546,6 +550,8 @@ const AddAgentForm: React.FC = ({ visible, onClose, accessTok selectedToolsets={mcpSelection?.toolsets ?? []} toolPermissions={mcpToolPermissions ?? {}} onChange={(toolPerms: Record) => form.setValue("mcp_tool_permissions", toolPerms)} + toolOverrides={mcpToolOverrides ?? {}} + onOverridesChange={(overrides) => form.setValue("mcp_tool_overrides", overrides)} /> diff --git a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_config.ts b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_config.ts index caa3b9ef472..fedfe0ac578 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_config.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_config.ts @@ -324,6 +324,7 @@ export const parseMcpPermissionsForForm = (agent: any) => ({ toolsets: agent.object_permission?.mcp_toolsets ?? [], }, mcp_tool_permissions: agent.object_permission?.mcp_tool_permissions ?? {}, + mcp_tool_overrides: agent.object_permission?.mcp_tool_overrides ?? {}, }); /** @@ -335,6 +336,7 @@ export const buildMcpObjectPermission = (values: any) => ({ mcp_access_groups: values.allowed_mcp_servers_and_groups?.accessGroups ?? [], mcp_toolsets: values.allowed_mcp_servers_and_groups?.toolsets ?? [], mcp_tool_permissions: values.mcp_tool_permissions ?? {}, + mcp_tool_overrides: values.mcp_tool_overrides ?? {}, }); /** diff --git a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_info.tsx b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_info.tsx index adac456f232..04fb0a73c5a 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_info.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/agent_info.tsx @@ -163,6 +163,7 @@ const AgentInfoView: React.FC = ({ agentId, onClose, accessT const watchedFormValues = useWatch({ control: form.control }); const mcpSelection = useWatch({ control: form.control, name: "allowed_mcp_servers_and_groups" }); const mcpToolPermissions = useWatch({ control: form.control, name: "mcp_tool_permissions" }); + const mcpToolOverrides = useWatch({ control: form.control, name: "mcp_tool_overrides" }); const { data: mcpServers = [] } = useMCPServers(); const { data: accessGroups = [] } = useAccessGroups(); @@ -570,6 +571,8 @@ const AgentInfoView: React.FC = ({ agentId, onClose, accessT onChange={(toolPerms: Record) => form.setValue("mcp_tool_permissions", toolPerms) } + toolOverrides={mcpToolOverrides ?? {}} + onOverridesChange={(overrides) => form.setValue("mcp_tool_overrides", overrides)} /> diff --git a/ui/litellm-dashboard/src/app/(dashboard)/users/_components/user_edit_view.tsx b/ui/litellm-dashboard/src/app/(dashboard)/users/_components/user_edit_view.tsx index b7a3486c78e..ca25509fbc5 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/users/_components/user_edit_view.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/users/_components/user_edit_view.tsx @@ -8,6 +8,7 @@ import { useSeededState } from "@/components/key_team_helpers/useSeededState"; import { getModelDisplayName } from "@/components/key_team_helpers/fetch_available_models_team_key"; import MCPServerSelector from "@/components/mcp_server_management/MCPServerSelector"; import MCPToolPermissions from "@/components/mcp_server_management/MCPToolPermissions"; +import { normalizeMcpToolOverrides } from "@/components/mcp_server_management/effectiveMcpServers"; import type { ObjectPermission } from "@/components/object_permission_types"; import { MultiSelect } from "@/components/shared/MultiSelect"; import { FieldGroup } from "@/components/ui/field"; @@ -55,6 +56,9 @@ const userEditShape = { metadata: z.string().nullish(), mcp_servers_and_groups: MCP_SELECTION_SHAPE.optional(), mcp_tool_permissions: z.record(z.string(), z.array(z.string())).optional(), + mcp_tool_overrides: z + .record(z.string(), z.object({ allow: z.array(z.string()), deny: z.array(z.string()) })) + .optional(), }; const budgetSchema = (unlimitedBudget: boolean) => @@ -78,6 +82,7 @@ const buildMcpFieldValues = (objectPermission: ObjectPermission | null | undefin toolsets: objectPermission?.mcp_toolsets ?? [], }, mcp_tool_permissions: objectPermission?.mcp_tool_permissions ?? {}, + mcp_tool_overrides: normalizeMcpToolOverrides(objectPermission?.mcp_tool_overrides), }); // antd only reported the fields that were actually mounted, so the identity and @@ -340,6 +345,8 @@ export function UserEditView({ selectedToolsets={form.watch("mcp_servers_and_groups")?.toolsets || []} toolPermissions={form.watch("mcp_tool_permissions") || {}} onChange={(toolPerms) => form.setValue("mcp_tool_permissions", toolPerms)} + toolOverrides={form.watch("mcp_tool_overrides") || {}} + onOverridesChange={(overrides) => form.setValue("mcp_tool_overrides", overrides)} /> )} diff --git a/ui/litellm-dashboard/src/app/(dashboard)/users/_components/view_users/user_info_view.tsx b/ui/litellm-dashboard/src/app/(dashboard)/users/_components/view_users/user_info_view.tsx index c95badc587a..74f98c1d202 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/users/_components/view_users/user_info_view.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/users/_components/view_users/user_info_view.tsx @@ -324,7 +324,8 @@ export default function UserInfoView({ const mcpEntitlement = extractMcpEntitlement(formValues, allMcpServers, allMcpToolsets); const userFields = Object.fromEntries( Object.entries(formValues).filter( - ([field]) => field !== "mcp_servers_and_groups" && field !== "mcp_tool_permissions", + ([field]) => + field !== "mcp_servers_and_groups" && field !== "mcp_tool_permissions" && field !== "mcp_tool_overrides", ), ); diff --git a/ui/litellm-dashboard/src/components/Teams.tsx b/ui/litellm-dashboard/src/components/Teams.tsx index c2a23cef83a..2842446d30d 100644 --- a/ui/litellm-dashboard/src/components/Teams.tsx +++ b/ui/litellm-dashboard/src/components/Teams.tsx @@ -100,6 +100,7 @@ const teamCreateFieldsSchema = z.object({ }) .optional(), mcp_tool_permissions: z.record(z.string(), z.array(z.string())).optional(), + mcp_tool_overrides: z.record(z.string(), z.object({ allow: z.array(z.string()), deny: z.array(z.string()) })).optional(), allowed_agents_and_groups: z.object({ agents: z.array(z.string()), accessGroups: z.array(z.string()) }).optional(), object_permission_search_tools: z.array(z.string()).optional(), object_permission_skills: z.array(z.string()).optional(), @@ -131,6 +132,7 @@ const EMPTY_TEAM_CREATE_VALUES: TeamCreateFormValues = { allowed_passthrough_routes: undefined, allowed_mcp_servers_and_groups: undefined, mcp_tool_permissions: {}, + mcp_tool_overrides: {}, allowed_agents_and_groups: undefined, object_permission_search_tools: undefined, object_permission_skills: undefined, @@ -256,6 +258,7 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser const watchedOrganizationId = form.watch("organization_id"); const watchedMcpSelection = form.watch("allowed_mcp_servers_and_groups"); const watchedToolPermissions = form.watch("mcp_tool_permissions"); + const watchedToolOverrides = form.watch("mcp_tool_overrides"); const [selectedTeam, setSelectedTeam] = useState(null); const [selectedTeamId, setSelectedTeamId] = useQueryState("team", parseAsString.withOptions({ history: "push" })); @@ -481,6 +484,10 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser formValues.object_permission.mcp_tool_permissions = formValues.mcp_tool_permissions; 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; + delete formValues.mcp_tool_overrides; + } } // Transform allowed_mcp_access_groups into object_permission @@ -1139,6 +1146,8 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser selectedToolsets={watchedMcpSelection?.toolsets || []} toolPermissions={watchedToolPermissions || {}} onChange={(toolPerms) => form.setValue("mcp_tool_permissions", toolPerms)} + toolOverrides={watchedToolOverrides || {}} + onOverridesChange={(overrides) => form.setValue("mcp_tool_overrides", overrides)} /> diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx index 149d231fff6..b6323e91307 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx @@ -111,11 +111,12 @@ describe("MCPToolPermissions", () => { error: false, }); + // A closed allowlist makes the server legacy-editable, so Select All writes the list. renderWithProviders( , ); @@ -224,19 +225,20 @@ describe("MCPToolPermissions", () => { expect(networking.listMCPTools).toHaveBeenCalledWith(mockAccessToken, groupServer.server_id); }); - it("shows every tool selected in flat view for an unrestricted access-group server", async () => { + it("shows non-delete tools checked and the delete unchecked in flat view for an unrestricted access-group server", async () => { vi.mocked(networking.fetchMCPServers).mockResolvedValue([groupServer]); vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); - const mockOnChange = vi.fn(); + const mockOnOverridesChange = vi.fn(); renderWithProviders( , ); @@ -245,10 +247,12 @@ describe("MCPToolPermissions", () => { const [listIssues, deleteIssue] = screen.getAllByRole("checkbox"); expect(listIssues).toBeChecked(); - expect(deleteIssue).toBeChecked(); + expect(deleteIssue).not.toBeChecked(); await userEvent.click(listIssues); - expect(mockOnChange).toHaveBeenCalledWith({ [groupServer.server_id]: ["delete_issue"] }); + expect(mockOnOverridesChange).toHaveBeenCalledWith({ + [groupServer.server_id]: { allow: [], deny: ["list_issues"] }, + }); }); it("marks an access-group server as inherited and leaves a directly selected one unmarked", async () => { @@ -522,10 +526,7 @@ describe("MCPToolPermissions", () => { expect(await screen.findByText("Payments")).toBeInTheDocument(); expect(screen.queryByText("srv-collide")).not.toBeInTheDocument(); - await waitFor(() => { - expect(mockOnChange).toHaveBeenCalledWith({ "srv-collide": ["list_issues"] }); - }); - expect(mockOnChange.mock.calls.every(([written]) => !Object.hasOwn(written, "srv-twin"))).toBe(true); + expect(mockOnChange).not.toHaveBeenCalled(); expect(networking.listMCPTools).not.toHaveBeenCalledWith(mockAccessToken, "srv-twin"); }); @@ -549,7 +550,7 @@ describe("MCPToolPermissions", () => { expect(mockOnChange).not.toHaveBeenCalled(); }); - it("keeps blocking delete tools by default for a directly selected server", async () => { + it("blocks delete tools by default for a directly selected server without writing an allowlist", async () => { const directServer = { server_id: "srv-direct-1", server_name: "Direct Server", alias: "Direct Server" }; vi.mocked(networking.fetchMCPServers).mockResolvedValue([directServer]); vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); @@ -565,9 +566,12 @@ describe("MCPToolPermissions", () => { />, ); - await waitFor(() => { - expect(mockOnChange).toHaveBeenCalledWith({ [directServer.server_id]: ["list_issues"] }); - }); + await screen.findByText("Direct Server"); + await userEvent.click(screen.getByText("Flat List")); + + expect(screen.getByRole("checkbox", { name: "list_issues" })).toBeChecked(); + expect(screen.getByRole("checkbox", { name: "delete_issue" })).not.toBeChecked(); + expect(mockOnChange).not.toHaveBeenCalled(); }); it("shows a server that only a stale tool-permission entry still entitles", async () => { @@ -989,3 +993,149 @@ describe("MCPToolPermissions", () => { }); }); }); + +describe("convention (unrestricted) server overrides", () => { + const mockAccessToken = "test-token"; + const mockServerId = "srv-conv-1"; + const mockServerName = "Convention Server"; + const convServer = { server_id: mockServerId, server_name: mockServerName, alias: mockServerName }; + const convTools = [ + { name: "list_items", description: "List items" }, + { name: "get_item", description: "Fetch one item" }, + { name: "delete_item", description: "Destroy an item" }, + ]; + + const renderConvention = ({ + toolOverrides, + onOverridesChange = vi.fn(), + onChange = vi.fn(), + }: { + toolOverrides?: Record; + onOverridesChange?: (overrides: Record) => void; + onChange?: (permissions: Record) => void; + }) => + renderWithProviders( + , + ); + + beforeEach(() => { + vi.clearAllMocks(); + testQueryClient.clear(); + vi.mocked(networking.fetchMCPAccessGroups).mockResolvedValue([]); + vi.mocked(networking.fetchMCPServers).mockResolvedValue([convServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: convTools, error: false }); + }); + + it("checks non-delete tools and leaves the delete unchecked, without writing anything", async () => { + const onChange = vi.fn(); + renderConvention({ onChange }); + + await screen.findByText(mockServerName); + await userEvent.click(screen.getByText("Flat List")); + + expect(screen.getByRole("checkbox", { name: "list_items" })).toBeChecked(); + expect(screen.getByRole("checkbox", { name: "delete_item" })).not.toBeChecked(); + expect(onChange).not.toHaveBeenCalled(); + }); + + it("honors a stored deny for a non-delete tool and a stored allow for a delete", async () => { + renderConvention({ toolOverrides: { [mockServerId]: { allow: ["delete_item"], deny: ["list_items"] } } }); + + await screen.findByText(mockServerName); + await userEvent.click(screen.getByText("Flat List")); + + expect(screen.getByRole("checkbox", { name: "list_items" })).not.toBeChecked(); + expect(screen.getByRole("checkbox", { name: "delete_item" })).toBeChecked(); + }); + + it("writes a deny when a non-delete tool is unchecked and removes it when re-checked", async () => { + const Harness = () => { + const [overrides, setOverrides] = useState>({}); + return ( + <> + + {JSON.stringify(overrides)} + + ); + }; + renderWithProviders(); + + await screen.findByText(mockServerName); + await userEvent.click(screen.getByText("Flat List")); + + await userEvent.click(screen.getByRole("checkbox", { name: "list_items" })); + expect(screen.getByRole("status")).toHaveTextContent(`"${mockServerId}":{"allow":[],"deny":["list_items"]}`); + + await userEvent.click(screen.getByRole("checkbox", { name: "list_items" })); + expect(screen.getByRole("status")).toHaveTextContent(`"${mockServerId}":{"allow":[],"deny":[]}`); + }); + + it("writes an allow for a delete tool and never writes it a deny", async () => { + const onOverridesChange = vi.fn(); + renderConvention({ onOverridesChange }); + + await screen.findByText(mockServerName); + await userEvent.click(screen.getByText("Flat List")); + + await userEvent.click(screen.getByRole("checkbox", { name: "delete_item" })); + expect(onOverridesChange).toHaveBeenCalledWith({ [mockServerId]: { allow: ["delete_item"], deny: [] } }); + }); + + it("leaves other servers' entries and non-displayed tools untouched on toggle", async () => { + const onOverridesChange = vi.fn(); + const toolOverrides = { + [mockServerId]: { allow: [], deny: ["hidden_tool"] }, + "srv-other": { allow: ["x"], deny: ["y"] }, + }; + renderConvention({ toolOverrides, onOverridesChange }); + + await screen.findByText(mockServerName); + await userEvent.click(screen.getByText("Flat List")); + await userEvent.click(screen.getByRole("checkbox", { name: "list_items" })); + + const written = onOverridesChange.mock.calls.at(-1)?.[0] as Record; + expect(written["srv-other"]).toEqual({ allow: ["x"], deny: ["y"] }); + expect(written[mockServerId]).toEqual({ allow: [], deny: ["hidden_tool", "list_items"] }); + }); + + it("Select All approves the delete and clears displayed denies only", async () => { + const onOverridesChange = vi.fn(); + const toolOverrides = { [mockServerId]: { allow: [], deny: ["list_items", "hidden_tool"] } }; + renderConvention({ toolOverrides, onOverridesChange }); + + await screen.findByText(mockServerName); + await userEvent.click(screen.getByText("Select All")); + + expect(onOverridesChange).toHaveBeenCalledWith({ + [mockServerId]: { allow: ["delete_item"], deny: ["hidden_tool"] }, + }); + }); + + it("Deselect All writes denies for displayed non-deletes and removes the delete's allow", async () => { + const onOverridesChange = vi.fn(); + const toolOverrides = { [mockServerId]: { allow: ["delete_item"], deny: ["hidden_tool"] } }; + renderConvention({ toolOverrides, onOverridesChange }); + + await screen.findByText(mockServerName); + await userEvent.click(screen.getByText("Deselect All")); + + expect(onOverridesChange).toHaveBeenCalledWith({ + [mockServerId]: { allow: [], deny: ["hidden_tool", "list_items", "get_item"] }, + }); + }); +}); diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx index e26f1a6f511..df8a1bf555c 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx +++ b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useRef, useState, useMemo } from "react"; +import React, { useEffect, useState, useMemo } from "react"; import { listMCPTools } from "../networking"; import { MCPTool } from "../mcp_tools/types"; import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group"; @@ -12,9 +12,13 @@ import { NO_MCP_SERVERS_SENTINEL } from "../mcp_tools/constants"; import { EffectiveMcpServer, McpGrantSource, + McpToolOverrideEntry, + applyToolOverrideWrite, + applyToolOverrideWrites, applyToolPermissionWrite, emptyMcpAccessGroups, - mcpAllowedToolsFor, + isConventionServer, + mcpToolState, resolveEffectiveMcpServers, } from "./effectiveMcpServers"; @@ -25,6 +29,8 @@ interface MCPToolPermissionsProps { selectedToolsets?: readonly string[]; toolPermissions: Record; onChange: (toolPermissions: Record) => void; + toolOverrides?: Record; + onOverridesChange?: (toolOverrides: Record) => void; disabled?: boolean; } @@ -55,6 +61,8 @@ const MCPToolPermissions: React.FC = ({ selectedToolsets = NO_SELECTION, toolPermissions, onChange, + toolOverrides = {}, + onOverridesChange, disabled = false, }) => { const { @@ -70,14 +78,6 @@ const MCPToolPermissions: React.FC = ({ const [toolErrors, setToolErrors] = useState>({}); const [viewModes, setViewModes] = useState>({}); - // Keep a ref to the latest toolPermissions so async fetch callbacks always - // read the current value and do not overwrite sibling servers' results when - // multiple fetches complete out-of-order (stale-closure race condition). - const toolPermissionsRef = useRef(toolPermissions); - useEffect(() => { - toolPermissionsRef.current = toolPermissions; - }, [toolPermissions]); - // Every server this permission level reaches, not just the directly selected ones: a server // reached through an access group or a toolset needs its allowlist visible and editable too. const effectiveMcpInput = { @@ -87,10 +87,11 @@ const MCPToolPermissions: React.FC = ({ selectedToolsets, toolsets, toolPermissions, + toolOverrides, }; const servers = useMemo( () => resolveEffectiveMcpServers(effectiveMcpInput), - [allServers, selectedServers, selectedAccessGroups, selectedToolsets, toolsets, toolPermissions], + [allServers, selectedServers, selectedAccessGroups, selectedToolsets, toolsets, toolPermissions, toolOverrides], ); // Fetch tools for a specific server; applies delete-blocked-by-default for new servers. @@ -109,20 +110,6 @@ const MCPToolPermissions: React.FC = ({ } else { const fetchedTools: MCPTool[] = response.tools || []; setServerTools((prev) => ({ ...prev, [serverId]: fetchedTools })); - - // Default only unrestricted direct servers to non-delete tools. - // Read latest permissions from the ref to avoid clobbering concurrent results. - const latestPermissions = toolPermissionsRef.current; - const isDirect = entry.source.kind === "direct"; - const unrestricted = - mcpAllowedToolsFor(entry.server, latestPermissions, allServers) === undefined && - entry.toolsetTools === undefined; - if (isDirect && unrestricted && (selectedToolsets.length === 0 || !toolsetsFailed) && fetchedTools.length > 0) { - const nonDeleteTools = fetchedTools - .filter((t) => classifyToolOp(t.name, t.description || "") !== "delete") - .map((t) => t.name); - onChange(applyToolPermissionWrite({ toolPermissions: latestPermissions, entry, allowed: nonDeleteTools })); - } } } catch (err) { console.error(`Error fetching tools for server ${serverId}:`, err); @@ -147,20 +134,55 @@ const MCPToolPermissions: React.FC = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [servers, accessToken, toolsetsLoading]); - // Every write goes through here so an edit is authoritative for the SERVER, not for one of the - // equivalent keys that may name it. + // Every allowlist write goes through here so an edit is authoritative for the SERVER, not for + // one of the equivalent keys that may name it. const writeAllowedTools = (entry: EffectiveMcpServer, allowed: string[]) => { onChange(applyToolPermissionWrite({ toolPermissions, entry, allowed })); }; - const handleSelectAll = (entry: EffectiveMcpServer) => { - const tools = serverTools[entry.server.server_id] || []; - writeAllowedTools( - entry, - tools.map((t) => t.name), + // On a convention server there is no snapshot to edit: the checkbox flips this tool's entry in + // the server's overrides and nothing else. A delete tool can only sit in `allow`; a non-delete + // tool is carved out with `deny`. Anything not on the checkbox is left alone. + const isDelete = (tool: MCPTool) => classifyToolOp(tool.name, tool.description || "") === "delete"; + + const writeToolToggle = (entry: EffectiveMcpServer, tool: MCPTool, checked: boolean) => { + if (isConventionServer(entry)) { + const write = { + toolOverrides, + permissionKey: entry.permissionKey, + toolName: tool.name, + isDeleteTool: isDelete(tool), + checked, + }; + onOverridesChange?.(applyToolOverrideWrite(write)); + return; + } + const current = entry.allowedTools ?? (serverTools[entry.server.server_id] || []).map((t) => t.name); + writeAllowedTools(entry, checked ? [...current, tool.name] : current.filter((name) => name !== tool.name)); + }; + + // Writes the override edits that make every editable displayed tool match `checked`. + const writeConventionBulk = (entry: EffectiveMcpServer, tools: readonly MCPTool[], checked: boolean) => { + onOverridesChange?.( + applyToolOverrideWrites({ + toolOverrides, + permissionKey: entry.permissionKey, + edits: tools + .filter((tool) => !mcpToolState(entry, tool.name, isDelete(tool)).locked) + .map((tool) => ({ toolName: tool.name, isDeleteTool: isDelete(tool), checked })), + }), ); }; + const handleBulk = (entry: EffectiveMcpServer, checked: boolean) => { + const tools = serverTools[entry.server.server_id] || []; + if (isConventionServer(entry)) { + writeConventionBulk(entry, tools, checked); + return; + } + writeAllowedTools(entry, checked ? tools.map((t) => t.name) : []); + }; + // The opt-out sentinel short-circuits the backend resolver to zero servers, so nothing stored // here is in force and showing a tool matrix would claim otherwise. if (selectedServers.includes(NO_MCP_SERVERS_SENTINEL)) { @@ -222,7 +244,9 @@ const MCPToolPermissions: React.FC = ({ const serverId = server.server_id; const serverName = server.server_name || server.alias || serverId; const tools = serverTools[serverId] || []; - const selectedTools = entry.allowedTools ?? tools.map((t) => t.name); + const stateFor = (tool: MCPTool) => + mcpToolState(entry, tool.name, classifyToolOp(tool.name, tool.description || "") === "delete"); + const selectedTools = tools.filter((tool) => stateFor(tool).checked).map((tool) => tool.name); const isLoading = loadingTools[serverId]; const error = toolErrors[serverId]; const viewMode = viewModes[serverId] ?? "crud"; @@ -282,7 +306,7 @@ const MCPToolPermissions: React.FC = ({