github-mirror/n8n
1
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2026-05-27 23:07:12 +02:00
Code Issues Actions 56 Packages Projects Releases Wiki Activity Graph

feat(core): Make variable update DTO fields optional for patch and add validation (#20348)

Browse Source
This commit is contained in:
Guillaume Jacquart 2025-10-03 17:47:52 +02:00 committed by GitHub
parent f331c5e600
commit e23bcfd63a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 448 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
packages/@n8n/api-types/src/dto/index.ts
View File

@ -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';

173
packages/@n8n/api-types/src/dto/variables/__tests__/update-variable-request.dto.test.ts Normal file
View File

@ -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));
});
});
});

24
packages/@n8n/api-types/src/dto/variables/base.dto.ts
View File

@ -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,
}) {}

11
packages/@n8n/api-types/src/dto/variables/update-variable-request.dto.ts Normal file
View File

@ -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(),
}) {}

225
packages/cli/src/environments.ee/variables/__tests__/variables.service.integration.test.ts
View File

@ -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 },
});
});
});
});

19
packages/cli/src/environments.ee/variables/variables.controller.ee.ts
View File

@ -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);

31
packages/cli/src/environments.ee/variables/variables.service.ee.ts
View File

@ -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<Variables> {
async update(user: User, id: string, variable: UpdateVariableRequestDto): Promise<Variables> {
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,

2
packages/cli/test/integration/variables.test.ts
View File

@ -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);