diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index 63fe24a3ba7..76a2eaa1c2f 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/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; + let workflowHistoryServiceMock: MockProxy; + let workflowRepositoryMock: MockProxy; + let workflowPublishHistoryRepositoryMock: MockProxy; + let activeWorkflowManagerMock: MockProxy; + let externalHooksMock: MockProxy; + + const WORKFLOW_ID = 'workflow-1'; + const PREVIOUS_VERSION_ID = 'v1'; + const TARGET_VERSION_ID = 'v2'; + + function makeWorkflowEntity(overrides: Partial = {}): 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(); + workflowHistoryServiceMock = mock(); + workflowRepositoryMock = mock(); + workflowPublishHistoryRepositoryMock = mock(); + activeWorkflowManagerMock = mock(); + externalHooksMock = mock(); + + 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(); + + 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(); + + 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(); + + 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); + }); + }); }); diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index c81645f3bff..61c47e042de 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/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 { 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);