mudhorn/n8n
1
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2024-12-25 12:44:07 -08:00
Code Issues Actions Packages Projects Releases Wiki Activity

fix(core): Permission check for subworkflow properly checking for workflow settings (#7576)

Browse source 
The `sharing` related code is legacy that was not removed. Subworkflow
execution should check workflow settings alone, and this is now
reflected in the code.

Github issue / Community forum post (link here to close automatically):

https://community.n8n.io/t/bug-when-using-the-execute-workflow-node-when-workflow-is-shared/32207

---------

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>
This commit is contained in:
Omar Ajoue 2023-11-01 18:31:34 +01:00 committed by GitHub
parent 100291e109
commit 437c95e84e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 12 additions and 54 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
packages/cli/src/UserManagement/PermissionChecker.ts
View file

@ -1,17 +1,11 @@
import type { INode, Workflow } from 'n8n-workflow';
import {
NodeOperationError,
SubworkflowOperationError,
WorkflowOperationError,
} from 'n8n-workflow';
import { NodeOperationError, SubworkflowOperationError } from 'n8n-workflow';
import type { FindOptionsWhere } from 'typeorm';
import { In } from 'typeorm';
import * as Db from '@/Db';
import config from '@/config';
import type { SharedCredentials } from '@db/entities/SharedCredentials';
import { isSharingEnabled } from './UserManagementHelper';
import { WorkflowsService } from '@/workflows/workflows.services';
import { UserService } from '@/services/user.service';
import { OwnershipService } from '@/services/ownership.service';
import Container from 'typedi';
import { RoleService } from '@/services/role.service';
@ -135,14 +129,7 @@ export class PermissionChecker {
}
if (policy === 'workflowsFromSameOwner') {
const user = await Container.get(UserService).findOne({ where: { id: userId } });
if (!user) {
throw new WorkflowOperationError(
'Fatal error: user not found. Please contact the system administrator.',
);
}
const sharing = await WorkflowsService.getSharing(user, subworkflow.id, ['role', 'user']);
if (!sharing || sharing.role.name !== 'owner') {
if (subworkflowOwner?.id !== userId) {
throw errorToThrow;
}
}

49
packages/cli/test/unit/PermissionChecker.test.ts
View file

@ -10,10 +10,8 @@ import { User } from '@db/entities/User';
import { SharedWorkflow } from '@db/entities/SharedWorkflow';
import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials';
import { NodeTypes } from '@/NodeTypes';
import { UserService } from '@/services/user.service';
import { PermissionChecker } from '@/UserManagement/PermissionChecker';
import * as UserManagementHelper from '@/UserManagement/UserManagementHelper';
import { WorkflowsService } from '@/workflows/workflows.services';
import { OwnershipService } from '@/services/ownership.service';
import { mockInstance } from '../integration/shared/utils/';
@ -225,18 +223,12 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
const nonOwnerMockRole = new Role();
nonOwnerMockRole.name = 'editor';
const sharedWorkflowNotOwner = new SharedWorkflow();
sharedWorkflowNotOwner.role = nonOwnerMockRole;
const userService = mockInstance(UserService);
const nonOwnerUser = new User();
nonOwnerUser.id = uuid();
test('sets default policy from environment when subworkflow has none', async () => {
config.set('workflows.callerPolicyDefaultOption', 'none');
jest
.spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => {
return fakeUser;
});
jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockResolvedValue(fakeUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
const subworkflow = new Workflow({
@ -251,15 +243,9 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
).rejects.toThrow(`Target workflow ID ${subworkflow.id} may not be called`);
});
test('if sharing is disabled, ensures that workflows are owner by same user', async () => {
jest
.spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser);
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);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false);
jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
return sharedWorkflowNotOwner;
});
const subworkflow = new Workflow({
nodes: [],
@ -267,6 +253,9 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
active: false,
nodeTypes: mockNodeTypes,
id: '2',
settings: {
callerPolicy: 'any',
},
});
await expect(
PermissionChecker.checkSubworkflowExecutePolicy(subworkflow, userId),
@ -284,16 +273,12 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
}
});
test('list of ids must include the parent workflow id', async () => {
test('should throw if allowed list does not contain parent workflow id', async () => {
const invalidParentWorkflowId = uuid();
jest
.spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
return sharedWorkflowNotOwner;
});
const subworkflow = new Workflow({
nodes: [],
@ -312,14 +297,8 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
});
test('sameOwner passes when both workflows are owned by the same user', async () => {
jest
.spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser);
jest.spyOn(ownershipService, 'getWorkflowOwnerCached').mockImplementation(async () => fakeUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(false);
jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
return sharedWorkflowOwner;
});
const subworkflow = new Workflow({
nodes: [],
@ -339,10 +318,6 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
.spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
return sharedWorkflowNotOwner;
});
const subworkflow = new Workflow({
nodes: [],
@ -365,10 +340,6 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => {
.spyOn(ownershipService, 'getWorkflowOwnerCached')
.mockImplementation(async (workflowId) => fakeUser);
jest.spyOn(UserManagementHelper, 'isSharingEnabled').mockReturnValue(true);
jest.spyOn(userService, 'findOne').mockImplementation(async () => fakeUser);
jest.spyOn(WorkflowsService, 'getSharing').mockImplementation(async () => {
return sharedWorkflowNotOwner;
});
const subworkflow = new Workflow({
nodes: [],