diff --git a/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.openapi.ts b/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.openapi.ts index 7423bef5aaf..f4a4c8655bd 100644 --- a/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.openapi.ts +++ b/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.openapi.ts @@ -78,6 +78,46 @@ export const credentialUnresolvedIssueOpenApi: ZodOpenAPIMetadata = { description: 'A credential reference that could not be resolved in the target project.', }; +export const credentialConflictFieldDocs = { + kind: { + description: + '`differs`: under `credentialConflictPolicy=fail`, the package would change the ' + + 'name or values of the destination credential. `divergent`: under `overwrite`, two or ' + + 'more package credentials with different names or values resolve to one destination ' + + 'credential. The import does not select one of them.', + }, + targetId: { description: 'Id of the destination credential.' }, + sourceIds: { + description: + 'Ids of the package credentials that resolve to the destination credential. For ' + + '`differs`, the package credentials that would change it.', + }, + name: { description: 'Name of the destination credential.' }, + usedBy: { description: 'Package consumers that use the listed package credentials.' }, +} as const satisfies Record; + +export const credentialConflictIssueOpenApi: ZodOpenAPIMetadata = { + description: + 'A matched credential that the credential conflict policy does not let the import ' + + 'change. Only a credential that the import can update is checked: a credential owned by ' + + 'a project of the import. Values are never reported.', +}; + +export const credentialUpdateForbiddenFieldDocs = { + targetId: { description: 'Id of the destination credential.' }, + sourceIds: { description: 'Ids of the package credentials that resolve to it.' }, + name: { description: 'Name of the destination credential.' }, + usedBy: { description: 'Package consumers that use the listed package credentials.' }, +} as const satisfies Record; + +export const credentialUpdateForbiddenIssueOpenApi: ZodOpenAPIMetadata = { + description: + 'A matched credential that the import can update, but the caller has no ' + + '`credential:update` permission on it. Only under `credentialConflictPolicy` `overwrite` ' + + 'or `fail`. The import does not read its values, so this blocks also when the package ' + + 'would not change it. Use `keep-existing` to bind it without a change.', +}; + export const projectConflictFieldDocs = { name: { description: "The project's name as it appears in the package." }, } as const satisfies Record; diff --git a/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.schema.ts b/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.schema.ts index 118bc982723..6bedefa2a39 100644 --- a/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.schema.ts +++ b/packages/@n8n/api-types/src/dto/packages/import-blocking-issue.schema.ts @@ -3,10 +3,14 @@ import '../../openapi-extend'; import { z } from 'zod'; import { + credentialConflictFieldDocs, + credentialConflictIssueOpenApi, credentialPolicyViolationFieldDocs, credentialPolicyViolationIssueOpenApi, credentialUnresolvedFieldDocs, credentialUnresolvedIssueOpenApi, + credentialUpdateForbiddenFieldDocs, + credentialUpdateForbiddenIssueOpenApi, dataTableSchemaOperationOpenApi, dataTableUnresolvedFieldDocs, dataTableUnresolvedIssueOpenApi, @@ -134,6 +138,29 @@ const credentialUnresolvedIssueSchema = z }) .openapi(credentialUnresolvedIssueOpenApi); +const credentialConflictIssueSchema = z + .object({ + type: z.literal('credential-conflict'), + kind: z.enum(['differs', 'divergent']).openapi(credentialConflictFieldDocs.kind), + targetId: z.string().openapi(credentialConflictFieldDocs.targetId), + sourceIds: z.array(z.string()).openapi(credentialConflictFieldDocs.sourceIds), + name: z.string().openapi(credentialConflictFieldDocs.name), + usedBy: z.array(packageRequirementConsumerSchema).openapi(credentialConflictFieldDocs.usedBy), + }) + .openapi(credentialConflictIssueOpenApi); + +const credentialUpdateForbiddenIssueSchema = z + .object({ + type: z.literal('credential-update-forbidden'), + targetId: z.string().openapi(credentialUpdateForbiddenFieldDocs.targetId), + sourceIds: z.array(z.string()).openapi(credentialUpdateForbiddenFieldDocs.sourceIds), + name: z.string().openapi(credentialUpdateForbiddenFieldDocs.name), + usedBy: z + .array(packageRequirementConsumerSchema) + .openapi(credentialUpdateForbiddenFieldDocs.usedBy), + }) + .openapi(credentialUpdateForbiddenIssueOpenApi); + const projectConflictIssueSchema = z .object({ type: z.literal('project-conflict'), @@ -376,6 +403,8 @@ export const importBlockingIssueSchema = z workflowFolderConflictIssueSchema, workflowArchiveForbiddenIssueSchema, credentialUnresolvedIssueSchema, + credentialConflictIssueSchema, + credentialUpdateForbiddenIssueSchema, projectConflictIssueSchema, folderConflictIssueSchema, workflowRemovalForbiddenIssueSchema, diff --git a/packages/@n8n/cli/src/__tests__/package-error.test.ts b/packages/@n8n/cli/src/__tests__/package-error.test.ts index 1ad31c04a37..0078e1c1386 100644 --- a/packages/@n8n/cli/src/__tests__/package-error.test.ts +++ b/packages/@n8n/cli/src/__tests__/package-error.test.ts @@ -260,6 +260,62 @@ describe('toPackagesError', () => { expect(hint).toContain('w2'); }); + it('lists credential-conflict issues for a 409, by kind', () => { + const result = toPackagesError( + new ApiError(409, 'Import blocked', undefined, { + issues: [ + { + type: 'credential-conflict', + kind: 'differs', + targetId: 'c1', + sourceIds: ['s1'], + name: 'GitHub', + usedBy: [{ kind: 'workflow', id: 'w1' }], + }, + { + type: 'credential-conflict', + kind: 'divergent', + targetId: 'c2', + sourceIds: ['s2', 's3'], + name: 'Slack', + usedBy: [ + { kind: 'workflow', id: 'w2' }, + { kind: 'agent', id: 'a1' }, + ], + }, + ], + }), + ); + + const hint = (result as ApiError).hint ?? ''; + expect(hint).toContain( + 'credential "GitHub" (c1) differs from package credential s1, used by workflow w1', + ); + expect(hint).toContain( + 'credential "Slack" (c2) receives different values from package credentials s2, s3, used by workflow w2, agent a1', + ); + }); + + it('lists credential-update-forbidden issues for a 422', () => { + const result = toPackagesError( + new ApiError(422, 'Import blocked', undefined, { + issues: [ + { + type: 'credential-update-forbidden', + targetId: 'c1', + sourceIds: ['s1'], + name: 'GitHub', + usedBy: [{ kind: 'workflow', id: 'w1' }], + }, + ], + }), + ); + + expect((result as ApiError).hint).toContain( + 'credential "GitHub" (c1) matches package credential s1, but you lack permission to update it. Use keep-existing or ask its owner. Used by workflow w1', + ); + }); + it('lists variable-limit-exceeded issues for a 422', () => { const result = toPackagesError( new ApiError(422, 'Import blocked', undefined, { diff --git a/packages/@n8n/cli/src/commands/package/package-error.ts b/packages/@n8n/cli/src/commands/package/package-error.ts index dd3f240cd51..f274b69bf8d 100644 --- a/packages/@n8n/cli/src/commands/package/package-error.ts +++ b/packages/@n8n/cli/src/commands/package/package-error.ts @@ -41,6 +41,21 @@ type BlockingIssue = usedBy: RequirementConsumer[]; violations: Array<{ message: string }>; } + | { + type: 'credential-conflict'; + kind: 'differs' | 'divergent'; + targetId: string; + sourceIds: string[]; + name: string; + usedBy: RequirementConsumer[]; + } + | { + type: 'credential-update-forbidden'; + targetId: string; + sourceIds: string[]; + name: string; + usedBy: RequirementConsumer[]; + } | { type: 'variable-unresolved'; name: string; usedBy: RequirementConsumer[] } | { type: 'variable-conflict'; name: string; projectId?: string; usedBy: RequirementConsumer[] } | { @@ -154,6 +169,19 @@ function formatIssue(issue: unknown): string { : ''; return `credential "${it.name ?? it.sourceId}" (${it.sourceId}, ${it.credentialType}) refused by policy: ${reasons}, used by ${usedBy}`; } + if (it.type === 'credential-conflict') { + const usedBy = formatConsumers(it.usedBy); + const sources = Array.isArray(it.sourceIds) ? it.sourceIds.join(', ') : ''; + if (it.kind === 'divergent') { + return `credential "${it.name}" (${it.targetId}) receives different values from package credentials ${sources}, used by ${usedBy}`; + } + return `credential "${it.name}" (${it.targetId}) differs from package credential ${sources}, used by ${usedBy}`; + } + if (it.type === 'credential-update-forbidden') { + const usedBy = formatConsumers(it.usedBy); + const sources = Array.isArray(it.sourceIds) ? it.sourceIds.join(', ') : ''; + return `credential "${it.name}" (${it.targetId}) matches package credential ${sources}, but you lack permission to update it. Use keep-existing or ask its owner. Used by ${usedBy}`; + } if (it.type === 'variable-unresolved') { const usedBy = formatConsumers(it.usedBy); return `variable "${it.name}" unresolved, used by ${usedBy}`; diff --git a/packages/cli/src/credentials/__tests__/credential-data-merge.test.ts b/packages/cli/src/credentials/__tests__/credential-data-merge.test.ts new file mode 100644 index 00000000000..d9f139dc6cc --- /dev/null +++ b/packages/cli/src/credentials/__tests__/credential-data-merge.test.ts @@ -0,0 +1,311 @@ +import type { ICredentialDataDecryptedObject } from 'n8n-workflow'; + +import { mergeCredentialData, type CredentialArrayMergeMode } from '../credential-data-merge'; + +const EXPRESSION = '={{ $vars.apiKey }}'; +const OTHER_EXPRESSION = '={{ $vars.otherKey }}'; + +describe('mergeCredentialData', () => { + describe.each(['merge-by-index', 'keep-existing'])( + 'with arrays "%s"', + (arrays) => { + it('replaces an existing plain value with an incoming expression', () => { + const result = mergeCredentialData({ + existing: { apiKey: 'secret' }, + incoming: { apiKey: EXPRESSION }, + arrays, + }); + + expect(result).toEqual({ apiKey: EXPRESSION }); + }); + + it('replaces an existing expression with an incoming expression', () => { + const result = mergeCredentialData({ + existing: { apiKey: OTHER_EXPRESSION }, + incoming: { apiKey: EXPRESSION }, + arrays, + }); + + expect(result).toEqual({ apiKey: EXPRESSION }); + }); + + it('keeps existing-only keys, including oauthTokenData', () => { + const oauthTokenData = { access_token: 'token', refresh_token: 'refresh' }; + + const result = mergeCredentialData({ + existing: { clientId: 'id', oauthTokenData }, + incoming: { clientSecret: EXPRESSION }, + arrays, + }); + + expect(result).toEqual({ clientId: 'id', clientSecret: EXPRESSION, oauthTokenData }); + }); + + it('keeps an existing-only key that shares a name with an inherited property', () => { + const result = mergeCredentialData({ + existing: { toString: 'secret', apiKey: 'key' }, + incoming: { apiKey: EXPRESSION }, + arrays, + }); + + expect(result).toEqual({ toString: 'secret', apiKey: EXPRESSION }); + }); + + it('does not read an inherited property as the existing value', () => { + const result = mergeCredentialData({ + existing: {}, + incoming: { toString: '' }, + arrays, + }); + + expect(result).toEqual({ toString: '' }); + }); + + it('adds an incoming-only key', () => { + const result = mergeCredentialData({ + existing: { apiKey: 'secret' }, + incoming: { domain: EXPRESSION }, + arrays, + }); + + expect(result).toEqual({ apiKey: 'secret', domain: EXPRESSION }); + }); + + it('merges nested objects key by key', () => { + const result = mergeCredentialData({ + existing: { auth: { user: 'admin', password: 'secret', realm: 'local' } }, + incoming: { auth: { user: EXPRESSION, password: '' } }, + arrays, + }); + + expect(result).toEqual({ auth: { user: EXPRESSION, password: 'secret', realm: 'local' } }); + }); + + it('replaces an existing non-object value with an incoming object', () => { + const result = mergeCredentialData({ + existing: { config: 'plain' }, + incoming: { config: { host: EXPRESSION } }, + arrays, + }); + + expect(result).toEqual({ config: { host: EXPRESSION } }); + }); + + it('keeps the existing value when the incoming value is an empty string', () => { + const result = mergeCredentialData({ + existing: { apiKey: 'secret' }, + incoming: { apiKey: '' }, + arrays, + }); + + expect(result).toEqual({ apiKey: 'secret' }); + }); + + it('keeps an empty string when the existing data has no value for the key', () => { + const result = mergeCredentialData({ + existing: {}, + incoming: { apiKey: '' }, + arrays, + }); + + expect(result).toEqual({ apiKey: '' }); + }); + + it('takes incoming numbers and booleans', () => { + const result = mergeCredentialData({ + existing: { port: 3000, ssl: false }, + incoming: { port: 5432, ssl: true }, + arrays, + }); + + expect(result).toEqual({ port: 5432, ssl: true }); + }); + + it('keeps the existing value when the incoming value is null', () => { + const result = mergeCredentialData({ + existing: { apiKey: 'secret' }, + incoming: { apiKey: null } as unknown as ICredentialDataDecryptedObject, + arrays, + }); + + expect(result).toEqual({ apiKey: 'secret' }); + }); + + it('does not mutate the inputs', () => { + const existing: ICredentialDataDecryptedObject = { + apiKey: 'secret', + auth: { user: 'admin' }, + headers: [{ name: 'X-Key', value: 'one' }], + }; + const incoming: ICredentialDataDecryptedObject = { + apiKey: EXPRESSION, + auth: { user: EXPRESSION }, + headers: [{ value: OTHER_EXPRESSION }], + }; + const existingBefore = structuredClone(existing); + const incomingBefore = structuredClone(incoming); + + mergeCredentialData({ existing, incoming, arrays }); + + expect(existing).toEqual(existingBefore); + expect(incoming).toEqual(incomingBefore); + }); + }, + ); + + describe('with arrays "merge-by-index"', () => { + const arrays = 'merge-by-index'; + + it('merges arrays of equal length by index', () => { + const result = mergeCredentialData({ + existing: { + headers: [ + { name: 'X-One', value: 'secret-one' }, + { name: 'X-Two', value: 'secret-two' }, + ], + }, + incoming: { headers: [{ name: '', value: EXPRESSION }, { value: '' }] }, + arrays, + }); + + expect(result).toEqual({ + headers: [ + { name: 'X-One', value: EXPRESSION }, + { name: 'X-Two', value: 'secret-two' }, + ], + }); + }); + + it('replaces an array of different length with the incoming array', () => { + const incomingHeaders = [{ value: EXPRESSION }]; + + const result = mergeCredentialData({ + existing: { + headers: [ + { name: 'X-One', value: 'secret-one' }, + { name: 'X-Two', value: 'secret-two' }, + ], + }, + incoming: { headers: incomingHeaders }, + arrays, + }); + + expect(result).toEqual({ headers: incomingHeaders }); + }); + + it('adds an incoming array when the existing data has none', () => { + const result = mergeCredentialData({ + existing: {}, + incoming: { scopes: [EXPRESSION] }, + arrays, + }); + + expect(result).toEqual({ scopes: [EXPRESSION] }); + }); + + it('applies the mode to arrays nested in objects inside arrays', () => { + const result = mergeCredentialData({ + existing: { groups: [{ tags: ['a', 'b'] }] }, + incoming: { groups: [{ tags: [EXPRESSION, ''] }] }, + arrays, + }); + + expect(result).toEqual({ groups: [{ tags: [EXPRESSION, 'b'] }] }); + }); + + it('adds a new incoming object that merges to no fields', () => { + const result = mergeCredentialData({ + existing: {}, + incoming: { config: { host: null } }, + arrays, + }); + + expect(result).toEqual({ config: {} }); + }); + }); + + describe('with arrays "keep-existing"', () => { + const arrays = 'keep-existing'; + + it('keeps an existing array of equal length as it is', () => { + const existingHeaders = [ + { name: 'X-One', value: 'secret-one' }, + { name: 'X-Two', value: 'secret-two' }, + ]; + + const result = mergeCredentialData({ + existing: { headers: existingHeaders }, + incoming: { headers: [{ value: EXPRESSION }, { value: OTHER_EXPRESSION }] }, + arrays, + }); + + expect(result).toEqual({ headers: existingHeaders }); + }); + + it('keeps an existing array of different length as it is', () => { + const existingHeaders = [ + { name: 'X-One', value: 'secret-one' }, + { name: 'X-Two', value: 'secret-two' }, + ]; + + const result = mergeCredentialData({ + existing: { headers: existingHeaders }, + incoming: { headers: [{ value: EXPRESSION }] }, + arrays, + }); + + expect(result).toEqual({ headers: existingHeaders }); + }); + + it('does not add an incoming array when the existing data has no value for the key', () => { + const result = mergeCredentialData({ + existing: { apiKey: 'secret' }, + incoming: { headers: [{ value: EXPRESSION }] }, + arrays, + }); + + expect(result).toEqual({ apiKey: 'secret' }); + expect(result).not.toHaveProperty('headers'); + }); + + it('keeps an array nested in an object', () => { + const result = mergeCredentialData({ + existing: { config: { host: 'db.local', hosts: ['a', 'b'] } }, + incoming: { config: { host: EXPRESSION, hosts: [EXPRESSION] } }, + arrays, + }); + + expect(result).toEqual({ config: { host: EXPRESSION, hosts: ['a', 'b'] } }); + }); + + it('does not add an array nested in a new incoming object', () => { + const result = mergeCredentialData({ + existing: {}, + incoming: { config: { host: EXPRESSION, hosts: [EXPRESSION] } }, + arrays, + }); + + expect(result).toEqual({ config: { host: EXPRESSION } }); + }); + + it('does not add a new incoming object that holds only arrays', () => { + const result = mergeCredentialData({ + existing: { apiKey: 'secret' }, + incoming: { config: { headers: [EXPRESSION], nested: { hosts: [EXPRESSION] } } }, + arrays, + }); + + expect(result).toEqual({ apiKey: 'secret' }); + }); + + it('keeps an existing object when the incoming object holds only arrays', () => { + const result = mergeCredentialData({ + existing: { config: {} }, + incoming: { config: { headers: [EXPRESSION] } }, + arrays, + }); + + expect(result).toEqual({ config: {} }); + }); + }); +}); diff --git a/packages/cli/src/credentials/credential-data-merge.ts b/packages/cli/src/credentials/credential-data-merge.ts new file mode 100644 index 00000000000..8021a0cd5af --- /dev/null +++ b/packages/cli/src/credentials/credential-data-merge.ts @@ -0,0 +1,113 @@ +import { isRecord } from '@n8n/utils/is-record'; +import type { ICredentialDataDecryptedObject, IDataObject } from 'n8n-workflow'; + +import { containsExpression } from '@/utils'; + +/** + * How to merge an incoming array into the existing value. + * - `merge-by-index`: merge item by item when the lengths match, otherwise use the incoming array. + * - `keep-existing`: keep the existing value. Use this when the incoming array is lossy. + */ +export type CredentialArrayMergeMode = 'merge-by-index' | 'keep-existing'; + +function mergeValue( + incoming: unknown, + existing: unknown, + arrays: CredentialArrayMergeMode, +): unknown { + if (typeof incoming === 'string') { + if (containsExpression(incoming)) { + return incoming; + } else if (existing !== undefined && existing !== null) { + // Existing value is preserved if it exists (secret handling) + return existing; + } + + // The incoming field exists as an empty string (key is part of the schema) + // but existing has no value for it. Preserve the empty string so the field + // is not silently dropped from the merged credential. + return ''; + } + + if (typeof incoming === 'number' || typeof incoming === 'boolean') { + return incoming; + } + + if (Array.isArray(incoming)) { + if (arrays === 'keep-existing') return existing; + + // Only merge by index if lengths match, otherwise array structure has changed + // and we can't reliably match items (could be additions/removals/reordering) + if (Array.isArray(existing) && existing.length === incoming.length) { + return incoming.map((item: unknown, index) => mergeValue(item, existing[index], arrays)); + } + + return incoming; + } + + if (isRecord(incoming)) { + // Merge into an empty object when there is nothing to merge into, so the + // array mode also applies to arrays nested in a new object. + const merged = mergeObjects(incoming, isRecord(existing) ? existing : {}, arrays); + + // Only kept arrays were in the new object. Do not add an empty object for them, + // so that a merge which changes nothing gives the existing data back. + if (arrays === 'keep-existing' && !isRecord(existing) && Object.keys(merged).length === 0) { + return existing; + } + + return merged; + } + + // `null` or another value that carries no data: keep what exists. + return existing; +} + +function mergeObjects( + incoming: Record, + existing: Record, + arrays: CredentialArrayMergeMode, +): Record { + const merged: Record = {}; + + for (const [key, incomingValue] of Object.entries(incoming)) { + // Own keys only, so a name such as `toString` does not resolve to an inherited value. + const existingValue = Object.hasOwn(existing, key) ? existing[key] : undefined; + const mergedValue = mergeValue(incomingValue, existingValue, arrays); + + if (mergedValue !== undefined) { + merged[key] = mergedValue; + } + } + + // Keep existing fields the incoming data does not carry. A field left at its default + // value is not persisted, so it never reaches the incoming data; an absent field carries + // the same "no value to give" meaning as a present-but-blank one, which is already + // preserved above. Without this it would be dropped and reset to its default. This also + // covers oauthTokenData, which incoming data never carries. + for (const [key, existingValue] of Object.entries(existing)) { + if (!Object.hasOwn(incoming, key)) { + merged[key] = existingValue; + } + } + + return merged; +} + +/** + * Merges incoming credential data into existing data. Incoming expressions, + * numbers and booleans overwrite existing values. Incoming plain strings do not + * carry secrets, so the existing value stays. Does not mutate the inputs. + */ +export function mergeCredentialData({ + existing, + incoming, + arrays, +}: { + existing: ICredentialDataDecryptedObject; + incoming: IDataObject; + arrays: CredentialArrayMergeMode; +}): ICredentialDataDecryptedObject { + // Every leaf comes from `existing`, `incoming` or is `''`, so the shape stays credential data. + return mergeObjects(incoming, existing, arrays) as ICredentialDataDecryptedObject; +} diff --git a/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts b/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts index 6abdc5f41c2..8d5f7a7593d 100644 --- a/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts @@ -72,6 +72,7 @@ describe('LogStreamingEventRelay', () => { workflowIdPolicy: 'new', credentialMatchingMode: 'id-only', credentialMissingMode: 'must-preexist', + credentialConflictPolicy: 'keep-existing', workflowPublishingPolicy: 'preserve-published-state', missingNodeTypeMode: 'fail', projectConflictPolicy: 'merge', @@ -153,6 +154,7 @@ describe('LogStreamingEventRelay', () => { workflowIdPolicy: 'new', credentialMatchingMode: 'id-only', credentialMissingMode: 'must-preexist', + credentialConflictPolicy: 'keep-existing', workflowPublishingPolicy: 'preserve-published-state', missingNodeTypeMode: 'fail', projectConflictPolicy: 'merge', diff --git a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts index 2909e2e979a..b47831baec2 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts @@ -3434,6 +3434,7 @@ describe('TelemetryEventRelay', () => { workflowIdPolicy: 'new', credentialMatchingMode: 'id-only', credentialMissingMode: 'must-preexist', + credentialConflictPolicy: 'fail', workflowPublishingPolicy: 'preserve-published-state', missingNodeTypeMode: 'fail', projectConflictPolicy: 'merge', @@ -3504,6 +3505,7 @@ describe('TelemetryEventRelay', () => { workflow_id_policy: 'new', credential_matching_mode: 'id-only', credential_missing_mode: 'must-preexist', + credential_conflict_policy: 'fail', workflow_publishing_policy: 'preserve-published-state', missing_node_type_mode: 'fail', project_conflict_policy: 'merge', diff --git a/packages/cli/src/events/relays/telemetry.event-relay.ts b/packages/cli/src/events/relays/telemetry.event-relay.ts index 5237397f2da..8930d2a6408 100644 --- a/packages/cli/src/events/relays/telemetry.event-relay.ts +++ b/packages/cli/src/events/relays/telemetry.event-relay.ts @@ -1405,6 +1405,7 @@ export class TelemetryEventRelay extends EventRelay { workflow_id_policy: options.workflowIdPolicy, credential_matching_mode: options.credentialMatchingMode, credential_missing_mode: options.credentialMissingMode, + credential_conflict_policy: options.credentialConflictPolicy, workflow_publishing_policy: options.workflowPublishingPolicy, missing_node_type_mode: options.missingNodeTypeMode, project_conflict_policy: options.projectConflictPolicy, diff --git a/packages/cli/src/modules/n8n-packages/__tests__/blocking-issue-schema-contract.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/blocking-issue-schema-contract.test.ts index babc329a7a0..72f0637a1f5 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/blocking-issue-schema-contract.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/blocking-issue-schema-contract.test.ts @@ -29,6 +29,14 @@ describe('BlockingIssue / ImportBlockingIssue contract', () => { expectTypeOf>().branded.toEqualTypeOf< Extract >(); + expectTypeOf>().branded.toEqualTypeOf< + Extract + >(); + expectTypeOf< + Extract + >().branded.toEqualTypeOf< + Extract + >(); expectTypeOf>().branded.toEqualTypeOf< Extract >(); diff --git a/packages/cli/src/modules/n8n-packages/__tests__/fixtures/package-fixtures.ts b/packages/cli/src/modules/n8n-packages/__tests__/fixtures/package-fixtures.ts index 23f6cf2c930..1b2f681221d 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/fixtures/package-fixtures.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/fixtures/package-fixtures.ts @@ -7,19 +7,19 @@ import { entityFilePath, workflowMetadataFilePath } from '../../io/manifest-entr import type { PackageWriter } from '../../io/package-writer'; import { TarPackageWriter } from '../../io/tar/tar-package-writer'; import { FORMAT_VERSION } from '../../spec/constants'; -import type { PackageManifest } from '../../spec/manifest.schema'; +import type { ManifestEntry, PackageManifest } from '../../spec/manifest.schema'; import type { PackageCredentialRequirement, PackageDataTableRequirement, PackageRequirementConsumer, PackageWorkflowRequirement, } from '../../spec/requirements.schema'; +import type { SerializedCredential } from '../../spec/serialized/credential.schema'; import type { SerializedDataTable } from '../../spec/serialized/data-table.schema'; import type { SerializedFolder } from '../../spec/serialized/folder.schema'; import type { SerializedProject } from '../../spec/serialized/project.schema'; import type { SerializedVariable } from '../../spec/serialized/variable.schema'; import type { SerializedWorkflowMetadata } from '../../spec/serialized/workflow-metadata.schema'; -import type { SerializedCredential } from '../../spec/serialized/credential.schema'; import type { SerializedWorkflow } from '../../spec/serialized/workflow.schema'; import { streamToBuffer } from '../utils/tar-support'; @@ -229,6 +229,20 @@ export function credentialRequirementsFromWorkflows( return [...byId.values()]; } +export function serializedCredential( + overrides: Partial & Pick, +): SerializedCredential { + return { name: 'GitHub', type: PACKAGE_GITHUB_CREDENTIAL_TYPE, ...overrides }; +} + +function credentialManifestEntry(credential: SerializedCredential): ManifestEntry { + return { + id: credential.id, + name: credential.name, + target: `credentials/credential-${credential.id}`, + }; +} + export async function buildImportPackageBuffer( workflows: PackageWorkflow[], options: { @@ -236,6 +250,7 @@ export async function buildImportPackageBuffer( sourceId?: string; /** Replaces every metadata file, or leaves it out, so tests can drive the rejections. */ workflowMetadata?: 'omit' | Record; + /** Credential files, written at the top-level `credentials/` folder (simulates export). */ credentials?: SerializedCredential[]; } = {}, ): Promise { @@ -254,11 +269,7 @@ export async function buildImportPackageBuffer( })), ...(options.credentials?.length ? { - credentials: options.credentials.map((credential, idx) => ({ - id: credential.id, - name: credential.name, - target: `credentials/cred-${idx}`, - })), + credentials: options.credentials.map((credential) => credentialManifestEntry(credential)), } : {}), ...options.manifestExtras, @@ -288,10 +299,11 @@ export async function buildImportPackageBuffer( ); } }); - (options.credentials ?? []).forEach((credential, idx) => { - writer.writeDirectory(`credentials/cred-${idx}`); - writer.writeFile(`credentials/cred-${idx}/credential.json`, JSON.stringify(credential)); - }); + for (const credential of options.credentials ?? []) { + const { target } = credentialManifestEntry(credential); + writer.writeDirectory(target); + writer.writeFile(entityFilePath('credentials', target), JSON.stringify(credential)); + } return await streamToBuffer(writer.finalize()); } @@ -380,8 +392,14 @@ export interface PackageVariableEntry { variable: SerializedVariable; } +export interface PackageCredentialEntry { + target: string; + credential: SerializedCredential; +} + export interface EntityPackageOptions { workflows?: PackageWorkflowEntry[]; + credentials?: PackageCredentialEntry[]; folders?: PackageFolderEntry[]; projects?: PackageProjectEntry[]; dataTables?: PackageDataTableEntry[]; @@ -399,6 +417,7 @@ async function writeEntityPackage( const projects = options.projects ?? []; const dataTables = options.dataTables ?? []; const variables = options.variables ?? []; + const credentials = options.credentials ?? []; const manifest: PackageManifest = { packageFormatVersion: FORMAT_VERSION, @@ -450,6 +469,15 @@ async function writeEntityPackage( })), } : {}), + ...(credentials.length > 0 + ? { + credentials: credentials.map(({ target, credential }) => ({ + id: credential.id, + name: credential.name, + target, + })), + } + : {}), ...options.manifestExtras, }; @@ -477,6 +505,10 @@ async function writeEntityPackage( await writer.writeDirectory(target); await writer.writeFile(entityFilePath('variables', target), JSON.stringify(variable)); } + for (const { target, credential } of credentials) { + await writer.writeDirectory(target); + await writer.writeFile(entityFilePath('credentials', target), JSON.stringify(credential)); + } } /** diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-credential-conflict.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-credential-conflict.integration.test.ts new file mode 100644 index 00000000000..18a3b867a04 --- /dev/null +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-credential-conflict.integration.test.ts @@ -0,0 +1,574 @@ +import { LicenseState } from '@n8n/backend-common'; +import { + createTeamProject, + linkUserToProject, + mockInstance, + testDb, + testModules, +} from '@n8n/backend-test-utils'; +import type { CredentialsEntity, Project, User } from '@n8n/db'; +import { + CredentialsRepository, + FolderRepository, + TagRepository, + WorkflowRepository, +} from '@n8n/db'; +import { Container } from '@n8n/di'; +import { ConflictError, UnprocessableRequestError } from '@n8n/errors'; + +import { ActiveWorkflowManager } from '@/active-workflow-manager'; +import { CredentialTypes } from '@/credential-types'; +import { + affixRoleToSaveCredential, + saveCredential, + shareCredentialWithProjects, +} from '@test-integration/db/credentials'; +import { createCustomRoleWithScopeSlugs } from '@test-integration/db/roles'; +import { createMember, createOwner } from '@test-integration/db/users'; +import { LicenseMocker } from '@test-integration/license'; +import { initNodeTypes } from '@test-integration/utils'; + +import { N8nPackagesService } from '../n8n-packages.service'; +import { importPackageRequest } from './fixtures/import-request'; +import { + buildEntityPackageBuffer, + buildImportPackageBuffer, + credentialRequirementsFromWorkflows, + githubCredentialPayload, + serializedCredential, + serializedProject, + serializedWorkflowWithCredential, +} from './fixtures/package-fixtures'; +import type { BlockingIssue, ImportPackageRequest } from '../n8n-packages.types'; +import type { SerializedCredential } from '../spec/serialized/credential.schema'; + +const EXPRESSION = '={{ $vars.githubToken }}'; +const OTHER_EXPRESSION = '={{ $vars.otherToken }}'; + +const licenseMocker = new LicenseMocker(); +const saveOwnedCredential = affixRoleToSaveCredential('credential:owner'); + +mockInstance(ActiveWorkflowManager); + +async function importPackage( + params: Pick & Partial, +) { + return await Container.get(N8nPackagesService).importPackage(importPackageRequest(params)); +} + +/** A workflow package with one workflow that uses `credential`, and the credential file. */ +async function workflowPackage( + credential: CredentialsEntity, + file: Omit | 'no-file', +): Promise { + return await buildImportPackageBuffer( + [ + serializedWorkflowWithCredential({ + id: 'wf-source', + name: 'Uses credential', + credentialId: credential.id, + credentialName: credential.name, + }), + ], + file === 'no-file' + ? {} + : { credentials: [serializedCredential({ id: credential.id, ...file })] }, + ); +} + +async function blockingIssuesOf( + promise: Promise, + errorClass: typeof ConflictError | typeof UnprocessableRequestError = ConflictError, +): Promise { + const error = await promise.then( + () => undefined, + (e: unknown) => e, + ); + const issues = (error as ConflictError).meta?.issues as BlockingIssue[]; + expect(error, JSON.stringify(issues)).toBeInstanceOf(errorClass); + return issues; +} + +/** Proves the import wrote nothing and left the credential row as it was. */ +async function expectNothingWritten(credential: CredentialsEntity) { + expect(await Container.get(WorkflowRepository).count()).toBe(0); + expect(await Container.get(FolderRepository).count()).toBe(0); + expect(await Container.get(TagRepository).count()).toBe(0); + const stored = await Container.get(CredentialsRepository).findOneByOrFail({ id: credential.id }); + expect(stored.name).toBe(credential.name); + expect(stored.data).toBe(credential.data); +} + +let owner: User; +let project: Project; + +beforeAll(async () => { + await testModules.loadModules(['n8n-packages']); + await testDb.init(); + await initNodeTypes(); + licenseMocker.mockLicenseState(Container.get(LicenseState)); + licenseMocker.setDefaults({ + features: ['feat:projectRole:admin', 'feat:folders'], + quotas: { 'quota:maxTeamProjects': 100 }, + }); + mockInstance(CredentialTypes).recognizes.mockReturnValue(true); +}); + +beforeEach(async () => { + await testDb.truncate([ + 'WorkflowEntity', + 'WorkflowHistory', + 'SharedWorkflow', + 'Folder', + 'TagEntity', + 'Project', + 'ProjectRelation', + 'CredentialsEntity', + 'SharedCredentials', + 'User', + ]); + owner = await createOwner(); + project = await createTeamProject('Target', owner); +}); + +afterAll(async () => { + await testDb.terminate(); +}); + +describe('credentialConflictPolicy', () => { + it('keep-existing (default) imports a differing match and leaves the credential as it is', async () => { + const credential = await saveOwnedCredential( + githubCredentialPayload({ name: 'GitHub', data: { accessToken: 'stored-secret' } }), + { project }, + ); + + await importPackage({ + user: owner, + projectId: project.id, + packageBuffer: await workflowPackage(credential, { + name: 'Renamed', + data: { accessToken: EXPRESSION }, + }), + }); + + expect(await Container.get(WorkflowRepository).count()).toBe(1); + const stored = await Container.get(CredentialsRepository).findOneByOrFail({ + id: credential.id, + }); + expect(stored.name).toBe('GitHub'); + expect(stored.data).toBe(credential.data); + }); + + it('fail blocks a rename and writes nothing', async () => { + const credential = await saveOwnedCredential( + githubCredentialPayload({ name: 'GitHub', data: { accessToken: EXPRESSION } }), + { project }, + ); + + const issues = await blockingIssuesOf( + importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { + name: 'Renamed', + data: { accessToken: EXPRESSION }, + }), + }), + ); + + expect(issues).toEqual([ + { + type: 'credential-conflict', + kind: 'differs', + targetId: credential.id, + sourceIds: [credential.id], + name: 'GitHub', + usedBy: [{ kind: 'workflow', id: 'wf-source' }], + }, + ]); + await expectNothingWritten(credential); + }); + + it('fail blocks an expression change', async () => { + const credential = await saveOwnedCredential( + githubCredentialPayload({ name: 'GitHub', data: { accessToken: 'stored-secret' } }), + { project }, + ); + + const issues = await blockingIssuesOf( + importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { + name: 'GitHub', + data: { accessToken: EXPRESSION }, + }), + }), + ); + + expect(issues).toEqual([ + expect.objectContaining({ type: 'credential-conflict', kind: 'differs' }), + ]); + await expectNothingWritten(credential); + }); + + it('fail imports an identical package', async () => { + const credential = await saveOwnedCredential( + githubCredentialPayload({ + name: 'GitHub', + data: { accessToken: EXPRESSION, clientSecret: 'stored-secret' }, + }), + { project }, + ); + + const result = await importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { + name: 'GitHub', + data: { accessToken: EXPRESSION }, + }), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + }); + + it('fail imports a match without a credential file', async () => { + const credential = await saveOwnedCredential(githubCredentialPayload({ name: 'GitHub' }), { + project, + }); + + const result = await importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, 'no-file'), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + }); + + it('fail only binds a credential owned by a project outside the import', async () => { + const otherProject = await createTeamProject('Other', owner); + const credential = await saveOwnedCredential(githubCredentialPayload({ name: 'GitHub' }), { + project: otherProject, + }); + await shareCredentialWithProjects(credential, [project]); + + const result = await importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { + name: 'Renamed', + data: { accessToken: EXPRESSION }, + }), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + }); + + it('fail only binds a global credential owned outside the import', async () => { + const credential = await saveCredential( + githubCredentialPayload({ name: 'Global GitHub', isGlobal: true }), + { user: owner, role: 'credential:owner' }, + ); + + const result = await importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { name: 'Renamed' }), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + }); + + it('overwrite only binds a global credential that a project admin cannot update', async () => { + const member = await createMember(); + await linkUserToProject(member, project, 'project:admin'); + const credential = await saveCredential( + githubCredentialPayload({ name: 'Global GitHub', isGlobal: true }), + { user: owner, role: 'credential:owner' }, + ); + + const result = await importPackage({ + user: member, + projectId: project.id, + credentialConflictPolicy: 'overwrite', + packageBuffer: await workflowPackage(credential, { + name: 'Renamed', + data: { accessToken: EXPRESSION }, + }), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + const stored = await Container.get(CredentialsRepository).findOneByOrFail({ + id: credential.id, + }); + expect(stored.name).toBe('Global GitHub'); + expect(stored.data).toBe(credential.data); + }); + + it('fail blocks a global credential owned by the import project', async () => { + const credential = await saveOwnedCredential( + githubCredentialPayload({ name: 'Global GitHub', isGlobal: true }), + { project }, + ); + + const issues = await blockingIssuesOf( + importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { name: 'Renamed' }), + }), + ); + + expect(issues).toEqual([ + expect.objectContaining({ + type: 'credential-conflict', + kind: 'differs', + targetId: credential.id, + }), + ]); + await expectNothingWritten(credential); + }); + + describe('credential in scope that the importing user cannot update', () => { + let member: User; + let credential: CredentialsEntity; + + beforeEach(async () => { + member = await createMember(); + // Project editors and admins always hold `credential:update`: only a custom role lacks it. + const role = await createCustomRoleWithScopeSlugs( + [ + 'project:read', + 'project:list', + 'workflow:create', + 'workflow:read', + 'workflow:update', + 'workflow:import', + 'workflow:list', + 'credential:read', + 'credential:list', + ], + { roleType: 'project' }, + ); + await linkUserToProject(member, project, role.slug); + credential = await saveOwnedCredential(githubCredentialPayload({ name: 'GitHub' }), { + project, + }); + }); + + it('fail blocks it and writes nothing', async () => { + const issues = await blockingIssuesOf( + importPackage({ + user: member, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { + name: 'Renamed', + data: { accessToken: EXPRESSION }, + }), + }), + UnprocessableRequestError, + ); + + expect(issues).toEqual([ + { + type: 'credential-update-forbidden', + targetId: credential.id, + sourceIds: [credential.id], + name: 'GitHub', + usedBy: [{ kind: 'workflow', id: 'wf-source' }], + }, + ]); + await expectNothingWritten(credential); + }); + + it('overwrite blocks it also when the package would not change it', async () => { + const issues = await blockingIssuesOf( + importPackage({ + user: member, + projectId: project.id, + credentialConflictPolicy: 'overwrite', + packageBuffer: await workflowPackage(credential, { name: 'GitHub' }), + }), + UnprocessableRequestError, + ); + + expect(issues).toEqual([ + expect.objectContaining({ + type: 'credential-update-forbidden', + targetId: credential.id, + }), + ]); + await expectNothingWritten(credential); + }); + + it('keep-existing binds it', async () => { + const result = await importPackage({ + user: member, + projectId: project.id, + credentialConflictPolicy: 'keep-existing', + packageBuffer: await workflowPackage(credential, { name: 'Renamed' }), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + }); + }); + + it('fail only binds a managed credential', async () => { + const credential = await saveOwnedCredential( + githubCredentialPayload({ name: 'Managed GitHub', isManaged: true }), + { project }, + ); + + const result = await importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { name: 'Renamed' }), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + }); + + it('fail only binds a resolvable credential', async () => { + const credential = await saveOwnedCredential( + githubCredentialPayload({ name: 'Resolvable GitHub', isResolvable: true }), + { project }, + ); + + const result = await importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'fail', + packageBuffer: await workflowPackage(credential, { name: 'Renamed' }), + }); + + expect(result.bindings.credentials).toEqual({ [credential.id]: credential.id }); + }); + + it('overwrite blocks two package credentials with different payloads for one destination', async () => { + const credential = await saveOwnedCredential(githubCredentialPayload({ name: 'GitHub' }), { + project, + }); + + const issues = await blockingIssuesOf( + importPackage({ + user: owner, + projectId: project.id, + credentialConflictPolicy: 'overwrite', + bindings: { + credentials: new Map([ + ['source-a', credential.id], + ['source-b', credential.id], + ]), + }, + packageBuffer: await buildImportPackageBuffer( + [ + serializedWorkflowWithCredential({ + id: 'wf-a', + name: 'Uses A', + credentialId: 'source-a', + credentialName: 'A', + }), + serializedWorkflowWithCredential({ + id: 'wf-b', + name: 'Uses B', + credentialId: 'source-b', + credentialName: 'B', + }), + ], + { + credentials: [ + serializedCredential({ + id: 'source-a', + name: 'A', + data: { accessToken: EXPRESSION }, + }), + serializedCredential({ + id: 'source-b', + name: 'B', + data: { accessToken: OTHER_EXPRESSION }, + }), + ], + }, + ), + }), + ); + + expect(issues).toEqual([ + { + type: 'credential-conflict', + kind: 'divergent', + targetId: credential.id, + sourceIds: ['source-a', 'source-b'], + name: 'GitHub', + usedBy: [ + { kind: 'workflow', id: 'wf-a' }, + { kind: 'workflow', id: 'wf-b' }, + ], + }, + ]); + await expectNothingWritten(credential); + }); + + it('fail blocks a credential owned by another project of a project package', async () => { + const secondProject = await createTeamProject('Second', owner); + const credential = await saveOwnedCredential(githubCredentialPayload({ name: 'GitHub' }), { + project: secondProject, + }); + await shareCredentialWithProjects(credential, [project]); + const workflow = serializedWorkflowWithCredential({ + id: 'wf-source', + name: 'Uses credential', + credentialId: credential.id, + credentialName: credential.name, + }); + + const issues = await blockingIssuesOf( + importPackage({ + user: owner, + credentialConflictPolicy: 'fail', + packageBuffer: await buildEntityPackageBuffer({ + projects: [ + { + target: 'projects/target', + project: serializedProject({ id: project.id, name: project.name }), + }, + { + target: 'projects/second', + project: serializedProject({ id: secondProject.id, name: secondProject.name }), + }, + ], + workflows: [{ target: 'projects/target/workflows/wf', workflow }], + credentials: [ + { + target: 'projects/second/credentials/github', + credential: serializedCredential({ id: credential.id, name: 'Renamed' }), + }, + ], + manifestExtras: { + requirements: { credentials: credentialRequirementsFromWorkflows([workflow]) }, + }, + }), + }), + ); + + expect(issues).toEqual([ + expect.objectContaining({ + type: 'credential-conflict', + kind: 'differs', + targetId: credential.id, + }), + ]); + await expectNothingWritten(credential); + }); +}); diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts index 6f5c6bb8feb..f4d06517c1d 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts @@ -121,6 +121,8 @@ describe('variable error consumers', () => { requirements: [], matchingMode: 'id-only', missingMode: 'must-preexist', + conflictPolicy: 'keep-existing', + updateScope: { projectIds: new Set([project.id]) }, }, dataTableRequest: { requirements: [], diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/credential-save-policy.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/credential-save-policy.test.ts index 1ded4e6d87c..9e77936b7ed 100644 --- a/packages/cli/src/modules/n8n-packages/engine/__tests__/credential-save-policy.test.ts +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/credential-save-policy.test.ts @@ -49,6 +49,8 @@ const request = ( requirements, matchingMode: 'id-only', missingMode, + conflictPolicy: 'keep-existing', + updateScope: { projectIds: new Set() }, }); const resolution = (...failures: CredentialResolutionFailure[]): CredentialResolution => ({ diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-blocked.error.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-blocked.error.test.ts index a86ad812809..0379789b2cb 100644 --- a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-blocked.error.test.ts +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-blocked.error.test.ts @@ -61,7 +61,24 @@ const tagUnresolved = ( usedBy: [{ kind: 'workflow', id: 'w1' }], }); +const credentialConflict = (kind: 'differs' | 'divergent'): BlockingIssue => ({ + type: 'credential-conflict', + kind, + targetId: 'c1', + sourceIds: ['s1'], + name: 'GitHub', + usedBy: [{ kind: 'workflow', id: 'w1' }], +}); + describe('toImportBlockedError', () => { + it.each(['differs', 'divergent'] as const)( + 'maps a %s credential-conflict to 409 Conflict', + (kind) => { + const error = toImportBlockedError([credentialConflict(kind), credentialUnresolved]); + expect(error).toBeInstanceOf(ConflictError); + }, + ); + it('maps a folder-conflict to 409 Conflict', () => { const error = toImportBlockedError([folderConflict]); expect(error).toBeInstanceOf(ConflictError); @@ -87,6 +104,19 @@ describe('toImportBlockedError', () => { expect(error).toBeInstanceOf(UnprocessableRequestError); }); + it('maps a credential-update-forbidden to 422', () => { + const error = toImportBlockedError([ + { + type: 'credential-update-forbidden', + targetId: 'c1', + sourceIds: ['s1'], + name: 'GitHub', + usedBy: [{ kind: 'workflow', id: 'w1' }], + }, + ]); + expect(error).toBeInstanceOf(UnprocessableRequestError); + }); + it('maps variable-only blocks to 422', () => { const error = toImportBlockedError([variableUnresolved]); expect(error).toBeInstanceOf(UnprocessableRequestError); diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts index 1421a1662db..2ad1cd62596 100644 --- a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts @@ -145,6 +145,8 @@ const scope = (input: { matchingMode: 'id-only', missingMode: 'create-stub', credentialBindings: undefined, + conflictPolicy: 'overwrite', + updateScope: { projectIds: new Set([context.projectId]) }, }, dataTableRequest: mock({ requirements: dt.requirements === 0 ? undefined : new Array(dt.requirements), @@ -172,6 +174,7 @@ const request = mock({ workflowIdPolicy: 'new', credentialMatchingMode: 'id-only', credentialMissingMode: 'create-stub', + credentialConflictPolicy: 'overwrite', workflowPublishingPolicy: 'preserve-published-state', folderConflictPolicy: 'overwrite', variableMissingMode: 'create-stub', @@ -275,6 +278,7 @@ describe('emitPackageImportedEvent', () => { tags: { matched: 1, created: 1, renamed: 1, reconciled: 1, skipped: 1, requirements: 5 }, }); expect(payload.packageSourceId).toBe('src-1'); + expect(payload.options.credentialConflictPolicy).toBe('overwrite'); expect(payload.options.variableMissingMode).toBe('create-stub'); expect(payload.options.variableConflictPolicy).toBe('overwrite'); expect(payload.options.variableParentPolicy).toBe('global'); diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/n8n-package-parser.credentials.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/n8n-package-parser.credentials.test.ts new file mode 100644 index 00000000000..a2861a308fc --- /dev/null +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/n8n-package-parser.credentials.test.ts @@ -0,0 +1,271 @@ +import type { Logger } from '@n8n/backend-common'; +import { UserError } from 'n8n-workflow'; +import { mock } from 'vitest-mock-extended'; + +import type { NodeTypes } from '@/node-types'; + +import type { WorkflowSerializer } from '../../entities/workflow/workflow.serializer'; +import type { PackageReader } from '../../io/package-reader'; +import { FORMAT_VERSION } from '../../spec/constants'; +import type { ManifestEntry, PackageManifest } from '../../spec/manifest.schema'; +import { N8nPackageParser } from '../n8n-package-parser'; + +function makeReader(manifest: PackageManifest, files: Record): PackageReader { + return { + readManifest: async () => await Promise.resolve(manifest), + readFile: async (path: string) => { + if (!(path in files)) throw new Error(`missing file ${path}`); + const content = files[path]; + const raw = typeof content === 'string' ? content : JSON.stringify(content); + return await Promise.resolve(Buffer.from(raw, 'utf-8')); + }, + listEntries: async () => await Promise.resolve(Object.keys(files)), + }; +} + +function manifestWith(extras: Partial = {}): PackageManifest { + return { + packageFormatVersion: FORMAT_VERSION, + exportedAt: '2026-01-01T00:00:00.000Z', + sourceN8nVersion: '1.0.0', + sourceId: 'test-source', + ...extras, + } as PackageManifest; +} + +const EXPRESSION = '={{ $vars.githubToken }}'; + +const topLevelEntry: ManifestEntry = { + id: 'cred-1', + name: 'GitHub', + target: 'credentials/github-cred-1', +}; + +describe('N8nPackageParser.getCredentials', () => { + const parser = new N8nPackageParser( + mock(), + mock(), + mock(), + ); + + it('reads a credential file from the top-level credentials folder, keyed by id', async () => { + const credential = { + id: 'cred-1', + name: 'GitHub', + type: 'githubApi', + data: { accessToken: EXPRESSION }, + }; + const reader = makeReader(manifestWith({ credentials: [topLevelEntry] }), { + 'credentials/github-cred-1/credential.json': credential, + }); + + const credentials = await parser.getCredentials(reader); + + expect(credentials).toEqual(new Map([['cred-1', credential]])); + }); + + it('reads a credential file from the owner project folder', async () => { + const credential = { id: 'cred-2', name: 'Slack', type: 'slackApi' }; + const reader = makeReader( + manifestWith({ + projects: [{ id: 'proj-1', name: 'billing', target: 'projects/billing-proj-1' }], + credentials: [ + { + id: 'cred-2', + name: 'Slack', + target: 'projects/billing-proj-1/credentials/slack-cred-2', + }, + ], + }), + { 'projects/billing-proj-1/credentials/slack-cred-2/credential.json': credential }, + ); + + const credentials = await parser.getCredentials(reader); + + expect(credentials.get('cred-2')).toEqual(credential); + }); + + it('reads only the requested credential files', async () => { + const credential = { id: 'cred-1', name: 'GitHub', type: 'githubApi' }; + const reader = makeReader( + manifestWith({ + credentials: [ + topLevelEntry, + { id: 'cred-2', name: 'Slack', target: 'credentials/slack-cred-2' }, + ], + }), + { 'credentials/github-cred-1/credential.json': credential }, + ); + + const credentials = await parser.getCredentials(reader, new Set(['cred-1'])); + + expect(credentials).toEqual(new Map([['cred-1', credential]])); + }); + + it('reads a credential file without data (no-values export)', async () => { + const reader = makeReader(manifestWith({ credentials: [topLevelEntry] }), { + 'credentials/github-cred-1/credential.json': { + id: 'cred-1', + name: 'GitHub', + type: 'githubApi', + }, + }); + + const credentials = await parser.getCredentials(reader); + + expect(credentials.get('cred-1')).toEqual({ id: 'cred-1', name: 'GitHub', type: 'githubApi' }); + expect(credentials.get('cred-1')).not.toHaveProperty('data'); + }); + + it('returns an empty map when the manifest lists no credential files', async () => { + const reader = makeReader( + manifestWith({ + requirements: { + credentials: [ + { + id: 'cred-1', + name: 'GitHub', + type: 'githubApi', + usedBy: [{ kind: 'workflow', id: 'wf-1' }], + }, + ], + }, + }), + {}, + ); + + const credentials = await parser.getCredentials(reader); + + expect(credentials.size).toBe(0); + }); + + it('throws a UserError when a listed credential file is missing', async () => { + const reader = makeReader(manifestWith({ credentials: [topLevelEntry] }), {}); + + const error = await parser.getCredentials(reader).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(UserError); + expect(error).toHaveProperty( + 'message', + 'Package manifest references a missing credential file at credentials/github-cred-1/credential.json.', + ); + }); + + it('throws a UserError when the credential file is not valid JSON', async () => { + const reader = makeReader(manifestWith({ credentials: [topLevelEntry] }), { + 'credentials/github-cred-1/credential.json': '{ not json', + }); + + const error = await parser.getCredentials(reader).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(UserError); + expect(error).toHaveProperty( + 'message', + 'Package credential file at credentials/github-cred-1/credential.json is not valid JSON.', + ); + }); + + it('throws a UserError when a data value is not an expression', async () => { + const reader = makeReader(manifestWith({ credentials: [topLevelEntry] }), { + 'credentials/github-cred-1/credential.json': { + id: 'cred-1', + name: 'GitHub', + type: 'githubApi', + data: { accessToken: 'plain-secret' }, + }, + }); + + const error = await parser.getCredentials(reader).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(UserError); + expect(error).toHaveProperty( + 'message', + 'Package credential file at credentials/github-cred-1/credential.json failed schema validation.', + ); + }); + + it('throws a UserError when the file id does not match the manifest id', async () => { + const reader = makeReader(manifestWith({ credentials: [topLevelEntry] }), { + 'credentials/github-cred-1/credential.json': { + id: 'cred-other', + name: 'GitHub', + type: 'githubApi', + }, + }); + + const error = await parser.getCredentials(reader).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(UserError); + expect(error).toHaveProperty( + 'message', + 'Package credential at credentials/github-cred-1/credential.json declares id "cred-other" but the manifest lists it as "cred-1".', + ); + }); + + it('throws a UserError when the file name does not match the manifest name', async () => { + const reader = makeReader(manifestWith({ credentials: [topLevelEntry] }), { + 'credentials/github-cred-1/credential.json': { + id: 'cred-1', + name: 'Renamed', + type: 'githubApi', + }, + }); + + const error = await parser.getCredentials(reader).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(UserError); + expect(error).toHaveProperty( + 'message', + 'Package credential at credentials/github-cred-1/credential.json declares name "Renamed" but the manifest lists it as "GitHub".', + ); + }); + it('throws a UserError when the file type does not match the requirement type', async () => { + const reader = makeReader( + manifestWith({ + credentials: [topLevelEntry], + requirements: { + credentials: [ + { + id: 'cred-1', + name: 'GitHub', + type: 'githubOAuth2Api', + usedBy: [{ kind: 'workflow', id: 'wf-1' }], + }, + ], + }, + }), + { + 'credentials/github-cred-1/credential.json': { + id: 'cred-1', + name: 'GitHub', + type: 'githubApi', + }, + }, + ); + + const error = await parser.getCredentials(reader).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(UserError); + expect(error).toHaveProperty( + 'message', + 'Package credential at credentials/github-cred-1/credential.json declares type "githubApi" but the requirement lists it as "githubOAuth2Api".', + ); + }); + + it('accepts a credential file whose requirement has no type', async () => { + const credential = { id: 'cred-1', name: 'GitHub', type: 'githubApi' }; + const reader = makeReader( + manifestWith({ + credentials: [topLevelEntry], + requirements: { + credentials: [{ id: 'cred-1', usedBy: [{ kind: 'agent', id: 'agent-1' }] }], + }, + }), + { 'credentials/github-cred-1/credential.json': credential }, + ); + + const credentials = await parser.getCredentials(reader); + + expect(credentials.get('cred-1')).toEqual(credential); + }); +}); diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts index 0e90bf4ca44..3d2d034778e 100644 --- a/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts @@ -68,18 +68,15 @@ describe('package-layout', () => { type: 'githubApi', usedBy: [{ kind: 'workflow', id: 'wf-1' }], }; - const bundled = (target: string, data?: SerializedCredential['data']) => + const bundled = (data?: SerializedCredential['data']) => new Map([ - [target, { id: 'cred-1', name: 'GitHub', type: 'githubApi', ...(data ? { data } : {}) }], + ['cred-1', { id: 'cred-1', name: 'GitHub', type: 'githubApi', ...(data ? { data } : {}) }], ]); - it('attaches bundled data by id regardless of which project dir carries the file', () => { + it('attaches bundled data by id', () => { const placed = placeCredentialData({ requirements: [requirement], - manifestCredentials: [entry('cred-1', 'projects/x/credentials/github')], - bundledCredentials: bundled('projects/x/credentials/github', { - token: '={{ $secrets.gh }}', - }), + bundledCredentials: bundled({ token: '={{ $secrets.gh }}' }), }); expect(placed).toEqual([{ ...requirement, packageData: { token: '={{ $secrets.gh }}' } }]); @@ -88,13 +85,11 @@ describe('package-layout', () => { it('leaves the requirement bare when the bundle carries no data or no file', () => { const noData = placeCredentialData({ requirements: [requirement], - manifestCredentials: [entry('cred-1', 'credentials/github')], - bundledCredentials: bundled('credentials/github'), + bundledCredentials: bundled(), }); const noFile = placeCredentialData({ requirements: [requirement], - manifestCredentials: [], - bundledCredentials: bundled('credentials/github', { token: '={{ $secrets.gh }}' }), + bundledCredentials: new Map(), }); expect(noData).toEqual([requirement]); diff --git a/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts b/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts index 17306f7193b..b644f68c035 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts @@ -20,6 +20,7 @@ export function toImportBlockedError( issue.type === 'project-conflict' || issue.type === 'folder-conflict' || issue.type === 'variable-conflict' || + issue.type === 'credential-conflict' || (issue.type === 'data-table-unresolved' && (issue.kind === 'id-conflict' || issue.kind === 'name-conflict')) || (issue.type === 'tag-unresolved' && diff --git a/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts b/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts index acc90e45a0c..0d2f07fe78c 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts @@ -4,11 +4,16 @@ import { Service } from '@n8n/di'; import { NodeTypes } from '@/node-types'; import type { PolicyActor } from '@/policy/policy-enforcement-backend'; +import { + credentialConflicts, + forbiddenCredentialUpdates, +} from '../entities/credential/credential-conflict-policy'; import { CredentialImporter } from '../entities/credential/credential-importer'; import { workflowsBlockedFromPublish } from '../entities/credential/credential-missing-mode'; import type { CredentialApplyResult, CredentialBindingRequest, + CredentialImportPlan, CredentialResolution, CredentialResolutionFailure, } from '../entities/credential/credential.types'; @@ -119,7 +124,7 @@ export interface ImportContentResult { export interface ImportPlan { input: ImportOrchestrationInput; folderContext: FolderImportContext; - credentialPlan: CredentialResolution; + credentialPlan: CredentialImportPlan; workflowPlan: WorkflowImportPlan; folderPlan: FolderImportPlan; dataTablePlan: DataTableImportPlan; @@ -223,6 +228,21 @@ export class ImportOrchestrator { ); } + // Across scopes: one destination credential can be reached from several projects. + if (plans.length > 0) { + const credentialPlans = plans.map((plan) => plan.credentialPlan); + for (const forbidden of forbiddenCredentialUpdates(credentialPlans)) { + issues.push({ type: 'credential-update-forbidden', ...forbidden }); + } + const conflicts = credentialConflicts( + plans[0].input.credentialRequest.conflictPolicy, + credentialPlans, + ); + for (const conflict of conflicts) { + issues.push({ type: 'credential-conflict', ...conflict }); + } + } + const quotaFailure = await this.variableImporter.quotaFailure( plans.flatMap((plan) => plan.variablePlan.creations), ); diff --git a/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts b/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts index 0c35283e560..02769525c7d 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts @@ -1,5 +1,6 @@ import type { EventService } from '@n8n/backend-services'; +import { resolveCredentialConflictPolicy } from '../entities/credential/credential-conflict-policy'; import type { CredentialBindingRequest } from '../entities/credential/credential.types'; import type { DataTableImportRequest } from '../entities/data-table/data-table.types'; import type { TagImportPlan, TagImportRequest } from '../entities/tag/tag.types'; @@ -110,6 +111,7 @@ export function emitPackageImportedEvent( workflowIdPolicy: request.workflowIdPolicy, credentialMatchingMode: request.credentialMatchingMode, credentialMissingMode: request.credentialMissingMode, + credentialConflictPolicy: resolveCredentialConflictPolicy(request.credentialConflictPolicy), workflowPublishingPolicy: request.workflowPublishingPolicy, missingNodeTypeMode: request.missingNodeTypeMode, projectConflictPolicy: request.projectConflictPolicy, diff --git a/packages/cli/src/modules/n8n-packages/engine/n8n-package-parser.ts b/packages/cli/src/modules/n8n-packages/engine/n8n-package-parser.ts index d5250626756..623cc7c98e6 100644 --- a/packages/cli/src/modules/n8n-packages/engine/n8n-package-parser.ts +++ b/packages/cli/src/modules/n8n-packages/engine/n8n-package-parser.ts @@ -131,22 +131,6 @@ export class N8nPackageParser { return projects; } - /** Bundled credential files, keyed by manifest target. */ - async getCredentials( - reader: PackageReader, - sourceIds: ReadonlySet, - ): Promise> { - const manifest = await this.getManifest(reader); - const credentials = new Map(); - - for (const entry of manifest.credentials ?? []) { - if (!sourceIds.has(entry.id)) continue; - credentials.set(entry.target, await this.readCredential(reader, entry)); - } - - return credentials; - } - /** Bundled variable files, keyed by manifest target. */ async getVariables(reader: PackageReader): Promise> { const manifest = await this.getManifest(reader); @@ -159,6 +143,33 @@ export class N8nPackageParser { return variables; } + /** + * Bundled credential files, keyed by credential id. A credential requirement + * without a file is valid: the exporting user could not read the credential. + */ + async getCredentials( + reader: PackageReader, + sourceIds?: ReadonlySet, + ): Promise> { + const manifest = await this.getManifest(reader); + const credentials = new Map(); + const requiredTypes = new Map( + (manifest.requirements?.credentials ?? []).map(({ id, type }) => [id, type]), + ); + + // The manifest target already points into the owner project folder or the + // top-level `credentials/` folder, so the import scope does not matter here. + for (const entry of manifest.credentials ?? []) { + if (sourceIds && !sourceIds.has(entry.id)) continue; + credentials.set( + entry.id, + await this.readCredential(reader, entry, requiredTypes.get(entry.id)), + ); + } + + return credentials; + } + private async readAgent(reader: PackageReader, entry: ManifestEntry): Promise { const path = entityFilePath('agents', entry.target); const agent = await this.readAgentFile(reader, path, serializedAgentSchema); @@ -407,34 +418,6 @@ export class N8nPackageParser { }; } - private async readCredential( - reader: PackageReader, - entry: ManifestEntry, - ): Promise { - const path = `${entry.target}/credential.json`; - const wire = await this.readJson(reader, path, 'credential'); - - let credential: SerializedCredential; - try { - credential = serializedCredentialSchema.parse(wire); - } catch (cause) { - if (cause instanceof ZodError) { - throw new UserError(`Package credential file at ${path} failed schema validation.`, { - cause, - }); - } - throw cause; - } - - if (credential.id !== entry.id) { - throw new UserError( - `Package credential at ${path} declares id "${credential.id}" but the manifest lists it as "${entry.id}".`, - ); - } - - return credential; - } - private async readVariable( reader: PackageReader, entry: ManifestEntry, @@ -463,6 +446,51 @@ export class N8nPackageParser { return variable; } + private async readCredential( + reader: PackageReader, + entry: ManifestEntry, + requiredType: string | undefined, + ): Promise { + const path = entityFilePath('credentials', entry.target); + const wire = await this.readJson(reader, path, 'credential'); + + let credential: SerializedCredential; + try { + credential = serializedCredentialSchema.parse(wire); + } catch (cause) { + if (cause instanceof ZodError) { + throw new UserError(`Package credential file at ${path} failed schema validation.`, { + cause, + }); + } + throw cause; + } + + // Requirements and bindings key off the manifest id, so a file with another + // identity would apply its data to the wrong credential. + if (credential.id !== entry.id) { + throw new UserError( + `Package credential at ${path} declares id "${credential.id}" but the manifest lists it as "${entry.id}".`, + ); + } + + if (credential.name !== entry.name) { + throw new UserError( + `Package credential at ${path} declares name "${credential.name}" but the manifest lists it as "${entry.name}".`, + ); + } + + // Matching binds a destination credential of the requirement type, so data of + // another type would merge into a credential with a different schema. + if (requiredType !== undefined && credential.type !== requiredType) { + throw new UserError( + `Package credential at ${path} declares type "${credential.type}" but the requirement lists it as "${requiredType}".`, + ); + } + + return credential; + } + private async readJson( reader: PackageReader, path: string, diff --git a/packages/cli/src/modules/n8n-packages/engine/package-layout.ts b/packages/cli/src/modules/n8n-packages/engine/package-layout.ts index 26232c8b54c..b078a5fb067 100644 --- a/packages/cli/src/modules/n8n-packages/engine/package-layout.ts +++ b/packages/cli/src/modules/n8n-packages/engine/package-layout.ts @@ -1,10 +1,15 @@ import { UserError } from 'n8n-workflow'; +import { resolveCredentialConflictPolicy } from '../entities/credential/credential-conflict-policy'; import { credentialMissingModeUsesPackageData } from '../entities/credential/credential-missing-mode'; import type { PlacedCredentialRequirement } from '../entities/credential/credential.types'; import { variableMissingModeUsesPackageValue } from '../entities/variable/variable-missing-mode'; import type { PlacedVariableRequirement } from '../entities/variable/variable.types'; -import { VariableConflictPolicy, VariableParentPolicy } from '../n8n-packages.types'; +import { + CredentialConflictPolicy, + VariableConflictPolicy, + VariableParentPolicy, +} from '../n8n-packages.types'; import type { ImportCredentialProperties, ImportVariableProperties } from '../n8n-packages.types'; import type { ManifestEntry } from '../spec/manifest.schema'; import type { @@ -46,16 +51,14 @@ export function needsBundledCredentialData( */ export function placeCredentialData({ requirements, - manifestCredentials, bundledCredentials, }: { requirements: PackageCredentialRequirement[] | undefined; - manifestCredentials: ManifestEntry[] | undefined; - bundledCredentials?: Map; + /** Package credential files by credential id. */ + bundledCredentials?: ReadonlyMap; }): PlacedCredentialRequirement[] | undefined { return requirements?.map((requirement) => { - const bundle = (manifestCredentials ?? []).find((entry) => entry.id === requirement.id); - const credential = bundle ? bundledCredentials?.get(bundle.target) : undefined; + const credential = bundledCredentials?.get(requirement.id); const packageData = credential?.data; return { @@ -79,6 +82,18 @@ export function needsBundledVariableValues( ); } +/** Only an update compares with the package credential files, so `keep-existing` never reads them. */ +export function needsBundledCredentialFiles( + request: Pick, + hasRequirements: boolean, +): boolean { + return ( + hasRequirements && + resolveCredentialConflictPolicy(request.credentialConflictPolicy) !== + CredentialConflictPolicy.KeepExisting + ); +} + /** The entry bundling `name` for one scope: its own variables/ entry beats a top-level one, a tie is ambiguous. */ function variableBundleEntry( entries: ManifestEntry[] | undefined, diff --git a/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts b/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts index 3edd8b02bfe..56b3f8bde31 100644 --- a/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts +++ b/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts @@ -3,7 +3,14 @@ import { Service } from '@n8n/di'; import { ForbiddenError } from '@n8n/errors'; -import type { CredentialBindingRequest } from '../entities/credential/credential.types'; +import { + credentialUpdateScope, + resolveCredentialConflictPolicy, +} from '../entities/credential/credential-conflict-policy'; +import type { + CredentialBindingRequest, + CredentialUpdateScope, +} from '../entities/credential/credential.types'; import { canStubNotFoundFailure } from '../entities/credential/credential-missing-mode'; import { removesUnpackagedWorkflows } from '../entities/folder/folder-conflict-policy'; import type { DataTableImportRequest } from '../entities/data-table/data-table.types'; @@ -35,6 +42,7 @@ import { import { toImportBlockedError } from './import-blocked.error'; import { needsBundledCredentialData, + needsBundledCredentialFiles, needsBundledVariableValues, placeByLayout, placeCredentialData, @@ -58,6 +66,7 @@ import { import type { ImportOutcome, PackageImportScope } from './import-telemetry'; import { N8nPackageParser } from './n8n-package-parser'; import type { ManifestEntry, PackageManifest } from '../spec/manifest.schema'; +import type { SerializedCredential } from '../spec/serialized/credential.schema'; import type { SerializedVariable } from '../spec/serialized/variable.schema'; @Service() @@ -108,6 +117,15 @@ export class ProjectPackageImporter { ) ? await this.packageParser.getVariables(reader) : undefined; + // Read once for the whole import: every scope looks its credentials up by id. + const packageCredentials = needsBundledCredentialFiles( + request, + (manifest.requirements?.credentials?.length ?? 0) > 0, + ) + ? await this.packageParser.getCredentials(reader) + : undefined; + // Projects match by id, so a package project id is also its destination project id. + const updateScope = credentialUpdateScope(selectedProjects.map(({ id }) => id)); // Projects the user is creating (vs matching an existing one). They will be admin of these, // so publish is always allowed and the project need not exist while its contents are planned. const pendingCreateIds = new Set( @@ -126,7 +144,7 @@ export class ProjectPackageImporter { manifest, project, pendingCreateIds.has(project.id), - bundledVariables, + { bundledVariables, packageCredentials, credentialUpdateScope: updateScope }, importSource, ); const plan = await this.importOrchestrator.plan(input); @@ -139,15 +157,13 @@ export class ProjectPackageImporter { ), ); if (needsBundledCredentialData(request, missingCredentialIds.size > 0)) { - const bundledCredentials = await this.packageParser.getCredentials( - reader, - missingCredentialIds, - ); + const bundledCredentials = + packageCredentials ?? + (await this.packageParser.getCredentials(reader, missingCredentialIds)); for (const { plan } of planned) { const { credentialRequest } = plan.input; credentialRequest.requirements = placeCredentialData({ requirements: credentialRequest.requirements, - manifestCredentials: manifest.credentials, bundledCredentials, }); } @@ -284,7 +300,12 @@ export class ProjectPackageImporter { manifest: PackageManifest, project: ManifestEntry, projectPendingCreation: boolean, - bundledVariables: Map | undefined, + /** Read or resolved once for every scope of the import. */ + importWide: { + bundledVariables: Map | undefined; + packageCredentials: Map | undefined; + credentialUpdateScope: CredentialUpdateScope; + }, importSource: PackageImportSource, ): Promise { const basePrefix = `${project.target}/`; @@ -309,6 +330,9 @@ export class ProjectPackageImporter { request.bindings?.credentials, requirements, ), + conflictPolicy: resolveCredentialConflictPolicy(request.credentialConflictPolicy), + packageCredentials: importWide.packageCredentials, + updateScope: importWide.credentialUpdateScope, }; const dataTableRequest: DataTableImportRequest = { @@ -324,7 +348,7 @@ export class ProjectPackageImporter { requirements: identifyRequirements(manifest.requirements?.variables, workflows), manifestVariables: manifest.variables, scopePrefix: basePrefix, - bundledVariables, + bundledVariables: importWide.bundledVariables, }), missingMode: request.variableMissingMode, conflictPolicy: request.variableConflictPolicy, diff --git a/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts b/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts index 984dd25d7e4..adb98060254 100644 --- a/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts +++ b/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts @@ -8,6 +8,10 @@ import { ForbiddenError, NotFoundError } from '@n8n/errors'; import { FolderService } from '@/services/folder.service'; import { ProjectService } from '@/services/project.service.ee'; +import { + credentialUpdateScope, + resolveCredentialConflictPolicy, +} from '../entities/credential/credential-conflict-policy'; import type { CredentialBindingRequest } from '../entities/credential/credential.types'; import { canStubNotFoundFailure } from '../entities/credential/credential-missing-mode'; import type { DataTableImportRequest } from '../entities/data-table/data-table.types'; @@ -35,6 +39,7 @@ import type { ImportOutcome, PackageImportScope } from './import-telemetry'; import { N8nPackageParser } from './n8n-package-parser'; import { needsBundledCredentialData, + needsBundledCredentialFiles, needsBundledVariableValues, placeByPolicy, placeCredentialData, @@ -84,6 +89,14 @@ export class WorkflowPackageImporter { matchingMode: request.credentialMatchingMode, missingMode: request.credentialMissingMode, credentialBindings: request.bindings?.credentials, + conflictPolicy: resolveCredentialConflictPolicy(request.credentialConflictPolicy), + packageCredentials: needsBundledCredentialFiles( + request, + (credentialRequirements?.length ?? 0) > 0, + ) + ? await this.packageParser.getCredentials(reader) + : undefined, + updateScope: credentialUpdateScope([context.projectId]), }; const dataTableRequirements = identifyRequirements( @@ -143,8 +156,9 @@ export class WorkflowPackageImporter { if (needsBundledCredentialData(request, missingCredentialIds.size > 0)) { credentialRequest.requirements = placeCredentialData({ requirements: credentialRequirements, - manifestCredentials: manifest.credentials, - bundledCredentials: await this.packageParser.getCredentials(reader, missingCredentialIds), + bundledCredentials: + credentialRequest.packageCredentials ?? + (await this.packageParser.getCredentials(reader, missingCredentialIds)), }); } diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-conflict-policy.test.ts b/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-conflict-policy.test.ts new file mode 100644 index 00000000000..b06d6ca1258 --- /dev/null +++ b/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-conflict-policy.test.ts @@ -0,0 +1,363 @@ +import type { SharedCredentials } from '@n8n/db'; + +import { + credentialConflicts, + credentialDiffers, + credentialUpdateScope, + divergentCredentials, + differingCredentials, + forbiddenCredentialUpdates, + isUpdateCandidate, + resolveCredentialConflictPolicy, + type UpdatableCredential, +} from '../credential-conflict-policy'; +import type { + CredentialMatch, + CredentialUpdateCandidate, + CredentialUpdateScope, +} from '../credential.types'; + +const EXPRESSION = '={{ $vars.token }}'; +const OTHER_EXPRESSION = '={{ $vars.otherToken }}'; + +const owned = (projectId: string): SharedCredentials[] => [ + { role: 'credential:owner', projectId } as SharedCredentials, + { role: 'credential:user', projectId: 'project-shared' } as SharedCredentials, +]; + +// `isGlobal` is not read, so a test can show that it does not change the scope. +type StoredCredential = UpdatableCredential & { isGlobal: boolean }; + +const credential = (overrides: Partial = {}): StoredCredential => ({ + isManaged: false, + isResolvable: false, + isGlobal: false, + usageScope: 'project', + shared: owned('project-a'), + ...overrides, +}); + +const scope: CredentialUpdateScope = { projectIds: new Set(['project-a', 'project-b']) }; + +const candidate = ( + overrides: Partial = {}, +): CredentialUpdateCandidate => ({ + sourceId: 'source-1', + targetId: 'target-1', + targetName: 'Destination', + name: 'Package', + data: { token: EXPRESSION }, + usedBy: [{ kind: 'workflow', id: 'wf-1' }], + ...overrides, +}); + +describe('resolveCredentialConflictPolicy', () => { + it('defaults to keep-existing', () => { + expect(resolveCredentialConflictPolicy(undefined)).toBe('keep-existing'); + expect(resolveCredentialConflictPolicy('fail')).toBe('fail'); + }); +}); + +describe('credentialUpdateScope', () => { + it('includes every destination project', () => { + expect(credentialUpdateScope(['project-a', 'project-b'])).toEqual(scope); + }); +}); + +describe('isUpdateCandidate', () => { + it('accepts a credential owned by a project of the import', () => { + expect(isUpdateCandidate(credential(), scope)).toBe(true); + expect(isUpdateCandidate(credential({ shared: owned('project-b') }), scope)).toBe(true); + }); + + it('rejects a credential owned by a project outside the import', () => { + expect(isUpdateCandidate(credential({ shared: owned('project-other') }), scope)).toBe(false); + }); + + it('does not count a project the credential is only shared with', () => { + const sharedOnly = credential({ + shared: [{ role: 'credential:user', projectId: 'project-a' } as SharedCredentials], + }); + + expect(isUpdateCandidate(sharedOnly, scope)).toBe(false); + }); + + it('rejects a global credential owned outside the import', () => { + const global = credential({ isGlobal: true, shared: owned('personal-project-of-someone') }); + + expect(isUpdateCandidate(global, scope)).toBe(false); + }); + + it('accepts a global credential owned by a project of the import', () => { + expect(isUpdateCandidate(credential({ isGlobal: true }), scope)).toBe(true); + }); + + it.each<[string, Partial]>([ + ['managed', { isManaged: true }], + ['resolvable', { isResolvable: true }], + ['instance-scoped', { usageScope: 'instance', shared: [] }], + ['managed and global', { isManaged: true, isGlobal: true }], + ])('rejects a %s credential', (_label, overrides) => { + expect(isUpdateCandidate(credential(overrides), scope)).toBe(false); + }); +}); + +describe('credentialDiffers', () => { + it('detects a name change only', () => { + expect( + credentialDiffers( + { name: 'Old', data: { token: EXPRESSION } }, + { name: 'New', data: { token: EXPRESSION } }, + ), + ).toBe(true); + }); + + it('detects an expression change', () => { + expect( + credentialDiffers( + { name: 'Same', data: { token: EXPRESSION } }, + { name: 'Same', data: { token: OTHER_EXPRESSION } }, + ), + ).toBe(true); + }); + + it('detects a package expression that replaces a stored plain value', () => { + expect( + credentialDiffers( + { name: 'Same', data: { token: 'stored-secret' } }, + { name: 'Same', data: { token: EXPRESSION } }, + ), + ).toBe(true); + }); + + it('ignores destination-only keys, including oauthTokenData', () => { + expect( + credentialDiffers( + { + name: 'Same', + data: { + token: EXPRESSION, + clientSecret: 'secret', + oauthTokenData: { access_token: 'token' }, + }, + }, + { name: 'Same', data: { token: EXPRESSION } }, + ), + ).toBe(false); + }); + + it('ignores array changes', () => { + expect( + credentialDiffers( + { name: 'Same', data: { headers: [{ name: 'X-Key', value: 'secret' }] } }, + { name: 'Same', data: { headers: [{ value: EXPRESSION }, { value: OTHER_EXPRESSION }] } }, + ), + ).toBe(false); + }); + + it('ignores an object that holds only arrays when the destination has no such object', () => { + expect( + credentialDiffers( + { name: 'Same', data: { token: 'stored-secret' } }, + { name: 'Same', data: { config: { headers: [EXPRESSION] } } }, + ), + ).toBe(false); + }); + + it('compares only the name for a file without data', () => { + const stored = { name: 'Same', data: { token: 'stored-secret' } }; + + expect(credentialDiffers(stored, { name: 'Same' })).toBe(false); + expect(credentialDiffers(stored, { name: 'Other' })).toBe(true); + }); +}); + +describe('differingCredentials', () => { + it('gives one conflict for each destination, with merged workflows', () => { + expect( + differingCredentials([ + candidate({ usedBy: [{ kind: 'workflow', id: 'wf-2' }] }), + candidate({ + usedBy: [ + { kind: 'workflow', id: 'wf-2' }, + { kind: 'workflow', id: 'wf-1' }, + ], + }), + candidate({ targetId: 'target-2', targetName: 'Second' }), + ]), + ).toEqual([ + { + kind: 'differs', + targetId: 'target-1', + sourceIds: ['source-1'], + name: 'Destination', + usedBy: [ + { kind: 'workflow', id: 'wf-1' }, + { kind: 'workflow', id: 'wf-2' }, + ], + }, + { + kind: 'differs', + targetId: 'target-2', + sourceIds: ['source-1'], + name: 'Second', + usedBy: [{ kind: 'workflow', id: 'wf-1' }], + }, + ]); + }); +}); + +describe('divergentCredentials', () => { + it('reports nothing for the same payload twice', () => { + expect(divergentCredentials([candidate(), candidate({ sourceId: 'source-2' })])).toEqual([]); + }); + + it('reports nothing for the same package credential from two scopes', () => { + expect(divergentCredentials([candidate(), candidate()])).toEqual([]); + }); + + it('reports one conflict for two payloads on one destination', () => { + expect( + divergentCredentials([ + candidate({ sourceId: 'source-2', usedBy: [{ kind: 'workflow', id: 'wf-2' }] }), + candidate({ data: { token: OTHER_EXPRESSION } }), + ]), + ).toEqual([ + { + kind: 'divergent', + targetId: 'target-1', + sourceIds: ['source-1', 'source-2'], + name: 'Destination', + usedBy: [ + { kind: 'workflow', id: 'wf-1' }, + { kind: 'workflow', id: 'wf-2' }, + ], + }, + ]); + }); + + it('reports a name difference', () => { + expect( + divergentCredentials([candidate(), candidate({ sourceId: 'source-2', name: 'Other' })]), + ).toHaveLength(1); + }); + + it('ignores payloads that differ only by arrays', () => { + expect( + divergentCredentials([ + candidate({ data: { token: EXPRESSION, scopes: [EXPRESSION] } }), + candidate({ sourceId: 'source-2', data: { token: EXPRESSION } }), + ]), + ).toEqual([]); + }); + + it('ignores a payload whose only difference is an object that holds only arrays', () => { + expect( + divergentCredentials([ + candidate({ data: { token: EXPRESSION, config: { headers: [EXPRESSION] } } }), + candidate({ sourceId: 'source-2', data: { token: EXPRESSION } }), + ]), + ).toEqual([]); + }); + + it('counts a candidate that equals the destination', () => { + // source-1 equals the destination and is not an update; writing source-2 would drop it. + expect( + divergentCredentials([ + candidate(), + candidate({ sourceId: 'source-2', data: { token: OTHER_EXPRESSION } }), + ]), + ).toEqual([ + expect.objectContaining({ kind: 'divergent', sourceIds: ['source-1', 'source-2'] }), + ]); + }); + + it('does not compare candidates of different destinations', () => { + expect( + divergentCredentials([ + candidate(), + candidate({ sourceId: 'source-2', targetId: 'target-2', name: 'Other' }), + ]), + ).toEqual([]); + }); +}); + +describe('credentialConflicts', () => { + const divergingPlans = [ + { candidates: [candidate()], updates: [candidate()] }, + { + candidates: [candidate({ sourceId: 'source-2', name: 'Other' })], + updates: [candidate({ sourceId: 'source-2', name: 'Other' })], + }, + ]; + + it('reports nothing under keep-existing', () => { + expect(credentialConflicts('keep-existing', divergingPlans)).toEqual([]); + }); + + it('reports differs and no divergence under fail', () => { + expect(credentialConflicts('fail', divergingPlans)).toEqual([ + expect.objectContaining({ kind: 'differs', sourceIds: ['source-1', 'source-2'] }), + ]); + }); + + it('reports divergence and no differs under overwrite', () => { + expect(credentialConflicts('overwrite', divergingPlans)).toEqual([ + expect.objectContaining({ kind: 'divergent', sourceIds: ['source-1', 'source-2'] }), + ]); + }); + + it('dedupes one destination reached from several scopes under fail', () => { + const plans = [ + { candidates: [candidate()], updates: [candidate()] }, + { candidates: [candidate()], updates: [candidate()] }, + ]; + + expect(credentialConflicts('fail', plans)).toHaveLength(1); + expect(credentialConflicts('overwrite', plans)).toEqual([]); + }); +}); + +describe('forbiddenCredentialUpdates', () => { + const match = (overrides: Partial = {}): CredentialMatch => ({ + sourceId: 'source-1', + targetId: 'target-1', + targetName: 'Destination', + usedBy: [{ kind: 'workflow', id: 'wf-1' }], + ...overrides, + }); + + it('gives one issue for each destination across scopes, with merged consumers', () => { + const plans = [ + { forbidden: [match(), match({ targetId: 'target-2', targetName: 'Other' })] }, + { + forbidden: [ + match({ usedBy: [{ kind: 'workflow', id: 'wf-2' }] }), + match({ sourceId: 'source-0' }), + ], + }, + ]; + + expect(forbiddenCredentialUpdates(plans)).toEqual([ + { + targetId: 'target-1', + sourceIds: ['source-0', 'source-1'], + name: 'Destination', + usedBy: [ + { kind: 'workflow', id: 'wf-1' }, + { kind: 'workflow', id: 'wf-2' }, + ], + }, + { + targetId: 'target-2', + sourceIds: ['source-1'], + name: 'Other', + usedBy: [{ kind: 'workflow', id: 'wf-1' }], + }, + ]); + }); + + it('reports nothing without forbidden matches', () => { + expect(forbiddenCredentialUpdates([{ forbidden: [] }])).toEqual([]); + }); +}); diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-importer.test.ts b/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-importer.test.ts index 8e3cbf04cde..5a9e5a965b2 100644 --- a/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-importer.test.ts +++ b/packages/cli/src/modules/n8n-packages/entities/credential/__tests__/credential-importer.test.ts @@ -1,4 +1,5 @@ -import type { Project, User } from '@n8n/db'; +import type { CredentialsFinderService } from '@n8n/backend-services'; +import type { CredentialsEntity, Project, SharedCredentials, User } from '@n8n/db'; import { Container } from '@n8n/di'; import { mock } from 'vitest-mock-extended'; @@ -9,6 +10,7 @@ import type { CredentialsService } from '@/credentials/credentials.service'; import type { ImportContext } from '../../../n8n-packages.types'; import { CredentialImporter } from '../credential-importer'; import { CredentialMatcherFactory } from '../credential-matcher-factory'; +import type { SerializedCredential } from '../../../spec/serialized/credential.schema'; import type { CredentialBindingRequest, CredentialResolutionFailure } from '../credential.types'; import { IdBasedCredentialMatcher } from '../id-based-credential-matcher'; import type { NameAndTypeCredentialMatcher } from '../name-and-type-credential-matcher'; @@ -24,6 +26,7 @@ const usable = (id: string, type = 'githubApi'): UsableCredential => describe('CredentialImporter', () => { const credentialsService = mock(); + const credentialsFinder = mock(); const credentialTypes = mock(); const targetProject = mock({ id: 'project-target' }); const user = mock({ id: 'user-1' }); @@ -38,12 +41,18 @@ describe('CredentialImporter', () => { credentialBindings?: CredentialBindingRequest['credentialBindings']; missingMode?: CredentialBindingRequest['missingMode']; matchingMode?: CredentialBindingRequest['matchingMode']; + conflictPolicy?: CredentialBindingRequest['conflictPolicy']; + packageCredentials?: CredentialBindingRequest['packageCredentials']; + updateScope?: CredentialBindingRequest['updateScope']; } = {}, ): CredentialBindingRequest => ({ requirements, matchingMode: options.matchingMode ?? 'id-only', missingMode: options.missingMode ?? 'must-preexist', credentialBindings: options.credentialBindings, + conflictPolicy: options.conflictPolicy ?? 'keep-existing', + packageCredentials: options.packageCredentials, + updateScope: options.updateScope ?? { projectIds: new Set([targetProject.id]) }, }); const packageCredential = ( @@ -81,6 +90,7 @@ describe('CredentialImporter', () => { mock(), ), credentialsService, + credentialsFinder, ); }); @@ -383,6 +393,236 @@ describe('CredentialImporter', () => { ).rejects.toBeInstanceOf(ForbiddenError); }); }); + describe('conflict policy', () => { + const EXPRESSION = '={{ $vars.githubToken }}'; + const requirement = packageCredential({ + id: 'cred-1', + usedBy: [ + { kind: 'workflow', id: 'wf-2' }, + { kind: 'workflow', id: 'wf-1' }, + ], + }); + + const packageFile = (overrides: Partial = {}): SerializedCredential => ({ + id: 'cred-1', + name: 'Source GitHub', + type: 'githubApi', + data: { accessToken: EXPRESSION }, + ...overrides, + }); + + const destination = (overrides: Partial = {}) => + mock({ + id: 'cred-1', + name: 'Source GitHub', + type: 'githubApi', + isManaged: false, + isResolvable: false, + isGlobal: false, + usageScope: 'project', + shared: [ + mock({ role: 'credential:owner', projectId: targetProject.id }), + ], + ...overrides, + }); + + beforeEach(() => { + credentialsService.getCredentialsAUserCanUseInAWorkflow.mockResolvedValue([usable('cred-1')]); + credentialsFinder.findCredentialIdsWithScopeForUser.mockImplementation( + async (ids) => await Promise.resolve(new Set(ids)), + ); + }); + + it('keep-existing loads and decrypts nothing', async () => { + const result = await importer.plan( + context, + bindingRequest([requirement], { + conflictPolicy: 'keep-existing', + packageCredentials: new Map([['cred-1', packageFile({ name: 'Renamed' })]]), + }), + ); + + expect(result.successes).toEqual(new Map([['cred-1', 'cred-1']])); + expect(result.candidates).toEqual([]); + expect(result.updates).toEqual([]); + expect(result.forbidden).toEqual([]); + expect(credentialsFinder.findById).not.toHaveBeenCalled(); + expect(credentialsFinder.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled(); + expect(credentialsService.decrypt).not.toHaveBeenCalled(); + }); + + it('plans an update when the package value differs from the stored value', async () => { + credentialsFinder.findById.mockResolvedValue(destination()); + credentialsService.decrypt.mockResolvedValue({ accessToken: 'stored-secret' }); + + const result = await importer.plan( + context, + bindingRequest([requirement], { + conflictPolicy: 'fail', + packageCredentials: new Map([['cred-1', packageFile()]]), + }), + ); + + const candidate = { + sourceId: 'cred-1', + targetId: 'cred-1', + targetName: 'Source GitHub', + name: 'Source GitHub', + data: { accessToken: EXPRESSION }, + usedBy: [ + { kind: 'workflow', id: 'wf-1' }, + { kind: 'workflow', id: 'wf-2' }, + ], + }; + expect(result.candidates).toEqual([candidate]); + expect(result.updates).toEqual([candidate]); + expect(credentialsFinder.findById).toHaveBeenCalledWith('cred-1', { + includeSharedProject: true, + }); + expect(credentialsService.decrypt).toHaveBeenCalledWith(expect.anything(), true); + }); + + it('keeps an equal candidate out of the updates', async () => { + credentialsFinder.findById.mockResolvedValue(destination()); + credentialsService.decrypt.mockResolvedValue({ accessToken: EXPRESSION, other: 'secret' }); + + const result = await importer.plan( + context, + bindingRequest([requirement], { + conflictPolicy: 'overwrite', + packageCredentials: new Map([['cred-1', packageFile()]]), + }), + ); + + expect(result.candidates).toHaveLength(1); + expect(result.updates).toEqual([]); + }); + + it('only binds a match without a package file', async () => { + const result = await importer.plan( + context, + bindingRequest([requirement], { conflictPolicy: 'fail', packageCredentials: new Map() }), + ); + + expect(result.successes).toEqual(new Map([['cred-1', 'cred-1']])); + expect(result.candidates).toEqual([]); + expect(credentialsFinder.findById).not.toHaveBeenCalled(); + }); + + it('only binds a match that is not an update candidate', async () => { + credentialsFinder.findById.mockResolvedValue(destination({ isManaged: true })); + + const result = await importer.plan( + context, + bindingRequest([requirement], { + conflictPolicy: 'fail', + packageCredentials: new Map([['cred-1', packageFile({ name: 'Renamed' })]]), + }), + ); + + expect(result.candidates).toEqual([]); + expect(result.updates).toEqual([]); + expect(credentialsService.decrypt).not.toHaveBeenCalled(); + }); + + it('reports a match in scope that the user cannot update as forbidden, and decrypts nothing', async () => { + credentialsFinder.findById.mockResolvedValue(destination()); + credentialsFinder.findCredentialIdsWithScopeForUser.mockResolvedValue(new Set()); + + const result = await importer.plan( + context, + bindingRequest([requirement], { + conflictPolicy: 'fail', + packageCredentials: new Map([['cred-1', packageFile({ name: 'Renamed' })]]), + }), + ); + + expect(result.successes).toEqual(new Map([['cred-1', 'cred-1']])); + expect(result.candidates).toEqual([]); + expect(result.updates).toEqual([]); + expect(result.forbidden).toEqual([ + { + sourceId: 'cred-1', + targetId: 'cred-1', + targetName: 'Source GitHub', + usedBy: [ + { kind: 'workflow', id: 'wf-1' }, + { kind: 'workflow', id: 'wf-2' }, + ], + }, + ]); + expect(credentialsFinder.findCredentialIdsWithScopeForUser).toHaveBeenCalledWith( + ['cred-1'], + user, + ['credential:update'], + ); + expect(credentialsService.decrypt).not.toHaveBeenCalled(); + }); + + it('only binds a match out of scope, and checks no permission for it', async () => { + credentialsFinder.findById.mockResolvedValue( + destination({ + shared: [ + mock({ role: 'credential:owner', projectId: 'project-other' }), + ], + }), + ); + + const result = await importer.plan( + context, + bindingRequest([requirement], { + conflictPolicy: 'overwrite', + packageCredentials: new Map([['cred-1', packageFile({ name: 'Renamed' })]]), + }), + ); + + expect(result.candidates).toEqual([]); + expect(result.forbidden).toEqual([]); + expect(credentialsFinder.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled(); + expect(credentialsService.decrypt).not.toHaveBeenCalled(); + }); + + it('only binds a match the finder does not return (an instance credential)', async () => { + credentialsFinder.findById.mockResolvedValue(null); + + const result = await importer.plan( + context, + bindingRequest([requirement], { + conflictPolicy: 'overwrite', + packageCredentials: new Map([['cred-1', packageFile()]]), + }), + ); + + expect(result.candidates).toEqual([]); + }); + + it('loads a destination once when two package credentials resolve to it', async () => { + credentialsService.getCredentialsAUserCanUseInAWorkflow.mockResolvedValue([ + usable('cred-target'), + ]); + credentialsFinder.findById.mockResolvedValue(destination({ id: 'cred-target' })); + credentialsService.decrypt.mockResolvedValue({ accessToken: 'stored-secret' }); + const other = packageCredential({ id: 'cred-2' }); + + const result = await importer.plan( + context, + bindingRequest([requirement, other], { + conflictPolicy: 'overwrite', + credentialBindings: new Map([ + ['cred-1', 'cred-target'], + ['cred-2', 'cred-target'], + ]), + packageCredentials: new Map([ + ['cred-1', packageFile()], + ['cred-2', packageFile({ id: 'cred-2' })], + ]), + }), + ); + + expect(result.candidates.map(({ sourceId }) => sourceId)).toEqual(['cred-1', 'cred-2']); + expect(credentialsFinder.findById).toHaveBeenCalledTimes(1); + }); + }); describe('create-with-values', () => { it('apply seeds a created credential from the requirement package data', async () => { diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/credential-conflict-policy.ts b/packages/cli/src/modules/n8n-packages/entities/credential/credential-conflict-policy.ts new file mode 100644 index 00000000000..d19ea7dc5ef --- /dev/null +++ b/packages/cli/src/modules/n8n-packages/entities/credential/credential-conflict-policy.ts @@ -0,0 +1,190 @@ +import type { CredentialsEntity } from '@n8n/db'; +import isEqual from 'lodash/isEqual'; +import type { ICredentialDataDecryptedObject } from 'n8n-workflow'; + +import { mergeCredentialData } from '@/credentials/credential-data-merge'; + +import type { + CredentialConflict, + CredentialImportPlan, + CredentialMatch, + CredentialUpdateCandidate, + CredentialUpdateForbidden, + CredentialUpdateScope, +} from './credential.types'; +import { CredentialConflictPolicy } from '../../n8n-packages.types'; +import { mergeRequirementConsumers } from '../../spec/requirement-consumers'; +import type { SerializedCredentialData } from '../../spec/serialized/credential.schema'; + +export function resolveCredentialConflictPolicy( + policy: CredentialConflictPolicy | undefined, +): CredentialConflictPolicy { + return policy ?? CredentialConflictPolicy.KeepExisting; +} + +/** + * A credential owned by any destination project of the import can change. Any other credential, + * also a global one, is only bound: a project import must not change a credential that its + * projects do not own. + */ +export function credentialUpdateScope(destinationProjectIds: string[]): CredentialUpdateScope { + return { projectIds: new Set(destinationProjectIds) }; +} + +export type UpdatableCredential = Pick< + CredentialsEntity, + 'isManaged' | 'isResolvable' | 'usageScope' | 'shared' +>; + +/** + * Whether a matched destination credential is in the update scope of the conflict policy. Any + * other match is only bound: it never changes and never blocks. A match in scope also needs + * `credential:update`, else it blocks: see `CredentialImporter`. + */ +export function isUpdateCandidate( + credential: UpdatableCredential, + scope: CredentialUpdateScope, +): boolean { + if (credential.isManaged || credential.isResolvable || credential.usageScope === 'instance') { + return false; + } + + // A credential has no owner column: its owner is the project on the `credential:owner` sharing. + const ownerProjectId = credential.shared?.find( + (sharing) => sharing.role === 'credential:owner', + )?.projectId; + return ownerProjectId !== undefined && scope.projectIds.has(ownerProjectId); +} + +/** Package credentials never carry arrays that can apply, see `credential-export-policy.ts`. */ +function mergeIntoStored( + stored: ICredentialDataDecryptedObject, + incoming: SerializedCredentialData, +): ICredentialDataDecryptedObject { + return mergeCredentialData({ existing: stored, incoming, arrays: 'keep-existing' }); +} + +/** + * Whether `overwrite` would change the destination credential: a different name, or stored + * data that changes when the package values merge into it. Arrays never count. + */ +export function credentialDiffers( + stored: { name: string; data: ICredentialDataDecryptedObject }, + incoming: { name: string; data?: SerializedCredentialData }, +): boolean { + if (stored.name !== incoming.name) return true; + if (incoming.data === undefined) return false; + + return !isEqual(mergeIntoStored(stored.data, incoming.data), stored.data); +} + +/** What a package credential would write, so that two payloads compare by effect. */ +function writtenPayload(candidate: CredentialUpdateCandidate) { + return { name: candidate.name, data: mergeIntoStored({}, candidate.data ?? {}) }; +} + +function uniqueSorted(values: string[]): string[] { + return [...new Set(values)].sort(); +} + +function groupByTarget(candidates: T[]): Map { + const byTarget = new Map(); + for (const candidate of candidates) { + const group = byTarget.get(candidate.targetId) ?? []; + // Scopes of one project package can plan the same package credential more than once, + // each with its own consumers: keep one entry, with the consumers of all scopes. + const index = group.findIndex(({ sourceId }) => sourceId === candidate.sourceId); + if (index === -1) { + group.push(candidate); + } else { + group[index] = { + ...group[index], + usedBy: mergeRequirementConsumers([...group[index].usedBy, ...candidate.usedBy]), + }; + } + byTarget.set(candidate.targetId, group); + } + return byTarget; +} + +function toTargetIssue(targetId: string, group: CredentialMatch[]): CredentialUpdateForbidden { + return { + targetId, + sourceIds: uniqueSorted(group.map(({ sourceId }) => sourceId)), + name: group[0].targetName, + usedBy: mergeRequirementConsumers(group.flatMap(({ usedBy }) => usedBy)), + }; +} + +function toConflict( + kind: CredentialConflict['kind'], + targetId: string, + group: CredentialMatch[], +): CredentialConflict { + return { kind, ...toTargetIssue(targetId, group) }; +} + +/** One `differs` conflict for each destination credential that the package would change. */ +export function differingCredentials(updates: CredentialUpdateCandidate[]): CredentialConflict[] { + return [...groupByTarget(updates)].map(([targetId, group]) => + toConflict('differs', targetId, group), + ); +} + +/** + * Scopes and matchers resolve independently, so two package credentials can land on one + * destination credential. When their payloads differ, the last write would silently win. + * All candidates count, also those equal to the destination: writing another one would drop + * them. + */ +export function divergentCredentials( + candidates: CredentialUpdateCandidate[], +): CredentialConflict[] { + return [...groupByTarget(candidates)] + .filter(([, group]) => { + const [first, ...others] = group.map(writtenPayload); + return others.some((payload) => !isEqual(payload, first)); + }) + .map(([targetId, group]) => toConflict('divergent', targetId, group)); +} + +/** Read-only: classifies which planned credential updates block the import, per policy. */ +/* eslint-disable @typescript-eslint/naming-convention -- API credential conflict policy keys */ +const CONFLICTS: Record< + CredentialConflictPolicy, + ( + candidates: CredentialUpdateCandidate[], + updates: CredentialUpdateCandidate[], + ) => CredentialConflict[] +> = { + 'keep-existing': () => [], + overwrite: (candidates) => divergentCredentials(candidates), + fail: (_candidates, updates) => differingCredentials(updates), +}; +/* eslint-enable @typescript-eslint/naming-convention */ + +/** + * The credential conflicts of a whole import. Takes every scope at once, so that a destination + * credential reached from several scopes gives one conflict. + */ +export function credentialConflicts( + policy: CredentialConflictPolicy, + plans: Array>, +): CredentialConflict[] { + return CONFLICTS[policy]( + plans.flatMap(({ candidates }) => candidates), + plans.flatMap(({ updates }) => updates), + ); +} + +/** + * One issue for each destination credential in the update scope that the user cannot update. + * Takes every scope at once, like {@link credentialConflicts}. + */ +export function forbiddenCredentialUpdates( + plans: Array>, +): CredentialUpdateForbidden[] { + return [...groupByTarget(plans.flatMap(({ forbidden }) => forbidden))].map(([targetId, group]) => + toTargetIssue(targetId, group), + ); +} diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/credential-importer.ts b/packages/cli/src/modules/n8n-packages/entities/credential/credential-importer.ts index b2cf8838c91..25519c50825 100644 --- a/packages/cli/src/modules/n8n-packages/entities/credential/credential-importer.ts +++ b/packages/cli/src/modules/n8n-packages/entities/credential/credential-importer.ts @@ -1,8 +1,11 @@ +import { CredentialsFinderService } from '@n8n/backend-services'; +import type { CredentialsEntity, User } from '@n8n/db'; import { Service } from '@n8n/di'; import { UnexpectedError, type ICredentialDataDecryptedObject } from 'n8n-workflow'; import { CredentialsService } from '@/credentials/credentials.service'; +import { credentialDiffers, isUpdateCandidate } from './credential-conflict-policy'; import { CredentialMatcherFactory } from './credential-matcher-factory'; import { credentialBlockingFailures, @@ -12,35 +15,131 @@ import { import type { CredentialApplyResult, CredentialBindingRequest, + CredentialImportPlan, + CredentialMatch, CredentialResolution, CredentialResolutionFailure, + CredentialUpdateCandidate, PlacedCredentialRequirement, } from './credential.types'; import type { ImportBindingMap, ImportContext } from '../../n8n-packages.types'; +import { mergeRequirementConsumers } from '../../spec/requirement-consumers'; +import type { SerializedCredential } from '../../spec/serialized/credential.schema'; @Service() export class CredentialImporter { constructor( private readonly credentialMatcherFactory: CredentialMatcherFactory, private readonly credentialsService: CredentialsService, + private readonly credentialsFinder: CredentialsFinderService, ) {} /** * Resolves which target-project credentials the package's references map to. * Read-only: returns matched `successes` and unresolved `failures` without - * acting on the failures. Mirrors the workflow side's `plan`. + * acting on the failures. Mirrors the workflow side's `plan`. Unless the conflict + * policy is `keep-existing`, also finds the matches the policy can update. */ async plan( context: ImportContext, request: CredentialBindingRequest, - ): Promise { - return await this.credentialMatcherFactory + ): Promise { + const resolution = await this.credentialMatcherFactory .getMatcher(request.matchingMode) .match(request.requirements, { projectId: context.projectId, user: context.user, credentialBindings: request.credentialBindings, }); + + if (request.conflictPolicy === 'keep-existing') { + return { ...resolution, candidates: [], updates: [], forbidden: [] }; + } + + return { ...resolution, ...(await this.planUpdates(context.user, request, resolution)) }; + } + + /** + * Compares each matched credential in the update scope that has a package file with its + * stored state. A match the user cannot update is `forbidden` instead, and is not read. + * Decrypted values stay in this method: the plan keeps only ids and the package payload. + */ + private async planUpdates( + user: User, + request: CredentialBindingRequest, + resolution: CredentialResolution, + ): Promise> { + const requirementsById = new Map( + (request.requirements ?? []).map((requirement) => [requirement.id, requirement]), + ); + const destinations = new Map(); + const inScope: Array<{ + match: CredentialMatch; + packageCredential: SerializedCredential; + destination: CredentialsEntity; + }> = []; + + for (const [sourceId, targetId] of resolution.successes) { + // No file means the exporting user could not read the credential: bind only. + const packageCredential = request.packageCredentials?.get(sourceId); + if (packageCredential === undefined) continue; + + if (!destinations.has(targetId)) { + destinations.set( + targetId, + await this.credentialsFinder.findById(targetId, { includeSharedProject: true }), + ); + } + const destination = destinations.get(targetId); + if (!destination || !isUpdateCandidate(destination, request.updateScope)) continue; + + inScope.push({ + match: { + sourceId, + targetId, + targetName: destination.name, + usedBy: mergeRequirementConsumers(requirementsById.get(sourceId)?.usedBy ?? []), + }, + packageCredential, + destination, + }); + } + + // Matching needs only use access. The comparison reads the stored secret, so a match in + // scope that the user cannot update blocks without being read. + const updatableIds = + inScope.length > 0 + ? await this.credentialsFinder.findCredentialIdsWithScopeForUser( + [...new Set(inScope.map(({ match }) => match.targetId))], + user, + ['credential:update'], + ) + : new Set(); + const candidates: CredentialUpdateCandidate[] = []; + const updates: CredentialUpdateCandidate[] = []; + const forbidden: CredentialMatch[] = []; + + for (const { match, packageCredential, destination } of inScope) { + if (!updatableIds.has(match.targetId)) { + forbidden.push(match); + continue; + } + + const candidate: CredentialUpdateCandidate = { + ...match, + name: packageCredential.name, + ...(packageCredential.data !== undefined ? { data: packageCredential.data } : {}), + }; + candidates.push(candidate); + + const differs = credentialDiffers( + { name: destination.name, data: await this.credentialsService.decrypt(destination, true) }, + packageCredential, + ); + if (differs) updates.push(candidate); + } + + return { candidates, updates, forbidden }; } /** diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/credential.exporter.ts b/packages/cli/src/modules/n8n-packages/entities/credential/credential.exporter.ts index c2ae2a21e13..b6592ca1908 100644 --- a/packages/cli/src/modules/n8n-packages/entities/credential/credential.exporter.ts +++ b/packages/cli/src/modules/n8n-packages/entities/credential/credential.exporter.ts @@ -104,10 +104,7 @@ export class CredentialExporter { return { entries, requirements }; } - /** - * A credential has no owner column: its owner is the project on the - * `credential:owner` sharing. Undefined for a global credential. - */ + /** A credential has no owner column: its owner is the project on the `credential:owner` sharing. */ private ownerProjectId(credential: CredentialsEntity): string | undefined { return credential.shared?.find((sharing) => sharing.role === 'credential:owner')?.projectId; } diff --git a/packages/cli/src/modules/n8n-packages/entities/credential/credential.types.ts b/packages/cli/src/modules/n8n-packages/entities/credential/credential.types.ts index f625906ecfd..0be3497f3f0 100644 --- a/packages/cli/src/modules/n8n-packages/entities/credential/credential.types.ts +++ b/packages/cli/src/modules/n8n-packages/entities/credential/credential.types.ts @@ -1,11 +1,18 @@ import type { ImportBindingMap, + CredentialConflictPolicy, CredentialMatchingMode, CredentialMissingMode, } from '../../n8n-packages.types'; -import type { PackageCredentialRequirement } from '../../spec/requirements.schema'; +import type { + PackageCredentialRequirement, + PackageRequirementConsumer, +} from '../../spec/requirements.schema'; +import type { + SerializedCredential, + SerializedCredentialData, +} from '../../spec/serialized/credential.schema'; import type { AgentRequirementSource } from '../requirement-source'; -import type { SerializedCredentialData } from '../../spec/serialized/credential.schema'; /** A requirement enriched with the expression data the package bundles for it, when any. */ export type PlacedCredentialRequirement = PackageCredentialRequirement & { @@ -50,6 +57,62 @@ export interface CredentialResolution { failures: CredentialResolutionFailure[]; } +/** A package credential and the destination credential it resolves to. */ +export interface CredentialMatch { + sourceId: string; + targetId: string; + /** Destination credential name, for reporting. */ + targetName: string; + usedBy: PackageRequirementConsumer[]; +} + +/** + * A matched destination credential that the conflict policy can update, with the package + * payload for it. Holds no stored values: the write decrypts and merges again. + */ +export interface CredentialUpdateCandidate extends CredentialMatch { + /** Package credential name. */ + name: string; + /** Package expression values. Absent when the package was exported without values. */ + data?: SerializedCredentialData; +} + +export interface CredentialImportPlan extends CredentialResolution { + /** Matched credentials the conflict policy can update. Empty under `keep-existing`. */ + candidates: CredentialUpdateCandidate[]; + /** The candidates that `overwrite` would change. */ + updates: CredentialUpdateCandidate[]; + /** Matches in the update scope that the user cannot update. Empty under `keep-existing`. */ + forbidden: CredentialMatch[]; +} + +/** + * - `differs`: under `fail`, the package would change the destination credential. + * - `divergent`: under `overwrite`, two or more package credentials with different + * payloads resolve to one destination credential. + */ +export interface CredentialConflict { + kind: 'differs' | 'divergent'; + targetId: string; + sourceIds: string[]; + /** Destination credential name. */ + name: string; + usedBy: PackageRequirementConsumer[]; +} + +/** + * A destination credential in the update scope that the importing user cannot update. + * Blocks under `overwrite` and `fail`, also when it would not change: its stored values are + * never read. + */ +export type CredentialUpdateForbidden = Omit; + +/** Which matched destination credentials the conflict policy can update. */ +export interface CredentialUpdateScope { + /** Destination projects of the import. A credential owned by one of them is in scope. */ + projectIds: ReadonlySet; +} + export interface CredentialApplyResult { bindings: ImportBindingMap; matched: string[]; @@ -62,6 +125,10 @@ export interface CredentialBindingRequest { matchingMode: CredentialMatchingMode; missingMode: CredentialMissingMode; credentialBindings?: ImportBindingMap; + conflictPolicy: CredentialConflictPolicy; + /** Package credential files by credential id. Read only when the policy is not `keep-existing`. */ + packageCredentials?: ReadonlyMap; + updateScope: CredentialUpdateScope; } export function createFailure( diff --git a/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts b/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts index bb648607058..6a950ce4ddb 100644 --- a/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts +++ b/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts @@ -104,6 +104,7 @@ const CHERRY_PICK_IMPORT_POLICY = { missingNodeTypeMode: 'fail', credentialMatchingMode: 'id-only', credentialMissingMode: 'must-preexist', + credentialConflictPolicy: 'keep-existing', dataTableMatchingMode: 'by-id', dataTableMissingMode: 'create', variableMissingMode: 'must-preexist', diff --git a/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts b/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts index 66fedac8fe3..cfe43d971a4 100644 --- a/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts +++ b/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts @@ -2,6 +2,10 @@ import type { PolicyViolation } from '@n8n/api-types'; import type { User } from '@n8n/db'; import type { Readable } from 'node:stream'; +import type { + CredentialConflict, + CredentialUpdateForbidden, +} from './entities/credential/credential.types'; import type { DataTableResolutionFailure } from './entities/data-table/data-table.types'; import type { TagResolutionFailure } from './entities/tag/tag.types'; import type { @@ -166,6 +170,15 @@ export const VariableConflictPolicy = { Fail: 'fail', } as const; +export const CredentialConflictPolicy = { + /** Does not change a matched credential, also when the package has a different name or different values. */ + KeepExisting: 'keep-existing', + /** Writes the package name and expression values to a matched credential. Keeps the other stored values. */ + Overwrite: 'overwrite', + /** Blocks the import when `overwrite` would change a matched credential. */ + Fail: 'fail', +} as const; + export const VariableParentPolicy = { Project: 'project', Global: 'global', @@ -224,6 +237,9 @@ export type VariableMissingMode = (typeof VariableMissingMode)[keyof typeof Vari export type VariableConflictPolicy = (typeof VariableConflictPolicy)[keyof typeof VariableConflictPolicy]; +export type CredentialConflictPolicy = + (typeof CredentialConflictPolicy)[keyof typeof CredentialConflictPolicy]; + export type VariableParentPolicy = (typeof VariableParentPolicy)[keyof typeof VariableParentPolicy]; export type TagMissingMode = (typeof TagMissingMode)[keyof typeof TagMissingMode]; @@ -314,6 +330,8 @@ export type ImportPackageSelectionRequest = ImportSelectionRequest & { export type ImportCredentialProperties = { credentialMatchingMode: CredentialMatchingMode; credentialMissingMode: CredentialMissingMode; + /** Omitted means `keep-existing`. */ + credentialConflictPolicy?: CredentialConflictPolicy; }; export type ImportWorkflowProperties = { @@ -379,7 +397,7 @@ export interface ImportContext { folderId: string | null; } -export type ImportPackageEventOptions = ImportCredentialProperties & +export type ImportPackageEventOptions = Required & ImportWorkflowProperties & ImportProjectProperties & ResolvedImportFolderProperties & @@ -563,6 +581,8 @@ export type BlockingIssue = actualType?: string; usedBy: PackageRequirementConsumer[]; } + | ({ type: 'credential-conflict' } & CredentialConflict) + | ({ type: 'credential-update-forbidden' } & CredentialUpdateForbidden) | ({ type: 'project-conflict' } & ProjectConflict) | ({ type: 'folder-conflict' } & FolderConflict) | ({ type: 'workflow-removal-forbidden' } & WorkflowRemovalFailure) diff --git a/packages/cli/src/modules/promotions.ee/__tests__/promotions.service.test.ts b/packages/cli/src/modules/promotions.ee/__tests__/promotions.service.test.ts index 792e2697253..353d3d31f4a 100644 --- a/packages/cli/src/modules/promotions.ee/__tests__/promotions.service.test.ts +++ b/packages/cli/src/modules/promotions.ee/__tests__/promotions.service.test.ts @@ -1237,6 +1237,7 @@ describe('PromotionsService', () => { missingNodeTypeMode: 'fail', credentialMatchingMode: 'id-only', credentialMissingMode: 'must-preexist', + credentialConflictPolicy: 'keep-existing', folderConflictPolicy: 'overwrite', overwriteDeletionPolicy: 'hard-delete', dataTableMatchingMode: 'by-id', diff --git a/packages/cli/src/modules/promotions.ee/promotions.service.ts b/packages/cli/src/modules/promotions.ee/promotions.service.ts index e008784902a..c1846b58329 100644 --- a/packages/cli/src/modules/promotions.ee/promotions.service.ts +++ b/packages/cli/src/modules/promotions.ee/promotions.service.ts @@ -29,6 +29,7 @@ import { PackageImportConfig } from '@/modules/n8n-packages/n8n-packages.config' import { N8nPackagesService } from '@/modules/n8n-packages/n8n-packages.service'; import { MANIFEST_FILE } from '@/modules/n8n-packages/spec/constants'; import { + CredentialConflictPolicy, DataTableMissingMode, DataTableSchemaConflictPolicy, ExportDependencyPolicy, @@ -93,6 +94,7 @@ const IMPORT_POLICY: Omit = { missingNodeTypeMode: MissingNodeTypeMode.Fail, credentialMatchingMode: 'id-only', credentialMissingMode: 'must-preexist', + credentialConflictPolicy: CredentialConflictPolicy.KeepExisting, folderConflictPolicy: FolderConflictPolicy.Overwrite, overwriteDeletionPolicy: OverwriteDeletionPolicy.HardDelete, dataTableMatchingMode: 'by-id', diff --git a/packages/cli/src/modules/source-control.ee/source-control-helper.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-helper.ee.ts index 4afcd8f8d59..d0d48de1b23 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-helper.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-helper.ee.ts @@ -9,7 +9,6 @@ import { deepCopy, jsonParse, UserError, - type CredentialInformation, type DataTableColumnType, type ICredentialDataDecryptedObject, } from 'n8n-workflow'; @@ -17,6 +16,7 @@ import { ok } from 'node:assert/strict'; import { readFile as fsReadFile } from 'node:fs/promises'; import path from 'path'; +import { mergeCredentialData } from '@/credentials/credential-data-merge'; import { License } from '@/license'; import { generateSshKeyPair as generateGitSshKeyPair } from '@/modules/promotions.ee/promotions-git.utils'; import { containsExpression } from '@/utils'; @@ -63,60 +63,6 @@ export function sanitizeCredentialData( return result; } -function isPlainObject(value: unknown): value is Record { - return value !== null && typeof value === 'object' && !Array.isArray(value); -} - -/** - * Recursively merges a single value based on its type. - * Handles strings, numbers, booleans, arrays, and plain objects. - */ -function mergeSingleValue(sanitizedRemoteValue: unknown, localValue: unknown): unknown { - if (typeof sanitizedRemoteValue === 'string') { - if (containsExpression(sanitizedRemoteValue)) { - return sanitizedRemoteValue; - } else if (localValue !== undefined && localValue !== null) { - // Local value is preserved if it exists (secret handling) - return localValue; - } - - // The remote field exists as an empty string (key is part of the schema) - // but local has no value for it. Preserve the empty string so the field - // is not silently dropped from the merged credential. - return ''; - } - - if (typeof sanitizedRemoteValue === 'number' || typeof sanitizedRemoteValue === 'boolean') { - return sanitizedRemoteValue; - } - - if (Array.isArray(sanitizedRemoteValue)) { - // Only merge by index if lengths match, otherwise array structure has changed - // and we can't reliably match items (could be additions/removals/reordering) - if (Array.isArray(localValue) && localValue.length === sanitizedRemoteValue.length) { - return sanitizedRemoteValue.map((sanitizedItem, index) => { - const localItem = localValue[index]; - return mergeSingleValue(sanitizedItem, localItem); - }); - } - - return sanitizedRemoteValue; - } - - if (isPlainObject(sanitizedRemoteValue)) { - if (isPlainObject(localValue)) { - return mergeRemoteCrendetialDataIntoLocalCredentialData({ - local: localValue as ICredentialDataDecryptedObject, - remote: sanitizedRemoteValue as ICredentialDataDecryptedObject, - }); - } - - return sanitizedRemoteValue; - } - - return undefined; -} - /** * Merges remote credential data into local data. * Remote expressions, numbers and boolean values overwrite local values. @@ -128,31 +74,11 @@ export function mergeRemoteCrendetialDataIntoLocalCredentialData({ local: ICredentialDataDecryptedObject; remote: ICredentialDataDecryptedObject; }): ICredentialDataDecryptedObject { - const merged: ICredentialDataDecryptedObject = {}; - - const sanitizedRemote = sanitizeCredentialData(remote); - - for (const [key, sanitizedRemoteValue] of Object.entries(sanitizedRemote)) { - const localValue = local[key]; - const mergedValue = mergeSingleValue(sanitizedRemoteValue, localValue); - - if (mergedValue !== undefined) { - merged[key] = mergedValue as CredentialInformation; - } - } - - // Keep local fields the remote stub does not carry. A field left at its default value - // is not persisted, so it never reaches the stub; an absent field carries the same - // "no value to give" meaning as a present-but-blank one, which is already preserved - // above. Without this it would be dropped on pull and reset to its default. This also - // covers oauthTokenData, which sanitization always strips from the remote. - for (const [key, localValue] of Object.entries(local)) { - if (!(key in sanitizedRemote)) { - merged[key] = localValue; - } - } - - return merged; + return mergeCredentialData({ + existing: local, + incoming: sanitizeCredentialData(remote), + arrays: 'merge-by-index', + }); } export function getWorkflowExportPath(workflowId: string, workflowExportFolder: string): string { diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/importBlockingIssue.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/importBlockingIssue.generated.yml index 1b5a5f485b2..8ba969d2e7a 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/importBlockingIssue.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/importBlockingIssue.generated.yml @@ -165,6 +165,71 @@ oneOf: - sourceId - usedBy description: A credential reference that could not be resolved in the target project. + - type: object + properties: + type: + type: string + enum: + - credential-conflict + kind: + type: string + enum: + - differs + - divergent + description: '`differs`: under `credentialConflictPolicy=fail`, the package would change the name or values of the destination credential. `divergent`: under `overwrite`, two or more package credentials with different names or values resolve to one destination credential. The import does not select one of them.' + targetId: + type: string + description: Id of the destination credential. + sourceIds: + type: array + items: + type: string + description: Ids of the package credentials that resolve to the destination credential. For `differs`, the package credentials that would change it. + name: + type: string + description: Name of the destination credential. + usedBy: + type: array + items: + $ref: packageRequirementConsumer.generated.yml + description: Package consumers that use the listed package credentials. + required: + - type + - kind + - targetId + - sourceIds + - name + - usedBy + description: 'A matched credential that the credential conflict policy does not let the import change. Only a credential that the import can update is checked: a credential owned by a project of the import. Values are never reported.' + - type: object + properties: + type: + type: string + enum: + - credential-update-forbidden + targetId: + type: string + description: Id of the destination credential. + sourceIds: + type: array + items: + type: string + description: Ids of the package credentials that resolve to it. + name: + type: string + description: Name of the destination credential. + usedBy: + type: array + items: + $ref: packageRequirementConsumer.generated.yml + description: Package consumers that use the listed package credentials. + required: + - type + - targetId + - sourceIds + - name + - usedBy + description: A matched credential that the import can update, but the caller has no `credential:update` permission on it. Only under `credentialConflictPolicy` `overwrite` or `fail`. The import does not read its values, so this blocks also when the package would not change it. Use `keep-existing` to bind it without a change. - type: object properties: type: