From 3206b449743881f2ca21614190f60c525f39c3ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 8 Dec 2023 11:21:43 +0100 Subject: [PATCH] test(core): Improve tests for subworkflow caller policy checks (no-changelog) (#7954) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Deduplicate, separate, organize and speed up tests for subworkflow caller policy checks. Follow-up to: https://github.com/n8n-io/n8n/pull/7913 ``` PASS test/unit/PermissionChecker.test.ts check() ✓ should allow if workflow has no creds (3 ms) ✓ should allow if requesting user is instance owner (83 ms) ✓ should allow if workflow creds are valid subset (151 ms) ✓ should deny if workflow creds are not valid subset (85 ms) checkSubworkflowExecutePolicy() no caller policy ✓ should fall back to N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION (1 ms) overridden caller policy ✓ if no sharing, policy becomes workflows-from-same-owner (1 ms) workflows-from-list caller policy ✓ should allow if caller list contains parent workflow ID ✓ should deny if caller list does not contain parent workflow ID (1 ms) any caller policy ✓ should not throw workflows-from-same-owner caller policy ✓ should deny if the two workflows are owned by different users (1 ms) ✓ should allow if both workflows are owned by the same user ``` ... #### How to test the change: 1. ... ## Issues fixed Include links to Github issue or Community forum post or **Linear ticket**: > Important in order to close automatically and provide context to reviewers ... ## Review / Merge checklist - [ ] PR title and summary are descriptive. **Remember, the title automatically goes into the changelog. Use `(no-changelog)` otherwise.** ([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md)) - [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up ticket created. - [ ] Tests included. > A bug is not considered fixed, unless a test is added to prevent it from happening again. A feature is not complete without tests. > > *(internal)* You can use Slack commands to trigger [e2e tests](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#a39f9e5ba64a48b58a71d81c837e8227) or [deploy test instance](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#f6a177d32bde4b57ae2da0b8e454bfce) or [deploy early access version on Cloud](https://www.notion.so/n8n/Cloudbot-3dbe779836004972b7057bc989526998?pvs=4#fef2d36ab02247e1a0f65a74f6fb534e). --- .../cli/test/unit/PermissionChecker.test.ts | 307 +++++++----------- 1 file changed, 124 insertions(+), 183 deletions(-) diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index 4b1420cfc8..d411db0ea3 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -4,13 +4,11 @@ import type { INodeTypes, WorkflowSettings } from 'n8n-workflow'; import { SubworkflowOperationError, Workflow } from 'n8n-workflow'; import config from '@/config'; -import { Role } from '@db/entities/Role'; +import type { Role } from '@db/entities/Role'; import { User } from '@db/entities/User'; -import { SharedWorkflow } from '@db/entities/SharedWorkflow'; import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import { NodeTypes } from '@/NodeTypes'; import { PermissionChecker } from '@/UserManagement/PermissionChecker'; -import * as UserManagementHelper from '@/UserManagement/UserManagementHelper'; import { OwnershipService } from '@/services/ownership.service'; import { mockInstance } from '../shared/mocking'; @@ -31,9 +29,35 @@ 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 }) { +export const toTargetCallErrorMsg = (subworkflowId: string) => + `Target workflow ID ${subworkflowId} may not be called`; + +export function createParentWorkflow() { + return Container.get(WorkflowRepository).create({ + id: generateNanoId(), + name: randomName(), + active: false, + connections: {}, + nodes: [ + { + name: '', + typeVersion: 1, + type: 'n8n-nodes-base.executeWorkflow', + position: [0, 0], + parameters: {}, + }, + ], + }); +} + +export function createSubworkflow({ + policy, + callerIds, +}: { + policy?: WorkflowSettings.CallerPolicy; + callerIds?: string; +} = {}) { return new Workflow({ id: uuid(), nodes: [], @@ -41,7 +65,8 @@ function createSubworkflow({ policy }: { policy: WorkflowSettings.CallerPolicy } active: false, nodeTypes: mockNodeTypes, settings: { - callerPolicy: policy, + ...(policy ? { callerPolicy: policy } : {}), + ...(callerIds ? { callerIds } : {}), }, }); } @@ -66,15 +91,15 @@ beforeAll(async () => { saveCredential = affixRoleToSaveCredential(credentialOwnerRole); }); -beforeEach(async () => { - await testDb.truncate(['SharedWorkflow', 'SharedCredentials', 'Workflow', 'Credentials', 'User']); -}); +describe('check()', () => { + beforeEach(async () => { + await testDb.truncate(['Workflow', 'Credentials']); + }); -afterAll(async () => { - await testDb.terminate(); -}); + afterAll(async () => { + await testDb.terminate(); + }); -describe('PermissionChecker.check()', () => { test('should allow if workflow has no creds', async () => { const userId = uuid(); @@ -233,206 +258,122 @@ describe('PermissionChecker.check()', () => { }); }); -describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { - const userId = uuid(); - const fakeUser = new User(); - fakeUser.id = userId; +describe('checkSubworkflowExecutePolicy()', () => { const ownershipService = mockInstance(OwnershipService); - const ownerMockRole = new Role(); - ownerMockRole.name = 'owner'; - const sharedWorkflowOwner = new SharedWorkflow(); - sharedWorkflowOwner.role = ownerMockRole; - - const nonOwnerMockRole = new Role(); - nonOwnerMockRole.name = 'editor'; - const nonOwnerUser = new User(); - nonOwnerUser.id = uuid(); - - let parentWorkflow: WorkflowEntity; + let license: LicenseMocker; 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: {}, - }, - ], + license = new LicenseMocker(); + license.mock(Container.get(License)); + license.enable('feat:sharing'); + }); + + describe('no caller policy', () => { + test('should fall back to N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION', async () => { + config.set('workflows.callerPolicyDefaultOption', 'none'); + + const parentWorkflow = createParentWorkflow(); + const subworkflow = createSubworkflow(); // no caller policy + + ownershipService.getWorkflowOwnerCached.mockResolvedValue(new User()); + + const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + + await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id)); + + config.load(config.default); }); }); - test('sets default policy from environment when subworkflow has none', async () => { - await Container.get(WorkflowRepository).save(parentWorkflow); + describe('overridden caller policy', () => { + test('if no sharing, should override policy to workflows-from-same-owner', async () => { + license.disable('feat:sharing'); - config.set('workflows.callerPolicyDefaultOption', 'none'); - jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(fakeUser); - jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); + const parentWorkflow = createParentWorkflow(); + const subworkflow = createSubworkflow({ policy: 'any' }); // should be overridden - const subworkflow = new Workflow({ - nodes: [], - connections: {}, - active: false, - nodeTypes: mockNodeTypes, - id: '2', - }); - await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), - ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); - }); + const firstUser = Container.get(UserRepository).create({ id: uuid() }); + const secondUser = Container.get(UserRepository).create({ id: uuid() }); - 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 () => { - await Container.get(WorkflowRepository).save(parentWorkflow); + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(firstUser); // parent workflow + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(secondUser); // subworkflow - jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValueOnce(fakeUser); - jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValueOnce(nonOwnerUser); - jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); + const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); - const subworkflow = new Workflow({ - nodes: [], - connections: {}, - active: false, - nodeTypes: mockNodeTypes, - id: '2', - settings: { - callerPolicy: 'any', - }, - }); - await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), - ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); + await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id)); - // Check description - try { - await PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, 'abcde'); - } catch (error) { - if (error instanceof SubworkflowOperationError) { - expect(error.description).toBe( - `${fakeUser.firstName} (${fakeUser.email}) can make this change. You may need to tell them the ID of this workflow, which is ${subworkflow.id}`, - ); + try { + await PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); + } catch (error) { + if (error instanceof SubworkflowOperationError) { + expect(error.description).toBe( + `${firstUser.firstName} (${firstUser.email}) can make this change. You may need to tell them the ID of this workflow, which is ${subworkflow.id}`, + ); + } } - } - }); - test('should throw if allowed list does not contain parent workflow id', async () => { - await Container.get(WorkflowRepository).save(parentWorkflow); - - jest - .spyOn(ownershipService, 'getWorkflowOwnerCached') - .mockImplementation(async (workflowId) => fakeUser); - jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); - - const subworkflow = new Workflow({ - nodes: [], - connections: {}, - active: false, - nodeTypes: mockNodeTypes, - id: '2', - settings: { - callerPolicy: 'workflowsFromAList', - callerIds: '123,456,bcdef ', - }, - }); - await expect( - 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); - - const subworkflow = new Workflow({ - nodes: [], - connections: {}, - active: false, - nodeTypes: mockNodeTypes, - id: '2', - }); - await expect( - PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id), - ).resolves.not.toThrow(); - }); - - test('workflowsFromAList works when the list contains the parent id', async () => { - await Container.get(WorkflowRepository).save(parentWorkflow); - - jest - .spyOn(ownershipService, 'getWorkflowOwnerCached') - .mockImplementation(async (workflowId) => fakeUser); - jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); - - const subworkflow = new Workflow({ - nodes: [], - connections: {}, - active: false, - nodeTypes: mockNodeTypes, - id: '2', - settings: { - callerPolicy: 'workflowsFromAList', - callerIds: `123,456,bcdef, ${parentWorkflow.id}`, - }, - }); - await expect( - 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); - jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); - - const subworkflow = new Workflow({ - nodes: [], - connections: {}, - active: false, - nodeTypes: mockNodeTypes, - id: '2', - settings: { - callerPolicy: 'any', - }, - }); - await expect( - 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'); }); + }); + describe('workflows-from-list caller policy', () => { + test('should allow if caller list contains parent workflow ID', async () => { + const parentWorkflow = createParentWorkflow(); + + const subworkflow = createSubworkflow({ + policy: 'workflowsFromAList', + callerIds: `123,456,bcdef, ${parentWorkflow.id}`, + }); + + const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + + await expect(check).resolves.not.toThrow(); + }); + + test('should deny if caller list does not contain parent workflow ID', async () => { + const parentWorkflow = createParentWorkflow(); + + const subworkflow = createSubworkflow({ + policy: 'workflowsFromAList', + callerIds: 'xyz', + }); + + const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + + await expect(check).rejects.toThrow(); + }); + }); + + describe('any caller policy', () => { + test('should not throw', async () => { + const parentWorkflow = createParentWorkflow(); + const subworkflow = createSubworkflow({ policy: 'any' }); + ownershipService.getWorkflowOwnerCached.mockResolvedValue(new User()); + + const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + + await expect(check).resolves.not.toThrow(); + }); + }); + + describe('workflows-from-same-owner caller policy', () => { 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); + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(parentWorkflowOwner); // parent workflow + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' }); const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); - await expect(check).rejects.toThrow(); + await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id)); }); test('should allow if both workflows are owned by the same user', async () => { - await Container.get(WorkflowRepository).save(parentWorkflow); + const parentWorkflow = createParentWorkflow(); const bothWorkflowsOwner = Container.get(UserRepository).create({ id: uuid() });