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 <krynble@gmail.com>

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;
 		}
 	}
 

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

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();

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();
+		});
+	});
 });

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(

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;

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();
 	}