From 96a0380a6d3ff6cdfa1770bf90a5fa4cfd84ae8f Mon Sep 17 00:00:00 2001 From: Ali Elkhateeb Date: Wed, 20 May 2026 12:13:48 +0300 Subject: [PATCH] fix(core): Require admin push when migrating legacy environments to projects folder (#30530) --- .../source-control-git.service.test.ts | 59 +++++++++++++++++- .../source-control.controller.ee.test.ts | 8 ++- .../__tests__/source-control.service.test.ts | 41 +++++++++++++ .../source-control/constants.ts | 2 + .../source-control-git.service.ee.ts | 37 +++++++++++ .../source-control.service.ee.ts | 24 +++++++- .../source-control.service.test.ts | 61 +++++++++++++++++-- .../components/MainHeader/WorkflowDetails.vue | 24 ++------ .../MainSidebarSourceControl.test.ts | 9 +++ .../components/MainSidebarSourceControl.vue | 29 ++------- .../components/SourceControlPushModal.vue | 4 +- .../sourceControl.store.test.ts | 27 ++++++++ .../sourceControl.ee/sourceControl.store.ts | 18 +++++- .../useSourceControlModalRouting.ts | 29 +++++++++ 14 files changed, 315 insertions(+), 57 deletions(-) create mode 100644 packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/useSourceControlModalRouting.ts diff --git a/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts b/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts index e2a3f6c6805..7215735507e 100644 --- a/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts +++ b/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts @@ -4,8 +4,8 @@ import { simpleGit } from 'simple-git'; import type { SimpleGit } from 'simple-git'; import { SourceControlGitService } from '../source-control-git.service.ee'; -import type { SourceControlPreferences } from '../types/source-control-preferences'; import type { SourceControlPreferencesService } from '../source-control-preferences.service.ee'; +import type { SourceControlPreferences } from '../types/source-control-preferences'; const MOCK_BRANCHES = { all: ['origin/master', 'origin/feature/branch'], @@ -19,6 +19,8 @@ const MOCK_BRANCHES = { const mockGitInstance = { branch: jest.fn().mockResolvedValue(MOCK_BRANCHES), env: jest.fn().mockReturnThis(), + fetch: jest.fn().mockResolvedValue(undefined), + raw: jest.fn().mockResolvedValue(''), }; jest.mock('simple-git', () => { @@ -507,4 +509,59 @@ describe('SourceControlGitService', () => { }); }); }); + + describe('requiresAdminPushForProjectsMigration', () => { + beforeEach(() => { + mockSourceControlPreferencesService.getPreferences.mockReturnValue({ + branchName: 'main', + } as SourceControlPreferences); + }); + + it('should return true when workflows exist on remote but projects do not', async () => { + mockGitInstance.raw.mockImplementation(async (args: string[]) => { + const path = args[2]; + if (path === 'workflows') { + return '040000 tree abc\tworkflows\n'; + } + if (path === 'projects') { + return ''; + } + return ''; + }); + + await expect(sourceControlGitService.requiresAdminPushForProjectsMigration()).resolves.toBe( + true, + ); + }); + + it('should return false when both workflows and projects exist on remote', async () => { + mockGitInstance.raw.mockImplementation(async (args: string[]) => { + const path = args[2]; + if (path === 'workflows' || path === 'projects') { + return `040000 tree abc\t${path}\n`; + } + return ''; + }); + + await expect(sourceControlGitService.requiresAdminPushForProjectsMigration()).resolves.toBe( + false, + ); + }); + + it('should return false when workflows do not exist on remote', async () => { + mockGitInstance.raw.mockResolvedValue(''); + + await expect(sourceControlGitService.requiresAdminPushForProjectsMigration()).resolves.toBe( + false, + ); + }); + + it('should reject when remote directory check fails (fail closed for migration guard)', async () => { + mockGitInstance.raw.mockRejectedValue(new Error('network error')); + + await expect(sourceControlGitService.requiresAdminPushForProjectsMigration()).rejects.toThrow( + 'network error', + ); + }); + }); }); diff --git a/packages/cli/src/environments.ee/source-control/__tests__/source-control.controller.ee.test.ts b/packages/cli/src/environments.ee/source-control/__tests__/source-control.controller.ee.test.ts index 3f0fefebb1d..a6637aae90a 100644 --- a/packages/cli/src/environments.ee/source-control/__tests__/source-control.controller.ee.test.ts +++ b/packages/cli/src/environments.ee/source-control/__tests__/source-control.controller.ee.test.ts @@ -7,6 +7,7 @@ import type { EventService } from '@/events/event.service'; import type { SourceControlPreferencesService } from '../source-control-preferences.service.ee'; import { SourceControlController } from '../source-control.controller.ee'; +import type { SourceControlScopedService } from '../source-control-scoped.service'; import type { SourceControlService } from '../source-control.service.ee'; import type { SourceControlRequest } from '../types/requests'; import type { SourceControlGetStatus } from '../types/source-control-get-status'; @@ -15,6 +16,7 @@ describe('SourceControlController', () => { let controller: SourceControlController; let sourceControlService: SourceControlService; let sourceControlPreferencesService: SourceControlPreferencesService; + let sourceControlScopedService: SourceControlScopedService; let eventService: EventService; beforeEach(() => { @@ -23,15 +25,19 @@ describe('SourceControlController', () => { pullWorkfolder: jest.fn().mockResolvedValue({ statusCode: 200 }), getStatus: jest.fn().mockResolvedValue([]), setGitUserDetails: jest.fn(), + sanityCheck: jest.fn().mockResolvedValue(undefined), } as unknown as SourceControlService; sourceControlPreferencesService = mock(); + sourceControlScopedService = { + ensureIsAllowedToPush: jest.fn().mockResolvedValue(undefined), + } as unknown as SourceControlScopedService; eventService = mock(); controller = new SourceControlController( sourceControlService, sourceControlPreferencesService, - mock(), + sourceControlScopedService, eventService, ); }); diff --git a/packages/cli/src/environments.ee/source-control/__tests__/source-control.service.test.ts b/packages/cli/src/environments.ee/source-control/__tests__/source-control.service.test.ts index 186d4c34b3e..550c78fd5d4 100644 --- a/packages/cli/src/environments.ee/source-control/__tests__/source-control.service.test.ts +++ b/packages/cli/src/environments.ee/source-control/__tests__/source-control.service.test.ts @@ -6,6 +6,7 @@ import { mock } from 'jest-mock-extended'; import { InstanceSettings } from 'n8n-core'; import type { PushResult } from 'simple-git'; +import { SOURCE_CONTROL_ADMIN_PUSH_REQUIRED_MESSAGE } from '@/environments.ee/source-control/constants'; import { SourceControlPreferencesService } from '@/environments.ee/source-control/source-control-preferences.service.ee'; import { SourceControlService } from '@/environments.ee/source-control/source-control.service.ee'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; @@ -59,6 +60,7 @@ describe('SourceControlService', () => { beforeEach(() => { jest.resetAllMocks(); jest.spyOn(sourceControlService, 'sanityCheck').mockResolvedValue(undefined); + gitService.requiresAdminPushForProjectsMigration.mockResolvedValue(false); // Reset mock implementations mockStatusService.getStatus.mockReset(); }); @@ -268,6 +270,45 @@ describe('SourceControlService', () => { }); }); + it('should throw ForbiddenError when projects migration requires admin push', async () => { + const user = Object.assign(new User(), { + role: GLOBAL_MEMBER_ROLE, + }); + gitService.requiresAdminPushForProjectsMigration.mockResolvedValueOnce(true); + + await expect( + sourceControlService.pushWorkfolder(user, { + fileNames: [], + commitMessage: 'Test', + }), + ).rejects.toThrow(new ForbiddenError(SOURCE_CONTROL_ADMIN_PUSH_REQUIRED_MESSAGE)); + + expect(mockStatusService.getStatus).not.toHaveBeenCalled(); + }); + + it('should allow instance admin to push when projects migration requires admin push', async () => { + const user = Object.assign(new User(), { + role: GLOBAL_ADMIN_ROLE, + }); + gitService.requiresAdminPushForProjectsMigration.mockResolvedValueOnce(true); + mockStatusService.getStatus.mockResolvedValueOnce([]); + sourceControlExportService.exportCredentialsToWorkFolder.mockResolvedValueOnce({ + count: 0, + missingIds: [], + folder: '', + files: [], + }); + gitService.push.mockResolvedValueOnce(mock()); + + await sourceControlService.pushWorkfolder(user, { + fileNames: [], + commitMessage: 'Test', + }); + + expect(gitService.requiresAdminPushForProjectsMigration).toHaveBeenCalled(); + expect(mockStatusService.getStatus).toHaveBeenCalled(); + }); + it('should throw an error if file path validation fails', async () => { const user = mock(); (isContainedWithin as jest.Mock).mockReturnValueOnce(false); diff --git a/packages/cli/src/environments.ee/source-control/constants.ts b/packages/cli/src/environments.ee/source-control/constants.ts index d35c67c42ef..895789cc603 100644 --- a/packages/cli/src/environments.ee/source-control/constants.ts +++ b/packages/cli/src/environments.ee/source-control/constants.ts @@ -17,3 +17,5 @@ export const SOURCE_CONTROL_README = ` `; export const SOURCE_CONTROL_DEFAULT_NAME = 'n8n user'; export const SOURCE_CONTROL_DEFAULT_EMAIL = 'n8n@example.com'; +export const SOURCE_CONTROL_ADMIN_PUSH_REQUIRED_MESSAGE = + 'The connected Git repository is missing the projects folder. An instance owner or instance admin must perform the next push so that projects are synced correctly with environments.'; diff --git a/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts b/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts index 402c694100b..c9f2f2615b8 100644 --- a/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts +++ b/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts @@ -22,6 +22,8 @@ import { SOURCE_CONTROL_DEFAULT_EMAIL, SOURCE_CONTROL_DEFAULT_NAME, SOURCE_CONTROL_ORIGIN, + SOURCE_CONTROL_PROJECT_EXPORT_FOLDER, + SOURCE_CONTROL_WORKFLOW_EXPORT_FOLDER, } from './constants'; import { sourceControlFoldersExistCheck } from './source-control-helper.ee'; import { SourceControlPreferencesService } from './source-control-preferences.service.ee'; @@ -491,4 +493,39 @@ export class SourceControlGitService { ); } } + + /** + * Instances that used environments before n8n 1.118.0 have workflows on the remote repository + * but no projects directory yet. The first push after upgrading must be done by an instance admin. + */ + async requiresAdminPushForProjectsMigration(): Promise { + const workflowsExist = await this.remoteDirectoryExists(SOURCE_CONTROL_WORKFLOW_EXPORT_FOLDER); + if (!workflowsExist) { + return false; + } + + const projectsExist = await this.remoteDirectoryExists(SOURCE_CONTROL_PROJECT_EXPORT_FOLDER); + return !projectsExist; + } + + /** + * Checks whether a directory exists on the remote tracking branch of the connected repository. + * On git or network failure, throws — callers must not treat errors as "directory absent", or + * non-admins could bypass the legacy migration admin push requirement (see LIGO-326). + */ + private async remoteDirectoryExists(directory: string): Promise { + if (!this.git) { + throw new UnexpectedError('Git is not initialized (remoteDirectoryExists)'); + } + + const branchName = this.sourceControlPreferencesService.getPreferences().branchName; + if (!branchName) { + return false; + } + + await this.fetch(); + const remoteRef = `${SOURCE_CONTROL_ORIGIN}/${branchName}`; + const result = await this.git.raw(['ls-tree', remoteRef, directory]); + return result.trim().length > 0; + } } diff --git a/packages/cli/src/environments.ee/source-control/source-control.service.ee.ts b/packages/cli/src/environments.ee/source-control/source-control.service.ee.ts index f48040f21df..8ef2c726a65 100644 --- a/packages/cli/src/environments.ee/source-control/source-control.service.ee.ts +++ b/packages/cli/src/environments.ee/source-control/source-control.service.ee.ts @@ -7,12 +7,14 @@ import { Logger } from '@n8n/backend-common'; import { type User } from '@n8n/db'; import { OnPubSubEvent } from '@n8n/decorators'; import { Service } from '@n8n/di'; +import { hasGlobalScope } from '@n8n/permissions'; import { writeFileSync } from 'fs'; import { UnexpectedError, UserError, jsonParse } from 'n8n-workflow'; import * as path from 'path'; import type { PushResult } from 'simple-git'; import { + SOURCE_CONTROL_ADMIN_PUSH_REQUIRED_MESSAGE, SOURCE_CONTROL_DEFAULT_EMAIL, SOURCE_CONTROL_DEFAULT_NAME, SOURCE_CONTROL_README, @@ -243,8 +245,11 @@ export class SourceControlService { return await this.gitService.setBranch(branch); } - // will reset the branch to the remote branch and pull - // this will discard all local changes + /** + * Resets the current local branch to match its remote counterpart, discarding all local changes. + * This operation overwrites any uncommitted or divergent local changes with the state from the remote branch. + * Use with caution, as this action cannot be undone. + */ async resetWorkfolder(): Promise { if (!this.gitService.git) { await this.initGitService(); @@ -275,6 +280,8 @@ export class SourceControlService { throw new BadRequestError('Cannot push onto read-only branch.'); } + await this.ensureCanPushWorkfolder(user); + const context = new SourceControlContext(user); let filesToPush: SourceControlledFile[] = options.fileNames.map((file) => { @@ -497,6 +504,11 @@ export class SourceControlService { async getStatus(user: User, options: SourceControlGetStatus) { await this.sanityCheck(); + + if (options.direction === 'push') { + await this.ensureCanPushWorkfolder(user); + } + return await this.sourceControlStatusService.getStatus(user, options); } @@ -542,4 +554,12 @@ export class SourceControlService { throw new BadRequestError(`Unsupported file type: ${type}`); } } + + private async ensureCanPushWorkfolder(user: User): Promise { + if (await this.gitService.requiresAdminPushForProjectsMigration()) { + if (!hasGlobalScope(user, 'sourceControl:push')) { + throw new ForbiddenError(SOURCE_CONTROL_ADMIN_PUSH_REQUIRED_MESSAGE); + } + } + } } diff --git a/packages/cli/test/integration/environments/source-control.service.test.ts b/packages/cli/test/integration/environments/source-control.service.test.ts index c05f5da159f..7b1479ee5ec 100644 --- a/packages/cli/test/integration/environments/source-control.service.test.ts +++ b/packages/cli/test/integration/environments/source-control.service.test.ts @@ -12,17 +12,14 @@ import { WorkflowEntity, } from '@n8n/db'; import { Container } from '@n8n/di'; -import { createCredentials } from '@test-integration/db/credentials'; -import { createFolder } from '@test-integration/db/folders'; -import { assignTagToWorkflow, createTag, updateTag } from '@test-integration/db/tags'; -import { createUser } from '@test-integration/db/users'; import * as fastGlob from 'fast-glob'; -import { mock } from 'jest-mock-extended'; +import { mock, type MockProxy } from 'jest-mock-extended'; import { Cipher } from 'n8n-core'; import fsp from 'node:fs/promises'; import { basename, isAbsolute } from 'node:path'; import { + SOURCE_CONTROL_ADMIN_PUSH_REQUIRED_MESSAGE, SOURCE_CONTROL_CREDENTIAL_EXPORT_FOLDER, SOURCE_CONTROL_FOLDERS_EXPORT_FILE, SOURCE_CONTROL_TAGS_EXPORT_FILE, @@ -42,6 +39,10 @@ import type { RemoteResourceOwner } from '@/environments.ee/source-control/types import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { EventService } from '@/events/event.service'; +import { createCredentials } from '@test-integration/db/credentials'; +import { createFolder } from '@test-integration/db/folders'; +import { assignTagToWorkflow, createTag, updateTag } from '@test-integration/db/tags'; +import { createUser } from '@test-integration/db/users'; jest.mock('fast-glob'); @@ -175,7 +176,7 @@ describe('SourceControlService', () => { let deletedOutOfScopeCredential: CredentialsEntity; let deletedInScopeCredential: CredentialsEntity; - let gitService: SourceControlGitService; + let gitService: MockProxy; let service: SourceControlService; let statusService: SourceControlStatusService; @@ -426,6 +427,7 @@ describe('SourceControlService', () => { }; gitService = mock(); + gitService.requiresAdminPushForProjectsMigration.mockResolvedValue(false); statusService = Container.get(SourceControlStatusService); service = new SourceControlService( @@ -810,6 +812,7 @@ describe('SourceControlService', () => { }); beforeEach(() => { + gitService.requiresAdminPushForProjectsMigration.mockResolvedValue(false); fsWriteFile.mockImplementation(async (path, data) => { updatedFiles[path as string] = data as string; }); @@ -1130,6 +1133,52 @@ describe('SourceControlService', () => { }); }); + describe('legacy environments repository without projects folder', () => { + beforeEach(() => { + gitService.requiresAdminPushForProjectsMigration.mockResolvedValue(true); + }); + + afterEach(() => { + gitService.requiresAdminPushForProjectsMigration.mockResolvedValue(false); + }); + + it('should deny push for project admin', async () => { + // getStatus(direction: 'push') also enforces migration; load changes without that guard first + gitService.requiresAdminPushForProjectsMigration.mockResolvedValue(false); + const allChanges = (await service.getStatus(projectAdmin, { + direction: 'push', + preferLocalVersion: true, + verbose: false, + })) as SourceControlledFile[]; + gitService.requiresAdminPushForProjectsMigration.mockResolvedValue(true); + + await expect( + service.pushWorkfolder(projectAdmin, { + fileNames: allChanges, + commitMessage: 'Test', + force: true, + }), + ).rejects.toThrow(new ForbiddenError(SOURCE_CONTROL_ADMIN_PUSH_REQUIRED_MESSAGE)); + }); + + it('should allow push for global admin', async () => { + const allChanges = (await service.getStatus(globalAdmin, { + direction: 'push', + preferLocalVersion: true, + verbose: false, + })) as SourceControlledFile[]; + + const result = await service.pushWorkfolder(globalAdmin, { + fileNames: allChanges, + commitMessage: 'Test', + force: true, + }); + + expect(result.statusCode).toBe(200); + expect(gitService.push).toBeCalled(); + }); + }); + describe('global:member', () => { it('should deny all changes', async () => { const allChanges = (await service.getStatus(globalAdmin, { diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.vue b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.vue index a60ccdc3832..d793842540b 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.vue +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowDetails.vue @@ -28,6 +28,7 @@ import { ResourceType } from '@/features/collaboration/projects/projects.utils'; import { useProjectsStore } from '@/features/collaboration/projects/projects.store'; import { useSettingsStore } from '@/app/stores/settings.store'; import { useSourceControlStore } from '@/features/integrations/sourceControl.ee/sourceControl.store'; +import { useSourceControlModalRouting } from '@/features/integrations/sourceControl.ee/useSourceControlModalRouting'; import { useTagsStore } from '@/features/shared/tags/tags.store'; import { useUIStore } from '@/app/stores/ui.store'; import { useUsersStore } from '@/features/settings/users/users.store'; @@ -117,6 +118,7 @@ const i18n = useI18n(); const router = useRouter(); const route = useRoute(); +const { openPushModal } = useSourceControlModalRouting(); const locale = useI18n(); const telemetry = useTelemetry(); @@ -617,27 +619,9 @@ async function onWorkflowMenuSelect(action: WORKFLOW_MENU_ACTIONS): Promise ({ RouterLink: vi.fn(), })); +const mockShowError = vi.fn(); + +vi.mock('@/app/composables/useToast', () => ({ + useToast: () => ({ + showError: mockShowError, + }), +})); + const renderComponent = createComponentRenderer(MainSidebarSourceControl); describe('MainSidebarSourceControl', () => { @@ -57,6 +65,7 @@ describe('MainSidebarSourceControl', () => { sourceControlStore = useSourceControlStore(); vi.spyOn(sourceControlStore, 'isEnterpriseSourceControlEnabled', 'get').mockReturnValue(true); + mockShowError.mockReset(); }); it('should render nothing when not instance owner', async () => { diff --git a/packages/frontend/editor-ui/src/app/components/MainSidebarSourceControl.vue b/packages/frontend/editor-ui/src/app/components/MainSidebarSourceControl.vue index f4451ad9c41..6d8947f89c8 100644 --- a/packages/frontend/editor-ui/src/app/components/MainSidebarSourceControl.vue +++ b/packages/frontend/editor-ui/src/app/components/MainSidebarSourceControl.vue @@ -4,8 +4,8 @@ import { useI18n } from '@n8n/i18n'; import { hasPermission } from '@/app/utils/rbac/permissions'; import { getResourcePermissions } from '@n8n/permissions'; import { useSourceControlStore } from '@/features/integrations/sourceControl.ee/sourceControl.store'; +import { useSourceControlModalRouting } from '@/features/integrations/sourceControl.ee/useSourceControlModalRouting'; import { useProjectsStore } from '@/features/collaboration/projects/projects.store'; -import { useRoute, useRouter } from 'vue-router'; import { N8nButton, N8nIcon, N8nTooltip } from '@n8n/design-system'; defineProps<{ @@ -15,8 +15,7 @@ defineProps<{ const sourceControlStore = useSourceControlStore(); const projectStore = useProjectsStore(); const i18n = useI18n(); -const route = useRoute(); -const router = useRouter(); +const { openPushModal, openPullModal } = useSourceControlModalRouting(); const tooltipOpenDelay = ref(300); const currentBranch = computed(() => { @@ -43,26 +42,6 @@ const sourceControlAvailable = computed( sourceControlStore.isEnterpriseSourceControlEnabled && (hasPullPermission.value || hasPushPermission.value), ); - -async function pushWorkfolder() { - // Navigate to route with sourceControl param - modal will handle data loading and loading states - void router.push({ - query: { - ...route.query, - sourceControl: 'push', - }, - }); -} - -function pullWorkfolder() { - // Navigate to route with sourceControl param - modal will handle the pull operation - void router.push({ - query: { - ...route.query, - sourceControl: 'pull', - }, - }); -}