diff --git a/packages/cli/src/collaboration/collaboration.service.ts b/packages/cli/src/collaboration/collaboration.service.ts index 67cd55a5182425..50ddbdec8ce8db 100644 --- a/packages/cli/src/collaboration/collaboration.service.ts +++ b/packages/cli/src/collaboration/collaboration.service.ts @@ -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; - } } diff --git a/packages/cli/src/errors/subworkflow-policy-denial.error.ts b/packages/cli/src/errors/subworkflow-policy-denial.error.ts index e4f4850dae9fdb..2d3169513062ff 100644 --- a/packages/cli/src/errors/subworkflow-policy-denial.error.ts +++ b/packages/cli/src/errors/subworkflow-policy-denial.error.ts @@ -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, + `Update sub-workflow settings 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(' '), + }; + + 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(`Target workflow ID ${subworkflowId} may not be called`, node, description); + super( + `The sub-workflow (${subworkflowId}) cannot be called by this workflow`, + node, + description(), + ); } } diff --git a/packages/cli/src/services/__tests__/ownership.service.test.ts b/packages/cli/src/services/__tests__/ownership.service.test.ts index 18061a4027290f..03b5c2fb146ce3 100644 --- a/packages/cli/src/services/__tests__/ownership.service.test.ts +++ b/packages/cli/src/services/__tests__/ownership.service.test.ts @@ -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(); }); diff --git a/packages/cli/src/services/__tests__/workflow-statistics.service.test.ts b/packages/cli/src/services/__tests__/workflow-statistics.service.test.ts index 34c8743b7374a9..0afb5de438e64b 100644 --- a/packages/cli/src/services/__tests__/workflow-statistics.service.test.ts +++ b/packages/cli/src/services/__tests__/workflow-statistics.service.test.ts @@ -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(); diff --git a/packages/cli/src/services/access.service.ts b/packages/cli/src/services/access.service.ts new file mode 100644 index 00000000000000..cc3580768f6e80 --- /dev/null +++ b/packages/cli/src/services/access.service.ts @@ -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; + } +} diff --git a/packages/cli/src/services/ownership.service.ts b/packages/cli/src/services/ownership.service.ts index dd4fb3acfe2c9c..e7363c9a6a0591 100644 --- a/packages/cli/src/services/ownership.service.ts +++ b/packages/cli/src/services/ownership.service.ts @@ -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 { + async getPersonalProjectOwnerCached(projectId: string): Promise { const cachedValue = await this.cacheService.getHashValue( 'project-owner', projectId, diff --git a/packages/cli/src/services/workflow-statistics.service.ts b/packages/cli/src/services/workflow-statistics.service.ts index fa01dabcfd2df5..ca93ecf9896368 100644 --- a/packages/cli/src/services/workflow-statistics.service.ts +++ b/packages/cli/src/services/workflow-statistics.service.ts @@ -72,7 +72,7 @@ export class WorkflowStatisticsService extends TypedEmitter - `Target workflow ID ${subworkflowId} may not be called`; - -const ownershipService = mockInstance(OwnershipService); -const memberPersonalProject = mock(); +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(); const globalConfig = mock({ workflows: { callerPolicyDefaultOption: 'workflowsFromSameOwner' }, }); - const checker = new SubworkflowPolicyChecker(mock(), license, ownershipService, globalConfig); + const accessService = mock(); + const urlService = mock(); + + 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({ workflows: { callerPolicyDefaultOption: 'none' } }), - ); + it('should fall back to `N8N_WORKFLOW_CALLER_POLICY_DEFAULT_OPTION`', async () => { + globalConfig.workflows.callerPolicyDefaultOption = 'none'; const parentWorkflow = mock(); - const subworkflow = mock(); // no caller policy + const subworkflowId = 'subworkflow-id'; + const subworkflow = mock({ id: subworkflowId }); // no caller policy - ownershipService.getWorkflowProjectCached.mockResolvedValue(memberPersonalProject); + const parentWorkflowProject = mock(); + const subworkflowProject = mock({ 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(); - const subworkflow = mock({ settings: { callerPolicy: 'any' } }); // should be overridden - - const firstProject = mock({ id: uuid() }); - const secondProject = mock({ 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({ id: uuid() }); const subworkflow = mock({ @@ -97,39 +76,62 @@ describe('SubworkflowPolicyChecker', () => { }); const parentWorkflowProject = mock({ id: uuid() }); - const subworkflowOwner = mock({ id: uuid() }); - ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow - ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow + const subworkflowProject = mock({ 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(); + it('should deny if caller list does not contain parent workflow ID', async () => { + const parentWorkflow = mock({ id: 'parent-workflow-id' }); + + const parentWorkflowProject = mock({ id: uuid() }); + const subworkflowProject = mock({ id: uuid(), type: 'team' }); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject); + const subworkflowId = 'subworkflow-id'; const subworkflow = mock({ + 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(); + const subworkflowId = 'subworkflow-id'; + const subworkflow = mock({ id: subworkflowId, settings: { callerPolicy: 'any' } }); // should be overridden + + const parentWorkflowProject = mock({ id: uuid() }); + const subworkflowProject = mock({ 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({ id: uuid() }); const subworkflow = mock({ settings: { callerPolicy: 'any' } }); const parentWorkflowProject = mock({ id: uuid() }); - const subworkflowOwner = mock({ id: uuid() }); - ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); // parent workflow - ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow - - ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(new Project()); + const subworkflowProject = mock({ 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({ id: uuid() }); - const subworkflowOwner = mock({ id: uuid() }); + const subworkflowProject = mock({ 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({ settings: { callerPolicy: 'workflowsFromSameOwner' } }); + const subworkflowId = 'subworkflow-id'; + const subworkflow = mock({ + 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(); const bothWorkflowsProject = mock({ 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({ 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({ id: uuid() }); + const subworkflowId = 'subworkflow-id'; + const subworkflow = mock({ id: subworkflowId, settings: { callerPolicy: 'none' } }); + + const parentWorkflowProject = mock({ id: uuid() }); + const subworkflowProject = mock({ id: uuid() }); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject); + + const subworkflowProjectOwner = mock({ 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(), + subworkflowProjectOwner.id, + ); + + await expect(check).rejects.toMatchObject({ + message: `The sub-workflow (${subworkflowId}) cannot be called by this workflow`, + description: `${SUBWORKFLOW_DENIAL_BASE_DESCRIPTION} Update sub-workflow settings to allow other workflows to call it.`, + }); + }); + + it('should contain description for inaccessible personal project case', async () => { + const parentWorkflow = mock({ id: uuid() }); + const subworkflowId = 'subworkflow-id'; + const subworkflow = mock({ id: subworkflowId, settings: { callerPolicy: 'none' } }); + + const parentWorkflowProject = mock({ id: uuid() }); + const subworkflowProject = mock({ id: uuid(), type: 'personal' }); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject); + + const subworkflowProjectOwner = mock({ + id: uuid(), + firstName: 'John', + lastName: 'Doe', + }); + ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(subworkflowProjectOwner); + accessService.hasReadAccess.mockResolvedValueOnce(false); + + const node = mock(); + + 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({ id: uuid() }); + const subworkflowId = 'subworkflow-id'; + const subworkflow = mock({ id: subworkflowId, settings: { callerPolicy: 'none' } }); + + const parentWorkflowProject = mock({ id: uuid() }); + const subworkflowProject = mock({ id: uuid(), type: 'team' }); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject); + + const subworkflowProjectOwner = mock({ + id: uuid(), + firstName: 'John', + lastName: 'Doe', + }); + ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(subworkflowProjectOwner); + accessService.hasReadAccess.mockResolvedValueOnce(false); + + const check = checker.check( + subworkflow, + parentWorkflow.id, + mock(), + 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({ id: uuid() }); + const subworkflowId = 'subworkflow-id'; + const subworkflow = mock({ id: subworkflowId, settings: { callerPolicy: 'none' } }); + + const parentWorkflowProject = mock({ id: uuid() }); + const subworkflowProject = mock({ id: uuid() }); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(parentWorkflowProject); + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(subworkflowProject); + + const subworkflowProjectOwner = mock({ id: uuid() }); + ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(subworkflowProjectOwner); + accessService.hasReadAccess.mockResolvedValueOnce(true); + + const check = checker.check(subworkflow, parentWorkflow.id, mock()); + + await expect(check).rejects.toMatchObject({ + message: `The sub-workflow (${subworkflowId}) cannot be called by this workflow`, + description: SUBWORKFLOW_DENIAL_BASE_DESCRIPTION, + }); + }); + }); }); diff --git a/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts b/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts index 09f46eadf05dc8..33e6e58f76f911 100644 --- a/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts +++ b/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts @@ -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; @@ -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 }); - - throw new SubworkflowPolicyDenialError({ - subworkflowId, - subworkflowProject, - areOwnedBySameProject, - node, - }); - } + if (policy === 'workflowsFromSameOwner' && areOwnedBySameProject) return; + + 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(',') diff --git a/packages/cli/src/user-management/permission-checker.ts b/packages/cli/src/user-management/permission-checker.ts index 965b3786a345e9..ec0dc2802a7be2 100644 --- a/packages/cli/src/user-management/permission-checker.ts +++ b/packages/cli/src/user-management/permission-checker.ts @@ -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. diff --git a/packages/cli/src/workflow-execute-additional-data.ts b/packages/cli/src/workflow-execute-additional-data.ts index 1c7923b2f84464..53cfdaee10a77a 100644 --- a/packages/cli/src/workflow-execute-additional-data.ts +++ b/packages/cli/src/workflow-execute-additional-data.ts @@ -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 diff --git a/packages/cli/test/integration/permission-checker.test.ts b/packages/cli/test/integration/permission-checker.test.ts index 32c9b83c939ce5..b64d840bf1fe1f 100644 --- a/packages/cli/test/integration/permission-checker.test.ts +++ b/packages/cli/test/integration/permission-checker.test.ts @@ -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),