fix(core): Fix user comparison in same-user subworkflow caller policy (#7913)

https://linear.app/n8n/issue/PAY-992


https://community.n8n.io/t/executing-workflow-using-owner-role-created-by-another-user-fails/33443

---------

Co-authored-by: Omar Ajoue <krynble@gmail.com>
This commit is contained in:
Iván Ovejero 2023-12-06 13:27:11 +01:00 committed by GitHub
parent f5502cc628
commit 92bab72cff
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 133 additions and 28 deletions

View file

@ -1,5 +1,5 @@
import type { INode, Workflow } from 'n8n-workflow'; import type { INode, Workflow } from 'n8n-workflow';
import { NodeOperationError, SubworkflowOperationError } from 'n8n-workflow'; import { NodeOperationError, WorkflowOperationError } from 'n8n-workflow';
import type { FindOptionsWhere } from 'typeorm'; import type { FindOptionsWhere } from 'typeorm';
import { In } from 'typeorm'; import { In } from 'typeorm';
import config from '@/config'; import config from '@/config';
@ -78,8 +78,8 @@ export class PermissionChecker {
static async checkSubworkflowExecutePolicy( static async checkSubworkflowExecutePolicy(
subworkflow: Workflow, subworkflow: Workflow,
userId: string, parentWorkflowId: string,
parentWorkflowId?: string, node?: INode,
) { ) {
/** /**
* Important considerations: both the current workflow and the parent can have empty IDs. * Important considerations: both the current workflow and the parent can have empty IDs.
@ -101,15 +101,22 @@ export class PermissionChecker {
policy = 'workflowsFromSameOwner'; policy = 'workflowsFromSameOwner';
} }
const parentWorkflowOwner =
await Container.get(OwnershipService).getWorkflowOwnerCached(parentWorkflowId);
const subworkflowOwner = await Container.get(OwnershipService).getWorkflowOwnerCached( const subworkflowOwner = await Container.get(OwnershipService).getWorkflowOwnerCached(
subworkflow.id, subworkflow.id,
); );
const errorToThrow = new SubworkflowOperationError( const description =
`Target workflow ID ${subworkflow.id ?? ''} may not be called`, subworkflowOwner.id === parentWorkflowOwner.id
subworkflowOwner.id === userId
? 'Change the settings of the sub-workflow so it can be called by this one.' ? 'Change the settings of the sub-workflow so it can be called by this one.'
: `${subworkflowOwner.firstName} (${subworkflowOwner.email}) can make this change. You may need to tell them the ID of this workflow, which is ${subworkflow.id}`, : `${subworkflowOwner.firstName} (${subworkflowOwner.email}) 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') { if (policy === 'none') {
@ -130,10 +137,8 @@ export class PermissionChecker {
} }
} }
if (policy === 'workflowsFromSameOwner') { if (policy === 'workflowsFromSameOwner' && subworkflowOwner?.id !== parentWorkflowOwner.id) {
if (subworkflowOwner?.id !== userId) { throw errorToThrow;
throw errorToThrow;
}
} }
} }

View file

@ -93,6 +93,12 @@ export function objectToError(errorObject: unknown, workflow: Workflow): Error {
error = new Error(errorObject.message as string); error = new Error(errorObject.message as string);
} }
if ('description' in errorObject) {
// @ts-expect-error Error descriptions are surfaced by the UI but
// not all backend errors account for this property yet.
error.description = errorObject.description as string;
}
if ('stack' in errorObject) { if ('stack' in errorObject) {
// If there's a 'stack' property, set it on the new Error instance. // If there's a 'stack' property, set it on the new Error instance.
error.stack = errorObject.stack as string; error.stack = errorObject.stack as string;
@ -724,6 +730,7 @@ async function executeWorkflow(
workflowInfo: IExecuteWorkflowInfo, workflowInfo: IExecuteWorkflowInfo,
additionalData: IWorkflowExecuteAdditionalData, additionalData: IWorkflowExecuteAdditionalData,
options: { options: {
node?: INode;
parentWorkflowId?: string; parentWorkflowId?: string;
inputData?: INodeExecutionData[]; inputData?: INodeExecutionData[];
parentExecutionId?: string; parentExecutionId?: string;
@ -777,8 +784,8 @@ async function executeWorkflow(
await PermissionChecker.check(workflow, additionalData.userId); await PermissionChecker.check(workflow, additionalData.userId);
await PermissionChecker.checkSubworkflowExecutePolicy( await PermissionChecker.checkSubworkflowExecutePolicy(
workflow, workflow,
additionalData.userId, options.parentWorkflowId!,
options.parentWorkflowId, options.node,
); );
// Create new additionalData to have different workflow loaded and to call // Create new additionalData to have different workflow loaded and to call

View file

@ -173,10 +173,13 @@ export async function executeErrorWorkflow(
}); });
try { try {
const failedNode = workflowErrorData.execution?.lastNodeExecuted
? workflowInstance.getNode(workflowErrorData.execution?.lastNodeExecuted)
: undefined;
await PermissionChecker.checkSubworkflowExecutePolicy( await PermissionChecker.checkSubworkflowExecutePolicy(
workflowInstance, workflowInstance,
runningUser.id, workflowErrorData.workflow.id!,
workflowErrorData.workflow.id, failedNode ?? undefined,
); );
} catch (error) { } catch (error) {
const initialNode = workflowInstance.getStartNode(); const initialNode = workflowInstance.getStartNode();

View file

@ -1,6 +1,6 @@
import { v4 as uuid } from 'uuid'; import { v4 as uuid } from 'uuid';
import { Container } from 'typedi'; import { Container } from 'typedi';
import type { INodeTypes } from 'n8n-workflow'; import type { INodeTypes, WorkflowSettings } from 'n8n-workflow';
import { SubworkflowOperationError, Workflow } from 'n8n-workflow'; import { SubworkflowOperationError, Workflow } from 'n8n-workflow';
import config from '@/config'; import config from '@/config';
@ -16,6 +16,7 @@ import { OwnershipService } from '@/services/ownership.service';
import { mockInstance } from '../shared/mocking'; import { mockInstance } from '../shared/mocking';
import { import {
randomCredentialPayload as randomCred, randomCredentialPayload as randomCred,
randomName,
randomPositiveDigit, randomPositiveDigit,
} from '../integration/shared/random'; } from '../integration/shared/random';
import * as testDb from '../integration/shared/testDb'; import * as testDb from '../integration/shared/testDb';
@ -26,6 +27,24 @@ import { getCredentialOwnerRole, getWorkflowOwnerRole } from '../integration/sha
import { createOwner, createUser } from '../integration/shared/db/users'; import { createOwner, createUser } from '../integration/shared/db/users';
import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { WorkflowRepository } from '@db/repositories/workflow.repository';
import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository';
import { UserRepository } from '@/databases/repositories/user.repository';
import { LicenseMocker } from '../integration/shared/license';
import { License } from '@/License';
import { generateNanoId } from '@/databases/utils/generators';
import type { WorkflowEntity } from '@/databases/entities/WorkflowEntity';
function createSubworkflow({ policy }: { policy: WorkflowSettings.CallerPolicy }) {
return new Workflow({
id: uuid(),
nodes: [],
connections: {},
active: false,
nodeTypes: mockNodeTypes,
settings: {
callerPolicy: policy,
},
});
}
let mockNodeTypes: INodeTypes; let mockNodeTypes: INodeTypes;
let credentialOwnerRole: Role; let credentialOwnerRole: Role;
@ -230,7 +249,29 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
const nonOwnerUser = new User(); const nonOwnerUser = new User();
nonOwnerUser.id = uuid(); nonOwnerUser.id = uuid();
let parentWorkflow: WorkflowEntity;
beforeAll(() => {
parentWorkflow = Container.get(WorkflowRepository).create({
id: generateNanoId(),
name: randomName(),
active: false,
connections: {},
nodes: [
{
name: '',
typeVersion: 1,
type: 'n8n-nodes-base.executeWorkflow',
position: [0, 0],
parameters: {},
},
],
});
});
test('sets default policy from environment when subworkflow has none', async () => { test('sets default policy from environment when subworkflow has none', async () => {
await Container.get(WorkflowRepository).save(parentWorkflow);
config.set('workflows.callerPolicyDefaultOption', 'none'); config.set('workflows.callerPolicyDefaultOption', 'none');
jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(fakeUser); jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(fakeUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
@ -243,12 +284,15 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
id: '2', id: '2',
}); });
await expect( await expect(
PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId), PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id),
).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`);
}); });
test('if sharing is disabled, ensures that workflows are owned by same user and reject running workflows belonging to another user even if setting allows execution', async () => { test('if sharing is disabled, ensures that workflows are owned by same user and reject running workflows belonging to another user even if setting allows execution', async () => {
jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(nonOwnerUser); await Container.get(WorkflowRepository).save(parentWorkflow);
jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValueOnce(fakeUser);
jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValueOnce(nonOwnerUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false);
const subworkflow = new Workflow({ const subworkflow = new Workflow({
@ -262,12 +306,12 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
}, },
}); });
await expect( await expect(
PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId), PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id),
).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`);
// Check description // Check description
try { try {
await PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, '', 'abcde'); await PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, 'abcde');
} catch (error) { } catch (error) {
if (error instanceof SubworkflowOperationError) { if (error instanceof SubworkflowOperationError) {
expect(error.description).toBe( expect(error.description).toBe(
@ -278,7 +322,8 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
}); });
test('should throw if allowed list does not contain parent workflow id', async () => { test('should throw if allowed list does not contain parent workflow id', async () => {
const invalidParentWorkflowId = uuid(); await Container.get(WorkflowRepository).save(parentWorkflow);
jest jest
.spyOn(ownershipService, 'getWorkflowOwnerCached') .spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser); .mockImplementation(async (workflowId) => fakeUser);
@ -296,11 +341,13 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
}, },
}); });
await expect( await expect(
PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId, invalidParentWorkflowId), PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id),
).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`); ).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`);
}); });
test('sameOwner passes when both workflows are owned by the same user', async () => { test('sameOwner passes when both workflows are owned by the same user', async () => {
await Container.get(WorkflowRepository).save(parentWorkflow);
jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockImplementation(async () => fakeUser); jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockImplementation(async () => fakeUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false); jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false);
@ -312,12 +359,13 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
id: '2', id: '2',
}); });
await expect( await expect(
PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId, userId), PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id),
).resolves.not.toThrow(); ).resolves.not.toThrow();
}); });
test('workflowsFromAList works when the list contains the parent id', async () => { test('workflowsFromAList works when the list contains the parent id', async () => {
const workflowId = uuid(); await Container.get(WorkflowRepository).save(parentWorkflow);
jest jest
.spyOn(ownershipService, 'getWorkflowOwnerCached') .spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser); .mockImplementation(async (workflowId) => fakeUser);
@ -331,15 +379,17 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
id: '2', id: '2',
settings: { settings: {
callerPolicy: 'workflowsFromAList', callerPolicy: 'workflowsFromAList',
callerIds: `123,456,bcdef, ${workflowId}`, callerIds: `123,456,bcdef, ${parentWorkflow.id}`,
}, },
}); });
await expect( await expect(
PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId, workflowId), PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id),
).resolves.not.toThrow(); ).resolves.not.toThrow();
}); });
test('should not throw when workflow policy is set to any', async () => { test('should not throw when workflow policy is set to any', async () => {
await Container.get(WorkflowRepository).save(parentWorkflow);
jest jest
.spyOn(ownershipService, 'getWorkflowOwnerCached') .spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser); .mockImplementation(async (workflowId) => fakeUser);
@ -356,7 +406,44 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
}, },
}); });
await expect( await expect(
PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId), PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id),
).resolves.not.toThrow(); ).resolves.not.toThrow();
}); });
describe('with workflows-from-same-owner caller policy', () => {
beforeAll(() => {
const license = new LicenseMocker();
license.mock(Container.get(License));
license.enable('feat:sharing');
});
test('should deny if the two workflows are owned by different users', async () => {
const parentWorkflowOwner = Container.get(UserRepository).create({ id: uuid() });
const subworkflowOwner = Container.get(UserRepository).create({ id: uuid() });
ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(parentWorkflowOwner);
ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(subworkflowOwner);
const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' });
const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid());
await expect(check).rejects.toThrow();
});
test('should allow if both workflows are owned by the same user', async () => {
await Container.get(WorkflowRepository).save(parentWorkflow);
const bothWorkflowsOwner = Container.get(UserRepository).create({ id: uuid() });
ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(bothWorkflowsOwner); // parent workflow
ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(bothWorkflowsOwner); // subworkflow
const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' });
const check = PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id);
await expect(check).resolves.not.toThrow();
});
});
}); });

View file

@ -3156,6 +3156,7 @@ export function getExecuteFunctions(
parentWorkflowId: workflow.id?.toString(), parentWorkflowId: workflow.id?.toString(),
inputData, inputData,
parentWorkflowSettings: workflow.settings, parentWorkflowSettings: workflow.settings,
node,
}) })
.then(async (result) => .then(async (result) =>
Container.get(BinaryDataService).duplicateBinaryData( Container.get(BinaryDataService).duplicateBinaryData(

View file

@ -1888,6 +1888,7 @@ export interface IWorkflowExecuteAdditionalData {
workflowInfo: IExecuteWorkflowInfo, workflowInfo: IExecuteWorkflowInfo,
additionalData: IWorkflowExecuteAdditionalData, additionalData: IWorkflowExecuteAdditionalData,
options: { options: {
node?: INode;
parentWorkflowId?: string; parentWorkflowId?: string;
inputData?: INodeExecutionData[]; inputData?: INodeExecutionData[];
parentExecutionId?: string; parentExecutionId?: string;

View file

@ -13,10 +13,11 @@ export class WorkflowOperationError extends ExecutionBaseError {
description: string | undefined; description: string | undefined;
constructor(message: string, node?: INode) { constructor(message: string, node?: INode, description?: string) {
super(message, { cause: undefined }); super(message, { cause: undefined });
this.severity = 'warning'; this.severity = 'warning';
this.name = this.constructor.name; this.name = this.constructor.name;
if (description) this.description = description;
this.node = node; this.node = node;
this.timestamp = Date.now(); this.timestamp = Date.now();
} }