From 92bab72cffb1083b495d211d0a31920e83e66769 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 6 Dec 2023 13:27:11 +0100 Subject: [PATCH] fix(core): Fix user comparison in same-user subworkflow caller policy (#7913) https://linear.app/n8n/issue/PAY-992 https://community.n8n.io/t/executing-workflow-using-owner-role-created-by-another-user-fails/33443 --------- Co-authored-by: Omar Ajoue --- .../src/UserManagement/PermissionChecker.ts | 27 +++-- .../cli/src/WorkflowExecuteAdditionalData.ts | 11 +- packages/cli/src/WorkflowHelpers.ts | 7 +- .../cli/test/unit/PermissionChecker.test.ts | 111 ++++++++++++++++-- packages/core/src/NodeExecuteFunctions.ts | 1 + packages/workflow/src/Interfaces.ts | 1 + .../src/errors/workflow-operation.error.ts | 3 +- 7 files changed, 133 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 2d18939b2e..514664814e 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -1,5 +1,5 @@ import type { INode, Workflow } from 'n8n-workflow'; -import { NodeOperationError, SubworkflowOperationError } from 'n8n-workflow'; +import { NodeOperationError, WorkflowOperationError } from 'n8n-workflow'; import type { FindOptionsWhere } from 'typeorm'; import { In } from 'typeorm'; import config from '@/config'; @@ -78,8 +78,8 @@ export class PermissionChecker { static async checkSubworkflowExecutePolicy( subworkflow: Workflow, - userId: string, - parentWorkflowId?: string, + parentWorkflowId: string, + node?: INode, ) { /** * Important considerations: both the current workflow and the parent can have empty IDs. @@ -101,15 +101,22 @@ export class PermissionChecker { policy = 'workflowsFromSameOwner'; } + const parentWorkflowOwner = + await Container.get(OwnershipService).getWorkflowOwnerCached(parentWorkflowId); + const subworkflowOwner = await Container.get(OwnershipService).getWorkflowOwnerCached( subworkflow.id, ); - const errorToThrow = new SubworkflowOperationError( - `Target workflow ID ${subworkflow.id ?? ''} may not be called`, - subworkflowOwner.id === userId + const description = + subworkflowOwner.id === parentWorkflowOwner.id ? 'Change the settings of the sub-workflow so it can be called by this one.' - : `${subworkflowOwner.firstName} (${subworkflowOwner.email}) can make this change. You may need to tell them the ID of this workflow, which is ${subworkflow.id}`, + : `${subworkflowOwner.firstName} (${subworkflowOwner.email}) can make this change. You may need to tell them the ID of the sub-workflow, which is ${subworkflow.id}`; + + const errorToThrow = new WorkflowOperationError( + `Target workflow ID ${subworkflow.id} may not be called`, + node, + description, ); if (policy === 'none') { @@ -130,10 +137,8 @@ export class PermissionChecker { } } - if (policy === 'workflowsFromSameOwner') { - if (subworkflowOwner?.id !== userId) { - throw errorToThrow; - } + if (policy === 'workflowsFromSameOwner' && subworkflowOwner?.id !== parentWorkflowOwner.id) { + throw errorToThrow; } } diff --git a/packages/cli/src/WorkflowExecuteAdditionalData.ts b/packages/cli/src/WorkflowExecuteAdditionalData.ts index 1eb95bfcf9..9a52e72052 100644 --- a/packages/cli/src/WorkflowExecuteAdditionalData.ts +++ b/packages/cli/src/WorkflowExecuteAdditionalData.ts @@ -93,6 +93,12 @@ export function objectToError(errorObject: unknown, workflow: Workflow): Error { error = new Error(errorObject.message as string); } + if ('description' in errorObject) { + // @ts-expect-error Error descriptions are surfaced by the UI but + // not all backend errors account for this property yet. + error.description = errorObject.description as string; + } + if ('stack' in errorObject) { // If there's a 'stack' property, set it on the new Error instance. error.stack = errorObject.stack as string; @@ -724,6 +730,7 @@ async function executeWorkflow( workflowInfo: IExecuteWorkflowInfo, additionalData: IWorkflowExecuteAdditionalData, options: { + node?: INode; parentWorkflowId?: string; inputData?: INodeExecutionData[]; parentExecutionId?: string; @@ -777,8 +784,8 @@ async function executeWorkflow( await PermissionChecker.check(workflow, additionalData.userId); await PermissionChecker.checkSubworkflowExecutePolicy( workflow, - additionalData.userId, - options.parentWorkflowId, + options.parentWorkflowId!, + options.node, ); // Create new additionalData to have different workflow loaded and to call diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index 135de736e8..477d0565dd 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -173,10 +173,13 @@ export async function executeErrorWorkflow( }); try { + const failedNode = workflowErrorData.execution?.lastNodeExecuted + ? workflowInstance.getNode(workflowErrorData.execution?.lastNodeExecuted) + : undefined; await PermissionChecker.checkSubworkflowExecutePolicy( workflowInstance, - runningUser.id, - workflowErrorData.workflow.id, + workflowErrorData.workflow.id!, + failedNode ?? undefined, ); } catch (error) { const initialNode = workflowInstance.getStartNode(); diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index df596a3058..4b1420cfc8 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -1,6 +1,6 @@ import { v4 as uuid } from 'uuid'; import { Container } from 'typedi'; -import type { INodeTypes } from 'n8n-workflow'; +import type { INodeTypes, WorkflowSettings } from 'n8n-workflow'; import { SubworkflowOperationError, Workflow } from 'n8n-workflow'; import config from '@/config'; @@ -16,6 +16,7 @@ import { OwnershipService } from '@/services/ownership.service'; import { mockInstance } from '../shared/mocking'; import { randomCredentialPayload as randomCred, + randomName, randomPositiveDigit, } from '../integration/shared/random'; import * as testDb from '../integration/shared/testDb'; @@ -26,6 +27,24 @@ import { getCredentialOwnerRole, getWorkflowOwnerRole } from '../integration/sha import { createOwner, createUser } from '../integration/shared/db/users'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; +import { UserRepository } from '@/databases/repositories/user.repository'; +import { LicenseMocker } from '../integration/shared/license'; +import { License } from '@/License'; +import { generateNanoId } from '@/databases/utils/generators'; +import type { WorkflowEntity } from '@/databases/entities/WorkflowEntity'; + +function createSubworkflow({ policy }: { policy: WorkflowSettings.CallerPolicy }) { + return new Workflow({ + id: uuid(), + nodes: [], + connections: {}, + active: false, + nodeTypes: mockNodeTypes, + settings: { + callerPolicy: policy, + }, + }); +} let mockNodeTypes: INodeTypes; let credentialOwnerRole: Role; @@ -230,7 +249,29 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { const nonOwnerUser = new User(); nonOwnerUser.id = uuid(); + let parentWorkflow: WorkflowEntity; + + beforeAll(() => { + parentWorkflow = Container.get(WorkflowRepository).create({ + id: generateNanoId(), + name: randomName(), + active: false, + connections: {}, + nodes: [ + { + name: '', + typeVersion: 1, + type: 'n8n-nodes-base.executeWorkflow', + position: [0, 0], + parameters: {}, + }, + ], + }); + }); + test('sets default policy from environment when subworkflow has none', async () => { + await Container.get(WorkflowRepository).save(parentWorkflow); + config.set('workflows.callerPolicyDefaultOption', 'none'); jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); @@ -243,12 +284,15 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { id: '2', }); await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId), + PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); }); test('if sharing is disabled, ensures that workflows are owned by same user and reject running workflows belonging to another user even if setting allows execution', async () => { - jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(nonOwnerUser); + await Container.get(WorkflowRepository).save(parentWorkflow); + + jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValueOnce(fakeUser); + jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValueOnce(nonOwnerUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); const subworkflow = new Workflow({ @@ -262,12 +306,12 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { }, }); await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId), + PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); // Check description try { - await PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, '', 'abcde'); + await PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, 'abcde'); } catch (error) { if (error instanceof SubworkflowOperationError) { expect(error.description).toBe( @@ -278,7 +322,8 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { }); test('should throw if allowed list does not contain parent workflow id', async () => { - const invalidParentWorkflowId = uuid(); + await Container.get(WorkflowRepository).save(parentWorkflow); + jest .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); @@ -296,11 +341,13 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { }, }); await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId, invalidParentWorkflowId), + PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); }); test('sameOwner passes when both workflows are owned by the same user', async () => { + await Container.get(WorkflowRepository).save(parentWorkflow); + jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockImplementation(async () => fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); @@ -312,12 +359,13 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { id: '2', }); await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId, userId), + PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), ).resolves.not.toThrow(); }); test('workflowsFromAList works when the list contains the parent id', async () => { - const workflowId = uuid(); + await Container.get(WorkflowRepository).save(parentWorkflow); + jest .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); @@ -331,15 +379,17 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { id: '2', settings: { callerPolicy: 'workflowsFromAList', - callerIds: `123,456,bcdef, ${workflowId}`, + callerIds: `123,456,bcdef, ${parentWorkflow.id}`, }, }); await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId, workflowId), + PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), ).resolves.not.toThrow(); }); test('should not throw when workflow policy is set to any', async () => { + await Container.get(WorkflowRepository).save(parentWorkflow); + jest .spyOn(ownershipService, 'getWorkflowOwnerCached') .mockImplementation(async (workflowId) => fakeUser); @@ -356,7 +406,44 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { }, }); await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId), + PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), ).resolves.not.toThrow(); }); + + describe('with workflows-from-same-owner caller policy', () => { + beforeAll(() => { + const license = new LicenseMocker(); + license.mock(Container.get(License)); + license.enable('feat:sharing'); + }); + + test('should deny if the two workflows are owned by different users', async () => { + const parentWorkflowOwner = Container.get(UserRepository).create({ id: uuid() }); + const subworkflowOwner = Container.get(UserRepository).create({ id: uuid() }); + + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(parentWorkflowOwner); + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(subworkflowOwner); + + const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' }); + + const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); + + await expect(check).rejects.toThrow(); + }); + + test('should allow if both workflows are owned by the same user', async () => { + await Container.get(WorkflowRepository).save(parentWorkflow); + + const bothWorkflowsOwner = Container.get(UserRepository).create({ id: uuid() }); + + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(bothWorkflowsOwner); // parent workflow + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(bothWorkflowsOwner); // subworkflow + + const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' }); + + const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + + await expect(check).resolves.not.toThrow(); + }); + }); }); diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index 70a80cdf49..a251a5c38e 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -3156,6 +3156,7 @@ export function getExecuteFunctions( parentWorkflowId: workflow.id?.toString(), inputData, parentWorkflowSettings: workflow.settings, + node, }) .then(async (result) => Container.get(BinaryDataService).duplicateBinaryData( diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index 12c9acc294..afa17b584f 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -1888,6 +1888,7 @@ export interface IWorkflowExecuteAdditionalData { workflowInfo: IExecuteWorkflowInfo, additionalData: IWorkflowExecuteAdditionalData, options: { + node?: INode; parentWorkflowId?: string; inputData?: INodeExecutionData[]; parentExecutionId?: string; diff --git a/packages/workflow/src/errors/workflow-operation.error.ts b/packages/workflow/src/errors/workflow-operation.error.ts index 5bac19ac07..214364dc2f 100644 --- a/packages/workflow/src/errors/workflow-operation.error.ts +++ b/packages/workflow/src/errors/workflow-operation.error.ts @@ -13,10 +13,11 @@ export class WorkflowOperationError extends ExecutionBaseError { description: string | undefined; - constructor(message: string, node?: INode) { + constructor(message: string, node?: INode, description?: string) { super(message, { cause: undefined }); this.severity = 'warning'; this.name = this.constructor.name; + if (description) this.description = description; this.node = node; this.timestamp = Date.now(); }