feat(core): Validate commit content for project admin role (#15687)

Co-authored-by: Guillaume Jacquart <jacquart.guillaume@gmail.com>

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

@@ -1,4 +1,5 @@
 import type { SourceControlledFile } from '@n8n/api-types';
+import { User } from '@n8n/db';
 import type { SharedCredentials } from '@n8n/db';
 import type { SharedWorkflow } from '@n8n/db';
 import type { FolderRepository } from '@n8n/db';
@@ -14,8 +15,16 @@ import fsp from 'node:fs/promises';
 
 import type { VariablesService } from '../../variables/variables.service.ee';
 import { SourceControlExportService } from '../source-control-export.service.ee';
+import type { SourceControlScopedService } from '../source-control-scoped.service';
+import { SourceControlContext } from '../types/source-control-context';
 
 describe('SourceControlExportService', () => {
+	const globalAdminContext = new SourceControlContext(
+		Object.assign(new User(), {
+			role: 'global:admin',
+		}),
+	);
+
 	const cipher = Container.get(Cipher);
 	const sharedCredentialsRepository = mock<SharedCredentialsRepository>();
 	const sharedWorkflowRepository = mock<SharedWorkflowRepository>();
@@ -24,6 +33,7 @@ describe('SourceControlExportService', () => {
 	const workflowTagMappingRepository = mock<WorkflowTagMappingRepository>();
 	const variablesService = mock<VariablesService>();
 	const folderRepository = mock<FolderRepository>();
+	const sourceControlScopedService = mock<SourceControlScopedService>();
 
 	const service = new SourceControlExportService(
 		mock(),
@@ -34,6 +44,7 @@ describe('SourceControlExportService', () => {
 		workflowRepository,
 		workflowTagMappingRepository,
 		folderRepository,
+		sourceControlScopedService,
 		mock<InstanceSettings>({ n8nFolder: '/mock/n8n' }),
 	);
 
@@ -172,7 +183,7 @@ describe('SourceControlExportService', () => {
 			workflowTagMappingRepository.find.mockResolvedValue([mock()]);
 
 			// Act
-			const result = await service.exportTagsToWorkFolder();
+			const result = await service.exportTagsToWorkFolder(globalAdminContext);
 
 			// Assert
 			expect(result.count).toBe(1);
@@ -184,7 +195,7 @@ describe('SourceControlExportService', () => {
 			tagRepository.find.mockResolvedValue([]);
 
 			// Act
-			const result = await service.exportTagsToWorkFolder();
+			const result = await service.exportTagsToWorkFolder(globalAdminContext);
 
 			// Assert
 			expect(result.count).toBe(0);
@@ -201,7 +212,7 @@ describe('SourceControlExportService', () => {
 			workflowRepository.find.mockResolvedValue([mock()]);
 
 			// Act
-			const result = await service.exportFoldersToWorkFolder();
+			const result = await service.exportFoldersToWorkFolder(globalAdminContext);
 
 			// Assert
 			expect(result.count).toBe(1);
@@ -213,7 +224,7 @@ describe('SourceControlExportService', () => {
 			folderRepository.find.mockResolvedValue([]);
 
 			// Act
-			const result = await service.exportFoldersToWorkFolder();
+			const result = await service.exportFoldersToWorkFolder(globalAdminContext);
 
 			// Assert
 			expect(result.count).toBe(0);

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

@@ -31,6 +31,7 @@ describe('SourceControlController', () => {
 		controller = new SourceControlController(
 			sourceControlService,
 			sourceControlPreferencesService,
+			mock(),
 			eventService,
 		);
 	});

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

@@ -31,13 +31,17 @@ import {
 	getFoldersPath,
 	getVariablesPath,
 	getWorkflowExportPath,
+	readFoldersFromSourceControlFile,
+	readTagAndMappingsFromSourceControlFile,
 	sourceControlFoldersExistCheck,
 	stringContainsExpression,
 } from './source-control-helper.ee';
+import { SourceControlScopedService } from './source-control-scoped.service';
 import type { ExportResult } from './types/export-result';
 import type { ExportableCredential } from './types/exportable-credential';
 import type { ExportableWorkflow } from './types/exportable-workflow';
 import type { ResourceOwner } from './types/resource-owner';
+import type { SourceControlContext } from './types/source-control-context';
 import { VariablesService } from '../variables/variables.service.ee';
 
 @Service()
@@ -57,6 +61,7 @@ export class SourceControlExportService {
 		private readonly workflowRepository: WorkflowRepository,
 		private readonly workflowTagMappingRepository: WorkflowTagMappingRepository,
 		private readonly folderRepository: FolderRepository,
+		private readonly sourceControlScopedService: SourceControlScopedService,
 		instanceSettings: InstanceSettings,
 	) {
 		this.gitFolder = path.join(instanceSettings.n8nFolder, SOURCE_CONTROL_GIT_FOLDER);
@@ -214,7 +219,7 @@ export class SourceControlExportService {
 		}
 	}
 
-	async exportFoldersToWorkFolder(): Promise<ExportResult> {
+	async exportFoldersToWorkFolder(context: SourceControlContext): Promise<ExportResult> {
 		try {
 			sourceControlFoldersExistCheck([this.gitFolder]);
 			const folders = await this.folderRepository.find({
@@ -231,6 +236,7 @@ export class SourceControlExportService {
 						id: true,
 					},
 				},
+				where: this.sourceControlScopedService.getFoldersInAdminProjectsFromContextFilter(context),
 			});
 
 			if (folders.length === 0) {
@@ -241,19 +247,38 @@ export class SourceControlExportService {
 				};
 			}
 
+			const allowedProjects =
+				await this.sourceControlScopedService.getAdminProjectsFromContext(context);
+
 			const fileName = getFoldersPath(this.gitFolder);
+
+			const existingFolders = await readFoldersFromSourceControlFile(fileName);
+
+			// keep all folders that are not accessible by the current user
+			// if allowedProjects is undefined, all folders are accessible by the current user
+			const foldersToKeepUnchanged =
+				allowedProjects === undefined
+					? []
+					: existingFolders.folders.filter((folder) => {
+							return !allowedProjects.some((project) => project.id === folder.homeProjectId);
+						});
+
+			const newFolders = foldersToKeepUnchanged.concat(
+				...folders.map((f) => ({
+					id: f.id,
+					name: f.name,
+					parentFolderId: f.parentFolder?.id ?? null,
+					homeProjectId: f.homeProject.id,
+					createdAt: f.createdAt.toISOString(),
+					updatedAt: f.updatedAt.toISOString(),
+				})),
+			);
+
 			await fsWriteFile(
 				fileName,
 				JSON.stringify(
 					{
-						folders: folders.map((f) => ({
-							id: f.id,
-							name: f.name,
-							parentFolderId: f.parentFolder?.id ?? null,
-							homeProjectId: f.homeProject.id,
-							createdAt: f.createdAt.toISOString(),
-							updatedAt: f.updatedAt.toISOString(),
-						})),
+						folders: newFolders,
 					},
 					null,
 					2,
@@ -274,7 +299,7 @@ export class SourceControlExportService {
 		}
 	}
 
-	async exportTagsToWorkFolder(): Promise<ExportResult> {
+	async exportTagsToWorkFolder(context: SourceControlContext): Promise<ExportResult> {
 		try {
 			sourceControlFoldersExistCheck([this.gitFolder]);
 			const tags = await this.tagRepository.find();
@@ -286,14 +311,33 @@ export class SourceControlExportService {
 					files: [],
 				};
 			}
-			const mappings = await this.workflowTagMappingRepository.find();
+			const mappingsOfAllowedWorkflows = await this.workflowTagMappingRepository.find({
+				where:
+					this.sourceControlScopedService.getWorkflowTagMappingInAdminProjectsFromContextFilter(
+						context,
+					),
+			});
+			const allowedWorkflows = await this.workflowRepository.find({
+				where:
+					this.sourceControlScopedService.getWorkflowsInAdminProjectsFromContextFilter(context),
+			});
 			const fileName = path.join(this.gitFolder, SOURCE_CONTROL_TAGS_EXPORT_FILE);
+			const existingTagsAndMapping = await readTagAndMappingsFromSourceControlFile(fileName);
+
+			// keep all mappings that are not accessible by the current user
+			const mappingsToKeep = existingTagsAndMapping.mappings.filter((mapping) => {
+				return !allowedWorkflows.some(
+					(allowedWorkflow) => allowedWorkflow.id === mapping.workflowId,
+				);
+			});
+
 			await fsWriteFile(
 				fileName,
 				JSON.stringify(
 					{
+						// overwrite all tags
 						tags: tags.map((tag) => ({ id: tag.id, name: tag.name })),
-						mappings,
+						mappings: mappingsToKeep.concat(mappingsOfAllowedWorkflows),
 					},
 					null,
 					2,

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

@@ -1,10 +1,12 @@
 import type { SourceControlledFile } from '@n8n/api-types';
 import { Logger } from '@n8n/backend-common';
+import type { TagEntity, WorkflowTagMapping } from '@n8n/db';
 import { Container } from '@n8n/di';
 import { generateKeyPairSync } from 'crypto';
 import { constants as fsConstants, mkdirSync, accessSync } from 'fs';
-import { UserError } from 'n8n-workflow';
+import { jsonParse, UserError } from 'n8n-workflow';
 import { ok } from 'node:assert/strict';
+import { readFile as fsReadFile } from 'node:fs/promises';
 import path from 'path';
 
 import { License } from '@/license';
@@ -16,6 +18,7 @@ import {
 	SOURCE_CONTROL_TAGS_EXPORT_FILE,
 	SOURCE_CONTROL_VARIABLES_EXPORT_FILE,
 } from './constants';
+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';
@@ -47,6 +50,22 @@ export function getFoldersPath(gitFolder: string): string {
 	return path.join(gitFolder, SOURCE_CONTROL_FOLDERS_EXPORT_FILE);
 }
 
+export async function readTagAndMappingsFromSourceControlFile(file: string): Promise<{
+	tags: TagEntity[];
+	mappings: WorkflowTagMapping[];
+}> {
+	return jsonParse<{ tags: TagEntity[]; mappings: WorkflowTagMapping[] }>(
+		await fsReadFile(file, { encoding: 'utf8' }),
+		{ fallbackValue: { tags: [], mappings: [] } },
+	);
+}
+
+export async function readFoldersFromSourceControlFile(file: string): Promise<ExportedFolders> {
+	return jsonParse<ExportedFolders>(await fsReadFile(file, { encoding: 'utf8' }), {
+		fallbackValue: { folders: [] },
+	});
+}
+
 export function sourceControlFoldersExistCheck(
 	folders: string[],
 	createIfNotExists = true,

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

@@ -8,10 +8,14 @@ import {
 	type WorkflowTagMapping,
 } from '@n8n/db';
 import { Service } from '@n8n/di';
+import { hasGlobalScope } from '@n8n/permissions';
 // eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import
 import type { FindOptionsWhere } from '@n8n/typeorm';
 
-import type { SourceControlContext } from './types/source-control-context';
+import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
+import type { AuthenticatedRequest } from '@/requests';
+
+import { SourceControlContext } from './types/source-control-context';
 
 @Service()
 export class SourceControlScopedService {
@@ -20,6 +24,19 @@ export class SourceControlScopedService {
 		private readonly workflowRepository: WorkflowRepository,
 	) {}
 
+	async ensureIsAllowedToPush(req: AuthenticatedRequest) {
+		if (hasGlobalScope(req.user, 'sourceControl:push')) {
+			return;
+		}
+
+		const ctx = new SourceControlContext(req.user);
+		const projectsWithAdminAccess = await this.getAdminProjectsFromContext(ctx);
+
+		if (projectsWithAdminAccess?.length === 0) {
+			throw new ForbiddenError('You are not allowed to push changes');
+		}
+	}
+
 	async getAdminProjectsFromContext(context: SourceControlContext): Promise<Project[] | undefined> {
 		if (context.hasAccessToAllProjects()) {
 			// In case the user is a global admin or owner, we don't need a filter
@@ -73,10 +90,10 @@ export class SourceControlScopedService {
 
 	getFoldersInAdminProjectsFromContextFilter(
 		context: SourceControlContext,
-	): FindOptionsWhere<Folder> | undefined {
+	): FindOptionsWhere<Folder> {
 		if (context.hasAccessToAllProjects()) {
 			// In case the user is a global admin or owner, we don't need a filter
-			return;
+			return {};
 		}
 
 		// We build a filter to only select folder, that belong to a team project
@@ -88,10 +105,10 @@ export class SourceControlScopedService {
 
 	getWorkflowsInAdminProjectsFromContextFilter(
 		context: SourceControlContext,
-	): FindOptionsWhere<WorkflowEntity> | undefined {
+	): FindOptionsWhere<WorkflowEntity> {
 		if (context.hasAccessToAllProjects()) {
 			// In case the user is a global admin or owner, we don't need a filter
-			return;
+			return {};
 		}
 
 		// We build a filter to only select workflows, that belong to a team project
@@ -106,10 +123,10 @@ export class SourceControlScopedService {
 
 	getCredentialsInAdminProjectsFromContextFilter(
 		context: SourceControlContext,
-	): FindOptionsWhere<CredentialsEntity> | undefined {
+	): FindOptionsWhere<CredentialsEntity> {
 		if (context.hasAccessToAllProjects()) {
 			// In case the user is a global admin or owner, we don't need a filter
-			return;
+			return {};
 		}
 
 		// We build a filter to only select workflows, that belong to a team project
@@ -124,10 +141,10 @@ export class SourceControlScopedService {
 
 	getWorkflowTagMappingInAdminProjectsFromContextFilter(
 		context: SourceControlContext,
-	): FindOptionsWhere<WorkflowTagMapping> | undefined {
+	): FindOptionsWhere<WorkflowTagMapping> {
 		if (context.hasAccessToAllProjects()) {
 			// In case the user is a global admin or owner, we don't need a filter
-			return;
+			return {};
 		}
 
 		// We build a filter to only select workflows, that belong to a team project

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

@@ -15,6 +15,7 @@ import {
 } from './middleware/source-control-enabled-middleware.ee';
 import { getRepoType } from './source-control-helper.ee';
 import { SourceControlPreferencesService } from './source-control-preferences.service.ee';
+import { SourceControlScopedService } from './source-control-scoped.service';
 import { SourceControlService } from './source-control.service.ee';
 import type { ImportResult } from './types/import-result';
 import { SourceControlRequest } from './types/requests';
@@ -26,6 +27,7 @@ export class SourceControlController {
 	constructor(
 		private readonly sourceControlService: SourceControlService,
 		private readonly sourceControlPreferencesService: SourceControlPreferencesService,
+		private readonly sourceControlScopedService: SourceControlScopedService,
 		private readonly eventService: EventService,
 	) {}
 
@@ -164,12 +166,13 @@ export class SourceControlController {
 	}
 
 	@Post('/push-workfolder', { middlewares: [sourceControlLicensedAndEnabledMiddleware] })
-	@GlobalScope('sourceControl:push')
 	async pushWorkfolder(
 		req: AuthenticatedRequest,
 		res: express.Response,
 		@Body payload: PushWorkFolderRequestDto,
 	): Promise<SourceControlledFile[]> {
+		await this.sourceControlScopedService.ensureIsAllowedToPush(req);
+
 		try {
 			await this.sourceControlService.setGitUserDetails(
 				`${req.user.firstName} ${req.user.lastName}`,

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

@@ -233,7 +233,9 @@ export class SourceControlService {
 			throw new BadRequestError('Cannot push onto read-only branch.');
 		}
 
-		const filesToPush = options.fileNames.map((file) => {
+		const context = new SourceControlContext(user);
+
+		let filesToPush = options.fileNames.map((file) => {
 			const normalizedPath = normalizeAndValidateSourceControlledFilePath(
 				this.gitFolder,
 				file.file,
@@ -245,23 +247,39 @@ export class SourceControlService {
 			};
 		});
 
-		// only determine file status if not provided by the frontend
-		let statusResult: SourceControlledFile[] = filesToPush;
-		if (statusResult.length === 0) {
-			statusResult = (await this.getStatus(user, {
-				direction: 'push',
-				verbose: false,
-				preferLocalVersion: true,
-			})) as SourceControlledFile[];
+		const allowedResources = (await this.getStatus(user, {
+			direction: 'push',
+			verbose: false,
+			preferLocalVersion: true,
+		})) as SourceControlledFile[];
+
+		// Fallback to all allowed resources if no fileNames are provided
+		if (!filesToPush.length) {
+			filesToPush = allowedResources;
 		}
 
+		// If fileNames are provided, we need to check if they are allowed
+		if (
+			filesToPush !== allowedResources &&
+			filesToPush.some(
+				(file) =>
+					!allowedResources.some((allowed) => {
+						return allowed.id === file.id && allowed.type === file.type;
+					}),
+			)
+		) {
+			throw new ForbiddenError('You are not allowed to push these changes');
+		}
+
+		let statusResult: SourceControlledFile[] = filesToPush;
+
 		if (!options.force) {
-			const possibleConflicts = statusResult?.filter((file) => file.conflict);
+			const possibleConflicts = filesToPush?.filter((file) => file.conflict);
 			if (possibleConflicts?.length > 0) {
 				return {
 					statusCode: 409,
 					pushResult: undefined,
-					statusResult,
+					statusResult: filesToPush,
 				};
 			}
 		}
@@ -307,13 +325,13 @@ export class SourceControlService {
 		const tagChanges = filesToPush.find((e) => e.type === 'tags');
 		if (tagChanges) {
 			filesToBePushed.add(tagChanges.file);
-			await this.sourceControlExportService.exportTagsToWorkFolder();
+			await this.sourceControlExportService.exportTagsToWorkFolder(context);
 		}
 
 		const folderChanges = filesToPush.find((e) => e.type === 'folders');
 		if (folderChanges) {
 			filesToBePushed.add(folderChanges.file);
-			await this.sourceControlExportService.exportFoldersToWorkFolder();
+			await this.sourceControlExportService.exportFoldersToWorkFolder(context);
 		}
 
 		const variablesChanges = filesToPush.find((e) => e.type === 'variables');
@@ -324,12 +342,8 @@ export class SourceControlService {
 
 		await this.gitService.stage(filesToBePushed, filesToBeDeleted);
 
-		for (let i = 0; i < statusResult.length; i++) {
-			// eslint-disable-next-line @typescript-eslint/no-loop-func
-			if (filesToPush.find((file) => file.file === statusResult[i].file)) {
-				statusResult[i].pushed = true;
-			}
-		}
+		// Set all results as pushed
+		statusResult.forEach((result) => (result.pushed = true));
 
 		await this.gitService.commit(options.commitMessage ?? 'Updated Workfolder');
 

packages/cli/src/environments.ee/source-control/types/exportable-folders.ts

@@ -6,3 +6,7 @@ export type ExportableFolder = {
 	createdAt: string;
 	updatedAt: string;
 };
+
+export type ExportedFolders = {
+	folders: ExportableFolder[];
+};

packages/cli/test/integration/shared/db/tags.ts

@@ -26,6 +26,12 @@ export async function createTag(attributes: Partial<TagEntity> = {}, workflow?:
 	return tag;
 }
 
+export async function updateTag(tag: TagEntity, attributes: Partial<TagEntity>) {
+	const tagRepository = Container.get(TagRepository);
+	const updatedTag = tagRepository.merge(tag, attributes);
+	return await tagRepository.save(updatedTag);
+}
+
 export async function assignTagToWorkflow(tag: TagEntity, workflow: WorkflowEntity) {
 	const mappingRepository = Container.get(WorkflowTagMappingRepository);