refactor(core): Improve UX on permission errors (no-changelog)

packages/cli/src/__tests__/workflow-execute-additional-data.test.ts

@@ -17,12 +17,14 @@ import { ExecutionRepository } from '@/databases/repositories/execution.reposito
 import { WorkflowRepository } from '@/databases/repositories/workflow.repository';
 import { VariablesService } from '@/environments.ee/variables/variables.service.ee';
 import { EventService } from '@/events/event.service';
+import {
+	CredentialsPermissionChecker,
+	SubworkflowPolicyChecker,
+} from '@/executions/pre-execution-checks';
 import { ExternalHooks } from '@/external-hooks';
 import { SecretsHelper } from '@/secrets-helpers.ee';
 import { WorkflowStatisticsService } from '@/services/workflow-statistics.service';
-import { SubworkflowPolicyChecker } from '@/subworkflows/subworkflow-policy-checker.service';
 import { Telemetry } from '@/telemetry';
-import { PermissionChecker } from '@/user-management/permission-checker';
 import { executeWorkflow, getBase, getRunData } from '@/workflow-execute-additional-data';
 import { mockInstance } from '@test/mocking';
 
@@ -91,7 +93,7 @@ describe('WorkflowExecuteAdditionalData', () => {
 	mockInstance(Telemetry);
 	const workflowRepository = mockInstance(WorkflowRepository);
 	const activeExecutions = mockInstance(ActiveExecutions);
-	mockInstance(PermissionChecker);
+	mockInstance(CredentialsPermissionChecker);
 	mockInstance(SubworkflowPolicyChecker);
 	mockInstance(WorkflowStatisticsService);
 

packages/cli/src/__tests__/workflow-runner.test.ts

@@ -21,8 +21,8 @@ import config from '@/config';
 import type { ExecutionEntity } from '@/databases/entities/execution-entity';
 import type { User } from '@/databases/entities/user';
 import { ExecutionNotFoundError } from '@/errors/execution-not-found-error';
+import { CredentialsPermissionChecker } from '@/executions/pre-execution-checks';
 import { Telemetry } from '@/telemetry';
-import { PermissionChecker } from '@/user-management/permission-checker';
 import { WorkflowRunner } from '@/workflow-runner';
 import { mockInstance } from '@test/mocking';
 import { createExecution } from '@test-integration/db/executions';
@@ -131,7 +131,7 @@ describe('run', () => {
 		const activeExecutions = Container.get(ActiveExecutions);
 		jest.spyOn(activeExecutions, 'add').mockResolvedValue('1');
 		jest.spyOn(activeExecutions, 'attachWorkflowExecution').mockReturnValueOnce();
-		const permissionChecker = Container.get(PermissionChecker);
+		const permissionChecker = Container.get(CredentialsPermissionChecker);
 		jest.spyOn(permissionChecker, 'check').mockResolvedValueOnce();
 
 		jest.spyOn(WorkflowExecute.prototype, 'processRunExecutionData').mockReturnValueOnce(
@@ -171,7 +171,7 @@ describe('run', () => {
 		const activeExecutions = Container.get(ActiveExecutions);
 		jest.spyOn(activeExecutions, 'add').mockResolvedValue('1');
 		jest.spyOn(activeExecutions, 'attachWorkflowExecution').mockReturnValueOnce();
-		const permissionChecker = Container.get(PermissionChecker);
+		const permissionChecker = Container.get(CredentialsPermissionChecker);
 		jest.spyOn(permissionChecker, 'check').mockResolvedValueOnce();
 
 		jest.spyOn(WorkflowExecute.prototype, 'processRunExecutionData').mockReturnValueOnce(

packages/cli/src/executions/__tests__/execution-data.service.test.ts

@@ -0,0 +1,47 @@
+import { mock } from 'jest-mock-extended';
+import { NodeOperationError } from 'n8n-workflow';
+import type { INode, WorkflowExecuteMode } from 'n8n-workflow';
+
+import { ExecutionDataService } from '../execution-data.service';
+
+describe('ExecutionDataService', () => {
+	const service = new ExecutionDataService();
+
+	describe('generateFailedExecutionFromError', () => {
+		const mode: WorkflowExecuteMode = 'manual';
+		const node = mock<INode>({ name: 'Test Node' });
+		const error = new NodeOperationError(node, 'Test error message');
+
+		it('should generate a failed execution with error details', () => {
+			const startTime = Date.now();
+
+			const result = service.generateFailedExecutionFromError(mode, error, node, startTime);
+
+			expect(result.mode).toBe(mode);
+			expect(result.status).toBe('error');
+			expect(result.startedAt).toBeInstanceOf(Date);
+			expect(result.stoppedAt).toBeInstanceOf(Date);
+			expect(result.data.resultData.error?.message).toBe(error.message);
+
+			const taskData = result.data.resultData.runData[node.name][0];
+			expect(taskData.error?.message).toBe(error.message);
+			expect(taskData.startTime).toBe(startTime);
+			expect(taskData.executionStatus).toBe('error');
+			expect(result.data.resultData.lastNodeExecuted).toBe(node.name);
+			expect(result.data.executionData?.nodeExecutionStack[0].node).toEqual(node);
+		});
+
+		it('should generate a failed execution without node details if node is undefined', () => {
+			const result = service.generateFailedExecutionFromError(mode, error, undefined);
+
+			expect(result.mode).toBe(mode);
+			expect(result.status).toBe('error');
+			expect(result.startedAt).toBeInstanceOf(Date);
+			expect(result.stoppedAt).toBeInstanceOf(Date);
+			expect(result.data.resultData.error?.message).toBe(error.message);
+			expect(result.data.resultData.runData).toEqual({});
+			expect(result.data.resultData.lastNodeExecuted).toBeUndefined();
+			expect(result.data.executionData).toBeUndefined();
+		});
+	});
+});

packages/cli/src/executions/execution-data.service.ts

@@ -0,0 +1,62 @@
+import { Service } from '@n8n/di';
+import type { ExecutionError, INode, IRun, WorkflowExecuteMode } from 'n8n-workflow';
+
+@Service()
+export class ExecutionDataService {
+	generateFailedExecutionFromError(
+		mode: WorkflowExecuteMode,
+		error: ExecutionError,
+		node: INode | undefined,
+		startTime = Date.now(),
+	): IRun {
+		const executionError = {
+			...error,
+			message: error.message,
+			stack: error.stack,
+		};
+		const returnData: IRun = {
+			data: {
+				resultData: {
+					error: executionError,
+					runData: {},
+				},
+			},
+			finished: false,
+			mode,
+			startedAt: new Date(),
+			stoppedAt: new Date(),
+			status: 'error',
+		};
+
+		if (node) {
+			returnData.data.startData = {
+				destinationNode: node.name,
+				runNodeFilter: [node.name],
+			};
+			returnData.data.resultData.lastNodeExecuted = node.name;
+			returnData.data.resultData.runData[node.name] = [
+				{
+					startTime,
+					executionTime: 0,
+					executionStatus: 'error',
+					error: executionError,
+					source: [],
+				},
+			];
+			returnData.data.executionData = {
+				contextData: {},
+				metadata: {},
+				waitingExecution: {},
+				waitingExecutionSource: {},
+				nodeExecutionStack: [
+					{
+						node,
+						data: {},
+						source: null,
+					},
+				],
+			};
+		}
+		return returnData;
+	}
+}

packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts

@@ -0,0 +1,108 @@
+import { mock } from 'jest-mock-extended';
+import type { INode } from 'n8n-workflow';
+
+import type { Project } from '@/databases/entities/project';
+import type { User } from '@/databases/entities/user';
+import type { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository';
+import type { OwnershipService } from '@/services/ownership.service';
+import type { ProjectService } from '@/services/project.service.ee';
+
+import { CredentialsPermissionChecker } from '../credentials-permission-checker';
+
+describe('CredentialsPermissionChecker', () => {
+	const sharedCredentialsRepository = mock<SharedCredentialsRepository>();
+	const ownershipService = mock<OwnershipService>();
+	const projectService = mock<ProjectService>();
+	const permissionChecker = new CredentialsPermissionChecker(
+		sharedCredentialsRepository,
+		ownershipService,
+		projectService,
+	);
+
+	const workflowId = 'workflow123';
+	const credentialId = 'cred123';
+	const personalProject = mock<Project>({
+		id: 'personal-project',
+		name: 'Personal Project',
+		type: 'personal',
+	});
+
+	const node = mock<INode>({
+		name: 'Test Node',
+		credentials: {
+			someCredential: {
+				id: credentialId,
+				name: 'Test Credential',
+			},
+		},
+		disabled: false,
+	});
+
+	beforeEach(async () => {
+		jest.resetAllMocks();
+
+		node.credentials!.someCredential.id = credentialId;
+		ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(personalProject);
+		projectService.findProjectsWorkflowIsIn.mockResolvedValueOnce([personalProject.id]);
+	});
+
+	it('should throw if a node has a credential without an id', async () => {
+		node.credentials!.someCredential.id = null;
+
+		await expect(permissionChecker.check(workflowId, [node])).rejects.toThrow(
+			'Node "Test Node" uses invalid credential',
+		);
+
+		expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId);
+		expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled();
+	});
+
+	it('should throw if a credential is not accessible', async () => {
+		ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(null);
+		sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValueOnce([]);
+
+		await expect(permissionChecker.check(workflowId, [node])).rejects.toThrow(
+			'Node "Test Node" does not have access to the credential',
+		);
+
+		expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId);
+		expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).toHaveBeenCalledWith(
+			[personalProject.id],
+			[credentialId],
+		);
+	});
+
+	it('should not throw an error if the workflow has no credentials', async () => {
+		await expect(permissionChecker.check(workflowId, [])).resolves.not.toThrow();
+
+		expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId);
+		expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled();
+	});
+
+	it('should not throw an error if all credentials are accessible', async () => {
+		ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(null);
+		sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValueOnce([
+			credentialId,
+		]);
+
+		await expect(permissionChecker.check(workflowId, [node])).resolves.not.toThrow();
+
+		expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId);
+		expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).toHaveBeenCalledWith(
+			[personalProject.id],
+			[credentialId],
+		);
+	});
+
+	it('should skip credential checks if the home project owner has global scope', async () => {
+		const projectOwner = mock<User>({
+			hasGlobalScope: (scope) => scope === 'credential:list',
+		});
+		ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(projectOwner);
+
+		await expect(permissionChecker.check(workflowId, [node])).resolves.not.toThrow();
+
+		expect(projectService.findProjectsWorkflowIsIn).not.toHaveBeenCalled();
+		expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled();
+	});
+});

packages/cli/src/subworkflows/__tests__/subworkflow-policy-checker.test.ts → packages/cli/src/executions/pre-execution-checks/__tests__/subworkflow-policy-checker.test.ts

@@ -15,7 +15,7 @@ import { OwnershipService } from '@/services/ownership.service';
 import type { UrlService } from '@/services/url.service';
 import { mockInstance } from '@test/mocking';
 
-import { SubworkflowPolicyChecker } from '../subworkflow-policy-checker.service';
+import { SubworkflowPolicyChecker } from '../subworkflow-policy-checker';
 
 describe('SubworkflowPolicyChecker', () => {
 	const ownershipService = mockInstance(OwnershipService);

packages/cli/src/user-management/permission-checker.ts → packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts

@@ -1,13 +1,36 @@
 import { Service } from '@n8n/di';
 import type { INode } from 'n8n-workflow';
-import { CredentialAccessError, NodeOperationError } from 'n8n-workflow';
+import { UserError } from 'n8n-workflow';
 
+import type { Project } from '@/databases/entities/project';
 import { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository';
 import { OwnershipService } from '@/services/ownership.service';
 import { ProjectService } from '@/services/project.service.ee';
 
+class InvalidCredentialError extends UserError {
+	override description = 'Please recreate the credential.';
+
+	constructor(readonly node: INode) {
+		super(`Node "${node.name}" uses invalid credential`);
+	}
+}
+
+class InaccessibleCredentialError extends UserError {
+	override description =
+		this.project.type === 'personal'
+			? 'Please recreate the credential or ask its owner to share it with you.'
+			: `Please make sure that the credential is shared with the project "${this.project.name}"`;
+
+	constructor(
+		readonly node: INode,
+		private readonly project: Project,
+	) {
+		super(`Node "${node.name}" does not have access to the credential`);
+	}
+}
+
 @Service()
-export class PermissionChecker {
+export class CredentialsPermissionChecker {
 	constructor(
 		private readonly sharedCredentialsRepository: SharedCredentialsRepository,
 		private readonly ownershipService: OwnershipService,
@@ -42,7 +65,7 @@ export class PermissionChecker {
 		for (const credentialsId of workflowCredIds) {
 			if (!accessible.includes(credentialsId)) {
 				const nodeToFlag = credIdsToNodes[credentialsId][0];
-				throw new CredentialAccessError(nodeToFlag, credentialsId, workflowId);
+				throw new InaccessibleCredentialError(nodeToFlag, homeProject);
 			}
 		}
 	}
@@ -52,12 +75,7 @@ export class PermissionChecker {
 			if (node.disabled || !node.credentials) return map;
 
 			Object.values(node.credentials).forEach((cred) => {
-				if (!cred.id) {
-					throw new NodeOperationError(node, 'Node uses invalid credential', {
-						description: 'Please recreate the credential.',
-						level: 'warning',
-					});
-				}
+				if (!cred.id) throw new InvalidCredentialError(node);
 
 				map[cred.id] = map[cred.id] ? [...map[cred.id], node] : [node];
 			});

packages/cli/src/executions/pre-execution-checks/index.ts

@@ -0,0 +1,2 @@
+export { CredentialsPermissionChecker } from './credentials-permission-checker';
+export { SubworkflowPolicyChecker } from './subworkflow-policy-checker';