From 5acb7b94c071ee2e6b2494866b4569747df4db0e Mon Sep 17 00:00:00 2001 From: Val <68596159+valya@users.noreply.github.com> Date: Mon, 27 Nov 2023 12:17:09 +0000 Subject: [PATCH] refactor: Refactor variables controller into a RestController (no-changelog) (#7822) Github issue / Community forum post (link here to close automatically): --- packages/cli/src/Server.ts | 9 +- packages/cli/src/WorkflowHelpers.ts | 2 +- .../sourceControlExport.service.ee.ts | 2 +- .../sourceControlImport.service.ee.ts | 2 +- .../variables/variables.controller.ee.ts | 106 +++++++++++------- .../variables/variables.controller.ts | 66 ----------- .../variables/variables.service.ee.ts | 51 ++++++++- .../variables/variables.service.ts | 53 --------- .../integration/shared/utils/testServer.ts | 6 +- .../cli/test/integration/variables.test.ts | 17 +-- .../oAuth1Credential.controller.test.ts | 2 +- .../oAuth2Credential.controller.test.ts | 2 +- 12 files changed, 135 insertions(+), 183 deletions(-) delete mode 100644 packages/cli/src/environments/variables/variables.controller.ts delete mode 100644 packages/cli/src/environments/variables/variables.service.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index b7e6b51250..f5f3af489b 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -92,7 +92,7 @@ import { License } from './License'; import { getStatusUsingPreviousExecutionStatusMethod } from './executions/executionHelpers'; import { SamlController } from './sso/saml/routes/saml.controller.ee'; import { SamlService } from './sso/saml/saml.service.ee'; -import { variablesController } from './environments/variables/variables.controller'; +import { VariablesController } from './environments/variables/variables.controller.ee'; import { LdapManager } from './Ldap/LdapManager.ee'; import { isLdapCurrentAuthenticationMethod, @@ -294,6 +294,7 @@ export class Server extends AbstractServer { Container.get(UserService), postHog, ), + Container.get(VariablesController), ]; if (isLdapEnabled()) { @@ -423,12 +424,6 @@ export class Server extends AbstractServer { this.logger.warn(`SAML initialization failed: ${error.message}`); } - // ---------------------------------------- - // Variables - // ---------------------------------------- - - this.app.use(`/${this.restEndpoint}/variables`, variablesController); - // ---------------------------------------- // Source Control // ---------------------------------------- diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index c841af0990..31407da9d1 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -43,7 +43,7 @@ import { RoleRepository } from '@db/repositories/role.repository'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { RoleService } from './services/role.service'; -import { VariablesService } from './environments/variables/variables.service'; +import { VariablesService } from './environments/variables/variables.service.ee'; import { Logger } from './Logger'; const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType'); diff --git a/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts b/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts index 767e7f3c3d..44d1c979ee 100644 --- a/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts +++ b/packages/cli/src/environments/sourceControl/sourceControlExport.service.ee.ts @@ -23,7 +23,7 @@ import { import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { In } from 'typeorm'; import type { SourceControlledFile } from './types/sourceControlledFile'; -import { VariablesService } from '../variables/variables.service'; +import { VariablesService } from '../variables/variables.service.ee'; import { TagRepository } from '@db/repositories/tag.repository'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { Logger } from '@/Logger'; diff --git a/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts b/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts index 41e14da8a4..96ae0a0886 100644 --- a/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts +++ b/packages/cli/src/environments/sourceControl/sourceControlImport.service.ee.ts @@ -24,7 +24,7 @@ import type { SourceControlWorkflowVersionId } from './types/sourceControlWorkfl import { getCredentialExportPath, getWorkflowExportPath } from './sourceControlHelper.ee'; import type { SourceControlledFile } from './types/sourceControlledFile'; import { RoleService } from '@/services/role.service'; -import { VariablesService } from '../variables/variables.service'; +import { VariablesService } from '../variables/variables.service.ee'; import { TagRepository } from '@db/repositories/tag.repository'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { UserRepository } from '@db/repositories/user.repository'; diff --git a/packages/cli/src/environments/variables/variables.controller.ee.ts b/packages/cli/src/environments/variables/variables.controller.ee.ts index 61e3d3a0db..66eafe6c11 100644 --- a/packages/cli/src/environments/variables/variables.controller.ee.ts +++ b/packages/cli/src/environments/variables/variables.controller.ee.ts @@ -1,43 +1,51 @@ -import express from 'express'; -import { Container } from 'typedi'; +import { Container, Service } from 'typedi'; import * as ResponseHelper from '@/ResponseHelper'; -import type { VariablesRequest } from '@/requests'; +import { VariablesRequest } from '@/requests'; +import { Authorized, Delete, Get, Patch, Post, RestController } from '@/decorators'; import { + VariablesService, VariablesLicenseError, - EEVariablesService, VariablesValidationError, } from './variables.service.ee'; import { isVariablesEnabled } from './enviromentHelpers'; import { Logger } from '@/Logger'; +import type { RequestHandler } from 'express'; -export const EEVariablesController = express.Router(); +const variablesLicensedMiddleware: RequestHandler = (req, res, next) => { + if (isVariablesEnabled()) { + next(); + } else { + res.status(403).json({ status: 'error', message: 'Unauthorized' }); + } +}; -EEVariablesController.use((req, res, next) => { - if (!isVariablesEnabled()) { - next('router'); - return; +@Service() +@Authorized() +@RestController('/variables') +export class VariablesController { + constructor( + private variablesService: VariablesService, + private logger: Logger, + ) {} + + @Get('/') + async getVariables() { + return Container.get(VariablesService).getAllCached(); } - next(); -}); - -EEVariablesController.post( - '/', - ResponseHelper.send(async (req: VariablesRequest.Create) => { + @Post('/', { middlewares: [variablesLicensedMiddleware] }) + async createVariable(req: VariablesRequest.Create) { if (req.user.globalRole.name !== 'owner') { - Container.get(Logger).info( - 'Attempt to update a variable blocked due to lack of permissions', - { - userId: req.user.id, - }, - ); - throw new ResponseHelper.AuthError('Unauthorized'); + this.logger.info('Attempt to update a variable blocked due to lack of permissions', { + userId: req.user.id, + }); + throw new ResponseHelper.UnauthorizedError('Unauthorized'); } const variable = req.body; delete variable.id; try { - return await Container.get(EEVariablesService).create(variable); + return await Container.get(VariablesService).create(variable); } catch (error) { if (error instanceof VariablesLicenseError) { throw new ResponseHelper.BadRequestError(error.message); @@ -46,27 +54,32 @@ EEVariablesController.post( } throw error; } - }), -); + } -EEVariablesController.patch( - '/:id(\\w+)', - ResponseHelper.send(async (req: VariablesRequest.Update) => { + @Get('/:id') + async getVariable(req: VariablesRequest.Get) { + const id = req.params.id; + const variable = await Container.get(VariablesService).getCached(id); + if (variable === null) { + throw new ResponseHelper.NotFoundError(`Variable with id ${req.params.id} not found`); + } + return variable; + } + + @Patch('/:id', { middlewares: [variablesLicensedMiddleware] }) + async updateVariable(req: VariablesRequest.Update) { const id = req.params.id; if (req.user.globalRole.name !== 'owner') { - Container.get(Logger).info( - 'Attempt to update a variable blocked due to lack of permissions', - { - id, - userId: req.user.id, - }, - ); - throw new ResponseHelper.AuthError('Unauthorized'); + this.logger.info('Attempt to update a variable blocked due to lack of permissions', { + id, + userId: req.user.id, + }); + throw new ResponseHelper.UnauthorizedError('Unauthorized'); } const variable = req.body; delete variable.id; try { - return await Container.get(EEVariablesService).update(id, variable); + return await Container.get(VariablesService).update(id, variable); } catch (error) { if (error instanceof VariablesLicenseError) { throw new ResponseHelper.BadRequestError(error.message); @@ -75,5 +88,20 @@ EEVariablesController.patch( } throw error; } - }), -); + } + + @Delete('/:id') + async deleteVariable(req: VariablesRequest.Delete) { + const id = req.params.id; + if (req.user.globalRole.name !== 'owner') { + this.logger.info('Attempt to delete a variable blocked due to lack of permissions', { + id, + userId: req.user.id, + }); + throw new ResponseHelper.UnauthorizedError('Unauthorized'); + } + await this.variablesService.delete(id); + + return true; + } +} diff --git a/packages/cli/src/environments/variables/variables.controller.ts b/packages/cli/src/environments/variables/variables.controller.ts deleted file mode 100644 index baaa8dd7a4..0000000000 --- a/packages/cli/src/environments/variables/variables.controller.ts +++ /dev/null @@ -1,66 +0,0 @@ -import express from 'express'; -import { Container } from 'typedi'; - -import * as ResponseHelper from '@/ResponseHelper'; -import type { VariablesRequest } from '@/requests'; -import { VariablesService } from './variables.service'; -import { EEVariablesController } from './variables.controller.ee'; -import { Logger } from '@/Logger'; - -export const variablesController = express.Router(); - -variablesController.use('/', EEVariablesController); -variablesController.use(EEVariablesController); - -variablesController.get( - '/', - ResponseHelper.send(async () => { - return Container.get(VariablesService).getAllCached(); - }), -); - -variablesController.post( - '/', - ResponseHelper.send(async () => { - throw new ResponseHelper.BadRequestError('No variables license found'); - }), -); - -variablesController.get( - '/:id(\\w+)', - ResponseHelper.send(async (req: VariablesRequest.Get) => { - const id = req.params.id; - const variable = await Container.get(VariablesService).getCached(id); - if (variable === null) { - throw new ResponseHelper.NotFoundError(`Variable with id ${req.params.id} not found`); - } - return variable; - }), -); - -variablesController.patch( - '/:id(\\w+)', - ResponseHelper.send(async () => { - throw new ResponseHelper.BadRequestError('No variables license found'); - }), -); - -variablesController.delete( - '/:id(\\w+)', - ResponseHelper.send(async (req: VariablesRequest.Delete) => { - const id = req.params.id; - if (req.user.globalRole.name !== 'owner') { - Container.get(Logger).info( - 'Attempt to delete a variable blocked due to lack of permissions', - { - id, - userId: req.user.id, - }, - ); - throw new ResponseHelper.AuthError('Unauthorized'); - } - await Container.get(VariablesService).delete(id); - - return true; - }), -); diff --git a/packages/cli/src/environments/variables/variables.service.ee.ts b/packages/cli/src/environments/variables/variables.service.ee.ts index 74e8926f8b..8253603f99 100644 --- a/packages/cli/src/environments/variables/variables.service.ee.ts +++ b/packages/cli/src/environments/variables/variables.service.ee.ts @@ -3,13 +3,60 @@ import type { Variables } from '@db/entities/Variables'; import { InternalHooks } from '@/InternalHooks'; import { generateNanoId } from '@db/utils/generators'; import { canCreateNewVariable } from './enviromentHelpers'; -import { VariablesService } from './variables.service'; +import { CacheService } from '@/services/cache.service'; +import { VariablesRepository } from '@db/repositories/variables.repository'; +import type { DeepPartial } from 'typeorm'; export class VariablesLicenseError extends Error {} export class VariablesValidationError extends Error {} @Service() -export class EEVariablesService extends VariablesService { +export class VariablesService { + constructor( + protected cacheService: CacheService, + protected variablesRepository: VariablesRepository, + ) {} + + async getAllCached(): Promise { + const variables = await this.cacheService.get('variables', { + async refreshFunction() { + // TODO: log refresh cache metric + return Container.get(VariablesService).findAll(); + }, + }); + return (variables as Array>).map((v) => + this.variablesRepository.create(v), + ); + } + + async getCount(): Promise { + return (await this.getAllCached()).length; + } + + async getCached(id: string): Promise { + const variables = await this.getAllCached(); + const foundVariable = variables.find((variable) => variable.id === id); + if (!foundVariable) { + return null; + } + return this.variablesRepository.create(foundVariable as DeepPartial); + } + + async delete(id: string): Promise { + await this.variablesRepository.delete(id); + await this.updateCache(); + } + + async updateCache(): Promise { + // TODO: log update cache metric + const variables = await this.findAll(); + await this.cacheService.set('variables', variables); + } + + async findAll(): Promise { + return this.variablesRepository.find(); + } + validateVariable(variable: Omit): void { if (variable.key.length > 50) { throw new VariablesValidationError('key cannot be longer than 50 characters'); diff --git a/packages/cli/src/environments/variables/variables.service.ts b/packages/cli/src/environments/variables/variables.service.ts deleted file mode 100644 index 31d5f473d7..0000000000 --- a/packages/cli/src/environments/variables/variables.service.ts +++ /dev/null @@ -1,53 +0,0 @@ -import type { Variables } from '@db/entities/Variables'; -import { CacheService } from '@/services/cache.service'; -import Container, { Service } from 'typedi'; -import { VariablesRepository } from '@db/repositories/variables.repository'; -import type { DeepPartial } from 'typeorm'; - -@Service() -export class VariablesService { - constructor( - protected cacheService: CacheService, - protected variablesRepository: VariablesRepository, - ) {} - - async getAllCached(): Promise { - const variables = await this.cacheService.get('variables', { - async refreshFunction() { - // TODO: log refresh cache metric - return Container.get(VariablesService).findAll(); - }, - }); - return (variables as Array>).map((v) => - this.variablesRepository.create(v), - ); - } - - async getCount(): Promise { - return (await this.getAllCached()).length; - } - - async getCached(id: string): Promise { - const variables = await this.getAllCached(); - const foundVariable = variables.find((variable) => variable.id === id); - if (!foundVariable) { - return null; - } - return this.variablesRepository.create(foundVariable as DeepPartial); - } - - async delete(id: string): Promise { - await this.variablesRepository.delete(id); - await this.updateCache(); - } - - async updateCache(): Promise { - // TODO: log update cache metric - const variables = await this.findAll(); - await this.cacheService.set('variables', variables); - } - - async findAll(): Promise { - return this.variablesRepository.find(); - } -} diff --git a/packages/cli/test/integration/shared/utils/testServer.ts b/packages/cli/test/integration/shared/utils/testServer.ts index e5c3b2ad08..ec57f70732 100644 --- a/packages/cli/test/integration/shared/utils/testServer.ts +++ b/packages/cli/test/integration/shared/utils/testServer.ts @@ -129,10 +129,10 @@ export const setupTestServer = ({ break; case 'variables': - const { variablesController } = await import( - '@/environments/variables/variables.controller' + const { VariablesController } = await import( + '@/environments/variables/variables.controller.ee' ); - app.use(`/${REST_PATH_SEGMENT}/variables`, variablesController); + registerController(app, config, Container.get(VariablesController)); break; case 'license': diff --git a/packages/cli/test/integration/variables.test.ts b/packages/cli/test/integration/variables.test.ts index 373e451654..f95db2fc50 100644 --- a/packages/cli/test/integration/variables.test.ts +++ b/packages/cli/test/integration/variables.test.ts @@ -4,7 +4,7 @@ import type { Variables } from '@db/entities/Variables'; import { VariablesRepository } from '@db/repositories/variables.repository'; import { generateNanoId } from '@db/utils/generators'; import { License } from '@/License'; -import { VariablesService } from '@/environments/variables/variables.service'; +import { VariablesService } from '@/environments/variables/variables.service.ee'; import { mockInstance } from '../shared/mocking'; import * as testDb from './shared/testDb'; @@ -15,7 +15,8 @@ let authOwnerAgent: SuperAgentTest; let authMemberAgent: SuperAgentTest; const licenseLike = { - isVariablesEnabled: jest.fn().mockReturnValue(true), + isFeatureEnabled: jest.fn().mockReturnValue(true), + isVariablesEnabled: () => licenseLike.isFeatureEnabled(), getVariablesLimit: jest.fn().mockReturnValue(-1), isWithinUsersLimit: jest.fn().mockReturnValue(true), }; @@ -59,7 +60,7 @@ beforeAll(async () => { beforeEach(async () => { await testDb.truncate(['Variables']); - licenseLike.isVariablesEnabled.mockReturnValue(true); + licenseLike.isFeatureEnabled.mockReturnValue(true); licenseLike.getVariablesLimit.mockReturnValue(-1); }); @@ -149,7 +150,7 @@ describe('POST /variables', () => { test('should not create a new variable and return it for a member', async () => { const response = await authMemberAgent.post('/variables').send(toCreate); - expect(response.statusCode).toBe(401); + expect(response.statusCode).toBe(403); expect(response.body.data?.key).not.toBe(toCreate.key); expect(response.body.data?.value).not.toBe(toCreate.value); @@ -158,9 +159,9 @@ describe('POST /variables', () => { }); test("POST /variables should not create a new variable and return it if the instance doesn't have a license", async () => { - licenseLike.isVariablesEnabled.mockReturnValue(false); + licenseLike.isFeatureEnabled.mockReturnValue(false); const response = await authOwnerAgent.post('/variables').send(toCreate); - expect(response.statusCode).toBe(400); + expect(response.statusCode).toBe(403); expect(response.body.data?.key).not.toBe(toCreate.key); expect(response.body.data?.value).not.toBe(toCreate.value); @@ -298,7 +299,7 @@ describe('PATCH /variables/:id', () => { test('should not modify existing variable if use is a member', async () => { const variable = await createVariable('test1', 'value1'); const response = await authMemberAgent.patch(`/variables/${variable.id}`).send(toModify); - expect(response.statusCode).toBe(401); + expect(response.statusCode).toBe(403); expect(response.body.data?.key).not.toBe(toModify.key); expect(response.body.data?.value).not.toBe(toModify.value); @@ -354,7 +355,7 @@ describe('DELETE /variables/:id', () => { ]); const delResponse = await authMemberAgent.delete(`/variables/${var1.id}`); - expect(delResponse.statusCode).toBe(401); + expect(delResponse.statusCode).toBe(403); const byId = await getVariableById(var1.id); expect(byId).not.toBeNull(); diff --git a/packages/cli/test/unit/controllers/oAuth1Credential.controller.test.ts b/packages/cli/test/unit/controllers/oAuth1Credential.controller.test.ts index 1fb036fe23..fb278f1616 100644 --- a/packages/cli/test/unit/controllers/oAuth1Credential.controller.test.ts +++ b/packages/cli/test/unit/controllers/oAuth1Credential.controller.test.ts @@ -12,7 +12,7 @@ import { CredentialsRepository } from '@db/repositories/credentials.repository'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; import { ExternalHooks } from '@/ExternalHooks'; import { Logger } from '@/Logger'; -import { VariablesService } from '@/environments/variables/variables.service'; +import { VariablesService } from '@/environments/variables/variables.service.ee'; import { SecretsHelper } from '@/SecretsHelpers'; import { CredentialsHelper } from '@/CredentialsHelper'; diff --git a/packages/cli/test/unit/controllers/oAuth2Credential.controller.test.ts b/packages/cli/test/unit/controllers/oAuth2Credential.controller.test.ts index 2824f7fd88..72c9d8bfe2 100644 --- a/packages/cli/test/unit/controllers/oAuth2Credential.controller.test.ts +++ b/packages/cli/test/unit/controllers/oAuth2Credential.controller.test.ts @@ -14,7 +14,7 @@ import { CredentialsRepository } from '@db/repositories/credentials.repository'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; import { ExternalHooks } from '@/ExternalHooks'; import { Logger } from '@/Logger'; -import { VariablesService } from '@/environments/variables/variables.service'; +import { VariablesService } from '@/environments/variables/variables.service.ee'; import { SecretsHelper } from '@/SecretsHelpers'; import { CredentialsHelper } from '@/CredentialsHelper';