diff --git a/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts b/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts index 6e108b02fa4..41c6fecab77 100644 --- a/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts +++ b/packages/cli/src/services/__tests__/dynamic-node-parameters.service.test.ts @@ -11,19 +11,25 @@ import { import { DynamicNodeParametersService } from '../dynamic-node-parameters.service'; import { WorkflowLoaderService } from '../workflow-loader.service'; +import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NodeTypes } from '@/node-types'; +import * as checkAccess from '@/permissions.ee/check-access'; import { SharedWorkflowRepository } from '@n8n/db'; +import type { User } from '@n8n/db'; describe('DynamicNodeParametersService', () => { const logger = mockInstance(Logger); const nodeTypes = mockInstance(NodeTypes); const workflowLoaderService = mockInstance(WorkflowLoaderService); const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository); + const credentialsFinderService = mockInstance(CredentialsFinderService); const service = new DynamicNodeParametersService( logger, nodeTypes, workflowLoaderService, sharedWorkflowRepository, + credentialsFinderService, ); beforeEach(() => { @@ -124,4 +130,132 @@ describe('DynamicNodeParametersService', () => { }); }); }); + + describe('refineResourceIds', () => { + const user = mock(); + + beforeEach(() => { + jest.spyOn(checkAccess, 'userHasScopes').mockResolvedValue(true); + sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue(undefined); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should not call findCredentialIdsWithScopeForUser when no credentials provided', async () => { + await service.refineResourceIds(user, { credentials: undefined }); + expect(credentialsFinderService.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled(); + }); + + it('should not call findCredentialIdsWithScopeForUser when credentials object is empty', async () => { + await service.refineResourceIds(user, { credentials: {} }); + expect(credentialsFinderService.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled(); + }); + + it('should not call findCredentialIdsWithScopeForUser when all credential entries have no id', async () => { + await service.refineResourceIds(user, { + credentials: { openAi: { id: null, name: 'My OpenAI' } }, + }); + expect(credentialsFinderService.findCredentialIdsWithScopeForUser).not.toHaveBeenCalled(); + }); + + it('should allow request when user has access to all supplied credentials', async () => { + credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue( + new Set(['cred-1']), + ); + + await expect( + service.refineResourceIds(user, { + credentials: { openAi: { id: 'cred-1', name: 'My OpenAI' } }, + }), + ).resolves.not.toThrow(); + }); + + it('should throw when user does not have access to a credential', async () => { + credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(new Set()); + + await expect( + service.refineResourceIds(user, { + credentials: { openAi: { id: 'cred-1', name: 'Foreign OpenAI' } }, + }), + ).rejects.toThrow(ForbiddenError); + }); + + it('should throw when any credential in a multi-credential request is inaccessible', async () => { + credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue( + new Set(['cred-1']), + ); + + await expect( + service.refineResourceIds(user, { + credentials: { + openAi: { id: 'cred-1', name: 'My OpenAI' }, + openAi2: { id: 'cred-2', name: 'Foreign OpenAI' }, + }, + }), + ).rejects.toThrow(ForbiddenError); + }); + + it('should allow request when all of multiple credentials are accessible', async () => { + credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue( + new Set(['cred-1', 'cred-2']), + ); + + await expect( + service.refineResourceIds(user, { + credentials: { + openAi: { id: 'cred-1', name: 'My OpenAI' }, + openAi2: { id: 'cred-2', name: 'My OpenAI 2' }, + }, + }), + ).resolves.not.toThrow(); + }); + + it('should skip credential entries without an id when validating', async () => { + credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue( + new Set(['cred-1']), + ); + + await service.refineResourceIds(user, { + credentials: { + anon: { id: null, name: 'anonymous' }, + named: { id: 'cred-1', name: 'Named Credential' }, + }, + }); + + expect(credentialsFinderService.findCredentialIdsWithScopeForUser).toHaveBeenCalledWith( + ['cred-1'], + user, + ['credential:read'], + ); + }); + + it('should call findCredentialIdsWithScopeForUser with credential:read scope', async () => { + credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue( + new Set(['cred-1']), + ); + + await service.refineResourceIds(user, { + credentials: { openAi: { id: 'cred-1', name: 'My OpenAI' } }, + }); + + expect(credentialsFinderService.findCredentialIdsWithScopeForUser).toHaveBeenCalledWith( + expect.any(Array), + user, + ['credential:read'], + ); + }); + + it('should run credential check regardless of whether workflowId is present', async () => { + credentialsFinderService.findCredentialIdsWithScopeForUser.mockResolvedValue(new Set()); + + await expect( + service.refineResourceIds(user, { + credentials: { openAi: { id: 'cred-1', name: 'Foreign OpenAI' } }, + // workflowId intentionally omitted + }), + ).rejects.toThrow(ForbiddenError); + }); + }); }); diff --git a/packages/cli/src/services/dynamic-node-parameters.service.ts b/packages/cli/src/services/dynamic-node-parameters.service.ts index 1c8c43a3262..c5c17b1f51e 100644 --- a/packages/cli/src/services/dynamic-node-parameters.service.ts +++ b/packages/cli/src/services/dynamic-node-parameters.service.ts @@ -23,6 +23,8 @@ import type { import { Workflow, UnexpectedError, createEmptyRunExecutionData } from 'n8n-workflow'; import { NodeTypes } from '@/node-types'; +import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { WorkflowLoaderService } from './workflow-loader.service'; import { SharedWorkflowRepository, User } from '@n8n/db'; @@ -58,9 +60,13 @@ export class DynamicNodeParametersService { private nodeTypes: NodeTypes, private workflowLoaderService: WorkflowLoaderService, private sharedWorkflowRepository: SharedWorkflowRepository, + private credentialsFinderService: CredentialsFinderService, ) {} - async refineResourceIds(user: User, payload: { projectId?: string; workflowId?: string }) { + async refineResourceIds( + user: User, + payload: { projectId?: string; workflowId?: string; credentials?: INodeCredentials }, + ) { // We want to avoid relying on generic project:read permissions to enable // a future with fine-grained permission control dependent on the respective resource // For now we use the dataTable:listProject scope as this is the existing consumer of @@ -77,25 +83,41 @@ export class DynamicNodeParametersService { payload.projectId = undefined; } - if (!payload.workflowId) return; + if (payload.workflowId) { + const hasAccess = await userHasScopes(user, ['workflow:read'], false, { + workflowId: payload.workflowId, + }); - const hasAccess = await userHasScopes(user, ['workflow:read'], false, { - workflowId: payload.workflowId, - }); - - if (!hasAccess) { - this.logger.warn( - `Scrubbed inaccessible workflowId ${payload.workflowId} from DynamicNodeParameters request`, - ); - payload.workflowId = undefined; - return; + if (!hasAccess) { + this.logger.warn( + `Scrubbed inaccessible workflowId ${payload.workflowId} from DynamicNodeParameters request`, + ); + payload.workflowId = undefined; + } else if (payload.projectId === undefined) { + const project = await this.sharedWorkflowRepository.getWorkflowOwningProject( + payload.workflowId, + ); + payload.projectId = project?.id; + } } - if (payload.projectId === undefined) { - const project = await this.sharedWorkflowRepository.getWorkflowOwningProject( - payload.workflowId, - ); - payload.projectId = project?.id; + if (payload.credentials) { + const credentialIds = Object.values(payload.credentials) + .map((details) => details.id) + .filter((id): id is string => id !== undefined && id !== null); + + if (credentialIds.length > 0) { + const accessibleIds = await this.credentialsFinderService.findCredentialIdsWithScopeForUser( + credentialIds, + user, + ['credential:read'], + ); + + const forbiddenId = credentialIds.find((id) => !accessibleIds.has(id)); + if (forbiddenId !== undefined) { + throw new ForbiddenError(); + } + } } } diff --git a/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts b/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts index dd70bcdf254..bb72f7ed32a 100644 --- a/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts +++ b/packages/cli/test/integration/controllers/dynamic-node-parameters.controller.test.ts @@ -1,4 +1,5 @@ -import { mockInstance } from '@n8n/backend-test-utils'; +import { randomCredentialPayload, testDb } from '@n8n/backend-test-utils'; +import { Container } from '@n8n/di'; import { mock } from 'jest-mock-extended'; import type { INodeListSearchResult, @@ -10,20 +11,20 @@ import type { import { DynamicNodeParametersService } from '@/services/dynamic-node-parameters.service'; import * as AdditionalData from '@/workflow-execute-additional-data'; -import { createOwner } from '../shared/db/users'; +import { saveCredential } from '../shared/db/credentials'; +import { createMember, createOwner } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; import { setupTestServer } from '../shared/utils'; describe('DynamicNodeParametersController', () => { const additionalData = mock(); - const service = mockInstance(DynamicNodeParametersService); const testServer = setupTestServer({ endpointGroups: ['dynamic-node-parameters'] }); let ownerAgent: SuperAgentTest; + let service: DynamicNodeParametersService; - beforeAll(async () => { - const owner = await createOwner(); - ownerAgent = testServer.authAgentFor(owner); + beforeAll(() => { + service = Container.get(DynamicNodeParametersService); }); beforeEach(() => { @@ -31,6 +32,10 @@ describe('DynamicNodeParametersController', () => { jest.spyOn(AdditionalData, 'getBase').mockResolvedValue(additionalData); }); + afterEach(() => { + jest.restoreAllMocks(); + }); + const commonRequestParams = { credentials: {}, currentNodeParameters: {}, @@ -40,7 +45,10 @@ describe('DynamicNodeParametersController', () => { describe('POST /dynamic-node-parameters/options', () => { it('should take params via body', async () => { - service.getOptionsViaMethodName.mockResolvedValue([]); + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + + jest.spyOn(service, 'getOptionsViaMethodName').mockResolvedValue([]); await ownerAgent .post('/dynamic-node-parameters/options') @@ -52,8 +60,11 @@ describe('DynamicNodeParametersController', () => { }); it('should take params with loadOptions', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + const expectedResult = [{ name: 'Test Option', value: 'test' }]; - service.getOptionsViaLoadOptions.mockResolvedValue(expectedResult); + jest.spyOn(service, 'getOptionsViaLoadOptions').mockResolvedValue(expectedResult); const response = await ownerAgent .post('/dynamic-node-parameters/options') @@ -67,6 +78,9 @@ describe('DynamicNodeParametersController', () => { }); it('should return empty array when no method or loadOptions provided', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + const response = await ownerAgent .post('/dynamic-node-parameters/options') .send({ @@ -80,8 +94,11 @@ describe('DynamicNodeParametersController', () => { describe('POST /dynamic-node-parameters/resource-locator-results', () => { it('should return resource locator results', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + const expectedResult: INodeListSearchResult = { results: [] }; - service.getResourceLocatorResults.mockResolvedValue(expectedResult); + jest.spyOn(service, 'getResourceLocatorResults').mockResolvedValue(expectedResult); const response = await ownerAgent .post('/dynamic-node-parameters/resource-locator-results') @@ -97,8 +114,10 @@ describe('DynamicNodeParametersController', () => { }); it('should handle resource locator results without pagination', async () => { - const mockResults = mock(); - service.getResourceLocatorResults.mockResolvedValue(mockResults); + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + + jest.spyOn(service, 'getResourceLocatorResults').mockResolvedValue({ results: [] }); await ownerAgent .post('/dynamic-node-parameters/resource-locator-results') @@ -110,6 +129,9 @@ describe('DynamicNodeParametersController', () => { }); it('should return a 400 if methodName is not defined', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + await ownerAgent .post('/dynamic-node-parameters/resource-locator-results') .send(commonRequestParams) @@ -119,8 +141,11 @@ describe('DynamicNodeParametersController', () => { describe('POST /dynamic-node-parameters/resource-mapper-fields', () => { it('should return resource mapper fields', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + const expectedResult: ResourceMapperFields = { fields: [] }; - service.getResourceMappingFields.mockResolvedValue(expectedResult); + jest.spyOn(service, 'getResourceMappingFields').mockResolvedValue(expectedResult); const response = await ownerAgent .post('/dynamic-node-parameters/resource-mapper-fields') @@ -135,6 +160,9 @@ describe('DynamicNodeParametersController', () => { }); it('should return a 400 if methodName is not defined', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + await ownerAgent .post('/dynamic-node-parameters/resource-mapper-fields') .send(commonRequestParams) @@ -144,8 +172,11 @@ describe('DynamicNodeParametersController', () => { describe('POST /dynamic-node-parameters/local-resource-mapper-fields', () => { it('should return local resource mapper fields', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + const expectedResult: ResourceMapperFields = { fields: [] }; - service.getLocalResourceMappingFields.mockResolvedValue(expectedResult); + jest.spyOn(service, 'getLocalResourceMappingFields').mockResolvedValue(expectedResult); const response = await ownerAgent .post('/dynamic-node-parameters/local-resource-mapper-fields') @@ -159,6 +190,9 @@ describe('DynamicNodeParametersController', () => { }); it('should return a 400 if methodName is not defined', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + await ownerAgent .post('/dynamic-node-parameters/local-resource-mapper-fields') .send(commonRequestParams) @@ -168,8 +202,11 @@ describe('DynamicNodeParametersController', () => { describe('POST /dynamic-node-parameters/action-result', () => { it('should return action result with handler', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + const expectedResult: NodeParameterValueType = { test: true }; - service.getActionResult.mockResolvedValue(expectedResult); + jest.spyOn(service, 'getActionResult').mockResolvedValue(expectedResult); const response = await ownerAgent .post('/dynamic-node-parameters/action-result') @@ -184,6 +221,9 @@ describe('DynamicNodeParametersController', () => { }); it('should return a 400 if handler is not defined', async () => { + const owner = await createOwner(); + ownerAgent = testServer.authAgentFor(owner); + await ownerAgent .post('/dynamic-node-parameters/action-result') .send({ @@ -193,4 +233,128 @@ describe('DynamicNodeParametersController', () => { .expect(400); }); }); + + describe('credential access enforcement', () => { + let member: Awaited>; + let memberAgent: SuperAgentTest; + + beforeEach(async () => { + await testDb.truncate(['SharedCredentials', 'CredentialsEntity']); + member = await createMember(); + memberAgent = testServer.authAgentFor(member); + }); + + it('should return 403 when user does not have access to supplied credential on /options', async () => { + const owner = await createOwner(); + const ownerCredential = await saveCredential(randomCredentialPayload(), { + user: owner, + role: 'credential:owner', + }); + + await memberAgent + .post('/dynamic-node-parameters/options') + .send({ + ...commonRequestParams, + credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } }, + }) + .expect(403); + }); + + it('should return 403 when user does not have access to supplied credential on /resource-locator-results', async () => { + const owner = await createOwner(); + const ownerCredential = await saveCredential(randomCredentialPayload(), { + user: owner, + role: 'credential:owner', + }); + + await memberAgent + .post('/dynamic-node-parameters/resource-locator-results') + .send({ + ...commonRequestParams, + methodName: 'searchItems', + credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } }, + }) + .expect(403); + }); + + it('should return 403 when user does not have access to supplied credential on /resource-mapper-fields', async () => { + const owner = await createOwner(); + const ownerCredential = await saveCredential(randomCredentialPayload(), { + user: owner, + role: 'credential:owner', + }); + + await memberAgent + .post('/dynamic-node-parameters/resource-mapper-fields') + .send({ + ...commonRequestParams, + methodName: 'getFields', + credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } }, + }) + .expect(403); + }); + + it('should return 403 when user does not have access to supplied credential on /action-result', async () => { + const owner = await createOwner(); + const ownerCredential = await saveCredential(randomCredentialPayload(), { + user: owner, + role: 'credential:owner', + }); + + await memberAgent + .post('/dynamic-node-parameters/action-result') + .send({ + ...commonRequestParams, + handler: 'handleAction', + payload: {}, + credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } }, + }) + .expect(403); + }); + + it('should return 403 when workflowId is absent but credential is inaccessible', async () => { + const owner = await createOwner(); + const ownerCredential = await saveCredential(randomCredentialPayload(), { + user: owner, + role: 'credential:owner', + }); + + // No workflowId — ensures the early-return bypass path is also blocked + await memberAgent + .post('/dynamic-node-parameters/options') + .send({ + ...commonRequestParams, + credentials: { httpBasicAuth: { id: ownerCredential.id, name: ownerCredential.name } }, + }) + .expect(403); + }); + + it('should not return 403 when user has access to the supplied credential', async () => { + const ownCredential = await saveCredential(randomCredentialPayload(), { + user: member, + role: 'credential:owner', + }); + + jest.spyOn(service, 'getOptionsViaMethodName').mockResolvedValue([]); + + const { status } = await memberAgent.post('/dynamic-node-parameters/options').send({ + ...commonRequestParams, + methodName: 'testMethod', + credentials: { httpBasicAuth: { id: ownCredential.id, name: ownCredential.name } }, + }); + + expect(status).not.toBe(403); + }); + + it('should not return 403 when no credentials are supplied', async () => { + jest.spyOn(service, 'getOptionsViaMethodName').mockResolvedValue([]); + + const { status } = await memberAgent.post('/dynamic-node-parameters/options').send({ + ...commonRequestParams, + methodName: 'testMethod', + }); + + expect(status).not.toBe(403); + }); + }); });