refactor(core): Split out subworkflow policy check from permission checker (no-changelog) (#10168)

This commit is contained in:
Iván Ovejero 2024-07-24 12:51:01 +02:00 committed by GitHub
parent 7848c19f54
commit f1a3791abc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 284 additions and 289 deletions

View file

@ -1,22 +1,17 @@
import { Service } from 'typedi';
import type { INode, Workflow } from 'n8n-workflow';
import { CredentialAccessError, NodeOperationError, WorkflowOperationError } from 'n8n-workflow';
import type { INode } from 'n8n-workflow';
import { CredentialAccessError, NodeOperationError } from 'n8n-workflow';
import config from '@/config';
import { License } from '@/License';
import { OwnershipService } from '@/services/ownership.service';
import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository';
import { ProjectService } from '@/services/project.service';
import { Logger } from '@/Logger';
@Service()
export class PermissionChecker {
constructor(
private readonly sharedCredentialsRepository: SharedCredentialsRepository,
private readonly ownershipService: OwnershipService,
private readonly license: License,
private readonly projectService: ProjectService,
private readonly logger: Logger,
) {}
/**
@ -50,104 +45,6 @@ export class PermissionChecker {
}
}
async checkSubworkflowExecutePolicy(
subworkflow: Workflow,
parentWorkflowId: string,
node?: INode,
) {
/**
* Important considerations: both the current workflow and the parent can have empty IDs.
* This happens when a user is executing an unsaved workflow manually running a workflow
* loaded from a file or code, for instance.
* This is an important topic to keep in mind for all security checks
*/
if (!subworkflow.id) {
// It's a workflow from code and not loaded from DB
// No checks are necessary since it doesn't have any sort of settings
return;
}
let policy =
subworkflow.settings?.callerPolicy ?? config.getEnv('workflows.callerPolicyDefaultOption');
const isSharingEnabled = this.license.isSharingEnabled();
if (!isSharingEnabled) {
// Community version allows only same owner workflows
policy = 'workflowsFromSameOwner';
}
const parentWorkflowOwner =
await this.ownershipService.getWorkflowProjectCached(parentWorkflowId);
const subworkflowOwner = await this.ownershipService.getWorkflowProjectCached(subworkflow.id);
const description =
subworkflowOwner.id === parentWorkflowOwner.id
? 'Change the settings of the sub-workflow so it can be called by this one.'
: `An admin for the ${subworkflowOwner.name} project can make this change. You may need to tell them the ID of the sub-workflow, which is ${subworkflow.id}`;
const errorToThrow = new WorkflowOperationError(
`Target workflow ID ${subworkflow.id} may not be called`,
node,
description,
);
if (policy === 'none') {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
reason: 'Subworkflow may not be called',
policy,
isSharingEnabled,
});
throw errorToThrow;
}
if (policy === 'workflowsFromAList') {
if (parentWorkflowId === undefined) {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
reason: 'Subworkflow may be called only by workflows from an allowlist',
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
policy,
isSharingEnabled,
});
throw errorToThrow;
}
const allowedCallerIds = subworkflow.settings.callerIds
?.split(',')
.map((id) => id.trim())
.filter((id) => id !== '');
if (!allowedCallerIds?.includes(parentWorkflowId)) {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
reason: 'Subworkflow may be called only by workflows from an allowlist',
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
allowlist: allowedCallerIds,
policy,
isSharingEnabled,
});
throw errorToThrow;
}
}
if (policy === 'workflowsFromSameOwner' && subworkflowOwner?.id !== parentWorkflowOwner.id) {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
reason: 'Subworkflow may be called only by workflows owned by the same project',
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
callerProjectId: parentWorkflowOwner.id,
subworkflowProjectId: subworkflowOwner.id,
policy,
isSharingEnabled,
});
throw errorToThrow;
}
}
private mapCredIdsToNodes(nodes: INode[]) {
return nodes.reduce<{ [credentialId: string]: INode[] }>((map, node) => {
if (node.disabled || !node.credentials) return map;

View file

@ -73,6 +73,7 @@ import { WorkflowExecutionService } from './workflows/workflowExecution.service'
import { MessageEventBus } from '@/eventbus/MessageEventBus/MessageEventBus';
import { EventService } from './eventbus/event.service';
import { GlobalConfig } from '@n8n/config';
import { SubworkflowPolicyChecker } from './subworkflows/subworkflow-policy-checker.service';
export function objectToError(errorObject: unknown, workflow: Workflow): Error {
// TODO: Expand with other error types
@ -826,7 +827,7 @@ async function executeWorkflow(
let data;
try {
await Container.get(PermissionChecker).check(workflowData.id, workflowData.nodes);
await Container.get(PermissionChecker).checkSubworkflowExecutePolicy(
await Container.get(SubworkflowPolicyChecker).check(
workflow,
options.parentWorkflowId,
options.node,

View file

@ -0,0 +1,161 @@
import { v4 as uuid } from 'uuid';
import type { Workflow } from 'n8n-workflow';
import { SubworkflowOperationError } from 'n8n-workflow';
import { Project } from '@/databases/entities/Project';
import { OwnershipService } from '@/services/ownership.service';
import { mockInstance } from '@test/mocking';
import config from '@/config';
import { mock } from 'jest-mock-extended';
import { SubworkflowPolicyChecker } from '../subworkflow-policy-checker.service';
import type { WorkflowEntity } from '@/databases/entities/WorkflowEntity';
import type { License } from '@/License';
const toTargetCallErrorMsg = (subworkflowId: string) =>
`Target workflow ID ${subworkflowId} may not be called`;
const ownershipService = mockInstance(OwnershipService);
const memberPersonalProject = mock<Project>();
describe('SubworkflowPolicyChecker', () => {
const license = mock<License>();
const checker = new SubworkflowPolicyChecker(mock(), license, ownershipService);
beforeEach(() => {
license.isSharingEnabled.mockReturnValue(true);
});
afterEach(() => {
jest.restoreAllMocks();
});
describe('no caller policy', () => {
test('should fall back to N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION', async () => {
config.set('workflows.callerPolicyDefaultOption', 'none');
const parentWorkflow = mock<WorkflowEntity>();
const subworkflow = mock<Workflow>(); // no caller policy
ownershipService.getWorkflowProjectCached.mockResolvedValue(memberPersonalProject);
const check = checker.check(subworkflow, parentWorkflow.id);
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
config.load(config.default);
});
});
describe('overridden caller policy', () => {
test('if no sharing, should override policy to workflows-from-same-owner', async () => {
license.isSharingEnabled.mockReturnValue(false);
const parentWorkflow = mock<WorkflowEntity>();
const subworkflow = mock<Workflow>({ settings: { callerPolicy: 'any' } }); // should be overridden
const firstProject = mock<Project>({ id: uuid() });
const secondProject = mock<Project>({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(firstProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(secondProject); // subworkflow
const check = checker.check(subworkflow, parentWorkflow.id);
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
try {
await checker.check(subworkflow, uuid());
} catch (error) {
if (error instanceof SubworkflowOperationError) {
expect(error.description).toBe(
`An admin for the ${firstProject.name} project can make this change. You may need to tell them the ID of the sub-workflow, which is ${subworkflow.id}`,
);
}
}
});
});
describe('workflows-from-list caller policy', () => {
// xyz
test('should allow if caller list contains parent workflow ID', async () => {
const parentWorkflow = mock<WorkflowEntity>({ id: uuid() });
const subworkflow = mock<Workflow>({
settings: {
callerPolicy: 'workflowsFromAList',
callerIds: `123,456,bcdef, ${parentWorkflow.id}`,
},
});
const parentWorkflowProject = mock<Project>({ id: uuid() });
const subworkflowOwner = mock<Project>({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow
const check = checker.check(subworkflow, parentWorkflow.id);
await expect(check).resolves.not.toThrow();
});
test('should deny if caller list does not contain parent workflow ID', async () => {
const parentWorkflow = mock<WorkflowEntity>();
const subworkflow = mock<Workflow>({
settings: { callerPolicy: 'workflowsFromAList', callerIds: 'xyz' },
});
const check = checker.check(subworkflow, parentWorkflow.id);
await expect(check).rejects.toThrow();
});
});
describe('any caller policy', () => {
test('should not throw', async () => {
const parentWorkflow = mock<WorkflowEntity>({ id: uuid() });
const subworkflow = mock<Workflow>({ settings: { callerPolicy: 'any' } });
const parentWorkflowProject = mock<Project>({ id: uuid() });
const subworkflowOwner = mock<Project>({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(new Project());
const check = checker.check(subworkflow, parentWorkflow.id);
await expect(check).resolves.not.toThrow();
});
});
describe('workflows-from-same-owner caller policy', () => {
test('should deny if the two workflows are owned by different users', async () => {
const parentWorkflowProject = mock<Project>({ id: uuid() });
const subworkflowOwner = mock<Project>({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow
const subworkflow = mock<Workflow>({ settings: { callerPolicy: 'workflowsFromSameOwner' } });
const check = checker.check(subworkflow, uuid());
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
});
test('should allow if both workflows are owned by the same user', async () => {
const parentWorkflow = mock<WorkflowEntity>();
const bothWorkflowsProject = mock<Project>({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // subworkflow
const subworkflow = mock<Workflow>({ settings: { callerPolicy: 'workflowsFromSameOwner' } });
const check = checker.check(subworkflow, parentWorkflow.id);
await expect(check).resolves.not.toThrow();
});
});
});

View file

@ -0,0 +1,110 @@
import { Service } from 'typedi';
import { WorkflowOperationError } from 'n8n-workflow';
import config from '@/config';
import { Logger } from '@/Logger';
import { License } from '@/License';
import { OwnershipService } from '@/services/ownership.service';
import type { Workflow, INode } from 'n8n-workflow';
@Service()
export class SubworkflowPolicyChecker {
constructor(
private readonly logger: Logger,
private readonly license: License,
private readonly ownershipService: OwnershipService,
) {}
async check(subworkflow: Workflow, parentWorkflowId: string, node?: INode) {
/**
* Important considerations: both the current workflow and the parent can have empty IDs.
* This happens when a user is executing an unsaved workflow manually running a workflow
* loaded from a file or code, for instance.
* This is an important topic to keep in mind for all security checks
*/
if (!subworkflow.id) {
// It's a workflow from code and not loaded from DB
// No checks are necessary since it doesn't have any sort of settings
return;
}
let policy =
subworkflow.settings?.callerPolicy ?? config.getEnv('workflows.callerPolicyDefaultOption');
const isSharingEnabled = this.license.isSharingEnabled();
if (!isSharingEnabled) {
// Community version allows only same owner workflows
policy = 'workflowsFromSameOwner';
}
const parentWorkflowOwner =
await this.ownershipService.getWorkflowProjectCached(parentWorkflowId);
const subworkflowOwner = await this.ownershipService.getWorkflowProjectCached(subworkflow.id);
const description =
subworkflowOwner.id === parentWorkflowOwner.id
? 'Change the settings of the sub-workflow so it can be called by this one.'
: `An admin for the ${subworkflowOwner.name} project can make this change. You may need to tell them the ID of the sub-workflow, which is ${subworkflow.id}`;
const errorToThrow = new WorkflowOperationError(
`Target workflow ID ${subworkflow.id} may not be called`,
node,
description,
);
if (policy === 'none') {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
reason: 'Subworkflow may not be called',
policy,
isSharingEnabled,
});
throw errorToThrow;
}
if (policy === 'workflowsFromAList') {
if (parentWorkflowId === undefined) {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
reason: 'Subworkflow may be called only by workflows from an allowlist',
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
policy,
isSharingEnabled,
});
throw errorToThrow;
}
const allowedCallerIds = subworkflow.settings.callerIds
?.split(',')
.map((id) => id.trim())
.filter((id) => id !== '');
if (!allowedCallerIds?.includes(parentWorkflowId)) {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
reason: 'Subworkflow may be called only by workflows from an allowlist',
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
allowlist: allowedCallerIds,
policy,
isSharingEnabled,
});
throw errorToThrow;
}
}
if (policy === 'workflowsFromSameOwner' && subworkflowOwner?.id !== parentWorkflowOwner.id) {
this.logger.warn('[PermissionChecker] Subworkflow execution denied', {
reason: 'Subworkflow may be called only by workflows owned by the same project',
callerWorkflowId: parentWorkflowId,
subworkflowId: subworkflow.id,
callerProjectId: parentWorkflowOwner.id,
subworkflowProjectId: subworkflowOwner.id,
policy,
isSharingEnabled,
});
throw errorToThrow;
}
}
}

View file

@ -32,9 +32,9 @@ import { WorkflowRunner } from '@/WorkflowRunner';
import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData';
import { TestWebhooks } from '@/TestWebhooks';
import { Logger } from '@/Logger';
import { PermissionChecker } from '@/UserManagement/PermissionChecker';
import type { Project } from '@/databases/entities/Project';
import { GlobalConfig } from '@n8n/config';
import { SubworkflowPolicyChecker } from '@/subworkflows/subworkflow-policy-checker.service';
@Service()
export class WorkflowExecutionService {
@ -44,9 +44,9 @@ export class WorkflowExecutionService {
private readonly workflowRepository: WorkflowRepository,
private readonly nodeTypes: NodeTypes,
private readonly testWebhooks: TestWebhooks,
private readonly permissionChecker: PermissionChecker,
private readonly workflowRunner: WorkflowRunner,
private readonly globalConfig: GlobalConfig,
private readonly subworkflowPolicyChecker: SubworkflowPolicyChecker,
) {}
async runWorkflow(
@ -188,7 +188,7 @@ export class WorkflowExecutionService {
const failedNode = workflowErrorData.execution?.lastNodeExecuted
? workflowInstance.getNode(workflowErrorData.execution?.lastNodeExecuted)
: undefined;
await this.permissionChecker.checkSubworkflowExecutePolicy(
await this.subworkflowPolicyChecker.check(
workflowInstance,
workflowErrorData.workflow.id!,
failedNode ?? undefined,

View file

@ -1,22 +1,17 @@
import { v4 as uuid } from 'uuid';
import { Container } from 'typedi';
import type { INode, WorkflowSettings } from 'n8n-workflow';
import { SubworkflowOperationError, Workflow, randomInt } from 'n8n-workflow';
import config from '@/config';
import type { INode } from 'n8n-workflow';
import { randomInt } from 'n8n-workflow';
import type { User } from '@db/entities/User';
import { WorkflowRepository } from '@db/repositories/workflow.repository';
import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository';
import { generateNanoId } from '@/databases/utils/generators';
import { License } from '@/License';
import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials';
import { NodeTypes } from '@/NodeTypes';
import { OwnershipService } from '@/services/ownership.service';
import { PermissionChecker } from '@/UserManagement/PermissionChecker';
import { mockInstance } from '../shared/mocking';
import { randomCredentialPayload as randomCred, randomName } from '../integration/shared/random';
import { LicenseMocker } from '../integration/shared/license';
import { randomCredentialPayload as randomCred } from '../integration/shared/random';
import * as testDb from '../integration/shared/testDb';
import type { SaveCredentialFunction } from '../integration/shared/types';
import { mockNodeTypesData } from '../unit/Helpers';
@ -25,50 +20,9 @@ import { createOwner, createUser } from '../integration/shared/db/users';
import { SharedCredentialsRepository } from '@/databases/repositories/sharedCredentials.repository';
import { getPersonalProject } from './shared/db/projects';
import type { WorkflowEntity } from '@/databases/entities/WorkflowEntity';
import { Project } from '@/databases/entities/Project';
import type { Project } from '@/databases/entities/Project';
import { ProjectRepository } from '@/databases/repositories/project.repository';
export const toTargetCallErrorMsg = (subworkflowId: string) =>
`Target workflow ID ${subworkflowId} may not be called`;
export function createParentWorkflow() {
return Container.get(WorkflowRepository).create({
id: generateNanoId(),
name: randomName(),
active: false,
connections: {},
nodes: [
{
name: '',
typeVersion: 1,
type: 'n8n-nodes-base.executeWorkflow',
position: [0, 0],
parameters: {},
},
],
});
}
export function createSubworkflow({
policy,
callerIds,
}: {
policy?: WorkflowSettings.CallerPolicy;
callerIds?: string;
} = {}) {
return new Workflow({
id: uuid(),
nodes: [],
connections: {},
active: false,
nodeTypes: mockNodeTypes,
settings: {
...(policy ? { callerPolicy: policy } : {}),
...(callerIds ? { callerIds } : {}),
},
});
}
const ownershipService = mockInstance(OwnershipService);
const createWorkflow = async (nodes: INode[], workflowOwner?: User): Promise<WorkflowEntity> => {
@ -289,132 +243,3 @@ describe('check()', () => {
).resolves.not.toThrow();
});
});
describe('checkSubworkflowExecutePolicy()', () => {
let license: LicenseMocker;
beforeAll(() => {
license = new LicenseMocker();
license.mock(Container.get(License));
license.enable('feat:sharing');
});
describe('no caller policy', () => {
test('should fall back to N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION', async () => {
config.set('workflows.callerPolicyDefaultOption', 'none');
const parentWorkflow = createParentWorkflow();
const subworkflow = createSubworkflow(); // no caller policy
ownershipService.getWorkflowProjectCached.mockResolvedValue(memberPersonalProject);
const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id);
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
config.load(config.default);
});
});
describe('overridden caller policy', () => {
test('if no sharing, should override policy to workflows-from-same-owner', async () => {
license.disable('feat:sharing');
const parentWorkflow = createParentWorkflow();
const subworkflow = createSubworkflow({ policy: 'any' }); // should be overridden
const firstProject = Container.get(ProjectRepository).create({ id: uuid() });
const secondProject = Container.get(ProjectRepository).create({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(firstProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(secondProject); // subworkflow
const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id);
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
try {
await permissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid());
} catch (error) {
if (error instanceof SubworkflowOperationError) {
expect(error.description).toBe(
`An admin for the ${firstProject.name} project can make this change. You may need to tell them the ID of the sub-workflow, which is ${subworkflow.id}`,
);
}
}
license.enable('feat:sharing');
});
});
describe('workflows-from-list caller policy', () => {
test('should allow if caller list contains parent workflow ID', async () => {
const parentWorkflow = createParentWorkflow();
const subworkflow = createSubworkflow({
policy: 'workflowsFromAList',
callerIds: `123,456,bcdef, ${parentWorkflow.id}`,
});
const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id);
await expect(check).resolves.not.toThrow();
});
test('should deny if caller list does not contain parent workflow ID', async () => {
const parentWorkflow = createParentWorkflow();
const subworkflow = createSubworkflow({
policy: 'workflowsFromAList',
callerIds: 'xyz',
});
const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id);
await expect(check).rejects.toThrow();
});
});
describe('any caller policy', () => {
test('should not throw', async () => {
const parentWorkflow = createParentWorkflow();
const subworkflow = createSubworkflow({ policy: 'any' });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(new Project());
const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id);
await expect(check).resolves.not.toThrow();
});
});
describe('workflows-from-same-owner caller policy', () => {
test('should deny if the two workflows are owned by different users', async () => {
const parentWorkflowProject = Container.get(ProjectRepository).create({ id: uuid() });
const subworkflowOwner = Container.get(ProjectRepository).create({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow
const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' });
const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid());
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
});
test('should allow if both workflows are owned by the same user', async () => {
const parentWorkflow = createParentWorkflow();
const bothWorkflowsProject = Container.get(ProjectRepository).create({ id: uuid() });
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // parent workflow
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // subworkflow
const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' });
const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id);
await expect(check).resolves.not.toThrow();
});
});
});

View file

@ -57,8 +57,9 @@ describe('WorkflowExecutionService', () => {
mock(),
mock(),
mock(),
mock(),
workflowRunner,
mock(),
mock(),
);
describe('runWorkflow()', () => {