diff --git a/litellm-proxy-extras/litellm_proxy_extras/migrations/20260401000000_restore_mcp_approval_columns/migration.sql b/litellm-proxy-extras/litellm_proxy_extras/migrations/20260401000000_restore_mcp_approval_columns/migration.sql new file mode 100644 index 000000000000..40da3c7b2460 --- /dev/null +++ b/litellm-proxy-extras/litellm_proxy_extras/migrations/20260401000000_restore_mcp_approval_columns/migration.sql @@ -0,0 +1,23 @@ +-- Restore BYOM approval workflow columns that were accidentally dropped by +-- 20260311180521_schema_sync. That migration was auto-generated because the +-- root schema.prisma was not updated when PR #23205 added the submission +-- workflow. This migration re-adds the columns and index. + +ALTER TABLE "LiteLLM_MCPServerTable" + ADD COLUMN IF NOT EXISTS "source_url" TEXT, + ADD COLUMN IF NOT EXISTS "approval_status" TEXT DEFAULT 'active', + ADD COLUMN IF NOT EXISTS "submitted_by" TEXT, + ADD COLUMN IF NOT EXISTS "submitted_at" TIMESTAMP(3), + ADD COLUMN IF NOT EXISTS "reviewed_at" TIMESTAMP(3), + ADD COLUMN IF NOT EXISTS "review_notes" TEXT; + +-- Back-fill existing rows: anything already in the table is implicitly active. +-- Also normalise the old "approved" default written by a prior schema version +-- that used @default("approved") instead of @default("active"). +UPDATE "LiteLLM_MCPServerTable" + SET "approval_status" = 'active' + WHERE "approval_status" IS NULL OR "approval_status" = 'approved'; + +-- CreateIndex +CREATE INDEX IF NOT EXISTS "LiteLLM_MCPServerTable_approval_status_idx" + ON "LiteLLM_MCPServerTable"("approval_status"); diff --git a/litellm-proxy-extras/litellm_proxy_extras/schema.prisma b/litellm-proxy-extras/litellm_proxy_extras/schema.prisma index a2c83295403f..4b70229a839d 100644 --- a/litellm-proxy-extras/litellm_proxy_extras/schema.prisma +++ b/litellm-proxy-extras/litellm_proxy_extras/schema.prisma @@ -320,11 +320,15 @@ model LiteLLM_MCPServerTable { is_byok Boolean @default(false) byok_description String[] @default([]) byok_api_key_help_url String? - approval_status String @default("approved") + source_url String? + // BYOM submission lifecycle + approval_status String? @default("active") submitted_by String? submitted_at DateTime? reviewed_at DateTime? review_notes String? + + @@index([approval_status]) } // Per-user BYOK credentials for MCP servers diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index f29a721ede80..0c92ada9d0eb 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -1288,17 +1288,18 @@ async def add_session_mcp_server( # Validate and normalize payload fields (alias/server name rules) validate_and_normalize_mcp_server_payload(payload) - # Restrict to proxy admins similar to the persistent create endpoint - if LitellmUserRoles.PROXY_ADMIN != user_api_key_dict.user_role: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail={ - "error": "User does not have permission to create temporary mcp servers. You can only create temporary mcp servers if you are a PROXY_ADMIN." - }, - ) - + # Session servers are ephemeral (in-memory, ~5 min TTL, no DB write) so + # any authenticated user may create one. This lets non-admin users run + # the OAuth auth-test before submitting a server for review. created_by = user_api_key_dict.user_id or LITELLM_PROXY_ADMIN_NAME - payload_with_credentials = _inherit_credentials_from_existing_server(payload) + # Only proxy admins may inherit credentials from an existing permanent + # server. Allowing non-admins to do so would let any key holder supply + # a known server_id and silently acquire that server's stored secrets + # (OAuth client_secret, AWS keys, etc.) into their session cache entry. + if user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN: + payload_with_credentials = _inherit_credentials_from_existing_server(payload) + else: + payload_with_credentials = payload temp_record = _build_temporary_mcp_server_record( payload_with_credentials, created_by, diff --git a/schema.prisma b/schema.prisma index fde9a466a283..4b70229a839d 100644 --- a/schema.prisma +++ b/schema.prisma @@ -320,6 +320,15 @@ model LiteLLM_MCPServerTable { is_byok Boolean @default(false) byok_description String[] @default([]) byok_api_key_help_url String? + source_url String? + // BYOM submission lifecycle + approval_status String? @default("active") + submitted_by String? + submitted_at DateTime? + reviewed_at DateTime? + review_notes String? + + @@index([approval_status]) } // Per-user BYOK credentials for MCP servers diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_creation_fixes.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_creation_fixes.py new file mode 100644 index 000000000000..7f7225189b86 --- /dev/null +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_creation_fixes.py @@ -0,0 +1,442 @@ +""" +Tests covering the MCP creation fixes from the fix_mcp_creation branch: + +1. _prepare_mcp_server_data includes approval_status when set (was silently + excluded, triggering a Prisma "Could not find field" error). +2. NewMCPServerRequest accepts oauth2_flow and stores the correct value. +3. admin direct-create endpoint enforces approval_status=active. +4. non-admin users can create session (ephemeral) MCP servers. +""" + +import pytest +from datetime import datetime +from typing import Optional +from unittest.mock import AsyncMock, MagicMock, patch + +from litellm.proxy._types import ( + LiteLLM_MCPServerTable, + LitellmUserRoles, + MCPApprovalStatus, + MCPTransport, + NewMCPServerRequest, + UserAPIKeyAuth, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_admin_auth(user_id: str = "admin-user") -> UserAPIKeyAuth: + return UserAPIKeyAuth( + api_key="sk-test", + user_id=user_id, + user_role=LitellmUserRoles.PROXY_ADMIN, + ) + + +def _make_internal_user_auth( + user_id: str = "user-abc", team_id: Optional[str] = None +) -> UserAPIKeyAuth: + return UserAPIKeyAuth( + api_key="sk-test", + user_id=user_id, + user_role=LitellmUserRoles.INTERNAL_USER, + team_id=team_id, + ) + + +def _make_db_record( + server_id: str = "srv-1", + alias: str = "Test Server", + approval_status: Optional[str] = "active", +) -> LiteLLM_MCPServerTable: + now = datetime.now() + record = LiteLLM_MCPServerTable( + server_id=server_id, + alias=alias, + url="https://example.com/mcp", + transport=MCPTransport.http, + created_at=now, + updated_at=now, + created_by="test", + updated_by="test", + ) + record.approval_status = approval_status + return record + + +# --------------------------------------------------------------------------- +# _prepare_mcp_server_data: approval_status must be included in data dict +# --------------------------------------------------------------------------- + + +class TestPrepareDataIncludesApprovalStatus: + """Ensure _prepare_mcp_server_data forwards approval_status to Prisma. + + Before the fix, model_dump(exclude_none=True) would drop approval_status + when the endpoint forgot to set it, and even when it was set to an enum + value the Prisma engine rejected it with "Could not find field" because the + root schema.prisma was missing the field. These tests guard both paths. + """ + + def test_active_status_included_in_dict(self): + from litellm.proxy._experimental.mcp_server.db import _prepare_mcp_server_data + + req = NewMCPServerRequest( + alias="My Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + ) + req.approval_status = MCPApprovalStatus.active + + data = _prepare_mcp_server_data(req) + + assert "approval_status" in data + assert data["approval_status"] == MCPApprovalStatus.active + + def test_pending_review_status_included_in_dict(self): + from litellm.proxy._experimental.mcp_server.db import _prepare_mcp_server_data + + req = NewMCPServerRequest( + alias="My Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + ) + req.approval_status = MCPApprovalStatus.pending_review + req.submitted_by = "user-abc" + + data = _prepare_mcp_server_data(req) + + assert data["approval_status"] == MCPApprovalStatus.pending_review + assert data["submitted_by"] == "user-abc" + + def test_submitted_at_included_when_set(self): + from datetime import timezone + from litellm.proxy._experimental.mcp_server.db import _prepare_mcp_server_data + + req = NewMCPServerRequest( + alias="My Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + ) + req.approval_status = MCPApprovalStatus.pending_review + req.submitted_at = datetime(2026, 4, 1, 12, 0, 0, tzinfo=timezone.utc) + + data = _prepare_mcp_server_data(req) + + assert "submitted_at" in data + assert data["submitted_at"] == req.submitted_at + + +# --------------------------------------------------------------------------- +# oauth2_flow: field accepted and stored correctly +# --------------------------------------------------------------------------- + + +class TestOAuth2FlowField: + """NewMCPServerRequest must accept oauth2_flow values that the UI now maps to.""" + + def test_client_credentials_accepted(self): + req = NewMCPServerRequest( + alias="M2M Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + oauth2_flow="client_credentials", + ) + assert req.oauth2_flow == "client_credentials" + + def test_authorization_code_accepted(self): + req = NewMCPServerRequest( + alias="Interactive Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + oauth2_flow="authorization_code", + ) + assert req.oauth2_flow == "authorization_code" + + def test_oauth2_flow_included_in_prepare_data(self): + from litellm.proxy._experimental.mcp_server.db import _prepare_mcp_server_data + + req = NewMCPServerRequest( + alias="M2M Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + oauth2_flow="client_credentials", + ) + req.approval_status = MCPApprovalStatus.active + + data = _prepare_mcp_server_data(req) + + assert data.get("oauth2_flow") == "client_credentials" + + def test_none_oauth2_flow_not_forwarded(self): + """When oauth2_flow is not set, it should be absent from the data dict + (exclude_none=True keeps the dict clean).""" + from litellm.proxy._experimental.mcp_server.db import _prepare_mcp_server_data + + req = NewMCPServerRequest( + alias="API Key Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + ) + req.approval_status = MCPApprovalStatus.active + + data = _prepare_mcp_server_data(req) + + assert "oauth2_flow" not in data + + +# --------------------------------------------------------------------------- +# add_mcp_server: admin endpoint always overrides to active +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_add_mcp_server_overrides_approval_status_to_active(): + """Even if the caller supplies approval_status='pending_review', the admin + create endpoint must override it to 'active' and clear submission fields.""" + try: + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + add_mcp_server, + ) + except ImportError: + pytest.skip("MCP management endpoints not available") + + payload = NewMCPServerRequest( + alias="My Server", + url="https://example.com/mcp", + transport=MCPTransport.http, + # Caller attempts to sneak in a pending status — must be overridden. + approval_status="pending_review", + submitted_by="attacker", + ) + admin = _make_admin_auth() + created_record = _make_db_record(approval_status="active") + + mock_manager = MagicMock() + mock_manager.add_server = AsyncMock() + mock_manager.reload_servers_from_database = AsyncMock() + + with ( + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw", + return_value=MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload", + MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.get_mcp_server", + AsyncMock(return_value=None), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server", + AsyncMock(return_value=created_record), + ) as mock_create, + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager", + mock_manager, + ), + ): + await add_mcp_server(payload=payload, user_api_key_dict=admin) + + call_payload: NewMCPServerRequest = mock_create.call_args[0][1] + assert call_payload.approval_status == MCPApprovalStatus.active + assert call_payload.submitted_by is None + assert call_payload.submitted_at is None + + +# --------------------------------------------------------------------------- +# add_session_mcp_server: non-admin users now allowed +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_add_session_mcp_server_allowed_for_non_admin(): + """Any authenticated user (not just PROXY_ADMIN) may create an ephemeral + session server. The session endpoint writes nothing to the database so + there is no meaningful security risk.""" + try: + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + TEMPORARY_MCP_SERVER_TTL_SECONDS, + add_session_mcp_server, + ) + except ImportError: + pytest.skip("MCP management endpoints not available") + + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + payload = NewMCPServerRequest( + alias="Temp Server", + server_id="temp-1", + url="https://temp.example.com/mcp", + transport=MCPTransport.http, + ) + non_admin = _make_internal_user_auth(user_id="submitter-user") + + built_server = MCPServer( + server_id="temp-1", + name="Temp Server", + url="https://temp.example.com/mcp", + transport=MCPTransport.http, + ) + mock_manager = MagicMock() + mock_manager.get_mcp_server_by_id.return_value = None + mock_manager.build_mcp_server_from_table = AsyncMock(return_value=built_server) + + with ( + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload", + MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager", + mock_manager, + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints._cache_temporary_mcp_server", + MagicMock(), + ) as cache_mock, + ): + response = await add_session_mcp_server( + payload=payload, + user_api_key_dict=non_admin, + ) + + # Should succeed and cache the server + cache_mock.assert_called_once() + assert response is not None + + +@pytest.mark.asyncio +async def test_add_session_mcp_server_created_by_reflects_non_admin_user_id(): + """created_by on the temp record uses the user's ID, not LITELLM_PROXY_ADMIN_NAME.""" + try: + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + add_session_mcp_server, + ) + except ImportError: + pytest.skip("MCP management endpoints not available") + + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + payload = NewMCPServerRequest( + alias="Temp Server", + server_id="temp-2", + url="https://temp.example.com/mcp", + transport=MCPTransport.http, + ) + non_admin = _make_internal_user_auth(user_id="non-admin-123") + + built_server = MCPServer( + server_id="temp-2", + name="Temp Server", + url="https://temp.example.com/mcp", + transport=MCPTransport.http, + ) + mock_manager = MagicMock() + mock_manager.get_mcp_server_by_id.return_value = None + mock_manager.build_mcp_server_from_table = AsyncMock(return_value=built_server) + captured_temp_record = {} + + def capture_cache(server, ttl_seconds): + pass + + with ( + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload", + MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager", + mock_manager, + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints._cache_temporary_mcp_server", + MagicMock(side_effect=capture_cache), + ), + ): + await add_session_mcp_server( + payload=payload, + user_api_key_dict=non_admin, + ) + + # build_mcp_server_from_table was called with the temp record + args, _ = mock_manager.build_mcp_server_from_table.call_args + temp_record = args[0] + assert temp_record.created_by == "non-admin-123" + + +@pytest.mark.asyncio +async def test_add_session_mcp_server_non_admin_does_not_inherit_credentials(): + """Non-admin callers must NOT inherit credentials from an existing permanent + server. Before the fix, _inherit_credentials_from_existing_server ran + unconditionally, so any key holder who knew a server_id could cause the + proxy to silently copy that server's OAuth / AWS secrets into their session + cache entry. + """ + try: + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + add_session_mcp_server, + ) + except ImportError: + pytest.skip("MCP management endpoints not available") + + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + # Payload deliberately omits credentials — non-admin is trying to inherit + # them from the permanent server "srv-with-secrets". + payload = NewMCPServerRequest( + alias="Hijack Attempt", + server_id="srv-with-secrets", + url="https://internal.example.com/mcp", + transport=MCPTransport.http, + ) + non_admin = _make_internal_user_auth(user_id="attacker-456") + + built_server = MCPServer( + server_id="srv-with-secrets", + name="Hijack Attempt", + url="https://internal.example.com/mcp", + transport=MCPTransport.http, + ) + + # The permanent server has sensitive credentials stored. + existing_server = MagicMock() + existing_server.authentication_token = "super-secret-token" + existing_server.client_secret = "oauth-client-secret" + existing_server.aws_secret_access_key = "aws-secret" + + mock_manager = MagicMock() + mock_manager.get_mcp_server_by_id.return_value = existing_server + mock_manager.build_mcp_server_from_table = AsyncMock(return_value=built_server) + + with ( + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload", + MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager", + mock_manager, + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints._cache_temporary_mcp_server", + MagicMock(), + ), + ): + await add_session_mcp_server( + payload=payload, + user_api_key_dict=non_admin, + ) + + # The temp record passed to build_mcp_server_from_table must NOT contain + # any credentials inherited from the permanent server. + args, _ = mock_manager.build_mcp_server_from_table.call_args + temp_record = args[0] + assert temp_record.credentials is None, ( + "Non-admin session server must not inherit credentials from permanent server" + ) diff --git a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py index 77ac3a040acc..55076335b6d5 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py @@ -1417,8 +1417,12 @@ async def test_add_session_mcp_server_caches_and_redacts_credentials(self): assert response.credentials is None @pytest.mark.asyncio - async def test_add_session_mcp_server_rejects_non_admins(self): + async def test_add_session_mcp_server_allows_non_admins(self): + """Non-admin users can create ephemeral session servers so they can run the + auth test before submitting an MCP for review. The session endpoint writes + nothing to the database, so there is no meaningful security concern.""" from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + TEMPORARY_MCP_SERVER_TTL_SECONDS, add_session_mcp_server, ) @@ -1430,19 +1434,36 @@ async def test_add_session_mcp_server_rejects_non_admins(self): ) non_admin = generate_mock_user_api_key_auth( user_role=LitellmUserRoles.INTERNAL_USER, + user_id="non-admin-user", ) - with patch( - "litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload", - MagicMock(), + built_server = generate_mock_mcp_server_config_record(server_id="temp-server") + mock_manager = MagicMock() + mock_manager.get_mcp_server_by_id.return_value = None + mock_manager.build_mcp_server_from_table = AsyncMock(return_value=built_server) + + with ( + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload", + MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager", + mock_manager, + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints._cache_temporary_mcp_server", + MagicMock(), + ) as cache_mock, ): - with pytest.raises(Exception) as exc_info: - await add_session_mcp_server( - payload=payload, - user_api_key_dict=non_admin, - ) + # Should NOT raise — non-admins are now permitted + response = await add_session_mcp_server( + payload=payload, + user_api_key_dict=non_admin, + ) - assert "permission" in str(exc_info.value) + cache_mock.assert_called_once() + assert response is not None @pytest.mark.asyncio async def test_mcp_authorize_proxies_to_discoverable_endpoint(self): @@ -2279,6 +2300,91 @@ async def test_reject_active_server_allowed(self): mock_manager.reload_servers_from_database.assert_awaited_once() + @pytest.mark.asyncio + async def test_register_mcp_server_rejects_proxy_admin(self): + """PROXY_ADMIN users must use POST /v1/mcp/server, not the submission endpoint.""" + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + register_mcp_server, + ) + + payload = NewMCPServerRequest( + alias="My Server", + url="https://example.com/mcp", + transport=MCPTransport.sse, + ) + admin = generate_mock_user_api_key_auth( + user_role=LitellmUserRoles.PROXY_ADMIN, + team_id="team-123", + ) + with pytest.raises(HTTPException) as exc_info: + await register_mcp_server(payload=payload, user_api_key_dict=admin) + assert exc_info.value.status_code == 403 + assert "/v1/mcp/server" in str(exc_info.value.detail) + + @pytest.mark.asyncio + async def test_add_mcp_server_sets_active_status_and_clears_submission_fields(self): + """Admin direct-create always sets approval_status=active and clears submitted_by/submitted_at, + regardless of what the caller provides in the payload.""" + from litellm.proxy._types import MCPApprovalStatus + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + add_mcp_server, + ) + + payload = NewMCPServerRequest( + alias="My Server", + url="https://example.com/mcp", + transport=MCPTransport.sse, + # Caller tries to sneak in submission fields — should be overridden. + approval_status="pending_review", + submitted_by="attacker", + ) + admin = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN) + created_record = generate_mock_mcp_server_db_record( + alias="My Server", + url="https://example.com/mcp", + ) + created_record.approval_status = MCPApprovalStatus.active + created_record.submitted_by = None + created_record.submitted_at = None + + mock_manager = MagicMock() + mock_manager.add_server = AsyncMock() + mock_manager.reload_servers_from_database = AsyncMock() + + with ( + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw", + return_value=MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.validate_and_normalize_mcp_server_payload", + MagicMock(), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.get_mcp_server", + AsyncMock(return_value=None), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.create_mcp_server", + AsyncMock(return_value=created_record), + ) as mock_create, + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager", + mock_manager, + ), + ): + result = await add_mcp_server( + payload=payload, + user_api_key_dict=admin, + ) + + call_payload = mock_create.call_args[0][1] + assert call_payload.approval_status == MCPApprovalStatus.active + assert call_payload.submitted_by is None + assert call_payload.submitted_at is None + assert result is not None + + class TestValidateMCPRequiredFields: """Tests for _validate_mcp_required_fields.""" diff --git a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx index d49c49446bbf..eed7875b3525 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx @@ -6,6 +6,7 @@ import CreateMCPServer from "./create_mcp_server"; vi.mock("../networking", () => ({ createMCPServer: vi.fn(), + registerMCPServer: vi.fn(), testMCPToolsListRequest: vi.fn().mockResolvedValue({ tools: [], error: null }), })); @@ -417,4 +418,187 @@ describe("CreateMCPServer", () => { expect(onBackToDiscovery).toHaveBeenCalledTimes(1); }); }); + + // --------------------------------------------------------------------------- + // oauth_flow_type → oauth2_flow mapping + // --------------------------------------------------------------------------- + // + // The UI form uses a UI-only field "oauth_flow_type" with values "m2m" / + // "interactive". The backend expects "oauth2_flow" with values + // "client_credentials" / "authorization_code". The submit handler must map + // the UI field to the backend field and remove oauth_flow_type from the + // payload entirely. + + describe("oauth_flow_type → oauth2_flow mapping", () => { + /** Helper: select HTTP transport and OAuth2 auth type so OAuthFormFields renders. */ + async function selectOAuth2Auth() { + render(); + await selectAntOption("Transport Type", "Streamable HTTP"); + await waitFor(() => + expect(screen.getByPlaceholderText("https://your-mcp-server.com")).toBeInTheDocument() + ); + await selectAntOption("Authentication", "OAuth2"); + // OAuthFormFields appears once OAuth2 is selected + await waitFor(() => + expect(screen.getByText("OAuth Flow Type")).toBeInTheDocument() + ); + } + + it( + "should send oauth2_flow: 'authorization_code' for Interactive OAuth and omit oauth_flow_type", + { timeout: 15000 }, + async () => { + await selectOAuth2Auth(); + + const user = userEvent.setup({ delay: null }); + + // Select Interactive (PKCE) flow + await selectAntOption("OAuth Flow Type", "Interactive"); + + // Fill required top-level fields + const nameInput = getServerNameInput(); + await user.type(nameInput, "OAuth_Interactive_Server"); + + const urlInput = screen.getByPlaceholderText("https://your-mcp-server.com"); + await user.type(urlInput, "https://interactive.example.com/mcp"); + + vi.mocked(networking.createMCPServer).mockResolvedValue({ + server_id: "oauth-server-1", + alias: "OAuth_Interactive_Server", + url: "https://interactive.example.com/mcp", + transport: "http", + auth_type: "oauth2", + }); + + const submitButton = screen.getByRole("button", { name: "Add MCP Server" }); + await act(async () => { + fireEvent.click(submitButton); + }); + + await waitFor(() => { + expect(networking.createMCPServer).toHaveBeenCalledTimes(1); + }); + + const [, payload] = vi.mocked(networking.createMCPServer).mock.calls[0]; + // Must use backend field name with correct value + expect(payload.oauth2_flow).toBe("authorization_code"); + // Must NOT forward the UI-only field + expect(payload.oauth_flow_type).toBeUndefined(); + } + ); + + it( + "should send oauth2_flow: 'client_credentials' for M2M OAuth and omit oauth_flow_type", + { timeout: 15000 }, + async () => { + await selectOAuth2Auth(); + + const user = userEvent.setup({ delay: null }); + + // Select M2M flow — this causes Client ID / Client Secret / Token URL fields to appear + await selectAntOption("OAuth Flow Type", "Machine-to-Machine"); + + await waitFor(() => + expect(screen.getByPlaceholderText(/Enter OAuth client ID/i)).toBeInTheDocument() + ); + + // Fill required M2M OAuth fields + const clientIdInput = screen.getByPlaceholderText(/Enter OAuth client ID/i); + await user.type(clientIdInput, "my-client-id"); + + const clientSecretInput = screen.getByPlaceholderText(/Enter OAuth client secret/i); + await user.type(clientSecretInput, "my-client-secret"); + + const tokenUrlInput = screen.getByPlaceholderText("https://auth.example.com/oauth/token"); + await user.type(tokenUrlInput, "https://auth.example.com/oauth/token"); + + // Fill required top-level fields + const nameInput = getServerNameInput(); + await user.type(nameInput, "OAuth_M2M_Server"); + + const urlInput = screen.getByPlaceholderText("https://your-mcp-server.com"); + await user.type(urlInput, "https://m2m.example.com/mcp"); + + vi.mocked(networking.createMCPServer).mockResolvedValue({ + server_id: "oauth-m2m-1", + alias: "OAuth_M2M_Server", + url: "https://m2m.example.com/mcp", + transport: "http", + auth_type: "oauth2", + }); + + const submitButton = screen.getByRole("button", { name: "Add MCP Server" }); + await act(async () => { + fireEvent.click(submitButton); + }); + + await waitFor(() => { + expect(networking.createMCPServer).toHaveBeenCalledTimes(1); + }); + + const [, payload] = vi.mocked(networking.createMCPServer).mock.calls[0]; + // Must use backend field name with correct value + expect(payload.oauth2_flow).toBe("client_credentials"); + // Must NOT forward the UI-only field + expect(payload.oauth_flow_type).toBeUndefined(); + } + ); + + it( + "should use registerMCPServer for non-admin with oauth2_flow mapped correctly", + { timeout: 15000 }, + async () => { + // Re-mock to also include registerMCPServer + vi.mocked(networking as any).registerMCPServer = vi.fn().mockResolvedValue({ + server_id: "pending-oauth-1", + alias: "OAuth_Submission", + approval_status: "pending_review", + }); + + render( + + ); + + // Non-admin sees the submission form + await waitFor(() => + expect(screen.getByText("Submit MCP Server for Review")).toBeInTheDocument() + ); + + await selectAntOption("Transport Type", "Streamable HTTP"); + await waitFor(() => + expect(screen.getByPlaceholderText("https://your-mcp-server.com")).toBeInTheDocument() + ); + await selectAntOption("Authentication", "OAuth2"); + await waitFor(() => + expect(screen.getByText("OAuth Flow Type")).toBeInTheDocument() + ); + + // Select Interactive flow + await selectAntOption("OAuth Flow Type", "Interactive"); + + const user = userEvent.setup({ delay: null }); + const nameInput = getServerNameInput(); + await user.type(nameInput, "OAuth_Submission_Server"); + + const urlInput = screen.getByPlaceholderText("https://your-mcp-server.com"); + await user.type(urlInput, "https://interactive.example.com/mcp"); + + const submitButton = screen.getByRole("button", { name: "Submit for Review" }); + await act(async () => { + fireEvent.click(submitButton); + }); + + await waitFor(() => { + expect((networking as any).registerMCPServer).toHaveBeenCalledTimes(1); + }); + + const [, payload] = (networking as any).registerMCPServer.mock.calls[0]; + expect(payload.oauth2_flow).toBe("authorization_code"); + expect(payload.oauth_flow_type).toBeUndefined(); + } + ); + }); }); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx index e6845402893d..77b72f667d33 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx @@ -355,6 +355,17 @@ const CreateMCPServer: React.FC = ({ restValues.transport = "http"; } + // Map UI-only oauth_flow_type ("m2m"/"interactive") to the backend field + // oauth2_flow ("client_credentials"/"authorization_code"), then remove the + // UI-only key so it isn't forwarded as an unknown field. + if (restValues.oauth_flow_type) { + restValues.oauth2_flow = + restValues.oauth_flow_type === OAUTH_FLOW.M2M + ? "client_credentials" + : "authorization_code"; + delete restValues.oauth_flow_type; + } + // Prepare the payload with cost configuration and allowed tools const payload: Record = { ...restValues, diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx index e33e2fff491f..d20f700be17b 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx @@ -37,6 +37,93 @@ vi.mock("./mcp_tool_configuration", () => ({ default: () =>
, })); +// --------------------------------------------------------------------------- +// oauth_flow_type → oauth2_flow mapping (edit path) +// --------------------------------------------------------------------------- + +describe("MCPServerEdit oauth_flow_type mapping", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + const httpOAuthBase = { + server_id: "srv-oauth", + server_name: "OAuthServer", + alias: "oauth-srv", + description: "OAuth server", + transport: "http" as const, + url: "https://example.com/mcp", + auth_type: "oauth2", + command: null, + args: null, + env: null, + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + mcp_access_groups: [] as string[], + authorization_url: "https://auth.example.com/authorize", + }; + + it("sends oauth2_flow: client_credentials for M2M server (token_url present)", async () => { + const mcpServer = { ...httpOAuthBase, token_url: "https://auth.example.com/token" }; + + vi.mocked(networking.updateMCPServer).mockResolvedValue({ ...mcpServer }); + + render( + , + ); + + const saveButtons = screen.getAllByRole("button", { name: "Save Changes" }); + await act(async () => { + fireEvent.click(saveButtons[0]); + }); + + await waitFor(() => { + expect(networking.updateMCPServer).toHaveBeenCalledTimes(1); + }); + + const [, payload] = vi.mocked(networking.updateMCPServer).mock.calls[0]; + expect(payload.oauth2_flow).toBe("client_credentials"); + expect(payload).not.toHaveProperty("oauth_flow_type"); + }); + + it("sends oauth2_flow: authorization_code for interactive server (no token_url)", async () => { + const mcpServer = { ...httpOAuthBase, token_url: undefined }; + + vi.mocked(networking.updateMCPServer).mockResolvedValue({ ...mcpServer }); + + render( + , + ); + + const saveButtons = screen.getAllByRole("button", { name: "Save Changes" }); + await act(async () => { + fireEvent.click(saveButtons[0]); + }); + + await waitFor(() => { + expect(networking.updateMCPServer).toHaveBeenCalledTimes(1); + }); + + const [, payload] = vi.mocked(networking.updateMCPServer).mock.calls[0]; + expect(payload.oauth2_flow).toBe("authorization_code"); + expect(payload).not.toHaveProperty("oauth_flow_type"); + }); +}); + describe("MCPServerEdit (stdio)", () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx index 04cce3430386..5f827e734099 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx @@ -521,6 +521,17 @@ const MCPServerEdit: React.FC = ({ restValues.transport = "http"; } + // Map UI-only oauth_flow_type ("m2m"/"interactive") to the backend field + // oauth2_flow ("client_credentials"/"authorization_code"), then remove the + // UI-only key so it isn't forwarded as an unknown field. + if (restValues.oauth_flow_type) { + restValues.oauth2_flow = + restValues.oauth_flow_type === OAUTH_FLOW.M2M + ? "client_credentials" + : "authorization_code"; + delete restValues.oauth_flow_type; + } + // Prepare the payload with cost configuration and permission fields const mcpInfoServerName = restValues.server_name ||