fix(core): Permission check for subworkflow properly checking for workflow settings (#7576)

The `sharing` related code is legacy that was not removed. Subworkflow
execution should check workflow settings alone, and this is now
reflected in the code.

Github issue / Community forum post (link here to close automatically):

https://community.n8n.io/t/bug-when-using-the-execute-workflow-node-when-workflow-is-shared/32207

---------

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

packages/cli/src/UserManagement/PermissionChecker.ts

@@ -1,17 +1,11 @@
 import type { INode, Workflow } from 'n8n-workflow';
-import {
+import { NodeOperationError, SubworkflowOperationError } from 'n8n-workflow';
-	NodeOperationError,
-	SubworkflowOperationError,
-	WorkflowOperationError,
-} from 'n8n-workflow';
 import type { FindOptionsWhere } from 'typeorm';
 import { In } from 'typeorm';
 import * as Db from '@/Db';
 import config from '@/config';
 import type { SharedCredentials } from '@db/entities/SharedCredentials';
 import { isSharingEnabled } from './UserManagementHelper';
-import { WorkflowsService } from '@/workflows/workflows.services';
-import { UserService } from '@/services/user.service';
 import { OwnershipService } from '@/services/ownership.service';
 import Container from 'typedi';
 import { RoleService } from '@/services/role.service';
@@ -135,14 +129,7 @@ export class PermissionChecker {
 		}
 
 		if (policy === 'workflowsFromSameOwner') {
-			const user = await Container.get(UserService).findOne({ where: { id: userId } });
+			if (subworkflowOwner?.id !== userId) {
-			if (!user) {
-				throw new WorkflowOperationError(
-					'Fatal error: user not found. Please contact the system administrator.',
-				);
-			}
-			const sharing = await WorkflowsService.getSharing(user, subworkflow.id, ['role', 'user']);
-			if (!sharing || sharing.role.name !== 'owner') {
 				throw errorToThrow;
 			}
 		}

packages/cli/test/unit/PermissionChecker.test.ts

@@ -10,10 +10,8 @@ import { User } from '@db/entities/User';
 import { SharedWorkflow } from '@db/entities/SharedWorkflow';
 import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials';
 import { NodeTypes } from '@/NodeTypes';
-import { UserService } from '@/services/user.service';
 import { PermissionChecker } from '@/UserManagement/PermissionChecker';
 import * as UserManagementHelper from '@/UserManagement/UserManagementHelper';
-import { WorkflowsService } from '@/workflows/workflows.services';
 import { OwnershipService } from '@/services/ownership.service';
 
 import { mockInstance } from '../integration/shared/utils/';
@@ -225,18 +223,12 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
 
 	const nonOwnerMockRole = new Role();
 	nonOwnerMockRole.name = 'editor';
-	const sharedWorkflowNotOwner = new SharedWorkflow();
+	const nonOwnerUser = new User();
-	sharedWorkflowNotOwner.role = nonOwnerMockRole;
+	nonOwnerUser.id = uuid();
-
-	const userService = mockInstance(UserService);
 
 	test('sets default policy from environment when subworkflow has none', async () => {
 		config.set('workflows.callerPolicyDefaultOption', 'none');
-		jest
+		jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(fakeUser);
-			.spyOn(ownershipService, 'getWorkflowOwnerCached')
-			.mockImplementation(async (workflowId) => {
-				return fakeUser;
-			});
 		jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
 
 		const subworkflow = new Workflow({
@@ -251,15 +243,9 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
 		).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`);
 	});
 
-	test('if sharing is disabled, ensures that workflows are owner by same user', async () => {
+	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
+		jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(nonOwnerUser);
-			.spyOn(ownershipService, 'getWorkflowOwnerCached')
-			.mockImplementation(async (workflowId) => fakeUser);
 		jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false);
-		jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
-		jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
-			return sharedWorkflowNotOwner;
-		});
 
 		const subworkflow = new Workflow({
 			nodes: [],
@@ -267,6 +253,9 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
 			active: false,
 			nodeTypes: mockNodeTypes,
 			id: '2',
+			settings: {
+				callerPolicy: 'any',
+			},
 		});
 		await expect(
 			PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId),
@@ -284,16 +273,12 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
 		}
 	});
 
-	test('list of ids must include the parent workflow id', async () => {
+	test('should throw if allowed list does not contain parent workflow id', async () => {
 		const invalidParentWorkflowId = uuid();
 		jest
 			.spyOn(ownershipService, 'getWorkflowOwnerCached')
 			.mockImplementation(async (workflowId) => fakeUser);
 		jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
-		jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
-		jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
-			return sharedWorkflowNotOwner;
-		});
 
 		const subworkflow = new Workflow({
 			nodes: [],
@@ -312,14 +297,8 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
 	});
 
 	test('sameOwner passes when both workflows are owned by the same user', async () => {
-		jest
+		jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockImplementation(async () => fakeUser);
-			.spyOn(ownershipService, 'getWorkflowOwnerCached')
-			.mockImplementation(async (workflowId) => fakeUser);
 		jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false);
-		jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
-		jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
-			return sharedWorkflowOwner;
-		});
 
 		const subworkflow = new Workflow({
 			nodes: [],
@@ -339,10 +318,6 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
 			.spyOn(ownershipService, 'getWorkflowOwnerCached')
 			.mockImplementation(async (workflowId) => fakeUser);
 		jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
-		jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
-		jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
-			return sharedWorkflowNotOwner;
-		});
 
 		const subworkflow = new Workflow({
 			nodes: [],
@@ -365,10 +340,6 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
 			.spyOn(ownershipService, 'getWorkflowOwnerCached')
 			.mockImplementation(async (workflowId) => fakeUser);
 		jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
-		jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
-		jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
-			return sharedWorkflowNotOwner;
-		});
 
 		const subworkflow = new Workflow({
 			nodes: [],