From 4d9cd43168f1ad2e3708ea391efae40c6a3eaf4e Mon Sep 17 00:00:00 2001 From: Predrag Ristic Date: Tue, 15 Sep 2026 10:05:51 +0000 Subject: [PATCH] fix(editor): Restore MCP connection popup retries (#37956) --- .../useInstanceAiInputMenuItems.test.ts | 10 ++ .../components/InstanceAiMcpConnectCard.vue | 23 ++- .../InstanceAiMcpConnectCard.test.ts | 52 ++++-- .../InstanceAiToolsConnectionModalWrapper.vue | 28 ++- ...tanceAiToolsConnectionModalWrapper.test.ts | 82 ++++++++- .../useInstanceAiInputMenuItems.ts | 3 + .../composables/useMcpServerConnect.test.ts | 169 ++++++++++++++++-- .../composables/useMcpServerConnect.ts | 145 +++++++++++---- .../__tests__/useCredentialOAuth.test.ts | 112 +++++++++++- .../composables/useCredentialOAuth.ts | 135 +++++++++----- 10 files changed, 636 insertions(+), 123 deletions(-) diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/useInstanceAiInputMenuItems.test.ts b/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/useInstanceAiInputMenuItems.test.ts index 3d1a031fa43..f1a68241b41 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/useInstanceAiInputMenuItems.test.ts +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/useInstanceAiInputMenuItems.test.ts @@ -14,6 +14,7 @@ const { ensureBrowserConnected, computerUseTelemetry, featureFlags, + ignorePendingConnectResult, mcpStore, mcpTelemetry, settingsStore, @@ -23,6 +24,7 @@ const { ensureBrowserConnected: vi.fn(), computerUseTelemetry: { trackModalOpened: vi.fn() }, featureFlags: { browserUse: true, computerUse: true, mcp: true }, + ignorePendingConnectResult: vi.fn(), mcpStore: { connections: [] as Array>, fetchConnectionsLazy: vi.fn(), @@ -102,6 +104,10 @@ vi.mock('../instanceAiMcp.store', () => ({ useInstanceAiMcpStore: () => mcpStore, })); +vi.mock('../composables/useMcpServerConnect', () => ({ + useMcpServerConnect: () => ({ ignorePendingConnectResult }), +})); + vi.mock('../instanceAiMcp.telemetry', () => ({ useInstanceAiMcpTelemetry: () => mcpTelemetry, })); @@ -292,6 +298,10 @@ describe('useInstanceAiInputMenuItems', () => { data: { connectionId: '1' }, }); expect(mcpStore.disconnect).toHaveBeenCalledWith('1'); + expect(ignorePendingConnectResult).toHaveBeenCalledWith('server-1'); + expect(ignorePendingConnectResult.mock.invocationCallOrder[0]).toBeLessThan( + mcpStore.disconnect.mock.invocationCallOrder[0] ?? 0, + ); expect(settingsStore.disconnectComputerUse).toHaveBeenCalledOnce(); expect(ensureBrowserConnected).toHaveBeenCalledWith('input_menu'); }); diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/InstanceAiMcpConnectCard.vue b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/InstanceAiMcpConnectCard.vue index 7dd278f58d9..3636554ccc0 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/InstanceAiMcpConnectCard.vue +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/InstanceAiMcpConnectCard.vue @@ -9,7 +9,6 @@ import { useCredentialsStore } from '@/features/credentials/credentials.store'; import ToolCredentialPicker from '@/features/shared/toolsConnection/ToolCredentialPicker.vue'; import { TOOL_CONNECTION_CREDENTIAL_ADAPTER_KEY, - hasToolConnection, type McpServerConnectionItem, type ToolCredentialRef, } from '@/features/shared/toolsConnection/types'; @@ -35,7 +34,13 @@ const uiStore = useUIStore(); const credentialsStore = useCredentialsStore(); const mcpStore = useInstanceAiMcpStore(); const mcpTelemetry = useInstanceAiMcpTelemetry(); -const { connectServer, connectWithCredential, createCredentialAdapter } = useMcpServerConnect(); +const { + connectServer, + connectWithCredential, + createCredentialAdapter, + ignorePendingConnectResult, + isConnectLocked, +} = useMcpServerConnect(); const isConnecting = ref(false); @@ -80,7 +85,7 @@ const rows = computed(() => id: connection?.id ?? server.serverSlug, kind: 'mcp-server', title: entry?.title ?? server.title, - status: connection?.status ?? 'none', + status: isConnectLocked(server.serverSlug) ? 'connecting' : (connection?.status ?? 'none'), credentials: credentialOptions.map(({ credentialType, name }) => ({ authType: credentialType, displayName: name, @@ -98,7 +103,7 @@ const isActionable = computed(() => !props.readOnly && !props.expired); const anyConnected = computed(() => rows.value.some(hasConnection)); function hasConnection(row: CardRow): boolean { - return hasToolConnection(row.item.status); + return mcpStore.connections.some((connection) => connection.serverSlug === row.serverSlug); } function finish(approved: boolean) { @@ -140,18 +145,12 @@ async function runConnect(attempt: () => Promise) { } async function connect(row: CardRow, credentialType: string, credentialTypes?: readonly string[]) { - await runConnect( - async () => - await connectServer({ - slug: row.serverSlug, - credentialType, - credentialTypes, - }), - ); + await connectServer({ slug: row.serverSlug, credentialType, credentialTypes }); } async function handleSelectCredential(row: CardRow, credentialId: string) { await runConnect(async () => { + ignorePendingConnectResult(row.serverSlug); mcpTelemetry.trackExistingCredentialSelected(row.serverSlug); return await connectWithCredential(row.serverSlug, credentialId); }); diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/__tests__/InstanceAiMcpConnectCard.test.ts b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/__tests__/InstanceAiMcpConnectCard.test.ts index d09261f8283..22ff398966a 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/__tests__/InstanceAiMcpConnectCard.test.ts +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/__tests__/InstanceAiMcpConnectCard.test.ts @@ -23,6 +23,8 @@ const { uiStoreMock, connectServerMock, connectWithCredentialMock, + ignorePendingConnectResultMock, + isConnectLockedMock, credentialsMock, } = vi.hoisted(() => ({ telemetryMock: { @@ -42,6 +44,8 @@ const { }, connectServerMock: vi.fn(), connectWithCredentialMock: vi.fn(), + ignorePendingConnectResultMock: vi.fn(), + isConnectLockedMock: vi.fn(), credentialsMock: { credentials: [] as Array<{ id: string; name: string; type: string }>, }, @@ -66,6 +70,8 @@ vi.mock('../../composables/useMcpServerConnect', () => ({ useMcpServerConnect: () => ({ connectServer: connectServerMock, connectWithCredential: connectWithCredentialMock, + ignorePendingConnectResult: ignorePendingConnectResultMock, + isConnectLocked: isConnectLockedMock, createCredentialAdapter: ( openNewCredential: ToolConnectionCredentialAdapter['openNewCredential'], ) => ({ @@ -124,6 +130,7 @@ describe('InstanceAiMcpConnectCard', () => { beforeEach(() => { vi.clearAllMocks(); credentialsMock.credentials = []; + isConnectLockedMock.mockReturnValue(false); setActivePinia(createTestingPinia({ stubActions: false })); mcpStoreMock.mockReturnValue(makeMcpStore()); }); @@ -155,6 +162,7 @@ describe('InstanceAiMcpConnectCard', () => { await nextTick(); await fireEvent.click(getByTestId('tool-credential-picker-trigger-connect')); + await nextTick(); expect(telemetryMock.trackFirstCredentialConnectionStart).toHaveBeenCalledWith('brave'); expect(connectServerMock).toHaveBeenCalledWith({ @@ -241,6 +249,20 @@ describe('InstanceAiMcpConnectCard', () => { expect(emitted().resolve).toBeUndefined(); }); + it('shows an external connection lock as noninteractive', async () => { + isConnectLockedMock.mockReturnValue(true); + const { emitted, getByTestId, getByText, queryByTestId } = renderComponent({ + props: { servers: [BRAVE_PAYLOAD] }, + }); + + expect(getByTestId('tool-credential-picker-trigger-connecting')).toBeVisible(); + expect(queryByTestId('tool-credential-picker-trigger-connect')).toBeNull(); + await fireEvent.click(getByText('Brave Search')); + + expect(uiStoreMock.openModalWithData).not.toHaveBeenCalled(); + expect(emitted().resolve).toBeUndefined(); + }); + it('resolves as unapproved when skipped', async () => { const { getByTestId, emitted } = renderComponent({ props: { servers: [BRAVE_PAYLOAD] } }); @@ -249,8 +271,8 @@ describe('InstanceAiMcpConnectCard', () => { expect(emitted().resolve).toEqual([[{ approved: false, connectedSlugs: [] }]]); }); - it('continues as approved with what was connected when some rows are left', async () => { - mcpStoreMock.mockReturnValue(makeMcpStore({ connections: [BRAVE_CONNECTION] })); + it.each(['connected', 'connecting'])('continues with a %s connection', async (status) => { + mcpStoreMock.mockReturnValue(makeMcpStore({ connections: [{ ...BRAVE_CONNECTION, status }] })); const { getByTestId, emitted } = renderComponent({ props: { servers: [ @@ -403,7 +425,11 @@ describe('InstanceAiMcpConnectCard', () => { await findAllByTestId('tool-credential-picker-row'); await fireEvent.click(getByText('Brave key')); + expect(ignorePendingConnectResultMock).toHaveBeenCalledWith('brave'); expect(connectWithCredentialMock).toHaveBeenCalledWith('brave', 'cred-1'); + expect(ignorePendingConnectResultMock.mock.invocationCallOrder[0]).toBeLessThan( + connectWithCredentialMock.mock.invocationCallOrder[0] ?? 0, + ); expect(connectServerMock).not.toHaveBeenCalled(); }); @@ -435,7 +461,7 @@ describe('InstanceAiMcpConnectCard', () => { }); }); - it('keeps the footer disabled while a connect is still in flight', async () => { + it('allows another attempt after the server lock clears', async () => { mcpStoreMock.mockReturnValue( makeMcpStore({ catalog: [ @@ -455,9 +481,14 @@ describe('InstanceAiMcpConnectCard', () => { credentialsMock.credentials = [ { id: 'cred-1', name: 'Brave key', type: 'braveMcpOAuth2Api' }, ]; - connectServerMock.mockReturnValue(new Promise(() => {})); + const lockedSlugs = reactive(new Set()); + isConnectLockedMock.mockImplementation((slug: string) => lockedSlugs.has(slug)); + connectServerMock.mockImplementation(({ slug }: { slug: string }) => { + lockedSlugs.add(slug); + return new Promise(() => {}); + }); - const { getAllByTestId, getByTestId, getByText, findAllByTestId } = renderComponent({ + const { getAllByTestId } = renderComponent({ props: { servers: [ BRAVE_PAYLOAD, @@ -473,14 +504,15 @@ describe('InstanceAiMcpConnectCard', () => { }); await fireEvent.click(getAllByTestId('tool-credential-picker-trigger-connect')[1]); - expect(getByTestId('instance-ai-mcp-connect-resolve')).toBeDisabled(); + expect(getAllByTestId('tool-credential-picker-trigger-connect')).toHaveLength(1); + expect(getAllByTestId('tool-credential-picker-trigger-connecting')).toHaveLength(1); - await fireEvent.click(getAllByTestId('tool-credential-picker-trigger-connect')[0]); - await findAllByTestId('tool-credential-picker-row'); - await fireEvent.click(getByText('Brave key')); + lockedSlugs.clear(); + await nextTick(); + await fireEvent.click(getAllByTestId('tool-credential-picker-trigger-connect')[1]); + expect(connectServerMock).toHaveBeenCalledTimes(2); expect(connectWithCredentialMock).not.toHaveBeenCalled(); - expect(getByTestId('instance-ai-mcp-connect-resolve')).toBeDisabled(); }); it('opens the connection settings on row click', async () => { diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/InstanceAiToolsConnectionModalWrapper.vue b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/InstanceAiToolsConnectionModalWrapper.vue index db209850e8e..882b2ee8e89 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/InstanceAiToolsConnectionModalWrapper.vue +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/InstanceAiToolsConnectionModalWrapper.vue @@ -12,7 +12,6 @@ import McpToolSettingsContent from '@/features/shared/toolsConnection/McpToolSet import ToolsConnectionModal from '@/features/shared/toolsConnection/ToolsConnectionModal.vue'; import McpRegistrySuggestionFooter from '@/app/components/McpRegistrySuggestionFooter.vue'; import { - hasToolConnection, TOOL_CONNECTION_CREDENTIAL_ADAPTER_KEY, type McpServerConnectionItem, type McpServerTool, @@ -111,14 +110,21 @@ const detailItem = computed(() => { }); const detailMode = computed<'detail' | 'settings'>(() => - detailItem.value?.kind === 'mcp-server' && hasToolConnection(detailItem.value.status) + detailItem.value?.kind === 'mcp-server' && + mcpStore.connections.some((connection) => connection.id === activeItemId.value) ? 'settings' : 'detail', ); type McpToolMetadata = McpRegistryServerToolResponse | InstanceAiMcpConnectionToolResponse; -const { connectServer, connectWithCredential, createCredentialAdapter } = useMcpServerConnect(); +const { + connectServer, + connectWithCredential, + createCredentialAdapter, + ignorePendingConnectResult, + isConnectLocked, +} = useMcpServerConnect(); /** Reveals the settings view of the server the user just connected */ function showConnectedServer(connectionId: string | null): void { @@ -206,7 +212,7 @@ function buildItem( title: server.title, description: server.tagline, longDescription: server.description, - status: connection?.status ?? 'none', + status: isConnectLocked(server.slug) ? 'connecting' : (connection?.status ?? 'none'), iconSource: iconForTool(server.icons, uiStore.appliedTheme), credentials: server.credentials.map(({ credentialType, name }) => ({ authType: credentialType, @@ -323,10 +329,14 @@ provide( }), ); -function findServerForItem(item: McpServerConnectionItem): McpRegistryServerResponse | undefined { +function serverSlugForItem(item: McpServerConnectionItem): string { const connection = mcpStore.connections.find((c) => c.id === item.id); - const slug = connection?.serverSlug ?? item.id; - return mcpStore.catalog?.find((s) => s.slug === slug); + return connection?.serverSlug ?? item.id; +} + +function findServerForItem(item: McpServerConnectionItem): McpRegistryServerResponse | undefined { + const serverSlug = serverSlugForItem(item); + return mcpStore.catalog?.find((server) => server.slug === serverSlug); } function trackMcpCredentialInteraction( @@ -365,6 +375,7 @@ async function handleSelectCredential( if (item.kind !== 'mcp-server') return; const server = findServerForItem(item); if (!server) return; + ignorePendingConnectResult(server.slug); mcpTelemetry.trackExistingCredentialSelected(server.slug); showConnectedServer(await connectWithCredential(server.slug, credentialId)); } @@ -386,6 +397,9 @@ async function handleSave(item: ToolConnectionItem, settings?: ToolConnectionSet } async function handleDisconnect(item: ToolConnectionItem) { + if (item.kind === 'mcp-server') { + ignorePendingConnectResult(serverSlugForItem(item)); + } const disconnected = await mcpStore.disconnect(item.id); if (!disconnected) return; activeItemId.value = null; diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/__tests__/InstanceAiToolsConnectionModalWrapper.test.ts b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/__tests__/InstanceAiToolsConnectionModalWrapper.test.ts index d715328e6f3..6f214e0c006 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/__tests__/InstanceAiToolsConnectionModalWrapper.test.ts +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/components/modals/__tests__/InstanceAiToolsConnectionModalWrapper.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { flushPromises } from '@vue/test-utils'; -import { defineComponent, nextTick, reactive } from 'vue'; +import { fireEvent } from '@testing-library/vue'; +import { defineComponent, nextTick, reactive, ref } from 'vue'; import { createComponentRenderer } from '@/__tests__/render'; import { CREDENTIAL_EDIT_MODAL_KEY } from '@/features/credentials/credentials.constants'; import InstanceAiToolsConnectionModalWrapper from '../InstanceAiToolsConnectionModalWrapper.vue'; @@ -42,12 +43,24 @@ vi.mock('@/experiments/instanceAiBrowserUse', () => ({ }), })); -const { mockConnect, mockUpdateConnection, mcpStoreMock } = vi.hoisted(() => { +const { + mockConnect, + mockConnectServer, + mockConnectWithCredential, + mockIgnorePendingConnectResult, + mockUpdateConnection, + mockIsConnectLocked, + mcpStoreMock, +} = vi.hoisted(() => { const mockConnect = vi.fn(); const mockUpdateConnection = vi.fn(); return { mockConnect, + mockConnectServer: vi.fn(), + mockConnectWithCredential: vi.fn(), + mockIgnorePendingConnectResult: vi.fn(), mockUpdateConnection, + mockIsConnectLocked: vi.fn(), mcpStoreMock: { connections: [] as Array<{ id: string; @@ -89,8 +102,10 @@ vi.mock('../../../instanceAiMcp.store', () => ({ vi.mock('../../../composables/useMcpServerConnect', () => ({ useMcpServerConnect: () => ({ - connectServer: vi.fn().mockResolvedValue(null), - connectWithCredential: vi.fn().mockResolvedValue(null), + connectServer: mockConnectServer, + connectWithCredential: mockConnectWithCredential, + ignorePendingConnectResult: mockIgnorePendingConnectResult, + isConnectLocked: mockIsConnectLocked, createCredentialAdapter: ( openNewCredential: ToolConnectionCredentialAdapter['openNewCredential'], ) => ({ @@ -289,7 +304,10 @@ describe('InstanceAiToolsConnectionModalWrapper', () => { uiStoreMock.modalsById.instanceAiToolsConnection.data = {}; delete uiStoreMock.modalsById[CREDENTIAL_EDIT_MODAL_KEY]; mockConnect.mockResolvedValue(null); + mockConnectServer.mockResolvedValue(null); + mockConnectWithCredential.mockResolvedValue(null); mockUpdateConnection.mockResolvedValue({ serverSlug: 'linear' }); + mockIsConnectLocked.mockReturnValue(false); }); it('configures the suggestion footer copy', () => { @@ -358,6 +376,57 @@ describe('InstanceAiToolsConnectionModalWrapper', () => { expect(mcpStoreMock.fetchConnectionToolsLazy).toHaveBeenCalledWith('conn-1'); }); + it('keeps unsaved settings while the connection is locked', async () => { + const isLocked = ref(false); + mockIsConnectLocked.mockImplementation(() => isLocked.value); + const connection = { + id: 'conn-1', + serverSlug: 'linear', + credentialId: 'cred-1', + status: 'connected' as const, + toolFilter: null, + }; + mcpStoreMock.connections = [connection]; + mcpStoreMock.connectionsByServerSlug = new Map([['linear', [connection]]]); + uiStoreMock.modalsById.instanceAiToolsConnection.data = { connectionId: 'conn-1' }; + + const { getByTestId, getByText } = renderComponent({ + global: { + stubs: { + ToolsConnectionModal: false, + N8nDialog: { template: '
' }, + }, + }, + }); + await flushPromises(); + const inclusionInput = () => + getByTestId('tools-connection-settings-inclusion').querySelector('input')!; + await fireEvent.click(inclusionInput()); + await fireEvent.click(getByText('All Except')); + + isLocked.value = true; + await nextTick(); + + expect(getByTestId('tool-credential-picker-trigger-connecting')).toBeVisible(); + expect(inclusionInput()).toHaveValue('All Except'); + + isLocked.value = false; + await nextTick(); + + expect(getByTestId('tool-credential-picker-trigger-connected')).toBeVisible(); + expect(inclusionInput()).toHaveValue('All Except'); + }); + + it('keeps a new connection in the detail view while the connection is locked', () => { + mockIsConnectLocked.mockReturnValue(true); + uiStoreMock.modalsById.instanceAiToolsConnection.data = { connectionId: 'linear' }; + + renderComponent(); + + expect(modalProps.detailItem).toMatchObject({ id: 'linear', status: 'connecting' }); + expect(modalProps.detailMode).toBe('detail'); + }); + it('hides and restores the selected connection while editing a credential', async () => { uiStoreMock.modalsById.instanceAiToolsConnection.data = { connectionId: 'linear' }; renderComponent(); @@ -425,6 +494,11 @@ describe('InstanceAiToolsConnectionModalWrapper', () => { await flushPromises(); expect(telemetryMock.trackExistingCredentialSelected).toHaveBeenCalledWith('linear'); + expect(mockIgnorePendingConnectResult).toHaveBeenCalledWith('linear'); + expect(mockConnectWithCredential).toHaveBeenCalledWith('linear', 'cred-1'); + expect(mockIgnorePendingConnectResult.mock.invocationCallOrder[0]).toBeLessThan( + mockConnectWithCredential.mock.invocationCallOrder[0] ?? 0, + ); }); it('tracks new credential connection start', () => { diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useInstanceAiInputMenuItems.ts b/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useInstanceAiInputMenuItems.ts index 2fa38030737..9c7b111be8a 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useInstanceAiInputMenuItems.ts +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useInstanceAiInputMenuItems.ts @@ -15,6 +15,7 @@ import { useInstanceAiMcpTelemetry } from '../instanceAiMcp.telemetry'; import { useInstanceAiComputerUseTelemetry } from '../instanceAiComputerUse.telemetry'; import { useInstanceAiSettingsStore } from '../instanceAiSettings.store'; import { useBrowserUseConnection } from './useBrowserUseConnection'; +import { useMcpServerConnect } from './useMcpServerConnect'; import { iconForTool } from '../toolIcons'; type InputMenuItemData = { @@ -30,6 +31,7 @@ export function useInstanceAiInputMenuItems(attachFiles: () => void) { const uiStore = useUIStore(); const settingsStore = useInstanceAiSettingsStore(); const mcpStore = useInstanceAiMcpStore(); + const { ignorePendingConnectResult } = useMcpServerConnect(); const mcpTelemetry = useInstanceAiMcpTelemetry(); const { ensureConnected: ensureBrowserConnected } = useBrowserUseConnection(); const computerUseTelemetry = useInstanceAiComputerUseTelemetry(); @@ -195,6 +197,7 @@ export function useInstanceAiInputMenuItems(attachFiles: () => void) { divided: true, data: { action: async () => { + ignorePendingConnectResult(connection.serverSlug); await mcpStore.disconnect(connection.id); }, }, diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.test.ts b/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.test.ts index 27d395cf4f6..f012facadb6 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.test.ts +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.test.ts @@ -91,6 +91,7 @@ describe('useMcpServerConnect', () => { }); afterEach(async () => { + vi.useRealTimers(); await closeCredentialModal(); }); @@ -154,28 +155,157 @@ describe('useMcpServerConnect', () => { await expect(useMcpServerConnect().connectServer(linear)).resolves.toBe('conn-new'); - expect(mockCreateAndAuthorize).toHaveBeenCalledWith('linearMcpOAuth2Api'); + expect(mockCreateAndAuthorize).toHaveBeenCalledWith('linearMcpOAuth2Api', undefined, { + onAuthorizationStarted: expect.any(Function), + }); expect(uiStore.modalsById[CREDENTIAL_EDIT_MODAL_KEY].open).toBe(false); }); - it('shares one attempt across concurrent quick-connect callers', async () => { + it('reopens one OAuth attempt after the two-second lock', async () => { + vi.useFakeTimers(); + let finishAttempt: ((credential: { id: string }) => void) | undefined; + const reopen = vi.fn(); mockCanQuickConnect.mockReturnValue(true); - let authorize!: (credential: { id: string }) => void; + mockCreateAndAuthorize + .mockReturnValueOnce( + new Promise<{ id: string }>((resolve) => { + finishAttempt = resolve; + }), + ) + .mockResolvedValue(null); + const { connectServer, isConnectLocked } = useMcpServerConnect(); + const connecting = connectServer(linear); + mockCreateAndAuthorize.mock.calls[0]?.[2]?.onAuthorizationStarted(reopen); + + const concurrent = connectServer(linear); + expect(reopen).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(1999); + void connectServer(linear); + expect(reopen).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(1); + void connectServer(linear); + expect(reopen).toHaveBeenCalledOnce(); + expect(isConnectLocked(linear.slug)).toBe(true); + void connectServer(linear); + expect(reopen).toHaveBeenCalledOnce(); + await vi.advanceTimersByTimeAsync(2000); + void connectServer(linear); + expect(reopen).toHaveBeenCalledTimes(2); + expect(mockCreateAndAuthorize).toHaveBeenCalledOnce(); + + finishAttempt?.({ id: 'cred-new' }); + await expect(connecting).resolves.toBe('conn-new'); + await expect(concurrent).resolves.toBe('conn-new'); + expect(mcpStore.connect).toHaveBeenCalledOnce(); + expect(isConnectLocked(linear.slug)).toBe(false); + await connectServer(linear); + expect(mockCreateAndAuthorize).toHaveBeenCalledTimes(2); + }); + + it('ignores the OAuth result after another credential wins', async () => { + mockCanQuickConnect.mockReturnValue(true); + let finishAttempt: ((credential: { id: string }) => void) | undefined; mockCreateAndAuthorize.mockReturnValue( new Promise<{ id: string }>((resolve) => { - authorize = resolve; + finishAttempt = resolve; }), ); - const { connectServer } = useMcpServerConnect(); + const { connectServer, ignorePendingConnectResult } = useMcpServerConnect(); + const connecting = connectServer(linear); - const first = connectServer(linear); - const second = connectServer(linear); - authorize({ id: 'cred-new' }); + ignorePendingConnectResult(linear.slug); + finishAttempt?.({ id: 'cred-new' }); - await expect(first).resolves.toBe('conn-new'); - await expect(second).resolves.toBe('conn-new'); - expect(mockCreateAndAuthorize).toHaveBeenCalledTimes(1); - expect(mcpStore.connect).toHaveBeenCalledTimes(1); + await expect(connecting).resolves.toBeNull(); + expect(mcpStore.connect).not.toHaveBeenCalled(); + }); + + it('keeps the credential request locked when an ignored OAuth attempt settles', async () => { + let finishAuthorization: ((credential: { id: string }) => void) | undefined; + let finishConnection: ((connection: InstanceAiMcpConnection) => void) | undefined; + const reopen = vi.fn(); + mockCanQuickConnect.mockReturnValue(true); + mockCreateAndAuthorize.mockReturnValue( + new Promise<{ id: string }>((resolve) => { + finishAuthorization = resolve; + }), + ); + mcpStore.connect.mockReturnValue( + new Promise((resolve) => { + finishConnection = resolve; + }), + ); + const { connectServer, connectWithCredential, ignorePendingConnectResult, isConnectLocked } = + useMcpServerConnect(); + const oauthAttempt = connectServer(linear); + mockCreateAndAuthorize.mock.calls[0]?.[2]?.onAuthorizationStarted(reopen); + + ignorePendingConnectResult(linear.slug); + const credentialConnect = connectWithCredential(linear.slug, 'cred-existing'); + expect(isConnectLocked(linear.slug)).toBe(true); + const concurrent = connectServer(linear); + expect(reopen).not.toHaveBeenCalled(); + + finishAuthorization?.({ id: 'cred-oauth' }); + + await expect(oauthAttempt).resolves.toBeNull(); + await expect(concurrent).resolves.toBeNull(); + expect(isConnectLocked(linear.slug)).toBe(true); + expect(mcpStore.connect).toHaveBeenCalledOnce(); + + finishConnection?.(makeConnection({ id: 'conn-new', credentialId: 'cred-existing' })); + await expect(credentialConnect).resolves.toBe('conn-new'); + expect(isConnectLocked(linear.slug)).toBe(false); + }); + + it('resumes the ignored OAuth attempt instead of starting another one', async () => { + mockCanQuickConnect.mockReturnValue(true); + let finishAttempt: ((credential: { id: string }) => void) | undefined; + const reopen = vi.fn(); + mockCreateAndAuthorize.mockReturnValue( + new Promise<{ id: string }>((resolve) => { + finishAttempt = resolve; + }), + ); + const { connectServer, ignorePendingConnectResult } = useMcpServerConnect(); + const connecting = connectServer(linear); + mockCreateAndAuthorize.mock.calls[0]?.[2]?.onAuthorizationStarted(reopen); + + ignorePendingConnectResult(linear.slug); + const resumed = connectServer(linear); + expect(reopen).toHaveBeenCalledOnce(); + expect(mockCreateAndAuthorize).toHaveBeenCalledOnce(); + finishAttempt?.({ id: 'cred-new' }); + + await expect(connecting).resolves.toBe('conn-new'); + await expect(resumed).resolves.toBe('conn-new'); + expect(mcpStore.connect).toHaveBeenCalledOnce(); + }); + + it('keeps the server locked while the MCP connection request is pending', async () => { + vi.useFakeTimers(); + let finishConnection: ((connection: InstanceAiMcpConnection) => void) | undefined; + mockCanQuickConnect.mockReturnValue(true); + mockCreateAndAuthorize.mockResolvedValue({ id: 'cred-new' }); + mcpStore.connect.mockReturnValue( + new Promise((resolve) => { + finishConnection = resolve; + }), + ); + const { connectServer, isConnectLocked } = useMcpServerConnect(); + const connecting = connectServer(linear); + await flushPromises(); + + await vi.advanceTimersByTimeAsync(5000); + expect(isConnectLocked(linear.slug)).toBe(true); + const concurrent = connectServer(linear); + expect(mockCreateAndAuthorize).toHaveBeenCalledOnce(); + + finishConnection?.(makeConnection({ id: 'conn-new', credentialId: 'cred-new' })); + await expect(connecting).resolves.toBe('conn-new'); + await expect(concurrent).resolves.toBe('conn-new'); + expect(isConnectLocked(linear.slug)).toBe(false); }); it('stops when the user aborts the OAuth flow', async () => { @@ -367,6 +497,21 @@ describe('useMcpServerConnect', () => { await expect(second).resolves.toBe('conn-new'); }); + it('keeps sharing a manual credential attempt after two seconds', async () => { + vi.useFakeTimers(); + const { connectServer } = useMcpServerConnect(); + const first = connectServer(linear); + await vi.advanceTimersByTimeAsync(2000); + const second = connectServer(linear); + + expect(credentialCreatedListeners.size).toBe(1); + + emitCredentialCreated('cred-new'); + await closeCredentialModal(); + await expect(first).resolves.toBe('conn-new'); + await expect(second).resolves.toBe('conn-new'); + }); + it('starts a fresh attempt after the previous one settled', async () => { const { connectServer } = useMcpServerConnect(); const cancelled = connectServer(linear); diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.ts b/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.ts index fe444537bb4..faa7dcdf7d9 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.ts +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/composables/useMcpServerConnect.ts @@ -1,8 +1,9 @@ -import { effectScope } from 'vue'; +import { effectScope, shallowReactive } from 'vue'; import { camelCase } from 'change-case'; import type { INode } from 'n8n-workflow'; import { i18n } from '@n8n/i18n'; import { useToast } from '@n8n/composables/useToast'; +import { TIME } from '@/app/constants/durations'; import { listenForModalChanges, useUIStore } from '@/app/stores/ui.store'; import { listenForCredentialChanges, @@ -19,7 +20,21 @@ export interface McpConnectTarget { credentialTypes?: readonly string[]; } -const inFlightConnectsByServerSlug = new Map>(); +interface McpConnectAttemptState { + acceptCredential: boolean; + reopen: (() => void) | undefined; + unlockTimer: ReturnType | undefined; +} + +interface McpConnectAttempt { + promise: Promise; + state: McpConnectAttemptState; +} + +const QUICK_CONNECT_LOCK = 2 * TIME.SECOND; +const connectAttemptsByServerSlug = new Map(); +const oauthLockedServerSlugs = shallowReactive(new Set()); +const credentialRequestLockedServerSlugs = shallowReactive(new Set()); /** * The credential half of connecting an MCP server, shared by the tools @@ -41,27 +56,26 @@ export function useMcpServerConnect() { serverSlug: string, credentialId: string, ): Promise { - const existing = mcpStore.connections.find((c) => c.serverSlug === serverSlug); + credentialRequestLockedServerSlugs.add(serverSlug); + try { + const existing = mcpStore.connections.find((c) => c.serverSlug === serverSlug); + if (existing?.credentialId === credentialId) return null; + + const connection = existing + ? await mcpStore.updateConnection(existing.id, { credentialId }) + : await mcpStore.connect({ serverSlug, credentialId }); + if (!connection) return null; - if (!existing) { - const created = await mcpStore.connect({ serverSlug, credentialId }); - if (!created) return null; toast.showMessage({ type: 'success', - title: i18n.baseText('instanceAi.mcp.success.connect'), + title: i18n.baseText( + existing ? 'instanceAi.mcp.success.changeCredential' : 'instanceAi.mcp.success.connect', + ), }); - return created.id; + return connection.id; + } finally { + credentialRequestLockedServerSlugs.delete(serverSlug); } - - if (existing.credentialId === credentialId) return null; - - const updated = await mcpStore.updateConnection(existing.id, { credentialId }); - if (!updated) return null; - toast.showMessage({ - type: 'success', - title: i18n.baseText('instanceAi.mcp.success.changeCredential'), - }); - return updated.id; } /** @@ -70,25 +84,82 @@ export function useMcpServerConnect() { * modal. Resolves once the user is done, with null if they backed out. */ async function connectServer(server: McpConnectTarget): Promise { - const inFlight = inFlightConnectsByServerSlug.get(server.slug); - if (inFlight) return await inFlight; - - const attempt = startConnect(server); - inFlightConnectsByServerSlug.set(server.slug, attempt); - try { - return await attempt; - } finally { - inFlightConnectsByServerSlug.delete(server.slug); + const activeAttempt = connectAttemptsByServerSlug.get(server.slug); + if (activeAttempt) { + if (!isConnectLocked(server.slug) && activeAttempt.state.reopen) { + activeAttempt.state.acceptCredential = true; + activeAttempt.state.reopen(); + lockConnect(server.slug, activeAttempt); + } + return await activeAttempt.promise; } + + const hasOneOption = (server.credentialTypes?.length ?? 0) <= 1; + const isQuickConnect = hasOneOption && canOAuthCredentialQuickConnect(server.credentialType); + const state: McpConnectAttemptState = { + acceptCredential: true, + reopen: undefined, + unlockTimer: undefined, + }; + + const connecting = isQuickConnect + ? connectViaOAuth(server, state) + : connectViaCredentialModal(server); + const promise = connecting.finally(() => { + if (state.unlockTimer) clearTimeout(state.unlockTimer); + connectAttemptsByServerSlug.delete(server.slug); + oauthLockedServerSlugs.delete(server.slug); + }); + const attempt: McpConnectAttempt = { + promise, + state, + }; + connectAttemptsByServerSlug.set(server.slug, attempt); + + if (isQuickConnect) lockConnect(server.slug, attempt); + + return await promise; } - async function startConnect(server: McpConnectTarget): Promise { - const hasOneOption = (server.credentialTypes?.length ?? 0) <= 1; - if (hasOneOption && canOAuthCredentialQuickConnect(server.credentialType)) { - const credential = await createAndAuthorize(server.credentialType); - return credential ? await connectWithCredential(server.slug, credential.id) : null; + function lockConnect(serverSlug: string, attempt: McpConnectAttempt): void { + const { state } = attempt; + if (state.unlockTimer) clearTimeout(state.unlockTimer); + oauthLockedServerSlugs.add(serverSlug); + state.unlockTimer = setTimeout(() => { + oauthLockedServerSlugs.delete(serverSlug); + state.unlockTimer = undefined; + }, QUICK_CONNECT_LOCK); + } + + async function connectViaOAuth( + server: McpConnectTarget, + state: McpConnectAttemptState, + ): Promise { + const credential = await createAndAuthorize(server.credentialType, undefined, { + onAuthorizationStarted: (reopen) => { + state.reopen = reopen; + }, + }).finally(() => { + state.reopen = undefined; + }); + if (!credential || !state.acceptCredential) return null; + return await connectWithCredential(server.slug, credential.id); + } + + function isConnectLocked(serverSlug: string): boolean { + return ( + oauthLockedServerSlugs.has(serverSlug) || credentialRequestLockedServerSlugs.has(serverSlug) + ); + } + + function ignorePendingConnectResult(serverSlug: string): void { + const attempt = connectAttemptsByServerSlug.get(serverSlug); + if (attempt) { + attempt.state.acceptCredential = false; + if (attempt.state.unlockTimer) clearTimeout(attempt.state.unlockTimer); + attempt.state.unlockTimer = undefined; } - return await connectViaCredentialModal(server); + oauthLockedServerSlugs.delete(serverSlug); } function registryContextNode(server: McpConnectTarget): INode { @@ -215,5 +286,11 @@ export function useMcpServerConnect() { }; } - return { connectServer, connectWithCredential, createCredentialAdapter }; + return { + connectServer, + connectWithCredential, + createCredentialAdapter, + ignorePendingConnectResult, + isConnectLocked, + }; } diff --git a/packages/frontend/editor-ui/src/features/credentials/composables/__tests__/useCredentialOAuth.test.ts b/packages/frontend/editor-ui/src/features/credentials/composables/__tests__/useCredentialOAuth.test.ts index 73df77a4a54..451de87b503 100644 --- a/packages/frontend/editor-ui/src/features/credentials/composables/__tests__/useCredentialOAuth.test.ts +++ b/packages/frontend/editor-ui/src/features/credentials/composables/__tests__/useCredentialOAuth.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { createTestingPinia } from '@pinia/testing'; import { setActivePinia } from 'pinia'; import { useCredentialOAuth } from '../useCredentialOAuth'; +import { OAUTH_FLOW_TIMEOUT } from '../oauthCallback'; import { useCredentialsStore } from '../../credentials.store'; import { useRootStore } from '@n8n/stores/useRootStore'; import { mockedStore } from '@/__tests__/utils'; @@ -813,6 +814,7 @@ describe('useCredentialOAuth', () => { let mockPopup: { closed: boolean; close: ReturnType; + focus: ReturnType; location: { href: string }; }; @@ -847,7 +849,8 @@ describe('useCredentialOAuth', () => { beforeEach(() => { mockTrack.mockClear(); mockShowError.mockClear(); - mockPopup = { closed: false, close: vi.fn(), location: { href: '' } }; + mockShowMessage.mockClear(); + mockPopup = { closed: false, close: vi.fn(), focus: vi.fn(), location: { href: '' } }; MockBroadcastChannel.silent = false; mockedStore(useCredentialsStore).deleteCredential.mockResolvedValue(); vi.stubGlobal('BroadcastChannel', MockBroadcastChannel); @@ -1032,6 +1035,113 @@ describe('useCredentialOAuth', () => { expect(mockPopup.close).toHaveBeenCalled(); }); + it('should expose a handle that focuses the active popup', async () => { + setupSuccessfulOAuthFlow(); + let reopen: (() => void) | undefined; + const { createAndAuthorize } = useCredentialOAuth(); + + const promise = createAndAuthorize('slackOAuth2Api', undefined, { + onAuthorizationStarted: (resume) => { + reopen = resume; + }, + }); + + expect(reopen).toBeTypeOf('function'); + reopen?.(); + expect(window.open).toHaveBeenCalledOnce(); + expect(mockPopup.focus).toHaveBeenCalledOnce(); + await promise; + }); + + it('should navigate a reopened popup when the authorization URL arrives', async () => { + const credentialsStore = setupSuccessfulOAuthFlow(); + let resolveAuthorizationUrl: ((url: string) => void) | undefined; + credentialsStore.oAuth2Authorize.mockReturnValueOnce( + new Promise((resolve) => { + resolveAuthorizationUrl = resolve; + }), + ); + const reopenedPopup = { + closed: false, + close: vi.fn(), + focus: vi.fn(), + location: { href: '' }, + }; + vi.stubGlobal( + 'open', + vi.fn().mockReturnValueOnce(mockPopup).mockReturnValueOnce(reopenedPopup), + ); + let reopen: (() => void) | undefined; + const { createAndAuthorize } = useCredentialOAuth(); + const promise = createAndAuthorize('slackOAuth2Api', undefined, { + onAuthorizationStarted: (resume) => { + reopen = resume; + }, + }); + + await vi.waitFor(() => expect(credentialsStore.oAuth2Authorize).toHaveBeenCalledOnce()); + mockPopup.closed = true; + reopen?.(); + expect(window.open).toHaveBeenLastCalledWith( + 'about:blank', + 'OAuth Authorization', + expect.any(String), + ); + + resolveAuthorizationUrl?.('https://oauth.example.com/auth'); + await promise; + + expect(reopenedPopup.location.href).toBe('https://oauth.example.com/auth'); + expect(reopenedPopup.close).toHaveBeenCalledOnce(); + expect(credentialsStore.createNewCredential).toHaveBeenCalledOnce(); + expect(credentialsStore.oAuth2Authorize).toHaveBeenCalledOnce(); + }); + + it('should refresh authorization and the timeout when a popup is reopened', async () => { + const credentialsStore = setupSuccessfulOAuthFlow(); + MockBroadcastChannel.silent = true; + credentialsStore.oAuth2Authorize + .mockResolvedValueOnce('https://oauth.example.com/auth?state=first') + .mockResolvedValueOnce('https://oauth.example.com/auth?state=second'); + const reopenedPopup = { + closed: false, + close: vi.fn(), + focus: vi.fn(), + location: { href: '' }, + }; + vi.stubGlobal('open', vi.fn().mockReturnValueOnce(mockPopup).mockReturnValue(reopenedPopup)); + let reopen: (() => void) | undefined; + + vi.useFakeTimers(); + try { + const { createAndAuthorize } = useCredentialOAuth(); + const promise = createAndAuthorize('slackOAuth2Api', undefined, { + onAuthorizationStarted: (resume) => { + reopen = resume; + }, + }); + await vi.advanceTimersByTimeAsync(OAUTH_FLOW_TIMEOUT - 1000); + mockPopup.closed = true; + reopen?.(); + await vi.advanceTimersByTimeAsync(2000); + + expect(credentialsStore.oAuth2Authorize).toHaveBeenCalledTimes(2); + expect(reopenedPopup.location.href).toBe('https://oauth.example.com/auth?state=second'); + expect(reopenedPopup.close).not.toHaveBeenCalled(); + expect(credentialsStore.deleteCredential).not.toHaveBeenCalled(); + + window.dispatchEvent( + new MessageEvent('message', { origin: window.location.origin, data: 'success' }), + ); + await expect(promise).resolves.toEqual(createdCredential); + expect(credentialsStore.createNewCredential).toHaveBeenCalledOnce(); + expect(reopenedPopup.close).toHaveBeenCalledOnce(); + expect(credentialsStore.deleteCredential).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + it('should keep the credential when OAuth succeeds via backend verification (COOP)', async () => { const credentialsStore = setupSuccessfulOAuthFlow(); // No callback message reaches the editor; the popup reads as closed diff --git a/packages/frontend/editor-ui/src/features/credentials/composables/useCredentialOAuth.ts b/packages/frontend/editor-ui/src/features/credentials/composables/useCredentialOAuth.ts index 08bfab73794..1ad6d8b6a18 100644 --- a/packages/frontend/editor-ui/src/features/credentials/composables/useCredentialOAuth.ts +++ b/packages/frontend/editor-ui/src/features/credentials/composables/useCredentialOAuth.ts @@ -17,11 +17,25 @@ import type { ICredentialsResponse } from '../credentials.types'; import { useTelemetry } from '@n8n/composables/useTelemetry'; import { useWorkflowsStore } from '@/app/stores/workflows.store'; import { useRootStore } from '@n8n/stores/useRootStore'; -import { getTrustedOAuthOrigins, hasOAuthTokenData, waitForOAuthCallback } from './oauthCallback'; +import { + getTrustedOAuthOrigins, + hasOAuthTokenData, + waitForOAuthCallback, + type OAuthFlowOutcome, +} from './oauthCallback'; + +interface OAuthPopupState { + window: Window; + onReopen?: () => void; +} interface OAuthAuthorizationOptions { abortOnPopupClose?: boolean; - preopenedPopup?: Window; + popup?: OAuthPopupState; +} + +interface CreateAndAuthorizeOptions { + onAuthorizationStarted?: (reopen: () => void) => void; } /** @@ -231,7 +245,7 @@ export function useCredentialOAuth() { * Returns true if OAuth was successful, false if cancelled or failed. * * Must be called synchronously from the click handler (or be given a popup - * that was opened synchronously from it, see `preopenedPopup`). + * that was opened synchronously from it, see `options.popup`). */ async function authorize( credential: ICredentialsResponse, @@ -243,11 +257,12 @@ export function useCredentialOAuth() { // slow connections (Chrome expires activation after ~5s) and in stricter // browsers (Safari) regardless of timing. Open a blank window now and // navigate it once the authorization URL is known. - const popup = options.preopenedPopup ?? openOAuthPopup('about:blank', signal); - if (!popup) { + const popupWindow = options.popup?.window ?? openOAuthPopup('about:blank', signal); + if (!popupWindow) { showPopupBlockedError(); return false; } + const popup: OAuthPopupState = options.popup ?? { window: popupWindow }; // Token presence in credential data can only confirm the flow for fixed // credentials that had no token before the popup opened: a reconnect's old @@ -256,48 +271,56 @@ export function useCredentialOAuth() { // data, so presence never changes there. const canVerifyConnected = !credential.isResolvable && !(await isConnected(credential.id)); - const urlResult = await getOAuthAuthorizationUrl(credential); - if (!urlResult.ok) { - popup.close(); - if (urlResult.error === 'no-url') showOAuthUrlError(); - return false; - } + let outcome: OAuthFlowOutcome; + while (true) { + const urlResult = await getOAuthAuthorizationUrl(credential); + if (!urlResult.ok) { + popup.window.close(); + if (urlResult.error === 'no-url') showOAuthUrlError(); + return false; + } - if (!isValidHttpUrl(urlResult.result)) { - popup.close(); - showOAuthUrlError(); - return false; - } + if (!isValidHttpUrl(urlResult.result)) { + popup.window.close(); + showOAuthUrlError(); + return false; + } - popup.location.href = urlResult.result; + popup.window.location.href = urlResult.result; + const retryController = new AbortController(); + popup.onReopen = () => retryController.abort(); + try { + outcome = await waitForOAuthCallback({ + popup: popup.window, + trustedOrigins: getTrustedOAuthOrigins(rootStore.urlBaseEditor), + signal: signal + ? AbortSignal.any([signal, retryController.signal]) + : retryController.signal, + verifyConnected: canVerifyConnected + ? async () => await isConnected(credential.id) + : undefined, + abortOnPopupClose: options.abortOnPopupClose, + }); + } finally { + popup.onReopen = undefined; + } - let outcome = await waitForOAuthCallback({ - popup, - trustedOrigins: getTrustedOAuthOrigins(rootStore.urlBaseEditor), - signal, - verifyConnected: canVerifyConnected - ? async () => await isConnected(credential.id) - : undefined, - abortOnPopupClose: options.abortOnPopupClose, - }); + // The callback can finish while a timeout, retry, or cancellation starts. + if ( + (outcome === 'timeout' || outcome === 'aborted') && + canVerifyConnected && + (await isConnected(credential.id)) + ) { + outcome = 'success'; + } - // Timeout and abort can race the backend committing the token: authorization - // can legitimately take longer than the timeout, and cancellation is not - // always explicit user intent (NodeCredentials also cancels on unmount). - // Re-check before treating the flow as failed — a wrong failure deletes the - // credential in createAndAuthorize and would resurface "Credential not - // found" on the callback page. - if ( - (outcome === 'timeout' || outcome === 'aborted') && - canVerifyConnected && - (await isConnected(credential.id)) - ) { - outcome = 'success'; + // Reopening needs fresh server state and a fresh callback timeout. + if (!retryController.signal.aborted || signal?.aborted || outcome !== 'aborted') break; } // No-op when the opener relationship was severed by the provider's COOP // policy; the callback page closes itself in that case. - popup.close(); + popup.window.close(); if (outcome === 'success') { toast.showMessage({ @@ -358,6 +381,7 @@ export function useCredentialOAuth() { async function createAndAuthorize( credentialTypeName: string, nodeType?: string, + options: CreateAndAuthorizeOptions = {}, ): Promise { const credentialType = credentialsStore.getCredentialTypeByName(credentialTypeName); if (!credentialType) { @@ -369,12 +393,35 @@ export function useCredentialOAuth() { // Opened before the credential-creation round trips so it stays within // the click's transient user activation (see authorize). - const popup = openOAuthPopup('about:blank', controller.signal); - if (!popup) { + const initialPopup = openOAuthPopup('about:blank', controller.signal); + if (!initialPopup) { showPopupBlockedError(); oauthAbortController.value = null; return null; } + const popup: OAuthPopupState = { window: initialPopup }; + let authorizationFinished = false; + + options.onAuthorizationStarted?.(() => { + if (authorizationFinished || controller.signal.aborted) return; + try { + if (!popup.window.closed) { + popup.window.focus(); + return; + } + } catch { + // A provider can isolate the popup from its opener. + } + + const reopenedPopup = openOAuthPopup('about:blank', controller.signal); + if (!reopenedPopup) { + showPopupBlockedError(); + return; + } + popup.window = reopenedPopup; + popup.onReopen?.(); + reopenedPopup.focus(); + }); const data: ICredentialDataDecryptedObject = {}; const allowedHttpRequestDomainsProperty = credentialType.properties.find( @@ -408,7 +455,7 @@ export function useCredentialOAuth() { workflow_id: workflowsStore.workflowId, }); } catch (error) { - popup.close(); + popup.window.close(); oauthAbortController.value = null; toast.showError(error, i18n.baseText('nodeCredentials.showMessage.title')); return null; @@ -416,7 +463,9 @@ export function useCredentialOAuth() { pendingCredentialId.value = credential.id; - const success = await authorize(credential, controller.signal, { preopenedPopup: popup }); + const success = await authorize(credential, controller.signal, { popup }).finally(() => { + authorizationFinished = true; + }); oauthAbortController.value = null; pendingCredentialId.value = null;