wip

packages/cli/src/databases/migrations/common/1729695079000-AddProjectToVariables.ts

@@ -1,12 +1,51 @@
+import { ApplicationError } from 'n8n-workflow';
+
 import type { MigrationContext, ReversibleMigration } from '@/databases/types';
 
 export class AddProjectToVariables1729695079000 implements ReversibleMigration {
-	async up({ schemaBuilder: { addColumns, column, addForeignKey } }: MigrationContext) {
+	async up({
+		dbType,
+		// escape,
+		// runQuery,
+		schemaBuilder: { addColumns, column, addForeignKey, dropIndex },
+		tablePrefix,
+	}: MigrationContext) {
 		await addColumns('variables', [column('projectId').varchar(36)]);
 		await addForeignKey('variables', 'projectId', ['project', 'id']);
+		// TODO test this
+		if (dbType === 'sqlite') {
+			await dropIndex('variables', ['key'], { customIndexName: `idx_${tablePrefix}variables_key` });
+		} else if (dbType === 'postgresdb') {
+			// await runQuery(`ALTER TABLE ${escape.tableName('variables')} DROP CONSTRAINT `);
+			await dropIndex('variables', ['key']);
+		} else if (dbType === 'mysqldb' || dbType === 'mariadb') {
+			await dropIndex('variables', ['key'], { customIndexName: 'key' });
+		}
 	}
 
-	async down({ schemaBuilder: { dropColumns } }: MigrationContext) {
+	async down({
+		escape,
+		logger,
+		runQuery,
+		schemaBuilder: { createIndex, dropColumns },
+	}: MigrationContext) {
+		const [{ count: projectVariables }] = await runQuery<[{ count: number }]>(
+			`SELECT COUNT(*) FROM ${escape.tableName('variables')} WHERE projectId IS NOT NuLL`,
+		);
+
+		if (projectVariables > 0) {
+			const message =
+				'Down migration only possible when there are no project variables. Please delete all project variable that were created via the UI first.';
+			logger.error(message);
+			throw new ApplicationError(message);
+		}
+
 		await dropColumns('variables', ['projectId']);
+
+		// TODO finish adding back previous constraints
+		// make sure they're the same name so that the up
+		// migration doesn't fail next time
+		let indexName: string | undefined = undefined;
+		await createIndex('variables', ['key'], true);
 	}
 }

packages/cli/src/environments/variables/variables.controller.ee.ts

@@ -1,15 +1,6 @@
 import { CreateVariableRequestDto, UpdateVariableRequestDto } from '@n8n/api-types';
 
-import {
+import { Body, Delete, Get, Licensed, Patch, Post, RestController } from '@/decorators';
-	Body,
-	Delete,
-	Get,
-	GlobalScope,
-	Licensed,
-	Patch,
-	Post,
-	RestController,
-} from '@/decorators';
 import { Param } from '@/decorators/args';
 import { BadRequestError } from '@/errors/response-errors/bad-request.error';
 import { NotFoundError } from '@/errors/response-errors/not-found.error';
@@ -38,7 +29,6 @@ export class VariablesController {
 		try {
 			return await this.variablesService.create(variable, req.user);
 		} catch (error) {
-			console.error(error);
 			if (error instanceof VariableCountLimitReachedError) {
 				throw new BadRequestError(error.message);
 			} else if (error instanceof VariableValidationError) {

packages/cli/src/environments/variables/variables.service.ee.ts

@@ -14,6 +14,7 @@ import { CacheService } from '@/services/cache/cache.service';
 import { ProjectService } from '@/services/project.service';
 
 import { canCreateNewVariable } from './environment-helpers';
+import { Project } from '@/databases/entities/project';
 
 @Service()
 export class VariablesService {
@@ -139,6 +140,20 @@ export class VariablesService {
 		}
 	}
 
+	async throwOnExistingKey(key: Variables['key'], projectId?: Project['id']) {
+		const variable = await this.variablesRepository.findOne({
+			where: {
+				key,
+				projectId,
+			},
+		});
+		if (variable) {
+			throw new VariableValidationError(
+				`Variable with this key already exists ${projectId ? 'in this project' : 'globally'}`,
+			);
+		}
+	}
+
 	async create(variable: CreateVariableRequestDto, user: User): Promise<Variables> {
 		if (!canCreateNewVariable(await this.getCount())) {
 			throw new VariableCountLimitReachedError('Variables limit reached');
@@ -146,7 +161,6 @@ export class VariablesService {
 
 		// Creating a global variable
 		if (!variable.projectId && !user.hasGlobalScope('globalVariable:create')) {
-			console.log(1, variable);
 			throw new MissingScopeError();
 		}
 
@@ -157,10 +171,11 @@ export class VariablesService {
 				'variable:create',
 			]))
 		) {
-			console.log(2, variable);
 			throw new MissingScopeError();
 		}
 
+		await this.throwOnExistingKey(variable.key, variable.projectId ?? undefined);
+
 		this.eventService.emit('variable-created');
 		const saveResult = await this.variablesRepository.save(
 			{
@@ -195,6 +210,10 @@ export class VariablesService {
 			throw new MissingScopeError();
 		}
 
+		if (variable.key !== originalVariable.key) {
+			await this.throwOnExistingKey(variable.key, originalVariable.projectId ?? undefined);
+		}
+
 		await this.variablesRepository.update(id, variable);
 		await this.updateCache();
 		return (await this.getCached(id))!;

packages/cli/test/integration/variables.test.ts

@@ -1,27 +1,40 @@
+// TODO add test to test adding project variables with the same name
+// as global variables and vice versa
+
 import { Container } from 'typedi';
 
+import type { Project } from '@/databases/entities/project';
 import type { Variables } from '@/databases/entities/variables';
 import { VariablesRepository } from '@/databases/repositories/variables.repository';
 import { generateNanoId } from '@/databases/utils/generators';
 import { VariablesService } from '@/environments/variables/variables.service.ee';
+import { createTeamProject, linkUserToProject } from '@test-integration/db/projects';
 
 import { createOwner, createUser } from './shared/db/users';
 import * as testDb from './shared/test-db';
 import type { SuperAgentTest } from './shared/types';
 import * as utils from './shared/utils/';
-import { createTeamProject, linkUserToProject } from '@test-integration/db/projects';
 
 let authOwnerAgent: SuperAgentTest;
 let authMemberAgent: SuperAgentTest;
+let projectAdminAgent: SuperAgentTest;
+let projectEditorAgent: SuperAgentTest;
+
+let project: Project;
 
 const testServer = utils.setupTestServer({ endpointGroups: ['variables'] });
 const license = testServer.license;
 
-async function createVariable(key: string, value: string) {
+async function createVariable({
+	key,
+	value,
+	projectId,
+}: { key: string; value: string; projectId?: string | null }) {
 	const result = await Container.get(VariablesRepository).save({
 		id: generateNanoId(),
 		key,
 		value,
+		projectId,
 	});
 	await Container.get(VariablesService).updateCache();
 	return result;
@@ -49,6 +62,13 @@ beforeAll(async () => {
 	const member = await createUser();
 	authMemberAgent = testServer.authAgentFor(member);
 
+	const projectAdmin = await createUser();
+	const projectEditor = await createUser();
+	project = await createTeamProject(undefined, projectAdmin);
+	await linkUserToProject(projectEditor, project, 'project:editor');
+	projectAdminAgent = testServer.authAgentFor(projectAdmin);
+	projectEditorAgent = testServer.authAgentFor(projectEditor);
+
 	license.setDefaults({
 		features: ['feat:variables'],
 		// quota: {
@@ -66,20 +86,36 @@ beforeEach(async () => {
 // ----------------------------------------
 describe('GET /variables', () => {
 	beforeEach(async () => {
-		await Promise.all([createVariable('test1', 'value1'), createVariable('test2', 'value2')]);
+		await Promise.all([
+			createVariable({ key: 'test1', value: 'value1' }),
+			createVariable({ key: 'test2', value: 'value2' }),
+			createVariable({ key: 'test3', value: 'value2', projectId: project.id }),
+		]);
 	});
 
-	test('should return all variables for an owner', async () => {
+	test('should return all global and project variables for an owner', async () => {
 		const response = await authOwnerAgent.get('/variables');
 		expect(response.statusCode).toBe(200);
-		expect(response.body.data.length).toBe(2);
+		expect(response.body.data.length).toBe(3);
 	});
 
-	test('should return all variables for a member', async () => {
+	test('should return all global variables for a member', async () => {
 		const response = await authMemberAgent.get('/variables');
 		expect(response.statusCode).toBe(200);
 		expect(response.body.data.length).toBe(2);
 	});
+
+	test('should return all global and project variables for a project admin', async () => {
+		const response = await projectAdminAgent.get('/variables');
+		expect(response.statusCode).toBe(200);
+		expect(response.body.data.length).toBe(3);
+	});
+
+	test('should return all global and project variables for an editor', async () => {
+		const response = await projectEditorAgent.get('/variables');
+		expect(response.statusCode).toBe(200);
+		expect(response.body.data.length).toBe(3);
+	});
 });
 
 // ----------------------------------------
@@ -89,8 +125,8 @@ describe('GET /variables/:id', () => {
 	let var1: Variables, var2: Variables;
 	beforeEach(async () => {
 		[var1, var2] = await Promise.all([
-			createVariable('test1', 'value1'),
+			createVariable({ key: 'test1', value: 'value1' }),
-			createVariable('test2', 'value2'),
+			createVariable({ key: 'test2', value: 'value2' }),
 		]);
 	});
 
@@ -234,9 +270,9 @@ describe('POST /variables', () => {
 	});
 
 	test('should fail to create a new variable and if one with the same key exists', async () => {
-		await createVariable(toCreate.key, toCreate.value);
+		await createVariable({ key: toCreate.key, value: toCreate.value });
 		const response = await authOwnerAgent.post('/variables').send(toCreate);
-		expect(response.statusCode).toBe(500);
+		expect(response.statusCode).toBe(400);
 		expect(response.body.data?.key).not.toBe(toCreate.key);
 		expect(response.body.data?.value).not.toBe(toCreate.value);
 	});
@@ -246,7 +282,7 @@ describe('POST /variables', () => {
 		let i = 1;
 		let toCreate = generatePayload(i);
 		while (i < 3) {
-			await createVariable(toCreate.key, toCreate.value);
+			await createVariable({ key: toCreate.key, value: toCreate.value });
 			i++;
 			toCreate = generatePayload(i);
 		}
@@ -261,7 +297,7 @@ describe('POST /variables', () => {
 		let i = 1;
 		let toCreate = generatePayload(i);
 		while (i < 6) {
-			await createVariable(toCreate.key, toCreate.value);
+			await createVariable({ key: toCreate.key, value: toCreate.value });
 			i++;
 			toCreate = generatePayload(i);
 		}
@@ -319,7 +355,7 @@ describe('PATCH /variables/:id', () => {
 	};
 
 	test('should modify existing variable if use is an owner', async () => {
-		const variable = await createVariable('test1', 'value1');
+		const variable = await createVariable({ key: 'test1', value: 'value1' });
 		const response = await authOwnerAgent.patch(`/variables/${variable.id}`).send(toModify);
 		expect(response.statusCode).toBe(200);
 		expect(response.body.data.key).toBe(toModify.key);
@@ -339,8 +375,8 @@ describe('PATCH /variables/:id', () => {
 		expect(byKey!.value).toBe(toModify.value);
 	});
 
-	test('should modify existing variable if use is an owner', async () => {
+	test('should modify existing variable if user is an owner', async () => {
-		const variable = await createVariable('test1', 'value1');
+		const variable = await createVariable({ key: 'test1', value: 'value1' });
 		const response = await authOwnerAgent.patch(`/variables/${variable.id}`).send(toModify);
 		expect(response.statusCode).toBe(200);
 		expect(response.body.data.key).toBe(toModify.key);
@@ -360,8 +396,8 @@ describe('PATCH /variables/:id', () => {
 		expect(byKey!.value).toBe(toModify.value);
 	});
 
-	test('should not modify existing variable if use is a member', async () => {
+	test('should not modify existing variable if user is a member', async () => {
-		const variable = await createVariable('test1', 'value1');
+		const variable = await createVariable({ key: 'test1', value: 'value1' });
 		const response = await authMemberAgent.patch(`/variables/${variable.id}`).send(toModify);
 		expect(response.statusCode).toBe(403);
 		expect(response.body.data?.key).not.toBe(toModify.key);
@@ -373,13 +409,68 @@ describe('PATCH /variables/:id', () => {
 		expect(byId!.value).not.toBe(toModify.value);
 	});
 
+	test('should modify existing project variable if user is an owner', async () => {
+		const variable = await createVariable({ key: 'test1', value: 'value1', projectId: project.id });
+		const response = await authOwnerAgent.patch(`/variables/${variable.id}`).send(toModify);
+		expect(response.statusCode).toBe(200);
+		expect(response.body.data.key).toBe(toModify.key);
+		expect(response.body.data.value).toBe(toModify.value);
+
+		const [byId, byKey] = await Promise.all([
+			getVariableById(response.body.data.id),
+			getVariableByKey(toModify.key),
+		]);
+
+		expect(byId).not.toBeNull();
+		expect(byId!.key).toBe(toModify.key);
+		expect(byId!.value).toBe(toModify.value);
+
+		expect(byKey).not.toBeNull();
+		expect(byKey!.id).toBe(response.body.data.id);
+		expect(byKey!.value).toBe(toModify.value);
+	});
+
+	test('should modify existing project variable if user is a project admin', async () => {
+		const variable = await createVariable({ key: 'test1', value: 'value1', projectId: project.id });
+		const response = await projectAdminAgent.patch(`/variables/${variable.id}`).send(toModify);
+		expect(response.statusCode).toBe(200);
+		expect(response.body.data.key).toBe(toModify.key);
+		expect(response.body.data.value).toBe(toModify.value);
+
+		const [byId, byKey] = await Promise.all([
+			getVariableById(response.body.data.id),
+			getVariableByKey(toModify.key),
+		]);
+
+		expect(byId).not.toBeNull();
+		expect(byId!.key).toBe(toModify.key);
+		expect(byId!.value).toBe(toModify.value);
+
+		expect(byKey).not.toBeNull();
+		expect(byKey!.id).toBe(response.body.data.id);
+		expect(byKey!.value).toBe(toModify.value);
+	});
+
+	test('should not modify existing project variable if user is an editor', async () => {
+		const variable = await createVariable({ key: 'test1', value: 'value1', projectId: project.id });
+		const response = await projectEditorAgent.patch(`/variables/${variable.id}`).send(toModify);
+		expect(response.statusCode).toBe(403);
+		expect(response.body.data?.key).not.toBe(toModify.key);
+		expect(response.body.data?.value).not.toBe(toModify.value);
+
+		const byId = await getVariableById(variable.id);
+		expect(byId).not.toBeNull();
+		expect(byId!.key).not.toBe(toModify.key);
+		expect(byId!.value).not.toBe(toModify.value);
+	});
+
 	test('should not modify existing variable if one with the same key exists', async () => {
 		const [var1] = await Promise.all([
-			createVariable('test1', 'value1'),
+			createVariable({ key: 'test1', value: 'value1' }),
-			createVariable(toModify.key, toModify.value),
+			createVariable({ key: toModify.key, value: 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);
 
@@ -396,9 +487,9 @@ describe('PATCH /variables/:id', () => {
 describe('DELETE /variables/:id', () => {
 	test('should delete a single variable for an owner', async () => {
 		const [var1] = await Promise.all([
-			createVariable('test1', 'value1'),
+			createVariable({ key: 'test1', value: 'value1' }),
-			createVariable('test2', 'value2'),
+			createVariable({ key: 'test2', value: 'value2' }),
-			createVariable('test3', 'value3'),
+			createVariable({ key: 'test3', value: 'value3' }),
 		]);
 
 		const delResponse = await authOwnerAgent.delete(`/variables/${var1.id}`);
@@ -413,9 +504,9 @@ describe('DELETE /variables/:id', () => {
 
 	test('should not delete a single variable for a member', async () => {
 		const [var1] = await Promise.all([
-			createVariable('test1', 'value1'),
+			createVariable({ key: 'test1', value: 'value1' }),
-			createVariable('test2', 'value2'),
+			createVariable({ key: 'test2', value: 'value2' }),
-			createVariable('test3', 'value3'),
+			createVariable({ key: 'test3', value: 'value3' }),
 		]);
 
 		const delResponse = await authMemberAgent.delete(`/variables/${var1.id}`);
@@ -427,4 +518,55 @@ describe('DELETE /variables/:id', () => {
 		const getResponse = await authMemberAgent.get('/variables');
 		expect(getResponse.body.data.length).toBe(3);
 	});
+
+	test('should delete a single project variable for an owner', async () => {
+		const [var1] = await Promise.all([
+			createVariable({ key: 'test1', value: 'value1', projectId: project.id }),
+			createVariable({ key: 'test2', value: 'value2', projectId: project.id }),
+			createVariable({ key: 'test3', value: 'value3', projectId: project.id }),
+		]);
+
+		const delResponse = await authOwnerAgent.delete(`/variables/${var1.id}`);
+		expect(delResponse.statusCode).toBe(200);
+
+		const byId = await getVariableById(var1.id);
+		expect(byId).toBeNull();
+
+		const getResponse = await authOwnerAgent.get('/variables');
+		expect(getResponse.body.data.length).toBe(2);
+	});
+
+	test('should delete a single project variable for a project admin', async () => {
+		const [var1] = await Promise.all([
+			createVariable({ key: 'test1', value: 'value1', projectId: project.id }),
+			createVariable({ key: 'test2', value: 'value2', projectId: project.id }),
+			createVariable({ key: 'test3', value: 'value3', projectId: project.id }),
+		]);
+
+		const delResponse = await projectAdminAgent.delete(`/variables/${var1.id}`);
+		expect(delResponse.statusCode).toBe(200);
+
+		const byId = await getVariableById(var1.id);
+		expect(byId).toBeNull();
+
+		const getResponse = await projectAdminAgent.get('/variables');
+		expect(getResponse.body.data.length).toBe(2);
+	});
+
+	test('should not delete a single project variable for an editor', async () => {
+		const [var1] = await Promise.all([
+			createVariable({ key: 'test1', value: 'value1', projectId: project.id }),
+			createVariable({ key: 'test2', value: 'value2', projectId: project.id }),
+			createVariable({ key: 'test3', value: 'value3', projectId: project.id }),
+		]);
+
+		const delResponse = await projectEditorAgent.delete(`/variables/${var1.id}`);
+		expect(delResponse.statusCode).toBe(403);
+
+		const byId = await getVariableById(var1.id);
+		expect(byId).not.toBeNull();
+
+		const getResponse = await projectEditorAgent.get('/variables');
+		expect(getResponse.body.data.length).toBe(3);
+	});
 });