fix: Detect resource owner change in source control (#20811)

packages/cli/src/environments.ee/source-control/__tests__/source-control-helper.ee.test.ts

@@ -10,6 +10,7 @@ import {
 	SOURCE_CONTROL_GIT_FOLDER,
 } from '@/environments.ee/source-control/constants';
 import {
+	areOwnersDifferent,
 	generateSshKeyPair,
 	getRepoType,
 	getTrackingInformationFromPostPushResult,
@@ -312,6 +313,76 @@ describe('isWorkflowModified', () => {
 
 		expect(isWorkflowModified(local, remote)).toBe(false);
 	});
+
+	it('should detect modifications when owner changes', () => {
+		const local = createWorkflowVersion({
+			owner: {
+				type: 'personal',
+				projectId: 'project1',
+				projectName: 'Project 1',
+			},
+		});
+		const remote = createWorkflowVersion({
+			owner: {
+				type: 'team',
+				projectId: 'team1',
+				projectName: 'Team 1',
+			},
+		});
+
+		expect(isWorkflowModified(local, remote)).toBe(true);
+	});
+});
+
+describe('areOwnersDifferent', () => {
+	it('should return true if the owners are different', () => {
+		const owner1 = {
+			type: 'personal' as const,
+			projectId: 'personal1',
+			projectName: 'Personal 1',
+		};
+		const owner2 = {
+			type: 'team' as const,
+			projectId: 'team1',
+			projectName: 'Team 1',
+		};
+
+		expect(areOwnersDifferent(owner1, owner2)).toBe(true);
+	});
+
+	it('should return false if the owners are the same', () => {
+		const owner = {
+			type: 'personal' as const,
+			projectId: 'personal1',
+			projectName: 'Personal 1',
+		};
+
+		expect(areOwnersDifferent(owner, { ...owner })).toBe(false);
+	});
+
+	describe('both owners undefined/null', () => {
+		it('should return false when both owners are undefined', () => {
+			expect(areOwnersDifferent(undefined, undefined)).toBe(false);
+			expect(areOwnersDifferent(undefined, null as any)).toBe(false);
+			expect(areOwnersDifferent(null as any, null as any)).toBe(false);
+		});
+	});
+
+	describe('one owner undefined', () => {
+		const owner = {
+			type: 'personal' as const,
+			projectId: 'project1',
+			projectName: 'Project 1',
+		};
+
+		it('should return true when owner1 is undefined and owner2 is defined', () => {
+			expect(areOwnersDifferent(undefined, owner)).toBe(true);
+		});
+
+		it('should return true when owner1 is defined and owner2 is undefined', () => {
+			expect(areOwnersDifferent(owner, undefined)).toBe(true);
+		});
+	});
 });
 
 describe('readTagAndMappingsFromSourceControlFile', () => {

packages/cli/src/environments.ee/source-control/__tests__/source-control-status.service.test.ts

@@ -705,4 +705,411 @@ describe('getStatus', () => {
 			});
 		});
 	});
+
+	describe('detect owner changes', () => {
+		describe('credentials', () => {
+			const mockCredentials = {
+				withOwner: {
+					id: 'cred1',
+					name: 'Test Credential',
+					type: 'testApi',
+					data: {},
+					filename: '/mock/n8n/git/credentials/cred1.json',
+					ownedBy: {
+						type: 'personal' as const,
+						projectId: 'project1',
+						projectName: 'Project 1',
+					},
+				} as StatusExportableCredential,
+				withDifferentOwner: {
+					id: 'cred1',
+					name: 'Test Credential',
+					type: 'testApi',
+					data: {},
+					filename: '/mock/n8n/git/credentials/cred1.json',
+					ownedBy: {
+						type: 'personal' as const,
+						projectId: 'project2',
+						projectName: 'Project 2',
+					},
+				} as StatusExportableCredential,
+			};
+
+			const user = mock<User>({ role: GLOBAL_ADMIN_ROLE });
+
+			it('should detect credential as modified when owner changes', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteCredentialsFromFiles.mockResolvedValue([
+					mockCredentials.withDifferentOwner,
+				]);
+				sourceControlImportService.getLocalCredentialsFromDb.mockResolvedValue([
+					mockCredentials.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.credModifiedInEither).toHaveLength(1);
+				expect(result.credModifiedInEither[0]).toMatchObject({
+					id: mockCredentials.withOwner.id,
+					name: mockCredentials.withDifferentOwner.name,
+					type: mockCredentials.withOwner.type,
+				});
+			});
+
+			it('should not detect change when there is no change', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteCredentialsFromFiles.mockResolvedValue([
+					mockCredentials.withOwner,
+				]);
+				sourceControlImportService.getLocalCredentialsFromDb.mockResolvedValue([
+					mockCredentials.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.credModifiedInEither).toHaveLength(0);
+				expect(result.sourceControlledFiles.filter((f) => f.type === 'credential')).toHaveLength(0);
+			});
+
+			it('should correctly populate owner field in sourceControlledFiles for modified credentials', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteCredentialsFromFiles.mockResolvedValue([
+					mockCredentials.withDifferentOwner,
+				]);
+				sourceControlImportService.getLocalCredentialsFromDb.mockResolvedValue([
+					mockCredentials.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: false,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (!Array.isArray(result)) {
+					fail('Expected result to be an array.');
+				}
+
+				const credentialFile = result.find((f) => f.type === 'credential');
+				expect(credentialFile).toBeDefined();
+				expect(credentialFile?.owner).toEqual(mockCredentials.withOwner.ownedBy);
+				expect(credentialFile?.status).toBe('modified');
+				expect(credentialFile?.conflict).toBe(true);
+			});
+
+			it('should detect modifications when both name/type and owner change', async () => {
+				// ARRANGE
+				const remoteCredential = {
+					...mockCredentials.withDifferentOwner,
+					name: 'Different Name',
+					type: 'differentApi',
+				};
+
+				sourceControlImportService.getRemoteCredentialsFromFiles.mockResolvedValue([
+					remoteCredential,
+				]);
+				sourceControlImportService.getLocalCredentialsFromDb.mockResolvedValue([
+					mockCredentials.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.credModifiedInEither).toHaveLength(1);
+				expect(result.credModifiedInEither[0]).toMatchObject({
+					id: mockCredentials.withOwner.id,
+					name: remoteCredential.name,
+					type: mockCredentials.withOwner.type,
+				});
+			});
+		});
+
+		describe('workflows', () => {
+			const mockWorkflows = {
+				withOwner: {
+					id: 'wf1',
+					name: 'Test Workflow',
+					versionId: 'version1',
+					filename: 'workflows/wf1.json',
+					parentFolderId: 'folder1',
+					updatedAt: '2023-07-10T10:10:59.000Z',
+					owner: {
+						type: 'personal' as const,
+						projectId: 'project1',
+						projectName: 'Project 1',
+					},
+				},
+				withDifferentOwner: {
+					id: 'wf1',
+					name: 'Test Workflow',
+					versionId: 'version1',
+					filename: 'workflows/wf1.json',
+					parentFolderId: 'folder1',
+					updatedAt: '2023-07-10T10:10:59.000Z',
+					owner: {
+						type: 'team' as const,
+						projectId: 'team1',
+						projectName: 'Team 1',
+					},
+				},
+			};
+
+			const user = mock<User>({ role: GLOBAL_ADMIN_ROLE });
+
+			it('should detect workflow as modified when owner changes', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteVersionIdsFromFiles.mockResolvedValue([
+					mockWorkflows.withDifferentOwner,
+				]);
+				sourceControlImportService.getLocalVersionIdsFromDb.mockResolvedValue([
+					mockWorkflows.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.wfModifiedInEither).toHaveLength(1);
+				expect(result.wfModifiedInEither[0]).toMatchObject({
+					id: mockWorkflows.withOwner.id,
+					versionId: mockWorkflows.withDifferentOwner.versionId,
+				});
+			});
+
+			it('should not detect workflow as modified when versionId, parentFolderId, and owner are the same', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteVersionIdsFromFiles.mockResolvedValue([
+					mockWorkflows.withOwner,
+				]);
+				sourceControlImportService.getLocalVersionIdsFromDb.mockResolvedValue([
+					mockWorkflows.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.wfModifiedInEither).toHaveLength(0);
+				expect(result.sourceControlledFiles.filter((f) => f.type === 'workflow')).toHaveLength(0);
+			});
+
+			it('should correctly populate owner field in sourceControlledFiles for modified workflows', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteVersionIdsFromFiles.mockResolvedValue([
+					mockWorkflows.withDifferentOwner,
+				]);
+				sourceControlImportService.getLocalVersionIdsFromDb.mockResolvedValue([
+					mockWorkflows.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: false,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (!Array.isArray(result)) {
+					fail('Expected result to be an array.');
+				}
+
+				const workflowFile = result.find((f) => f.type === 'workflow');
+				expect(workflowFile).toBeDefined();
+				expect(workflowFile?.owner).toEqual(mockWorkflows.withOwner.owner);
+				expect(workflowFile?.status).toBe('modified');
+				expect(workflowFile?.conflict).toBe(true);
+			});
+
+			it('should detect modifications when both versionId/parentFolderId and owner change', async () => {
+				// ARRANGE
+				const remoteWorkflow = {
+					...mockWorkflows.withDifferentOwner,
+					versionId: 'version2',
+					parentFolderId: 'folder2',
+				};
+
+				sourceControlImportService.getRemoteVersionIdsFromFiles.mockResolvedValue([remoteWorkflow]);
+				sourceControlImportService.getLocalVersionIdsFromDb.mockResolvedValue([
+					mockWorkflows.withOwner,
+				]);
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.wfModifiedInEither).toHaveLength(1);
+				expect(result.wfModifiedInEither[0]).toMatchObject({
+					id: mockWorkflows.withOwner.id,
+					versionId: remoteWorkflow.versionId,
+				});
+			});
+		});
+
+		describe('folders', () => {
+			const mockFolders = {
+				withProject: {
+					id: 'folder1',
+					name: 'Test Folder',
+					homeProjectId: 'project1',
+					parentFolderId: null,
+					createdAt: '2023-07-10T10:10:59.000Z',
+					updatedAt: '2023-07-10T10:10:59.000Z',
+				},
+				withDifferentProject: {
+					id: 'folder1',
+					name: 'Test Folder',
+					homeProjectId: 'project2',
+					parentFolderId: null,
+					createdAt: '2023-07-10T10:10:59.000Z',
+					updatedAt: '2023-07-10T10:10:59.000Z',
+				},
+			};
+
+			const user = mock<User>({ role: GLOBAL_ADMIN_ROLE });
+
+			it('should detect folder as modified when homeProjectId changes', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteFoldersAndMappingsFromFile.mockResolvedValue({
+					folders: [mockFolders.withDifferentProject],
+				});
+				sourceControlImportService.getLocalFoldersAndMappingsFromDb.mockResolvedValue({
+					folders: [mockFolders.withProject],
+				});
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.foldersModifiedInEither).toHaveLength(1);
+				expect(result.foldersModifiedInEither[0]).toMatchObject({
+					id: mockFolders.withProject.id,
+					homeProjectId: mockFolders.withDifferentProject.homeProjectId,
+				});
+			});
+
+			it('should not detect folder as modified when name, parentFolderId, and homeProjectId are the same', async () => {
+				// ARRANGE
+				sourceControlImportService.getRemoteFoldersAndMappingsFromFile.mockResolvedValue({
+					folders: [mockFolders.withProject],
+				});
+				sourceControlImportService.getLocalFoldersAndMappingsFromDb.mockResolvedValue({
+					folders: [mockFolders.withProject],
+				});
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.foldersModifiedInEither).toHaveLength(0);
+				expect(result.sourceControlledFiles.filter((f) => f.type === 'folders')).toHaveLength(0);
+			});
+
+			it('should detect modifications when both name/parentFolderId and homeProjectId change', async () => {
+				// ARRANGE
+				const remoteFolder = {
+					...mockFolders.withDifferentProject,
+					name: 'Different Folder Name',
+					parentFolderId: 'parent1',
+				};
+
+				sourceControlImportService.getRemoteFoldersAndMappingsFromFile.mockResolvedValue({
+					folders: [remoteFolder],
+				});
+				sourceControlImportService.getLocalFoldersAndMappingsFromDb.mockResolvedValue({
+					folders: [mockFolders.withProject],
+				});
+
+				// ACT
+				const result = await sourceControlStatusService.getStatus(user, {
+					direction: 'push',
+					verbose: true,
+					preferLocalVersion: false,
+				});
+
+				// ASSERT
+				if (Array.isArray(result)) {
+					fail('Expected result to be an object.');
+				}
+
+				expect(result.foldersModifiedInEither).toHaveLength(1);
+				expect(result.foldersModifiedInEither[0]).toMatchObject({
+					id: mockFolders.withProject.id,
+					name: remoteFolder.name,
+					homeProjectId: remoteFolder.homeProjectId,
+				});
+			});
+		});
+	});
 });

packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts

@@ -21,6 +21,7 @@ import type { ExportedFolders } from './types/exportable-folders';
 import type { KeyPair } from './types/key-pair';
 import type { KeyPairType } from './types/key-pair-type';
 import type { SourceControlWorkflowVersionId } from './types/source-control-workflow-version-id';
+import type { StatusResourceOwner } from './types/resource-owner';
 
 export function stringContainsExpression(testString: string): boolean {
 	return /^=.*\{\{.*\}\}/.test(testString);
@@ -246,6 +247,15 @@ export function normalizeAndValidateSourceControlledFilePath(
 	return normalizedPath;
 }
 
+export function areOwnersDifferent(
+	owner1?: StatusResourceOwner,
+	owner2?: StatusResourceOwner,
+): boolean {
+	if (!owner1 && !owner2) return false;
+
+	return owner1?.projectId !== owner2?.projectId;
+}
+
 /**
  * Checks if a workflow has been modified by comparing version IDs and parent folder IDs
  * between local and remote versions
@@ -254,8 +264,10 @@ export function isWorkflowModified(
 	local: SourceControlWorkflowVersionId,
 	remote: SourceControlWorkflowVersionId,
 ): boolean {
-	return (
-		remote.versionId !== local.versionId ||
-		(remote.parentFolderId !== undefined && remote.parentFolderId !== local.parentFolderId)
-	);
+	const hasVersionIdChanged = remote.versionId !== local.versionId;
+	const hasParentFolderIdChanged =
+		remote.parentFolderId !== undefined && remote.parentFolderId !== local.parentFolderId;
+	const hasOwnerChanged = areOwnersDifferent(remote.owner, local.owner);
+
+	return hasVersionIdChanged || hasParentFolderIdChanged || hasOwnerChanged;
 }

packages/cli/src/environments.ee/source-control/source-control-import.service.ee.ts

@@ -248,6 +248,7 @@ export class SourceControlImportService {
 					project: {
 						projectRelations: {
 							user: true,
+							role: true,
 						},
 					},
 				},
@@ -266,6 +267,8 @@ export class SourceControlImportService {
 						name: true,
 						type: true,
 						projectRelations: {
+							// Even if the userId is not used, it seems that this is needed to get the other nested properties populated
+							userId: true,
 							role: {
 								slug: true,
 							},
@@ -293,6 +296,7 @@ export class SourceControlImportService {
 				});
 				updatedAt = isNaN(Date.parse(local.updatedAt)) ? new Date() : new Date(local.updatedAt);
 			}
+
 			const remoteOwnerProject = local.shared?.find((s) => s.role === 'workflow:owner')?.project;
 
 			return {
@@ -371,6 +375,7 @@ export class SourceControlImportService {
 					project: {
 						projectRelations: {
 							user: true,
+							role: true,
 						},
 					},
 				},
@@ -385,6 +390,8 @@ export class SourceControlImportService {
 						name: true,
 						type: true,
 						projectRelations: {
+							// Even if the userId is not used, it seems that this is needed to get the other nested properties populated
+							userId: true,
 							role: {
 								slug: true,
 							},

packages/cli/src/environments.ee/source-control/source-control-status.service.ee.ts

@@ -10,6 +10,7 @@ import { EventService } from '@/events/event.service';
 
 import { SourceControlGitService } from './source-control-git.service.ee';
 import {
+	areOwnersDifferent,
 	getFoldersPath,
 	getTagsPath,
 	getTrackingInformationFromPrePushResult,
@@ -303,12 +304,18 @@ export class SourceControlStatusService {
 			(local) => credRemoteIds.findIndex((remote) => remote.id === local.id) === -1,
 		);
 
-		// only compares the name, since that is the only change synced for credentials
 		const credModifiedInEither: StatusExportableCredential[] = [];
 		credLocalIds.forEach((local) => {
+			// Compare name, type and owner since those are the synced properties for credentials
 			const mismatchingCreds = credRemoteIds.find((remote) => {
-				return remote.id === local.id && (remote.name !== local.name || remote.type !== local.type);
+				return (
+					remote.id === local.id &&
+					(remote.name !== local.name ||
+						remote.type !== local.type ||
+						areOwnersDifferent(remote.ownedBy, local.ownedBy))
+				);
 			});
+
 			if (mismatchingCreds) {
 				credModifiedInEither.push({
 					...local,
@@ -358,6 +365,7 @@ export class SourceControlStatusService {
 				owner: item.ownedBy,
 			});
 		});
+
 		return {
 			credMissingInLocal,
 			credMissingInRemote,
@@ -569,12 +577,15 @@ export class SourceControlStatusService {
 			const mismatchingIds = foldersMappingsRemote.folders.find(
 				(remote) =>
 					remote.id === local.id &&
-					(remote.name !== local.name || remote.parentFolderId !== local.parentFolderId),
+					(remote.name !== local.name ||
+						remote.parentFolderId !== local.parentFolderId ||
+						remote.homeProjectId !== local.homeProjectId),
 			);
 
 			if (!mismatchingIds) {
 				return;
 			}
+
 			foldersModifiedInEither.push(options.preferLocalVersion ? local : mismatchingIds);
 		});