mirror of
https://github.com/n8n-io/n8n.git
synced 2024-11-09 22:24:05 -08:00
refactor(core): Revamp subworkflow policy checker errors (#10752)
This commit is contained in:
Iván Ovejero
GitHub
parent
db61bde9da
commit
8c6cd014a0
|
|
@ -7,9 +7,9 @@ import type { ICollaboratorsChanged } from '@/interfaces';
|
|||
import type { OnPushMessage } from '@/push/types';
|
||||
import { UserRepository } from '@/databases/repositories/user.repository';
|
||||
import type { User } from '@/databases/entities/user';
|
||||
import { SharedWorkflowRepository } from '@/databases/repositories/shared-workflow.repository';
|
||||
import { CollaborationState } from '@/collaboration/collaboration.state';
|
||||
import { AccessService } from '@/services/access.service';
|
||||
|
||||
import { CollaborationState } from './collaboration.state';
|
||||
import type { WorkflowClosedMessage, WorkflowOpenedMessage } from './collaboration.message';
|
||||
import { parseWorkflowMessage } from './collaboration.message';
|
||||
|
||||
|
|
@ -23,7 +23,7 @@ export class CollaborationService {
|
|||
private readonly push: Push,
|
||||
private readonly state: CollaborationState,
|
||||
private readonly userRepository: UserRepository,
|
||||
private readonly sharedWorkflowRepository: SharedWorkflowRepository,
|
||||
private readonly accessService: AccessService,
|
||||
) {}
|
||||
|
||||
init() {
|
||||
|
|
@ -57,7 +57,7 @@ export class CollaborationService {
|
|||
private async handleWorkflowOpened(userId: User['id'], msg: WorkflowOpenedMessage) {
|
||||
const { workflowId } = msg;
|
||||
|
||||
if (!(await this.hasUserAccessToWorkflow(userId, workflowId))) {
|
||||
if (!(await this.accessService.hasReadAccess(userId, workflowId))) {
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -69,7 +69,7 @@ export class CollaborationService {
|
|||
private async handleWorkflowClosed(userId: User['id'], msg: WorkflowClosedMessage) {
|
||||
const { workflowId } = msg;
|
||||
|
||||
if (!(await this.hasUserAccessToWorkflow(userId, workflowId))) {
|
||||
if (!(await this.accessService.hasReadAccess(userId, workflowId))) {
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -99,19 +99,4 @@ export class CollaborationService {
|
|||
|
||||
this.push.sendToUsers('collaboratorsChanged', msgData, userIds);
|
||||
}
|
||||
|
||||
private async hasUserAccessToWorkflow(userId: User['id'], workflowId: Workflow['id']) {
|
||||
const user = await this.userRepository.findOneBy({
|
||||
id: userId,
|
||||
});
|
||||
if (!user) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const workflow = await this.sharedWorkflowRepository.findWorkflowForUser(workflowId, user, [
|
||||
'workflow:read',
|
||||
]);
|
||||
|
||||
return !!workflow;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,24 +2,66 @@ import { WorkflowOperationError } from 'n8n-workflow';
|
|||
import type { Project } from '@/databases/entities/project';
|
||||
import type { INode } from 'n8n-workflow';
|
||||
|
||||
type SubworkflowPolicyDenialErrorParams = {
|
||||
type Options = {
|
||||
/** ID of the subworkflow whose execution was denied. */
|
||||
subworkflowId: string;
|
||||
|
||||
/** Project that owns the subworkflow whose execution was denied. */
|
||||
subworkflowProject: Project;
|
||||
areOwnedBySameProject?: boolean;
|
||||
|
||||
/** Whether the user has read access to the subworkflow based on their project and scope. */
|
||||
hasReadAccess: boolean;
|
||||
|
||||
/** URL of the n8n instance. */
|
||||
instanceUrl: string;
|
||||
|
||||
/** Full name of the user who owns the personal project that owns the subworkflow. Absent if team project. */
|
||||
ownerName?: string;
|
||||
|
||||
/** Node that triggered the execution of the subworkflow whose execution was denied. */
|
||||
node?: INode;
|
||||
};
|
||||
|
||||
export const SUBWORKFLOW_DENIAL_BASE_DESCRIPTION =
|
||||
'The sub-workflow you’re trying to execute limits which workflows it can be called by.';
|
||||
|
||||
export class SubworkflowPolicyDenialError extends WorkflowOperationError {
|
||||
constructor({
|
||||
subworkflowId,
|
||||
subworkflowProject,
|
||||
areOwnedBySameProject,
|
||||
instanceUrl,
|
||||
hasReadAccess,
|
||||
ownerName,
|
||||
node,
|
||||
}: SubworkflowPolicyDenialErrorParams) {
|
||||
const description = areOwnedBySameProject
|
||||
? 'Change the settings of the sub-workflow so it can be called by this one.'
|
||||
: `An admin for the ${subworkflowProject.name} project can make this change. You may need to tell them the ID of the sub-workflow, which is ${subworkflowId}`;
|
||||
}: Options) {
|
||||
const descriptions = {
|
||||
default: SUBWORKFLOW_DENIAL_BASE_DESCRIPTION,
|
||||
accessible: [
|
||||
SUBWORKFLOW_DENIAL_BASE_DESCRIPTION,
|
||||
`<a href="${instanceUrl}/workflow/${subworkflowId}" target="_blank">Update sub-workflow settings</a> to allow other workflows to call it.`,
|
||||
].join(' '),
|
||||
inaccessiblePersonalProject: [
|
||||
SUBWORKFLOW_DENIAL_BASE_DESCRIPTION,
|
||||
`You will need ${ownerName} to update the sub-workflow (${subworkflowId}) settings to allow this workflow to call it.`,
|
||||
].join(' '),
|
||||
inaccesibleTeamProject: [
|
||||
SUBWORKFLOW_DENIAL_BASE_DESCRIPTION,
|
||||
`You will need an admin from the ${subworkflowProject.name} project to update the sub-workflow (${subworkflowId}) settings to allow this workflow to call it.`,
|
||||
].join(' '),
|
||||
};
|
||||
|
||||
super(`Target workflow ID ${subworkflowId} may not be called`, node, description);
|
||||
const description = () => {
|
||||
if (hasReadAccess) return descriptions.accessible;
|
||||
if (subworkflowProject.type === 'personal') return descriptions.inaccessiblePersonalProject;
|
||||
if (subworkflowProject.type === 'team') return descriptions.inaccesibleTeamProject;
|
||||
|
||||
return descriptions.default;
|
||||
};
|
||||
|
||||
super(
|
||||
`The sub-workflow (${subworkflowId}) cannot be called by this workflow`,
|
||||
node,
|
||||
description(),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -64,7 +64,7 @@ describe('OwnershipService', () => {
|
|||
|
||||
projectRelationRepository.getPersonalProjectOwners.mockResolvedValueOnce([projectRelation]);
|
||||
|
||||
const returnedOwner = await ownershipService.getProjectOwnerCached('some-project-id');
|
||||
const returnedOwner = await ownershipService.getPersonalProjectOwnerCached('some-project-id');
|
||||
|
||||
expect(returnedOwner).toBe(mockOwner);
|
||||
});
|
||||
|
|
@ -72,7 +72,7 @@ describe('OwnershipService', () => {
|
|||
test('should not throw if no project owner found, should return null instead', async () => {
|
||||
projectRelationRepository.getPersonalProjectOwners.mockResolvedValueOnce([]);
|
||||
|
||||
const owner = await ownershipService.getProjectOwnerCached('some-project-id');
|
||||
const owner = await ownershipService.getPersonalProjectOwnerCached('some-project-id');
|
||||
|
||||
expect(owner).toBeNull();
|
||||
});
|
||||
|
|
@ -91,7 +91,7 @@ describe('OwnershipService', () => {
|
|||
|
||||
projectRelationRepository.getPersonalProjectOwners.mockResolvedValueOnce([projectRelation]);
|
||||
|
||||
const returnedOwner = await ownershipService.getProjectOwnerCached('some-project-id');
|
||||
const returnedOwner = await ownershipService.getPersonalProjectOwnerCached('some-project-id');
|
||||
|
||||
expect(returnedOwner).toBe(mockOwner);
|
||||
});
|
||||
|
|
@ -99,7 +99,7 @@ describe('OwnershipService', () => {
|
|||
test('should not throw if no project owner found, should return null instead', async () => {
|
||||
projectRelationRepository.getPersonalProjectOwners.mockResolvedValueOnce([]);
|
||||
|
||||
const owner = await ownershipService.getProjectOwnerCached('some-project-id');
|
||||
const owner = await ownershipService.getPersonalProjectOwnerCached('some-project-id');
|
||||
|
||||
expect(owner).toBeNull();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -42,7 +42,7 @@ describe('WorkflowStatisticsService', () => {
|
|||
config.set('diagnostics.enabled', true);
|
||||
config.set('deployment.type', 'n8n-testing');
|
||||
mocked(ownershipService.getWorkflowProjectCached).mockResolvedValue(fakeProject);
|
||||
mocked(ownershipService.getProjectOwnerCached).mockResolvedValue(fakeUser);
|
||||
mocked(ownershipService.getPersonalProjectOwnerCached).mockResolvedValue(fakeUser);
|
||||
const updateSettingsMock = jest.spyOn(userService, 'updateSettings').mockImplementation();
|
||||
|
||||
const eventService = mock<EventService>();
|
||||
|
|
|
|||
29
packages/cli/src/services/access.service.ts
Normal file
29
packages/cli/src/services/access.service.ts
Normal file
|
|
@ -0,0 +1,29 @@
|
|||
import { Service } from 'typedi';
|
||||
import { SharedWorkflowRepository } from '@/databases/repositories/shared-workflow.repository';
|
||||
import { UserRepository } from '@/databases/repositories/user.repository';
|
||||
import type { User } from '@/databases/entities/user';
|
||||
import type { Workflow } from 'n8n-workflow';
|
||||
|
||||
/**
|
||||
* Responsible for checking whether a user has access to a resource.
|
||||
*/
|
||||
@Service()
|
||||
export class AccessService {
|
||||
constructor(
|
||||
private readonly userRepository: UserRepository,
|
||||
private readonly sharedWorkflowRepository: SharedWorkflowRepository,
|
||||
) {}
|
||||
|
||||
/** Whether a user has read access to a workflow based on their project and scope. */
|
||||
async hasReadAccess(userId: User['id'], workflowId: Workflow['id']) {
|
||||
const user = await this.userRepository.findOneBy({ id: userId });
|
||||
|
||||
if (!user) return false;
|
||||
|
||||
const workflow = await this.sharedWorkflowRepository.findWorkflowForUser(workflowId, user, [
|
||||
'workflow:read',
|
||||
]);
|
||||
|
||||
return workflow !== null;
|
||||
}
|
||||
}
|
||||
|
|
@ -40,9 +40,10 @@ export class OwnershipService {
|
|||
}
|
||||
|
||||
/**
|
||||
* Retrieve the user that owns the project, or null if it's not an ownable project. Note that project ownership is **immutable**.
|
||||
* Retrieve the user who owns the personal project, or `null` if non-personal project.
|
||||
* Personal project ownership is **immutable**.
|
||||
*/
|
||||
async getProjectOwnerCached(projectId: string): Promise<User | null> {
|
||||
async getPersonalProjectOwnerCached(projectId: string): Promise<User | null> {
|
||||
const cachedValue = await this.cacheService.getHashValue<User | null>(
|
||||
'project-owner',
|
||||
projectId,
|
||||
|
|
|
|||
|
|
@ -72,7 +72,7 @@ export class WorkflowStatisticsService extends TypedEmitter<WorkflowStatisticsEv
|
|||
if (name === StatisticsNames.productionSuccess && upsertResult === 'insert') {
|
||||
const project = await this.ownershipService.getWorkflowProjectCached(workflowId);
|
||||
if (project.type === 'personal') {
|
||||
const owner = await this.ownershipService.getProjectOwnerCached(project.id);
|
||||
const owner = await this.ownershipService.getPersonalProjectOwnerCached(project.id);
|
||||
|
||||
if (owner && !owner.settings?.userActivated) {
|
||||
await this.userService.updateSettings(owner.id, {
|
||||
|
|
@ -105,7 +105,7 @@ export class WorkflowStatisticsService extends TypedEmitter<WorkflowStatisticsEv
|
|||
|
||||
// Compile the metrics since this was a new data loaded event
|
||||
const project = await this.ownershipService.getWorkflowProjectCached(workflowId);
|
||||
const owner = await this.ownershipService.getProjectOwnerCached(project.id);
|
||||
const owner = await this.ownershipService.getPersonalProjectOwnerCached(project.id);
|
||||
|
||||
let metrics = {
|
||||
userId: owner!.id,
|
||||
|
|
|
|||
|
|
@ -1,29 +1,39 @@
|
|||
import { v4 as uuid } from 'uuid';
|
||||
import type { Workflow } from 'n8n-workflow';
|
||||
import { SubworkflowOperationError } from 'n8n-workflow';
|
||||
import { Project } from '@/databases/entities/project';
|
||||
import type { INode, Workflow } from 'n8n-workflow';
|
||||
import type { 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/workflow-entity';
|
||||
import type { License } from '@/license';
|
||||
import type { GlobalConfig } from '@n8n/config';
|
||||
|
||||
const toTargetCallErrorMsg = (subworkflowId: string) =>
|
||||
`Target workflow ID ${subworkflowId} may not be called`;
|
||||
|
||||
const ownershipService = mockInstance(OwnershipService);
|
||||
const memberPersonalProject = mock<Project>();
|
||||
import type { AccessService } from '@/services/access.service';
|
||||
import type { User } from '@/databases/entities/user';
|
||||
import {
|
||||
SUBWORKFLOW_DENIAL_BASE_DESCRIPTION,
|
||||
SubworkflowPolicyDenialError,
|
||||
} from '@/errors/subworkflow-policy-denial.error';
|
||||
import type { UrlService } from '@/services/url.service';
|
||||
|
||||
describe('SubworkflowPolicyChecker', () => {
|
||||
const ownershipService = mockInstance(OwnershipService);
|
||||
const license = mock<License>();
|
||||
const globalConfig = mock<GlobalConfig>({
|
||||
workflows: { callerPolicyDefaultOption: 'workflowsFromSameOwner' },
|
||||
});
|
||||
const checker = new SubworkflowPolicyChecker(mock(), license, ownershipService, globalConfig);
|
||||
const accessService = mock<AccessService>();
|
||||
const urlService = mock<UrlService>();
|
||||
|
||||
const checker = new SubworkflowPolicyChecker(
|
||||
mock(),
|
||||
license,
|
||||
ownershipService,
|
||||
globalConfig,
|
||||
accessService,
|
||||
urlService,
|
||||
);
|
||||
|
||||
beforeEach(() => {
|
||||
license.isSharingEnabled.mockReturnValue(true);
|
||||
|
|
@ -34,59 +44,28 @@ describe('SubworkflowPolicyChecker', () => {
|
|||
});
|
||||
|
||||
describe('no caller policy', () => {
|
||||
test('should fall back to N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION', async () => {
|
||||
const checker = new SubworkflowPolicyChecker(
|
||||
mock(),
|
||||
license,
|
||||
ownershipService,
|
||||
mock<GlobalConfig>({ workflows: { callerPolicyDefaultOption: 'none' } }),
|
||||
);
|
||||
it('should fall back to `N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION`', async () => {
|
||||
globalConfig.workflows.callerPolicyDefaultOption = 'none';
|
||||
|
||||
const parentWorkflow = mock<WorkflowEntity>();
|
||||
const subworkflow = mock<Workflow>(); // no caller policy
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({ id: subworkflowId }); // no caller policy
|
||||
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValue(memberPersonalProject);
|
||||
const parentWorkflowProject = mock<Project>();
|
||||
const subworkflowProject = mock<Project>({ type: 'team' });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const check = checker.check(subworkflow, parentWorkflow.id);
|
||||
|
||||
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
|
||||
await expect(check).rejects.toThrowError(SubworkflowPolicyDenialError);
|
||||
|
||||
config.load(config.default);
|
||||
globalConfig.workflows.callerPolicyDefaultOption = 'workflowsFromSameOwner';
|
||||
});
|
||||
});
|
||||
|
||||
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 () => {
|
||||
describe('`workflows-from-list` caller policy', () => {
|
||||
it('should allow if caller list contains parent workflow ID', async () => {
|
||||
const parentWorkflow = mock<WorkflowEntity>({ id: uuid() });
|
||||
|
||||
const subworkflow = mock<Workflow>({
|
||||
|
|
@ -97,39 +76,62 @@ describe('SubworkflowPolicyChecker', () => {
|
|||
});
|
||||
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowOwner = mock<Project>({ id: uuid() });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow
|
||||
const subworkflowProject = mock<Project>({ id: uuid() });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
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>();
|
||||
it('should deny if caller list does not contain parent workflow ID', async () => {
|
||||
const parentWorkflow = mock<WorkflowEntity>({ id: 'parent-workflow-id' });
|
||||
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowProject = mock<Project>({ id: uuid(), type: 'team' });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({
|
||||
id: subworkflowId,
|
||||
settings: { callerPolicy: 'workflowsFromAList', callerIds: 'xyz' },
|
||||
});
|
||||
|
||||
const check = checker.check(subworkflow, parentWorkflow.id);
|
||||
|
||||
await expect(check).rejects.toThrow();
|
||||
await expect(check).rejects.toThrowError(SubworkflowPolicyDenialError);
|
||||
});
|
||||
});
|
||||
|
||||
describe('any caller policy', () => {
|
||||
test('should not throw', async () => {
|
||||
describe('`any` caller policy', () => {
|
||||
it('if no sharing, should be overriden to `workflows-from-same-owner`', async () => {
|
||||
license.isSharingEnabled.mockReturnValueOnce(false);
|
||||
|
||||
const parentWorkflow = mock<WorkflowEntity>();
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({ id: subworkflowId, settings: { callerPolicy: 'any' } }); // should be overridden
|
||||
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowProject = mock<Project>({ id: uuid(), type: 'team' });
|
||||
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const check = checker.check(subworkflow, parentWorkflow.id);
|
||||
|
||||
await expect(check).rejects.toThrowError(SubworkflowPolicyDenialError);
|
||||
});
|
||||
|
||||
it('should not throw on a regular subworkflow call', 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 subworkflowProject = mock<Project>({ id: uuid() });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const check = checker.check(subworkflow, parentWorkflow.id);
|
||||
|
||||
|
|
@ -137,28 +139,32 @@ describe('SubworkflowPolicyChecker', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('workflows-from-same-owner caller policy', () => {
|
||||
test('should deny if the two workflows are owned by different users', async () => {
|
||||
describe('`workflows-from-same-owner` caller policy', () => {
|
||||
it('should deny if the two workflows are owned by different projects', async () => {
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowOwner = mock<Project>({ id: uuid() });
|
||||
const subworkflowProject = mock<Project>({ id: uuid(), type: 'team' });
|
||||
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const subworkflow = mock<Workflow>({ settings: { callerPolicy: 'workflowsFromSameOwner' } });
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({
|
||||
id: subworkflowId,
|
||||
settings: { callerPolicy: 'workflowsFromSameOwner' },
|
||||
});
|
||||
|
||||
const check = checker.check(subworkflow, uuid());
|
||||
|
||||
await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id));
|
||||
await expect(check).rejects.toThrowError(SubworkflowPolicyDenialError);
|
||||
});
|
||||
|
||||
test('should allow if both workflows are owned by the same user', async () => {
|
||||
it('should allow if both workflows are owned by the same project', async () => {
|
||||
const parentWorkflow = mock<WorkflowEntity>();
|
||||
|
||||
const bothWorkflowsProject = mock<Project>({ id: uuid() });
|
||||
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // parent workflow
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // subworkflow
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // parent workflow project
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(bothWorkflowsProject); // subworkflow project
|
||||
|
||||
const subworkflow = mock<Workflow>({ settings: { callerPolicy: 'workflowsFromSameOwner' } });
|
||||
|
||||
|
|
@ -167,4 +173,117 @@ describe('SubworkflowPolicyChecker', () => {
|
|||
await expect(check).resolves.not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe('error details', () => {
|
||||
it('should contain description for accessible case', async () => {
|
||||
const parentWorkflow = mock<WorkflowEntity>({ id: uuid() });
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({ id: subworkflowId, settings: { callerPolicy: 'none' } });
|
||||
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowProject = mock<Project>({ id: uuid() });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const subworkflowProjectOwner = mock<User>({ id: uuid() });
|
||||
ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(subworkflowProjectOwner);
|
||||
accessService.hasReadAccess.mockResolvedValueOnce(true);
|
||||
|
||||
const instanceUrl = 'https://n8n.test.com';
|
||||
urlService.getInstanceBaseUrl.mockReturnValueOnce(instanceUrl);
|
||||
|
||||
const check = checker.check(
|
||||
subworkflow,
|
||||
parentWorkflow.id,
|
||||
mock<INode>(),
|
||||
subworkflowProjectOwner.id,
|
||||
);
|
||||
|
||||
await expect(check).rejects.toMatchObject({
|
||||
message: `The sub-workflow (${subworkflowId}) cannot be called by this workflow`,
|
||||
description: `${SUBWORKFLOW_DENIAL_BASE_DESCRIPTION} <a href=\"${instanceUrl}/workflow/subworkflow-id\" target=\"_blank\">Update sub-workflow settings</a> to allow other workflows to call it.`,
|
||||
});
|
||||
});
|
||||
|
||||
it('should contain description for inaccessible personal project case', async () => {
|
||||
const parentWorkflow = mock<WorkflowEntity>({ id: uuid() });
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({ id: subworkflowId, settings: { callerPolicy: 'none' } });
|
||||
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowProject = mock<Project>({ id: uuid(), type: 'personal' });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const subworkflowProjectOwner = mock<User>({
|
||||
id: uuid(),
|
||||
firstName: 'John',
|
||||
lastName: 'Doe',
|
||||
});
|
||||
ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(subworkflowProjectOwner);
|
||||
accessService.hasReadAccess.mockResolvedValueOnce(false);
|
||||
|
||||
const node = mock<INode>();
|
||||
|
||||
const check = checker.check(subworkflow, parentWorkflow.id, node, subworkflowProjectOwner.id);
|
||||
|
||||
await expect(check).rejects.toMatchObject({
|
||||
message: `The sub-workflow (${subworkflowId}) cannot be called by this workflow`,
|
||||
description: `${SUBWORKFLOW_DENIAL_BASE_DESCRIPTION} You will need John Doe to update the sub-workflow (${subworkflowId}) settings to allow this workflow to call it.`,
|
||||
});
|
||||
});
|
||||
|
||||
it('should contain description for inaccessible team project case', async () => {
|
||||
const parentWorkflow = mock<WorkflowEntity>({ id: uuid() });
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({ id: subworkflowId, settings: { callerPolicy: 'none' } });
|
||||
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowProject = mock<Project>({ id: uuid(), type: 'team' });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const subworkflowProjectOwner = mock<User>({
|
||||
id: uuid(),
|
||||
firstName: 'John',
|
||||
lastName: 'Doe',
|
||||
});
|
||||
ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(subworkflowProjectOwner);
|
||||
accessService.hasReadAccess.mockResolvedValueOnce(false);
|
||||
|
||||
const check = checker.check(
|
||||
subworkflow,
|
||||
parentWorkflow.id,
|
||||
mock<INode>(),
|
||||
subworkflowProjectOwner.id,
|
||||
);
|
||||
|
||||
await expect(check).rejects.toMatchObject({
|
||||
message: `The sub-workflow (${subworkflowId}) cannot be called by this workflow`,
|
||||
description: `${SUBWORKFLOW_DENIAL_BASE_DESCRIPTION} You will need an admin from the ${subworkflowProject.name} project to update the sub-workflow (${subworkflowId}) settings to allow this workflow to call it.`,
|
||||
});
|
||||
});
|
||||
|
||||
it('should contain description for default (e.g. error workflow) case', async () => {
|
||||
const parentWorkflow = mock<WorkflowEntity>({ id: uuid() });
|
||||
const subworkflowId = 'subworkflow-id';
|
||||
const subworkflow = mock<Workflow>({ id: subworkflowId, settings: { callerPolicy: 'none' } });
|
||||
|
||||
const parentWorkflowProject = mock<Project>({ id: uuid() });
|
||||
const subworkflowProject = mock<Project>({ id: uuid() });
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject);
|
||||
|
||||
const subworkflowProjectOwner = mock<User>({ id: uuid() });
|
||||
ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(subworkflowProjectOwner);
|
||||
accessService.hasReadAccess.mockResolvedValueOnce(true);
|
||||
|
||||
const check = checker.check(subworkflow, parentWorkflow.id, mock<INode>());
|
||||
|
||||
await expect(check).rejects.toMatchObject({
|
||||
message: `The sub-workflow (${subworkflowId}) cannot be called by this workflow`,
|
||||
description: SUBWORKFLOW_DENIAL_BASE_DESCRIPTION,
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -3,8 +3,12 @@ import { GlobalConfig } from '@n8n/config';
|
|||
import { Logger } from '@/logger';
|
||||
import { License } from '@/license';
|
||||
import { OwnershipService } from '@/services/ownership.service';
|
||||
import type { Workflow, INode, WorkflowSettings } from 'n8n-workflow';
|
||||
import { type Workflow, type INode, type WorkflowSettings } from 'n8n-workflow';
|
||||
import { SubworkflowPolicyDenialError } from '@/errors/subworkflow-policy-denial.error';
|
||||
import { AccessService } from '@/services/access.service';
|
||||
import type { Project } from '@/databases/entities/project';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { UrlService } from '@/services/url.service';
|
||||
|
||||
type Policy = WorkflowSettings.CallerPolicy;
|
||||
type DenialPolicy = Exclude<Policy, 'any'>;
|
||||
|
|
@ -16,12 +20,14 @@ export class SubworkflowPolicyChecker {
|
|||
private readonly license: License,
|
||||
private readonly ownershipService: OwnershipService,
|
||||
private readonly globalConfig: GlobalConfig,
|
||||
private readonly accessService: AccessService,
|
||||
private readonly urlService: UrlService,
|
||||
) {}
|
||||
|
||||
/**
|
||||
* Check whether the parent workflow is allowed to call the subworkflow.
|
||||
*/
|
||||
async check(subworkflow: Workflow, parentWorkflowId: string, node?: INode) {
|
||||
async check(subworkflow: Workflow, parentWorkflowId: string, node?: INode, userId?: string) {
|
||||
const { id: subworkflowId } = subworkflow;
|
||||
|
||||
if (!subworkflowId) return; // e.g. when running a subworkflow loaded from a file
|
||||
|
|
@ -30,6 +36,8 @@ export class SubworkflowPolicyChecker {
|
|||
|
||||
if (policy === 'any') return;
|
||||
|
||||
if (policy === 'workflowsFromAList' && this.isListed(subworkflow, parentWorkflowId)) return;
|
||||
|
||||
const { parentWorkflowProject, subworkflowProject } = await this.findProjects({
|
||||
parentWorkflowId,
|
||||
subworkflowId,
|
||||
|
|
@ -37,20 +45,36 @@ export class SubworkflowPolicyChecker {
|
|||
|
||||
const areOwnedBySameProject = parentWorkflowProject.id === subworkflowProject.id;
|
||||
|
||||
if (
|
||||
policy === 'none' ||
|
||||
(policy === 'workflowsFromAList' && !this.hasParentListed(subworkflow, parentWorkflowId)) ||
|
||||
(policy === 'workflowsFromSameOwner' && !areOwnedBySameProject)
|
||||
) {
|
||||
this.logDenial({ parentWorkflowId, subworkflowId, policy });
|
||||
if (policy === 'workflowsFromSameOwner' && areOwnedBySameProject) return;
|
||||
|
||||
throw new SubworkflowPolicyDenialError({
|
||||
subworkflowId,
|
||||
subworkflowProject,
|
||||
areOwnedBySameProject,
|
||||
node,
|
||||
});
|
||||
}
|
||||
this.logDenial({ parentWorkflowId, subworkflowId, policy });
|
||||
|
||||
const errorDetails = await this.errorDetails(subworkflowProject, subworkflow, userId);
|
||||
|
||||
throw new SubworkflowPolicyDenialError({
|
||||
subworkflowId,
|
||||
subworkflowProject,
|
||||
node,
|
||||
instanceUrl: this.urlService.getInstanceBaseUrl(),
|
||||
...errorDetails,
|
||||
});
|
||||
}
|
||||
|
||||
private async errorDetails(subworkflowProject: Project, subworkflow: Workflow, userId?: string) {
|
||||
const hasReadAccess = userId
|
||||
? await this.accessService.hasReadAccess(userId, subworkflow.id)
|
||||
: false; /* no user ID in policy check for error workflow, so `false` to keep error message generic */
|
||||
|
||||
if (subworkflowProject.type === 'team') return { hasReadAccess };
|
||||
|
||||
const owner = await this.ownershipService.getPersonalProjectOwnerCached(subworkflowProject.id);
|
||||
|
||||
assert(owner !== null); // only `null` if not personal
|
||||
|
||||
return {
|
||||
hasReadAccess,
|
||||
ownerName: owner.firstName + ' ' + owner.lastName,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -85,7 +109,7 @@ export class SubworkflowPolicyChecker {
|
|||
/**
|
||||
* Whether the subworkflow has the parent workflow listed as a caller.
|
||||
*/
|
||||
private hasParentListed(subworkflow: Workflow, parentWorkflowId: string) {
|
||||
private isListed(subworkflow: Workflow, parentWorkflowId: string) {
|
||||
const callerIds =
|
||||
subworkflow.settings.callerIds
|
||||
?.split(',')
|
||||
|
|
|
|||
|
|
@ -19,7 +19,9 @@ export class PermissionChecker {
|
|||
*/
|
||||
async check(workflowId: string, nodes: INode[]) {
|
||||
const homeProject = await this.ownershipService.getWorkflowProjectCached(workflowId);
|
||||
const homeProjectOwner = await this.ownershipService.getProjectOwnerCached(homeProject.id);
|
||||
const homeProjectOwner = await this.ownershipService.getPersonalProjectOwnerCached(
|
||||
homeProject.id,
|
||||
);
|
||||
if (homeProject.type === 'personal' && homeProjectOwner?.hasGlobalScope('credential:list')) {
|
||||
// Workflow belongs to a project by a user with privileges
|
||||
// so all credentials are usable. Skip credential checks.
|
||||
|
|
|
|||
|
|
@ -807,6 +807,7 @@ async function executeWorkflow(
|
|||
workflow,
|
||||
options.parentWorkflowId,
|
||||
options.node,
|
||||
additionalData.userId,
|
||||
);
|
||||
|
||||
// Create new additionalData to have different workflow loaded and to call
|
||||
|
|
|
|||
|
|
@ -265,7 +265,7 @@ describe('check()', () => {
|
|||
|
||||
const workflowEntity = await createWorkflow(nodes, owner);
|
||||
ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(ownerPersonalProject);
|
||||
ownershipService.getProjectOwnerCached.mockResolvedValueOnce(owner);
|
||||
ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(owner);
|
||||
|
||||
await expect(
|
||||
permissionChecker.check(workflowEntity.id, workflowEntity.nodes),
|
||||
|
|
|
|||
Loading…
Reference in a new issue