Skip to content

Commit

Permalink
feat(core): Move execution permission checks earlier in the lifecycle (…
Browse files Browse the repository at this point in the history 
…#8677)
  • Loading branch information
@netroy
netroy authored Feb 21, 2024
1 parent a573146 commit 059d281
Show file tree
Hide file tree
Showing 10 changed files with 142 additions and 204 deletions.
47 changes: 22 additions & 25 deletions packages/cli/src/UserManagement/PermissionChecker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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;

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

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',
});
}

map[cred.id] = map[cred.id] ? [...map[cred.id], node] : [node];
});

return map;
}, {});
}
}
32 changes: 15 additions & 17 deletions packages/cli/src/WorkflowExecuteAdditionalData.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -809,7 +813,6 @@ async function executeWorkflow(
runData.executionMode,
executionId,
workflowData,
{ parentProcessMode: additionalData.hooks!.mode },
);
additionalDataIntegrated.executionId = executionId;

Expand Down Expand Up @@ -1011,18 +1014,16 @@ 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) {
hookFunctions[key] = [];
}
hookFunctions[key]!.push.apply(hookFunctions[key], preExecuteFunctions[key]);
}
return new WorkflowHooks(hookFunctions, mode, executionId, workflowData, optionalParameters);
return new WorkflowHooks(hookFunctions, mode, executionId, workflowData);
}

/**
Expand Down Expand Up @@ -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] = [];
Expand Down Expand Up @@ -1105,7 +1106,6 @@ export function getWorkflowHooksWorkerMain(
export function getWorkflowHooksMain(
data: IWorkflowExecutionDataProcess,
executionId: string,
isMainProcess = false,
): WorkflowHooks {
const hookFunctions = hookFunctionsSave();
const pushFunctions = hookFunctionsPush();
Expand All @@ -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 = [];
Expand Down
37 changes: 16 additions & 21 deletions packages/cli/src/WorkflowRunner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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> => {
Expand Down
26 changes: 2 additions & 24 deletions packages/cli/src/commands/worker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 = {
Expand Down
Loading

0 comments on commit 059d281

Please sign in to comment.