1
1
mirror of https://github.com/n8n-io/n8n.git synced 2024-09-11 13:15:28 +03:00

feat(core): Move execution permission checks earlier in the lifecycle (#8677)

This commit is contained in:
कारतोफ्फेलस्क्रिप्ट™ 2024-02-21 14:47:02 +01:00 committed by GitHub
parent a573146135
commit 059d281fd1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 139 additions and 201 deletions

View File

@ -22,10 +22,10 @@ export class PermissionChecker {
/**
* Check if a user is permitted to execute a workflow.
*/
async check(workflow: Workflow, userId: string) {
async check(workflowId: string, userId: string, nodes: INode[]) {
// allow if no nodes in this workflow use creds
const credIdsToNodes = this.mapCredIdsToNodes(workflow);
const credIdsToNodes = this.mapCredIdsToNodes(nodes);
const workflowCredIds = Object.keys(credIdsToNodes);
@ -46,8 +46,8 @@ export class PermissionChecker {
let workflowUserIds = [userId];
if (workflow.id && isSharingEnabled) {
workflowUserIds = await this.sharedWorkflowRepository.getSharedUserIds(workflow.id);
if (workflowId && isSharingEnabled) {
workflowUserIds = await this.sharedWorkflowRepository.getSharedUserIds(workflowId);
}
const accessibleCredIds = isSharingEnabled
@ -62,7 +62,7 @@ export class PermissionChecker {
const inaccessibleCredId = inaccessibleCredIds[0];
const nodeToFlag = credIdsToNodes[inaccessibleCredId][0];
throw new CredentialAccessError(nodeToFlag, inaccessibleCredId, workflow);
throw new CredentialAccessError(nodeToFlag, inaccessibleCredId, workflowId);
}
async checkSubworkflowExecutePolicy(
@ -129,25 +129,22 @@ export class PermissionChecker {
}
}
private mapCredIdsToNodes(workflow: Workflow) {
return Object.values(workflow.nodes).reduce<{ [credentialId: string]: INode[] }>(
(map, node) => {
if (node.disabled || !node.credentials) return map;
private mapCredIdsToNodes(nodes: INode[]) {
return nodes.reduce<{ [credentialId: string]: INode[] }>((map, node) => {
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',
});
}
Object.values(node.credentials).forEach((cred) => {
if (!cred.id) {
throw new NodeOperationError(node, 'Node uses invalid credential', {
description: 'Please recreate the credential.',
level: 'warning',
});
}
map[cred.id] = map[cred.id] ? [...map[cred.id], node] : [node];
});
map[cred.id] = map[cred.id] ? [...map[cred.id], node] : [node];
});
return map;
},
{},
);
return map;
}, {});
}
}

View File

@ -389,7 +389,7 @@ export function hookFunctionsPreExecute(): IWorkflowExecuteHooks {
* Returns hook functions to save workflow execution and call error workflow
*
*/
function hookFunctionsSave(parentProcessMode?: string): IWorkflowExecuteHooks {
function hookFunctionsSave(): IWorkflowExecuteHooks {
const logger = Container.get(Logger);
const internalHooks = Container.get(InternalHooks);
const eventsService = Container.get(EventsService);
@ -418,7 +418,7 @@ function hookFunctionsSave(parentProcessMode?: string): IWorkflowExecuteHooks {
await restoreBinaryDataId(fullRunData, this.executionId, this.mode);
const isManualMode = [this.mode, parentProcessMode].includes('manual');
const isManualMode = this.mode === 'manual';
try {
if (!isManualMode && isWorkflowIdValid(this.workflowData.id) && newStaticData) {
@ -795,7 +795,11 @@ async function executeWorkflow(
let data;
try {
await Container.get(PermissionChecker).check(workflow, additionalData.userId);
await Container.get(PermissionChecker).check(
workflowData.id,
additionalData.userId,
workflowData.nodes,
);
await Container.get(PermissionChecker).checkSubworkflowExecutePolicy(
workflow,
options.parentWorkflowId,
@ -809,7 +813,6 @@ async function executeWorkflow(
runData.executionMode,
executionId,
workflowData,
{ parentProcessMode: additionalData.hooks!.mode },
);
additionalDataIntegrated.executionId = executionId;
@ -1011,10 +1014,8 @@ function getWorkflowHooksIntegrated(
mode: WorkflowExecuteMode,
executionId: string,
workflowData: IWorkflowBase,
optionalParameters?: IWorkflowHooksOptionalParameters,
): WorkflowHooks {
optionalParameters = optionalParameters || {};
const hookFunctions = hookFunctionsSave(optionalParameters.parentProcessMode);
const hookFunctions = hookFunctionsSave();
const preExecuteFunctions = hookFunctionsPreExecute();
for (const key of Object.keys(preExecuteFunctions)) {
if (hookFunctions[key] === undefined) {
@ -1022,7 +1023,7 @@ function getWorkflowHooksIntegrated(
}
hookFunctions[key]!.push.apply(hookFunctions[key], preExecuteFunctions[key]);
}
return new WorkflowHooks(hookFunctions, mode, executionId, workflowData, optionalParameters);
return new WorkflowHooks(hookFunctions, mode, executionId, workflowData);
}
/**
@ -1064,7 +1065,7 @@ export function getWorkflowHooksWorkerMain(
// TODO: simplifying this for now to just leave the bare minimum hooks
// const hookFunctions = hookFunctionsPush();
// const preExecuteFunctions = hookFunctionsPreExecute(optionalParameters.parentProcessMode);
// const preExecuteFunctions = hookFunctionsPreExecute();
// for (const key of Object.keys(preExecuteFunctions)) {
// if (hookFunctions[key] === undefined) {
// hookFunctions[key] = [];
@ -1105,7 +1106,6 @@ export function getWorkflowHooksWorkerMain(
export function getWorkflowHooksMain(
data: IWorkflowExecutionDataProcess,
executionId: string,
isMainProcess = false,
): WorkflowHooks {
const hookFunctions = hookFunctionsSave();
const pushFunctions = hookFunctionsPush();
@ -1116,14 +1116,12 @@ export function getWorkflowHooksMain(
hookFunctions[key]!.push.apply(hookFunctions[key], pushFunctions[key]);
}
if (isMainProcess) {
const preExecuteFunctions = hookFunctionsPreExecute();
for (const key of Object.keys(preExecuteFunctions)) {
if (hookFunctions[key] === undefined) {
hookFunctions[key] = [];
}
hookFunctions[key]!.push.apply(hookFunctions[key], preExecuteFunctions[key]);
const preExecuteFunctions = hookFunctionsPreExecute();
for (const key of Object.keys(preExecuteFunctions)) {
if (hookFunctions[key] === undefined) {
hookFunctions[key] = [];
}
hookFunctions[key]!.push.apply(hookFunctions[key], preExecuteFunctions[key]);
}
if (!hookFunctions.nodeExecuteBefore) hookFunctions.nodeExecuteBefore = [];

View File

@ -158,6 +158,21 @@ export class WorkflowRunner {
): Promise<string> {
// Register a new execution
const executionId = await this.activeExecutions.add(data, restartExecutionId);
const { id: workflowId, nodes } = data.workflowData;
try {
await this.permissionChecker.check(workflowId, data.userId, nodes);
} catch (error) {
// Create a failed execution with the data for the node, save it and abort execution
const runData = generateFailedExecutionFromError(data.executionMode, error, error.node);
const workflowHooks = WorkflowExecuteAdditionalData.getWorkflowHooksMain(data, executionId);
await workflowHooks.executeHookFunctions('workflowExecuteBefore', []);
await workflowHooks.executeHookFunctions('workflowExecuteAfter', [runData]);
responsePromise?.reject(error);
this.activeExecutions.remove(executionId);
return executionId;
}
if (responsePromise) {
this.activeExecutions.attachResponsePromise(executionId, responsePromise);
}
@ -267,27 +282,7 @@ export class WorkflowRunner {
await this.executionRepository.updateStatus(executionId, 'running');
try {
additionalData.hooks = WorkflowExecuteAdditionalData.getWorkflowHooksMain(
data,
executionId,
true,
);
try {
await this.permissionChecker.check(workflow, data.userId);
} catch (error) {
ErrorReporter.error(error);
// Create a failed execution with the data for the node
// save it and abort execution
const failedExecution = generateFailedExecutionFromError(
data.executionMode,
error,
error.node,
);
await additionalData.hooks.executeHookFunctions('workflowExecuteAfter', [failedExecution]);
this.activeExecutions.remove(executionId, failedExecution);
return;
}
additionalData.hooks = WorkflowExecuteAdditionalData.getWorkflowHooksMain(data, executionId);
additionalData.hooks.hookFunctions.sendResponse = [
async (response: IExecuteResponsePromiseData): Promise<void> => {

View File

@ -4,24 +4,16 @@ import express from 'express';
import http from 'http';
import type PCancelable from 'p-cancelable';
import { WorkflowExecute } from 'n8n-core';
import type {
ExecutionError,
ExecutionStatus,
IExecuteResponsePromiseData,
INodeTypes,
IRun,
} from 'n8n-workflow';
import { Workflow, NodeOperationError, sleep, ApplicationError } from 'n8n-workflow';
import type { ExecutionStatus, IExecuteResponsePromiseData, INodeTypes, IRun } from 'n8n-workflow';
import { Workflow, sleep, ApplicationError } from 'n8n-workflow';
import * as Db from '@/Db';
import * as ResponseHelper from '@/ResponseHelper';
import * as WebhookHelpers from '@/WebhookHelpers';
import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData';
import { PermissionChecker } from '@/UserManagement/PermissionChecker';
import config from '@/config';
import type { Job, JobId, JobResponse, WebhookResponse } from '@/Queue';
import { Queue } from '@/Queue';
import { generateFailedExecutionFromError } from '@/WorkflowHelpers';
import { N8N_VERSION } from '@/constants';
import { ExecutionRepository } from '@db/repositories/execution.repository';
import { WorkflowRepository } from '@db/repositories/workflow.repository';
@ -180,20 +172,6 @@ export class Worker extends BaseCommand {
},
);
try {
await Container.get(PermissionChecker).check(workflow, workflowOwner.id);
} catch (error) {
if (error instanceof NodeOperationError) {
const failedExecution = generateFailedExecutionFromError(
fullExecutionData.mode,
error,
error.node,
);
await additionalData.hooks.executeHookFunctions('workflowExecuteAfter', [failedExecution]);
}
return { success: true, error: error as ExecutionError };
}
additionalData.hooks.hookFunctions.sendResponse = [
async (response: IExecuteResponsePromiseData): Promise<void> => {
const progress: WebhookResponse = {

View File

@ -1,6 +1,6 @@
import { v4 as uuid } from 'uuid';
import { Container } from 'typedi';
import type { WorkflowSettings } from 'n8n-workflow';
import type { INode, WorkflowSettings } from 'n8n-workflow';
import { SubworkflowOperationError, Workflow } from 'n8n-workflow';
import config from '@/config';
@ -87,6 +87,8 @@ beforeAll(async () => {
});
describe('check()', () => {
const workflowId = randomPositiveDigit().toString();
beforeEach(async () => {
await testDb.truncate(['Workflow', 'Credentials']);
});
@ -97,56 +99,40 @@ describe('check()', () => {
test('should allow if workflow has no creds', async () => {
const userId = uuid();
const nodes: INode[] = [
{
id: uuid(),
name: 'Start',
type: 'n8n-nodes-base.start',
typeVersion: 1,
parameters: {},
position: [0, 0],
},
];
const workflow = new Workflow({
id: randomPositiveDigit().toString(),
name: 'test',
active: false,
connections: {},
nodeTypes: mockNodeTypes,
nodes: [
{
id: uuid(),
name: 'Start',
type: 'n8n-nodes-base.start',
typeVersion: 1,
parameters: {},
position: [0, 0],
},
],
});
expect(async () => await permissionChecker.check(workflow, userId)).not.toThrow();
expect(async () => await permissionChecker.check(workflowId, userId, nodes)).not.toThrow();
});
test('should allow if requesting user is instance owner', async () => {
const owner = await createOwner();
const workflow = new Workflow({
id: randomPositiveDigit().toString(),
name: 'test',
active: false,
connections: {},
nodeTypes: mockNodeTypes,
nodes: [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: randomPositiveDigit().toString(),
name: 'Action Network Account',
},
const nodes: INode[] = [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: randomPositiveDigit().toString(),
name: 'Action Network Account',
},
},
],
});
},
];
expect(async () => await permissionChecker.check(workflow, owner.id)).not.toThrow();
expect(async () => await permissionChecker.check(workflowId, owner.id, nodes)).not.toThrow();
});
test('should allow if workflow creds are valid subset', async () => {
@ -154,46 +140,38 @@ describe('check()', () => {
const ownerCred = await saveCredential(randomCred(), { user: owner });
const memberCred = await saveCredential(randomCred(), { user: member });
const workflow = new Workflow({
id: randomPositiveDigit().toString(),
name: 'test',
active: false,
connections: {},
nodeTypes: mockNodeTypes,
nodes: [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: ownerCred.id,
name: ownerCred.name,
},
const nodes: INode[] = [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: ownerCred.id,
name: ownerCred.name,
},
},
{
id: uuid(),
name: 'Action Network 2',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: memberCred.id,
name: memberCred.name,
},
},
{
id: uuid(),
name: 'Action Network 2',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: memberCred.id,
name: memberCred.name,
},
},
],
});
},
];
expect(async () => await permissionChecker.check(workflow, owner.id)).not.toThrow();
expect(async () => await permissionChecker.check(workflowId, owner.id, nodes)).not.toThrow();
});
test('should deny if workflow creds are not valid subset', async () => {
@ -247,9 +225,9 @@ describe('check()', () => {
role: 'workflow:owner',
});
const workflow = new Workflow(workflowDetails);
await expect(permissionChecker.check(workflow, member.id)).rejects.toThrow();
await expect(
permissionChecker.check(workflowDetails.id, member.id, workflowDetails.nodes),
).rejects.toThrow();
});
});

View File

@ -1,4 +1,4 @@
import { type INodeTypes, Workflow } from 'n8n-workflow';
import type { INode } from 'n8n-workflow';
import { mock } from 'jest-mock-extended';
import type { User } from '@db/entities/User';
import type { UserRepository } from '@db/repositories/user.repository';
@ -21,36 +21,30 @@ describe('PermissionChecker', () => {
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',
},
const workflowId = '1';
const nodes: INode[] = [
{
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();
await expect(permissionChecker.check(workflowId, '123', nodes)).rejects.toThrow();
expect(license.isSharingEnabled).not.toHaveBeenCalled();
expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled();
expect(sharedCredentialsRepo.getOwnedCredentialIds).not.toHaveBeenCalled();
@ -60,7 +54,7 @@ describe('PermissionChecker', () => {
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();
await expect(permissionChecker.check(workflowId, user.id, nodes)).resolves.not.toThrow();
expect(license.isSharingEnabled).not.toHaveBeenCalled();
expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled();
expect(sharedCredentialsRepo.getOwnedCredentialIds).not.toHaveBeenCalled();
@ -77,7 +71,7 @@ describe('PermissionChecker', () => {
it('should validate credential access using only owned credentials', async () => {
sharedCredentialsRepo.getOwnedCredentialIds.mockResolvedValue(['cred-id']);
await expect(permissionChecker.check(workflow, user.id)).resolves.not.toThrow();
await expect(permissionChecker.check(workflowId, user.id, nodes)).resolves.not.toThrow();
expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled();
expect(sharedCredentialsRepo.getOwnedCredentialIds).toBeCalledWith([user.id]);
@ -87,7 +81,7 @@ describe('PermissionChecker', () => {
it('should throw when the user does not have access to the credential', async () => {
sharedCredentialsRepo.getOwnedCredentialIds.mockResolvedValue(['cred-id2']);
await expect(permissionChecker.check(workflow, user.id)).rejects.toThrow(
await expect(permissionChecker.check(workflowId, user.id, nodes)).rejects.toThrow(
'Node has no access to credential',
);
@ -108,9 +102,9 @@ describe('PermissionChecker', () => {
it('should validate credential access using only owned credentials', async () => {
sharedCredentialsRepo.getAccessibleCredentialIds.mockResolvedValue(['cred-id']);
await expect(permissionChecker.check(workflow, user.id)).resolves.not.toThrow();
await expect(permissionChecker.check(workflowId, user.id, nodes)).resolves.not.toThrow();
expect(sharedWorkflowRepo.getSharedUserIds).toBeCalledWith(workflow.id);
expect(sharedWorkflowRepo.getSharedUserIds).toBeCalledWith(workflowId);
expect(sharedCredentialsRepo.getAccessibleCredentialIds).toBeCalledWith([
user.id,
'another-user',
@ -121,7 +115,7 @@ describe('PermissionChecker', () => {
it('should throw when the user does not have access to the credential', async () => {
sharedCredentialsRepo.getAccessibleCredentialIds.mockResolvedValue(['cred-id2']);
await expect(permissionChecker.check(workflow, user.id)).rejects.toThrow(
await expect(permissionChecker.check(workflowId, user.id, nodes)).rejects.toThrow(
'Node has no access to credential',
);

View File

@ -264,7 +264,8 @@ export const pushConnection = defineComponent({
pushData = receivedData.data as IPushDataExecutionFinished;
}
if (this.workflowsStore.activeExecutionId === pushData.executionId) {
const { activeExecutionId } = this.workflowsStore;
if (activeExecutionId === pushData.executionId) {
const activeRunData =
this.workflowsStore.workflowExecutionData?.data?.resultData?.runData;
if (activeRunData) {
@ -285,7 +286,6 @@ export const pushConnection = defineComponent({
return false;
}
const { activeExecutionId } = this.workflowsStore;
if (activeExecutionId !== pushData.executionId) {
// The workflow which did finish execution did either not get started
// by this session or we do not have the execution id yet.
@ -318,7 +318,6 @@ export const pushConnection = defineComponent({
const workflow = this.workflowHelpers.getCurrentWorkflow();
if (runDataExecuted.waitTill !== undefined) {
const activeExecutionId = this.workflowsStore.activeExecutionId;
const workflowSettings = this.workflowsStore.workflowSettings;
const saveManualExecutions = this.rootStore.saveManualExecutions;

View File

@ -1191,6 +1191,7 @@ export const useWorkflowsStore = defineStore(STORES.WORKFLOWS, {
return;
}
this.activeExecutions.unshift(newActiveExecution);
this.activeExecutionId = newActiveExecution.id;
},
finishActiveExecution(
finishedActiveExecution: IPushDataExecutionFinished | IPushDataUnsavedExecutionFinished,

View File

@ -2083,7 +2083,6 @@ export type WorkflowActivateMode =
| 'leadershipChange';
export interface IWorkflowHooksOptionalParameters {
parentProcessMode?: string;
retryOf?: string;
sessionId?: string;
}

View File

@ -10,16 +10,15 @@ export class CredentialAccessError extends ExecutionBaseError {
constructor(
readonly node: INode,
credentialId: string,
workflow: { id: string; name?: string },
workflowId: string,
) {
super('Node has no access to credential', {
tags: {
nodeType: node.type,
},
extra: {
workflowId: workflow.id,
workflowName: workflow.name ?? '',
credentialId,
workflowId,
},
});
}