diff --git a/packages/cli/src/executions/__tests__/execution.service.test.ts b/packages/cli/src/executions/__tests__/execution.service.test.ts index 7122e6cb180..e4cedaa2b20 100644 --- a/packages/cli/src/executions/__tests__/execution.service.test.ts +++ b/packages/cli/src/executions/__tests__/execution.service.test.ts @@ -70,12 +70,13 @@ describe('ExecutionService', () => { /** * Arrange */ - executionRepository.findSingleExecution.mockResolvedValue(undefined); + executionRepository.findWithUnflattenedData.mockResolvedValue(undefined); + const req = mock({ params: { id: '1234' } }); /** * Act */ - const stop = executionService.stop('inexistent-123'); + const stop = executionService.stop(req.params.id, []); /** * Assert @@ -88,12 +89,13 @@ describe('ExecutionService', () => { * Arrange */ const execution = mock({ id: '123', status: 'success' }); - executionRepository.findSingleExecution.mockResolvedValue(execution); + executionRepository.findWithUnflattenedData.mockResolvedValue(execution); + const req = mock({ params: { id: execution.id } }); /** * Act */ - const stop = executionService.stop(execution.id); + const stop = executionService.stop(req.params.id, [execution.id]); /** * Assert @@ -107,16 +109,18 @@ describe('ExecutionService', () => { * Arrange */ const execution = mock({ id: '123', status: 'running' }); - executionRepository.findSingleExecution.mockResolvedValue(execution); + executionRepository.findWithUnflattenedData.mockResolvedValue(execution); concurrencyControl.has.mockReturnValue(false); activeExecutions.has.mockReturnValue(true); waitTracker.has.mockReturnValue(false); executionRepository.stopDuringRun.mockResolvedValue(mock()); + const req = mock({ params: { id: execution.id } }); + /** * Act */ - await executionService.stop(execution.id); + await executionService.stop(req.params.id, [execution.id]); /** * Assert @@ -132,16 +136,18 @@ describe('ExecutionService', () => { * Arrange */ const execution = mock({ id: '123', status: 'waiting' }); - executionRepository.findSingleExecution.mockResolvedValue(execution); + executionRepository.findWithUnflattenedData.mockResolvedValue(execution); concurrencyControl.has.mockReturnValue(false); activeExecutions.has.mockReturnValue(true); waitTracker.has.mockReturnValue(true); executionRepository.stopDuringRun.mockResolvedValue(mock()); + const req = mock({ params: { id: execution.id } }); + /** * Act */ - await executionService.stop(execution.id); + await executionService.stop(req.params.id, [execution.id]); /** * Assert @@ -157,16 +163,18 @@ describe('ExecutionService', () => { * Arrange */ const execution = mock({ id: '123', status: 'new', mode: 'trigger' }); - executionRepository.findSingleExecution.mockResolvedValue(execution); + executionRepository.findWithUnflattenedData.mockResolvedValue(execution); concurrencyControl.has.mockReturnValue(true); activeExecutions.has.mockReturnValue(false); waitTracker.has.mockReturnValue(false); executionRepository.stopBeforeRun.mockResolvedValue(mock()); + const req = mock({ params: { id: execution.id } }); + /** * Act */ - await executionService.stop(execution.id); + await executionService.stop(req.params.id, [execution.id]); /** * Assert @@ -193,11 +201,13 @@ describe('ExecutionService', () => { mode: 'manual', status: 'running', }); - executionRepository.findSingleExecution.mockResolvedValue(execution); + executionRepository.findWithUnflattenedData.mockResolvedValue(execution); concurrencyControl.has.mockReturnValue(false); activeExecutions.has.mockReturnValue(true); waitTracker.has.mockReturnValue(false); - const job = mock({ data: { executionId: '123' } }); + + const req = mock({ params: { id: execution.id } }); + const job = mock({ data: { executionId: execution.id } }); scalingService.findJobsByStatus.mockResolvedValue([job]); executionRepository.stopDuringRun.mockResolvedValue(mock()); // @ts-expect-error Private method @@ -206,7 +216,7 @@ describe('ExecutionService', () => { /** * Act */ - await executionService.stop(execution.id); + await executionService.stop(req.params.id, [execution.id]); /** * Assert @@ -228,16 +238,18 @@ describe('ExecutionService', () => { */ config.set('executions.mode', 'queue'); const execution = mock({ id: '123', status: 'running' }); - executionRepository.findSingleExecution.mockResolvedValue(execution); + executionRepository.findWithUnflattenedData.mockResolvedValue(execution); waitTracker.has.mockReturnValue(false); - const job = mock({ data: { executionId: '123' } }); + + const req = mock({ params: { id: execution.id } }); + const job = mock({ data: { executionId: execution.id } }); scalingService.findJobsByStatus.mockResolvedValue([job]); executionRepository.stopDuringRun.mockResolvedValue(mock()); /** * Act */ - await executionService.stop(execution.id); + await executionService.stop(req.params.id, [execution.id]); /** * Assert @@ -255,16 +267,18 @@ describe('ExecutionService', () => { */ config.set('executions.mode', 'queue'); const execution = mock({ id: '123', status: 'waiting' }); - executionRepository.findSingleExecution.mockResolvedValue(execution); + executionRepository.findWithUnflattenedData.mockResolvedValue(execution); waitTracker.has.mockReturnValue(true); - const job = mock({ data: { executionId: '123' } }); + + const req = mock({ params: { id: execution.id } }); + const job = mock({ data: { executionId: execution.id } }); scalingService.findJobsByStatus.mockResolvedValue([job]); executionRepository.stopDuringRun.mockResolvedValue(mock()); /** * Act */ - await executionService.stop(execution.id); + await executionService.stop(req.params.id, [execution.id]); /** * Assert diff --git a/packages/cli/src/executions/__tests__/executions.controller.test.ts b/packages/cli/src/executions/__tests__/executions.controller.test.ts index d5f8f849a34..bd8fd5f4206 100644 --- a/packages/cli/src/executions/__tests__/executions.controller.test.ts +++ b/packages/cli/src/executions/__tests__/executions.controller.test.ts @@ -138,7 +138,7 @@ describe('ExecutionsController', () => { const executionId = '999'; const req = mock({ params: { id: executionId } }); - it('should 404 when execution is inaccessible for user', async () => { + it('should throw expected NotFoundError when all workflows are inaccessible for user', async () => { workflowSharingService.getSharedWorkflowIds.mockResolvedValue([]); const promise = executionsController.stop(req); @@ -147,12 +147,12 @@ describe('ExecutionsController', () => { expect(executionService.stop).not.toHaveBeenCalled(); }); - it('should call ask for an execution to be stopped', async () => { - workflowSharingService.getSharedWorkflowIds.mockResolvedValue(['123']); + it('should call execution service with expected data when user has accessible workflows', async () => { + const mockAccessibleWorkflowIds = ['1234', '999']; + workflowSharingService.getSharedWorkflowIds.mockResolvedValue(mockAccessibleWorkflowIds); await executionsController.stop(req); - - expect(executionService.stop).toHaveBeenCalledWith(executionId); + expect(executionService.stop).toHaveBeenCalledWith(req.params.id, mockAccessibleWorkflowIds); }); }); }); diff --git a/packages/cli/src/executions/execution.service.ts b/packages/cli/src/executions/execution.service.ts index 889882aa576..b07fe70e89a 100644 --- a/packages/cli/src/executions/execution.service.ts +++ b/packages/cli/src/executions/execution.service.ts @@ -420,13 +420,19 @@ export class ExecutionService { ); } - async stop(executionId: string): Promise { - const execution = await this.executionRepository.findSingleExecution(executionId, { - includeData: true, - unflattenData: true, - }); + async stop(executionId: string, sharedWorkflowIds: string[]): Promise { + const execution = await this.executionRepository.findWithUnflattenedData( + executionId, + sharedWorkflowIds, + ); - if (!execution) throw new MissingExecutionStopError(executionId); + if (!execution) { + this.logger.info(`Unable to stop execution "${executionId}" as it was not found`, { + executionId, + }); + + throw new MissingExecutionStopError(executionId); + } this.assertStoppable(execution); diff --git a/packages/cli/src/executions/executions.controller.ts b/packages/cli/src/executions/executions.controller.ts index ec68a05df62..7bc336cd2ee 100644 --- a/packages/cli/src/executions/executions.controller.ts +++ b/packages/cli/src/executions/executions.controller.ts @@ -96,7 +96,9 @@ export class ExecutionsController { if (workflowIds.length === 0) throw new NotFoundError('Execution not found'); - return await this.executionService.stop(req.params.id); + const executionId = req.params.id; + + return await this.executionService.stop(executionId, workflowIds); } @Post('/:id/retry') diff --git a/packages/cli/test/integration/executions.controller.test.ts b/packages/cli/test/integration/executions.controller.test.ts index 0652006b8ba..2a75757a6f8 100644 --- a/packages/cli/test/integration/executions.controller.test.ts +++ b/packages/cli/test/integration/executions.controller.test.ts @@ -3,7 +3,11 @@ import type { User } from '@n8n/db'; import { ConcurrencyControlService } from '@/concurrency/concurrency-control.service'; import { WaitTracker } from '@/wait-tracker'; -import { createSuccessfulExecution, getAllExecutions } from './shared/db/executions'; +import { + createSuccessfulExecution, + createWaitingExecution, + getAllExecutions, +} from './shared/db/executions'; import { createTeamProject, linkUserToProject } from './shared/db/projects'; import { createMember, createOwner } from './shared/db/users'; import { createWorkflow, shareWorkflowWithUsers } from './shared/db/workflows'; @@ -27,6 +31,11 @@ const saveExecution = async ({ belongingTo }: { belongingTo: User }) => { return await createSuccessfulExecution(workflow); }; +const saveWaitingExecution = async ({ belongingTo }: { belongingTo: User }) => { + const workflow = await createWorkflow({}, belongingTo); + return await createWaitingExecution(workflow); +}; + beforeEach(async () => { await testDb.truncate(['ExecutionEntity', 'WorkflowEntity', 'SharedWorkflow']); testServer.license.reset(); @@ -117,3 +126,21 @@ describe('POST /executions/delete', () => { expect(executions).toHaveLength(0); }); }); + +describe('POST /executions/stop', () => { + test('should not stop an execution we do not have access to', async () => { + await saveExecution({ belongingTo: owner }); + const incorrectExecutionId = '1234'; + + await testServer + .authAgentFor(owner) + .post(`/executions/${incorrectExecutionId}/stop`) + .expect(500); + }); + + test('should stop an execution we have access to', async () => { + const execution = await saveWaitingExecution({ belongingTo: owner }); + + await testServer.authAgentFor(owner).post(`/executions/${execution.id}/stop`).expect(200); + }); +});