fix(core): Assign credential ownership correctly in source control import (#8955)

packages/cli/src/databases/repositories/sharedCredentials.repository.ts

@@ -23,11 +23,12 @@ export class SharedCredentialsRepository extends Repository<SharedCredentials> {
 		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,
 			},
 		});
 	}

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,

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 }, [

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;
 }

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;

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

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