diff --git a/packages/cli/src/__tests__/workflow-helpers.test.ts b/packages/cli/src/__tests__/workflow-helpers.test.ts index 2e29118e60f..eee9b79d409 100644 --- a/packages/cli/src/__tests__/workflow-helpers.test.ts +++ b/packages/cli/src/__tests__/workflow-helpers.test.ts @@ -6,6 +6,7 @@ import { GROUP_DESCRIPTION_MAX_LENGTH, STICKY_NODE_TYPE } from 'n8n-workflow'; import type { DynamicCredentialsUsage, ExecutionError, + INodeCredentialsDetails, IRun, ITaskData, IWorkflowBase, @@ -298,6 +299,70 @@ describe('replaceInvalidCredentials', () => { }); }); + it('should reuse a shared credential cache across workflows', async () => { + const credential = { id: 'cred-1', name: 'My Cred' } as CredentialsEntity; + credentialsRepository.findOneBy.mockResolvedValue(credential); + const cache = new Map(); + const firstWorkflow = makeWorkflow({ + httpHeaderAuth: { id: 'cred-1', name: 'My Cred' }, + }); + const secondWorkflow = makeWorkflow({ + httpHeaderAuth: { id: 'cred-1', name: 'My Cred' }, + }); + + await replaceInvalidCredentials(firstWorkflow, 'project-1', cache); + await replaceInvalidCredentials(secondWorkflow, 'project-1', cache); + + expect(credentialsRepository.findOneBy).toHaveBeenCalledTimes(1); + expect(firstWorkflow.nodes[0].credentials!.httpHeaderAuth).not.toBe( + secondWorkflow.nodes[0].credentials!.httpHeaderAuth, + ); + }); + + it('should cache a name fallback for the stale credential id and name', async () => { + const credential = { id: 'cred-new', name: 'My Cred' } as CredentialsEntity; + credentialsRepository.findOneBy.mockResolvedValue(null); + credentialsRepository.findByNameAndTypeInProject.mockResolvedValue([credential]); + const cache = new Map(); + + for (let index = 0; index < 2; index++) { + await replaceInvalidCredentials( + makeWorkflow({ httpHeaderAuth: { id: 'cred-stale', name: 'My Cred' } }), + 'project-1', + cache, + ); + } + + expect(credentialsRepository.findOneBy).toHaveBeenCalledTimes(1); + expect(credentialsRepository.findByNameAndTypeInProject).toHaveBeenCalledTimes(1); + }); + + it('should resolve the same stale credential id independently when names differ', async () => { + credentialsRepository.findOneBy.mockResolvedValue(null); + credentialsRepository.findByNameAndTypeInProject + .mockResolvedValueOnce([{ id: 'resolved-First', name: 'First' } as CredentialsEntity]) + .mockResolvedValueOnce([{ id: 'resolved-Second', name: 'Second' } as CredentialsEntity]); + const cache = new Map(); + const firstWorkflow = makeWorkflow({ + httpHeaderAuth: { id: 'cred-stale', name: 'First' }, + }); + const secondWorkflow = makeWorkflow({ + httpHeaderAuth: { id: 'cred-stale', name: 'Second' }, + }); + + await replaceInvalidCredentials(firstWorkflow, 'project-1', cache); + await replaceInvalidCredentials(secondWorkflow, 'project-1', cache); + + expect(firstWorkflow.nodes[0].credentials!.httpHeaderAuth).toEqual({ + id: 'resolved-First', + name: 'First', + }); + expect(secondWorkflow.nodes[0].credentials!.httpHeaderAuth).toEqual({ + id: 'resolved-Second', + name: 'Second', + }); + }); + it('should skip credential types that resolve to object internal keys', async () => { // JSON.parse keeps `__proto__` as an own enumerable key, unlike an object literal. const credentials = JSON.parse( diff --git a/packages/cli/src/credentials/credentials-finder.service.ts b/packages/cli/src/credentials/credentials-finder.service.ts index 6fac68eef7a..b26832a8f1f 100644 --- a/packages/cli/src/credentials/credentials-finder.service.ts +++ b/packages/cli/src/credentials/credentials-finder.service.ts @@ -1,5 +1,10 @@ import type { SharedCredentials, User } from '@n8n/db'; -import { CredentialsEntity, CredentialsRepository, SharedCredentialsRepository } from '@n8n/db'; +import { + CredentialsEntity, + CredentialsRepository, + chunkIds, + SharedCredentialsRepository, +} from '@n8n/db'; import { Service } from '@n8n/di'; import { hasGlobalScope } from '@n8n/permissions'; import type { CredentialSharingRole, ProjectRole, Scope } from '@n8n/permissions'; @@ -305,32 +310,40 @@ export class CredentialsFinderService { }; } - const sharedCredentials = await this.sharedCredentialsRepository.find({ - select: { credentialsId: true }, - where, - }); - - const result = new Set(sharedCredentials.map((sc) => sc.credentialsId)); + const result = new Set(); + for (const chunk of chunkIds(credentialIds)) { + const sharedCredentials = await this.sharedCredentialsRepository.find({ + select: { credentialsId: true }, + where: { ...where, credentialsId: In(chunk) }, + }); + for (const sharedCredential of sharedCredentials) { + result.add(sharedCredential.credentialsId); + } + } // Also include global credentials if scopes allow read-only access if (this.hasGlobalReadOnlyAccess(scopes)) { - const globalCreds = await this.credentialsRepository.find({ - where: { id: In(credentialIds), isGlobal: true, usageScope: 'project' }, - select: ['id'], - }); - for (const gc of globalCreds) result.add(gc.id); + for (const chunk of chunkIds(credentialIds)) { + const globalCreds = await this.credentialsRepository.find({ + where: { id: In(chunk), isGlobal: true, usageScope: 'project' }, + select: ['id'], + }); + for (const gc of globalCreds) result.add(gc.id); + } } else if (this.hasGlobalConnectAccess(scopes)) { // Only end-user (resolvable) global credentials grant connect access. - const globalCreds = await this.credentialsRepository.find({ - where: { - id: In(credentialIds), - isGlobal: true, - usageScope: 'project', - isResolvable: true, - }, - select: ['id'], - }); - for (const gc of globalCreds) result.add(gc.id); + for (const chunk of chunkIds(credentialIds)) { + const globalCreds = await this.credentialsRepository.find({ + where: { + id: In(chunk), + isGlobal: true, + usageScope: 'project', + isResolvable: true, + }, + select: ['id'], + }); + for (const gc of globalCreds) result.add(gc.id); + } } return result; diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-requirements.extractor.test.ts b/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-requirements.extractor.test.ts index f6c4cff230e..66926cf03c9 100644 --- a/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-requirements.extractor.test.ts +++ b/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-requirements.extractor.test.ts @@ -117,6 +117,43 @@ describe('CredentialRequirementsExtractor', () => { ]); }); + it('extracts credentials used by an inline workflow', () => { + const workflow = makeWorkflow({ + id: 'wf-inline', + nodes: [ + { + id: 'n1', + name: 'Execute Workflow', + type: 'n8n-nodes-base.executeWorkflow', + typeVersion: 1, + position: [0, 0], + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ + nodes: [ + { + credentials: { + httpHeaderAuth: { id: 'cred-inline', name: 'Inline credential' }, + }, + }, + ], + connections: {}, + }), + }, + }, + ], + }); + + expect(extractor.extract(workflow)).toEqual([ + { + workflowId: 'wf-inline', + credentialId: 'cred-inline', + credentialName: 'Inline credential', + credentialType: 'httpHeaderAuth', + }, + ]); + }); + it('skips slots that have no credential id selected yet', () => { const workflow = makeWorkflow({ id: 'wf-blank-slot', diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/credential-requirements.extractor.ts b/packages/cli/src/modules/n8n-packages/entities/credential/credential-requirements.extractor.ts index 378aa67e10f..95b8cfc0250 100644 --- a/packages/cli/src/modules/n8n-packages/entities/credential/credential-requirements.extractor.ts +++ b/packages/cli/src/modules/n8n-packages/entities/credential/credential-requirements.extractor.ts @@ -2,6 +2,7 @@ import type { WorkflowEntity } from '@n8n/db'; import { Service } from '@n8n/di'; import type { WorkflowCredentialRequirement } from './credential.types'; +import { visitWorkflowCredentials } from './workflow-credential-references'; import type { RequirementsExtractor } from '../requirements-extractor'; @Service() @@ -11,18 +12,17 @@ export class CredentialRequirementsExtractor extract(workflow: WorkflowEntity): WorkflowCredentialRequirement[] { const byId = new Map(); - for (const node of workflow.nodes ?? []) { - for (const [credentialType, details] of Object.entries(node.credentials ?? {})) { - if (!details?.id || byId.has(details.id)) continue; + visitWorkflowCredentials(workflow.nodes, (credentialType, details) => { + if (!details.id || byId.has(details.id)) return false; - byId.set(details.id, { - workflowId: workflow.id, - credentialId: details.id, - credentialName: details.name, - credentialType, - }); - } - } + byId.set(details.id, { + workflowId: workflow.id, + credentialId: details.id, + credentialName: details.name, + credentialType, + }); + return false; + }); return [...byId.values()]; } diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/workflow-credential-references.ts b/packages/cli/src/modules/n8n-packages/entities/credential/workflow-credential-references.ts new file mode 100644 index 00000000000..83c26eb69b6 --- /dev/null +++ b/packages/cli/src/modules/n8n-packages/entities/credential/workflow-credential-references.ts @@ -0,0 +1,49 @@ +import { + isNodeWithWorkflowSelector, + jsonParse, + type INode, + type INodeCredentialsDetails, + type IWorkflowBase, +} from 'n8n-workflow'; + +type CredentialVisitor = (credentialType: string, details: INodeCredentialsDetails) => boolean; + +/** + * Visits credentials in a workflow and its inline sub-workflows. The visitor + * returns whether it mutated a reference so nested workflow JSON can be updated. + */ +export function visitWorkflowCredentials( + nodes: INode[] | undefined, + visitor: CredentialVisitor, +): boolean { + if (!nodes) return false; + + let changed = false; + for (const node of nodes) { + for (const [credentialType, details] of Object.entries(node.credentials ?? {})) { + changed = visitor(credentialType, details) || changed; + } + + if (!isNodeWithWorkflowSelector(node)) continue; + + const workflowJson = node.parameters.workflowJson; + if (typeof workflowJson !== 'string') continue; + + let inlineWorkflow: Partial; + try { + inlineWorkflow = jsonParse>(workflowJson); + } catch { + continue; + } + if (!inlineWorkflow || !Array.isArray(inlineWorkflow.nodes)) continue; + + if (visitWorkflowCredentials(inlineWorkflow.nodes, visitor)) { + // Sub-workflow nodes are a parsed copy of a JSON string, not live refs, + // so visitor mutations are lost unless we serialize them back. + node.parameters.workflowJson = JSON.stringify(inlineWorkflow); + changed = true; + } + } + + return changed; +} diff --git a/packages/cli/src/modules/n8n-packages/entities/workflow/__tests__/workflow-importer.test.ts b/packages/cli/src/modules/n8n-packages/entities/workflow/__tests__/workflow-importer.test.ts new file mode 100644 index 00000000000..7d2bdb98051 --- /dev/null +++ b/packages/cli/src/modules/n8n-packages/entities/workflow/__tests__/workflow-importer.test.ts @@ -0,0 +1,226 @@ +import { WorkflowEntity, type User } from '@n8n/db'; +import { jsonParse } from 'n8n-workflow'; +import { mock } from 'vitest-mock-extended'; + +import type { + WorkflowCreateBatchContext, + WorkflowCreationService, +} from '@/workflows/workflow-creation.service'; +import type { WorkflowService } from '@/workflows/workflow.service'; + +import type { PackageImportBindings } from '../../../n8n-packages.types'; +import type { WorkflowImportMatchService } from '../workflow-import-match.service'; +import type { + WorkflowImportContext, + WorkflowImportPlan, + WorkflowPlanItem, +} from '../workflow-import.types'; +import { WorkflowImporter } from '../workflow-importer'; + +const makeWorkflow = (id: string): WorkflowEntity => + Object.assign(new WorkflowEntity(), { id, name: id, nodes: [], connections: {} }); + +const bindings: PackageImportBindings = { + credentials: new Map(), + workflows: new Map(), +}; + +const user = mock({ id: 'user-1' }); +const context = { + user, + projectId: 'project-1', + folderId: 'fallback-folder', + droppedTagIds: new Set(), +} as WorkflowImportContext; + +describe('WorkflowImporter.apply', () => { + it('prepares one batch context and passes it only to created workflows', async () => { + const prepareBatchContext = vi.fn(); + const createWorkflow = vi.fn(); + const updateWorkflow = vi.fn(); + const workflowCreationService = mock({ + prepareBatchContext, + createWorkflow, + }); + const workflowService = mock({ update: updateWorkflow }); + const importer = new WorkflowImporter( + mock(), + workflowCreationService, + workflowService, + ); + const batchContext = mock(); + const createEntity = makeWorkflow('source-create'); + const updateEntity = makeWorkflow('source-update'); + const existingUpdate = makeWorkflow('existing-update'); + const existingSkip = makeWorkflow('existing-skip'); + const created = makeWorkflow('created'); + const updated = makeWorkflow('updated'); + const items = [ + { + action: 'create', + sourceWorkflowId: 'source-create', + entity: createEntity, + decidedId: 'created-id', + parentFolderId: 'folder-1', + sourcePublished: false, + }, + { + action: 'update', + sourceWorkflowId: 'source-update', + entity: updateEntity, + existing: existingUpdate, + parentFolderId: null, + sourcePublished: false, + }, + { + action: 'skip', + sourceWorkflowId: 'source-skip', + entity: makeWorkflow('source-skip'), + existing: existingSkip, + parentFolderId: null, + sourcePublished: false, + }, + ] satisfies WorkflowPlanItem[]; + const plan: WorkflowImportPlan = { + items, + conflicts: [], + idConflicts: [], + folderConflicts: [], + }; + prepareBatchContext.mockResolvedValue(batchContext); + createWorkflow.mockResolvedValue(created); + updateWorkflow.mockResolvedValue(updated); + + await importer.apply(context, plan, bindings); + + expect(prepareBatchContext).toHaveBeenCalledExactlyOnceWith( + user, + 'project-1', + ['folder-1'], + [createEntity], + bindings.credentials, + ); + expect(createWorkflow).toHaveBeenCalledWith( + user, + expect.any(WorkflowEntity), + expect.objectContaining({ batchContext }), + ); + expect(updateWorkflow).toHaveBeenCalledWith( + user, + expect.any(WorkflowEntity), + 'existing-update', + { publicApi: true, source: 'import' }, + ); + }); + + it('does not prepare a batch context when no workflows are created', async () => { + const prepareBatchContext = vi.fn(); + const updateWorkflow = vi.fn(); + const workflowCreationService = mock({ prepareBatchContext }); + const workflowService = mock({ update: updateWorkflow }); + const importer = new WorkflowImporter( + mock(), + workflowCreationService, + workflowService, + ); + const existing = makeWorkflow('existing'); + const plan: WorkflowImportPlan = { + items: [ + { + action: 'update', + sourceWorkflowId: 'source', + entity: makeWorkflow('source'), + existing, + parentFolderId: null, + sourcePublished: false, + }, + ] satisfies WorkflowPlanItem[], + conflicts: [], + idConflicts: [], + folderConflicts: [], + }; + updateWorkflow.mockResolvedValue(existing); + + await importer.apply(context, plan, bindings); + + expect(prepareBatchContext).not.toHaveBeenCalled(); + }); + + it('applies credential bindings recursively to inline workflows', async () => { + const prepareBatchContext = vi.fn(); + const createWorkflow = vi.fn(); + const workflowCreationService = mock({ + prepareBatchContext, + createWorkflow, + }); + const importer = new WorkflowImporter( + mock(), + workflowCreationService, + mock(), + ); + const source = makeWorkflow('source'); + const deepestWorkflow = JSON.stringify({ + nodes: [ + { + credentials: { + httpHeaderAuth: { id: 'source-credential', name: 'Inline credential' }, + }, + }, + ], + connections: {}, + }); + source.nodes = [ + { + id: 'outer', + name: 'Outer', + type: 'n8n-nodes-base.executeWorkflow', + typeVersion: 1, + position: [0, 0], + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ + nodes: [ + { + type: 'n8n-nodes-base.executeWorkflow', + parameters: { source: 'parameter', workflowJson: deepestWorkflow }, + }, + ], + connections: {}, + }), + }, + }, + ]; + const plan: WorkflowImportPlan = { + items: [ + { + action: 'create', + sourceWorkflowId: 'source', + entity: source, + decidedId: 'created', + parentFolderId: null, + sourcePublished: false, + }, + ], + conflicts: [], + idConflicts: [], + folderConflicts: [], + }; + const batchContext = mock(); + prepareBatchContext.mockResolvedValue(batchContext); + createWorkflow.mockResolvedValue(makeWorkflow('created')); + + await importer.apply(context, plan, { + credentials: new Map([['source-credential', 'target-credential']]), + workflows: new Map(), + }); + + const persisted = createWorkflow.mock.calls[0][1]; + const outerWorkflow = jsonParse<{ + nodes: Array<{ parameters: { workflowJson: string } }>; + }>(persisted.nodes[0].parameters.workflowJson as string); + const innerWorkflow = jsonParse<{ + nodes: Array<{ credentials: { httpHeaderAuth: { id: string } } }>; + }>(outerWorkflow.nodes[0].parameters.workflowJson); + expect(innerWorkflow.nodes[0].credentials.httpHeaderAuth.id).toBe('target-credential'); + }); +}); diff --git a/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts b/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts index 4e69eb0a043..7a705c1ccda 100644 --- a/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts +++ b/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts @@ -1,7 +1,10 @@ import { WorkflowEntity } from '@n8n/db'; import { Service } from '@n8n/di'; -import { WorkflowCreationService } from '@/workflows/workflow-creation.service'; +import { + WorkflowCreationService, + type WorkflowCreateBatchContext, +} from '@/workflows/workflow-creation.service'; import { WorkflowService } from '@/workflows/workflow.service'; import { workflowReferences } from './references/workflow-references'; @@ -29,6 +32,7 @@ import type { PackageImportBindings, WorkflowIdPolicy, } from '../../n8n-packages.types'; +import { visitWorkflowCredentials } from '../credential/workflow-credential-references'; export interface WorkflowImportResult { outcomes: PersistedWorkflowOutcome[]; @@ -148,10 +152,24 @@ export class WorkflowImporter { ...collectPlannedWorkflowBindings(plan.items), ]); const resolvedBindings: PackageImportBindings = { ...bindings, workflows: workflowBindings }; + const createItems = plan.items.filter((item) => item.action === 'create'); + const batchContext = + createItems.length === 0 + ? undefined + : await this.workflowCreationService.prepareBatchContext( + context.user, + context.projectId, + createItems.flatMap((item) => { + const folderId = item.parentFolderId ?? context.folderId; + return folderId ? [folderId] : []; + }), + createItems.map(({ entity }) => entity), + resolvedBindings.credentials, + ); const outcomes: PersistedWorkflowOutcome[] = []; for (const item of plan.items) { - outcomes.push(await this.applyItem(context, item, resolvedBindings)); + outcomes.push(await this.applyItem(context, item, resolvedBindings, batchContext)); } return { outcomes, bindings: resolvedBindings }; @@ -161,6 +179,7 @@ export class WorkflowImporter { context: WorkflowImportContext, item: WorkflowPlanItem, bindings: PackageImportBindings, + batchContext: WorkflowCreateBatchContext | undefined, ): Promise { if (item.action === 'skip') { return { @@ -172,7 +191,7 @@ export class WorkflowImporter { return { status: item.action === 'create' ? 'created' : 'updated', - workflow: await this.persistWorkflow(context, item, bindings), + workflow: await this.persistWorkflow(context, item, bindings, batchContext), sourceWorkflowId: item.sourceWorkflowId, item, }; @@ -182,6 +201,7 @@ export class WorkflowImporter { context: WorkflowImportContext, item: PersistedWorkflowPlanItem, bindings: PackageImportBindings, + batchContext: WorkflowCreateBatchContext | undefined, ): Promise { const tagIds = item.tagIds && [...new Set(item.tagIds)].filter((id) => !context.droppedTagIds.has(id)); @@ -194,6 +214,7 @@ export class WorkflowImporter { publicApi: true, source: 'import', sourceWorkflowId: item.sourceWorkflowId, + ...(batchContext ? { batchContext } : {}), ...(tagIds !== undefined ? { tagIds } : {}), }); } @@ -236,16 +257,15 @@ function applyCredentialBindingsInPlace( entity: WorkflowEntity, credentialBindings: ImportBindingMap, ): void { - for (const node of entity.nodes) { - for (const details of Object.values(node.credentials ?? {})) { - if (!details.id) continue; + visitWorkflowCredentials(entity.nodes, (_credentialType, details) => { + if (!details.id) return false; - const targetId = credentialBindings.get(details.id); - if (targetId) { - details.id = targetId; - } - } - } + const targetId = credentialBindings.get(details.id); + if (!targetId || targetId === details.id) return false; + + details.id = targetId; + return true; + }); } function toPlanItem( diff --git a/packages/cli/src/workflow-helpers.ts b/packages/cli/src/workflow-helpers.ts index 7d4edda634b..d760a9b52ff 100644 --- a/packages/cli/src/workflow-helpers.ts +++ b/packages/cli/src/workflow-helpers.ts @@ -297,18 +297,21 @@ export function removeDefaultValues( return cleanedSettings; } +export type ReplaceInvalidCredentialsCache = Map; + +function credentialCacheKey(parts: readonly string[]): string { + return JSON.stringify(parts) ?? ''; +} + // Checking if credentials of old format are in use and run a DB check if they might exist uniquely export async function replaceInvalidCredentials( workflow: T, projectId: string, + cache: ReplaceInvalidCredentialsCache = new Map(), ): Promise { const { nodes } = workflow; if (!nodes) return workflow; - // caching - const credentialsByName: Record> = {}; - const credentialsById: Record> = {}; - // for loop to run DB fetches sequential and use cache to keep pressure off DB // trade-off: longer response time for less DB queries @@ -334,11 +337,9 @@ export async function replaceInvalidCredentials( // Check if Node applies old credentials style if (typeof nodeCredentials === 'string' || nodeCredentials.id === null) { const name = typeof nodeCredentials === 'string' ? nodeCredentials : nodeCredentials.name; - // init cache for type - if (!credentialsByName[nodeCredentialType]) { - credentialsByName[nodeCredentialType] = {}; - } - if (credentialsByName[nodeCredentialType][name] === undefined) { + const cacheKey = credentialCacheKey(['name', nodeCredentialType, name]); + const cachedCredential = cache.get(cacheKey); + if (cachedCredential === undefined) { const credentials = await Container.get(CredentialsRepository).findByNameAndTypeInProject( name, nodeCredentialType, @@ -346,47 +347,57 @@ export async function replaceInvalidCredentials( ); // if credential name-type combination is unique, use it if (credentials?.length === 1) { - credentialsByName[nodeCredentialType][name] = { + const resolvedCredential = { id: credentials[0].id, name: credentials[0].name, }; - node.credentials[nodeCredentialType] = credentialsByName[nodeCredentialType][name]; + cache.set(cacheKey, resolvedCredential); + node.credentials[nodeCredentialType] = { ...resolvedCredential }; continue; } // nothing found - add invalid credentials to cache to prevent further DB checks - credentialsByName[nodeCredentialType][name] = { + cache.set(cacheKey, { id: null, name, - }; + }); } else { // get credentials from cache - node.credentials[nodeCredentialType] = credentialsByName[nodeCredentialType][name]; + node.credentials[nodeCredentialType] = { ...cachedCredential }; } continue; } // Node has credentials with an ID - // init cache for type - if (!credentialsById[nodeCredentialType]) { - credentialsById[nodeCredentialType] = {}; + const idCacheKey = credentialCacheKey(['id', nodeCredentialType, nodeCredentials.id]); + const idAndNameCacheKey = credentialCacheKey([ + 'idAndName', + nodeCredentialType, + nodeCredentials.id, + nodeCredentials.name, + ]); + const cachedFallback = cache.get(idAndNameCacheKey); + if (cachedFallback !== undefined) { + node.credentials[nodeCredentialType] = { ...cachedFallback }; + continue; } // check if credentials for ID-type are not yet cached - if (credentialsById[nodeCredentialType][nodeCredentials.id] === undefined) { + const cachedById = cache.get(idCacheKey); + if (cachedById === undefined) { // check first if ID-type combination exists const credentials = await Container.get(CredentialsRepository).findOneBy({ id: nodeCredentials.id, type: nodeCredentialType, }); if (credentials) { - credentialsById[nodeCredentialType][nodeCredentials.id] = { + const resolvedCredential = { id: credentials.id, name: credentials.name, }; - node.credentials[nodeCredentialType] = - credentialsById[nodeCredentialType][nodeCredentials.id]; + cache.set(idCacheKey, resolvedCredential); + node.credentials[nodeCredentialType] = { ...resolvedCredential }; continue; } // no credentials found for ID, check if some exist for name @@ -398,23 +409,26 @@ export async function replaceInvalidCredentials( // if credential name-type combination is unique, take it if (credsByName?.length === 1) { // add found credential to cache - credentialsById[nodeCredentialType][credsByName[0].id] = { + const resolvedCredential = { id: credsByName[0].id, name: credsByName[0].name, }; - node.credentials[nodeCredentialType] = - credentialsById[nodeCredentialType][credsByName[0].id]; + cache.set(idAndNameCacheKey, resolvedCredential); + cache.set( + credentialCacheKey(['id', nodeCredentialType, credsByName[0].id]), + resolvedCredential, + ); + node.credentials[nodeCredentialType] = { ...resolvedCredential }; continue; } // nothing found - add invalid credentials to cache to prevent further DB checks - credentialsById[nodeCredentialType][nodeCredentials.id] = nodeCredentials; + cache.set(idAndNameCacheKey, { ...nodeCredentials }); continue; } // get credentials from cache - node.credentials[nodeCredentialType] = - credentialsById[nodeCredentialType][nodeCredentials.id]; + node.credentials[nodeCredentialType] = { ...cachedById }; } } diff --git a/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts b/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts index cd62adf3b04..bf828d38cb3 100644 --- a/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts @@ -1,10 +1,10 @@ import type { Logger, LicenseState } from '@n8n/backend-common'; -import type { ProjectRepository, Role, User } from '@n8n/db'; +import type { Folder, Project, ProjectRepository, Role, User } from '@n8n/db'; import { WorkflowEntity } from '@n8n/db'; import type { MockProxy } from 'vitest-mock-extended'; import { mock } from 'vitest-mock-extended'; -import type { CredentialsService } from '@/credentials/credentials.service'; +import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; @@ -15,6 +15,7 @@ import type { NodeTypes } from '@/node-types'; import { userHasScopes } from '@/permissions.ee/check-access'; import type { PolicyEnforcementService } from '@/policy/policy-enforcement.service'; import type { ProjectService } from '@/services/project.service.ee'; +import type { FolderService } from '@/services/folder.service'; import * as WorkflowHelpers from '@/workflow-helpers'; import type { WorkflowHookContextService } from '@/workflow-hook-context.service'; import { WorkflowCreationService } from '@/workflows/workflow-creation.service'; @@ -31,11 +32,12 @@ describe('WorkflowCreationService', () => { const userHasScopesMock = vi.mocked(userHasScopes); let workflowCreationService: WorkflowCreationService; - let credentialsServiceMock: MockProxy; + let credentialsFinderServiceMock: MockProxy; let enterpriseWorkflowServiceMock: MockProxy; let licenseStateMock: MockProxy; let projectServiceMock: MockProxy; let projectRepositoryMock: MockProxy; + let folderServiceMock: MockProxy; let workflowValidationServiceMock: MockProxy; let instanceRedactionEnforcementServiceMock: MockProxy; let workflowHistoryServiceMock: MockProxy; @@ -50,11 +52,12 @@ describe('WorkflowCreationService', () => { vi.clearAllMocks(); loggerMock = mock(); - credentialsServiceMock = mock(); + credentialsFinderServiceMock = mock(); enterpriseWorkflowServiceMock = mock(); licenseStateMock = mock(); projectServiceMock = mock(); projectRepositoryMock = mock(); + folderServiceMock = mock(); workflowValidationServiceMock = mock(); instanceRedactionEnforcementServiceMock = mock(); workflowHistoryServiceMock = mock(); @@ -64,6 +67,10 @@ describe('WorkflowCreationService', () => { workflowValidationServiceMock.validateCredentialNodeRestrictions.mockReturnValue({ isValid: true, }); + enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ + ids: new Set(), + hasUnresolved: false, + }); // Default: no active floor. Tests opt into a floor explicitly. instanceRedactionEnforcementServiceMock.get.mockResolvedValue('off'); @@ -88,8 +95,8 @@ describe('WorkflowCreationService', () => { licenseStateMock, projectRepositoryMock, mock(), // tagRepository - credentialsServiceMock, - mock(), // folderService + credentialsFinderServiceMock, + folderServiceMock, enterpriseWorkflowServiceMock, mock(), workflowValidationServiceMock, @@ -100,6 +107,43 @@ describe('WorkflowCreationService', () => { ); }); + describe('prepareBatchContext()', () => { + it('resolves import-wide reads once and folders by unique id', async () => { + const user = mock(); + const project = { id: 'project-1' } as Project; + const folder = { id: 'folder-1', homeProject: project } as Folder; + projectServiceMock.getProjectWithScope.mockResolvedValue(project); + folderServiceMock.getFoldersByIds.mockResolvedValue([folder]); + credentialsFinderServiceMock.findCredentialIdsWithScopeForUser.mockResolvedValue(new Set()); + mcpSettingsService.getAutoExposeNewWorkflows.mockResolvedValue(false); + enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ + ids: new Set(['source-credential']), + hasUnresolved: false, + }); + + const context = await workflowCreationService.prepareBatchContext( + user, + project.id, + ['folder-1', 'folder-1'], + [makeWorkflow(), makeWorkflow()], + new Map([['source-credential', 'target-credential']]), + ); + + expect(projectServiceMock.getProjectWithScope).toHaveBeenCalledTimes(1); + expect(folderServiceMock.getFoldersByIds).toHaveBeenCalledWith(['folder-1']); + expect(mcpSettingsService.getAutoExposeNewWorkflows).toHaveBeenCalledTimes(1); + expect(credentialsFinderServiceMock.findCredentialIdsWithScopeForUser).toHaveBeenCalledTimes( + 1, + ); + expect(credentialsFinderServiceMock.findCredentialIdsWithScopeForUser).toHaveBeenCalledWith( + [], + user, + ['credential:read'], + ); + expect(context.allowedCredentialIds).toEqual(new Set(['target-credential'])); + }); + }); + function makeWorkflow(overrides: Partial = {}): WorkflowEntity { const workflow = new WorkflowEntity(); workflow.name = 'Test'; @@ -193,11 +237,17 @@ describe('WorkflowCreationService', () => { }); describe('credential retrieval', () => { - it('should include global credentials when checking credential permissions', async () => { + it('should fetch only credential ids referenced by the workflow', async () => { /** * Arrange */ - credentialsServiceMock.getMany.mockResolvedValue([]); + enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ + ids: new Set(['credential-1']), + hasUnresolved: false, + }); + credentialsFinderServiceMock.findCredentialIdsWithScopeForUser.mockResolvedValue( + new Set(['credential-1']), + ); licenseStateMock.isSharingLicensed.mockReturnValue(true); enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser.mockImplementation(() => { throw new Error('Stopping for test'); @@ -217,9 +267,57 @@ describe('WorkflowCreationService', () => { /** * Assert */ - expect(credentialsServiceMock.getMany).toHaveBeenCalledWith(user, { - includeGlobal: true, + expect(credentialsFinderServiceMock.findCredentialIdsWithScopeForUser).toHaveBeenCalledWith( + ['credential-1'], + user, + ['credential:read'], + ); + }); + + it('should skip credential lookup when the workflow references none', async () => { + licenseStateMock.isSharingLicensed.mockReturnValue(true); + enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser.mockImplementation(() => { + throw new Error('Stopping for test'); }); + projectServiceMock.getProjectWithScope.mockResolvedValue({ id: 'project-1' } as never); + + await expect( + workflowCreationService.createWorkflow(mock(), new WorkflowEntity(), { + projectId: 'project-1', + }), + ).rejects.toThrow(); + + expect( + credentialsFinderServiceMock.findCredentialIdsWithScopeForUser, + ).not.toHaveBeenCalled(); + }); + + it('should reject unresolved credential references', async () => { + const user = mock(); + const newWorkflow = new WorkflowEntity(); + licenseStateMock.isSharingLicensed.mockReturnValue(true); + projectServiceMock.getProjectWithScope.mockResolvedValue({ id: 'project-1' } as never); + enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ + ids: new Set(['credential-1']), + hasUnresolved: true, + }); + credentialsFinderServiceMock.findCredentialIdsWithScopeForUser.mockResolvedValue( + new Set(['credential-1']), + ); + enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser.mockImplementation( + (_workflow, allowedCredentialIds) => { + expect(allowedCredentialIds).toEqual(new Set()); + throw new Error('Unresolved credential'); + }, + ); + + await expect( + workflowCreationService.createWorkflow(user, newWorkflow, { + projectId: 'project-1', + }), + ).rejects.toThrow( + 'The workflow you are trying to save contains credentials that are not shared with you', + ); }); }); @@ -227,7 +325,6 @@ describe('WorkflowCreationService', () => { /** * Arrange */ - credentialsServiceMock.getMany.mockResolvedValue([]); licenseStateMock.isSharingLicensed.mockReturnValue(true); enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser.mockImplementation(() => { throw new Error('User does not have access'); diff --git a/packages/cli/src/workflows/workflow-creation.service.ts b/packages/cli/src/workflows/workflow-creation.service.ts index d3e2432747a..9eafc3da9ce 100644 --- a/packages/cli/src/workflows/workflow-creation.service.ts +++ b/packages/cli/src/workflows/workflow-creation.service.ts @@ -13,7 +13,7 @@ import { Service } from '@n8n/di'; import { PROJECT_ROOT } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; -import { CredentialsService } from '@/credentials/credentials.service'; +import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { InternalServerError } from '@/errors/response-errors/internal-server.error'; @@ -41,6 +41,15 @@ import { WorkflowHistoryService } from './workflow-history/workflow-history.serv import { WorkflowValidationService } from './workflow-validation.service'; import { EnterpriseWorkflowService } from './workflow.service.ee'; +export interface WorkflowCreateBatchContext { + project: Project; + allowedCredentialIds: Set; + checkedCredentialIds: Set; + credentialResolutionCache: WorkflowHelpers.ReplaceInvalidCredentialsCache; + redactionFloor: RedactionFloor; + autoExposeNewWorkflows: boolean; +} + @Service() export class WorkflowCreationService { constructor( @@ -56,7 +65,7 @@ export class WorkflowCreationService { private readonly licenseState: LicenseState, private readonly projectRepository: ProjectRepository, private readonly tagRepository: TagRepository, - private readonly credentialsService: CredentialsService, + private readonly credentialsFinderService: CredentialsFinderService, private readonly folderService: FolderService, private readonly enterpriseWorkflowService: EnterpriseWorkflowService, private readonly nodeTypes: NodeTypes, @@ -67,6 +76,66 @@ export class WorkflowCreationService { private readonly policyEnforcementService: PolicyEnforcementService, ) {} + async prepareBatchContext( + user: User, + projectId: string, + parentFolderIds: string[], + workflows: WorkflowEntity[], + credentialBindings: ReadonlyMap, + ): Promise { + const project = await this.projectService.getProjectWithScope(user, projectId, [ + 'workflow:create', + ]); + if (!project) { + if (!(await this.projectRepository.exists({ where: { id: projectId } }))) { + throw new NotFoundError('Project not found'); + } + throw new ForbiddenError( + "You don't have the permissions to save the workflow in this project.", + ); + } + + const uniqueFolderIds = [...new Set(parentFolderIds)].filter((id) => id !== PROJECT_ROOT); + const folders = await this.folderService.getFoldersByIds(uniqueFolderIds); + const parentFolders = new Map( + folders + .filter((folder) => folder.homeProject.id === projectId) + .map((folder) => [folder.id, folder]), + ); + const missingFolderId = uniqueFolderIds.find((id) => !parentFolders.has(id)); + if (missingFolderId) { + throw new NotFoundError(`Could not find the folder: ${missingFolderId}`); + } + + const referencedCredentialIds = new Set(); + for (const workflow of workflows) { + const { ids } = this.enterpriseWorkflowService.collectCredentialReferences(workflow); + for (const id of ids) referencedCredentialIds.add(credentialBindings.get(id) ?? id); + } + const validatedCredentialIds = new Set(credentialBindings.values()); + const credentialIdsToCheck = [...referencedCredentialIds].filter( + (id) => !validatedCredentialIds.has(id), + ); + + const [redactionFloor, autoExposeNewWorkflows, accessibleCredentialIds] = await Promise.all([ + this.readActiveRedactionFloor(), + this.readAutoExposeNewWorkflows(), + this.credentialsFinderService.findCredentialIdsWithScopeForUser(credentialIdsToCheck, user, [ + 'credential:read', + ]), + ]); + const allowedCredentialIds = new Set([...validatedCredentialIds, ...accessibleCredentialIds]); + + return { + project, + allowedCredentialIds, + checkedCredentialIds: new Set([...validatedCredentialIds, ...referencedCredentialIds]), + credentialResolutionCache: new Map(), + redactionFloor, + autoExposeNewWorkflows, + }; + } + async createWorkflow( user: User, newWorkflow: WorkflowEntity, @@ -81,6 +150,7 @@ export class WorkflowCreationService { source?: WorkflowActionSource; versionName?: string; versionDescription?: string; + batchContext?: WorkflowCreateBatchContext; } = {}, ): Promise { const { @@ -94,6 +164,7 @@ export class WorkflowCreationService { source = 'ui', versionName, versionDescription, + batchContext, } = options; // Ensure workflow is created as inactive @@ -111,13 +182,15 @@ export class WorkflowCreationService { // Resolve target project and require workflow:create before credential checks const effectiveProjectId = - projectId ?? (await this.projectRepository.getPersonalProjectForUserOrFail(user.id)).id; + batchContext?.project.id ?? + projectId ?? + (await this.projectRepository.getPersonalProjectForUserOrFail(user.id)).id; - let project: Project | null = await this.projectService.getProjectWithScope( - user, - effectiveProjectId, - ['workflow:create'], - ); + let project: Project | null = + batchContext?.project ?? + (await this.projectService.getProjectWithScope(user, effectiveProjectId, [ + 'workflow:create', + ])); if (!project) { if (!(await this.projectRepository.exists({ where: { id: effectiveProjectId } }))) { throw new NotFoundError('Project not found'); @@ -129,7 +202,11 @@ export class WorkflowCreationService { throw new BadRequestError(message); } - await WorkflowHelpers.replaceInvalidCredentials(newWorkflow, effectiveProjectId); + await WorkflowHelpers.replaceInvalidCredentials( + newWorkflow, + effectiveProjectId, + batchContext?.credentialResolutionCache, + ); WorkflowHelpers.addNodeIds(newWorkflow); WorkflowHelpers.resolveNodeWebhookIds(newWorkflow, this.nodeTypes); @@ -140,7 +217,9 @@ export class WorkflowCreationService { ); if (parentFolderId && parentFolderId !== PROJECT_ROOT) { - await this.findParentFolderInProjectOrFail(parentFolderId, effectiveProjectId); + if (!batchContext) { + await this.findParentFolderInProjectOrFail(parentFolderId, effectiveProjectId); + } } if ('pinData' in newWorkflow) { @@ -150,15 +229,37 @@ export class WorkflowCreationService { if (this.licenseState.isSharingLicensed()) { // This is a new workflow, so we simply check if the user has access to // all used credentials - - const allCredentials = await this.credentialsService.getMany(user, { - includeGlobal: true, - }); + const { ids: credentialIds, hasUnresolved } = + this.enterpriseWorkflowService.collectCredentialReferences(newWorkflow); + if (batchContext) { + const uncheckedIds = [...credentialIds].filter( + (id) => !batchContext.checkedCredentialIds.has(id), + ); + if (uncheckedIds.length > 0) { + const accessibleIds = + await this.credentialsFinderService.findCredentialIdsWithScopeForUser( + uncheckedIds, + user, + ['credential:read'], + ); + for (const id of uncheckedIds) batchContext.checkedCredentialIds.add(id); + for (const id of accessibleIds) batchContext.allowedCredentialIds.add(id); + } + } + const accessibleCredentialIds = + batchContext?.allowedCredentialIds ?? + (credentialIds.size === 0 + ? new Set() + : await this.credentialsFinderService.findCredentialIdsWithScopeForUser( + [...credentialIds], + user, + ['credential:read'], + )); try { this.enterpriseWorkflowService.validateCredentialPermissionsToUser( newWorkflow, - allCredentials, + hasUnresolved ? new Set() : accessibleCredentialIds, ); } catch (error) { throw new BadRequestError( @@ -192,7 +293,7 @@ export class WorkflowCreationService { projectId: effectiveProjectId, }); - const floor = await this.readActiveRedactionFloor(); + const floor = batchContext?.redactionFloor ?? (await this.readActiveRedactionFloor()); const { manager: dbManager } = this.projectRepository; @@ -220,7 +321,11 @@ export class WorkflowCreationService { floor, ); - await this.resolveMcpExposureOnCreate(newWorkflow, transactionManager); + await this.resolveMcpExposureOnCreate( + newWorkflow, + transactionManager, + batchContext?.autoExposeNewWorkflows, + ); if (parentFolderId && parentFolderId !== PROJECT_ROOT) { newWorkflow.parentFolder = await this.findParentFolderInProjectOrFail( @@ -311,6 +416,17 @@ export class WorkflowCreationService { return await this.instanceRedactionEnforcementService.get(); } + private async readAutoExposeNewWorkflows(): Promise { + try { + return await this.mcpSettingsService.getAutoExposeNewWorkflows(); + } catch (error) { + this.logger.warn('Failed to resolve auto-expose setting for new workflow', { + cause: error instanceof Error ? error.message : String(error), + }); + return false; + } + } + private async resolveRedactionPolicyOnCreate( newWorkflow: WorkflowEntity, user: User, @@ -357,9 +473,16 @@ export class WorkflowCreationService { private async resolveMcpExposureOnCreate( newWorkflow: WorkflowEntity, transactionManager: EntityManager, + autoExposeNewWorkflows?: boolean, ): Promise { if (newWorkflow.settings?.availableInMCP !== undefined) return; + if (autoExposeNewWorkflows !== undefined) { + if (!autoExposeNewWorkflows) return; + newWorkflow.settings = { ...(newWorkflow.settings ?? {}), availableInMCP: true }; + return; + } + try { // Read through the create transaction's connection: a settings read on a // separate pool connection would deadlock small pools (the transaction diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index 1dfe9b7479c..88f2ba1ce31 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/packages/cli/src/workflows/workflow.service.ee.ts @@ -1,6 +1,5 @@ import { Logger } from '@n8n/backend-common'; import type { - CredentialsEntity, CredentialUsedByWorkflow, User, WorkflowEntity, @@ -154,12 +153,14 @@ export class EnterpriseWorkflowService { validateCredentialPermissionsToUser( workflow: IWorkflowBase, - allowedCredentials: CredentialsEntity[], + allowedCredentials: Array<{ id: string }> | ReadonlySet, ) { // Reuse the shared collector so inline sub-workflow credentials (Execute // Sub-workflow, Workflow Tool, Workflow Retriever) and unresolved name-only // references are inspected too, keeping this check aligned with the update path. - const allowedCredentialIds = allowedCredentials.map(({ id }) => id); + const allowedCredentialIds: ReadonlySet = Array.isArray(allowedCredentials) + ? new Set(allowedCredentials.map(({ id }) => id)) + : allowedCredentials; const inaccessibleNodes = this.getNodesWithInaccessibleCreds(workflow, allowedCredentialIds); if (inaccessibleNodes.length > 0) { throw new UserError('The workflow contains credentials that you do not have access to'); @@ -265,16 +266,33 @@ export class EnterpriseWorkflowService { * non-managed reference (id null/empty) that could resolve by name to a * credential the user does not own. Inline sub-workflow credentials are included. */ - getNodesWithInaccessibleCreds(workflow: IWorkflowBase, userCredIds: string[]) { + getNodesWithInaccessibleCreds(workflow: IWorkflowBase, userCredIds: Iterable) { if (!workflow.nodes) { return []; } + const allowedCredentialIds = userCredIds instanceof Set ? userCredIds : new Set(userCredIds); return workflow.nodes.filter((node) => { const { ids, hasUnresolved } = this.getNodeCredentialRefs(node); - return hasUnresolved || ids.some((credId) => !userCredIds.includes(credId)); + return hasUnresolved || ids.some((credId) => !allowedCredentialIds.has(credId)); }); } + collectCredentialReferences(workflow: IWorkflowBase): { + ids: Set; + hasUnresolved: boolean; + } { + const ids = new Set(); + let hasUnresolved = false; + + for (const node of workflow.nodes ?? []) { + const references = this.getNodeCredentialRefs(node); + for (const id of references.ids) ids.add(id); + hasUnresolved ||= references.hasUnresolved; + } + + return { ids, hasUnresolved }; + } + /** * Collect the credential references a node uses. Besides the node's own * `credentials`, a node with an inline workflow selector (Execute