From a5ad95438c77cb7acb684c78feeb9b0b3f7761c2 Mon Sep 17 00:00:00 2001 From: Valya Bullions Date: Tue, 29 Oct 2024 17:28:40 +0000 Subject: [PATCH] wip --- .../1729695079000-AddProjectToVariables.ts | 43 +++- .../variables/variables.controller.ee.ts | 12 +- .../variables/variables.service.ee.ts | 23 ++- .../cli/test/integration/variables.test.ts | 194 +++++++++++++++--- 4 files changed, 231 insertions(+), 41 deletions(-) diff --git a/packages/cli/src/databases/migrations/common/1729695079000-AddProjectToVariables.ts b/packages/cli/src/databases/migrations/common/1729695079000-AddProjectToVariables.ts index d382004305..c920cc89dd 100644 --- a/packages/cli/src/databases/migrations/common/1729695079000-AddProjectToVariables.ts +++ b/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); } } diff --git a/packages/cli/src/environments/variables/variables.controller.ee.ts b/packages/cli/src/environments/variables/variables.controller.ee.ts index e5f135b59a..c45b13507c 100644 --- a/packages/cli/src/environments/variables/variables.controller.ee.ts +++ b/packages/cli/src/environments/variables/variables.controller.ee.ts @@ -1,15 +1,6 @@ import { CreateVariableRequestDto, UpdateVariableRequestDto } from '@n8n/api-types'; -import { - Body, - Delete, - Get, - GlobalScope, - Licensed, - Patch, - Post, - RestController, -} from '@/decorators'; +import { Body, Delete, Get, 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) { diff --git a/packages/cli/src/environments/variables/variables.service.ee.ts b/packages/cli/src/environments/variables/variables.service.ee.ts index c49833d9fb..c69eb95107 100644 --- a/packages/cli/src/environments/variables/variables.service.ee.ts +++ b/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 { 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))!; diff --git a/packages/cli/test/integration/variables.test.ts b/packages/cli/test/integration/variables.test.ts index 00687b5729..5dd4deae3d 100644 --- a/packages/cli/test/integration/variables.test.ts +++ b/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('test2', 'value2'), + createVariable({ key: 'test1', value: 'value1' }), + 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 () => { - const variable = await createVariable('test1', 'value1'); + test('should modify existing variable if user is an owner', async () => { + 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 () => { - const variable = await createVariable('test1', 'value1'); + test('should not modify existing variable if user is a member', async () => { + 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(toModify.key, toModify.value), + createVariable({ key: 'test1', value: 'value1' }), + 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('test2', 'value2'), - createVariable('test3', 'value3'), + createVariable({ key: 'test1', value: 'value1' }), + createVariable({ key: 'test2', value: 'value2' }), + 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('test2', 'value2'), - createVariable('test3', 'value3'), + createVariable({ key: 'test1', value: 'value1' }), + createVariable({ key: 'test2', value: 'value2' }), + 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); + }); });