mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-24 00:52:24 +00:00
fix(mcp): keep config-defined servers read-only (#42299)
* fix(mcp): persist config server edits in the database * test(mcp): cover config loading and failed promotion responses * test(mcp): preserve auth policy during config server promotion * fix(mcp): preserve existing YAML metadata during config loading * fix(mcp): reuse bounded traversal for config secret checks * fix(mcp): honor database access groups after config promotion * fix(mcp): keep config-defined servers read-only * fix(ui): clear frontend warnings and require warning-free green checks * fix(ui): preserve legacy MCP access group labels * ci: remove remaining frontend action runtime warnings * fix(mcp): limit read-only fix to ticket scope * fix(mcp): preserve API stability and remove unrelated guidance --------- Co-authored-by: Joshua Valluru <326636767+joshua-berri@users.noreply.github.com>
This commit is contained in:
parent
5a8c4f48e4
commit
25af172b85
9 changed files with 120 additions and 4 deletions
|
|
@ -50,6 +50,7 @@ class LiteLLM_MCPServerTable(LiteLLMPydanticObjectBase):
|
|||
"""Represents a LiteLLM_MCPServerTable record"""
|
||||
|
||||
server_id: str
|
||||
is_config: bool = Field(default=False, description="Whether this server is defined in config and is read-only.")
|
||||
server_name: str | None = None
|
||||
alias: str | None = None
|
||||
description: str | None = None
|
||||
|
|
|
|||
|
|
@ -7081,6 +7081,7 @@ class MCPServerManager:
|
|||
def _build_mcp_server_table(self, server: MCPServer) -> LiteLLM_MCPServerTable:
|
||||
return LiteLLM_MCPServerTable(
|
||||
server_id=server.server_id,
|
||||
is_config=self.is_config_declared_server(server.server_id) and server.server_id not in self.registry,
|
||||
server_name=server.server_name,
|
||||
alias=server.alias,
|
||||
description=(server.mcp_info.get("description") if server.mcp_info else None),
|
||||
|
|
|
|||
|
|
@ -24767,6 +24767,12 @@
|
|||
"title": "Is Byok",
|
||||
"type": "boolean"
|
||||
},
|
||||
"is_config": {
|
||||
"default": false,
|
||||
"description": "Whether this server is defined in config and is read-only.",
|
||||
"title": "Is Config",
|
||||
"type": "boolean"
|
||||
},
|
||||
"issuer": {
|
||||
"anyOf": [
|
||||
{
|
||||
|
|
@ -27817,6 +27823,12 @@
|
|||
"title": "Is Byok",
|
||||
"type": "boolean"
|
||||
},
|
||||
"is_config": {
|
||||
"default": false,
|
||||
"description": "Whether this server is defined in config and is read-only.",
|
||||
"title": "Is Config",
|
||||
"type": "boolean"
|
||||
},
|
||||
"issuer": {
|
||||
"anyOf": [
|
||||
{
|
||||
|
|
|
|||
|
|
@ -14470,6 +14470,21 @@ async def test_request_selected_during_guardrail_runs_concurrently_with_tool(mon
|
|||
assert result.content[0].text == "executed"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("in_config,in_db,expected", [(True, False, True), (False, True, False), (True, True, False)])
|
||||
async def test_server_response_identifies_read_only_config(in_config, in_db, expected):
|
||||
manager = MCPServerManager()
|
||||
server = MCPServer(server_id="source-server", name="source_server", transport=MCPTransport.http)
|
||||
manager.config_mcp_servers = {server.server_id: server} if in_config else {}
|
||||
manager.registry = {server.server_id: server} if in_db else {}
|
||||
|
||||
listed = await manager.get_all_mcp_servers_unfiltered()
|
||||
|
||||
assert len(listed) == 1
|
||||
assert listed[0].model_dump().get("is_config") is expected
|
||||
assert manager._build_mcp_server_table(server).model_dump().get("is_config") is expected
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("with_caller,legacy_factory", [(True, False), (False, False), (True, True)])
|
||||
async def test_client_sampling_does_not_fill_explicit_context_from_another_ambient_caller(with_caller, legacy_factory):
|
||||
|
|
|
|||
|
|
@ -7900,3 +7900,43 @@ class TestGetMcpToolsWireShape:
|
|||
assert tool["outputSchema"] == {"type": "integer"}
|
||||
assert "_meta" in tool
|
||||
assert not {"input_schema", "output_schema", "meta"} & tool.keys()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("role,expected_status", [
|
||||
(LitellmUserRoles.PROXY_ADMIN, 404),
|
||||
(LitellmUserRoles.INTERNAL_USER, 403),
|
||||
])
|
||||
async def test_config_server_edit_preserves_api_contract_without_creating_rows(role, expected_status):
|
||||
from litellm.proxy._experimental.mcp_server.mcp_server_manager import MCPServerManager
|
||||
|
||||
manager = MCPServerManager()
|
||||
server = generate_mock_mcp_server_config_record(server_id="read-only-config")
|
||||
manager.config_mcp_servers = {server.server_id: server}
|
||||
original = server.model_dump()
|
||||
prisma = MagicMock()
|
||||
prisma.db.litellm_mcpservertable.find_unique = AsyncMock(return_value=None)
|
||||
prisma.db.litellm_mcpservertable.update = AsyncMock(return_value=None)
|
||||
with (
|
||||
patch.object(mgmt_endpoints, "global_mcp_server_manager", manager),
|
||||
patch.object(mgmt_endpoints, "get_prisma_client_or_throw", return_value=prisma),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await mgmt_endpoints.edit_mcp_server(
|
||||
payload=UpdateMCPServerRequest(server_id=server.server_id, description="UI edit"),
|
||||
user_api_key_dict=UserAPIKeyAuth(user_id="actor", user_role=role),
|
||||
)
|
||||
|
||||
assert exc.value.status_code == expected_status
|
||||
if role == LitellmUserRoles.PROXY_ADMIN:
|
||||
assert exc.value.detail == {
|
||||
"error": f"MCP Server not found, passed server_id={server.server_id}"
|
||||
}
|
||||
prisma.db.litellm_mcpservertable.update.assert_awaited_once()
|
||||
else:
|
||||
prisma.db.litellm_mcpservertable.update.assert_not_awaited()
|
||||
prisma.db.litellm_mcpservertable.create.assert_not_called()
|
||||
prisma.db.litellm_mcpservertable.create_many.assert_not_called()
|
||||
prisma.tx.assert_not_called()
|
||||
assert server.model_dump() == original
|
||||
assert manager.registry == {}
|
||||
|
|
|
|||
|
|
@ -4,6 +4,8 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
|
|||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { MCPServerView } from "./mcp_server_view";
|
||||
import * as networking from "@/components/networking";
|
||||
import { setSecureItem } from "@/utils/secureStorage";
|
||||
import { EDIT_OAUTH_UI_STATE_KEY } from "./mcp_server_edit";
|
||||
import type { MCPServer } from "@/components/mcp_tools/types";
|
||||
|
||||
vi.mock(".", () => ({
|
||||
|
|
@ -68,6 +70,7 @@ const openUserCredentials = async (props: Record<string, unknown>) => {
|
|||
describe("MCPServerView", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
sessionStorage.clear();
|
||||
});
|
||||
|
||||
// Name, alias and description each label the header and a Settings row, so
|
||||
|
|
@ -146,6 +149,37 @@ describe("MCPServerView", () => {
|
|||
expect(screen.queryByRole("button", { name: "Edit Settings" })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it.each([false, true])("keeps config settings read-only with isEditing=%s", async (isEditing) => {
|
||||
renderView({ is_config: true }, { isEditing });
|
||||
|
||||
await userEvent.click(screen.getByRole("tab", { name: "Settings" }));
|
||||
|
||||
expect(screen.getByRole("button", { name: "Edit Settings" })).toBeDisabled();
|
||||
expect(screen.getByText("Defined in config. Edit your YAML configuration to make changes")).toBeVisible();
|
||||
expect(screen.queryByText("edit form")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it.each([true, false])("honors config read-only state on OAuth return: %s", async (isConfig) => {
|
||||
setSecureItem(EDIT_OAUTH_UI_STATE_KEY, JSON.stringify({ serverId: "srv-1" }));
|
||||
renderView({ is_config: isConfig });
|
||||
|
||||
await userEvent.click(screen.getByRole("tab", { name: "Settings" }));
|
||||
|
||||
if (isConfig) {
|
||||
expect(screen.queryByText("edit form")).not.toBeInTheDocument();
|
||||
expect(screen.getByRole("button", { name: "Edit Settings" })).toBeDisabled();
|
||||
} else {
|
||||
expect(screen.getByText("edit form")).toBeVisible();
|
||||
}
|
||||
});
|
||||
|
||||
it("does not open the editor for a view-only admin", async () => {
|
||||
renderView({}, { isViewOnly: true, isEditing: true });
|
||||
await userEvent.click(screen.getByRole("tab", { name: "Settings" }));
|
||||
expect(screen.getByRole("button", { name: "Edit Settings" })).toBeDisabled();
|
||||
expect(screen.queryByText("edit form")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("opens on the tab named by initialTabIndex", async () => {
|
||||
renderView({}, { initialTabIndex: 1 });
|
||||
|
||||
|
|
|
|||
|
|
@ -62,7 +62,8 @@ export const MCPServerView: React.FC<MCPServerViewProps> = ({
|
|||
}) => {
|
||||
// Open the editing Settings tab on first render when returning from the edit OAuth
|
||||
// redirect, so the "token fetched" feedback shows where the user left off (Settings=2).
|
||||
const returningFromEditOAuth = isReturningFromEditOAuth(isProxyAdmin, mcpServer.server_id);
|
||||
const canEdit = isProxyAdmin && !isViewOnly && !mcpServer.is_config;
|
||||
const returningFromEditOAuth = isReturningFromEditOAuth(canEdit, mcpServer.server_id);
|
||||
const [editing, setEditing] = useState(isEditing || returningFromEditOAuth);
|
||||
const [showFullUrl, setShowFullUrl] = useState(false);
|
||||
const [copiedStates, setCopiedStates] = useState<Record<string, boolean>>({});
|
||||
|
|
@ -224,13 +225,18 @@ export const MCPServerView: React.FC<MCPServerViewProps> = ({
|
|||
<Card className="p-6">
|
||||
<div className="mb-4 flex items-center justify-between">
|
||||
<h2 className="text-lg font-medium">MCP Server Settings</h2>
|
||||
{editing ? null : (
|
||||
<Button variant="outline" onClick={() => setEditing(true)}>
|
||||
{editing && canEdit ? null : (
|
||||
<Button variant="outline" disabled={!canEdit} onClick={() => setEditing(true)}>
|
||||
Edit Settings
|
||||
</Button>
|
||||
)}
|
||||
</div>
|
||||
{editing ? (
|
||||
{mcpServer.is_config && (
|
||||
<p className="mb-4 text-sm text-muted-foreground">
|
||||
Defined in config. Edit your YAML configuration to make changes
|
||||
</p>
|
||||
)}
|
||||
{editing && canEdit ? (
|
||||
<MCPServerEdit
|
||||
mcpServer={mcpServer}
|
||||
accessToken={accessToken}
|
||||
|
|
|
|||
|
|
@ -407,6 +407,7 @@ export interface MCPToolsViewerProps {
|
|||
|
||||
export interface MCPServer {
|
||||
server_id: string;
|
||||
is_config?: boolean;
|
||||
server_name?: string | null;
|
||||
alias?: string | null;
|
||||
description?: string | null;
|
||||
|
|
|
|||
6
ui/litellm-dashboard/src/lib/http/schema.d.ts
generated
vendored
6
ui/litellm-dashboard/src/lib/http/schema.d.ts
generated
vendored
|
|
@ -30680,6 +30680,12 @@ export interface components {
|
|||
* @default false
|
||||
*/
|
||||
is_byok: boolean;
|
||||
/**
|
||||
* Is Config
|
||||
* @description Whether this server is defined in config and is read-only.
|
||||
* @default false
|
||||
*/
|
||||
is_config: boolean;
|
||||
/** Issuer */
|
||||
issuer?: string | null;
|
||||
/** Last Health Check */
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue