refactor(core): Use DI in PermissionChecker (no-changelog) (#8344)

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;

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,

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,

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

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(

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(

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();
 		});