diff --git a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.test.tsx index 4e637344e2c..ccac244f019 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.test.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.test.tsx @@ -24,6 +24,30 @@ vi.mock("./agent_form_fields", () => ({ default: () =>
, })); +vi.mock("@/components/mcp_server_management/MCPServerSelector", () => ({ + default: ({ + onChange, + }: { + onChange: (selection: { servers: string[]; accessGroups: string[]; toolsets: string[] }) => void; + }) => ( + + ), +})); + +vi.mock("@/components/mcp_server_management/MCPToolPermissions", () => ({ + default: () => null, +})); + +vi.mock("@/components/common_components/team_dropdown", () => ({ + default: () => null, +})); + const a2aInfo: AgentCreateInfo = { agent_type: "a2a", agent_type_display_name: "A2A Agent", @@ -97,4 +121,25 @@ describe("AddAgentForm logos", () => { expect(warnSpy).toHaveBeenCalledTimes(2); warnSpy.mockRestore(); }); + + it("includes selected MCP toolsets in the create payload", async () => { + const user = userEvent.setup({ pointerEventsCheck: PointerEventsCheckLevel.Never }); + vi.mocked(networking.createAgentCall).mockResolvedValue({ + agent_id: "agent-1", + agent_name: "Test Agent", + } as never); + vi.mocked(networking.keyListCall).mockResolvedValue({ keys: [] }); + + renderForm(); + await user.click(screen.getByRole("button", { name: "Next →" })); + await user.click(screen.getByTestId("select-mcp-toolset")); + await user.click(screen.getByRole("button", { name: "Next →" })); + await user.click(screen.getByRole("button", { name: "Next →" })); + await user.click(screen.getByText(/Skip for now/)); + await user.click(screen.getByRole("button", { name: "Create Agent →" })); + + await vi.waitFor(() => expect(networking.createAgentCall).toHaveBeenCalled()); + const [, payload] = vi.mocked(networking.createAgentCall).mock.calls[0]; + expect(payload.object_permission).toEqual({ mcp_toolsets: ["ts-1"] }); + }); }); 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 51445ec1bdb..108bae977e1 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 @@ -361,6 +361,7 @@ const AddAgentForm: React.FC = ({ visible, onClose, accessTok const objectPermission: Record = { ...(mcpServersAndGroups.servers?.length ? { mcp_servers: mcpServersAndGroups.servers } : {}), ...(mcpServersAndGroups.accessGroups?.length ? { mcp_access_groups: mcpServersAndGroups.accessGroups } : {}), + ...(mcpServersAndGroups.toolsets?.length ? { mcp_toolsets: mcpServersAndGroups.toolsets } : {}), ...(Object.keys(toolPermissions).length ? { mcp_tool_permissions: toolPermissions } : {}), ...(entitlementModels.length ? { models: entitlementModels } : {}), ...(entitlementAgents.length ? { agents: entitlementAgents } : {}), @@ -520,6 +521,8 @@ const AddAgentForm: React.FC = ({ visible, onClose, accessTok ) => form.setValue("mcp_tool_permissions", toolPerms)} /> diff --git a/ui/litellm-dashboard/src/components/Teams.test.tsx b/ui/litellm-dashboard/src/components/Teams.test.tsx index 2bceb00aae1..c7df35197e6 100644 --- a/ui/litellm-dashboard/src/components/Teams.test.tsx +++ b/ui/litellm-dashboard/src/components/Teams.test.tsx @@ -17,6 +17,22 @@ import { import Teams from "./Teams"; import { chooseSelectOption } from "../../tests/test-utils"; +vi.mock("./mcp_server_management/MCPServerSelector", () => ({ + default: ({ + onChange, + }: { + onChange: (selection: { servers: string[]; accessGroups: string[]; toolsets: string[] }) => void; + }) => ( + + ), +})); + const can = vi.fn(); vi.mock("@/app/(dashboard)/hooks/useCan", () => ({ default: (...args: unknown[]) => can(...args), @@ -1343,6 +1359,16 @@ describe("Teams - the exact bytes the create call sends", () => { }); }); + it("includes selected MCP toolsets in the create object permission", async () => { + await openCreateModal(); + await openSection("MCP Settings", /Allowed MCP Servers/); + fireEvent.click(screen.getByTestId("select-mcp-toolset")); + + const payload = await submit(); + + expect(payload.object_permission).toStrictEqual({ mcp_toolsets: ["ts-1"] }); + }); + it.each([ ["MCP Settings", /Allowed MCP Servers/, ["allowed_mcp_servers_and_groups", "mcp_tool_permissions"]], ["Agent Settings", /Allowed Agents/, ["allowed_agents_and_groups"]], diff --git a/ui/litellm-dashboard/src/components/Teams.tsx b/ui/litellm-dashboard/src/components/Teams.tsx index 4be91f22339..c4060163c78 100644 --- a/ui/litellm-dashboard/src/components/Teams.tsx +++ b/ui/litellm-dashboard/src/components/Teams.tsx @@ -443,6 +443,7 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser (formValues.allowed_mcp_servers_and_groups && (formValues.allowed_mcp_servers_and_groups.servers?.length > 0 || formValues.allowed_mcp_servers_and_groups.accessGroups?.length > 0 || + formValues.allowed_mcp_servers_and_groups.toolsets?.length > 0 || formValues.allowed_mcp_servers_and_groups.toolPermissions)) ) { if (!formValues.object_permission) { @@ -453,13 +454,16 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser delete formValues.allowed_vector_store_ids; } if (formValues.allowed_mcp_servers_and_groups) { - const { servers, accessGroups } = formValues.allowed_mcp_servers_and_groups; + const { servers, accessGroups, toolsets } = formValues.allowed_mcp_servers_and_groups; if (servers && servers.length > 0) { formValues.object_permission.mcp_servers = servers; } if (accessGroups && accessGroups.length > 0) { formValues.object_permission.mcp_access_groups = accessGroups; } + if (toolsets && toolsets.length > 0) { + formValues.object_permission.mcp_toolsets = toolsets; + } delete formValues.allowed_mcp_servers_and_groups; } @@ -1086,6 +1090,8 @@ const Teams: React.FC = ({ accessToken, userID, userRole, premiumUser form.setValue("mcp_tool_permissions", toolPerms)} /> 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 91f1a45f858..69a761b4723 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 @@ -2,9 +2,11 @@ import { useState } from "react"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { renderWithProviders } from "../../../tests/test-utils"; +import { renderWithProviders, testQueryClient } from "../../../tests/test-utils"; import MCPToolPermissions from "./MCPToolPermissions"; import * as networking from "../networking"; +import { NO_MCP_SERVERS_SENTINEL } from "../mcp_tools/constants"; +import type { MCPToolset } from "../mcp_tools/types"; vi.mock("../networking"); @@ -15,6 +17,8 @@ describe("MCPToolPermissions", () => { beforeEach(() => { vi.clearAllMocks(); + testQueryClient.clear(); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); }); it("should update tool permissions when user selects a tool", async () => { @@ -71,9 +75,10 @@ describe("MCPToolPermissions", () => { await userEvent.click(screen.getByRole("checkbox", { name: "read_wiki_structure" })); // Verify onChange was called with read_wiki_structure removed - expect(mockOnChange).toHaveBeenCalledWith({ + const expectedToolPermissions = { [mockServerId]: ["read_wiki_contents", "ask_question"], - }); + }; + expect(mockOnChange).toHaveBeenCalledWith(expectedToolPermissions); // Verify API calls // Note: useMCPServers uses useAuthorized() internally, which returns "123" from global mock @@ -184,6 +189,654 @@ describe("MCPToolPermissions", () => { }); }); + describe("servers reached indirectly", () => { + const groupServer = { + server_id: "srv-group-1", + server_name: "Group Server", + alias: "Group Server", + mcp_access_groups: ["production-group"], + }; + const groupTools = [ + { name: "list_issues", description: "List issues" }, + { name: "delete_issue", description: "Delete an issue" }, + ]; + + it("renders the tool matrix for a server granted only through an access group", 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(); + renderWithProviders( + , + ); + + expect(await screen.findByText("Group Server")).toBeInTheDocument(); + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + expect(screen.getByText("delete_issue")).toBeInTheDocument(); + expect(networking.listMCPTools).toHaveBeenCalledWith(mockAccessToken, groupServer.server_id); + }); + + it("shows every tool selected 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(); + renderWithProviders( + , + ); + + await screen.findByText("Group Server"); + await userEvent.click(screen.getByText("Flat List")); + + const [listIssues, deleteIssue] = screen.getAllByRole("checkbox"); + expect(listIssues).toBeChecked(); + expect(deleteIssue).toBeChecked(); + + await userEvent.click(listIssues); + expect(mockOnChange).toHaveBeenCalledWith({ [groupServer.server_id]: ["delete_issue"] }); + }); + + it("marks an access-group server as inherited and leaves a directly selected one unmarked", async () => { + const directServer = { server_id: "srv-direct-1", server_name: "Direct Server", alias: "Direct Server" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue([directServer, groupServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + renderWithProviders( + , + ); + + expect(await screen.findByText("Direct Server")).toBeInTheDocument(); + expect(await screen.findByText("Group Server")).toBeInTheDocument(); + expect(screen.getByText("Via access group: production-group")).toBeInTheDocument(); + expect(screen.queryAllByText(/^Via /)).toHaveLength(1); + }); + + it("renders a toolset server as inherited from that toolset", async () => { + const toolsetServer = { server_id: "srv-toolset-1", server_name: "Toolset Server", alias: "Toolset Server" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue([toolsetServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([ + { + toolset_id: "ts-1", + toolset_name: "Support Toolset", + tools: [{ server_id: toolsetServer.server_id, tool_name: "list_issues" }], + }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + renderWithProviders( + , + ); + + expect(await screen.findByText("Toolset Server")).toBeInTheDocument(); + expect(screen.getByText("Via toolset: Support Toolset")).toBeInTheDocument(); + }); + + // The backend adds a toolset's tools to whatever mcp_tool_permissions holds, so showing the + // server as unrestricted would invite a deselection that grants every other tool on it. + it("shows a toolset's own tools as the allowed set and locks them", async () => { + const toolsetServer = { server_id: "srv-toolset-1", server_name: "Toolset Server", alias: "Toolset Server" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue([toolsetServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([ + { + toolset_id: "ts-1", + toolset_name: "Support Toolset", + tools: [{ server_id: toolsetServer.server_id, tool_name: "list_issues" }], + }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + expect( + screen.getByText( + "list_issues is granted by a selected toolset, so it stays allowed here; edit the toolset to revoke it", + ), + ).toBeInTheDocument(); + + await userEvent.click(screen.getByText("Flat List")); + const [listIssues, deleteIssue] = screen.getAllByRole("checkbox"); + expect(listIssues).toBeChecked(); + expect(listIssues).toBeDisabled(); + expect(deleteIssue).not.toBeChecked(); + + await userEvent.click(listIssues); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it("ignores a click on a locked tool in the risk-group view", async () => { + const toolsetServer = { server_id: "srv-toolset-1", server_name: "Toolset Server", alias: "Toolset Server" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue([toolsetServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([ + { + toolset_id: "ts-1", + toolset_name: "Support Toolset", + tools: [{ server_id: toolsetServer.server_id, tool_name: "list_issues" }], + }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + await userEvent.click(await screen.findByText("list_issues")); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + // Turning a risk group off must not drop a tool the entry grants in its own right, which the + // toolset happens to grant too: that tool outlives the toolset and the admin did not clear it. + it("keeps a locked tool the entry also grants when its risk group is turned off", async () => { + const toolsetServer = { server_id: "srv-toolset-1", server_name: "Toolset Server", alias: "Toolset Server" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue([toolsetServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([ + { + toolset_id: "ts-1", + toolset_name: "Support Toolset", + tools: [{ server_id: toolsetServer.server_id, tool_name: "list_issues" }], + }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + // First checkbox is the header toggle of the group holding list_issues. + await userEvent.click(screen.getAllByRole("checkbox")[0]); + + expect(mockOnChange).toHaveBeenCalledWith({ [toolsetServer.server_id]: ["list_issues"] }); + }); + + // Copying the toolset's tools into the entry would outlive the toolset, so a write keeps only + // what this level grants on its own. + it("leaves a toolset's tools out of the entry a Select All writes", async () => { + const toolsetServer = { server_id: "srv-toolset-1", server_name: "Toolset Server", alias: "Toolset Server" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue([toolsetServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([ + { + toolset_id: "ts-1", + toolset_name: "Support Toolset", + tools: [{ server_id: toolsetServer.server_id, tool_name: "list_issues" }], + }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + await userEvent.click(screen.getByText("Select All")); + + expect(mockOnChange).toHaveBeenCalledWith({ [toolsetServer.server_id]: ["delete_issue"] }); + }); + + // The default narrows an unrestricted server; against a toolset-restricted one it would widen + // the grant to every non-delete tool the server exposes. + it("does not write the delete-blocked default for a directly selected server a toolset restricts", 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([ + { + toolset_id: "ts-1", + toolset_name: "Support Toolset", + tools: [{ server_id: directServer.server_id, tool_name: "list_issues" }], + }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it("waits for toolsets before writing the delete-blocked default", async () => { + const directServer = { server_id: "srv-direct-1", server_name: "Direct Server", alias: "Direct Server" }; + let resolveToolsets: (toolsets: MCPToolset[]) => void = () => {}; + const pendingToolsets = new Promise((resolve) => { + resolveToolsets = resolve; + }); + vi.mocked(networking.fetchMCPServers).mockResolvedValue([directServer]); + vi.mocked(networking.fetchMCPToolsets).mockReturnValue(pendingToolsets); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + await screen.findByText("Direct Server"); + expect(mockOnChange).not.toHaveBeenCalled(); + + resolveToolsets([ + { + toolset_id: "ts-1", + toolset_name: "Support Toolset", + tools: [{ server_id: directServer.server_id, tool_name: "list_issues" }], + }, + ]); + + await screen.findByText("list_issues"); + expect(screen.getByRole("checkbox", { name: "list_issues" })).toHaveAttribute("aria-disabled", "true"); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + // The backend resolves a selection that is a registry id to that server alone. Rendering the + // server merely named after it would fire the default write against a server nobody granted, + // and a tool-permission entry is itself a grant. + it.each([ + { label: "id owner first", idOwnerFirst: true }, + { label: "name twin first", idOwnerFirst: false }, + ])("does not offer a server merely named after a selected id ($label)", async ({ idOwnerFirst }) => { + const idOwner = { server_id: "srv-collide", server_name: "Payments", alias: "Payments" }; + const nameTwin = { server_id: "srv-twin", server_name: "srv-collide", alias: "srv-collide" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue(idOwnerFirst ? [idOwner, nameTwin] : [nameTwin, idOwner]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + 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(networking.listMCPTools).not.toHaveBeenCalledWith(mockAccessToken, "srv-twin"); + }); + + it("does not write a default allowlist for an inherited 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(); + renderWithProviders( + , + ); + + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it("keeps blocking delete tools by default for a directly selected server", 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([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + await waitFor(() => { + expect(mockOnChange).toHaveBeenCalledWith({ [directServer.server_id]: ["list_issues"] }); + }); + }); + + it("shows a server that only a stale tool-permission entry still entitles", async () => { + vi.mocked(networking.fetchMCPServers).mockResolvedValue([groupServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + renderWithProviders( + , + ); + + expect(await screen.findByText("Group Server")).toBeInTheDocument(); + expect(screen.getByText("Via tool permissions")).toBeInTheDocument(); + }); + + it("shows nothing for a principal blocked from every MCP server", async () => { + vi.mocked(networking.fetchMCPServers).mockResolvedValue([groupServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false }); + + const { container } = renderWithProviders( + , + ); + + expect(container).toBeEmptyDOMElement(); + expect(networking.listMCPTools).not.toHaveBeenCalled(); + }); + + it("warns instead of showing no inherited servers when the server list cannot be loaded", async () => { + vi.mocked(networking.fetchMCPServers).mockRejectedValue(new Error("boom")); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + + renderWithProviders( + , + ); + + expect(await screen.findByText("Unable to load MCP servers")).toBeInTheDocument(); + }); + + it("warns when the selected toolsets cannot be resolved to servers", async () => { + vi.mocked(networking.fetchMCPServers).mockResolvedValue([]); + vi.mocked(networking.fetchMCPToolsets).mockRejectedValue(new Error("boom")); + + renderWithProviders( + , + ); + + expect(await screen.findByText("Unable to load toolsets")).toBeInTheDocument(); + }); + }); + + describe("grants keyed by server name", () => { + const namedServer = { + server_id: "1f4bd6c1-0000-4000-8000-000000000001", + server_name: "github_mcp", + alias: "GitHub", + }; + const namedTools = [ + { name: "list_issues", description: "List issues" }, + { name: "delete_issue", description: "Delete an issue" }, + ]; + + it("renders the tool matrix for a grant that names the server instead of its id", async () => { + vi.mocked(networking.fetchMCPServers).mockResolvedValue([namedServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: namedTools, error: false }); + + renderWithProviders( + , + ); + + expect(await screen.findByText("github_mcp")).toBeInTheDocument(); + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + expect(screen.getByText("delete_issue")).toBeInTheDocument(); + expect(networking.listMCPTools).toHaveBeenCalledWith(mockAccessToken, namedServer.server_id); + }); + + it("writes an edit back to the name key instead of adding a second id-keyed entry", async () => { + vi.mocked(networking.fetchMCPServers).mockResolvedValue([namedServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: namedTools, error: false }); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + await userEvent.click(screen.getByRole("button", { name: "Deselect All" })); + + expect(mockOnChange).toHaveBeenCalledWith({ github_mcp: [] }); + }); + }); + + describe("a server named by several equivalent keys", () => { + const namedServer = { + server_id: "1f4bd6c1-0000-4000-8000-000000000001", + server_name: "github_mcp", + alias: "GitHub", + mcp_access_groups: ["production-group"], + }; + const namedTools = [ + { name: "list_issues", description: "List issues" }, + { name: "create_issue", description: "Open an issue" }, + { name: "delete_issue", description: "Delete an issue" }, + ]; + + const renderWithBothKeys = (onChange: () => void) => + renderWithProviders( + , + ); + + beforeEach(() => { + vi.mocked(networking.fetchMCPServers).mockResolvedValue([namedServer]); + vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: namedTools, error: false }); + }); + + it("renders one card showing the union both keys grant", async () => { + renderWithBothKeys(vi.fn()); + + expect(await screen.findByText("github_mcp")).toBeInTheDocument(); + expect(screen.getAllByText("github_mcp")).toHaveLength(1); + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + + // Flat view keeps checkbox order identical to the fetched tool order. + await userEvent.click(screen.getByText("Flat List")); + const [listIssues, createIssue, deleteIssue] = screen.getAllByRole("checkbox"); + expect(listIssues).toBeChecked(); + expect(createIssue).toBeChecked(); + expect(deleteIssue).not.toBeChecked(); + }); + + it("removes a deselected tool from every equivalent key, leaving one entry for the server", async () => { + const mockOnChange = vi.fn(); + renderWithBothKeys(mockOnChange); + + expect(await screen.findByText("list_issues")).toBeInTheDocument(); + await userEvent.click(screen.getByText("Flat List")); + await userEvent.click(screen.getAllByRole("checkbox")[0]); + + const written = mockOnChange.mock.calls.at(-1)?.[0] as Record; + expect(Object.keys(written)).toEqual([namedServer.server_id]); + expect(written[namedServer.server_id]).not.toContain("list_issues"); + expect(written[namedServer.server_id]).toContain("create_issue"); + }); + + // Both catalog orders, because a name resolves to two servers here and a first-match + // implementation is only wrong in one of them. + it.each([ + { label: "edited server first", editedFirst: true }, + { label: "twin first", editedFirst: false }, + ])( + "says on the card when a key names another server too, since its tools cannot be revoked here ($label)", + async ({ editedFirst }) => { + const twin = { server_id: "1f4bd6c1-0000-4000-8000-000000000002", server_name: "github_mcp", alias: "Twin" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue( + editedFirst ? [namedServer, twin] : [twin, namedServer], + ); + + renderWithProviders( + , + ); + + // Both cards say it: the shared key grants on either server and neither card can revoke it, + // so an admin looking at either one has to be told the same thing. + expect( + await screen.findAllByText( + 'Also granted by "github_mcp", which names another server too. Those tools stay allowed here until the servers no longer share that name', + ), + ).toHaveLength(2); + }, + ); + + // The shared key is the twin's only entry, so it would otherwise be the key an edit writes, + // and writing it would move the allowlist of the server the admin is not looking at. + it.each([ + { label: "edited server first", editedFirst: true }, + { label: "twin first", editedFirst: false }, + ])("edits the twin through its own id rather than the shared key ($label)", async ({ editedFirst }) => { + const twin = { server_id: "1f4bd6c1-0000-4000-8000-000000000002", server_name: "github_mcp", alias: "Twin" }; + vi.mocked(networking.fetchMCPServers).mockResolvedValue(editedFirst ? [namedServer, twin] : [twin, namedServer]); + + const mockOnChange = vi.fn(); + renderWithProviders( + , + ); + + // The directly selected twin is the first card; both share the display name "github_mcp". + expect(await screen.findAllByText("list_issues")).toHaveLength(2); + await userEvent.click(screen.getAllByText("Select All")[0]); + + const written = mockOnChange.mock.calls.at(-1)?.[0] as Record; + expect(written["github_mcp"]).toEqual(["list_issues"]); + expect(written[twin.server_id]).toEqual(["list_issues", "create_issue", "delete_issue"]); + }); + + it("says nothing about shared names when every key names one server", async () => { + renderWithBothKeys(vi.fn()); + + expect(await screen.findByText("github_mcp")).toBeInTheDocument(); + expect(screen.queryByText(/names another server too/)).not.toBeInTheDocument(); + }); + + it("badges the server once, by its strongest grant, when a key and a group both name it", async () => { + renderWithProviders( + , + ); + + expect(await screen.findByText("github_mcp")).toBeInTheDocument(); + expect(screen.getByText("Via access group: production-group")).toBeInTheDocument(); + expect(screen.queryByText("Via tool permissions")).not.toBeInTheDocument(); + expect(screen.queryAllByText(/^Via /)).toHaveLength(1); + }); + }); + describe("risk-group (CRUD) view", () => { const crudTools = [ { name: "list_documents", description: "List every document" }, 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 9d7c8cd452b..c866e9cc011 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx +++ b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx @@ -1,28 +1,62 @@ import React, { useEffect, useRef, useState, useMemo } from "react"; import { listMCPTools } from "../networking"; -import { MCPTool, MCPServer } from "../mcp_tools/types"; +import { MCPTool } from "../mcp_tools/types"; import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group"; import { UiLoadingSpinner } from "@/components/ui/ui-loading-spinner"; import { useMCPServers } from "../../app/(dashboard)/hooks/mcpServers/useMCPServers"; +import { useMCPToolsets } from "../../app/(dashboard)/hooks/mcpServers/useMCPToolsets"; import McpCrudPermissionPanel from "../mcp_tools/McpCrudPermissionPanel"; import { classifyToolOp } from "../../utils/mcpToolCrudClassification"; +import { NO_MCP_SERVERS_SENTINEL } from "../mcp_tools/constants"; +import { + EffectiveMcpServer, + McpGrantSource, + applyToolPermissionWrite, + mcpAllowedToolsFor, + resolveEffectiveMcpServers, +} from "./effectiveMcpServers"; interface MCPToolPermissionsProps { accessToken: string; - selectedServers: string[]; + selectedServers: readonly string[]; + selectedAccessGroups?: readonly string[]; + selectedToolsets?: readonly string[]; toolPermissions: Record; onChange: (toolPermissions: Record) => void; disabled?: boolean; } +const NO_SELECTION: readonly string[] = []; + +interface InheritedBadge { + readonly label: string; + readonly className: string; +} + +const inheritedBadgeFor = (source: McpGrantSource): InheritedBadge | null => { + switch (source.kind) { + case "direct": + return null; + case "accessGroup": + return { label: `Via access group: ${source.name}`, className: "text-green-700 bg-green-50 border-green-200" }; + case "toolset": + return { label: `Via toolset: ${source.name}`, className: "text-purple-700 bg-purple-50 border-purple-200" }; + case "toolPermission": + return { label: "Via tool permissions", className: "text-amber-700 bg-amber-50 border-amber-200" }; + } +}; + const MCPToolPermissions: React.FC = ({ accessToken, selectedServers, + selectedAccessGroups = NO_SELECTION, + selectedToolsets = NO_SELECTION, toolPermissions, onChange, disabled = false, }) => { - const { data: allServers = [] } = useMCPServers(); + const { data: allServers = [], isError: serversFailed, isLoading: serversLoading } = useMCPServers(); + const { data: toolsets = [], isError: toolsetsFailed, isLoading: toolsetsLoading } = useMCPToolsets(); const [serverTools, setServerTools] = useState>({}); const [loadingTools, setLoadingTools] = useState>({}); const [toolErrors, setToolErrors] = useState>({}); @@ -36,15 +70,25 @@ const MCPToolPermissions: React.FC = ({ toolPermissionsRef.current = toolPermissions; }, [toolPermissions]); - // Filter servers based on selectedServers - const servers = useMemo(() => { - if (selectedServers.length === 0) return []; - return allServers.filter((server: MCPServer) => selectedServers.includes(server.server_id)); - }, [allServers, selectedServers]); + // 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 = { + allServers, + selectedServers, + selectedAccessGroups, + selectedToolsets, + toolsets, + toolPermissions, + }; + const servers = useMemo( + () => resolveEffectiveMcpServers(effectiveMcpInput), + [allServers, selectedServers, selectedAccessGroups, selectedToolsets, toolsets, toolPermissions], + ); // Fetch tools for a specific server; applies delete-blocked-by-default for new servers. // `token` is passed explicitly so the closure never captures a stale accessToken. - const fetchToolsForServer = async (serverId: string, token: string) => { + const fetchToolsForServer = async (entry: EffectiveMcpServer, token: string) => { + const serverId = entry.server.server_id; setLoadingTools((prev) => ({ ...prev, [serverId]: true })); setToolErrors((prev) => ({ ...prev, [serverId]: "" })); @@ -58,14 +102,18 @@ const MCPToolPermissions: React.FC = ({ const fetchedTools: MCPTool[] = response.tools || []; setServerTools((prev) => ({ ...prev, [serverId]: fetchedTools })); - // For servers that have no permissions stored yet, block delete tools by default. + // Default only unrestricted direct servers to non-delete tools. // Read latest permissions from the ref to avoid clobbering concurrent results. const latestPermissions = toolPermissionsRef.current; - if (!latestPermissions[serverId] && fetchedTools.length > 0) { + 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({ ...latestPermissions, [serverId]: nonDeleteTools }); + onChange(applyToolPermissionWrite({ toolPermissions: latestPermissions, entry, allowed: nonDeleteTools })); } } } catch (err) { @@ -79,58 +127,124 @@ const MCPToolPermissions: React.FC = ({ // Auto-fetch tools when servers or accessToken change useEffect(() => { - servers.forEach((server) => { - if (!serverTools[server.server_id] && !loadingTools[server.server_id]) { - fetchToolsForServer(server.server_id, accessToken); + if (toolsetsLoading) return; + servers.forEach((entry) => { + const serverId = entry.server.server_id; + if (!serverTools[serverId] && !loadingTools[serverId]) { + fetchToolsForServer(entry, accessToken); } }); // fetchToolsForServer is defined in this render scope but receives `accessToken` // as an explicit argument, so it is safe to omit from deps here. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [servers, accessToken]); + }, [servers, accessToken, toolsetsLoading]); - const handleCrudPanelChange = (serverId: string, allowed: string[]) => { - onChange({ ...toolPermissions, [serverId]: allowed }); + // Every 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 = (serverId: string) => { - const tools = serverTools[serverId] || []; - onChange({ ...toolPermissions, [serverId]: tools.map((t) => t.name) }); + const handleSelectAll = (entry: EffectiveMcpServer) => { + const tools = serverTools[entry.server.server_id] || []; + writeAllowedTools( + entry, + tools.map((t) => t.name), + ); }; - const handleDeselectAll = (serverId: string) => { - onChange({ ...toolPermissions, [serverId]: [] }); - }; + // 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)) { + return null; + } - if (selectedServers.length === 0) { + const selectionSizes = [ + selectedServers.length, + selectedAccessGroups.length, + selectedToolsets.length, + Object.keys(toolPermissions).length, + ]; + if (!selectionSizes.some((size) => size > 0)) { return null; } return (
- {servers.map((server) => { - const serverName = server.server_name || server.alias || server.server_id; - const tools = serverTools[server.server_id] || []; - const selectedTools = toolPermissions[server.server_id] || []; - const isLoading = loadingTools[server.server_id]; - const error = toolErrors[server.server_id]; - const viewMode = viewModes[server.server_id] ?? "crud"; + {serversFailed && ( +
+

Unable to load MCP servers

+

+ This list is incomplete; servers granted directly or through an access group may be missing. Reload before + changing tool permissions +

+
+ )} + + {toolsetsFailed && selectedToolsets.length > 0 && ( +
+

Unable to load toolsets

+

+ Servers reached through the selected toolsets are not listed below +

+
+ )} + + {serversLoading && ( +
+ +

Loading MCP servers...

+
+ )} + + {servers.map((entry) => { + const server = entry.server; + 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 isLoading = loadingTools[serverId]; + const error = toolErrors[serverId]; + const viewMode = viewModes[serverId] ?? "crud"; + const inherited = inheritedBadgeFor(entry.source); + // The backend adds a toolset's tools to whatever this map allows, so these stay on however + // the boxes are ticked. Locking them is what keeps the matrix an honest picture of the grant. + const toolsetTools = entry.toolsetTools ?? []; return ( -
+
{/* Header */}
-

{serverName}

+
+

{serverName}

+ {inherited && ( + + {inherited.label} + + )} +
{server.description &&

{server.description}

} + {entry.ambiguousKeys.length > 0 && ( +

+ {`Also granted by ${entry.ambiguousKeys.map((key) => `"${key}"`).join(", ")}, which names another server too. Those tools stay allowed here until the servers no longer share that name`} +

+ )} + {toolsetTools.length > 0 && ( +

+ {toolsetTools.length === 1 + ? `${toolsetTools[0]} is granted by a selected toolset, so it stays allowed here; edit the toolset to revoke it` + : `${toolsetTools.join(", ")} are granted by a selected toolset, so they stay allowed here; edit the toolset to revoke them`} +

+ )}
{!disabled && tools.length > 0 && ( - setViewModes((prev) => ({ ...prev, [server.server_id]: next as "crud" | "flat" })) - } + onValueChange={(next) => setViewModes((prev) => ({ ...prev, [serverId]: next as "crud" | "flat" }))} className="flex w-auto items-center gap-4" >