From 64ceb16af6343761ba5efd567dfa2a919e97a2d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jan 2024 14:15:29 +0100 Subject: [PATCH] refactor(core): Use DI in PermissionChecker (no-changelog) (#8344) --- .../src/UserManagement/PermissionChecker.ts | 34 ++++++++----- .../cli/src/WorkflowExecuteAdditionalData.ts | 4 +- packages/cli/src/WorkflowHelpers.ts | 2 +- packages/cli/src/WorkflowRunner.ts | 2 +- packages/cli/src/WorkflowRunnerProcess.ts | 2 +- packages/cli/src/commands/worker.ts | 2 +- .../cli/test/unit/PermissionChecker.test.ts | 48 ++++++++++--------- 7 files changed, 52 insertions(+), 42 deletions(-) diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index a5e1ffdb8a413..e2e42a4481a5d 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -1,22 +1,32 @@ +import { Service } from 'typedi'; import type { INode, Workflow } from 'n8n-workflow'; import { NodeOperationError, WorkflowOperationError } from 'n8n-workflow'; + import config from '@/config'; import { isSharingEnabled } from './UserManagementHelper'; import { OwnershipService } from '@/services/ownership.service'; -import Container from 'typedi'; import { RoleService } from '@/services/role.service'; import { UserRepository } from '@db/repositories/user.repository'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; +@Service() export class PermissionChecker { + constructor( + private readonly userRepository: UserRepository, + private readonly sharedCredentialsRepository: SharedCredentialsRepository, + private readonly sharedWorkflowRepository: SharedWorkflowRepository, + private readonly roleService: RoleService, + private readonly ownershipService: OwnershipService, + ) {} + /** * Check if a user is permitted to execute a workflow. */ - static async check(workflow: Workflow, userId: string) { + async check(workflow: Workflow, userId: string) { // allow if no nodes in this workflow use creds - const credIdsToNodes = PermissionChecker.mapCredIdsToNodes(workflow); + const credIdsToNodes = this.mapCredIdsToNodes(workflow); const workflowCredIds = Object.keys(credIdsToNodes); @@ -24,7 +34,7 @@ export class PermissionChecker { // allow if requesting user is instance owner - const user = await Container.get(UserRepository).findOneOrFail({ + const user = await this.userRepository.findOneOrFail({ where: { id: userId }, relations: ['globalRole'], }); @@ -37,7 +47,7 @@ export class PermissionChecker { let workflowUserIds = [userId]; if (workflow.id && isSharingEnabled()) { - const workflowSharings = await Container.get(SharedWorkflowRepository).find({ + const workflowSharings = await this.sharedWorkflowRepository.find({ relations: ['workflow'], where: { workflowId: workflow.id }, select: ['userId'], @@ -45,9 +55,9 @@ export class PermissionChecker { workflowUserIds = workflowSharings.map((s) => s.userId); } - const roleId = await Container.get(RoleService).findCredentialOwnerRoleId(); + const roleId = await this.roleService.findCredentialOwnerRoleId(); - const credentialSharings = await Container.get(SharedCredentialsRepository).findSharings( + const credentialSharings = await this.sharedCredentialsRepository.findSharings( workflowUserIds, roleId, ); @@ -68,7 +78,7 @@ export class PermissionChecker { }); } - static async checkSubworkflowExecutePolicy( + async checkSubworkflowExecutePolicy( subworkflow: Workflow, parentWorkflowId: string, node?: INode, @@ -94,11 +104,9 @@ export class PermissionChecker { } const parentWorkflowOwner = - await Container.get(OwnershipService).getWorkflowOwnerCached(parentWorkflowId); + await this.ownershipService.getWorkflowOwnerCached(parentWorkflowId); - const subworkflowOwner = await Container.get(OwnershipService).getWorkflowOwnerCached( - subworkflow.id, - ); + const subworkflowOwner = await this.ownershipService.getWorkflowOwnerCached(subworkflow.id); const description = subworkflowOwner.id === parentWorkflowOwner.id @@ -134,7 +142,7 @@ export class PermissionChecker { } } - private static mapCredIdsToNodes(workflow: Workflow) { + private mapCredIdsToNodes(workflow: Workflow) { return Object.values(workflow.nodes).reduce<{ [credentialId: string]: INode[] }>( (map, node) => { if (node.disabled || !node.credentials) return map; diff --git a/packages/cli/src/WorkflowExecuteAdditionalData.ts b/packages/cli/src/WorkflowExecuteAdditionalData.ts index 143ef169a4aca..a170d01f85cc3 100644 --- a/packages/cli/src/WorkflowExecuteAdditionalData.ts +++ b/packages/cli/src/WorkflowExecuteAdditionalData.ts @@ -795,8 +795,8 @@ async function executeWorkflow( let data; try { - await PermissionChecker.check(workflow, additionalData.userId); - await PermissionChecker.checkSubworkflowExecutePolicy( + await Container.get(PermissionChecker).check(workflow, additionalData.userId); + await Container.get(PermissionChecker).checkSubworkflowExecutePolicy( workflow, options.parentWorkflowId, options.node, diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index 010dee308aadc..9186ea5e8b821 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -176,7 +176,7 @@ export async function executeErrorWorkflow( const failedNode = workflowErrorData.execution?.lastNodeExecuted ? workflowInstance.getNode(workflowErrorData.execution?.lastNodeExecuted) : undefined; - await PermissionChecker.checkSubworkflowExecutePolicy( + await Container.get(PermissionChecker).checkSubworkflowExecutePolicy( workflowInstance, workflowErrorData.workflow.id!, failedNode ?? undefined, diff --git a/packages/cli/src/WorkflowRunner.ts b/packages/cli/src/WorkflowRunner.ts index 1242bc876dc80..a6534af3376c0 100644 --- a/packages/cli/src/WorkflowRunner.ts +++ b/packages/cli/src/WorkflowRunner.ts @@ -327,7 +327,7 @@ export class WorkflowRunner { ); try { - await PermissionChecker.check(workflow, data.userId); + await Container.get(PermissionChecker).check(workflow, data.userId); } catch (error) { ErrorReporter.error(error); // Create a failed execution with the data for the node diff --git a/packages/cli/src/WorkflowRunnerProcess.ts b/packages/cli/src/WorkflowRunnerProcess.ts index 4bb8ba24ccfd6..199a418416170 100644 --- a/packages/cli/src/WorkflowRunnerProcess.ts +++ b/packages/cli/src/WorkflowRunnerProcess.ts @@ -142,7 +142,7 @@ class WorkflowRunnerProcess { pinData: this.data.pinData, }); try { - await PermissionChecker.check(this.workflow, userId); + await Container.get(PermissionChecker).check(this.workflow, userId); } catch (error) { const caughtError = error as NodeOperationError; const failedExecutionData = generateFailedExecutionFromError( diff --git a/packages/cli/src/commands/worker.ts b/packages/cli/src/commands/worker.ts index 4848ff7d109cd..d1fe4e935c085 100644 --- a/packages/cli/src/commands/worker.ts +++ b/packages/cli/src/commands/worker.ts @@ -185,7 +185,7 @@ export class Worker extends BaseCommand { ); try { - await PermissionChecker.check(workflow, workflowOwner.id); + await Container.get(PermissionChecker).check(workflow, workflowOwner.id); } catch (error) { if (error instanceof NodeOperationError) { const failedExecution = generateFailedExecutionFromError( diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index d411db0ea346d..07c24fa11fa95 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -1,15 +1,20 @@ import { v4 as uuid } from 'uuid'; import { Container } from 'typedi'; -import type { INodeTypes, WorkflowSettings } from 'n8n-workflow'; +import type { WorkflowSettings } from 'n8n-workflow'; import { SubworkflowOperationError, Workflow } from 'n8n-workflow'; import config from '@/config'; import type { Role } from '@db/entities/Role'; import { User } from '@db/entities/User'; +import { WorkflowRepository } from '@db/repositories/workflow.repository'; +import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; +import { UserRepository } from '@/databases/repositories/user.repository'; +import { generateNanoId } from '@/databases/utils/generators'; +import { License } from '@/License'; import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import { NodeTypes } from '@/NodeTypes'; -import { PermissionChecker } from '@/UserManagement/PermissionChecker'; import { OwnershipService } from '@/services/ownership.service'; +import { PermissionChecker } from '@/UserManagement/PermissionChecker'; import { mockInstance } from '../shared/mocking'; import { @@ -17,18 +22,13 @@ import { randomName, randomPositiveDigit, } from '../integration/shared/random'; +import { LicenseMocker } from '../integration/shared/license'; import * as testDb from '../integration/shared/testDb'; import type { SaveCredentialFunction } from '../integration/shared/types'; import { mockNodeTypesData } from './Helpers'; import { affixRoleToSaveCredential } from '../integration/shared/db/credentials'; import { getCredentialOwnerRole, getWorkflowOwnerRole } from '../integration/shared/db/roles'; 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'; export const toTargetCallErrorMsg = (subworkflowId: string) => `Target workflow ID ${subworkflowId} may not be called`; @@ -71,24 +71,26 @@ export function createSubworkflow({ }); } -let mockNodeTypes: INodeTypes; let credentialOwnerRole: Role; let workflowOwnerRole: Role; let saveCredential: SaveCredentialFunction; +const mockNodeTypes = mockInstance(NodeTypes); mockInstance(LoadNodesAndCredentials, { loadedNodes: mockNodeTypesData(['start', 'actionNetwork']), }); +let permissionChecker: PermissionChecker; + beforeAll(async () => { await testDb.init(); - mockNodeTypes = Container.get(NodeTypes); - credentialOwnerRole = await getCredentialOwnerRole(); workflowOwnerRole = await getWorkflowOwnerRole(); saveCredential = affixRoleToSaveCredential(credentialOwnerRole); + + permissionChecker = Container.get(PermissionChecker); }); describe('check()', () => { @@ -121,7 +123,7 @@ describe('check()', () => { ], }); - expect(async () => PermissionChecker.check(workflow, userId)).not.toThrow(); + expect(async () => permissionChecker.check(workflow, userId)).not.toThrow(); }); test('should allow if requesting user is instance owner', async () => { @@ -151,7 +153,7 @@ describe('check()', () => { ], }); - expect(async () => PermissionChecker.check(workflow, owner.id)).not.toThrow(); + expect(async () => permissionChecker.check(workflow, owner.id)).not.toThrow(); }); test('should allow if workflow creds are valid subset', async () => { @@ -198,7 +200,7 @@ describe('check()', () => { ], }); - expect(async () => PermissionChecker.check(workflow, owner.id)).not.toThrow(); + expect(async () => permissionChecker.check(workflow, owner.id)).not.toThrow(); }); test('should deny if workflow creds are not valid subset', async () => { @@ -254,7 +256,7 @@ describe('check()', () => { const workflow = new Workflow(workflowDetails); - await expect(PermissionChecker.check(workflow, member.id)).rejects.toThrow(); + await expect(permissionChecker.check(workflow, member.id)).rejects.toThrow(); }); }); @@ -278,7 +280,7 @@ describe('checkSubworkflowExecutePolicy()', () => { ownershipService.getWorkflowOwnerCached.mockResolvedValue(new User()); - const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id)); @@ -299,12 +301,12 @@ describe('checkSubworkflowExecutePolicy()', () => { ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(firstUser); // parent workflow ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(secondUser); // subworkflow - const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id)); try { - await PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); + await permissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); } catch (error) { if (error instanceof SubworkflowOperationError) { expect(error.description).toBe( @@ -326,7 +328,7 @@ describe('checkSubworkflowExecutePolicy()', () => { callerIds: `123,456,bcdef, ${parentWorkflow.id}`, }); - const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); await expect(check).resolves.not.toThrow(); }); @@ -339,7 +341,7 @@ describe('checkSubworkflowExecutePolicy()', () => { callerIds: 'xyz', }); - const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); await expect(check).rejects.toThrow(); }); @@ -351,7 +353,7 @@ describe('checkSubworkflowExecutePolicy()', () => { const subworkflow = createSubworkflow({ policy: 'any' }); ownershipService.getWorkflowOwnerCached.mockResolvedValue(new User()); - const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); await expect(check).resolves.not.toThrow(); }); @@ -367,7 +369,7 @@ describe('checkSubworkflowExecutePolicy()', () => { const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' }); - const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); + const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id)); }); @@ -382,7 +384,7 @@ describe('checkSubworkflowExecutePolicy()', () => { const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' }); - const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); await expect(check).resolves.not.toThrow(); });