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

@@ -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
 		// ----------------------------------------

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

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

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

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

@@ -1,43 +1,51 @@
-import express from 'express';
+import { Container, Service } from 'typedi';
-import { Container } 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) => {
+@Service()
-	if (!isVariablesEnabled()) {
+@Authorized()
-		next('router');
+@RestController('/variables')
-		return;
+export class VariablesController {
+	constructor(
+		private variablesService: VariablesService,
+		private logger: Logger,
+	) {}
+
+	@Get('/')
+	async getVariables() {
+		return Container.get(VariablesService).getAllCached();
 	}
 
-	next();
+	@Post('/', { middlewares: [variablesLicensedMiddleware] })
-});
+	async createVariable(req: VariablesRequest.Create) {
-
-EEVariablesController.post(
-	'/',
-	ResponseHelper.send(async (req: VariablesRequest.Create) => {
 		if (req.user.globalRole.name !== 'owner') {
-			Container.get(Logger).info(
+			this.logger.info('Attempt to update a variable blocked due to lack of permissions', {
-				'Attempt to update a variable blocked due to lack of permissions',
-				{
 				userId: req.user.id,
-				},
+			});
-			);
+			throw new ResponseHelper.UnauthorizedError('Unauthorized');
-			throw new ResponseHelper.AuthError('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(
+	@Get('/:id')
-	'/:id(\\w+)',
+	async getVariable(req: VariablesRequest.Get) {
-	ResponseHelper.send(async (req: VariablesRequest.Update) => {
+		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(
+			this.logger.info('Attempt to update a variable blocked due to lack of permissions', {
-				'Attempt to update a variable blocked due to lack of permissions',
-				{
 				id,
 				userId: req.user.id,
-				},
+			});
-			);
+			throw new ResponseHelper.UnauthorizedError('Unauthorized');
-			throw new ResponseHelper.AuthError('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;
+	}
+}

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

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

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<Variables[]> {
+		const variables = await this.cacheService.get('variables', {
+			async refreshFunction() {
+				// TODO: log refresh cache metric
+				return Container.get(VariablesService).findAll();
+			},
+		});
+		return (variables as Array<DeepPartial<Variables>>).map((v) =>
+			this.variablesRepository.create(v),
+		);
+	}
+
+	async getCount(): Promise<number> {
+		return (await this.getAllCached()).length;
+	}
+
+	async getCached(id: string): Promise<Variables | null> {
+		const variables = await this.getAllCached();
+		const foundVariable = variables.find((variable) => variable.id === id);
+		if (!foundVariable) {
+			return null;
+		}
+		return this.variablesRepository.create(foundVariable as DeepPartial<Variables>);
+	}
+
+	async delete(id: string): Promise<void> {
+		await this.variablesRepository.delete(id);
+		await this.updateCache();
+	}
+
+	async updateCache(): Promise<void> {
+		// TODO: log update cache metric
+		const variables = await this.findAll();
+		await this.cacheService.set('variables', variables);
+	}
+
+	async findAll(): Promise<Variables[]> {
+		return this.variablesRepository.find();
+	}
+
 	validateVariable(variable: Omit<Variables, 'id'>): void {
 		if (variable.key.length > 50) {
 			throw new VariablesValidationError('key cannot be longer than 50 characters');

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

@@ -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<Variables[]> {
-		const variables = await this.cacheService.get('variables', {
-			async refreshFunction() {
-				// TODO: log refresh cache metric
-				return Container.get(VariablesService).findAll();
-			},
-		});
-		return (variables as Array<DeepPartial<Variables>>).map((v) =>
-			this.variablesRepository.create(v),
-		);
-	}
-
-	async getCount(): Promise<number> {
-		return (await this.getAllCached()).length;
-	}
-
-	async getCached(id: string): Promise<Variables | null> {
-		const variables = await this.getAllCached();
-		const foundVariable = variables.find((variable) => variable.id === id);
-		if (!foundVariable) {
-			return null;
-		}
-		return this.variablesRepository.create(foundVariable as DeepPartial<Variables>);
-	}
-
-	async delete(id: string): Promise<void> {
-		await this.variablesRepository.delete(id);
-		await this.updateCache();
-	}
-
-	async updateCache(): Promise<void> {
-		// TODO: log update cache metric
-		const variables = await this.findAll();
-		await this.cacheService.set('variables', variables);
-	}
-
-	async findAll(): Promise<Variables[]> {
-		return this.variablesRepository.find();
-	}
-}

packages/cli/test/integration/shared/utils/testServer.ts

@@ -129,10 +129,10 @@ export const setupTestServer = ({
 						break;
 
 					case 'variables':
-						const { variablesController } = await import(
+						const { VariablesController } = await import(
-							'@/environments/variables/variables.controller'
+							'@/environments/variables/variables.controller.ee'
 						);
-						app.use(`/${REST_PATH_SEGMENT}/variables`, variablesController);
+						registerController(app, config, Container.get(VariablesController));
 						break;
 
 					case 'license':

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

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

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