From 6dd7797c39c052f5006d7fbaffe4d38b8cccad27 Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Thu, 11 Sep 2025 09:21:40 +0200 Subject: [PATCH] refactor(core): Store projectId on additionalData independent of dataStore context (no-changelog) (#19093) --- .../dynamic-node-parameters.controller.ts | 43 ++++--------------- .../data-table/data-store-proxy.service.ts | 8 ++-- .../dynamic-node-parameters.service.ts | 22 ++++++++++ packages/core/src/execution-engine/index.ts | 5 ++- .../utils/data-store-helper-functions.ts | 4 +- packages/workflow/src/interfaces.ts | 4 +- 6 files changed, 43 insertions(+), 43 deletions(-) diff --git a/packages/cli/src/controllers/dynamic-node-parameters.controller.ts b/packages/cli/src/controllers/dynamic-node-parameters.controller.ts index 386817da34..650f9a052d 100644 --- a/packages/cli/src/controllers/dynamic-node-parameters.controller.ts +++ b/packages/cli/src/controllers/dynamic-node-parameters.controller.ts @@ -10,7 +10,6 @@ import type { INodePropertyOptions, NodeParameterValueType } from 'n8n-workflow' import { DynamicNodeParametersService } from '@/services/dynamic-node-parameters.service'; import { getBase } from '@/workflow-execute-additional-data'; -import { userHasScopes } from '@/permissions.ee/check-access'; @RestController('/dynamic-node-parameters') export class DynamicNodeParametersController { @@ -22,6 +21,8 @@ export class DynamicNodeParametersController { _res: Response, @Body payload: OptionsRequestDto, ): Promise { + await this.service.scrubInaccessibleProjectId(req.user, payload); + const { credentials, currentNodeParameters, @@ -33,17 +34,7 @@ export class DynamicNodeParametersController { } = payload; const additionalData = await getBase(req.user.id, currentNodeParameters); - - if (projectId) { - if (await userHasScopes(req.user, ['dataStore:listProject'], false, { projectId })) { - // Project ID is currently only added on the additionalData if the user - // has data store listing permission for that project. We should consider - // turning this into a more general check, but as of now data stores are - // the only nodes with project specific resource locators where we want to ensure - // that only data stores belonging to their respective projects are shown. - additionalData.dataStoreProjectId = projectId; - } - } + additionalData.dataTableProjectId = projectId; if (methodName) { return await this.service.getOptionsViaMethodName( @@ -75,6 +66,8 @@ export class DynamicNodeParametersController { _res: Response, @Body payload: ResourceLocatorRequestDto, ) { + await this.service.scrubInaccessibleProjectId(req.user, payload); + const { path, methodName, @@ -87,17 +80,7 @@ export class DynamicNodeParametersController { } = payload; const additionalData = await getBase(req.user.id, currentNodeParameters); - - if (projectId) { - if (await userHasScopes(req.user, ['dataStore:listProject'], false, { projectId })) { - // Project ID is currently only added on the additionalData if the user - // has data store listing permission for that project. We should consider - // turning this into a more general check, but as of now data stores are - // the only nodes with project specific resource locators where we want to ensure - // that only data stores belonging to their respective projects are shown. - additionalData.dataStoreProjectId = projectId; - } - } + additionalData.dataTableProjectId = projectId; return await this.service.getResourceLocatorResults( methodName, @@ -117,21 +100,13 @@ export class DynamicNodeParametersController { _res: Response, @Body payload: ResourceMapperFieldsRequestDto, ) { + await this.service.scrubInaccessibleProjectId(req.user, payload); + const { path, methodName, credentials, currentNodeParameters, nodeTypeAndVersion, projectId } = payload; const additionalData = await getBase(req.user.id, currentNodeParameters); - - if (projectId) { - if (await userHasScopes(req.user, ['dataStore:listProject'], false, { projectId })) { - // Project ID is currently only added on the additionalData if the user - // has data store listing permission for that project. We should consider - // turning this into a more general check, but as of now data stores are - // the only nodes with project specific resource locators where we want to ensure - // that only data stores belonging to their respective projects are shown. - additionalData.dataStoreProjectId = projectId; - } - } + additionalData.dataTableProjectId = projectId; return await this.service.getResourceMappingFields( methodName, diff --git a/packages/cli/src/modules/data-table/data-store-proxy.service.ts b/packages/cli/src/modules/data-table/data-store-proxy.service.ts index 24ac2fffc3..ad405794df 100644 --- a/packages/cli/src/modules/data-table/data-store-proxy.service.ts +++ b/packages/cli/src/modules/data-table/data-store-proxy.service.ts @@ -49,10 +49,10 @@ export class DataStoreProxyService implements DataStoreProxyProvider { async getDataStoreAggregateProxy( workflow: Workflow, node: INode, - dataStoreProjectId?: string, + projectId?: string, ): Promise { this.validateRequest(node); - const projectId = dataStoreProjectId ?? (await this.getProjectId(workflow)); + projectId = projectId ?? (await this.getProjectId(workflow)); return this.makeAggregateOperations(projectId); } @@ -61,10 +61,10 @@ export class DataStoreProxyService implements DataStoreProxyProvider { workflow: Workflow, node: INode, dataStoreId: string, - dataStoreProjectId?: string, + projectId?: string, ): Promise { this.validateRequest(node); - const projectId = dataStoreProjectId ?? (await this.getProjectId(workflow)); + projectId = projectId ?? (await this.getProjectId(workflow)); return this.makeDataStoreOperations(projectId, dataStoreId); } diff --git a/packages/cli/src/services/dynamic-node-parameters.service.ts b/packages/cli/src/services/dynamic-node-parameters.service.ts index a060d09ffc..e4267c327c 100644 --- a/packages/cli/src/services/dynamic-node-parameters.service.ts +++ b/packages/cli/src/services/dynamic-node-parameters.service.ts @@ -26,6 +26,9 @@ import { Workflow, UnexpectedError } from 'n8n-workflow'; import { NodeTypes } from '@/node-types'; import { WorkflowLoaderService } from './workflow-loader.service'; +import { User } from '@n8n/db'; +import { userHasScopes } from '@/permissions.ee/check-access'; +import { Logger } from '@n8n/backend-common'; type LocalResourceMappingMethod = ( this: ILocalLoadOptionsFunctions, @@ -52,10 +55,29 @@ type NodeMethod = @Service() export class DynamicNodeParametersService { constructor( + private logger: Logger, private nodeTypes: NodeTypes, private workflowLoaderService: WorkflowLoaderService, ) {} + async scrubInaccessibleProjectId(user: User, payload: { projectId?: string }) { + // 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 dataStore:listProject scope as this is the existing consumer of + // the project id + if ( + payload.projectId && + !(await userHasScopes(user, ['dataStore:listProject'], false, { + projectId: payload.projectId, + })) + ) { + this.logger.warn( + `Scrubbed inaccessible projectId ${payload.projectId} from DynamicNodeParameters request`, + ); + payload.projectId = undefined; + } + } + /** Returns the available options via a predefined method */ async getOptionsViaMethodName( methodName: string, diff --git a/packages/core/src/execution-engine/index.ts b/packages/core/src/execution-engine/index.ts index c42052b710..d0b44d9216 100644 --- a/packages/core/src/execution-engine/index.ts +++ b/packages/core/src/execution-engine/index.ts @@ -8,7 +8,10 @@ declare module 'n8n-workflow' { hooks?: ExecutionLifecycleHooks; externalSecretsProxy: ExternalSecretsProxy; 'data-table'?: { dataStoreProxyProvider: DataStoreProxyProvider }; - dataStoreProjectId?: string; + // Project ID is currently only added on the additionalData if the user + // has data table listing permission for that project. We should consider + // that only data tables belonging to their respective projects are shown. + dataTableProjectId?: string; } } diff --git a/packages/core/src/execution-engine/node-execution-context/utils/data-store-helper-functions.ts b/packages/core/src/execution-engine/node-execution-context/utils/data-store-helper-functions.ts index 4c7d57d808..e782e2168d 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/data-store-helper-functions.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/data-store-helper-functions.ts @@ -17,14 +17,14 @@ export function getDataStoreHelperFunctions( await dataStoreProxyProvider.getDataStoreAggregateProxy( workflow, node, - additionalData.dataStoreProjectId, + additionalData.dataTableProjectId, ), getDataStoreProxy: async (dataStoreId: string) => await dataStoreProxyProvider.getDataStoreProxy( workflow, node, dataStoreId, - additionalData.dataStoreProjectId, + additionalData.dataTableProjectId, ), }; } diff --git a/packages/workflow/src/interfaces.ts b/packages/workflow/src/interfaces.ts index 35568b4837..8b900e7bec 100644 --- a/packages/workflow/src/interfaces.ts +++ b/packages/workflow/src/interfaces.ts @@ -924,13 +924,13 @@ export type DataStoreProxyProvider = { getDataStoreAggregateProxy( workflow: Workflow, node: INode, - dataStoreProjectId?: string, + projectId?: string, ): Promise; getDataStoreProxy( workflow: Workflow, node: INode, dataStoreId: string, - dataStoreProjectId?: string, + projectId?: string, ): Promise; };