From 910a1aa0a60ded8a31c7640e3c73fd48f834a5c3 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Wed, 10 Apr 2024 14:33:00 +0200 Subject: [PATCH 01/10] don't allow to create multiple owners for the same credential --- .../cli/src/commands/import/credentials.ts | 56 +++- .../commands/credentials.cmd.test.ts | 304 +++++++++++++++++- .../credentials-updated.json | 14 + .../separate-updated/separate-credential.json | 12 + .../separate/separate-credential.json | 12 + .../test/integration/shared/db/credentials.ts | 4 + 6 files changed, 392 insertions(+), 10 deletions(-) create mode 100644 packages/cli/test/integration/commands/importCredentials/credentials-updated.json create mode 100644 packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json create mode 100644 packages/cli/test/integration/commands/importCredentials/separate/separate-credential.json diff --git a/packages/cli/src/commands/import/credentials.ts b/packages/cli/src/commands/import/credentials.ts index 2ff971b2d2826..fc8f526bfc8bb 100644 --- a/packages/cli/src/commands/import/credentials.ts +++ b/packages/cli/src/commands/import/credentials.ts @@ -15,6 +15,7 @@ import type { ICredentialsEncrypted } from 'n8n-workflow'; import { ApplicationError, jsonParse } from 'n8n-workflow'; import { UM_FIX_INSTRUCTION } from '@/constants'; import { UserRepository } from '@db/repositories/user.repository'; +import { SharedCredentialsRepository } from '@/databases/repositories/sharedCredentials.repository'; export class ImportCredentialsCommand extends BaseCommand { static description = 'Import credentials'; @@ -89,6 +90,17 @@ export class ImportCredentialsCommand extends BaseCommand { const credential = jsonParse( fs.readFileSync(file, { encoding: 'utf8' }), ); + + if (credential.id && flags.userId) { + const credentialOwner = await this.getCredentialOwner(credential.id); + + if (credentialOwner && credentialOwner !== flags.userId) { + throw new ApplicationError( + `The credential with id "${credential.id}" is already owned by the user with the id "${credentialOwner}". It can't be re-owned by the user with the id "${flags.userId}"`, + ); + } + } + if (typeof credential.data === 'object') { // plain data / decrypted input. Should be encrypted first. credential.data = cipher.encrypt(credential.data); @@ -116,6 +128,15 @@ export class ImportCredentialsCommand extends BaseCommand { await Db.getConnection().transaction(async (transactionManager) => { this.transactionManager = transactionManager; for (const credential of credentials) { + if (credential.id && flags.userId) { + const credentialOwner = await this.getCredentialOwner(credential.id); + if (credentialOwner && credentialOwner !== flags.userId) { + throw new ApplicationError( + `The credential with id "${credential.id}" is already owned by the user with the id "${credentialOwner}". It can't be re-owned by the user with the id "${flags.userId}"`, + ); + } + } + if (typeof credential.data === 'object') { // plain data / decrypted input. Should be encrypted first. credential.data = cipher.encrypt(credential.data); @@ -142,15 +163,23 @@ export class ImportCredentialsCommand extends BaseCommand { private async storeCredential(credential: Partial, user: User) { const result = await this.transactionManager.upsert(CredentialsEntity, credential, ['id']); - await this.transactionManager.upsert( - SharedCredentials, - { - credentialsId: result.identifiers[0].id as string, - userId: user.id, - role: 'credential:owner', - }, - ['credentialsId', 'userId'], - ); + + const sharingExists = await this.transactionManager.existsBy(SharedCredentials, { + credentialsId: credential.id, + role: 'credential:owner', + }); + + if (!sharingExists) { + await this.transactionManager.upsert( + SharedCredentials, + { + credentialsId: result.identifiers[0].id as string, + userId: user.id, + role: 'credential:owner', + }, + ['credentialsId', 'userId'], + ); + } } private async getOwner() { @@ -171,4 +200,13 @@ export class ImportCredentialsCommand extends BaseCommand { return user; } + + private async getCredentialOwner(credentialsId: string) { + const sharedCredential = await Container.get(SharedCredentialsRepository).findOneBy({ + credentialsId, + role: 'credential:owner', + }); + + return sharedCredential?.userId; + } } diff --git a/packages/cli/test/integration/commands/credentials.cmd.test.ts b/packages/cli/test/integration/commands/credentials.cmd.test.ts index 1ec68f72633cd..db278d7cb0cdf 100644 --- a/packages/cli/test/integration/commands/credentials.cmd.test.ts +++ b/packages/cli/test/integration/commands/credentials.cmd.test.ts @@ -6,7 +6,8 @@ import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import { mockInstance } from '../../shared/mocking'; import * as testDb from '../shared/testDb'; -import { getAllCredentials } from '../shared/db/credentials'; +import { getAllCredentials, getAllSharedCredentials } from '../shared/db/credentials'; +import { createMember, createOwner } from '../shared/db/users'; const oclifConfig = new Config({ root: __dirname }); @@ -47,3 +48,304 @@ test('import:credentials should import a credential', async () => { expect(after[0].id).toBe('123'); mockExit.mockRestore(); }); + +test('`import:credentials --userId ...` should fail if the credential is exists already and is owned by somebody else', async () => { + // + // ARRANGE + // + const owner = await createOwner(); + const member = await createMember(); + + // import credential the first time, assigning it to the owner + const importer1 = new ImportCredentialsCommand( + [ + '--input=./test/integration/commands/importCredentials/credentials.json', + `--userId=${owner.id}`, + ], + oclifConfig, + ); + await importer1.init(); + await importer1.run(); + + // making sure the import worked + const before = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + expect(before).toMatchObject({ + credentials: [expect.objectContaining({ id: '123', name: 'cred-aws-test' })], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: owner.id, + role: 'credential:owner', + }), + ], + }); + + // Prepare the second import, while updating the name we try to assign the + // credential to another user. + const importer2 = new ImportCredentialsCommand( + [ + '--input=./test/integration/commands/importCredentials/credentials-updated.json', + `--userId=${member.id}`, + ], + oclifConfig, + ); + await importer2.init(); + + // + // ACT + // + await expect(importer2.run()).rejects.toThrowError( + `The credential with id "123" is already owned by the user with the id "${owner.id}". It can't be re-owned by the user with the id "${member.id}"`, + ); + + // + // ASSERT + // + const after = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + + expect(after).toMatchObject({ + credentials: [ + expect.objectContaining({ + id: '123', + // only the name was updated + name: 'cred-aws-test', + }), + ], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: owner.id, + role: 'credential:owner', + }), + ], + }); +}); + +test('`import:credentials --separate --userId ...` should fail if the credential is exists already and is owned by somebody else', async () => { + // + // ARRANGE + // + const owner = await createOwner(); + const member = await createMember(); + + // import credential the first time, assigning it to the owner + const importer1 = new ImportCredentialsCommand( + [ + '--separate', + '--input=./test/integration/commands/importCredentials/separate', + `--userId=${owner.id}`, + ], + oclifConfig, + ); + await importer1.init(); + await importer1.run(); + + // making sure the import worked + const before = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + expect(before).toMatchObject({ + credentials: [expect.objectContaining({ id: '123', name: 'cred-aws-test' })], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: owner.id, + role: 'credential:owner', + }), + ], + }); + + // Prepare the second import, while updating the name we try to assign the + // credential to another user. + const importer2 = new ImportCredentialsCommand( + [ + '--separate', + '--input=./test/integration/commands/importCredentials/separate-updated', + `--userId=${member.id}`, + ], + oclifConfig, + ); + await importer2.init(); + + // + // ACT + // + await expect(importer2.run()).rejects.toThrowError( + `The credential with id "123" is already owned by the user with the id "${owner.id}". It can't be re-owned by the user with the id "${member.id}"`, + ); + + // + // ASSERT + // + const after = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + + // nothing should have changed + expect(after).toMatchObject({ + credentials: [ + expect.objectContaining({ + id: '123', + name: 'cred-aws-test', + }), + ], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: owner.id, + role: 'credential:owner', + }), + ], + }); +}); + +test("only update credential, don't create or update owner if `--userId` is not passed, while using `--separate`", async () => { + // + // ARRANGE + // + const member = await createMember(); + + // import credential the first time, assigning it to a member + const importer1 = new ImportCredentialsCommand( + [ + '--separate', + '--input=./test/integration/commands/importCredentials/separate', + `--userId=${member.id}`, + ], + oclifConfig, + ); + await importer1.init(); + await importer1.run(); + + // making sure the import worked + const before = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + expect(before).toMatchObject({ + credentials: [expect.objectContaining({ id: '123', name: 'cred-aws-test' })], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: member.id, + role: 'credential:owner', + }), + ], + }); + + // prepare the second import, trying to assign the same credential to another user + const importer2 = new ImportCredentialsCommand( + ['--separate', '--input=./test/integration/commands/importCredentials/separate-updated'], + oclifConfig, + ); + await importer2.init(); + + // + // ACT + // + await importer2.run(); + + // + // ASSERT + // + const after = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + + expect(after).toMatchObject({ + credentials: [ + expect.objectContaining({ + id: '123', + // only the name was updated + name: 'cred-aws-prod', + }), + ], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: member.id, + role: 'credential:owner', + }), + ], + }); +}); + +test("only update credential, don't create or update owner if `--userId` is not passed", async () => { + // + // ARRANGE + // + const member = await createMember(); + + // import credential the first time, assigning it to a member + const importer1 = new ImportCredentialsCommand( + [ + '--input=./test/integration/commands/importCredentials/credentials.json', + `--userId=${member.id}`, + ], + oclifConfig, + ); + await importer1.init(); + await importer1.run(); + + // making sure the import worked + const before = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + expect(before).toMatchObject({ + credentials: [expect.objectContaining({ id: '123', name: 'cred-aws-test' })], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: member.id, + role: 'credential:owner', + }), + ], + }); + + // prepare the second import, only updating the name and omitting `--userId` + const importer2 = new ImportCredentialsCommand( + ['--input=./test/integration/commands/importCredentials/credentials-updated.json'], + oclifConfig, + ); + await importer2.init(); + + // + // ACT + // + await importer2.run(); + + // + // ASSERT + // + const after = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + + expect(after).toMatchObject({ + credentials: [ + expect.objectContaining({ + id: '123', + // only the name was updated + name: 'cred-aws-prod', + }), + ], + sharings: [ + expect.objectContaining({ + credentialsId: '123', + userId: member.id, + role: 'credential:owner', + }), + ], + }); +}); diff --git a/packages/cli/test/integration/commands/importCredentials/credentials-updated.json b/packages/cli/test/integration/commands/importCredentials/credentials-updated.json new file mode 100644 index 0000000000000..67fad38ef79de --- /dev/null +++ b/packages/cli/test/integration/commands/importCredentials/credentials-updated.json @@ -0,0 +1,14 @@ +[ + { + "createdAt": "2023-07-10T14:50:49.193Z", + "updatedAt": "2023-10-27T13:34:42.917Z", + "id": "123", + "name": "cred-aws-prod", + "data": { + "region": "eu-west-1", + "accessKeyId": "999999999999", + "secretAccessKey": "aaaaaaaaaaaaa" + }, + "type": "aws" + } +] diff --git a/packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json b/packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json new file mode 100644 index 0000000000000..bbb275debc7d4 --- /dev/null +++ b/packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json @@ -0,0 +1,12 @@ +{ + "createdAt": "2023-07-10T14:50:49.193Z", + "updatedAt": "2023-10-27T13:34:42.917Z", + "id": "123", + "name": "cred-aws-prod", + "data": { + "region": "eu-west-1", + "accessKeyId": "999999999999", + "secretAccessKey": "aaaaaaaaaaaaa" + }, + "type": "aws" +} diff --git a/packages/cli/test/integration/commands/importCredentials/separate/separate-credential.json b/packages/cli/test/integration/commands/importCredentials/separate/separate-credential.json new file mode 100644 index 0000000000000..24ce8467ed757 --- /dev/null +++ b/packages/cli/test/integration/commands/importCredentials/separate/separate-credential.json @@ -0,0 +1,12 @@ +{ + "createdAt": "2023-07-10T14:50:49.193Z", + "updatedAt": "2023-10-27T13:34:42.917Z", + "id": "123", + "name": "cred-aws-test", + "data": { + "region": "eu-west-1", + "accessKeyId": "999999999999", + "secretAccessKey": "aaaaaaaaaaaaa" + }, + "type": "aws" +} diff --git a/packages/cli/test/integration/shared/db/credentials.ts b/packages/cli/test/integration/shared/db/credentials.ts index 4e77b2fcc2322..85b46d26a3e6d 100644 --- a/packages/cli/test/integration/shared/db/credentials.ts +++ b/packages/cli/test/integration/shared/db/credentials.ts @@ -91,3 +91,7 @@ export async function getAllCredentials() { export const getCredentialById = async (id: string) => await Container.get(CredentialsRepository).findOneBy({ id }); + +export async function getAllSharedCredentials() { + return await Container.get(SharedCredentialsRepository).find(); +} From acb86712b52eaa766daf917d0ea13296290ee407 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 09:51:00 +0200 Subject: [PATCH 02/10] fix grammar --- .../cli/test/integration/commands/credentials.cmd.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/cli/test/integration/commands/credentials.cmd.test.ts b/packages/cli/test/integration/commands/credentials.cmd.test.ts index db278d7cb0cdf..5d9febce1cfc8 100644 --- a/packages/cli/test/integration/commands/credentials.cmd.test.ts +++ b/packages/cli/test/integration/commands/credentials.cmd.test.ts @@ -49,7 +49,7 @@ test('import:credentials should import a credential', async () => { mockExit.mockRestore(); }); -test('`import:credentials --userId ...` should fail if the credential is exists already and is owned by somebody else', async () => { +test('`import:credentials --userId ...` should fail if the credential exists already and is owned by somebody else', async () => { // // ARRANGE // @@ -127,7 +127,7 @@ test('`import:credentials --userId ...` should fail if the credential is exists }); }); -test('`import:credentials --separate --userId ...` should fail if the credential is exists already and is owned by somebody else', async () => { +test('`import:credentials --separate --userId ...` should fail if the credential exists already and is owned by somebody else', async () => { // // ARRANGE // @@ -207,7 +207,7 @@ test('`import:credentials --separate --userId ...` should fail if the credential }); }); -test("only update credential, don't create or update owner if `--userId` is not passed, while using `--separate`", async () => { +test("only update the credential, don't create or update the owner if `--userId` is not passed, while using `--separate`", async () => { // // ARRANGE // From 4be19412ba5bf63c04c7d0690eebe3393542d38c Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 09:51:33 +0200 Subject: [PATCH 03/10] don't allow to create multiple owners for the same workflow --- packages/cli/src/commands/import/workflow.ts | 124 ++++++++++------- packages/cli/src/services/import.service.ts | 19 ++- .../integration/commands/import.cmd.test.ts | 128 +++++++++++++++++- .../combined-with-update/original.json | 81 +++++++++++ .../combined-with-update/updated.json | 81 +++++++++++ .../test/integration/import.service.test.ts | 13 ++ .../test/integration/shared/db/workflows.ts | 4 + 7 files changed, 392 insertions(+), 58 deletions(-) create mode 100644 packages/cli/test/integration/commands/importWorkflows/combined-with-update/original.json create mode 100644 packages/cli/test/integration/commands/importWorkflows/combined-with-update/updated.json diff --git a/packages/cli/src/commands/import/workflow.ts b/packages/cli/src/commands/import/workflow.ts index 21c3d82501cc4..149e39e54798a 100644 --- a/packages/cli/src/commands/import/workflow.ts +++ b/packages/cli/src/commands/import/workflow.ts @@ -4,15 +4,14 @@ import { ApplicationError, jsonParse } from 'n8n-workflow'; import fs from 'fs'; import glob from 'fast-glob'; -import { UM_FIX_INSTRUCTION } from '@/constants'; import { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { disableAutoGeneratedIds } from '@db/utils/commandHelpers'; import { generateNanoId } from '@db/utils/generators'; -import { UserRepository } from '@db/repositories/user.repository'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import type { IWorkflowToImport } from '@/Interfaces'; import { ImportService } from '@/services/import.service'; import { BaseCommand } from '../BaseCommand'; +import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflow.repository'; function assertHasWorkflowsToImport(workflows: unknown): asserts workflows is IWorkflowToImport[] { if (!Array.isArray(workflows)) { @@ -78,53 +77,95 @@ export class ImportWorkflowsCommand extends BaseCommand { } } - const user = flags.userId ? await this.getAssignee(flags.userId) : await this.getOwner(); + const workflows = await this.readWorkflows(flags.input, flags.separate); - let totalImported = 0; + const result = await this.checkRelations(workflows, flags.userId); + if (!result.success) { + throw new ApplicationError(result.message); + } - if (flags.separate) { - let { input: inputPath } = flags; + this.logger.info(`Importing ${workflows.length} workflows...`); - if (process.platform === 'win32') { - inputPath = inputPath.replace(/\\/g, '/'); - } + await Container.get(ImportService).importWorkflows(workflows, flags.userId); - const files = await glob('*.json', { - cwd: inputPath, - absolute: true, - }); + this.reportSuccess(workflows.length); + } - totalImported = files.length; - this.logger.info(`Importing ${totalImported} workflows...`); + private async checkRelations(workflows: WorkflowEntity[], userId: string | undefined) { + if (!userId) { + return { + success: true as const, + message: undefined, + }; + } - for (const file of files) { - const workflow = jsonParse(fs.readFileSync(file, { encoding: 'utf8' })); - if (!workflow.id) { - workflow.id = generateNanoId(); - } + for (const workflow of workflows) { + if (!(await this.workflowExists(workflow))) { + continue; + } - const _workflow = Container.get(WorkflowRepository).create(workflow); + const ownerId = await this.getWorkflowOwner(workflow); + if (!ownerId) { + continue; + } - await Container.get(ImportService).importWorkflows([_workflow], user.id); + if (ownerId !== userId) { + return { + success: false as const, + message: `The credential with id "${workflow.id}" is already owned by the user with the id "${ownerId}". It can't be re-owned by the user with the id "${userId}"`, + }; } + } + + return { + success: true as const, + message: undefined, + }; + } + + private async getWorkflowOwner(workflow: WorkflowEntity) { + const sharing = await Container.get(SharedWorkflowRepository).findOneBy({ + workflowId: workflow.id, + role: 'workflow:owner', + }); - this.reportSuccess(totalImported); - process.exit(); + return sharing?.userId; + } + + private async workflowExists(workflow: WorkflowEntity) { + return await Container.get(WorkflowRepository).existsBy({ id: workflow.id }); + } + + private async readWorkflows(path: string, separate: boolean): Promise { + if (process.platform === 'win32') { + path = path.replace(/\\/g, '/'); } - const workflows = jsonParse( - fs.readFileSync(flags.input, { encoding: 'utf8' }), - ); + if (separate) { + const files = await glob('*.json', { + cwd: path, + absolute: true, + }); + const workflowInstances = files.map((file) => { + const workflow = jsonParse(fs.readFileSync(file, { encoding: 'utf8' })); + if (!workflow.id) { + workflow.id = generateNanoId(); + } - const _workflows = workflows.map((w) => Container.get(WorkflowRepository).create(w)); + const workflowInstance = Container.get(WorkflowRepository).create(workflow); - assertHasWorkflowsToImport(workflows); + return workflowInstance; + }); - totalImported = workflows.length; + return workflowInstances; + } else { + const workflows = jsonParse(fs.readFileSync(path, { encoding: 'utf8' })); - await Container.get(ImportService).importWorkflows(_workflows, user.id); + const workflowInstances = workflows.map((w) => Container.get(WorkflowRepository).create(w)); + assertHasWorkflowsToImport(workflows); - this.reportSuccess(totalImported); + return workflowInstances; + } } async catch(error: Error) { @@ -135,23 +176,4 @@ export class ImportWorkflowsCommand extends BaseCommand { private reportSuccess(total: number) { this.logger.info(`Successfully imported ${total} ${total === 1 ? 'workflow.' : 'workflows.'}`); } - - private async getOwner() { - const owner = await Container.get(UserRepository).findOneBy({ role: 'global:owner' }); - if (!owner) { - throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); - } - - return owner; - } - - private async getAssignee(userId: string) { - const user = await Container.get(UserRepository).findOneBy({ id: userId }); - - if (!user) { - throw new ApplicationError('Failed to find user', { extra: { userId } }); - } - - return user; - } } diff --git a/packages/cli/src/services/import.service.ts b/packages/cli/src/services/import.service.ts index 32f6894f9b844..db6b976970abf 100644 --- a/packages/cli/src/services/import.service.ts +++ b/packages/cli/src/services/import.service.ts @@ -1,6 +1,6 @@ import { Service } from 'typedi'; import { v4 as uuid } from 'uuid'; -import { type INode, type INodeCredentialsDetails } from 'n8n-workflow'; +import { ApplicationError, type INode, type INodeCredentialsDetails } from 'n8n-workflow'; import { Logger } from '@/Logger'; import * as Db from '@/Db'; @@ -12,6 +12,7 @@ import { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { WorkflowTagMapping } from '@db/entities/WorkflowTagMapping'; import type { TagEntity } from '@db/entities/TagEntity'; import type { ICredentialsDb } from '@/Interfaces'; +import { User } from '@/databases/entities/User'; @Service() export class ImportService { @@ -30,7 +31,7 @@ export class ImportService { this.dbTags = await this.tagRepository.find(); } - async importWorkflows(workflows: WorkflowEntity[], userId: string) { + async importWorkflows(workflows: WorkflowEntity[], userId?: string) { await this.initRecords(); for (const workflow of workflows) { @@ -57,10 +58,16 @@ export class ImportService { const workflowId = upsertResult.identifiers.at(0)?.id as string; - await tx.upsert(SharedWorkflow, { workflowId, userId, role: 'workflow:owner' }, [ - 'workflowId', - 'userId', - ]); + if (userId) { + if (!(await tx.existsBy(User, { id: userId }))) { + throw new ApplicationError('Failed to find user', { extra: { userId } }); + } + + await tx.upsert(SharedWorkflow, { workflowId, userId, role: 'workflow:owner' }, [ + 'workflowId', + 'userId', + ]); + } if (!workflow.tags?.length) continue; diff --git a/packages/cli/test/integration/commands/import.cmd.test.ts b/packages/cli/test/integration/commands/import.cmd.test.ts index 211fde564156e..4851ecf0f9e05 100644 --- a/packages/cli/test/integration/commands/import.cmd.test.ts +++ b/packages/cli/test/integration/commands/import.cmd.test.ts @@ -6,7 +6,8 @@ import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import { mockInstance } from '../../shared/mocking'; import * as testDb from '../shared/testDb'; -import { getAllWorkflows } from '../shared/db/workflows'; +import { getAllSharedWorkflows, getAllWorkflows } from '../shared/db/workflows'; +import { createMember, createOwner } from '../shared/db/users'; const oclifConfig = new Config({ root: __dirname }); @@ -75,3 +76,128 @@ test('import:workflow should import active workflow from combined file and deact expect(after[1].active).toBe(false); mockExit.mockRestore(); }); + +async function importWorkflow(argv: string[]) { + const importer = new ImportWorkflowsCommand(argv, oclifConfig); + await importer.init(); + await importer.run(); +} + +test('`import:workflow --userId ...` should fail if the workflow exists already and is owned by somebody else', async () => { + // + // ARRANGE + // + const owner = await createOwner(); + const member = await createMember(); + + // Import workflow the first time, assigning it to a member. + await importWorkflow([ + '--input=./test/integration/commands/importWorkflows/combined-with-update/original.json', + `--userId=${owner.id}`, + ]); + + const before = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + // Make sure the workflow and sharing have been created. + expect(before).toMatchObject({ + workflows: [expect.objectContaining({ id: '998', name: 'active-workflow' })], + sharings: [ + expect.objectContaining({ + workflowId: '998', + userId: owner.id, + role: 'workflow:owner', + }), + ], + }); + + // + // ACT + // + // Import the same workflow again, with another name but the same ID, and try + // to assign it to the member. + await expect( + importWorkflow([ + '--input=./test/integration/commands/importWorkflows/combined-with-update/updated.json', + `--userId=${member.id}`, + ]), + ).rejects.toThrowError( + `The credential with id "998" is already owned by the user with the id "${owner.id}". It can't be re-owned by the user with the id "${member.id}"`, + ); + + // + // ASSERT + // + const after = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + // Make sure there is no new sharing and that the name DID NOT change. + expect(after).toMatchObject({ + workflows: [expect.objectContaining({ id: '998', name: 'active-workflow' })], + sharings: [ + expect.objectContaining({ + workflowId: '998', + userId: owner.id, + role: 'workflow:owner', + }), + ], + }); +}); + +test("only update the workflow, don't create or update the owner if `--userId` is not passed", async () => { + // + // ARRANGE + // + const member = await createMember(); + + // Import workflow the first time, assigning it to a member. + await importWorkflow([ + '--input=./test/integration/commands/importWorkflows/combined-with-update/original.json', + `--userId=${member.id}`, + ]); + + const before = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + // Make sure the workflow and sharing have been created. + expect(before).toMatchObject({ + workflows: [expect.objectContaining({ id: '998', name: 'active-workflow' })], + sharings: [ + expect.objectContaining({ + workflowId: '998', + userId: member.id, + role: 'workflow:owner', + }), + ], + }); + + // + // ACT + // + // Import the same workflow again, with another name but the same ID. + await importWorkflow([ + '--input=./test/integration/commands/importWorkflows/combined-with-update/updated.json', + ]); + + // + // ASSERT + // + const after = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + // Make sure there is no new sharing and that the name changed. + expect(after).toMatchObject({ + workflows: [expect.objectContaining({ id: '998', name: 'active-workflow updated' })], + sharings: [ + expect.objectContaining({ + workflowId: '998', + userId: member.id, + role: 'workflow:owner', + }), + ], + }); +}); diff --git a/packages/cli/test/integration/commands/importWorkflows/combined-with-update/original.json b/packages/cli/test/integration/commands/importWorkflows/combined-with-update/original.json new file mode 100644 index 0000000000000..bbef96a0a9e07 --- /dev/null +++ b/packages/cli/test/integration/commands/importWorkflows/combined-with-update/original.json @@ -0,0 +1,81 @@ +[ + { + "name": "active-workflow", + "nodes": [ + { + "parameters": { + "path": "e20b4873-fcf7-4bce-88fc-a1a56d66b138", + "responseMode": "responseNode", + "options": {} + }, + "id": "c26d8782-bd57-43d0-86dc-0c618a7e4024", + "name": "Webhook", + "type": "n8n-nodes-base.webhook", + "typeVersion": 1, + "position": [800, 580], + "webhookId": "e20b4873-fcf7-4bce-88fc-a1a56d66b138" + }, + { + "parameters": { + "values": { + "boolean": [ + { + "name": "hooked", + "value": true + } + ] + }, + "options": {} + }, + "id": "9701b1ef-9ab0-432a-b086-cf76981b097d", + "name": "Set", + "type": "n8n-nodes-base.set", + "typeVersion": 1, + "position": [1020, 580] + }, + { + "parameters": { + "options": {} + }, + "id": "d0f086b8-c2b2-4404-b347-95d3f91e555a", + "name": "Respond to Webhook", + "type": "n8n-nodes-base.respondToWebhook", + "typeVersion": 1, + "position": [1240, 580] + } + ], + "pinData": {}, + "connections": { + "Webhook": { + "main": [ + [ + { + "node": "Set", + "type": "main", + "index": 0 + } + ] + ] + }, + "Set": { + "main": [ + [ + { + "node": "Respond to Webhook", + "type": "main", + "index": 0 + } + ] + ] + } + }, + "active": true, + "settings": {}, + "versionId": "40a70df1-740f-47e7-8e16-50a0bcd5b70f", + "id": "998", + "meta": { + "instanceId": "95977dc4769098fc608439605527ee75d23f10d551aed6b87a3eea1a252c0ba9" + }, + "tags": [] + } +] diff --git a/packages/cli/test/integration/commands/importWorkflows/combined-with-update/updated.json b/packages/cli/test/integration/commands/importWorkflows/combined-with-update/updated.json new file mode 100644 index 0000000000000..fc1ddbf3ead48 --- /dev/null +++ b/packages/cli/test/integration/commands/importWorkflows/combined-with-update/updated.json @@ -0,0 +1,81 @@ +[ + { + "name": "active-workflow updated", + "nodes": [ + { + "parameters": { + "path": "e20b4873-fcf7-4bce-88fc-a1a56d66b138", + "responseMode": "responseNode", + "options": {} + }, + "id": "c26d8782-bd57-43d0-86dc-0c618a7e4024", + "name": "Webhook", + "type": "n8n-nodes-base.webhook", + "typeVersion": 1, + "position": [800, 580], + "webhookId": "e20b4873-fcf7-4bce-88fc-a1a56d66b138" + }, + { + "parameters": { + "values": { + "boolean": [ + { + "name": "hooked", + "value": true + } + ] + }, + "options": {} + }, + "id": "9701b1ef-9ab0-432a-b086-cf76981b097d", + "name": "Set", + "type": "n8n-nodes-base.set", + "typeVersion": 1, + "position": [1020, 580] + }, + { + "parameters": { + "options": {} + }, + "id": "d0f086b8-c2b2-4404-b347-95d3f91e555a", + "name": "Respond to Webhook", + "type": "n8n-nodes-base.respondToWebhook", + "typeVersion": 1, + "position": [1240, 580] + } + ], + "pinData": {}, + "connections": { + "Webhook": { + "main": [ + [ + { + "node": "Set", + "type": "main", + "index": 0 + } + ] + ] + }, + "Set": { + "main": [ + [ + { + "node": "Respond to Webhook", + "type": "main", + "index": 0 + } + ] + ] + } + }, + "active": true, + "settings": {}, + "versionId": "40a70df1-740f-47e7-8e16-50a0bcd5b70f", + "id": "998", + "meta": { + "instanceId": "95977dc4769098fc608439605527ee75d23f10d551aed6b87a3eea1a252c0ba9" + }, + "tags": [] + } +] diff --git a/packages/cli/test/integration/import.service.test.ts b/packages/cli/test/integration/import.service.test.ts index 4809e58138aeb..1ca70c115e7ab 100644 --- a/packages/cli/test/integration/import.service.test.ts +++ b/packages/cli/test/integration/import.service.test.ts @@ -68,6 +68,19 @@ describe('ImportService', () => { expect(dbSharing.userId).toBe(owner.id); }); + test('should not create a new owner if `userId` is not passed in', async () => { + const workflowToImport = await createWorkflow(); + + await importService.importWorkflows([workflowToImport]); + + const dbSharing = await Container.get(SharedWorkflowRepository).findOneBy({ + workflowId: workflowToImport.id, + role: 'workflow:owner', + }); + + expect(dbSharing).toBeNull(); + }); + test('should deactivate imported workflow if active', async () => { const workflowToImport = await createWorkflow({ active: true }); diff --git a/packages/cli/test/integration/shared/db/workflows.ts b/packages/cli/test/integration/shared/db/workflows.ts index f0758088f1667..4abd2977a6e79 100644 --- a/packages/cli/test/integration/shared/db/workflows.ts +++ b/packages/cli/test/integration/shared/db/workflows.ts @@ -121,5 +121,9 @@ export async function getAllWorkflows() { return await Container.get(WorkflowRepository).find(); } +export async function getAllSharedWorkflows() { + return await Container.get(SharedWorkflowRepository).find(); +} + export const getWorkflowById = async (id: string) => await Container.get(WorkflowRepository).findOneBy({ id }); From c2d4fad128f8f92bcd7c1469620317f6ebb22ee8 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 11:40:47 +0200 Subject: [PATCH 04/10] refactor workflow tests and make sure the owner sharing is created when `--userId` is not passed and the workflows didn't exist in the db yet --- packages/cli/src/commands/import/workflow.ts | 15 ++++++++++++++- packages/cli/src/services/import.service.ts | 12 +++++------- .../test/integration/import.service.test.ts | 17 ++--------------- .../test/integration/shared/db/workflows.ts | 18 +++++++++++------- 4 files changed, 32 insertions(+), 30 deletions(-) diff --git a/packages/cli/src/commands/import/workflow.ts b/packages/cli/src/commands/import/workflow.ts index 149e39e54798a..843d187ea2883 100644 --- a/packages/cli/src/commands/import/workflow.ts +++ b/packages/cli/src/commands/import/workflow.ts @@ -12,6 +12,8 @@ import type { IWorkflowToImport } from '@/Interfaces'; import { ImportService } from '@/services/import.service'; import { BaseCommand } from '../BaseCommand'; import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflow.repository'; +import { UserRepository } from '@/databases/repositories/user.repository'; +import { UM_FIX_INSTRUCTION } from '@/constants'; function assertHasWorkflowsToImport(workflows: unknown): asserts workflows is IWorkflowToImport[] { if (!Array.isArray(workflows)) { @@ -77,6 +79,8 @@ export class ImportWorkflowsCommand extends BaseCommand { } } + const owner = await this.getOwner(); + const workflows = await this.readWorkflows(flags.input, flags.separate); const result = await this.checkRelations(workflows, flags.userId); @@ -86,7 +90,7 @@ export class ImportWorkflowsCommand extends BaseCommand { this.logger.info(`Importing ${workflows.length} workflows...`); - await Container.get(ImportService).importWorkflows(workflows, flags.userId); + await Container.get(ImportService).importWorkflows(workflows, flags.userId ?? owner.id); this.reportSuccess(workflows.length); } @@ -132,6 +136,15 @@ export class ImportWorkflowsCommand extends BaseCommand { return sharing?.userId; } + private async getOwner() { + const owner = await Container.get(UserRepository).findOneBy({ role: 'global:owner' }); + if (!owner) { + throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); + } + + return owner; + } + private async workflowExists(workflow: WorkflowEntity) { return await Container.get(WorkflowRepository).existsBy({ id: workflow.id }); } diff --git a/packages/cli/src/services/import.service.ts b/packages/cli/src/services/import.service.ts index db6b976970abf..4d9f5bdaccca4 100644 --- a/packages/cli/src/services/import.service.ts +++ b/packages/cli/src/services/import.service.ts @@ -31,7 +31,7 @@ export class ImportService { this.dbTags = await this.tagRepository.find(); } - async importWorkflows(workflows: WorkflowEntity[], userId?: string) { + async importWorkflows(workflows: WorkflowEntity[], userId: string) { await this.initRecords(); for (const workflow of workflows) { @@ -54,15 +54,13 @@ export class ImportService { this.logger.info(`Deactivating workflow "${workflow.name}". Remember to activate later.`); } - const upsertResult = await tx.upsert(WorkflowEntity, workflow, ['id']); + const exists = workflow.id ? await tx.existsBy(WorkflowEntity, { id: workflow.id }) : false; + const upsertResult = await tx.upsert(WorkflowEntity, workflow, ['id']); const workflowId = upsertResult.identifiers.at(0)?.id as string; - if (userId) { - if (!(await tx.existsBy(User, { id: userId }))) { - throw new ApplicationError('Failed to find user', { extra: { userId } }); - } - + // Create relationship if the workflow was inserted instead of updated. + if (!exists) { await tx.upsert(SharedWorkflow, { workflowId, userId, role: 'workflow:owner' }, [ 'workflowId', 'userId', diff --git a/packages/cli/test/integration/import.service.test.ts b/packages/cli/test/integration/import.service.test.ts index 1ca70c115e7ab..d9757ed776769 100644 --- a/packages/cli/test/integration/import.service.test.ts +++ b/packages/cli/test/integration/import.service.test.ts @@ -13,7 +13,7 @@ import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflo import * as testDb from './shared/testDb'; import { mockInstance } from '../shared/mocking'; import { createOwner } from './shared/db/users'; -import { createWorkflow, getWorkflowById } from './shared/db/workflows'; +import { createWorkflow, getWorkflowById, newWorkflow } from './shared/db/workflows'; import type { User } from '@db/entities/User'; @@ -57,7 +57,7 @@ describe('ImportService', () => { }); test('should make user owner of imported workflow', async () => { - const workflowToImport = await createWorkflow(); + const workflowToImport = newWorkflow(); await importService.importWorkflows([workflowToImport], owner.id); @@ -68,19 +68,6 @@ describe('ImportService', () => { expect(dbSharing.userId).toBe(owner.id); }); - test('should not create a new owner if `userId` is not passed in', async () => { - const workflowToImport = await createWorkflow(); - - await importService.importWorkflows([workflowToImport]); - - const dbSharing = await Container.get(SharedWorkflowRepository).findOneBy({ - workflowId: workflowToImport.id, - role: 'workflow:owner', - }); - - expect(dbSharing).toBeNull(); - }); - test('should deactivate imported workflow if active', async () => { const workflowToImport = await createWorkflow({ active: true }); diff --git a/packages/cli/test/integration/shared/db/workflows.ts b/packages/cli/test/integration/shared/db/workflows.ts index 4abd2977a6e79..18a97a693b6ba 100644 --- a/packages/cli/test/integration/shared/db/workflows.ts +++ b/packages/cli/test/integration/shared/db/workflows.ts @@ -19,12 +19,7 @@ export async function createManyWorkflows( return await Promise.all(workflowRequests); } -/** - * Store a workflow in the DB (without a trigger) and optionally assign it to a user. - * @param attributes workflow attributes - * @param user user to assign the workflow to - */ -export async function createWorkflow(attributes: Partial = {}, user?: User) { +export function newWorkflow(attributes: Partial = {}): WorkflowEntity { const { active, name, nodes, connections, versionId } = attributes; const workflowEntity = Container.get(WorkflowRepository).create({ @@ -45,7 +40,16 @@ export async function createWorkflow(attributes: Partial = {}, u ...attributes, }); - const workflow = await Container.get(WorkflowRepository).save(workflowEntity); + return workflowEntity; +} + +/** + * Store a workflow in the DB (without a trigger) and optionally assign it to a user. + * @param attributes workflow attributes + * @param user user to assign the workflow to + */ +export async function createWorkflow(attributes: Partial = {}, user?: User) { + const workflow = await Container.get(WorkflowRepository).save(newWorkflow(attributes)); if (user) { await Container.get(SharedWorkflowRepository).save({ From a6d0eef6d860c13499107ef1ce775493aec884b8 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 11:50:10 +0200 Subject: [PATCH 05/10] refactor credential import and remove redundant tests There is no need to test the `--separate` flag in combination with the `--userId` flag, because the check for the relationships is now happening in one place --- .../cli/src/commands/import/credentials.ts | 153 ++++++++++-------- .../commands/credentials.cmd.test.ts | 136 ++-------------- .../integration/commands/import.cmd.test.ts | 130 +++++++++------ 3 files changed, 179 insertions(+), 240 deletions(-) diff --git a/packages/cli/src/commands/import/credentials.ts b/packages/cli/src/commands/import/credentials.ts index fc8f526bfc8bb..3af4bfc303b19 100644 --- a/packages/cli/src/commands/import/credentials.ts +++ b/packages/cli/src/commands/import/credentials.ts @@ -15,7 +15,6 @@ import type { ICredentialsEncrypted } from 'n8n-workflow'; import { ApplicationError, jsonParse } from 'n8n-workflow'; import { UM_FIX_INSTRUCTION } from '@/constants'; import { UserRepository } from '@db/repositories/user.repository'; -import { SharedCredentialsRepository } from '@/databases/repositories/sharedCredentials.repository'; export class ImportCredentialsCommand extends BaseCommand { static description = 'Import credentials'; @@ -65,87 +64,103 @@ export class ImportCredentialsCommand extends BaseCommand { } } - let totalImported = 0; - - const cipher = Container.get(Cipher); const user = flags.userId ? await this.getAssignee(flags.userId) : await this.getOwner(); - if (flags.separate) { - let { input: inputPath } = flags; + const credentials = await this.readCredentials(flags.input, flags.separate); + + await Db.getConnection().transaction(async (transactionManager) => { + this.transactionManager = transactionManager; + + const result = await this.checkRelations(credentials, flags.userId); - if (process.platform === 'win32') { - inputPath = inputPath.replace(/\\/g, '/'); + if (!result.success) { + throw new ApplicationError(result.message); } - const files = await glob('*.json', { - cwd: inputPath, - absolute: true, - }); + for (const credential of credentials) { + await this.storeCredential(credential, user); + } + }); - totalImported = files.length; - - await Db.getConnection().transaction(async (transactionManager) => { - this.transactionManager = transactionManager; - for (const file of files) { - const credential = jsonParse( - fs.readFileSync(file, { encoding: 'utf8' }), - ); - - if (credential.id && flags.userId) { - const credentialOwner = await this.getCredentialOwner(credential.id); - - if (credentialOwner && credentialOwner !== flags.userId) { - throw new ApplicationError( - `The credential with id "${credential.id}" is already owned by the user with the id "${credentialOwner}". It can't be re-owned by the user with the id "${flags.userId}"`, - ); - } - } - - if (typeof credential.data === 'object') { - // plain data / decrypted input. Should be encrypted first. - credential.data = cipher.encrypt(credential.data); - } - await this.storeCredential(credential, user); - } - }); + this.reportSuccess(credentials.length); + } - this.reportSuccess(totalImported); - return; + private async checkRelations(credentials: ICredentialsEncrypted[], userId?: string) { + if (!userId) { + return { + success: true as const, + message: undefined, + }; } - const credentials = jsonParse( - fs.readFileSync(flags.input, { encoding: 'utf8' }), - ); + for (const credential of credentials) { + if (credential.id === undefined) { + continue; + } - totalImported = credentials.length; + if (!(await this.credentialExists(credential.id))) { + continue; + } - if (!Array.isArray(credentials)) { - throw new ApplicationError( - 'File does not seem to contain credentials. Make sure the credentials are contained in an array.', - ); + const ownerId = await this.getCredentialOwner(credential.id); + if (!ownerId) { + continue; + } + + if (ownerId !== userId) { + return { + success: false as const, + message: `The credential with id "${credential.id}" is already owned by the user with the id "${ownerId}". It can't be re-owned by the user with the id "${userId}"`, + }; + } } - await Db.getConnection().transaction(async (transactionManager) => { - this.transactionManager = transactionManager; - for (const credential of credentials) { - if (credential.id && flags.userId) { - const credentialOwner = await this.getCredentialOwner(credential.id); - if (credentialOwner && credentialOwner !== flags.userId) { - throw new ApplicationError( - `The credential with id "${credential.id}" is already owned by the user with the id "${credentialOwner}". It can't be re-owned by the user with the id "${flags.userId}"`, - ); - } - } + return { + success: true as const, + message: undefined, + }; + } - if (typeof credential.data === 'object') { - // plain data / decrypted input. Should be encrypted first. - credential.data = cipher.encrypt(credential.data); - } - await this.storeCredential(credential, user); + private async readCredentials(path: string, separate: boolean): Promise { + const cipher = Container.get(Cipher); + + if (process.platform === 'win32') { + path = path.replace(/\\/g, '/'); + } + + let credentials: ICredentialsEncrypted[]; + + if (separate) { + const files = await glob('*.json', { + cwd: path, + absolute: true, + }); + + credentials = files.map((file) => + jsonParse(fs.readFileSync(file, { encoding: 'utf8' })), + ); + } else { + const credentialsUnchecked = jsonParse( + fs.readFileSync(path, { encoding: 'utf8' }), + ); + + if (!Array.isArray(credentialsUnchecked)) { + throw new ApplicationError( + 'File does not seem to contain credentials. Make sure the credentials are contained in an array.', + ); } - }); - this.reportSuccess(totalImported); + credentials = credentialsUnchecked; + } + + return credentials.map((credential) => { + if (typeof credential.data === 'object') { + // plain data / decrypted input. Should be encrypted first. + credential.data = cipher.encrypt(credential.data); + } + + return credential; + }); } async catch(error: Error) { @@ -202,11 +217,15 @@ export class ImportCredentialsCommand extends BaseCommand { } private async getCredentialOwner(credentialsId: string) { - const sharedCredential = await Container.get(SharedCredentialsRepository).findOneBy({ + const sharedCredential = await this.transactionManager.findOneBy(SharedCredentials, { credentialsId, role: 'credential:owner', }); return sharedCredential?.userId; } + + private async credentialExists(credentialId: string) { + return await this.transactionManager.existsBy(CredentialsEntity, { id: credentialId }); + } } diff --git a/packages/cli/test/integration/commands/credentials.cmd.test.ts b/packages/cli/test/integration/commands/credentials.cmd.test.ts index 5d9febce1cfc8..a8bbf0ca94283 100644 --- a/packages/cli/test/integration/commands/credentials.cmd.test.ts +++ b/packages/cli/test/integration/commands/credentials.cmd.test.ts @@ -18,7 +18,7 @@ beforeAll(async () => { }); beforeEach(async () => { - await testDb.truncate(['Credentials']); + await testDb.truncate(['Credentials', 'SharedCredentials', 'User']); }); afterAll(async () => { @@ -26,6 +26,7 @@ afterAll(async () => { }); test('import:credentials should import a credential', async () => { + await createOwner(); const before = await getAllCredentials(); expect(before.length).toBe(0); const importer = new ImportCredentialsCommand( @@ -49,57 +50,22 @@ test('import:credentials should import a credential', async () => { mockExit.mockRestore(); }); -test('`import:credentials --userId ...` should fail if the credential exists already and is owned by somebody else', async () => { +test('import:credentials should import a credential from separated files', async () => { // // ARRANGE // const owner = await createOwner(); - const member = await createMember(); - - // import credential the first time, assigning it to the owner - const importer1 = new ImportCredentialsCommand( - [ - '--input=./test/integration/commands/importCredentials/credentials.json', - `--userId=${owner.id}`, - ], - oclifConfig, - ); - await importer1.init(); - await importer1.run(); - - // making sure the import worked - const before = { - credentials: await getAllCredentials(), - sharings: await getAllSharedCredentials(), - }; - expect(before).toMatchObject({ - credentials: [expect.objectContaining({ id: '123', name: 'cred-aws-test' })], - sharings: [ - expect.objectContaining({ - credentialsId: '123', - userId: owner.id, - role: 'credential:owner', - }), - ], - }); - - // Prepare the second import, while updating the name we try to assign the - // credential to another user. - const importer2 = new ImportCredentialsCommand( - [ - '--input=./test/integration/commands/importCredentials/credentials-updated.json', - `--userId=${member.id}`, - ], - oclifConfig, - ); - await importer2.init(); // // ACT // - await expect(importer2.run()).rejects.toThrowError( - `The credential with id "123" is already owned by the user with the id "${owner.id}". It can't be re-owned by the user with the id "${member.id}"`, + // import credential the first time, assigning it to the owner + const importer = new ImportCredentialsCommand( + ['--separate', '--input=./test/integration/commands/importCredentials/separate'], + oclifConfig, ); + await importer.init(); + await importer.run(); // // ASSERT @@ -113,7 +79,6 @@ test('`import:credentials --userId ...` should fail if the credential exists alr credentials: [ expect.objectContaining({ id: '123', - // only the name was updated name: 'cred-aws-test', }), ], @@ -127,7 +92,7 @@ test('`import:credentials --userId ...` should fail if the credential exists alr }); }); -test('`import:credentials --separate --userId ...` should fail if the credential exists already and is owned by somebody else', async () => { +test('`import:credentials --userId ...` should fail if the credential exists already and is owned by somebody else', async () => { // // ARRANGE // @@ -137,8 +102,7 @@ test('`import:credentials --separate --userId ...` should fail if the credential // import credential the first time, assigning it to the owner const importer1 = new ImportCredentialsCommand( [ - '--separate', - '--input=./test/integration/commands/importCredentials/separate', + '--input=./test/integration/commands/importCredentials/credentials.json', `--userId=${owner.id}`, ], oclifConfig, @@ -166,8 +130,7 @@ test('`import:credentials --separate --userId ...` should fail if the credential // credential to another user. const importer2 = new ImportCredentialsCommand( [ - '--separate', - '--input=./test/integration/commands/importCredentials/separate-updated', + '--input=./test/integration/commands/importCredentials/credentials-updated.json', `--userId=${member.id}`, ], oclifConfig, @@ -189,11 +152,11 @@ test('`import:credentials --separate --userId ...` should fail if the credential sharings: await getAllSharedCredentials(), }; - // nothing should have changed expect(after).toMatchObject({ credentials: [ expect.objectContaining({ id: '123', + // only the name was updated name: 'cred-aws-test', }), ], @@ -207,82 +170,11 @@ test('`import:credentials --separate --userId ...` should fail if the credential }); }); -test("only update the credential, don't create or update the owner if `--userId` is not passed, while using `--separate`", async () => { - // - // ARRANGE - // - const member = await createMember(); - - // import credential the first time, assigning it to a member - const importer1 = new ImportCredentialsCommand( - [ - '--separate', - '--input=./test/integration/commands/importCredentials/separate', - `--userId=${member.id}`, - ], - oclifConfig, - ); - await importer1.init(); - await importer1.run(); - - // making sure the import worked - const before = { - credentials: await getAllCredentials(), - sharings: await getAllSharedCredentials(), - }; - expect(before).toMatchObject({ - credentials: [expect.objectContaining({ id: '123', name: 'cred-aws-test' })], - sharings: [ - expect.objectContaining({ - credentialsId: '123', - userId: member.id, - role: 'credential:owner', - }), - ], - }); - - // prepare the second import, trying to assign the same credential to another user - const importer2 = new ImportCredentialsCommand( - ['--separate', '--input=./test/integration/commands/importCredentials/separate-updated'], - oclifConfig, - ); - await importer2.init(); - - // - // ACT - // - await importer2.run(); - - // - // ASSERT - // - const after = { - credentials: await getAllCredentials(), - sharings: await getAllSharedCredentials(), - }; - - expect(after).toMatchObject({ - credentials: [ - expect.objectContaining({ - id: '123', - // only the name was updated - name: 'cred-aws-prod', - }), - ], - sharings: [ - expect.objectContaining({ - credentialsId: '123', - userId: member.id, - role: 'credential:owner', - }), - ], - }); -}); - test("only update credential, don't create or update owner if `--userId` is not passed", async () => { // // ARRANGE // + await createOwner(); const member = await createMember(); // import credential the first time, assigning it to a member diff --git a/packages/cli/test/integration/commands/import.cmd.test.ts b/packages/cli/test/integration/commands/import.cmd.test.ts index 4851ecf0f9e05..9e33578233566 100644 --- a/packages/cli/test/integration/commands/import.cmd.test.ts +++ b/packages/cli/test/integration/commands/import.cmd.test.ts @@ -11,6 +11,12 @@ import { createMember, createOwner } from '../shared/db/users'; const oclifConfig = new Config({ root: __dirname }); +async function importWorkflow(argv: string[]) { + const importer = new ImportWorkflowsCommand(argv, oclifConfig); + await importer.init(); + await importer.run(); +} + beforeAll(async () => { mockInstance(InternalHooks); mockInstance(LoadNodesAndCredentials); @@ -18,7 +24,7 @@ beforeAll(async () => { }); beforeEach(async () => { - await testDb.truncate(['Workflow']); + await testDb.truncate(['Workflow', 'SharedWorkflow', 'User']); }); afterAll(async () => { @@ -26,62 +32,83 @@ afterAll(async () => { }); test('import:workflow should import active workflow and deactivate it', async () => { - const before = await getAllWorkflows(); - expect(before.length).toBe(0); - const importer = new ImportWorkflowsCommand( - ['--separate', '--input=./test/integration/commands/importWorkflows/separate'], - oclifConfig, - ); - const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => { - throw new Error('process.exit'); - }); + // + // ARRANGE + // + const owner = await createOwner(); - await importer.init(); - try { - await importer.run(); - } catch (error) { - expect(error.message).toBe('process.exit'); - } - const after = await getAllWorkflows(); - expect(after.length).toBe(2); - expect(after[0].name).toBe('active-workflow'); - expect(after[0].active).toBe(false); - expect(after[1].name).toBe('inactive-workflow'); - expect(after[1].active).toBe(false); - mockExit.mockRestore(); + const before = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + expect(before.workflows).toHaveLength(0); + expect(before.sharings).toHaveLength(0); + + // + // ACT + // + await importWorkflow([ + '--separate', + '--input=./test/integration/commands/importWorkflows/separate', + ]); + + // + // ASSERT + // + const after = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + expect(after).toMatchObject({ + workflows: [ + expect.objectContaining({ name: 'active-workflow', active: false }), + expect.objectContaining({ name: 'inactive-workflow', active: false }), + ], + sharings: [ + expect.objectContaining({ workflowId: '998', userId: owner.id, role: 'workflow:owner' }), + expect.objectContaining({ workflowId: '999', userId: owner.id, role: 'workflow:owner' }), + ], + }); }); test('import:workflow should import active workflow from combined file and deactivate it', async () => { - const before = await getAllWorkflows(); - expect(before.length).toBe(0); - const importer = new ImportWorkflowsCommand( - ['--input=./test/integration/commands/importWorkflows/combined/combined.json'], - oclifConfig, - ); - const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => { - throw new Error('process.exit'); - }); + // + // ARRANGE + // + const owner = await createOwner(); - await importer.init(); - try { - await importer.run(); - } catch (error) { - expect(error.message).toBe('process.exit'); - } - const after = await getAllWorkflows(); - expect(after.length).toBe(2); - expect(after[0].name).toBe('active-workflow'); - expect(after[0].active).toBe(false); - expect(after[1].name).toBe('inactive-workflow'); - expect(after[1].active).toBe(false); - mockExit.mockRestore(); -}); + const before = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + expect(before.workflows).toHaveLength(0); + expect(before.sharings).toHaveLength(0); -async function importWorkflow(argv: string[]) { - const importer = new ImportWorkflowsCommand(argv, oclifConfig); - await importer.init(); - await importer.run(); -} + // + // ACT + // + await importWorkflow([ + '--input=./test/integration/commands/importWorkflows/combined/combined.json', + ]); + + // + // ASSERT + // + const after = { + workflows: await getAllWorkflows(), + sharings: await getAllSharedWorkflows(), + }; + expect(after).toMatchObject({ + workflows: [ + expect.objectContaining({ name: 'active-workflow', active: false }), + expect.objectContaining({ name: 'inactive-workflow', active: false }), + ], + sharings: [ + expect.objectContaining({ workflowId: '998', userId: owner.id, role: 'workflow:owner' }), + expect.objectContaining({ workflowId: '999', userId: owner.id, role: 'workflow:owner' }), + ], + }); +}); test('`import:workflow --userId ...` should fail if the workflow exists already and is owned by somebody else', async () => { // @@ -150,6 +177,7 @@ test("only update the workflow, don't create or update the owner if `--userId` i // // ARRANGE // + await createOwner(); const member = await createMember(); // Import workflow the first time, assigning it to a member. From 9872849c30ff8fcb2f4d107276c7a664b72e8749 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 11:59:30 +0200 Subject: [PATCH 06/10] simplify and unify the tests --- .../commands/credentials.cmd.test.ts | 120 ++++++++---------- .../integration/commands/import.cmd.test.ts | 14 -- 2 files changed, 55 insertions(+), 79 deletions(-) diff --git a/packages/cli/test/integration/commands/credentials.cmd.test.ts b/packages/cli/test/integration/commands/credentials.cmd.test.ts index a8bbf0ca94283..730a0cd5dd1da 100644 --- a/packages/cli/test/integration/commands/credentials.cmd.test.ts +++ b/packages/cli/test/integration/commands/credentials.cmd.test.ts @@ -11,6 +11,12 @@ import { createMember, createOwner } from '../shared/db/users'; const oclifConfig = new Config({ root: __dirname }); +async function importCredential(argv: string[]) { + const importer = new ImportCredentialsCommand(argv, oclifConfig); + await importer.init(); + await importer.run(); +} + beforeAll(async () => { mockInstance(InternalHooks); mockInstance(LoadNodesAndCredentials); @@ -26,28 +32,31 @@ afterAll(async () => { }); test('import:credentials should import a credential', async () => { - await createOwner(); - const before = await getAllCredentials(); - expect(before.length).toBe(0); - const importer = new ImportCredentialsCommand( - ['--input=./test/integration/commands/importCredentials/credentials.json'], - oclifConfig, - ); - const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => { - throw new Error('process.exit'); - }); + // + // ARRANGE + // + const owner = await createOwner(); - await importer.init(); - try { - await importer.run(); - } catch (error) { - expect(error.message).toBe('process.exit'); - } - const after = await getAllCredentials(); - expect(after.length).toBe(1); - expect(after[0].name).toBe('cred-aws-test'); - expect(after[0].id).toBe('123'); - mockExit.mockRestore(); + // + // ACT + // + await importCredential([ + '--input=./test/integration/commands/importCredentials/credentials.json', + ]); + + // + // ASSERT + // + const after = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + expect(after).toMatchObject({ + credentials: [expect.objectContaining({ id: '123', name: 'cred-aws-test' })], + sharings: [ + expect.objectContaining({ credentialsId: '123', userId: owner.id, role: 'credential:owner' }), + ], + }); }); test('import:credentials should import a credential from separated files', async () => { @@ -60,12 +69,10 @@ test('import:credentials should import a credential from separated files', async // ACT // // import credential the first time, assigning it to the owner - const importer = new ImportCredentialsCommand( - ['--separate', '--input=./test/integration/commands/importCredentials/separate'], - oclifConfig, - ); - await importer.init(); - await importer.run(); + await importCredential([ + '--separate', + '--input=./test/integration/commands/importCredentials/separate', + ]); // // ASSERT @@ -100,15 +107,10 @@ test('`import:credentials --userId ...` should fail if the credential exists alr const member = await createMember(); // import credential the first time, assigning it to the owner - const importer1 = new ImportCredentialsCommand( - [ - '--input=./test/integration/commands/importCredentials/credentials.json', - `--userId=${owner.id}`, - ], - oclifConfig, - ); - await importer1.init(); - await importer1.run(); + await importCredential([ + '--input=./test/integration/commands/importCredentials/credentials.json', + `--userId=${owner.id}`, + ]); // making sure the import worked const before = { @@ -126,21 +128,18 @@ test('`import:credentials --userId ...` should fail if the credential exists alr ], }); - // Prepare the second import, while updating the name we try to assign the - // credential to another user. - const importer2 = new ImportCredentialsCommand( - [ - '--input=./test/integration/commands/importCredentials/credentials-updated.json', - `--userId=${member.id}`, - ], - oclifConfig, - ); - await importer2.init(); - // // ACT // - await expect(importer2.run()).rejects.toThrowError( + + // Import again while updating the name we try to assign the + // credential to another user. + await expect( + importCredential([ + '--input=./test/integration/commands/importCredentials/credentials-updated.json', + `--userId=${member.id}`, + ]), + ).rejects.toThrowError( `The credential with id "123" is already owned by the user with the id "${owner.id}". It can't be re-owned by the user with the id "${member.id}"`, ); @@ -178,15 +177,10 @@ test("only update credential, don't create or update owner if `--userId` is not const member = await createMember(); // import credential the first time, assigning it to a member - const importer1 = new ImportCredentialsCommand( - [ - '--input=./test/integration/commands/importCredentials/credentials.json', - `--userId=${member.id}`, - ], - oclifConfig, - ); - await importer1.init(); - await importer1.run(); + await importCredential([ + '--input=./test/integration/commands/importCredentials/credentials.json', + `--userId=${member.id}`, + ]); // making sure the import worked const before = { @@ -204,17 +198,13 @@ test("only update credential, don't create or update owner if `--userId` is not ], }); - // prepare the second import, only updating the name and omitting `--userId` - const importer2 = new ImportCredentialsCommand( - ['--input=./test/integration/commands/importCredentials/credentials-updated.json'], - oclifConfig, - ); - await importer2.init(); - // // ACT // - await importer2.run(); + // Import again only updating the name and omitting `--userId` + await importCredential([ + '--input=./test/integration/commands/importCredentials/credentials-updated.json', + ]); // // ASSERT diff --git a/packages/cli/test/integration/commands/import.cmd.test.ts b/packages/cli/test/integration/commands/import.cmd.test.ts index 9e33578233566..e65325c47153c 100644 --- a/packages/cli/test/integration/commands/import.cmd.test.ts +++ b/packages/cli/test/integration/commands/import.cmd.test.ts @@ -37,13 +37,6 @@ test('import:workflow should import active workflow and deactivate it', async () // const owner = await createOwner(); - const before = { - workflows: await getAllWorkflows(), - sharings: await getAllSharedWorkflows(), - }; - expect(before.workflows).toHaveLength(0); - expect(before.sharings).toHaveLength(0); - // // ACT // @@ -77,13 +70,6 @@ test('import:workflow should import active workflow from combined file and deact // const owner = await createOwner(); - const before = { - workflows: await getAllWorkflows(), - sharings: await getAllSharedWorkflows(), - }; - expect(before.workflows).toHaveLength(0); - expect(before.sharings).toHaveLength(0); - // // ACT // From 0aaddc0ec6e6b71b5bdf61c07791d93fa3e2d255 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 12:33:35 +0200 Subject: [PATCH 07/10] fix lints --- packages/cli/src/services/import.service.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/cli/src/services/import.service.ts b/packages/cli/src/services/import.service.ts index 4d9f5bdaccca4..11b726225663f 100644 --- a/packages/cli/src/services/import.service.ts +++ b/packages/cli/src/services/import.service.ts @@ -1,6 +1,6 @@ import { Service } from 'typedi'; import { v4 as uuid } from 'uuid'; -import { ApplicationError, type INode, type INodeCredentialsDetails } from 'n8n-workflow'; +import { type INode, type INodeCredentialsDetails } from 'n8n-workflow'; import { Logger } from '@/Logger'; import * as Db from '@/Db'; @@ -12,7 +12,6 @@ import { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { WorkflowTagMapping } from '@db/entities/WorkflowTagMapping'; import type { TagEntity } from '@db/entities/TagEntity'; import type { ICredentialsDb } from '@/Interfaces'; -import { User } from '@/databases/entities/User'; @Service() export class ImportService { From f87da4af2ab0c4f5a42b58f92c2f852f3f8c955f Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 11 Apr 2024 12:38:53 +0200 Subject: [PATCH 08/10] add missing test case --- .../test/integration/import.service.test.ts | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/cli/test/integration/import.service.test.ts b/packages/cli/test/integration/import.service.test.ts index d9757ed776769..2cfdbe4080411 100644 --- a/packages/cli/test/integration/import.service.test.ts +++ b/packages/cli/test/integration/import.service.test.ts @@ -12,8 +12,13 @@ import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflo import * as testDb from './shared/testDb'; import { mockInstance } from '../shared/mocking'; -import { createOwner } from './shared/db/users'; -import { createWorkflow, getWorkflowById, newWorkflow } from './shared/db/workflows'; +import { createMember, createOwner } from './shared/db/users'; +import { + createWorkflow, + getAllSharedWorkflows, + getWorkflowById, + newWorkflow, +} from './shared/db/workflows'; import type { User } from '@db/entities/User'; @@ -68,6 +73,23 @@ describe('ImportService', () => { expect(dbSharing.userId).toBe(owner.id); }); + test('should not change the owner if it already exists', async () => { + const member = await createMember(); + const workflowToImport = await createWorkflow(undefined, owner); + + await importService.importWorkflows([workflowToImport], member.id); + + const sharings = await getAllSharedWorkflows(); + + expect(sharings).toMatchObject([ + expect.objectContaining({ + workflowId: workflowToImport.id, + userId: owner.id, + role: 'workflow:owner', + }), + ]); + }); + test('should deactivate imported workflow if active', async () => { const workflowToImport = await createWorkflow({ active: true }); From e25b5f94a0eefeac4582bedf16ebdc41afe4a333 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 12 Apr 2024 07:34:50 +0200 Subject: [PATCH 09/10] remove unused test file --- .../separate-updated/separate-credential.json | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json diff --git a/packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json b/packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json deleted file mode 100644 index bbb275debc7d4..0000000000000 --- a/packages/cli/test/integration/commands/importCredentials/separate-updated/separate-credential.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "createdAt": "2023-07-10T14:50:49.193Z", - "updatedAt": "2023-10-27T13:34:42.917Z", - "id": "123", - "name": "cred-aws-prod", - "data": { - "region": "eu-west-1", - "accessKeyId": "999999999999", - "secretAccessKey": "aaaaaaaaaaaaa" - }, - "type": "aws" -} From 65c9426d02bc5c1bd2d193aa1b9c016a427be4ff Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 12 Apr 2024 07:35:01 +0200 Subject: [PATCH 10/10] reorganize class to make diff more readable --- .../cli/src/commands/import/credentials.ts | 86 +++++++++---------- packages/cli/src/commands/import/workflow.ts | 36 ++++---- 2 files changed, 61 insertions(+), 61 deletions(-) diff --git a/packages/cli/src/commands/import/credentials.ts b/packages/cli/src/commands/import/credentials.ts index 3af4bfc303b19..52440e1149bf7 100644 --- a/packages/cli/src/commands/import/credentials.ts +++ b/packages/cli/src/commands/import/credentials.ts @@ -85,6 +85,49 @@ export class ImportCredentialsCommand extends BaseCommand { this.reportSuccess(credentials.length); } + async catch(error: Error) { + this.logger.error( + 'An error occurred while importing credentials. See log messages for details.', + ); + this.logger.error(error.message); + } + + private reportSuccess(total: number) { + this.logger.info( + `Successfully imported ${total} ${total === 1 ? 'credential.' : 'credentials.'}`, + ); + } + + private async storeCredential(credential: Partial, user: User) { + const result = await this.transactionManager.upsert(CredentialsEntity, credential, ['id']); + + const sharingExists = await this.transactionManager.existsBy(SharedCredentials, { + credentialsId: credential.id, + role: 'credential:owner', + }); + + if (!sharingExists) { + await this.transactionManager.upsert( + SharedCredentials, + { + credentialsId: result.identifiers[0].id as string, + userId: user.id, + role: 'credential:owner', + }, + ['credentialsId', 'userId'], + ); + } + } + + private async getOwner() { + const owner = await Container.get(UserRepository).findOneBy({ role: 'global:owner' }); + if (!owner) { + throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); + } + + return owner; + } + private async checkRelations(credentials: ICredentialsEncrypted[], userId?: string) { if (!userId) { return { @@ -163,49 +206,6 @@ export class ImportCredentialsCommand extends BaseCommand { }); } - async catch(error: Error) { - this.logger.error( - 'An error occurred while importing credentials. See log messages for details.', - ); - this.logger.error(error.message); - } - - private reportSuccess(total: number) { - this.logger.info( - `Successfully imported ${total} ${total === 1 ? 'credential.' : 'credentials.'}`, - ); - } - - private async storeCredential(credential: Partial, user: User) { - const result = await this.transactionManager.upsert(CredentialsEntity, credential, ['id']); - - const sharingExists = await this.transactionManager.existsBy(SharedCredentials, { - credentialsId: credential.id, - role: 'credential:owner', - }); - - if (!sharingExists) { - await this.transactionManager.upsert( - SharedCredentials, - { - credentialsId: result.identifiers[0].id as string, - userId: user.id, - role: 'credential:owner', - }, - ['credentialsId', 'userId'], - ); - } - } - - private async getOwner() { - const owner = await Container.get(UserRepository).findOneBy({ role: 'global:owner' }); - if (!owner) { - throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); - } - - return owner; - } - private async getAssignee(userId: string) { const user = await Container.get(UserRepository).findOneBy({ id: userId }); diff --git a/packages/cli/src/commands/import/workflow.ts b/packages/cli/src/commands/import/workflow.ts index 843d187ea2883..40404482fb455 100644 --- a/packages/cli/src/commands/import/workflow.ts +++ b/packages/cli/src/commands/import/workflow.ts @@ -4,16 +4,16 @@ import { ApplicationError, jsonParse } from 'n8n-workflow'; import fs from 'fs'; import glob from 'fast-glob'; +import { UM_FIX_INSTRUCTION } from '@/constants'; import { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { disableAutoGeneratedIds } from '@db/utils/commandHelpers'; import { generateNanoId } from '@db/utils/generators'; +import { UserRepository } from '@db/repositories/user.repository'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import type { IWorkflowToImport } from '@/Interfaces'; import { ImportService } from '@/services/import.service'; import { BaseCommand } from '../BaseCommand'; -import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflow.repository'; -import { UserRepository } from '@/databases/repositories/user.repository'; -import { UM_FIX_INSTRUCTION } from '@/constants'; +import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; function assertHasWorkflowsToImport(workflows: unknown): asserts workflows is IWorkflowToImport[] { if (!Array.isArray(workflows)) { @@ -127,13 +127,13 @@ export class ImportWorkflowsCommand extends BaseCommand { }; } - private async getWorkflowOwner(workflow: WorkflowEntity) { - const sharing = await Container.get(SharedWorkflowRepository).findOneBy({ - workflowId: workflow.id, - role: 'workflow:owner', - }); + async catch(error: Error) { + this.logger.error('An error occurred while importing workflows. See log messages for details.'); + this.logger.error(error.message); + } - return sharing?.userId; + private reportSuccess(total: number) { + this.logger.info(`Successfully imported ${total} ${total === 1 ? 'workflow.' : 'workflows.'}`); } private async getOwner() { @@ -145,6 +145,15 @@ export class ImportWorkflowsCommand extends BaseCommand { return owner; } + private async getWorkflowOwner(workflow: WorkflowEntity) { + const sharing = await Container.get(SharedWorkflowRepository).findOneBy({ + workflowId: workflow.id, + role: 'workflow:owner', + }); + + return sharing?.userId; + } + private async workflowExists(workflow: WorkflowEntity) { return await Container.get(WorkflowRepository).existsBy({ id: workflow.id }); } @@ -180,13 +189,4 @@ export class ImportWorkflowsCommand extends BaseCommand { return workflowInstances; } } - - async catch(error: Error) { - this.logger.error('An error occurred while importing workflows. See log messages for details.'); - this.logger.error(error.message); - } - - private reportSuccess(total: number) { - this.logger.info(`Successfully imported ${total} ${total === 1 ? 'workflow.' : 'workflows.'}`); - } }