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(); +}