From 5832d3ca4695ec812e028e40b41811ca2215c0e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Fri, 2 Feb 2024 12:21:53 +0100 Subject: [PATCH] fix(core): Fix PermissionChecker.check, and add additional unit tests (#8528) --- .../src/UserManagement/PermissionChecker.ts | 18 +- .../src/credentials/credentials.service.ts | 2 +- .../sharedCredentials.repository.ts | 32 +- .../repositories/sharedWorkflow.repository.ts | 9 + .../integration/PermissionChecker.test.ts | 385 +++++++++++++++ .../cli/test/unit/PermissionChecker.test.ts | 452 ++++-------------- 6 files changed, 520 insertions(+), 378 deletions(-) create mode 100644 packages/cli/test/integration/PermissionChecker.test.ts diff --git a/packages/cli/src/UserManagement/PermissionChecker.ts b/packages/cli/src/UserManagement/PermissionChecker.ts index 7b6261c4d6..de1e60f610 100644 --- a/packages/cli/src/UserManagement/PermissionChecker.ts +++ b/packages/cli/src/UserManagement/PermissionChecker.ts @@ -39,24 +39,20 @@ export class PermissionChecker { if (user.hasGlobalScope('workflow:execute')) return; + const isSharingEnabled = this.license.isSharingEnabled(); + // allow if all creds used in this workflow are a subset of // all creds accessible to users who have access to this workflow let workflowUserIds = [userId]; - if (workflow.id && this.license.isSharingEnabled()) { - const workflowSharings = await this.sharedWorkflowRepository.find({ - relations: ['workflow'], - where: { workflowId: workflow.id }, - select: ['userId'], - }); - workflowUserIds = workflowSharings.map((s) => s.userId); + if (workflow.id && isSharingEnabled) { + workflowUserIds = await this.sharedWorkflowRepository.getSharedUserIds(workflow.id); } - const credentialSharings = - await this.sharedCredentialsRepository.findOwnedSharings(workflowUserIds); - - const accessibleCredIds = credentialSharings.map((s) => s.credentialsId); + const accessibleCredIds = isSharingEnabled + ? await this.sharedCredentialsRepository.getAccessibleCredentialIds(workflowUserIds) + : await this.sharedCredentialsRepository.getOwnedCredentialIds(workflowUserIds); const inaccessibleCredIds = workflowCredIds.filter((id) => !accessibleCredIds.includes(id)); diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 9141ce6e29..0d40482990 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -63,7 +63,7 @@ export class CredentialsService { : credentials; } - const ids = await this.sharedCredentialsRepository.getAccessibleCredentials(user.id); + const ids = await this.sharedCredentialsRepository.getAccessibleCredentialIds([user.id]); const credentials = await this.credentialsRepository.findMany( options.listQueryOptions, diff --git a/packages/cli/src/databases/repositories/sharedCredentials.repository.ts b/packages/cli/src/databases/repositories/sharedCredentials.repository.ts index 0a52f52153..eb1d194dd2 100644 --- a/packages/cli/src/databases/repositories/sharedCredentials.repository.ts +++ b/packages/cli/src/databases/repositories/sharedCredentials.repository.ts @@ -1,7 +1,7 @@ import { Service } from 'typedi'; import type { EntityManager } from 'typeorm'; import { DataSource, In, Not, Repository } from 'typeorm'; -import { SharedCredentials } from '../entities/SharedCredentials'; +import { type CredentialSharingRole, SharedCredentials } from '../entities/SharedCredentials'; import type { User } from '../entities/User'; @Service() @@ -36,27 +36,27 @@ export class SharedCredentialsRepository extends Repository { return await this.update({ userId: Not(user.id), role: 'credential:owner' }, { user }); } - /** - * Get the IDs of all credentials owned by or shared with a user. - */ - async getAccessibleCredentials(userId: string) { - const sharings = await this.find({ - where: { - userId, - role: In(['credential:owner', 'credential:user']), - }, - }); - - return sharings.map((s) => s.credentialsId); + /** Get the IDs of all credentials owned by a user */ + async getOwnedCredentialIds(userIds: string[]) { + return await this.getCredentialIdsByUserAndRole(userIds, ['credential:owner']); } - async findOwnedSharings(userIds: string[]) { - return await this.find({ + /** Get the IDs of all credentials owned by or shared with a user */ + async getAccessibleCredentialIds(userIds: string[]) { + return await this.getCredentialIdsByUserAndRole(userIds, [ + 'credential:owner', + 'credential:user', + ]); + } + + private async getCredentialIdsByUserAndRole(userIds: string[], roles: CredentialSharingRole[]) { + const sharings = await this.find({ where: { userId: In(userIds), - role: 'credential:owner', + role: In(roles), }, }); + return sharings.map((s) => s.credentialsId); } async deleteByIds(transaction: EntityManager, sharedCredentialsIds: string[], user?: User) { diff --git a/packages/cli/src/databases/repositories/sharedWorkflow.repository.ts b/packages/cli/src/databases/repositories/sharedWorkflow.repository.ts index e3d321cab4..a457efb5a8 100644 --- a/packages/cli/src/databases/repositories/sharedWorkflow.repository.ts +++ b/packages/cli/src/databases/repositories/sharedWorkflow.repository.ts @@ -22,6 +22,15 @@ export class SharedWorkflowRepository extends Repository { return await this.exist({ where }); } + /** Get the IDs of all users this workflow is shared with */ + async getSharedUserIds(workflowId: string) { + const sharedWorkflows = await this.find({ + select: ['userId'], + where: { workflowId }, + }); + return sharedWorkflows.map((sharing) => sharing.userId); + } + async getSharedWorkflowIds(workflowIds: string[]) { const sharedWorkflows = await this.find({ select: ['workflowId'], diff --git a/packages/cli/test/integration/PermissionChecker.test.ts b/packages/cli/test/integration/PermissionChecker.test.ts new file mode 100644 index 0000000000..b8035926e8 --- /dev/null +++ b/packages/cli/test/integration/PermissionChecker.test.ts @@ -0,0 +1,385 @@ +import { v4 as uuid } from 'uuid'; +import { Container } from 'typedi'; +import type { WorkflowSettings } from 'n8n-workflow'; +import { SubworkflowOperationError, Workflow } from 'n8n-workflow'; + +import config from '@/config'; +import { User } from '@db/entities/User'; +import { WorkflowRepository } from '@db/repositories/workflow.repository'; +import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; +import { UserRepository } from '@/databases/repositories/user.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, + randomPositiveDigit, +} from '../integration/shared/random'; +import { LicenseMocker } from '../integration/shared/license'; +import * as testDb from '../integration/shared/testDb'; +import type { SaveCredentialFunction } from '../integration/shared/types'; +import { mockNodeTypesData } from '../unit/Helpers'; +import { affixRoleToSaveCredential } from '../integration/shared/db/credentials'; +import { createOwner, createUser } from '../integration/shared/db/users'; + +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 } : {}), + }, + }); +} + +let saveCredential: SaveCredentialFunction; + +const mockNodeTypes = mockInstance(NodeTypes); +mockInstance(LoadNodesAndCredentials, { + loadedNodes: mockNodeTypesData(['start', 'actionNetwork']), +}); + +let permissionChecker: PermissionChecker; + +beforeAll(async () => { + await testDb.init(); + + saveCredential = affixRoleToSaveCredential('credential:owner'); + + permissionChecker = Container.get(PermissionChecker); +}); + +describe('check()', () => { + beforeEach(async () => { + await testDb.truncate(['Workflow', 'Credentials']); + }); + + afterAll(async () => { + await testDb.terminate(); + }); + + test('should allow if workflow has no creds', async () => { + const userId = uuid(); + + const workflow = new Workflow({ + id: randomPositiveDigit().toString(), + name: 'test', + active: false, + connections: {}, + nodeTypes: mockNodeTypes, + nodes: [ + { + id: uuid(), + name: 'Start', + type: 'n8n-nodes-base.start', + typeVersion: 1, + parameters: {}, + position: [0, 0], + }, + ], + }); + + expect(async () => await permissionChecker.check(workflow, userId)).not.toThrow(); + }); + + test('should allow if requesting user is instance owner', async () => { + const owner = await createOwner(); + + const workflow = new Workflow({ + id: randomPositiveDigit().toString(), + name: 'test', + active: false, + connections: {}, + nodeTypes: mockNodeTypes, + nodes: [ + { + id: uuid(), + name: 'Action Network', + type: 'n8n-nodes-base.actionNetwork', + parameters: {}, + typeVersion: 1, + position: [0, 0], + credentials: { + actionNetworkApi: { + id: randomPositiveDigit().toString(), + name: 'Action Network Account', + }, + }, + }, + ], + }); + + expect(async () => await permissionChecker.check(workflow, owner.id)).not.toThrow(); + }); + + test('should allow if workflow creds are valid subset', async () => { + const [owner, member] = await Promise.all([createOwner(), createUser()]); + + const ownerCred = await saveCredential(randomCred(), { user: owner }); + const memberCred = await saveCredential(randomCred(), { user: member }); + + const workflow = new Workflow({ + id: randomPositiveDigit().toString(), + name: 'test', + active: false, + connections: {}, + nodeTypes: mockNodeTypes, + nodes: [ + { + id: uuid(), + name: 'Action Network', + type: 'n8n-nodes-base.actionNetwork', + parameters: {}, + typeVersion: 1, + position: [0, 0], + credentials: { + actionNetworkApi: { + id: ownerCred.id, + name: ownerCred.name, + }, + }, + }, + { + id: uuid(), + name: 'Action Network 2', + type: 'n8n-nodes-base.actionNetwork', + parameters: {}, + typeVersion: 1, + position: [0, 0], + credentials: { + actionNetworkApi: { + id: memberCred.id, + name: memberCred.name, + }, + }, + }, + ], + }); + + expect(async () => await permissionChecker.check(workflow, owner.id)).not.toThrow(); + }); + + test('should deny if workflow creds are not valid subset', async () => { + const member = await createUser(); + + const memberCred = await saveCredential(randomCred(), { user: member }); + + const workflowDetails = { + id: randomPositiveDigit().toString(), + name: 'test', + active: false, + connections: {}, + nodeTypes: mockNodeTypes, + nodes: [ + { + id: uuid(), + name: 'Action Network', + type: 'n8n-nodes-base.actionNetwork', + parameters: {}, + typeVersion: 1, + position: [0, 0] as [number, number], + credentials: { + actionNetworkApi: { + id: memberCred.id, + name: memberCred.name, + }, + }, + }, + { + id: uuid(), + name: 'Action Network 2', + type: 'n8n-nodes-base.actionNetwork', + parameters: {}, + typeVersion: 1, + position: [0, 0] as [number, number], + credentials: { + actionNetworkApi: { + id: 'non-existing-credential-id', + name: 'Non-existing credential name', + }, + }, + }, + ], + }; + + const workflowEntity = await Container.get(WorkflowRepository).save(workflowDetails); + + await Container.get(SharedWorkflowRepository).save({ + workflow: workflowEntity, + user: member, + role: 'workflow:owner', + }); + + const workflow = new Workflow(workflowDetails); + + await expect(permissionChecker.check(workflow, member.id)).rejects.toThrow(); + }); +}); + +describe('checkSubworkflowExecutePolicy()', () => { + const ownershipService = mockInstance(OwnershipService); + + 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.getWorkflowOwnerCached.mockResolvedValue(new User()); + + 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 firstUser = Container.get(UserRepository).create({ id: uuid() }); + const secondUser = Container.get(UserRepository).create({ id: uuid() }); + + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(firstUser); // parent workflow + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(secondUser); // 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( + `${firstUser.firstName} (${firstUser.email}) can make this change. You may need to tell them the ID of this 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.getWorkflowOwnerCached.mockResolvedValue(new User()); + + 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 parentWorkflowOwner = Container.get(UserRepository).create({ id: uuid() }); + const subworkflowOwner = Container.get(UserRepository).create({ id: uuid() }); + + ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(parentWorkflowOwner); // parent workflow + ownershipService.getWorkflowOwnerCached.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 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(); + }); + }); +}); diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index 5eb4b6e0ea..7e3336230c 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -1,385 +1,137 @@ -import { v4 as uuid } from 'uuid'; -import { Container } from 'typedi'; -import type { WorkflowSettings } from 'n8n-workflow'; -import { SubworkflowOperationError, Workflow } from 'n8n-workflow'; - -import config from '@/config'; -import { User } from '@db/entities/User'; -import { WorkflowRepository } from '@db/repositories/workflow.repository'; -import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; -import { UserRepository } from '@/databases/repositories/user.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 { type INodeTypes, Workflow } from 'n8n-workflow'; +import { mock } from 'jest-mock-extended'; +import type { User } from '@db/entities/User'; +import type { UserRepository } from '@db/repositories/user.repository'; +import type { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; +import type { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; +import type { License } from '@/License'; import { PermissionChecker } from '@/UserManagement/PermissionChecker'; -import { mockInstance } from '../shared/mocking'; -import { - randomCredentialPayload as randomCred, - randomName, - randomPositiveDigit, -} from '../integration/shared/random'; -import { LicenseMocker } from '../integration/shared/license'; -import * as testDb from '../integration/shared/testDb'; -import type { SaveCredentialFunction } from '../integration/shared/types'; -import { mockNodeTypesData } from './Helpers'; -import { affixRoleToSaveCredential } from '../integration/shared/db/credentials'; -import { createOwner, createUser } from '../integration/shared/db/users'; +describe('PermissionChecker', () => { + const user = mock(); + const userRepo = mock(); + const sharedCredentialsRepo = mock(); + const sharedWorkflowRepo = mock(); + const license = mock(); + const permissionChecker = new PermissionChecker( + userRepo, + sharedCredentialsRepo, + sharedWorkflowRepo, + mock(), + license, + ); -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(), + const workflow = new Workflow({ + id: '1', + name: 'test', active: false, connections: {}, + nodeTypes: mock(), nodes: [ { - name: '', - typeVersion: 1, - type: 'n8n-nodes-base.executeWorkflow', - position: [0, 0], + id: 'node-id', + name: 'HTTP Request', + type: 'n8n-nodes-base.httpRequest', parameters: {}, + typeVersion: 1, + position: [0, 0], + credentials: { + oAuth2Api: { + id: 'cred-id', + name: 'Custom oAuth2', + }, + }, }, ], }); -} -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 } : {}), - }, - }); -} + beforeEach(() => jest.clearAllMocks()); -let saveCredential: SaveCredentialFunction; - -const mockNodeTypes = mockInstance(NodeTypes); -mockInstance(LoadNodesAndCredentials, { - loadedNodes: mockNodeTypesData(['start', 'actionNetwork']), -}); - -let permissionChecker: PermissionChecker; - -beforeAll(async () => { - await testDb.init(); - - saveCredential = affixRoleToSaveCredential('credential:owner'); - - permissionChecker = Container.get(PermissionChecker); -}); - -describe('check()', () => { - beforeEach(async () => { - await testDb.truncate(['Workflow', 'Credentials']); - }); - - afterAll(async () => { - await testDb.terminate(); - }); - - test('should allow if workflow has no creds', async () => { - const userId = uuid(); - - const workflow = new Workflow({ - id: randomPositiveDigit().toString(), - name: 'test', - active: false, - connections: {}, - nodeTypes: mockNodeTypes, - nodes: [ - { - id: uuid(), - name: 'Start', - type: 'n8n-nodes-base.start', - typeVersion: 1, - parameters: {}, - position: [0, 0], - }, - ], + describe('check', () => { + it('should throw if no user is found', async () => { + userRepo.findOneOrFail.mockRejectedValue(new Error('Fail')); + await expect(permissionChecker.check(workflow, '123')).rejects.toThrow(); + expect(license.isSharingEnabled).not.toHaveBeenCalled(); + expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled(); + expect(sharedCredentialsRepo.getOwnedCredentialIds).not.toHaveBeenCalled(); + expect(sharedCredentialsRepo.getAccessibleCredentialIds).not.toHaveBeenCalled(); }); - expect(async () => await permissionChecker.check(workflow, userId)).not.toThrow(); - }); - - test('should allow if requesting user is instance owner', async () => { - const owner = await createOwner(); - - const workflow = new Workflow({ - id: randomPositiveDigit().toString(), - name: 'test', - active: false, - connections: {}, - nodeTypes: mockNodeTypes, - nodes: [ - { - id: uuid(), - name: 'Action Network', - type: 'n8n-nodes-base.actionNetwork', - parameters: {}, - typeVersion: 1, - position: [0, 0], - credentials: { - actionNetworkApi: { - id: randomPositiveDigit().toString(), - name: 'Action Network Account', - }, - }, - }, - ], + it('should allow a user if they have a global `workflow:execute` scope', async () => { + userRepo.findOneOrFail.mockResolvedValue(user); + user.hasGlobalScope.calledWith('workflow:execute').mockReturnValue(true); + await expect(permissionChecker.check(workflow, user.id)).resolves.not.toThrow(); + expect(license.isSharingEnabled).not.toHaveBeenCalled(); + expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled(); + expect(sharedCredentialsRepo.getOwnedCredentialIds).not.toHaveBeenCalled(); + expect(sharedCredentialsRepo.getAccessibleCredentialIds).not.toHaveBeenCalled(); }); - expect(async () => await permissionChecker.check(workflow, owner.id)).not.toThrow(); - }); - - test('should allow if workflow creds are valid subset', async () => { - const [owner, member] = await Promise.all([createOwner(), createUser()]); - - const ownerCred = await saveCredential(randomCred(), { user: owner }); - const memberCred = await saveCredential(randomCred(), { user: member }); - - const workflow = new Workflow({ - id: randomPositiveDigit().toString(), - name: 'test', - active: false, - connections: {}, - nodeTypes: mockNodeTypes, - nodes: [ - { - id: uuid(), - name: 'Action Network', - type: 'n8n-nodes-base.actionNetwork', - parameters: {}, - typeVersion: 1, - position: [0, 0], - credentials: { - actionNetworkApi: { - id: ownerCred.id, - name: ownerCred.name, - }, - }, - }, - { - id: uuid(), - name: 'Action Network 2', - type: 'n8n-nodes-base.actionNetwork', - parameters: {}, - typeVersion: 1, - position: [0, 0], - credentials: { - actionNetworkApi: { - id: memberCred.id, - name: memberCred.name, - }, - }, - }, - ], - }); - - expect(async () => await permissionChecker.check(workflow, owner.id)).not.toThrow(); - }); - - test('should deny if workflow creds are not valid subset', async () => { - const member = await createUser(); - - const memberCred = await saveCredential(randomCred(), { user: member }); - - const workflowDetails = { - id: randomPositiveDigit().toString(), - name: 'test', - active: false, - connections: {}, - nodeTypes: mockNodeTypes, - nodes: [ - { - id: uuid(), - name: 'Action Network', - type: 'n8n-nodes-base.actionNetwork', - parameters: {}, - typeVersion: 1, - position: [0, 0] as [number, number], - credentials: { - actionNetworkApi: { - id: memberCred.id, - name: memberCred.name, - }, - }, - }, - { - id: uuid(), - name: 'Action Network 2', - type: 'n8n-nodes-base.actionNetwork', - parameters: {}, - typeVersion: 1, - position: [0, 0] as [number, number], - credentials: { - actionNetworkApi: { - id: 'non-existing-credential-id', - name: 'Non-existing credential name', - }, - }, - }, - ], - }; - - const workflowEntity = await Container.get(WorkflowRepository).save(workflowDetails); - - await Container.get(SharedWorkflowRepository).save({ - workflow: workflowEntity, - user: member, - role: 'workflow:owner', - }); - - const workflow = new Workflow(workflowDetails); - - await expect(permissionChecker.check(workflow, member.id)).rejects.toThrow(); - }); -}); - -describe('checkSubworkflowExecutePolicy()', () => { - const ownershipService = mockInstance(OwnershipService); - - 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.getWorkflowOwnerCached.mockResolvedValue(new User()); - - 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 firstUser = Container.get(UserRepository).create({ id: uuid() }); - const secondUser = Container.get(UserRepository).create({ id: uuid() }); - - ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(firstUser); // parent workflow - ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(secondUser); // 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( - `${firstUser.firstName} (${firstUser.email}) can make this change. You may need to tell them the ID of this 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}`, + describe('When sharing is disabled', () => { + beforeEach(() => { + userRepo.findOneOrFail.mockResolvedValue(user); + user.hasGlobalScope.calledWith('workflow:execute').mockReturnValue(false); + license.isSharingEnabled.mockReturnValue(false); }); - const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + it('should validate credential access using only owned credentials', async () => { + sharedCredentialsRepo.getOwnedCredentialIds.mockResolvedValue(['cred-id']); - await expect(check).resolves.not.toThrow(); - }); + await expect(permissionChecker.check(workflow, user.id)).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', + expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled(); + expect(sharedCredentialsRepo.getOwnedCredentialIds).toBeCalledWith([user.id]); + expect(sharedCredentialsRepo.getAccessibleCredentialIds).not.toHaveBeenCalled(); }); - const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + it('should throw when the user does not have access to the credential', async () => { + sharedCredentialsRepo.getOwnedCredentialIds.mockResolvedValue(['cred-id2']); - await expect(check).rejects.toThrow(); - }); - }); + await expect(permissionChecker.check(workflow, user.id)).rejects.toThrow( + 'Node has no access to credential', + ); - describe('any caller policy', () => { - test('should not throw', async () => { - const parentWorkflow = createParentWorkflow(); - const subworkflow = createSubworkflow({ policy: 'any' }); - ownershipService.getWorkflowOwnerCached.mockResolvedValue(new User()); - - 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 parentWorkflowOwner = Container.get(UserRepository).create({ id: uuid() }); - const subworkflowOwner = Container.get(UserRepository).create({ id: uuid() }); - - ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(parentWorkflowOwner); // parent workflow - ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(subworkflowOwner); // subworkflow - - const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' }); - - const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, uuid()); - - await expect(check).rejects.toThrow(toTargetCallErrorMsg(subworkflow.id)); + expect(sharedWorkflowRepo.getSharedUserIds).not.toBeCalled(); + expect(sharedCredentialsRepo.getOwnedCredentialIds).toBeCalledWith([user.id]); + expect(sharedCredentialsRepo.getAccessibleCredentialIds).not.toHaveBeenCalled(); + }); }); - test('should allow if both workflows are owned by the same user', async () => { - const parentWorkflow = createParentWorkflow(); + describe('When sharing is enabled', () => { + beforeEach(() => { + userRepo.findOneOrFail.mockResolvedValue(user); + user.hasGlobalScope.calledWith('workflow:execute').mockReturnValue(false); + license.isSharingEnabled.mockReturnValue(true); + sharedWorkflowRepo.getSharedUserIds.mockResolvedValue([user.id, 'another-user']); + }); - const bothWorkflowsOwner = Container.get(UserRepository).create({ id: uuid() }); + it('should validate credential access using only owned credentials', async () => { + sharedCredentialsRepo.getAccessibleCredentialIds.mockResolvedValue(['cred-id']); - ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(bothWorkflowsOwner); // parent workflow - ownershipService.getWorkflowOwnerCached.mockResolvedValueOnce(bothWorkflowsOwner); // subworkflow + await expect(permissionChecker.check(workflow, user.id)).resolves.not.toThrow(); - const subworkflow = createSubworkflow({ policy: 'workflowsFromSameOwner' }); + expect(sharedWorkflowRepo.getSharedUserIds).toBeCalledWith(workflow.id); + expect(sharedCredentialsRepo.getAccessibleCredentialIds).toBeCalledWith([ + user.id, + 'another-user', + ]); + expect(sharedCredentialsRepo.getOwnedCredentialIds).not.toHaveBeenCalled(); + }); - const check = permissionChecker.checkSubworkflowExecutePolicy(subworkflow, parentWorkflow.id); + it('should throw when the user does not have access to the credential', async () => { + sharedCredentialsRepo.getAccessibleCredentialIds.mockResolvedValue(['cred-id2']); - await expect(check).resolves.not.toThrow(); + await expect(permissionChecker.check(workflow, user.id)).rejects.toThrow( + 'Node has no access to credential', + ); + + expect(sharedWorkflowRepo.find).not.toBeCalled(); + expect(sharedCredentialsRepo.getAccessibleCredentialIds).toBeCalledWith([ + user.id, + 'another-user', + ]); + expect(sharedCredentialsRepo.getOwnedCredentialIds).not.toHaveBeenCalled(); + }); }); }); });