From 8127df69fbf47836a204f936a8936a873bc55378 Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Wed, 1 Nov 2023 17:08:04 +0100 Subject: [PATCH 1/3] fix(core): Permission check for subworkflow should now correctly check for workflow settings only --- .../src/UserManagement/PermissionChecker.ts | 17 +------ .../cli/test/unit/PermissionChecker.test.ts | 49 +++++-------------- 2 files changed, 14 insertions(+), 52 deletions(-) diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 4f4eb2bed31f5..73eaf6ca714a2 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 || 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..f88b655ec84b8 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,14 @@ 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').mockImplementation(async () => { + return fakeUser; + }); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); const subworkflow = new Workflow({ @@ -251,15 +245,11 @@ 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 .spyOn(ownershipService, 'getWorkflowOwnerCached') - .mockImplementation(async (workflowId) => fakeUser); + .mockImplementation(async () => 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 +257,9 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { active: false, nodeTypes: mockNodeTypes, id: '2', + settings: { + callerPolicy: 'any', + }, }); await expect( PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId), @@ -284,16 +277,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 +301,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 +322,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 +344,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: [], From 7c7d9d80d701c71d001b9ecc4de0bb87ac1a3bc8 Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Wed, 1 Nov 2023 17:40:45 +0100 Subject: [PATCH 2/3] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Iván Ovejero --- packages/cli/src/UserManagement/PermissionChecker.ts | 2 +- packages/cli/test/unit/PermissionChecker.test.ts | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 73eaf6ca714a2..36dff30e2f8d3 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -129,7 +129,7 @@ export class PermissionChecker { } if (policy === 'workflowsFromSameOwner') { - if (!subworkflowOwner || subworkflowOwner.id !== userId) { + 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 f88b655ec84b8..0456fd417488c 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -228,9 +228,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { test('sets default policy from environment when subworkflow has none', async () => { config.set('workflows.callerPolicyDefaultOption', 'none'); - jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockImplementation(async () => { - return fakeUser; - }); + jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(fakeUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); const subworkflow = new Workflow({ @@ -248,7 +246,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { 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') - .mockImplementation(async () => nonOwnerUser); + .mockResolvedValue(nonOwnerUser) jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); const subworkflow = new Workflow({ From 665ca7e396c707ce6cd7e165c479be8e956d82bd Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Wed, 1 Nov 2023 17:56:41 +0100 Subject: [PATCH 3/3] fix: Lint issue --- packages/cli/test/unit/PermissionChecker.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index 0456fd417488c..e0c8e33b0b8fb 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -244,9 +244,7 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { }); 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(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(nonOwnerUser); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); const subworkflow = new Workflow({