From e12df06f8d301a441c8322fff0511d959e684a0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ir=C3=A9n=C3=A9e?= Date: Wed, 15 Oct 2025 16:19:49 +0100 Subject: [PATCH] fix: Detect resource owner change in source control (#20811) --- .../source-control-helper.ee.test.ts | 71 +++ .../source-control-status.service.test.ts | 407 ++++++++++++++++++ .../source-control-helper.ee.ts | 20 +- .../source-control-import.service.ee.ts | 7 + .../source-control-status.service.ee.ts | 17 +- 5 files changed, 515 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/environments.ee/source-control/__tests__/source-control-helper.ee.test.ts b/packages/cli/src/environments.ee/source-control/__tests__/source-control-helper.ee.test.ts index 036ed8fecb7..49381ddd11d 100644 --- a/packages/cli/src/environments.ee/source-control/__tests__/source-control-helper.ee.test.ts +++ b/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', () => { diff --git a/packages/cli/src/environments.ee/source-control/__tests__/source-control-status.service.test.ts b/packages/cli/src/environments.ee/source-control/__tests__/source-control-status.service.test.ts index ba90229f558..fa9f66cb68b 100644 --- a/packages/cli/src/environments.ee/source-control/__tests__/source-control-status.service.test.ts +++ b/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({ 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({ 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({ 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, + }); + }); + }); + }); }); diff --git a/packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts b/packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts index 846f2557ea8..b33dcc5d90c 100644 --- a/packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts +++ b/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; } diff --git a/packages/cli/src/environments.ee/source-control/source-control-import.service.ee.ts b/packages/cli/src/environments.ee/source-control/source-control-import.service.ee.ts index 20af85429de..232a75c4047 100644 --- a/packages/cli/src/environments.ee/source-control/source-control-import.service.ee.ts +++ b/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, }, diff --git a/packages/cli/src/environments.ee/source-control/source-control-status.service.ee.ts b/packages/cli/src/environments.ee/source-control/source-control-status.service.ee.ts index 8c00e72d8b8..95286381063 100644 --- a/packages/cli/src/environments.ee/source-control/source-control-status.service.ee.ts +++ b/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); });