From e23bcfd63aac7d06afe372fda08bf1df8fe8a11b Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Fri, 3 Oct 2025 17:47:52 +0200 Subject: [PATCH] feat(core): Make variable update DTO fields optional for patch and add validation (#20348) --- packages/@n8n/api-types/src/dto/index.ts | 1 + .../update-variable-request.dto.test.ts | 173 ++++++++++++++ .../api-types/src/dto/variables/base.dto.ts | 24 +- .../variables/update-variable-request.dto.ts | 11 + .../variables.service.integration.test.ts | 225 +++++++++++++++++- .../variables/variables.controller.ee.ts | 19 +- .../variables/variables.service.ee.ts | 31 ++- .../cli/test/integration/variables.test.ts | 2 +- 8 files changed, 448 insertions(+), 38 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/variables/__tests__/update-variable-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/variables/update-variable-request.dto.ts diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 29ecdd72da5..cc604724a5d 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -49,6 +49,7 @@ export { PushWorkFolderRequestDto } from './source-control/push-work-folder-requ export { CreateCredentialDto } from './credentials/create-credential.dto'; export { VariableListRequestDto } from './variables/variables-list-request.dto'; export { CreateVariableRequestDto } from './variables/create-variable-request.dto'; +export { UpdateVariableRequestDto } from './variables/update-variable-request.dto'; export { CredentialsGetOneRequestQuery } from './credentials/credentials-get-one-request.dto'; export { CredentialsGetManyRequestQuery } from './credentials/credentials-get-many-request.dto'; export { GenerateCredentialNameRequestQuery } from './credentials/generate-credential-name.dto'; diff --git a/packages/@n8n/api-types/src/dto/variables/__tests__/update-variable-request.dto.test.ts b/packages/@n8n/api-types/src/dto/variables/__tests__/update-variable-request.dto.test.ts new file mode 100644 index 00000000000..9dfd8727c3c --- /dev/null +++ b/packages/@n8n/api-types/src/dto/variables/__tests__/update-variable-request.dto.test.ts @@ -0,0 +1,173 @@ +import { UpdateVariableRequestDto } from '../update-variable-request.dto'; + +describe('UpdateVariableRequestDto', () => { + describe('Valid requests', () => { + test.each([ + { + name: 'valid request with all fields', + request: { + key: 'MY_VARIABLE', + type: 'string', + value: 'test value', + projectId: '2gQLpmP5V4wOY627', + }, + parsedResult: { + key: 'MY_VARIABLE', + type: 'string', + value: 'test value', + projectId: '2gQLpmP5V4wOY627', + }, + }, + { + name: 'valid request with only key', + request: { + key: 'MY_VARIABLE', + }, + parsedResult: { + key: 'MY_VARIABLE', + }, + }, + { + name: 'valid request with only value', + request: { + value: 'new value', + }, + parsedResult: { + value: 'new value', + }, + }, + { + name: 'valid request with only projectId', + request: { + projectId: '2gQLpmP5V4wOY627', + }, + parsedResult: { + projectId: '2gQLpmP5V4wOY627', + }, + }, + { + name: 'valid request with UUID projectId', + request: { + key: 'MY_VAR', + type: 'string', + value: 'value', + projectId: '550e8400-e29b-41d4-a716-446655440000', + }, + parsedResult: { + key: 'MY_VAR', + type: 'string', + value: 'value', + projectId: '550e8400-e29b-41d4-a716-446655440000', + }, + }, + { + name: 'valid request with key and value', + request: { + key: 'MY_VAR', + value: 'value', + }, + parsedResult: { + key: 'MY_VAR', + value: 'value', + }, + }, + { + name: 'valid request with all base validations', + request: { + key: 'A'.repeat(50), + type: 'string', + value: 'x'.repeat(1000), + projectId: 'proj', + }, + parsedResult: { + key: 'A'.repeat(50), + type: 'string', + value: 'x'.repeat(1000), + projectId: 'proj', + }, + }, + { + name: 'valid empty request (all fields optional)', + request: {}, + parsedResult: {}, + }, + ])('should validate $name', ({ request, parsedResult }) => { + const result = UpdateVariableRequestDto.safeParse(request); + expect(result.success).toBe(true); + if (parsedResult) { + expect(result.data).toMatchObject(parsedResult); + } + }); + }); + + describe('Invalid requests', () => { + test.each([ + { + name: 'projectId too long (37 characters)', + request: { + key: 'MY_VAR', + type: 'string', + value: 'value', + projectId: 'a'.repeat(37), + }, + expectedErrorPaths: ['projectId'], + }, + { + name: 'projectId is not a string', + request: { + key: 'MY_VAR', + type: 'string', + value: 'value', + projectId: 123, + }, + expectedErrorPaths: ['projectId'], + }, + { + name: 'invalid key', + request: { + key: 'INVALID-KEY', + type: 'string', + value: 'value', + projectId: 'project123', + }, + expectedErrorPaths: ['key'], + }, + { + name: 'invalid value length', + request: { + key: 'MY_VAR', + type: 'string', + value: 'x'.repeat(1001), + projectId: 'project123', + }, + expectedErrorPaths: ['value'], + }, + { + name: 'invalid type', + request: { + key: 'MY_VAR', + type: 'number', + value: 'value', + projectId: 'project123', + }, + expectedErrorPaths: ['type'], + }, + { + name: 'empty key', + request: { + key: '', + type: 'string', + value: 'value', + projectId: 'project123', + }, + expectedErrorPaths: ['key'], + }, + ])('should fail validation for $name', ({ request, expectedErrorPaths }) => { + const result = UpdateVariableRequestDto.safeParse(request); + const issuesPaths = new Set(result.error?.issues.map((issue) => issue.path[0])); + + expect(result.success).toBe(false); + expect(new Set(issuesPaths)).toEqual(new Set(expectedErrorPaths)); + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/variables/base.dto.ts b/packages/@n8n/api-types/src/dto/variables/base.dto.ts index ff31a138569..ce85c2d8d14 100644 --- a/packages/@n8n/api-types/src/dto/variables/base.dto.ts +++ b/packages/@n8n/api-types/src/dto/variables/base.dto.ts @@ -7,14 +7,20 @@ export const VALUE_MAX_LENGTH = 1000; export const TYPE_ENUM = ['string'] as const; export const TYPE_DEFAULT: (typeof TYPE_ENUM)[number] = 'string'; +export const variableKeySchema = z + .string() + .min(1, 'key must be at least 1 character long') + .max(KEY_MAX_LENGTH, 'key cannot be longer than 50 characters') + .regex(KEY_NAME_REGEX, 'key can only contain characters A-Za-z0-9_'); + +export const variableValueSchema = z + .string() + .max(VALUE_MAX_LENGTH, `value cannot be longer than ${VALUE_MAX_LENGTH} characters`); + +export const variableTypeSchema = z.enum(TYPE_ENUM).default(TYPE_DEFAULT); + export class BaseVariableRequestDto extends Z.class({ - key: z - .string() - .min(1, 'key must be at least 1 character long') - .max(KEY_MAX_LENGTH, 'key cannot be longer than 50 characters') - .regex(KEY_NAME_REGEX, 'key can only contain characters A-Za-z0-9_'), - type: z.enum(TYPE_ENUM).default(TYPE_DEFAULT), - value: z - .string() - .max(VALUE_MAX_LENGTH, `value cannot be longer than ${VALUE_MAX_LENGTH} characters`), + key: variableKeySchema, + type: variableTypeSchema, + value: variableValueSchema, }) {} diff --git a/packages/@n8n/api-types/src/dto/variables/update-variable-request.dto.ts b/packages/@n8n/api-types/src/dto/variables/update-variable-request.dto.ts new file mode 100644 index 00000000000..faea9404a51 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/variables/update-variable-request.dto.ts @@ -0,0 +1,11 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +import { variableKeySchema, variableTypeSchema, variableValueSchema } from './base.dto'; + +export class UpdateVariableRequestDto extends Z.class({ + key: variableKeySchema.optional(), + type: variableTypeSchema.optional(), + value: variableValueSchema.optional(), + projectId: z.string().max(36).optional().nullable(), +}) {} diff --git a/packages/cli/src/environments.ee/variables/__tests__/variables.service.integration.test.ts b/packages/cli/src/environments.ee/variables/__tests__/variables.service.integration.test.ts index 620ff764bb9..c68d8233308 100644 --- a/packages/cli/src/environments.ee/variables/__tests__/variables.service.integration.test.ts +++ b/packages/cli/src/environments.ee/variables/__tests__/variables.service.integration.test.ts @@ -2,15 +2,15 @@ import { createTeamProject, linkUserToProject, testDb } from '@n8n/backend-test- import { VariablesRepository } from '@n8n/db'; import { Container } from '@n8n/di'; import type { AssignableProjectRole } from '@n8n/permissions'; -import { createAdmin, createMember } from '@test-integration/db/users'; -import { createProjectVariable, createVariable } from '@test-integration/db/variables'; import { mock } from 'jest-mock-extended'; -import { VariablesService } from '../variables.service.ee'; - import type { EventService } from '@/events/event.service'; import { CacheService } from '@/services/cache/cache.service'; import { ProjectService } from '@/services/project.service.ee'; +import { createAdmin, createMember } from '@test-integration/db/users'; +import { createProjectVariable, createVariable } from '@test-integration/db/variables'; + +import { VariablesService } from '../variables.service.ee'; describe('VariablesService', () => { let variablesService: VariablesService; @@ -409,4 +409,221 @@ describe('VariablesService', () => { }, ); }); + + describe('update', () => { + it('user without global variable:update scope should not be able to update a global variable', async () => { + // ARRANGE + const user = await createMember(); + const variable = await createVariable('VAR1', 'value1'); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable.id, { + key: 'VAR1', + type: 'string', + value: 'value2', + }), + ).rejects.toThrow('You are not allowed to update this variable'); + }); + + it('user with global variable:update scope should be able to update a global variable', async () => { + // ARRANGE + const user = await createAdmin(); + const variable = await createVariable('VAR1', 'value1'); + + // ACT + const updatedVariable = await variablesService.update(user, variable.id, { + value: 'value2', + }); + + // ASSERT + expect(updatedVariable).toBeDefined(); + expect(updatedVariable).toMatchObject({ + id: variable.id, + key: 'VAR1', + type: 'string', + value: 'value2', + }); + }); + + it('user without projectVariable:update scope should not be able to update a project variable', async () => { + // ARRANGE + const user = await createMember(); + const project = await createTeamProject(); + const variable = await createProjectVariable('VAR1', 'value1', project); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable.id, { + value: 'value2', + }), + ).rejects.toThrow('You are not allowed to update this variable'); + }); + + it('user without projectVariable:update scope on destination project should not be able to update a project variable', async () => { + // ARRANGE + const user = await createMember(); + const project1 = await createTeamProject(); + const project2 = await createTeamProject(); + await linkUserToProject(user, project1, 'project:editor'); + + const variable = await createProjectVariable('VAR1', 'value1', project1); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable.id, { + key: 'VAR1', + type: 'string', + value: 'value2', + projectId: project2.id, + }), + ).rejects.toThrow('You are not allowed to move this variable to the specified project'); + }); + + it('user without global projectVariable:update scope should not be able to update a project variable to global', async () => { + // ARRANGE + const user = await createMember(); + const project = await createTeamProject(); + await linkUserToProject(user, project, 'project:editor'); + + const variable = await createProjectVariable('VAR1', 'value1', project); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable.id, { + key: 'VAR1', + type: 'string', + value: 'value2', + projectId: null, + }), + ).rejects.toThrow('You are not allowed to move this variable to the global scope'); + }); + + it('user with projectVariable:update scope should be able to update a project variable in that project', async () => { + // ARRANGE + const user = await createMember(); + const project = await createTeamProject(); + await linkUserToProject(user, project, 'project:editor'); + + const variable = await createProjectVariable('VAR1', 'value1', project); + + // ACT + const updatedVariable = await variablesService.update(user, variable.id, { + key: 'VAR1', + type: 'string', + value: 'value2', + projectId: project.id, + }); + + // ASSERT + expect(updatedVariable).toBeDefined(); + expect(updatedVariable).toMatchObject({ + id: variable.id, + key: 'VAR1', + type: 'string', + value: 'value2', + project: { id: project.id, name: project.name }, + }); + }); + + it('user should not be able to change variable key to one that already exists (global)', async () => { + // ARRANGE + const user = await createAdmin(); + await createVariable('VAR1', 'value1'); + const variable2 = await createVariable('VAR2', 'value2'); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable2.id, { + key: 'VAR1', + type: 'string', + value: 'value2', + }), + ).rejects.toThrow('A global variable with key "VAR1" already exists'); + }); + + it('user should not be able to change variable key to one that already exists (project)', async () => { + // ARRANGE + const user = await createAdmin(); + const project = await createTeamProject(); + + await createProjectVariable('VAR1', 'value1', project); + const variable2 = await createProjectVariable('VAR2', 'value2', project); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable2.id, { + key: 'VAR1', + type: 'string', + value: 'value2', + projectId: project.id, + }), + ).rejects.toThrow('A variable with key "VAR1" already exists in the specified project'); + }); + + it('user should not be able to change variable key to one that already exists (global -> project)', async () => { + // ARRANGE + const user = await createAdmin(); + const variable1 = await createVariable('VAR1', 'value1'); + const project = await createTeamProject(); + await createProjectVariable('VAR2', 'value2', project); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable1.id, { + key: 'VAR2', + type: 'string', + value: 'value1', + projectId: project.id, + }), + ).rejects.toThrow('A variable with key "VAR2" already exists in the specified project'); + }); + + it('user should not be able to change variable key to one that already exists (project -> global)', async () => { + // ARRANGE + const user = await createAdmin(); + const project = await createTeamProject(); + const variable1 = await createProjectVariable('VAR1', 'value1', project); + await createVariable('VAR2', 'value2'); + + // ACT & ASSERT + await expect( + variablesService.update(user, variable1.id, { + key: 'VAR2', + type: 'string', + value: 'value1', + projectId: null, + }), + ).rejects.toThrow('A global variable with key "VAR2" already exists'); + }); + + it('user with projectVariable:update scope on both projects should be able to update variable on destination project', async () => { + // ARRANGE + const user = await createMember(); + const project1 = await createTeamProject(); + const project2 = await createTeamProject(); + await linkUserToProject(user, project1, 'project:editor'); + await linkUserToProject(user, project2, 'project:editor'); + + const variable = await createProjectVariable('VAR1', 'value1', project1); + + // ACT + const updatedVariable = await variablesService.update(user, variable.id, { + key: 'VAR1', + type: 'string', + value: 'value2', + projectId: project2.id, + }); + + // ASSERT + expect(updatedVariable).toBeDefined(); + expect(updatedVariable).toMatchObject({ + id: variable.id, + key: 'VAR1', + type: 'string', + value: 'value2', + project: { id: project2.id, name: project2.name }, + }); + }); + }); }); diff --git a/packages/cli/src/environments.ee/variables/variables.controller.ee.ts b/packages/cli/src/environments.ee/variables/variables.controller.ee.ts index edffca4be6d..86b85480bc5 100644 --- a/packages/cli/src/environments.ee/variables/variables.controller.ee.ts +++ b/packages/cli/src/environments.ee/variables/variables.controller.ee.ts @@ -1,25 +1,15 @@ import { CreateVariableRequestDto, VariableListRequestDto } from '@n8n/api-types'; import { AuthenticatedRequest } from '@n8n/db'; -import { - Body, - Delete, - Get, - GlobalScope, - Licensed, - Patch, - Post, - Query, - RestController, -} from '@n8n/decorators'; +import { Body, Delete, Get, Licensed, Patch, Post, Query, RestController } from '@n8n/decorators'; import type { Response } from 'express'; -import { VariablesService } from './variables.service.ee'; - import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { VariableCountLimitReachedError } from '@/errors/variable-count-limit-reached.error'; import { VariableValidationError } from '@/errors/variable-validation.error'; +import { VariablesService } from './variables.service.ee'; + @RestController('/variables') export class VariablesController { constructor(private readonly variablesService: VariablesService) {} @@ -53,7 +43,6 @@ export class VariablesController { } @Get('/:id') - @GlobalScope('variable:read') async getVariable(req: AuthenticatedRequest<{ id: string }>) { const variable = await this.variablesService.getForUser(req.user, req.params.id); if (variable === null) { @@ -64,7 +53,6 @@ export class VariablesController { @Patch('/:id') @Licensed('feat:variables') - @GlobalScope('variable:update') async updateVariable( req: AuthenticatedRequest<{ id: string }>, _res: Response, @@ -84,7 +72,6 @@ export class VariablesController { } @Delete('/:id') - @GlobalScope('variable:delete') async deleteVariable(req: AuthenticatedRequest<{ id: string }>) { await this.variablesService.deleteForUser(req.user, req.params.id); diff --git a/packages/cli/src/environments.ee/variables/variables.service.ee.ts b/packages/cli/src/environments.ee/variables/variables.service.ee.ts index 2ec4a20d212..0961f15800d 100644 --- a/packages/cli/src/environments.ee/variables/variables.service.ee.ts +++ b/packages/cli/src/environments.ee/variables/variables.service.ee.ts @@ -1,4 +1,4 @@ -import { CreateVariableRequestDto } from '@n8n/api-types'; +import { CreateVariableRequestDto, UpdateVariableRequestDto } from '@n8n/api-types'; import { LicenseState } from '@n8n/backend-common'; import type { User, Variables } from '@n8n/db'; import { generateNanoId, VariablesRepository } from '@n8n/db'; @@ -7,6 +7,7 @@ import { hasGlobalScope, Scope } from '@n8n/permissions'; import { FeatureNotLicensedError } from '@/errors/feature-not-licensed.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; +import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { VariableCountLimitReachedError } from '@/errors/variable-count-limit-reached.error'; import { EventService } from '@/events/event.service'; import { CacheService } from '@/services/cache/cache.service'; @@ -167,8 +168,10 @@ export class VariablesService { } } - async validateUniqueVariable(key: string, projectId?: string) { - const existingVariablesByKey = (await this.getAllCached()).filter((v) => v.key === key); + async validateUniqueVariable(key: string, projectId?: string, id?: string) { + const existingVariablesByKey = (await this.getAllCached()).filter( + (v) => v.key === key && (!id || v.id !== id), + ); // If variable is global, check that it does not already exist with the same key if (!projectId && existingVariablesByKey.find((v) => !v.project)) { throw new VariableCountLimitReachedError( @@ -215,8 +218,11 @@ export class VariablesService { return saveResult; } - async update(user: User, id: string, variable: CreateVariableRequestDto): Promise { + async update(user: User, id: string, variable: UpdateVariableRequestDto): Promise { const existingVariable = await this.getCached(id); + if (!existingVariable) { + throw new NotFoundError(`Variable with id ${id} not found`); + } const userHasRightOnExistingVariable = await this.isAuthorizedForVariable( user, @@ -227,21 +233,30 @@ export class VariablesService { throw new ForbiddenError('You are not allowed to update this variable'); } + // Project id can be undefined (not provided, keep the existing) or null (move to global scope) or a string (move to project) + // Default to existing project id if not provided in the update + let newProjectId = existingVariable.project?.id; + if (typeof variable.projectId !== 'undefined') { + newProjectId = variable.projectId ?? undefined; + } + // Check that user has rights to move the variable to the new project (if projectId changed) - if (existingVariable?.project?.id !== variable.projectId) { + if (existingVariable?.project?.id !== newProjectId) { const userHasRightOnNewProject = await this.isAuthorizedForVariable( user, 'variable:update', - variable.projectId ?? undefined, + newProjectId, ); if (!userHasRightOnNewProject) { throw new ForbiddenError( - 'You are not allowed to move this variable to the specified project', + `You are not allowed to move this variable to ${variable.projectId ? 'the specified project' : 'the global scope'}`, ); } } - // TODO: implement guards when changing variable projectId or moving from global to project variable or vice versa + if (variable.key) { + await this.validateUniqueVariable(variable.key, newProjectId, id); + } await this.variablesRepository.update(id, { key: variable.key, diff --git a/packages/cli/test/integration/variables.test.ts b/packages/cli/test/integration/variables.test.ts index ba6b8c7539e..6c4b54942f5 100644 --- a/packages/cli/test/integration/variables.test.ts +++ b/packages/cli/test/integration/variables.test.ts @@ -305,7 +305,7 @@ describe('PATCH /variables/:id', () => { createVariable(toModify.key, toModify.value), ]); const response = await authOwnerAgent.patch(`/variables/${var1.id}`).send(toModify); - expect(response.statusCode).toBe(500); + expect(response.statusCode).toBe(400); expect(response.body.data?.key).not.toBe(toModify.key); expect(response.body.data?.value).not.toBe(toModify.value);