From 437c95e84e144cc77f2866a74d6b670c415895cd Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Wed, 1 Nov 2023 18:31:34 +0100 Subject: [PATCH] fix(core): Permission check for subworkflow properly checking for workflow settings (#7576) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/UserManagement/PermissionChecker.ts | 17 +------ .../cli/test/unit/PermissionChecker.test.ts | 49 ++++--------------- 2 files changed, 12 insertions(+), 54 deletions(-) diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 4f4eb2bed31f5..36dff30e2f8d3 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -1,17 +1,11 @@ import type { INode, Workflow } from 'n8n-workflow'; -import { - NodeOperationError, - SubworkflowOperationError, - WorkflowOperationError, -} from 'n8n-workflow'; +import { NodeOperationError, SubworkflowOperationError } 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 (!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') { + if (subworkflowOwner?.id !== userId) { throw errorToThrow; } } diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index 460f701941d10..e0c8e33b0b8fb 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/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(); - sharedWorkflowNotOwner.role = nonOwnerMockRole; - - const userService = mockInstance(UserService); + const nonOwnerUser = new User(); + nonOwnerUser.id = uuid(); test('sets default policy from environment when subworkflow has none', async () => { config.set('workflows.callerPolicyDefaultOption', 'none'); - jest - .spyOn(ownershipService, 'getWorkflowOwnerCached') - .mockImplementation(async (workflowId) => { - return fakeUser; - }); + jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(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 () => { - jest - .spyOn(ownershipService, 'getWorkflowOwnerCached') - .mockImplementation(async (workflowId) => fakeUser); + 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); 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 - .spyOn(ownershipService, 'getWorkflowOwnerCached') - .mockImplementation(async (workflowId) => fakeUser); + jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockImplementation(async () => 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: [],