fix(core): Keep workflow active when external hook rejects publish (#30707)

packages/cli/src/workflows/__tests__/workflow.service.test.ts

@@ -1,17 +1,23 @@
 import type { LicenseState } from '@n8n/backend-common';
-import type { Project, User, WorkflowEntity } from '@n8n/db';
+import type { Project, User, WorkflowRepository, WorkflowPublishHistoryRepository } from '@n8n/db';
+import { WorkflowEntity, WorkflowHistory } from '@n8n/db';
 import type { Scope } from '@n8n/permissions';
 import type { MockProxy } from 'jest-mock-extended';
 import { mock } from 'jest-mock-extended';
+import type { IConnections, INode } from 'n8n-workflow';
 
+import type { ActiveWorkflowManager } from '@/active-workflow-manager';
 import { BadRequestError } from '@/errors/response-errors/bad-request.error';
 import { UnprocessableRequestError } from '@/errors/response-errors/unprocessable.error';
+import { WorkflowActivationBadRequestError } from '@/errors/response-errors/workflow-activation-bad-request.error';
+import type { ExternalHooks } from '@/external-hooks';
 import type { RedactionEnforcementService } from '@/modules/redaction/redaction-enforcement.service';
 import { userHasScopes } from '@/permissions.ee/check-access';
 import type { OwnershipService } from '@/services/ownership.service';
 import type { RoleService } from '@/services/role.service';
 import type { WebhookService } from '@/webhooks/webhook.service';
 import type { WorkflowFinderService } from '@/workflows/workflow-finder.service';
+import type { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service';
 import { WorkflowService } from '@/workflows/workflow.service';
 import * as WorkflowHelpers from '@/workflow-helpers';
 
@@ -495,4 +501,166 @@ describe('WorkflowService', () => {
 			);
 		});
 	});
+
+	describe('workflow.activate hook', () => {
+		let workflowService: WorkflowService;
+		let workflowFinderServiceMock: MockProxy<WorkflowFinderService>;
+		let workflowHistoryServiceMock: MockProxy<WorkflowHistoryService>;
+		let workflowRepositoryMock: MockProxy<WorkflowRepository>;
+		let workflowPublishHistoryRepositoryMock: MockProxy<WorkflowPublishHistoryRepository>;
+		let activeWorkflowManagerMock: MockProxy<ActiveWorkflowManager>;
+		let externalHooksMock: MockProxy<ExternalHooks>;
+
+		const WORKFLOW_ID = 'workflow-1';
+		const PREVIOUS_VERSION_ID = 'v1';
+		const TARGET_VERSION_ID = 'v2';
+
+		function makeWorkflowEntity(overrides: Partial<WorkflowEntity> = {}): WorkflowEntity {
+			const workflow = new WorkflowEntity();
+			workflow.id = WORKFLOW_ID;
+			workflow.name = 'My workflow';
+			workflow.isArchived = false;
+			workflow.versionId = TARGET_VERSION_ID;
+			workflow.activeVersionId = PREVIOUS_VERSION_ID;
+			workflow.active = true;
+			workflow.nodes = [{ name: 'Draft node' } as INode];
+			workflow.connections = { Draft: {} } as IConnections;
+			workflow.settings = {};
+			workflow.updatedAt = new Date();
+			Object.assign(workflow, overrides);
+			return workflow;
+		}
+
+		function makeVersionToActivate(): WorkflowHistory {
+			const version = new WorkflowHistory();
+			version.versionId = TARGET_VERSION_ID;
+			version.nodes = [{ name: 'Activated node' } as INode];
+			version.connections = { Activated: {} } as IConnections;
+			return version;
+		}
+
+		beforeEach(() => {
+			workflowFinderServiceMock = mock<WorkflowFinderService>();
+			workflowHistoryServiceMock = mock<WorkflowHistoryService>();
+			workflowRepositoryMock = mock();
+			workflowPublishHistoryRepositoryMock = mock();
+			activeWorkflowManagerMock = mock();
+			externalHooksMock = mock<ExternalHooks>();
+
+			workflowRepositoryMock.create.mockImplementation(
+				(data) => Object.assign(new WorkflowEntity(), data) as WorkflowEntity,
+			);
+
+			workflowService = new WorkflowService(
+				mock(), // logger
+				mock(), // sharedWorkflowRepository
+				workflowRepositoryMock, // workflowRepository
+				mock(), // workflowTagMappingRepository
+				mock(), // binaryDataService
+				mock(), // ownershipService
+				mock(), // tagService
+				workflowHistoryServiceMock, // workflowHistoryService
+				externalHooksMock, // externalHooks
+				activeWorkflowManagerMock, // activeWorkflowManager
+				mock(), // roleService
+				mock(), // projectService
+				mock(), // executionRepository
+				mock(), // eventService
+				mock(), // globalConfig
+				mock(), // folderRepository
+				workflowFinderServiceMock, // workflowFinderService
+				mock(), // workflowPublishedVersionRepository
+				workflowPublishHistoryRepositoryMock, // workflowPublishHistoryRepository
+				mock(), // workflowValidationService
+				mock(), // nodeTypes
+				mock(), // webhookService
+				mock(), // licenseState
+				mock(), // projectRepository
+				mock(), // redactionEnforcementService
+			);
+
+			// Bypass validation internals
+			jest
+				.spyOn(workflowService as never, '_detectWebhookConflicts')
+				.mockResolvedValue(undefined as never);
+			jest.spyOn(workflowService as never, '_validateNodes').mockReturnValue(undefined as never);
+			jest
+				.spyOn(workflowService as never, '_validateDynamicCredentials')
+				.mockResolvedValue(undefined as never);
+			jest
+				.spyOn(workflowService as never, '_validateSubWorkflowReferences')
+				.mockResolvedValue(undefined as never);
+		});
+
+		test('republish blocked by hook leaves previous active version untouched', async () => {
+			const workflow = makeWorkflowEntity({ activeVersionId: PREVIOUS_VERSION_ID });
+			const versionToActivate = makeVersionToActivate();
+			workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
+			workflowHistoryServiceMock.getVersion.mockResolvedValue(versionToActivate);
+
+			externalHooksMock.run.mockRejectedValue(new Error('Publish gate rejected'));
+
+			const user = mock<User>();
+
+			await expect(
+				workflowService.activateWorkflow(user, WORKFLOW_ID, { versionId: TARGET_VERSION_ID }),
+			).rejects.toBeInstanceOf(WorkflowActivationBadRequestError);
+
+			expect(workflow.active).toBe(true);
+			expect(workflow.activeVersionId).toBe(PREVIOUS_VERSION_ID);
+			expect(workflowRepositoryMock.update).not.toHaveBeenCalled();
+			expect(activeWorkflowManagerMock.remove).not.toHaveBeenCalled();
+			expect(workflowPublishHistoryRepositoryMock.addRecord).not.toHaveBeenCalled();
+		});
+
+		test('first-time activate blocked by hook leaves the workflow row untouched', async () => {
+			const workflow = makeWorkflowEntity({ active: false, activeVersionId: null });
+			workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
+			workflowHistoryServiceMock.getVersion.mockResolvedValue(makeVersionToActivate());
+
+			externalHooksMock.run.mockRejectedValue(new Error('Publish gate rejected'));
+
+			const user = mock<User>();
+
+			await expect(
+				workflowService.activateWorkflow(user, WORKFLOW_ID, { versionId: TARGET_VERSION_ID }),
+			).rejects.toBeInstanceOf(WorkflowActivationBadRequestError);
+
+			expect(workflowRepositoryMock.update).not.toHaveBeenCalled();
+			expect(activeWorkflowManagerMock.add).not.toHaveBeenCalled();
+		});
+
+		test('hook receives a candidate workflow targeting the activation version', async () => {
+			const workflow = makeWorkflowEntity({ activeVersionId: PREVIOUS_VERSION_ID });
+			const versionToActivate = makeVersionToActivate();
+			workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
+			workflowHistoryServiceMock.getVersion.mockResolvedValue(versionToActivate);
+			workflowRepositoryMock.findOne.mockResolvedValue(workflow);
+
+			externalHooksMock.run.mockResolvedValue(undefined);
+
+			jest
+				.spyOn(workflowService as never, '_addToActiveWorkflowManager')
+				.mockResolvedValue(undefined as never);
+
+			const user = mock<User>();
+
+			await workflowService.activateWorkflow(user, WORKFLOW_ID, {
+				versionId: TARGET_VERSION_ID,
+			});
+
+			expect(externalHooksMock.run).toHaveBeenCalledTimes(1);
+			const [hookName, hookArgs] = externalHooksMock.run.mock.calls[0] as [
+				string,
+				[WorkflowEntity],
+			];
+			expect(hookName).toBe('workflow.activate');
+			const [candidate] = hookArgs;
+			expect(candidate.active).toBe(true);
+			expect(candidate.activeVersionId).toBe(TARGET_VERSION_ID);
+			expect(candidate.activeVersion).toBe(versionToActivate);
+			expect(candidate.nodes).toBe(workflow.nodes);
+			expect(candidate.connections).toBe(workflow.connections);
+		});
+	});
 });

packages/cli/src/workflows/workflow.service.ts

@@ -3,10 +3,10 @@ import { LicenseState, Logger } from '@n8n/backend-common';
 import { GlobalConfig } from '@n8n/config';
 import type {
 	User,
-	WorkflowEntity,
 	ListQueryDb,
 	WorkflowFolderUnionFull,
 	WorkflowHistory,
+	WorkflowEntity,
 } from '@n8n/db';
 import {
 	SharedWorkflow,
@@ -32,7 +32,13 @@ import pick from 'lodash/pick';
 import { FileLocation, BinaryDataService } from 'n8n-core';
 
 import type { INode, INodes, IWorkflowSettings, JsonValue, IConnections } from 'n8n-workflow';
-import { PROJECT_ROOT, Workflow, assert, calculateWorkflowChecksum } from 'n8n-workflow';
+import {
+	PROJECT_ROOT,
+	Workflow,
+	assert,
+	calculateWorkflowChecksum,
+	ensureError,
+} from 'n8n-workflow';
 import { v4 as uuid } from 'uuid';
 
 import { getErrorDescription, getErrorNodeId } from './utils';
@@ -564,7 +570,6 @@ export class WorkflowService {
 	): Promise<void> {
 		let didPublish = false;
 		try {
-			await this.externalHooks.run('workflow.activate', [workflow]);
 			await this.activeWorkflowManager.add(workflowId, mode);
 			didPublish = true;
 		} catch (error) {
@@ -670,6 +675,7 @@ export class WorkflowService {
 	 * @param publicApi - Whether this is called from the public API (affects event emission)
 	 * @returns The activated workflow
 	 */
+	// eslint-disable-next-line complexity
 	async activateWorkflow(
 		user: User,
 		workflowId: string,
@@ -732,6 +738,24 @@ export class WorkflowService {
 		await this._validateDynamicCredentials(workflowId, versionToActivate.nodes, workflow.settings);
 		await this._validateSubWorkflowReferences(workflowId, versionToActivate.nodes);
 
+		// Run hook before destructive state changes so a rejection leaves
+		// the previous active version running instead of deactivating it.
+		const candidateWorkflow = this.workflowRepository.create({
+			...workflow,
+			active: true,
+			activeVersionId: versionIdToActivate,
+			activeVersion: versionToActivate,
+		});
+
+		try {
+			await this.externalHooks.run('workflow.activate', [candidateWorkflow]);
+		} catch (error) {
+			throw new WorkflowActivationBadRequestError(ensureError(error).message, {
+				nodeId: getErrorNodeId(error),
+				description: getErrorDescription(error),
+			});
+		}
+
 		if (previousActiveVersionId) {
 			await this.activeWorkflowManager.remove(workflowId);