From 260bc07ca9484b6e82cc9dc82c68a6c1c58f4a49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 26 Mar 2024 17:18:41 +0100 Subject: [PATCH] fix(core): Assign credential ownership correctly in source control import (#8955) --- .../sharedCredentials.repository.ts | 3 +- .../sourceControlExport.service.ee.ts | 33 ++-- .../sourceControlImport.service.ee.ts | 25 ++- .../types/exportableCredential.ts | 6 + .../types/sourceControlPullWorkFolder.ts | 1 + .../source-control-import.service.test.ts | 152 ++++++++++++++++++ .../cli/test/integration/shared/db/users.ts | 4 + 7 files changed, 202 insertions(+), 22 deletions(-) create mode 100644 packages/cli/test/integration/environments/source-control-import.service.test.ts diff --git a/packages/cli/src/databases/repositories/sharedCredentials.repository.ts b/packages/cli/src/databases/repositories/sharedCredentials.repository.ts index f6e5b1946a..4b08c2174f 100644 --- a/packages/cli/src/databases/repositories/sharedCredentials.repository.ts +++ b/packages/cli/src/databases/repositories/sharedCredentials.repository.ts @@ -23,11 +23,12 @@ export class SharedCredentialsRepository extends Repository { return sharedCredential.credentials; } - async findByCredentialIds(credentialIds: string[]) { + async findByCredentialIds(credentialIds: string[], role: CredentialSharingRole) { return await this.find({ relations: ['credentials', 'user'], where: { credentialsId: In(credentialIds), + role, }, }); } diff --git a/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts b/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts index 771d4fdee4..30c0174fa1 100644 --- a/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts +++ b/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts @@ -230,7 +230,7 @@ export class SourceControlExportService { const credentialIds = candidates.map((e) => e.id); const credentialsToBeExported = await Container.get( SharedCredentialsRepository, - ).findByCredentialIds(credentialIds); + ).findByCredentialIds(credentialIds, 'credential:owner'); let missingIds: string[] = []; if (credentialsToBeExported.length !== credentialIds.length) { const foundCredentialIds = credentialsToBeExported.map((e) => e.credentialsId); @@ -239,23 +239,26 @@ export class SourceControlExportService { ); } await Promise.all( - credentialsToBeExported.map(async (sharedCredential) => { - const { name, type, nodesAccess, data, id } = sharedCredential.credentials; - const credentialObject = new Credentials({ id, name }, type, nodesAccess, data); - const plainData = credentialObject.getData(); - const sanitizedData = this.replaceCredentialData(plainData); - const fileName = this.getCredentialsPath(sharedCredential.credentials.id); - const sanitizedCredential: ExportableCredential = { - id: sharedCredential.credentials.id, - name: sharedCredential.credentials.name, - type: sharedCredential.credentials.type, - data: sanitizedData, - nodesAccess: sharedCredential.credentials.nodesAccess, + credentialsToBeExported.map(async (sharing) => { + const { name, type, nodesAccess, data, id } = sharing.credentials; + const credentials = new Credentials({ id, name }, type, nodesAccess, data); + + const stub: ExportableCredential = { + id, + name, + type, + data: this.replaceCredentialData(credentials.getData()), + nodesAccess, + ownedBy: sharing.user.email, }; - this.logger.debug(`Writing credential ${sharedCredential.credentials.id} to ${fileName}`); - return await fsWriteFile(fileName, JSON.stringify(sanitizedCredential, null, 2)); + + const filePath = this.getCredentialsPath(id); + this.logger.debug(`Writing credentials stub "${name}" (ID ${id}) to: ${filePath}`); + + return await fsWriteFile(filePath, JSON.stringify(stub, null, 2)); }), ); + return { count: credentialsToBeExported.length, folder: this.credentialExportFolder, diff --git a/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts b/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts index 25626d1a66..bf0adc6bb0 100644 --- a/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts +++ b/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts @@ -310,7 +310,10 @@ export class SourceControlImportService { }>; } - public async importCredentialsFromWorkFolder(candidates: SourceControlledFile[], userId: string) { + public async importCredentialsFromWorkFolder( + candidates: SourceControlledFile[], + importingUserId: string, + ) { const candidateIds = candidates.map((c) => c.id); const existingCredentials = await Container.get(CredentialsRepository).find({ where: { @@ -335,9 +338,6 @@ export class SourceControlImportService { const existingCredential = existingCredentials.find( (e) => e.id === credential.id && e.type === credential.type, ); - const sharedOwner = existingSharedCredentials.find( - (e) => e.credentialsId === credential.id, - ); const { name, type, data, id, nodesAccess } = credential; const newCredentialObject = new Credentials({ id, name }, type, []); @@ -351,10 +351,23 @@ export class SourceControlImportService { this.logger.debug(`Updating credential id ${newCredentialObject.id as string}`); await Container.get(CredentialsRepository).upsert(newCredentialObject, ['id']); - if (!sharedOwner) { + const isOwnedLocally = existingSharedCredentials.some( + (c) => c.credentialsId === credential.id, + ); + + if (!isOwnedLocally) { + const remoteOwnerId = credential.ownedBy + ? await Container.get(UserRepository) + .findOne({ + where: { email: credential.ownedBy }, + select: { id: true }, + }) + .then((user) => user?.id) + : null; + const newSharedCredential = new SharedCredentials(); newSharedCredential.credentialsId = newCredentialObject.id as string; - newSharedCredential.userId = userId; + newSharedCredential.userId = remoteOwnerId ?? importingUserId; newSharedCredential.role = 'credential:owner'; await Container.get(SharedCredentialsRepository).upsert({ ...newSharedCredential }, [ diff --git a/packages/cli/src/environments/sourceControl/types/exportableCredential.ts b/packages/cli/src/environments/sourceControl/types/exportableCredential.ts index 917b74132c..2bf8903cc9 100644 --- a/packages/cli/src/environments/sourceControl/types/exportableCredential.ts +++ b/packages/cli/src/environments/sourceControl/types/exportableCredential.ts @@ -6,4 +6,10 @@ export interface ExportableCredential { type: string; data: ICredentialDataDecryptedObject; nodesAccess: ICredentialNodeAccess[]; + + /** + * Email of the user who owns this credential at the source instance. + * Ownership is mirrored at target instance if user is also present there. + */ + ownedBy: string | null; } diff --git a/packages/cli/src/environments/sourceControl/types/sourceControlPullWorkFolder.ts b/packages/cli/src/environments/sourceControl/types/sourceControlPullWorkFolder.ts index e91f615fac..b87c970f0e 100644 --- a/packages/cli/src/environments/sourceControl/types/sourceControlPullWorkFolder.ts +++ b/packages/cli/src/environments/sourceControl/types/sourceControlPullWorkFolder.ts @@ -19,6 +19,7 @@ export class SourceControlPullWorkFolder { } export class SourceControllPullOptions { + /** ID of user performing a source control pull. */ userId: string; force?: boolean; diff --git a/packages/cli/test/integration/environments/source-control-import.service.test.ts b/packages/cli/test/integration/environments/source-control-import.service.test.ts new file mode 100644 index 0000000000..d0615f1dd4 --- /dev/null +++ b/packages/cli/test/integration/environments/source-control-import.service.test.ts @@ -0,0 +1,152 @@ +import fsp from 'node:fs/promises'; +import Container from 'typedi'; +import { mock } from 'jest-mock-extended'; +import * as utils from 'n8n-workflow'; +import { Cipher } from 'n8n-core'; +import { nanoid } from 'nanoid'; +import type { InstanceSettings } from 'n8n-core'; + +import * as testDb from '../shared/testDb'; +import { SourceControlImportService } from '@/environments/sourceControl/sourceControlImport.service.ee'; +import { createMember, getGlobalOwner } from '../shared/db/users'; +import { SharedCredentialsRepository } from '@/databases/repositories/sharedCredentials.repository'; +import { mockInstance } from '../../shared/mocking'; +import type { SourceControlledFile } from '@/environments/sourceControl/types/sourceControlledFile'; +import type { ExportableCredential } from '@/environments/sourceControl/types/exportableCredential'; + +describe('SourceControlImportService', () => { + let service: SourceControlImportService; + const cipher = mockInstance(Cipher); + + beforeAll(async () => { + service = new SourceControlImportService( + mock(), + mock(), + mock(), + mock(), + mock({ n8nFolder: '/some-path' }), + ); + + await testDb.init(); + }); + + afterEach(async () => { + await testDb.truncate(['Credentials', 'SharedCredentials']); + + jest.restoreAllMocks(); + }); + + afterAll(async () => { + await testDb.terminate(); + }); + + describe('importCredentialsFromWorkFolder()', () => { + describe('if user email specified by `ownedBy` exists at target instance', () => { + it('should assign credential ownership to original user', async () => { + const [importingUser, member] = await Promise.all([getGlobalOwner(), createMember()]); + + fsp.readFile = jest.fn().mockResolvedValue(Buffer.from('some-content')); + + const CREDENTIAL_ID = nanoid(); + + const stub: ExportableCredential = { + id: CREDENTIAL_ID, + name: 'My Credential', + type: 'someCredentialType', + data: {}, + nodesAccess: [], + ownedBy: member.email, // user at source instance owns credential + }; + + jest.spyOn(utils, 'jsonParse').mockReturnValue(stub); + + cipher.encrypt.mockReturnValue('some-encrypted-data'); + + await service.importCredentialsFromWorkFolder( + [mock({ id: CREDENTIAL_ID })], + importingUser.id, + ); + + const sharing = await Container.get(SharedCredentialsRepository).findOneBy({ + credentialsId: CREDENTIAL_ID, + userId: member.id, + role: 'credential:owner', + }); + + expect(sharing).toBeTruthy(); // same user at target instance owns credential + }); + }); + + describe('if user email specified by `ownedBy` is `null`', () => { + it('should assign credential ownership to importing user', async () => { + const importingUser = await getGlobalOwner(); + + fsp.readFile = jest.fn().mockResolvedValue(Buffer.from('some-content')); + + const CREDENTIAL_ID = nanoid(); + + const stub: ExportableCredential = { + id: CREDENTIAL_ID, + name: 'My Credential', + type: 'someCredentialType', + data: {}, + nodesAccess: [], + ownedBy: null, + }; + + jest.spyOn(utils, 'jsonParse').mockReturnValue(stub); + + cipher.encrypt.mockReturnValue('some-encrypted-data'); + + await service.importCredentialsFromWorkFolder( + [mock({ id: CREDENTIAL_ID })], + importingUser.id, + ); + + const sharing = await Container.get(SharedCredentialsRepository).findOneBy({ + credentialsId: CREDENTIAL_ID, + userId: importingUser.id, + role: 'credential:owner', + }); + + expect(sharing).toBeTruthy(); // original user has no email, so importing user owns credential + }); + }); + + describe('if user email specified by `ownedBy` does not exist at target instance', () => { + it('should assign credential ownership to importing user', async () => { + const importingUser = await getGlobalOwner(); + + fsp.readFile = jest.fn().mockResolvedValue(Buffer.from('some-content')); + + const CREDENTIAL_ID = nanoid(); + + const stub: ExportableCredential = { + id: CREDENTIAL_ID, + name: 'My Credential', + type: 'someCredentialType', + data: {}, + nodesAccess: [], + ownedBy: 'user@test.com', // user at source instance owns credential + }; + + jest.spyOn(utils, 'jsonParse').mockReturnValue(stub); + + cipher.encrypt.mockReturnValue('some-encrypted-data'); + + await service.importCredentialsFromWorkFolder( + [mock({ id: CREDENTIAL_ID })], + importingUser.id, + ); + + const sharing = await Container.get(SharedCredentialsRepository).findOneBy({ + credentialsId: CREDENTIAL_ID, + userId: importingUser.id, + role: 'credential:owner', + }); + + expect(sharing).toBeTruthy(); // original user missing, so importing user owns credential + }); + }); + }); +}); diff --git a/packages/cli/test/integration/shared/db/users.ts b/packages/cli/test/integration/shared/db/users.ts index 2ee01524bf..80508191ad 100644 --- a/packages/cli/test/integration/shared/db/users.ts +++ b/packages/cli/test/integration/shared/db/users.ts @@ -137,3 +137,7 @@ export const getLdapIdentities = async () => where: { providerType: 'ldap' }, relations: ['user'], }); + +export async function getGlobalOwner() { + return await Container.get(UserRepository).findOneByOrFail({ role: 'global:owner' }); +}