add tests

packages/cli/src/UserManagement/PermissionChecker.ts

@@ -47,20 +47,13 @@ export class PermissionChecker {
 		let workflowUserIds = [userId];
 
 		if (workflow.id && isSharingEnabled) {
-			const workflowSharings = await this.sharedWorkflowRepository.find({
-				relations: ['workflow'],
-				where: { workflowId: workflow.id },
-				select: ['userId'],
-			});
-			workflowUserIds = workflowSharings.map((s) => s.userId);
+			workflowUserIds = await this.sharedWorkflowRepository.getSharedUserIds(workflow.id);
 		}
 
-		const credentialSharings =
-			await this.sharedCredentialsRepository[
-				isSharingEnabled ? 'findAccessibleSharings' : 'findOwnedSharings'
-			](workflowUserIds);
-
-		const accessibleCredIds = credentialSharings.map((s) => s.credentialsId);
+		const accessibleCredIds = await this.sharedCredentialsRepository.getCredentialIdsForUserAndRole(
+			workflowUserIds,
+			isSharingEnabled ? ['credential:owner', 'credential:user'] : ['credential:owner'],
+		);
 
 		const inaccessibleCredIds = workflowCredIds.filter((id) => !accessibleCredIds.includes(id));
 

packages/cli/src/credentials/credentials.service.ts

@@ -63,7 +63,7 @@ export class CredentialsService {
 				: credentials;
 		}
 
-		const ids = await this.sharedCredentialsRepository.getAccessibleCredentials(user.id);
+		const ids = await this.sharedCredentialsRepository.getAccessibleCredentialIds(user.id);
 
 		const credentials = await this.credentialsRepository.findMany(
 			options.listQueryOptions,

packages/cli/src/databases/repositories/sharedCredentials.repository.ts

@@ -1,7 +1,7 @@
 import { Service } from 'typedi';
 import type { EntityManager } from 'typeorm';
 import { DataSource, In, Not, Repository } from 'typeorm';
-import { SharedCredentials } from '../entities/SharedCredentials';
+import { type CredentialSharingRole, SharedCredentials } from '../entities/SharedCredentials';
 import type { User } from '../entities/User';
 
 @Service()
@@ -36,33 +36,25 @@ export class SharedCredentialsRepository extends Repository<SharedCredentials> {
 		return await this.update({ userId: Not(user.id), role: 'credential:owner' }, { user });
 	}
 
-	/**
-	 * Get the IDs of all credentials owned by or shared with a user.
-	 */
-	async getAccessibleCredentials(userId: string) {
+	/** Get the IDs of all credentials owned by or shared with a user */
+	async getAccessibleCredentialIds(userIds: string | string[]) {
+		return await this.getCredentialIdsForUserAndRole(userIds, [
+			'credential:owner',
+			'credential:user',
+		]);
+	}
+
+	async getCredentialIdsForUserAndRole(userIds: string | string[], roles: CredentialSharingRole[]) {
+		if (!Array.isArray(userIds)) userIds = [userIds];
 		const sharings = await this.find({
 			where: {
-				userId,
-				role: In(['credential:owner', 'credential:user']),
+				userId: In(userIds),
+				role: In(roles),
 			},
 		});
-
 		return sharings.map((s) => s.credentialsId);
 	}
 
-	async findAccessibleSharings(userIds: string[]) {
-		return await this.findBy({
-			userId: In(userIds),
-		});
-	}
-
-	async findOwnedSharings(userIds: string[]) {
-		return await this.findBy({
-			userId: In(userIds),
-			role: 'credential:owner',
-		});
-	}
-
 	async deleteByIds(transaction: EntityManager, sharedCredentialsIds: string[], user?: User) {
 		return await transaction.delete(SharedCredentials, {
 			user,

packages/cli/src/databases/repositories/sharedWorkflow.repository.ts

@@ -22,6 +22,14 @@ export class SharedWorkflowRepository extends Repository<SharedWorkflow> {
 		return await this.exist({ where });
 	}
 
+	async getSharedUserIds(workflowId: string) {
+		const sharedWorkflows = await this.find({
+			select: ['userId'],
+			where: { workflowId },
+		});
+		return sharedWorkflows.map((sharing) => sharing.userId);
+	}
+
 	async getSharedWorkflowIds(workflowIds: string[]) {
 		const sharedWorkflows = await this.find({
 			select: ['workflowId'],

packages/cli/test/unit/PermissionChecker.test.ts

@@ -0,0 +1,135 @@
+import { type INodeTypes, Workflow } from 'n8n-workflow';
+import { mock } from 'jest-mock-extended';
+import type { User } from '@db/entities/User';
+import type { UserRepository } from '@db/repositories/user.repository';
+import type { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository';
+import type { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository';
+import type { License } from '@/License';
+import { PermissionChecker } from '@/UserManagement/PermissionChecker';
+
+describe('PermissionChecker', () => {
+	const user = mock<User>();
+	const userRepo = mock<UserRepository>();
+	const sharedCredentialsRepo = mock<SharedCredentialsRepository>();
+	const sharedWorkflowRepo = mock<SharedWorkflowRepository>();
+	const license = mock<License>();
+	const permissionChecker = new PermissionChecker(
+		userRepo,
+		sharedCredentialsRepo,
+		sharedWorkflowRepo,
+		mock(),
+		license,
+	);
+
+	const workflow = new Workflow({
+		id: '1',
+		name: 'test',
+		active: false,
+		connections: {},
+		nodeTypes: mock<INodeTypes>(),
+		nodes: [
+			{
+				id: 'node-id',
+				name: 'HTTP Request',
+				type: 'n8n-nodes-base.httpRequest',
+				parameters: {},
+				typeVersion: 1,
+				position: [0, 0],
+				credentials: {
+					oAuth2Api: {
+						id: 'cred-id',
+						name: 'Custom oAuth2',
+					},
+				},
+			},
+		],
+	});
+
+	beforeEach(() => jest.clearAllMocks());
+
+	describe('check', () => {
+		it('should throw if no user is found', async () => {
+			userRepo.findOneOrFail.mockRejectedValue(new Error('Fail'));
+			await expect(permissionChecker.check(workflow, '123')).rejects.toThrow();
+			expect(license.isSharingEnabled).not.toHaveBeenCalled();
+			expect(sharedCredentialsRepo.getCredentialIdsForUserAndRole).not.toHaveBeenCalled();
+		});
+
+		it('should allow a user if they have a global `workflow:execute` scope', async () => {
+			userRepo.findOneOrFail.mockResolvedValue(user);
+			user.hasGlobalScope.calledWith('workflow:execute').mockReturnValue(true);
+			await expect(permissionChecker.check(workflow, user.id)).resolves.not.toThrow();
+			expect(license.isSharingEnabled).not.toHaveBeenCalled();
+			expect(sharedCredentialsRepo.getCredentialIdsForUserAndRole).not.toHaveBeenCalled();
+		});
+
+		describe('When sharing is disabled', () => {
+			beforeEach(() => {
+				userRepo.findOneOrFail.mockResolvedValue(user);
+				user.hasGlobalScope.calledWith('workflow:execute').mockReturnValue(false);
+				license.isSharingEnabled.mockReturnValue(false);
+			});
+
+			it('should validate credential access using only owned credentials', async () => {
+				sharedCredentialsRepo.getCredentialIdsForUserAndRole.mockResolvedValue(['cred-id']);
+
+				await expect(permissionChecker.check(workflow, user.id)).resolves.not.toThrow();
+
+				expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled();
+				expect(sharedCredentialsRepo.getCredentialIdsForUserAndRole).toBeCalledWith(
+					[user.id],
+					['credential:owner'],
+				);
+			});
+
+			it('should throw when the user does not have access to the credential', async () => {
+				sharedCredentialsRepo.getCredentialIdsForUserAndRole.mockResolvedValue(['cred-id2']);
+
+				await expect(permissionChecker.check(workflow, user.id)).rejects.toThrow(
+					'Node has no access to credential',
+				);
+
+				expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled();
+				expect(sharedCredentialsRepo.getCredentialIdsForUserAndRole).toBeCalledWith(
+					[user.id],
+					['credential:owner'],
+				);
+			});
+		});
+
+		describe('When sharing is enabled', () => {
+			beforeEach(() => {
+				userRepo.findOneOrFail.mockResolvedValue(user);
+				user.hasGlobalScope.calledWith('workflow:execute').mockReturnValue(false);
+				license.isSharingEnabled.mockReturnValue(true);
+				sharedWorkflowRepo.getSharedUserIds.mockResolvedValue([user.id, 'another-user']);
+			});
+
+			it('should validate credential access using only owned credentials', async () => {
+				sharedCredentialsRepo.getCredentialIdsForUserAndRole.mockResolvedValue(['cred-id']);
+
+				await expect(permissionChecker.check(workflow, user.id)).resolves.not.toThrow();
+
+				expect(sharedWorkflowRepo.getSharedUserIds).toBeCalledWith(workflow.id);
+				expect(sharedCredentialsRepo.getCredentialIdsForUserAndRole).toBeCalledWith(
+					[user.id, 'another-user'],
+					['credential:owner', 'credential:user'],
+				);
+			});
+
+			it('should throw when the user does not have access to the credential', async () => {
+				sharedCredentialsRepo.getCredentialIdsForUserAndRole.mockResolvedValue(['cred-id2']);
+
+				await expect(permissionChecker.check(workflow, user.id)).rejects.toThrow(
+					'Node has no access to credential',
+				);
+
+				expect(sharedWorkflowRepo.find).not.toBeCalled();
+				expect(sharedCredentialsRepo.getCredentialIdsForUserAndRole).toBeCalledWith(
+					[user.id, 'another-user'],
+					['credential:owner', 'credential:user'],
+				);
+			});
+		});
+	});
+});