From 8399317ccb1e84a34f21497fddb18a3b1ea93034 Mon Sep 17 00:00:00 2001 From: Savelii Date: Fri, 4 Sep 2026 10:15:44 +0000 Subject: [PATCH] fix(editor): Scope end-user credential trigger warning to reachable nodes (#37081) Co-authored-by: Claude Opus 4.8 --- .../app/composables/useNodeHelpers.test.ts | 130 ++++++++++++++++-- .../src/app/composables/useNodeHelpers.ts | 109 +++++++++++---- 2 files changed, 200 insertions(+), 39 deletions(-) diff --git a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts index 899a29c512a..52b79d3cfcc 100644 --- a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts +++ b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.test.ts @@ -7,7 +7,7 @@ import type { INodeTypeDescription, Workflow, } from 'n8n-workflow'; -import { NodeConnectionTypes, NodeHelpers } from 'n8n-workflow'; +import { NodeConnectionTypes, NodeHelpers, mapConnectionsByDestination } from 'n8n-workflow'; import { createTestingPinia } from '@pinia/testing'; import { useNodeHelpers } from '@/app/composables/useNodeHelpers'; import { createTestNode, createMockEnterpriseSettings } from '@/__tests__/mocks'; @@ -42,6 +42,8 @@ const mockDocumentStore = { usedCredentials: mockDocumentStoreUsedCredentials, allNodes: [] as INodeUi[], workflowTriggerNodes: [] as INodeUi[], + connectionsBySourceNode: {} as Record, + connectionsByDestinationNode: {} as Record, getNodeByName: vi.fn(), setNodeIssue: vi.fn(), updateNodeProperties: vi.fn(), @@ -866,14 +868,30 @@ describe('useNodeHelpers()', () => { ...overrides, }) as ICredentialsResponse; - const buildNotionNode = (): INodeUi => + const buildNotionNode = (name = 'Notion'): INodeUi => createTestNode({ + name, type: 'n8n-nodes-base.notion', credentials: { [NOTION_API]: { id: 'cred-123', name: 'My Notion' } }, }); const buildTriggerNode = (type: string, overrides: Partial = {}): INodeUi => - createTestNode({ type, ...overrides }); + createTestNode({ name: 'Trigger', type, ...overrides }); + + // Wires source nodes straight into `target` so the graph reflects reachability. + // Only the mixed-trigger path reads connections; single-trigger tests can skip this. + const wire = (target: INodeUi, ...sources: INodeUi[]) => { + const bySource: Record = {}; + for (const source of sources) { + bySource[source.name] = { + main: [[{ node: target.name, type: NodeConnectionTypes.Main, index: 0 }]], + }; + } + mockDocumentStore.connectionsBySourceNode = bySource; + mockDocumentStore.connectionsByDestinationNode = mapConnectionsByDestination( + bySource as never, + ); + }; beforeEach(() => { // Every type resolves to notionNodeType (which carries creds) — the @@ -885,6 +903,8 @@ describe('useNodeHelpers()', () => { afterEach(() => { mockDocumentStore.workflowTriggerNodes = []; + mockDocumentStore.connectionsBySourceNode = {}; + mockDocumentStore.connectionsByDestinationNode = {}; mockDocumentStore.settings = {}; mockedStore(useSettingsStore).settings.envFeatureFlags = {}; }); @@ -1349,23 +1369,111 @@ describe('useNodeHelpers()', () => { expect(result).toBeNull(); }); - it('warns when a compatible trigger is combined with an incompatible one', () => { - // Mirrors the backend: every enabled trigger must establish the n8n user - // identity, so a manual trigger cannot mask an incompatible trigger. + // IAM-1277: with a compatible trigger present, only nodes an incompatible + // trigger can actually reach are warned — the valid branch is left alone. + it('warns a node reachable from the incompatible trigger in a mixed-trigger workflow', () => { mockConnectedPrivateCred(true); - mockDocumentStore.workflowTriggerNodes = [ - buildTriggerNode(MANUAL_TRIGGER), - buildTriggerNode(WEBHOOK_TRIGGER), - ]; + const manual = buildTriggerNode(MANUAL_TRIGGER, { name: 'Manual' }); + const webhook = buildTriggerNode(WEBHOOK_TRIGGER, { name: 'Webhook' }); + const notion = buildNotionNode('Notion'); + mockDocumentStore.workflowTriggerNodes = [manual, webhook]; + // Notion sits on the incompatible (webhook) branch. + wire(notion, webhook); const { getNodeCredentialIssues } = useNodeHelpers(); - const result = getNodeCredentialIssues(buildNotionNode(), notionNodeType); + const result = getNodeCredentialIssues(notion, notionNodeType); expect(result?.credentials?.[NOTION_API]?.[0]).toContain( "End-user credentials aren't supported by this workflow's trigger", ); }); + it('does not warn a node reachable only from the compatible trigger in a mixed-trigger workflow', () => { + mockConnectedPrivateCred(true); + const manual = buildTriggerNode(MANUAL_TRIGGER, { name: 'Manual' }); + const webhook = buildTriggerNode(WEBHOOK_TRIGGER, { name: 'Webhook' }); + const notion = buildNotionNode('Notion'); + mockDocumentStore.workflowTriggerNodes = [manual, webhook]; + // Notion sits only on the compatible (manual) branch; the incompatible + // webhook trigger is a separate, disjoint chain. + wire(notion, manual); + + const { getNodeCredentialIssues } = useNodeHelpers(); + const result = getNodeCredentialIssues(notion, notionNodeType); + + expect(result).toBeNull(); + }); + + it('leaves a form-trigger branch alone when a separate polling trigger is incompatible', () => { + // The exact IAM-1277 scenario: a supported Form trigger plus a separate + // unsupported polling trigger. The node on the form branch must stay clean. + mockConnectedPrivateCred(true); + const form = buildTriggerNode(FORM_TRIGGER, { + name: 'Form', + parameters: { authentication: 'n8nUserAuth' }, + }); + const polling = buildTriggerNode('n8n-nodes-base.gmailTrigger', { name: 'Gmail Trigger' }); + const notion = buildNotionNode('Notion'); + mockDocumentStore.workflowTriggerNodes = [form, polling]; + wire(notion, form); + + const { getNodeCredentialIssues } = useNodeHelpers(); + const result = getNodeCredentialIssues(notion, notionNodeType); + + expect(result).toBeNull(); + }); + + it('warns a shared node reachable from both compatible and incompatible triggers', () => { + mockConnectedPrivateCred(true); + const manual = buildTriggerNode(MANUAL_TRIGGER, { name: 'Manual' }); + const webhook = buildTriggerNode(WEBHOOK_TRIGGER, { name: 'Webhook' }); + const notion = buildNotionNode('Notion'); + mockDocumentStore.workflowTriggerNodes = [manual, webhook]; + // Both branches converge on Notion — the incompatible run still reaches it. + wire(notion, manual, webhook); + + const { getNodeCredentialIssues } = useNodeHelpers(); + const result = getNodeCredentialIssues(notion, notionNodeType); + + expect(result?.credentials?.[NOTION_API]?.[0]).toContain( + "End-user credentials aren't supported by this workflow's trigger", + ); + }); + + it('does not warn a disjoint node that a reachable node serves as an ai_tool', () => { + // Webhook (incompatible) → Notion (main); Notion is ALSO the ai_tool SOURCE + // feeding DisjointAgent, which runs on its own branch. A forward walk must not + // cross Notion's outgoing ai_tool edge and pull DisjointAgent into the blocked + // set. (A plain 'ALL' walk would; the main-only walk here does not.) + mockConnectedPrivateCred(true); + const manual = buildTriggerNode(MANUAL_TRIGGER, { name: 'Manual' }); + const webhook = buildTriggerNode(WEBHOOK_TRIGGER, { name: 'Webhook' }); + const notion = buildNotionNode('Notion'); + const disjoint = buildNotionNode('DisjointAgent'); + mockDocumentStore.workflowTriggerNodes = [manual, webhook]; + const bySource = { + Webhook: { main: [[{ node: 'Notion', type: NodeConnectionTypes.Main, index: 0 }]] }, + Notion: { + [NodeConnectionTypes.AiTool]: [ + [{ node: 'DisjointAgent', type: NodeConnectionTypes.AiTool, index: 0 }], + ], + }, + }; + mockDocumentStore.connectionsBySourceNode = bySource; + mockDocumentStore.connectionsByDestinationNode = mapConnectionsByDestination( + bySource as never, + ); + + const { getNodeCredentialIssues } = useNodeHelpers(); + + // DisjointAgent is not reachable from the incompatible Webhook trigger. + expect(getNodeCredentialIssues(disjoint, notionNodeType)).toBeNull(); + // Sanity: Notion, on the webhook branch, is still warned. + expect( + getNodeCredentialIssues(notion, notionNodeType)?.credentials?.[NOTION_API]?.[0], + ).toContain("End-user credentials aren't supported by this workflow's trigger"); + }); + it('warns when no trigger in a multi-trigger workflow is compatible', () => { mockConnectedPrivateCred(true); mockDocumentStore.workflowTriggerNodes = [ diff --git a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts index 47e92161040..c1a2736d58d 100644 --- a/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts +++ b/packages/frontend/editor-ui/src/app/composables/useNodeHelpers.ts @@ -1,4 +1,4 @@ -import { ref } from 'vue'; +import { computed, ref } from 'vue'; import { SYSTEM_RESOLVER_ID } from '@n8n/api-types'; import { useHistoryStore } from '@/app/stores/history.store'; import { CUSTOM_API_CALL_KEY, EnterpriseEditionFeature } from '@/app/constants'; @@ -7,6 +7,8 @@ import { NodeHelpers, NodeConnectionTypes, classifyTriggerIdentity, + getChildNodes, + getParentNodes, nodeIssuesToString, } from 'n8n-workflow'; import type { @@ -417,17 +419,34 @@ export function useNodeHelpers() { return null; } - // Returns which resolver kind is in effect when a trigger blocks end-user - // credentials, or null when the workflow is compatible. Mirror the backend publish - // check: the effective resolver decides which identity every enabled trigger must - // establish, so a single incompatible trigger blocks publish even when a compatible - // one (e.g. a manual trigger) is also present. The system resolver (self-connect) - // keys on the n8n user identity; a custom resolver keys on an external identity - // extracted from the trigger data. + // Decides which nodes should carry the end-user-credential trigger warning, instead of + // poisoning every node whenever any trigger is incompatible. Returns: + // - null — nothing to warn (no triggers, or none is blocking) + // - { reachable: null } — warn every end-user-credential node (no compatible + // trigger exists, so no branch can resolve them) + // - { reachable: Set } — warn only nodes reachable from a blocking trigger + // (a compatible trigger also exists; its branch is fine) + // plus the resolver kind, which selects the message. // - // A workflow with no triggers is left un-warned: it's a transient state while - // building. The backend still catches it at publish time. - function getBlockingTrigger(): { isSystemResolver: boolean } | null { + // A trigger is "blocking" when it can't establish the identity the effective resolver + // keys on: the system resolver (self-connect) needs the n8n user identity, a custom + // resolver needs an external identity extracted from the trigger data. + // + // The reachable set is the forward main closure of each blocking trigger plus the AI + // sub-nodes (tool / model / memory) attached to any reachable node — a sub-node is the + // source of an `ai_*` connection into its parent, so a plain forward walk misses it. + // This mirrors execution: a node runs on a given trigger iff it's in that closure. + // + // A workflow with no triggers is left un-warned: it's a transient state while building. + // The backend still catches incompatible workflows at publish time. + // Computed so the graph walk runs once per reactive change rather than once per node: + // `collectPrivateCredentialIssues` reads it inside `updateNodesCredentialsIssues`, which + // loops over every node, so recomputing per call would be quadratic. Vue caches the result + // until the triggers / connections / resolver setting actually change. + const privateCredentialTriggerWarning = computed<{ + reachable: Set | null; + isSystemResolver: boolean; + } | null>(() => { const triggers = workflowDocumentStore.value.workflowTriggerNodes.filter( (trigger) => !trigger.disabled, ); @@ -436,17 +455,49 @@ export function useNodeHelpers() { const resolverId = workflowDocumentStore.value.settings?.credentialResolverId; const isSystemResolver = !resolverId || resolverId === SYSTEM_RESOLVER_ID; - const hasBlockingTrigger = triggers.some((trigger) => { + const isBlocking = (trigger: INodeUi) => { const { providesN8nIdentity, providesExternalIdentity } = classifyTriggerIdentity( trigger.type, trigger.parameters, { isChatOAuth2Enabled: envFeatureFlag.value('CHAT_TRIGGER_OAUTH2') }, ); return isSystemResolver ? !providesN8nIdentity : !providesExternalIdentity; - }); + }; - return hasBlockingTrigger ? { isSystemResolver } : null; - } + const blockingTriggers = triggers.filter(isBlocking); + if (blockingTriggers.length === 0) return null; + + // No compatible trigger anywhere: no branch can resolve end-user credentials, so + // every such node is blocked regardless of graph shape — warn them all. + if (blockingTriggers.length === triggers.length) { + return { reachable: null, isSystemResolver }; + } + + // Mixed triggers: only nodes an incompatible trigger can actually reach are blocked. + // Nodes sitting only on a compatible trigger's branch resolve fine at run time. + const bySource = workflowDocumentStore.value.connectionsBySourceNode; + const byDestination = workflowDocumentStore.value.connectionsByDestinationNode; + + const reachable = new Set(); + for (const trigger of blockingTriggers) { + reachable.add(trigger.name); + // Forward walk follows `main` only. A sub-node wired into a reachable node is the + // source of an `ai_*` connection, so walking 'ALL' here would also cross that edge + // backwards and pull in the sub-node's own (possibly unreachable) parent. The + // reverse ALL_NON_MAIN pass below is what attaches sub-nodes, correctly. + for (const child of getChildNodes(bySource, trigger.name, NodeConnectionTypes.Main)) { + reachable.add(child); + } + } + // Pull in sub-nodes of every reachable node (their connections point backwards). + for (const nodeName of [...reachable]) { + for (const subNode of getParentNodes(byDestination, nodeName, 'ALL_NON_MAIN')) { + reachable.add(subNode); + } + } + + return { reachable, isSystemResolver }; + }); function collectPrivateCredentialIssues( node: INodeUi, @@ -454,7 +505,12 @@ export function useNodeHelpers() { ): void { if (!isPrivateCredentialsEnabled.value) return; - const blockingTrigger = getBlockingTrigger(); + const warning = privateCredentialTriggerWarning.value; + if (!warning) return; + // With a compatible trigger present, warn only the nodes the incompatible trigger + // can reach; a node on the valid branch is left alone. `reachable === null` means + // no compatible trigger exists, so every end-user-credential node is warned. + if (warning.reachable !== null && !warning.reachable.has(node.name)) return; for (const [credTypeName, details] of Object.entries(node.credentials ?? {})) { if (foundIssues[credTypeName]?.length) continue; @@ -463,18 +519,15 @@ export function useNodeHelpers() { const credential = credentialsStore.getCredentialById(details.id); if (!credential?.isResolvable) continue; - // Mirror the backend publish check: trigger incompatibility blocks publish - // regardless of who connected the credential, so warn on it here too. A - // merely-not-yet-connected credential is surfaced via the callout/banner. - // The message depends on the resolver: the system resolver needs a trigger - // that establishes the n8n user identity, a custom resolver needs one that - // extracts an external identity. - if (blockingTrigger) { - const messageKey: BaseTextKey = blockingTrigger.isSystemResolver - ? 'nodeIssues.credentials.privateRequiresIdentityTriggerWithFormAndWebhook' - : 'nodeIssues.credentials.privateRequiresIdentityExtractor'; - foundIssues[credTypeName] = [i18n.baseText(messageKey)]; - } + // Trigger incompatibility blocks this node regardless of who connected the + // credential, so warn on it here too. A merely-not-yet-connected credential is + // surfaced via the callout/banner. The message depends on the resolver: the + // system resolver needs a trigger that establishes the n8n user identity, a + // custom resolver needs one that extracts an external identity. + const messageKey: BaseTextKey = warning.isSystemResolver + ? 'nodeIssues.credentials.privateRequiresIdentityTriggerWithFormAndWebhook' + : 'nodeIssues.credentials.privateRequiresIdentityExtractor'; + foundIssues[credTypeName] = [i18n.baseText(messageKey)]; } }