From fb585d7737133f16aac66ed83f2808d4717eedd1 Mon Sep 17 00:00:00 2001 From: centdix Date: Mon, 8 Jun 2026 17:23:48 +0200 Subject: [PATCH] fix: improve mcp oauth connection flow --- backend/windmill-api/src/mcp_tools.rs | 18 +- backend/windmill-mcp/src/client/mod.rs | 29 +- backend/windmill-mcp/src/lib.rs | 2 +- .../flows/content/McpOAuthConnect.svelte | 356 +++++++++++++----- .../flows/content/McpToolEditor.svelte | 225 +++++++++-- 5 files changed, 481 insertions(+), 149 deletions(-) diff --git a/backend/windmill-api/src/mcp_tools.rs b/backend/windmill-api/src/mcp_tools.rs index bba945ac54..dddef6144e 100644 --- a/backend/windmill-api/src/mcp_tools.rs +++ b/backend/windmill-api/src/mcp_tools.rs @@ -101,9 +101,21 @@ pub(crate) async fn get_mcp_tools( None }; - let client = windmill_mcp::McpClient::from_resource(mcp_resource, token) - .await - .map_err(|e| Error::ExecutionErr(format!("Failed to connect to MCP server: {}", e)))?; + let client = match windmill_mcp::McpClient::from_resource(mcp_resource, token).await { + Ok(client) => client, + Err(e) if windmill_mcp::is_auth_required_error(&e) => { + return Err(Error::NotAuthorized(format!( + "MCP server requires authentication: {}", + e + ))); + } + Err(e) => { + return Err(Error::ExecutionErr(format!( + "Failed to connect to MCP server: {}", + e + ))); + } + }; let tools: Vec = client .available_tools() diff --git a/backend/windmill-mcp/src/client/mod.rs b/backend/windmill-mcp/src/client/mod.rs index 1a555c141f..2ee268327c 100644 --- a/backend/windmill-mcp/src/client/mod.rs +++ b/backend/windmill-mcp/src/client/mod.rs @@ -14,9 +14,10 @@ use rmcp::{ CallToolRequestParams, ClientCapabilities, ClientInfo, Implementation, InitializeRequestParams, Tool as McpTool, }, - service::RunningService, + service::{ClientInitializeError, RunningService}, transport::{ - streamable_http_client::StreamableHttpClientTransportConfig, StreamableHttpClientTransport, + streamable_http_client::{StreamableHttpClientTransportConfig, StreamableHttpError}, + StreamableHttpClientTransport, }, RoleClient, ServiceExt, }; @@ -31,6 +32,30 @@ pub struct McpClient { available_tools: Vec, } +pub fn is_auth_required_error(error: &anyhow::Error) -> bool { + error.chain().any(|source| { + if matches!( + source.downcast_ref::>(), + Some(StreamableHttpError::AuthRequired(_)) + ) { + return true; + } + + if let Some(ClientInitializeError::TransportError { error, .. }) = + source.downcast_ref::() + { + return matches!( + error + .error + .downcast_ref::>(), + Some(StreamableHttpError::AuthRequired(_)) + ); + } + + false + }) +} + impl McpClient { /// Create a new MCP client from a resource configuration. /// diff --git a/backend/windmill-mcp/src/lib.rs b/backend/windmill-mcp/src/lib.rs index 7df6ee9f39..ddd43e7092 100644 --- a/backend/windmill-mcp/src/lib.rs +++ b/backend/windmill-mcp/src/lib.rs @@ -20,7 +20,7 @@ pub use common::{ }; // Re-export client types at crate root for backward compatibility -pub use client::{McpClient, McpResource, McpToolSource}; +pub use client::{is_auth_required_error, McpClient, McpResource, McpToolSource}; // Re-export rmcp types for client usage pub use rmcp::model::Tool as McpTool; diff --git a/frontend/src/lib/components/flows/content/McpOAuthConnect.svelte b/frontend/src/lib/components/flows/content/McpOAuthConnect.svelte index f97036df20..29e1aa3f70 100644 --- a/frontend/src/lib/components/flows/content/McpOAuthConnect.svelte +++ b/frontend/src/lib/components/flows/content/McpOAuthConnect.svelte @@ -12,14 +12,32 @@ import Path from '$lib/components/Path.svelte' import { sendUserToast } from '$lib/toast' import { sameTopDomainOrigin } from '$lib/cookies' - import { onDestroy } from 'svelte' + import { onDestroy, onMount } from 'svelte' interface Props { onConnected: (resourcePath: string, resourceName: string) => void - onCancel: () => void + onCancel?: () => void + onDiscoveryStatus?: ( + status: 'discovering' | 'supported' | 'unsupported', + error?: string + ) => void + initialServerUrl?: string + initialResourceName?: string + initialResourcePath?: string + updateExistingResource?: boolean + hideUntilDiscovered?: boolean } - let { onConnected, onCancel }: Props = $props() + let { + onConnected, + onCancel, + onDiscoveryStatus, + initialServerUrl = '', + initialResourceName = '', + initialResourcePath = '', + updateExistingResource = false, + hideUntilDiscovered = false + }: Props = $props() let serverUrl = $state('') let discoveryResult = $state(null) @@ -27,29 +45,60 @@ let resourceName = $state('') let resourcePath = $state('') let pathError = $state('') - let status = $state<'idle' | 'discovering' | 'discovered' | 'connecting'>('idle') + let status = $state<'idle' | 'discovering' | 'discovered' | 'unsupported' | 'connecting'>( + 'idle' + ) let error = $state(null) + let connectDisabled = $derived( + !resourcePath || (!updateExistingResource && pathError !== '') + ) + let shouldRender = $derived( + !hideUntilDiscovered || status === 'discovered' || status === 'connecting' + ) + + initializeFromProps() + + function initializeFromProps() { + serverUrl = initialServerUrl + resourceName = initialResourceName + resourcePath = initialResourcePath + if (initialServerUrl) { + status = 'discovering' + } + } + + onMount(() => { + if (initialServerUrl) { + void discoverOAuth() + } + }) + async function discoverOAuth() { status = 'discovering' error = null + onDiscoveryStatus?.('discovering') try { discoveryResult = await McpOauthService.discoverMcpOauth({ requestBody: { mcp_server_url: serverUrl } }) selectedScopes = discoveryResult?.scopes_supported ?? [] - try { - const urlObj = new URL(serverUrl) - resourceName = urlObj.hostname.replace(/\./g, '_') - } catch { - resourceName = 'mcp_server' + if (!resourceName.trim()) { + try { + const urlObj = new URL(serverUrl) + resourceName = urlObj.hostname.replace(/\./g, '_') + } catch { + resourceName = 'mcp_server' + } } status = 'discovered' - } catch (e) { + onDiscoveryStatus?.('supported') + } catch (e: any) { console.error('Error discovering OAuth settings', e) const errorMessage = e.body?.message || e.body || e.message || 'Unknown error' error = `Failed to discover OAuth settings: ${errorMessage}` - status = 'idle' + status = 'unsupported' + onDiscoveryStatus?.('unsupported', error) } } @@ -102,6 +151,38 @@ window.removeEventListener('storage', handleStorageEvent) } + function getVariablePathFromTokenRef(token: unknown): string | undefined { + if (typeof token !== 'string') { + return undefined + } + + const variablePath = token.trim().startsWith('$var:') + ? token.trim().slice('$var:'.length).trim() + : '' + + return variablePath.length > 0 ? variablePath : undefined + } + + async function getTokenVariablePath(existingTokenPath?: string): Promise { + if (existingTokenPath) { + return existingTokenPath + } + + const basePath = `${resourcePath}_token` + for (let index = 0; index < 20; index += 1) { + const candidatePath = index === 0 ? basePath : `${basePath}_${index + 1}` + const exists = await VariableService.existsVariable({ + workspace: $workspaceStore!, + path: candidatePath + }) + if (!exists) { + return candidatePath + } + } + + throw new Error('Could not find an available variable path for the MCP OAuth token') + } + async function createMcpResource(data: { access_token: string refresh_token?: string @@ -123,31 +204,79 @@ accountId = Number(accountIdStr) } - await VariableService.createVariable({ + let existingValue: Record = {} + if (updateExistingResource) { + const existingResource = await ResourceService.getResource({ + workspace: $workspaceStore!, + path: resourcePath + }) + existingValue = + existingResource.value && typeof existingResource.value === 'object' + ? (existingResource.value as Record) + : {} + } + + const existingTokenPath = getVariablePathFromTokenRef(existingValue.token) + const tokenVariablePath = await getTokenVariablePath(existingTokenPath) + const variableDescription = `MCP OAuth token for ${data.mcp_server_url}` + const variableExists = await VariableService.existsVariable({ workspace: $workspaceStore!, - requestBody: { - path: resourcePath, - value: data.access_token, - is_secret: true, - is_oauth: true, - account: accountId, - description: `MCP OAuth token for ${data.mcp_server_url}` - } + path: tokenVariablePath }) - await ResourceService.createResource({ - workspace: $workspaceStore!, - requestBody: { - resource_type: 'mcp', + if (variableExists) { + await VariableService.updateVariable({ + workspace: $workspaceStore!, + path: tokenVariablePath, + requestBody: { + path: tokenVariablePath, + value: data.access_token, + is_secret: true, + description: variableDescription + } + }) + } else { + await VariableService.createVariable({ + workspace: $workspaceStore!, + requestBody: { + path: tokenVariablePath, + value: data.access_token, + is_secret: true, + is_oauth: true, + account: accountId, + description: variableDescription + } + }) + } + + const updatedResourceValue = { + name: resourceName, + url: data.mcp_server_url, + token: `$var:${tokenVariablePath}` + } + + if (updateExistingResource) { + await ResourceService.updateResourceValue({ + workspace: $workspaceStore!, path: resourcePath, - value: { - name: resourceName, - url: data.mcp_server_url, - token: `$var:${resourcePath}` - }, - description: `MCP server connected via OAuth` - } - }) + requestBody: { + value: { + ...existingValue, + ...updatedResourceValue + } + } + }) + } else { + await ResourceService.createResource({ + workspace: $workspaceStore!, + requestBody: { + resource_type: 'mcp', + path: resourcePath, + value: updatedResourceValue, + description: `MCP server connected via OAuth` + } + }) + } sendUserToast('Connected to MCP server') onConnected(resourcePath, resourceName) @@ -160,84 +289,105 @@ onDestroy(cleanup) -
-
- Connect MCP Server with OAuth - -
- - - - {#if status === 'idle'} - - {:else if status === 'discovering'} -
Discovering OAuth settings...
- {:else if status === 'discovered' && discoveryResult} -
- ✓ OAuth supported - {#if discoveryResult.supports_dynamic_registration} - (Dynamic Client Registration available) +{#if shouldRender} +
+
+ + {updateExistingResource + ? 'Connect selected MCP resource with OAuth' + : 'Connect MCP Server with OAuth'} + + {#if onCancel} + {/if}
- {#if discoveryResult.scopes_supported && discoveryResult.scopes_supported.length > 0} - - {/if} - -
+ + + {#if updateExistingResource} + + {:else} + + {/if} + + + {:else if status === 'connecting'} +
Complete authentication in popup window...
+ {/if} + + {#if error && status !== 'unsupported'} +
{error}
+ {/if} +
+{/if} diff --git a/frontend/src/lib/components/flows/content/McpToolEditor.svelte b/frontend/src/lib/components/flows/content/McpToolEditor.svelte index 895fc27e2d..5c0b9c30a6 100644 --- a/frontend/src/lib/components/flows/content/McpToolEditor.svelte +++ b/frontend/src/lib/components/flows/content/McpToolEditor.svelte @@ -27,32 +27,146 @@ import ResourcePicker from '$lib/components/ResourcePicker.svelte' import { usePromise } from '$lib/svelte5Utils.svelte' import { untrack } from 'svelte' - import Alert from '$lib/components/common/alert/Alert.svelte' import McpOAuthConnect from './McpOAuthConnect.svelte' + import Alert from '$lib/components/common/alert/Alert.svelte' interface Props { tool: McpTool } + type McpResourceValue = { + name?: string + url: string + token?: string | null + } + + type OAuthDiscoveryStatus = 'none' | 'discovering' | 'supported' | 'unsupported' + + function getMcpResourceValue(value: unknown): McpResourceValue | undefined { + if (!value || typeof value !== 'object') { + return undefined + } + + const candidate = value as Record + const url = typeof candidate.url === 'string' ? candidate.url : undefined + + if (!url) { + return undefined + } + + return { + name: typeof candidate.name === 'string' ? candidate.name : undefined, + url, + token: + typeof candidate.token === 'string' || candidate.token === null + ? candidate.token + : undefined + } + } + + function getFallbackMcpResourceName(path: string, url?: string): string { + if (path) { + const parts = path.split('/') + return parts[parts.length - 1] || path + } + + if (url) { + try { + return new URL(url).hostname.replace(/\./g, '_') + } catch { + return 'mcp_server' + } + } + + return 'mcp_server' + } + + function getErrorMessage(error: any): string | undefined { + return error?.body?.message || error?.body || error?.message + } + + function isUnauthorizedError(error: any): boolean { + const status = error?.status + const message = String(getErrorMessage(error) ?? '') + + return ( + status === 401 || + status === 403 || + /\b(unauthorized|unauthorised|401|403)\b/i.test(message) + ) + } + let { tool = $bindable() }: Props = $props() - let showOAuthForm = $state(false) let refreshCount = $state(0) let resourcePicker: ResourcePicker | undefined = $state() + let lastResourcePath = $state() + let oauthDiscoveryStatus = $state('none') + + let resourcePath = $derived(tool.value.resource_path) let tools = usePromise( async () => await loadToolsCached({ workspace: $workspaceStore!, - path: tool.value.resource_path, + path: resourcePath, refreshCount }), - { loadInit: false, clearValueOnRefresh: false } + { loadInit: false } + ) + + let selectedResource = usePromise( + async () => + resourcePath && $workspaceStore + ? await ResourceService.getResource({ + workspace: $workspaceStore, + path: resourcePath + }) + : undefined, + { loadInit: false } ) let toolOptions = $derived(safeSelectItems((tools.value ?? []).map((t) => t.name))) - let resourcePath = $derived(tool.value.resource_path) - let error = $derived(tools.error?.body?.message || tools.error?.message) + let error = $derived(getErrorMessage(tools.error)) + let selectedMcpResourceValue = $derived(getMcpResourceValue(selectedResource.value?.value)) + let selectedMcpHasToken = $derived( + typeof selectedMcpResourceValue?.token === 'string' && + selectedMcpResourceValue.token.trim().length > 0 + ) + let selectedMcpUrlOnly = $derived(Boolean(selectedMcpResourceValue?.url && !selectedMcpHasToken)) + let selectedMcpServerUrl = $derived(selectedMcpResourceValue?.url) + let selectedMcpResourceName = $derived( + selectedMcpResourceValue?.name || + getFallbackMcpResourceName(resourcePath, selectedMcpServerUrl) + ) + let selectedMcpUnauthorized = $derived( + Boolean(selectedMcpUrlOnly && isUnauthorizedError(tools.error)) + ) + let connectionEstablished = $derived( + Boolean(resourcePath?.length > 0 && tools.status !== 'loading' && !tools.error && tools.value) + ) + let oauthDiscoveryPending = $derived( + selectedMcpUnauthorized && + (oauthDiscoveryStatus === 'none' || oauthDiscoveryStatus === 'discovering') + ) + let showConnecting = $derived( + Boolean( + resourcePath?.length > 0 && + !connectionEstablished && + (tools.status === 'loading' || selectedResource.status === 'loading' || oauthDiscoveryPending) + ) + ) + let showOAuthFlow = $derived( + selectedMcpUnauthorized && oauthDiscoveryStatus !== 'unsupported' + ) + let connectionError = $derived( + resourcePath?.length > 0 && + !connectionEstablished && + !showConnecting && + (!selectedMcpUnauthorized || oauthDiscoveryStatus === 'unsupported') + ? error + : undefined + ) $effect(() => { resourcePath @@ -65,6 +179,34 @@ }) }) + $effect(() => { + resourcePath + $workspaceStore + untrack(() => { + if (resourcePath?.length > 0 && $workspaceStore) { + selectedResource.refresh() + } else { + selectedResource.clear() + } + }) + }) + + $effect(() => { + const currentPath = resourcePath + untrack(() => { + if (currentPath !== lastResourcePath) { + lastResourcePath = currentPath + oauthDiscoveryStatus = 'none' + } + }) + }) + + $effect(() => { + if (!selectedMcpUnauthorized && oauthDiscoveryStatus !== 'none') { + oauthDiscoveryStatus = 'none' + } + }) + $effect(() => { if (!tool.value.include_tools) { tool.value.include_tools = [] @@ -80,11 +222,17 @@ } }) - async function handleOAuthConnected(resourcePath: string, resourceName: string) { + function handleOAuthDiscoveryStatus(status: Exclude) { + oauthDiscoveryStatus = status + } + + async function handleOAuthConnected(connectedResourcePath: string, resourceName: string) { await resourcePicker?.refreshResources() - tool.value.resource_path = resourcePath + tool.value.resource_path = connectedResourcePath tool.summary = `MCP: ${resourceName}` - showOAuthForm = false + oauthDiscoveryStatus = 'none' + selectedResource.refresh() + refreshCount += 1 } @@ -109,20 +257,29 @@
- {#if !resourcePath} - {#if !showOAuthForm} - - {:else} - (showOAuthForm = false)} - /> - {/if} + {#if showConnecting} +
connecting to mcp server...
{/if} - {#if resourcePath?.length > 0} + {#if showOAuthFlow} + {#key `${resourcePath ?? ''}:${selectedMcpServerUrl ?? ''}`} + + {/key} + {/if} + + {#if connectionError} +
{`Failed to connect to MCP server: ${connectionError}`}
+ {/if} + + {#if connectionEstablished}