feat: check for cred when updating workflow and remove credential_usage table (#4350) (no-changelog)

* feat: check for cred when updating workflow and remove credential_usage table

packages/cli/test/integration/shared/testDb.ts

@@ -237,7 +237,6 @@ function toTableName(sourceName: CollectionName | MappingName) {
 
 	return {
 		Credentials: 'credentials_entity',
-		CredentialUsage: 'credential_usage',
 		Workflow: 'workflow_entity',
 		Execution: 'execution_entity',
 		Tag: 'tag_entity',
@@ -642,18 +641,6 @@ export async function getWorkflowSharing(workflow: WorkflowEntity) {
 	});
 }
 
-// ----------------------------------
-//        credential usage
-// ----------------------------------
-
-export async function getCredentialUsageInWorkflow(workflowId: number) {
-	return Db.collections.CredentialUsage.find({
-		where: {
-			workflowId,
-		},
-	});
-}
-
 // ----------------------------------
 //        connection options
 // ----------------------------------

packages/cli/test/integration/workflows.controller.ee.test.ts

@@ -11,6 +11,7 @@ import config from '../../config';
 import type { AuthAgent, SaveCredentialFunction } from './shared/types';
 import { makeWorkflow } from './shared/utils';
 import { randomCredentialPayload } from './shared/random';
+import { INode, INodes } from 'n8n-workflow';
 
 jest.mock('../../src/telemetry');
 
@@ -72,7 +73,7 @@ describe('PUT /workflows/:id', () => {
 		expect(sharedWorkflows).toHaveLength(2);
 	});
 
-	test('PUT /workflows/:id/share should not fail when sharing with invalid user-id', async () => {
+	test('PUT /workflows/:id/share should succeed when sharing with invalid user-id', async () => {
 		const owner = await testDb.createUser({ globalRole: globalOwnerRole });
 		const workflow = await createWorkflow({}, owner);
 
@@ -218,12 +219,9 @@ describe('POST /workflows', () => {
 		const response = await authAgent(owner).post('/workflows').send(workflow);
 
 		expect(response.statusCode).toBe(200);
-
-		const usedCredentials = await testDb.getCredentialUsageInWorkflow(response.body.data.id);
-		expect(usedCredentials).toHaveLength(0);
 	});
 
-	it('Should save credential usage when saving a new workflow', async () => {
+	it('Should save a new workflow with credentials', async () => {
 		const owner = await testDb.createUser({ globalRole: globalOwnerRole });
 
 		const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner });
@@ -235,9 +233,6 @@ describe('POST /workflows', () => {
 		const response = await authAgent(owner).post('/workflows').send(workflow);
 
 		expect(response.statusCode).toBe(200);
-
-		const usedCredentials = await testDb.getCredentialUsageInWorkflow(response.body.data.id);
-		expect(usedCredentials).toHaveLength(1);
 	});
 
 	it('Should not allow saving a workflow using credential you have no access', async () => {
@@ -273,8 +268,6 @@ describe('POST /workflows', () => {
 		const response = await authAgent(owner).post('/workflows').send(workflow);
 
 		expect(response.statusCode).toBe(200);
-		const usedCredentials = await testDb.getCredentialUsageInWorkflow(response.body.data.id);
-		expect(usedCredentials).toHaveLength(1);
 	});
 
 	it('Should allow saving a workflow using a credential owned by others and shared with you', async () => {
@@ -291,7 +284,162 @@ describe('POST /workflows', () => {
 
 		const response = await authAgent(member2).post('/workflows').send(workflow);
 		expect(response.statusCode).toBe(200);
-		const usedCredentials = await testDb.getCredentialUsageInWorkflow(response.body.data.id);
-		expect(usedCredentials).toHaveLength(1);
+	});
+});
+
+describe('PATCH /workflows/:id', () => {
+	it('Should succeed when saving unchanged workflow nodes', async () => {
+		const owner = await testDb.createUser({ globalRole: globalOwnerRole });
+
+		const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner });
+		const workflow = await createWorkflow(
+			{
+				nodes: [
+					{
+						id: 'uuid-1234',
+						name: 'Start',
+						parameters: {},
+						position: [-20, 260],
+						type: 'n8n-nodes-base.start',
+						typeVersion: 1,
+						credentials: {
+							default: {
+								id: savedCredential.id.toString(),
+								name: savedCredential.name,
+							},
+						},
+					},
+				],
+			},
+			owner,
+		);
+
+		const response = await authAgent(owner).patch(`/workflows/${workflow.id}`).send({
+			name: 'new name',
+		});
+
+		expect(response.statusCode).toBe(200);
+	});
+
+	it('Should allow owner to add node containing credential not shared with the owner', async () => {
+		const owner = await testDb.createUser({ globalRole: globalOwnerRole });
+		const member = await testDb.createUser({ globalRole: globalMemberRole });
+
+		const savedCredential = await saveCredential(randomCredentialPayload(), { user: member });
+		const workflow = await createWorkflow({}, owner);
+
+		const response = await authAgent(owner)
+			.patch(`/workflows/${workflow.id}`)
+			.send({
+				nodes: [
+					{
+						id: 'uuid-1234',
+						name: 'Start',
+						parameters: {},
+						position: [-20, 260],
+						type: 'n8n-nodes-base.start',
+						typeVersion: 1,
+						credentials: {
+							default: {
+								id: savedCredential.id.toString(),
+								name: savedCredential.name,
+							},
+						},
+					},
+				],
+			});
+
+		expect(response.statusCode).toBe(200);
+	});
+
+	it('Should prevent member from adding node containing credential inaccessible to member', async () => {
+		const owner = await testDb.createUser({ globalRole: globalOwnerRole });
+		const member = await testDb.createUser({ globalRole: globalMemberRole });
+
+		const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner });
+		const workflow = await createWorkflow({}, member);
+
+		const response = await authAgent(member)
+			.patch(`/workflows/${workflow.id}`)
+			.send({
+				nodes: [
+					{
+						id: 'uuid-1234',
+						name: 'Start',
+						parameters: {},
+						position: [-20, 260],
+						type: 'n8n-nodes-base.start',
+						typeVersion: 1,
+						credentials: {},
+					},
+					{
+						id: 'uuid-12345',
+						name: 'Start',
+						parameters: {},
+						position: [-20, 260],
+						type: 'n8n-nodes-base.start',
+						typeVersion: 1,
+						credentials: {
+							default: {
+								id: savedCredential.id.toString(),
+								name: savedCredential.name,
+							},
+						},
+					},
+				],
+			});
+
+		expect(response.statusCode).toBe(400);
+	});
+
+	it('Should succeed but prevent modifying nodes that are read-only for the requester', async () => {
+		const member1 = await testDb.createUser({ globalRole: globalMemberRole });
+		const member2 = await testDb.createUser({ globalRole: globalMemberRole });
+
+		const savedCredential = await saveCredential(randomCredentialPayload(), { user: member1 });
+
+		const originalNodes: INode[] = [
+			{
+				id: 'uuid-1234',
+				name: 'Start',
+				parameters: {},
+				position: [-20, 260],
+				type: 'n8n-nodes-base.start',
+				typeVersion: 1,
+				credentials: {
+					default: {
+						id: savedCredential.id.toString(),
+						name: savedCredential.name,
+					},
+				},
+			},
+		];
+
+		const changedNodes: INode[] = [
+			{
+				id: 'uuid-1234',
+				name: 'End',
+				parameters: {},
+				position: [-20, 260],
+				type: 'n8n-nodes-base.no-op',
+				typeVersion: 1,
+				credentials: {
+					default: {
+						id: '200',
+						name: 'fake credential',
+					},
+				},
+			},
+		];
+
+		const workflow = await createWorkflow({ nodes: originalNodes }, member1);
+		await testDb.shareWorkflowWithUsers(workflow, [member2]);
+
+		const response = await authAgent(member2).patch(`/workflows/${workflow.id}`).send({
+			nodes: changedNodes,
+		});
+
+		expect(response.statusCode).toBe(200);
+		expect(response.body.data.nodes).toMatchObject(originalNodes);
 	});
 });

packages/cli/test/unit/WorkflowHelpers.test.ts

@@ -0,0 +1,224 @@
+import { INode, LoggerProxy } from 'n8n-workflow';
+import { WorkflowEntity } from '../../src/databases/entities/WorkflowEntity';
+import { CredentialsEntity } from '../../src/databases/entities/CredentialsEntity';
+import {
+	getNodesWithInaccessibleCreds,
+	validateWorkflowCredentialUsage,
+} from '../../src/WorkflowHelpers';
+import { getLogger } from '../../src/Logger';
+
+const FIRST_CREDENTIAL_ID = '1';
+const SECOND_CREDENTIAL_ID = '2';
+const THIRD_CREDENTIAL_ID = '3';
+
+const NODE_WITH_NO_CRED = '0133467b-df4a-473d-9295-fdd9d01fa45a';
+const NODE_WITH_ONE_CRED = '4673f869-f2dc-4a33-b053-ca3193bc5226';
+const NODE_WITH_TWO_CRED = '9b4208bd-8f10-4a6a-ad3b-da47a326f7da';
+
+beforeAll(() => {
+	LoggerProxy.init(getLogger());
+});
+
+describe('WorkflowHelpers', () => {
+	describe('getNodesWithInaccessibleCreds', () => {
+		test('Should return an empty list for a workflow without nodes', () => {
+			const workflow = getWorkflow();
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, []);
+			expect(nodesWithInaccessibleCreds).toHaveLength(0);
+		});
+
+		test('Should return an empty list for a workflow with nodes without credentials', () => {
+			const workflow = getWorkflow({ addNodeWithoutCreds: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, []);
+			expect(nodesWithInaccessibleCreds).toHaveLength(0);
+		});
+
+		test('Should return an element for a node with a credential without access', () => {
+			const workflow = getWorkflow({ addNodeWithOneCred: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, []);
+			expect(nodesWithInaccessibleCreds).toHaveLength(1);
+		});
+
+		test('Should return an empty list for a node with a credential with access', () => {
+			const workflow = getWorkflow({ addNodeWithOneCred: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, [
+				FIRST_CREDENTIAL_ID,
+			]);
+			expect(nodesWithInaccessibleCreds).toHaveLength(0);
+		});
+
+		test('Should return an element for a node with two credentials and mixed access', () => {
+			const workflow = getWorkflow({ addNodeWithTwoCreds: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, [
+				SECOND_CREDENTIAL_ID,
+			]);
+			expect(nodesWithInaccessibleCreds).toHaveLength(1);
+		});
+
+		test('Should return one node for a workflow with two nodes and two credentials', () => {
+			const workflow = getWorkflow({ addNodeWithOneCred: true, addNodeWithTwoCreds: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, [
+				SECOND_CREDENTIAL_ID,
+				THIRD_CREDENTIAL_ID,
+			]);
+			expect(nodesWithInaccessibleCreds).toHaveLength(1);
+		});
+
+		test('Should return one element for a workflows with two nodes and one credential', () => {
+			const workflow = getWorkflow({
+				addNodeWithoutCreds: true,
+				addNodeWithOneCred: true,
+				addNodeWithTwoCreds: true,
+			});
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, [
+				FIRST_CREDENTIAL_ID,
+			]);
+			expect(nodesWithInaccessibleCreds).toHaveLength(1);
+		});
+
+		test('Should return one element for a workflows with two nodes and partial credential access', () => {
+			const workflow = getWorkflow({ addNodeWithOneCred: true, addNodeWithTwoCreds: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, [
+				FIRST_CREDENTIAL_ID,
+				SECOND_CREDENTIAL_ID,
+			]);
+			expect(nodesWithInaccessibleCreds).toHaveLength(1);
+		});
+
+		test('Should return two elements for a workflows with two nodes and partial credential access', () => {
+			const workflow = getWorkflow({ addNodeWithOneCred: true, addNodeWithTwoCreds: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, [
+				SECOND_CREDENTIAL_ID,
+			]);
+			expect(nodesWithInaccessibleCreds).toHaveLength(2);
+		});
+
+		test('Should return two elements for a workflows with two nodes and no credential access', () => {
+			const workflow = getWorkflow({ addNodeWithOneCred: true, addNodeWithTwoCreds: true });
+			const nodesWithInaccessibleCreds = getNodesWithInaccessibleCreds(workflow, []);
+			expect(nodesWithInaccessibleCreds).toHaveLength(2);
+		});
+	});
+
+	describe('validateWorkflowCredentialUsage', () => {
+		it('Should throw error saving a workflow using credential without access', () => {
+			const newWorkflowVersion = getWorkflow({ addNodeWithOneCred: true });
+			const previousWorkflowVersion = getWorkflow();
+			expect(() => {
+				validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, []);
+			}).toThrow();
+		});
+
+		it('Should not throw error when saving a workflow using credential with access', () => {
+			const newWorkflowVersion = getWorkflow({ addNodeWithOneCred: true });
+			const previousWorkflowVersion = getWorkflow();
+			expect(() => {
+				validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, [
+					generateCredentialEntity(FIRST_CREDENTIAL_ID),
+				]);
+			}).not.toThrow();
+		});
+
+		it('Should not throw error when saving a workflow removing node without credential access', () => {
+			const newWorkflowVersion = getWorkflow();
+			const previousWorkflowVersion = getWorkflow({ addNodeWithOneCred: true });
+			expect(() => {
+				validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, [
+					generateCredentialEntity(FIRST_CREDENTIAL_ID),
+				]);
+			}).not.toThrow();
+		});
+
+		it('Should save fine when not making changes to workflow without access', () => {
+			const workflowWithOneCredential = getWorkflow({ addNodeWithOneCred: true });
+			expect(() => {
+				validateWorkflowCredentialUsage(workflowWithOneCredential, workflowWithOneCredential, []);
+			}).not.toThrow();
+		});
+
+		it('Should throw error saving a workflow adding node without credential access', () => {
+			const newWorkflowVersion = getWorkflow({
+				addNodeWithOneCred: true,
+				addNodeWithTwoCreds: true,
+			});
+			const previousWorkflowVersion = getWorkflow({ addNodeWithOneCred: true });
+			expect(() => {
+				validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, []);
+			}).toThrow();
+		});
+	});
+});
+
+function generateCredentialEntity(credentialId: string) {
+	const credentialEntity = new CredentialsEntity();
+	credentialEntity.id = parseInt(credentialId, 10);
+	return credentialEntity;
+}
+
+function getWorkflow(options?: {
+	addNodeWithoutCreds?: boolean;
+	addNodeWithOneCred?: boolean;
+	addNodeWithTwoCreds?: boolean;
+}) {
+	const workflow = new WorkflowEntity();
+
+	workflow.nodes = [];
+
+	if (options?.addNodeWithoutCreds) {
+		workflow.nodes.push(nodeWithNoCredentials);
+	}
+
+	if (options?.addNodeWithOneCred) {
+		workflow.nodes.push(nodeWithOneCredential);
+	}
+
+	if (options?.addNodeWithTwoCreds) {
+		workflow.nodes.push(nodeWithTwoCredentials);
+	}
+
+	return workflow;
+}
+
+const nodeWithNoCredentials: INode = {
+	id: NODE_WITH_NO_CRED,
+	name: 'Node with no Credential',
+	typeVersion: 1,
+	type: 'n8n-nodes-base.fakeNode',
+	position: [0, 0],
+	credentials: {},
+	parameters: {},
+};
+
+const nodeWithOneCredential: INode = {
+	id: NODE_WITH_ONE_CRED,
+	name: 'Node with a single credential',
+	typeVersion: 1,
+	type: '',
+	position: [0, 0],
+	credentials: {
+		test: {
+			id: FIRST_CREDENTIAL_ID,
+			name: 'First fake credential',
+		},
+	},
+	parameters: {},
+};
+
+const nodeWithTwoCredentials: INode = {
+	id: NODE_WITH_TWO_CRED,
+	name: 'Node with two credentials',
+	typeVersion: 1,
+	type: '',
+	position: [0, 0],
+	credentials: {
+		mcTest: {
+			id: SECOND_CREDENTIAL_ID,
+			name: 'Second fake credential',
+		},
+		mcTest2: {
+			id: THIRD_CREDENTIAL_ID,
+			name: 'Third fake credential',
+		},
+	},
+	parameters: {},
+};