mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-06 02:48:13 +00:00
fix(ui): show inherited MCP servers on the internal user editor and flag access groups with no members (#40036)
* fix(ui): show inherited MCP servers on the internal-user editor and flag access groups with no members Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(ui): consult the unfiltered access group registry before calling a group empty Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: yassin <yassin@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
192e38fa7b
commit
e11a8c59ff
6 changed files with 146 additions and 2 deletions
|
|
@ -1,8 +1,11 @@
|
|||
import { cleanup, fireEvent, screen, waitFor } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { renderWithProviders } from "../../../../../tests/test-utils";
|
||||
import { renderWithProviders, testQueryClient } from "../../../../../tests/test-utils";
|
||||
import { UserEditView } from "./user_edit_view";
|
||||
import * as networking from "@/components/networking";
|
||||
|
||||
vi.mock("@/components/networking");
|
||||
|
||||
vi.mock("@/components/key_team_helpers/fetch_available_models_team_key", () => ({
|
||||
getModelDisplayName: vi.fn((model: string) => model),
|
||||
|
|
@ -59,6 +62,10 @@ describe("UserEditView", () => {
|
|||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
testQueryClient.clear();
|
||||
vi.mocked(networking.fetchMCPServers).mockResolvedValue([]);
|
||||
vi.mocked(networking.fetchMCPAccessGroups).mockResolvedValue([]);
|
||||
vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
|
|
@ -579,6 +586,44 @@ describe("UserEditView", () => {
|
|||
expect(budgetInput.closest("form")).not.toHaveAttribute("novalidate");
|
||||
});
|
||||
|
||||
it("shows the tool matrix for servers the user reaches only through an access group or toolset", async () => {
|
||||
vi.mocked(networking.fetchMCPServers).mockResolvedValue([
|
||||
{ server_id: "srv-group", server_name: "Group Server", alias: "Group Server", mcp_access_groups: ["group-a"] },
|
||||
{ server_id: "srv-toolset", server_name: "Toolset Server", alias: "Toolset Server" },
|
||||
]);
|
||||
vi.mocked(networking.fetchMCPAccessGroups).mockResolvedValue(["group-a"]);
|
||||
vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([
|
||||
{
|
||||
toolset_id: "toolset-a",
|
||||
toolset_name: "Toolset A",
|
||||
tools: [{ server_id: "srv-toolset", tool_name: "list_issues" }],
|
||||
} as never,
|
||||
]);
|
||||
vi.mocked(networking.listMCPTools).mockResolvedValue({
|
||||
tools: [{ name: "list_issues", description: "List issues" }],
|
||||
error: false,
|
||||
});
|
||||
|
||||
renderWithProviders(
|
||||
<UserEditView
|
||||
{...defaultProps}
|
||||
objectPermission={
|
||||
{
|
||||
mcp_servers: [],
|
||||
mcp_access_groups: ["group-a"],
|
||||
mcp_toolsets: ["toolset-a"],
|
||||
mcp_tool_permissions: {},
|
||||
} as never
|
||||
}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(await screen.findByText("Via access group: group-a")).toBeInTheDocument();
|
||||
expect(await screen.findByText("Via toolset: Toolset A")).toBeInTheDocument();
|
||||
expect(networking.listMCPTools).toHaveBeenCalledWith("test-token", "srv-group");
|
||||
expect(networking.listMCPTools).toHaveBeenCalledWith("test-token", "srv-toolset");
|
||||
});
|
||||
|
||||
it("should send objects for the mcp keys seeded from objectPermission", async () => {
|
||||
const payload = await submittedPayload({
|
||||
objectPermission: {
|
||||
|
|
|
|||
|
|
@ -336,6 +336,8 @@ export function UserEditView({
|
|||
<MCPToolPermissions
|
||||
accessToken={accessToken || ""}
|
||||
selectedServers={form.watch("mcp_servers_and_groups")?.servers || []}
|
||||
selectedAccessGroups={form.watch("mcp_servers_and_groups")?.accessGroups || []}
|
||||
selectedToolsets={form.watch("mcp_servers_and_groups")?.toolsets || []}
|
||||
toolPermissions={form.watch("mcp_tool_permissions") || {}}
|
||||
onChange={(toolPerms) => form.setValue("mcp_tool_permissions", toolPerms)}
|
||||
/>
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ describe("MCPToolPermissions", () => {
|
|||
vi.clearAllMocks();
|
||||
testQueryClient.clear();
|
||||
vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]);
|
||||
vi.mocked(networking.fetchMCPAccessGroups).mockResolvedValue([]);
|
||||
});
|
||||
|
||||
it("should update tool permissions when user selects a tool", async () => {
|
||||
|
|
@ -621,6 +622,45 @@ describe("MCPToolPermissions", () => {
|
|||
);
|
||||
|
||||
expect(await screen.findByText("Unable to load MCP servers")).toBeInTheDocument();
|
||||
expect(screen.queryByText(/has 0 servers/)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("tells the admin when a loaded access group has no member servers", async () => {
|
||||
vi.mocked(networking.fetchMCPServers).mockResolvedValue([groupServer]);
|
||||
vi.mocked(networking.fetchMCPToolsets).mockResolvedValue([]);
|
||||
vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: groupTools, error: false });
|
||||
|
||||
renderWithProviders(
|
||||
<MCPToolPermissions
|
||||
accessToken={mockAccessToken}
|
||||
selectedServers={[]}
|
||||
selectedAccessGroups={["production-group", "ops_readonly"]}
|
||||
toolPermissions={{}}
|
||||
onChange={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(await screen.findByText('Access group "ops_readonly" has 0 servers')).toBeInTheDocument();
|
||||
expect(screen.getByText("Group Server")).toBeInTheDocument();
|
||||
expect(screen.queryByText('Access group "production-group" has 0 servers')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("does not call a group empty when its servers are only hidden from the caller's catalog", async () => {
|
||||
vi.mocked(networking.fetchMCPServers).mockResolvedValue([]);
|
||||
vi.mocked(networking.fetchMCPAccessGroups).mockResolvedValue(["production-group"]);
|
||||
|
||||
renderWithProviders(
|
||||
<MCPToolPermissions
|
||||
accessToken={mockAccessToken}
|
||||
selectedServers={[]}
|
||||
selectedAccessGroups={["production-group", "ops_readonly"]}
|
||||
toolPermissions={{}}
|
||||
onChange={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(await screen.findByText('Access group "ops_readonly" has 0 servers')).toBeInTheDocument();
|
||||
expect(screen.queryByText('Access group "production-group" has 0 servers')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("warns when the selected toolsets cannot be resolved to servers", async () => {
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ 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 { useMCPAccessGroups } from "../../app/(dashboard)/hooks/mcpServers/useMCPAccessGroups";
|
||||
import { useMCPToolsets } from "../../app/(dashboard)/hooks/mcpServers/useMCPToolsets";
|
||||
import McpCrudPermissionPanel from "../mcp_tools/McpCrudPermissionPanel";
|
||||
import { classifyToolOp } from "../../utils/mcpToolCrudClassification";
|
||||
|
|
@ -12,6 +13,7 @@ import {
|
|||
EffectiveMcpServer,
|
||||
McpGrantSource,
|
||||
applyToolPermissionWrite,
|
||||
emptyMcpAccessGroups,
|
||||
mcpAllowedToolsFor,
|
||||
resolveEffectiveMcpServers,
|
||||
} from "./effectiveMcpServers";
|
||||
|
|
@ -55,7 +57,13 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
|
|||
onChange,
|
||||
disabled = false,
|
||||
}) => {
|
||||
const { data: allServers = [], isError: serversFailed, isLoading: serversLoading } = useMCPServers();
|
||||
const {
|
||||
data: allServers = [],
|
||||
isError: serversFailed,
|
||||
isLoading: serversLoading,
|
||||
isSuccess: serversLoaded,
|
||||
} = useMCPServers();
|
||||
const { data: populatedAccessGroups = [], isSuccess: accessGroupsLoaded } = useMCPAccessGroups();
|
||||
const { data: toolsets = [], isError: toolsetsFailed, isLoading: toolsetsLoading } = useMCPToolsets();
|
||||
const [serverTools, setServerTools] = useState<Record<string, MCPTool[]>>({});
|
||||
const [loadingTools, setLoadingTools] = useState<Record<string, boolean>>({});
|
||||
|
|
@ -181,6 +189,18 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
|
|||
</div>
|
||||
)}
|
||||
|
||||
{serversLoaded &&
|
||||
accessGroupsLoaded &&
|
||||
emptyMcpAccessGroups(allServers, populatedAccessGroups, selectedAccessGroups).map((group) => (
|
||||
<div key={group} className="p-4 bg-yellow-50 border border-yellow-200 rounded-lg">
|
||||
<p className="text-sm text-yellow-800 font-medium">Access group "{group}" has 0 servers</p>
|
||||
<p className="text-sm text-yellow-700 mt-1">
|
||||
No MCP server lists this group, so it grants nothing. A server defined in config.yaml joins a group
|
||||
through its <code>access_groups</code> key; <code>mcp_access_groups</code> is ignored there
|
||||
</p>
|
||||
</div>
|
||||
))}
|
||||
|
||||
{toolsetsFailed && selectedToolsets.length > 0 && (
|
||||
<div className="p-4 bg-yellow-50 border border-yellow-200 rounded-lg">
|
||||
<p className="text-sm text-yellow-800 font-medium">Unable to load toolsets</p>
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest";
|
|||
import { MCPServer, MCPToolset } from "../mcp_tools/types";
|
||||
import {
|
||||
applyToolPermissionWrite,
|
||||
emptyMcpAccessGroups,
|
||||
mcpAllowedToolsFor,
|
||||
mcpServersForIdentifier,
|
||||
mcpToolPermissionKeyFor,
|
||||
|
|
@ -101,6 +102,29 @@ describe("mcpToolPermissionKeyFor", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("emptyMcpAccessGroups", () => {
|
||||
const grouped = server({ server_id: "srv-group", server_name: "Grouped", mcp_access_groups: ["prod"] });
|
||||
const objectGrouped = {
|
||||
...grouped,
|
||||
server_id: "srv-obj",
|
||||
mcp_access_groups: [{ name: "legacy" }],
|
||||
} as unknown as MCPServer;
|
||||
|
||||
it("names only the selected groups no loaded server belongs to", () => {
|
||||
expect(emptyMcpAccessGroups([grouped, objectGrouped], [], ["prod", "legacy", "ops_readonly"])).toEqual([
|
||||
"ops_readonly",
|
||||
]);
|
||||
});
|
||||
|
||||
it("names every selected group when no server is loaded and the registry is empty", () => {
|
||||
expect(emptyMcpAccessGroups([], [], ["prod"])).toEqual(["prod"]);
|
||||
});
|
||||
|
||||
it("trusts the group registry when the caller's catalog hides the member servers", () => {
|
||||
expect(emptyMcpAccessGroups([], ["prod"], ["prod", "ops_readonly"])).toEqual(["ops_readonly"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("resolveEffectiveMcpServers", () => {
|
||||
const direct = server({ server_id: "srv-direct", server_name: "Direct" });
|
||||
const grouped = server({ server_id: "srv-group", server_name: "Grouped", mcp_access_groups: ["prod"] });
|
||||
|
|
|
|||
|
|
@ -54,6 +54,19 @@ const accessGroupNamesOf = (server: MCPServer): readonly string[] =>
|
|||
return [typeof parsed.data === "string" ? parsed.data : parsed.data.name];
|
||||
});
|
||||
|
||||
// The server catalog can be trimmed to the caller's grants, so the unfiltered group registry
|
||||
// (GET /v1/mcp/access_groups) has to agree before a group is called empty.
|
||||
export const emptyMcpAccessGroups = (
|
||||
allServers: readonly MCPServer[],
|
||||
populatedAccessGroups: readonly string[],
|
||||
selectedAccessGroups: readonly string[],
|
||||
): readonly string[] =>
|
||||
selectedAccessGroups.filter(
|
||||
(group) =>
|
||||
!populatedAccessGroups.includes(group) &&
|
||||
!allServers.some((server) => accessGroupNamesOf(server).includes(group)),
|
||||
);
|
||||
|
||||
// Which servers an identifier names, with the same precedence the backend's expand_permission_list
|
||||
// applies: a string that is a registry server id names exactly that server, and only a string that
|
||||
// is not falls back to server_name/alias, which can name several. Matching all three fields at once
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue