refactor(core): Remove unnecessary indirection in SAML code (no-chang…

…elog) (#9103)
n8n-io · Apr 10, 2024 · 9403657 · 9403657
1 parent a7108d1
commit 9403657
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 107 deletions.
diff --git a/packages/cli/src/sso/saml/constants.ts b/packages/cli/src/sso/saml/constants.ts
@@ -1,31 +1,3 @@
-export class SamlUrls {
-	static readonly samlRESTRoot = '/rest/sso/saml';
-
-	static readonly initSSO = '/initsso';
-
-	static readonly acs = '/acs';
-
-	static readonly restAcs = this.samlRESTRoot + this.acs;
-
-	static readonly metadata = '/metadata';
-
-	static readonly restMetadata = this.samlRESTRoot + this.metadata;
-
-	static readonly config = '/config';
-
-	static readonly configTest = '/config/test';
-
-	static readonly configTestReturn = '/config/test/return';
-
-	static readonly configToggleEnabled = '/config/toggle';
-
-	static readonly defaultRedirect = '/';
-
-	static readonly samlOnboarding = '/saml/onboarding';
-}
-
 export const SAML_PREFERENCES_DB_KEY = 'features.saml';
-
 export const SAML_LOGIN_LABEL = 'sso.saml.loginLabel';
-
 export const SAML_LOGIN_ENABLED = 'sso.saml.loginEnabled';
diff --git a/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts b/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts
@@ -1,15 +1,15 @@
 import type { RequestHandler } from 'express';
 import { isSamlLicensed, isSamlLicensedAndEnabled } from '../samlHelpers';
 
-export const samlLicensedAndEnabledMiddleware: RequestHandler = (req, res, next) => {
+export const samlLicensedAndEnabledMiddleware: RequestHandler = (_, res, next) => {
 	if (isSamlLicensedAndEnabled()) {
 		next();
 	} else {
 		res.status(403).json({ status: 'error', message: 'Unauthorized' });
 	}
 };
 
-export const samlLicensedMiddleware: RequestHandler = (req, res, next) => {
+export const samlLicensedMiddleware: RequestHandler = (_, res, next) => {
 	if (isSamlLicensed()) {
 		next();
 	} else {

diff --git a/packages/cli/src/sso/saml/routes/saml.controller.ee.ts b/packages/cli/src/sso/saml/routes/saml.controller.ee.ts
@@ -12,7 +12,6 @@ import { BadRequestError } from '@/errors/response-errors/bad-request.error';
 import { AuthError } from '@/errors/response-errors/auth.error';
 import { UrlService } from '@/services/url.service';
 
-import { SamlUrls } from '../constants';
 import {
 	getServiceProviderConfigTestReturnUrl,
 	getServiceProviderEntityId,
@@ -39,18 +38,17 @@ export class SamlController {
 		private readonly internalHooks: InternalHooks,
 	) {}
 
-	@Get(SamlUrls.metadata, { skipAuth: true })
+	@Get('/metadata', { skipAuth: true })
 	async getServiceProviderMetadata(_: express.Request, res: express.Response) {
 		return res
 			.header('Content-Type', 'text/xml')
 			.send(this.samlService.getServiceProviderInstance().getMetadata());
 	}
 
 	/**
-	 * GET /sso/saml/config
 	 * Return SAML config
 	 */
-	@Get(SamlUrls.config, { middlewares: [samlLicensedMiddleware] })
+	@Get('/config', { middlewares: [samlLicensedMiddleware] })
 	async configGet() {
 		const prefs = this.samlService.samlPreferences;
 		return {
@@ -61,10 +59,9 @@ export class SamlController {
 	}
 
 	/**
-	 * POST /sso/saml/config
 	 * Set SAML config
 	 */
-	@Post(SamlUrls.config, { middlewares: [samlLicensedMiddleware] })
+	@Post('/config', { middlewares: [samlLicensedMiddleware] })
 	@GlobalScope('saml:manage')
 	async configPost(req: SamlConfiguration.Update) {
 		const validationResult = await validate(req.body);
@@ -80,10 +77,9 @@ export class SamlController {
 	}
 
 	/**
-	 * POST /sso/saml/config/toggle
-	 * Set SAML config
+	 * Toggle SAML status
 	 */
-	@Post(SamlUrls.configToggleEnabled, { middlewares: [samlLicensedMiddleware] })
+	@Post('/config/toggle', { middlewares: [samlLicensedMiddleware] })
 	@GlobalScope('saml:manage')
 	async toggleEnabledPost(req: SamlConfiguration.Toggle, res: express.Response) {
 		if (req.body.loginEnabled === undefined) {
@@ -94,19 +90,17 @@ export class SamlController {
 	}
 
 	/**
-	 * GET /sso/saml/acs
 	 * Assertion Consumer Service endpoint
 	 */
-	@Get(SamlUrls.acs, { middlewares: [samlLicensedMiddleware], skipAuth: true })
+	@Get('/acs', { middlewares: [samlLicensedMiddleware], skipAuth: true })
 	async acsGet(req: SamlConfiguration.AcsRequest, res: express.Response) {
 		return await this.acsHandler(req, res, 'redirect');
 	}
 
 	/**
-	 * POST /sso/saml/acs
 	 * Assertion Consumer Service endpoint
 	 */
-	@Post(SamlUrls.acs, { middlewares: [samlLicensedMiddleware], skipAuth: true })
+	@Post('/acs', { middlewares: [samlLicensedMiddleware], skipAuth: true })
 	async acsPost(req: SamlConfiguration.AcsRequest, res: express.Response) {
 		return await this.acsHandler(req, res, 'post');
 	}
@@ -140,9 +134,9 @@ export class SamlController {
 				if (isSamlLicensedAndEnabled()) {
 					this.authService.issueCookie(res, loginResult.authenticatedUser, req.browserId);
 					if (loginResult.onboardingRequired) {
-						return res.redirect(this.urlService.getInstanceBaseUrl() + SamlUrls.samlOnboarding);
+						return res.redirect(this.urlService.getInstanceBaseUrl() + '/saml/onboarding');
 					} else {
-						const redirectUrl = req.body?.RelayState ?? SamlUrls.defaultRedirect;
+						const redirectUrl = req.body?.RelayState ?? '/';
 						return res.redirect(this.urlService.getInstanceBaseUrl() + redirectUrl);
 					}
 				} else {
@@ -167,11 +161,10 @@ export class SamlController {
 	}
 
 	/**
-	 * GET /sso/saml/initsso
 	 * Access URL for implementing SP-init SSO
 	 * This endpoint is available if SAML is licensed and enabled
 	 */
-	@Get(SamlUrls.initSSO, { middlewares: [samlLicensedAndEnabledMiddleware], skipAuth: true })
+	@Get('/initsso', { middlewares: [samlLicensedAndEnabledMiddleware], skipAuth: true })
 	async initSsoGet(req: express.Request, res: express.Response) {
 		let redirectUrl = '';
 		try {
@@ -192,13 +185,12 @@ export class SamlController {
 	}
 
 	/**
-	 * GET /sso/saml/config/test
 	 * Test SAML config
 	 * This endpoint is available if SAML is licensed and the requestor is an instance owner
 	 */
-	@Get(SamlUrls.configTest, { middlewares: [samlLicensedMiddleware] })
+	@Get('/config/test', { middlewares: [samlLicensedMiddleware] })
 	@GlobalScope('saml:manage')
-	async configTestGet(req: AuthenticatedRequest, res: express.Response) {
+	async configTestGet(_: AuthenticatedRequest, res: express.Response) {
 		return await this.handleInitSSO(res, getServiceProviderConfigTestReturnUrl());
 	}
 

diff --git a/packages/cli/src/sso/saml/serviceProvider.ee.ts b/packages/cli/src/sso/saml/serviceProvider.ee.ts
@@ -2,21 +2,21 @@
 import { Container } from 'typedi';
 import type { ServiceProviderInstance } from 'samlify';
 import { UrlService } from '@/services/url.service';
-import { SamlUrls } from './constants';
 import type { SamlPreferences } from './types/samlPreferences';
 
 let serviceProviderInstance: ServiceProviderInstance | undefined;
 
 export function getServiceProviderEntityId(): string {
-	return Container.get(UrlService).getInstanceBaseUrl() + SamlUrls.restMetadata;
+	return Container.get(UrlService).getInstanceBaseUrl() + '/rest/sso/saml/metadata';
 }
 
 export function getServiceProviderReturnUrl(): string {
-	return Container.get(UrlService).getInstanceBaseUrl() + SamlUrls.restAcs;
+	return Container.get(UrlService).getInstanceBaseUrl() + '/rest/sso/saml/acs';
 }
 
 export function getServiceProviderConfigTestReturnUrl(): string {
-	return Container.get(UrlService).getInstanceBaseUrl() + SamlUrls.configTestReturn;
+	// TODO: what is this URL?
+	return Container.get(UrlService).getInstanceBaseUrl() + '/config/test/return';
 }
 
 // TODO:SAML: make these configurable for the end user

diff --git a/packages/cli/test/integration/saml/saml.api.test.ts b/packages/cli/test/integration/saml/saml.api.test.ts
@@ -4,7 +4,6 @@ import type { AuthenticationMethod } from 'n8n-workflow';
 import type { User } from '@db/entities/User';
 import { setSamlLoginEnabled } from '@/sso/saml/samlHelpers';
 import { getCurrentAuthenticationMethod, setCurrentAuthenticationMethod } from '@/sso/ssoHelpers';
-import { SamlUrls } from '@/sso/saml/constants';
 import { InternalHooks } from '@/InternalHooks';
 import { SamlService } from '@/sso/saml/saml.service.ee';
 import type { SamlUserAttributes } from '@/sso/saml/types/samlUserAttributes';
@@ -146,123 +145,123 @@ describe('Check endpoint permissions', () => {
 	beforeEach(async () => {
 		await enableSaml(true);
 	});
+
 	describe('Owner', () => {
-		test(`should be able to access ${SamlUrls.metadata}`, async () => {
-			await authOwnerAgent.get(`/sso/saml${SamlUrls.metadata}`).expect(200);
+		test('should be able to access GET /sso/saml/metadata', async () => {
+			await authOwnerAgent.get('/sso/saml/metadata').expect(200);
 		});
 
-		test(`should be able to access GET ${SamlUrls.config}`, async () => {
-			await authOwnerAgent.get(`/sso/saml${SamlUrls.config}`).expect(200);
+		test('should be able to access GET /sso/saml/config', async () => {
+			await authOwnerAgent.get('/sso/saml/config').expect(200);
 		});
 
-		test(`should be able to access POST ${SamlUrls.config}`, async () => {
-			await authOwnerAgent.post(`/sso/saml${SamlUrls.config}`).expect(200);
+		test('should be able to access POST /sso/saml/config', async () => {
+			await authOwnerAgent.post('/sso/saml/config').expect(200);
 		});
 
-		test(`should be able to access POST ${SamlUrls.configToggleEnabled}`, async () => {
-			await authOwnerAgent.post(`/sso/saml${SamlUrls.configToggleEnabled}`).expect(400);
+		test('should be able to access POST /sso/saml/config/toggle', async () => {
+			await authOwnerAgent.post('/sso/saml/config/toggle').expect(400);
 		});
 
-		test(`should be able to access GET ${SamlUrls.acs}`, async () => {
+		test('should be able to access GET /sso/saml/acs', async () => {
 			// Note that 401 here is coming from the missing SAML object,
 			// not from not being able to access the endpoint, so this is expected!
-			const response = await authOwnerAgent.get(`/sso/saml${SamlUrls.acs}`).expect(401);
+			const response = await authOwnerAgent.get('/sso/saml/acs').expect(401);
 			expect(response.text).toContain('SAML Authentication failed');
 		});
 
-		test(`should be able to access POST ${SamlUrls.acs}`, async () => {
+		test('should be able to access POST /sso/saml/acs', async () => {
 			// Note that 401 here is coming from the missing SAML object,
 			// not from not being able to access the endpoint, so this is expected!
-			const response = await authOwnerAgent.post(`/sso/saml${SamlUrls.acs}`).expect(401);
+			const response = await authOwnerAgent.post('/sso/saml/acs').expect(401);
 			expect(response.text).toContain('SAML Authentication failed');
 		});
 
-		test(`should be able to access GET ${SamlUrls.initSSO}`, async () => {
-			await authOwnerAgent.get(`/sso/saml${SamlUrls.initSSO}`).expect(200);
+		test('should be able to access GET /sso/saml/initsso', async () => {
+			await authOwnerAgent.get('/sso/saml/initsso').expect(200);
 		});
 
-		test(`should be able to access GET ${SamlUrls.configTest}`, async () => {
-			await authOwnerAgent.get(`/sso/saml${SamlUrls.configTest}`).expect(200);
+		test('should be able to access GET /sso/saml/config/test', async () => {
+			await authOwnerAgent.get('/sso/saml/config/test').expect(200);
 		});
 	});
+
 	describe('Authenticated Member', () => {
-		test(`should be able to access ${SamlUrls.metadata}`, async () => {
-			await authMemberAgent.get(`/sso/saml${SamlUrls.metadata}`).expect(200);
+		test('should be able to access GET /sso/saml/metadata', async () => {
+			await authMemberAgent.get('/sso/saml/metadata').expect(200);
 		});
 
-		test(`should be able to access GET ${SamlUrls.config}`, async () => {
-			await authMemberAgent.get(`/sso/saml${SamlUrls.config}`).expect(200);
+		test('should be able to access GET /sso/saml/config', async () => {
+			await authMemberAgent.get('/sso/saml/config').expect(200);
 		});
 
-		test(`should NOT be able to access POST ${SamlUrls.config}`, async () => {
-			await authMemberAgent.post(`/sso/saml${SamlUrls.config}`).expect(403);
+		test('should NOT be able to access POST /sso/saml/config', async () => {
+			await authMemberAgent.post('/sso/saml/config').expect(403);
 		});
 
-		test(`should NOT be able to access POST ${SamlUrls.configToggleEnabled}`, async () => {
-			await authMemberAgent.post(`/sso/saml${SamlUrls.configToggleEnabled}`).expect(403);
+		test('should NOT be able to access POST /sso/saml/config/toggle', async () => {
+			await authMemberAgent.post('/sso/saml/config/toggle').expect(403);
 		});
 
-		test(`should be able to access GET ${SamlUrls.acs}`, async () => {
+		test('should be able to access GET /sso/saml/acs', async () => {
 			// Note that 401 here is coming from the missing SAML object,
 			// not from not being able to access the endpoint, so this is expected!
-			const response = await authMemberAgent.get(`/sso/saml${SamlUrls.acs}`).expect(401);
+			const response = await authMemberAgent.get('/sso/saml/acs').expect(401);
 			expect(response.text).toContain('SAML Authentication failed');
 		});
 
-		test(`should be able to access POST ${SamlUrls.acs}`, async () => {
+		test('should be able to access POST /sso/saml/acs', async () => {
 			// Note that 401 here is coming from the missing SAML object,
 			// not from not being able to access the endpoint, so this is expected!
-			const response = await authMemberAgent.post(`/sso/saml${SamlUrls.acs}`).expect(401);
+			const response = await authMemberAgent.post('/sso/saml/acs').expect(401);
 			expect(response.text).toContain('SAML Authentication failed');
 		});
 
-		test(`should be able to access GET ${SamlUrls.initSSO}`, async () => {
-			await authMemberAgent.get(`/sso/saml${SamlUrls.initSSO}`).expect(200);
+		test('should be able to access GET /sso/saml/initsso', async () => {
+			await authMemberAgent.get('/sso/saml/initsso').expect(200);
 		});
 
-		test(`should NOT be able to access GET ${SamlUrls.configTest}`, async () => {
-			await authMemberAgent.get(`/sso/saml${SamlUrls.configTest}`).expect(403);
+		test('should NOT be able to access GET /sso/saml/config/test', async () => {
+			await authMemberAgent.get('/sso/saml/config/test').expect(403);
 		});
 	});
 	describe('Non-Authenticated User', () => {
-		test(`should be able to access ${SamlUrls.metadata}`, async () => {
-			await testServer.authlessAgent.get(`/sso/saml${SamlUrls.metadata}`).expect(200);
+		test('should be able to access /sso/saml/metadata', async () => {
+			await testServer.authlessAgent.get('/sso/saml/metadata').expect(200);
 		});
 
-		test(`should NOT be able to access GET ${SamlUrls.config}`, async () => {
-			await testServer.authlessAgent.get(`/sso/saml${SamlUrls.config}`).expect(401);
+		test('should NOT be able to access GET /sso/saml/config', async () => {
+			await testServer.authlessAgent.get('/sso/saml/config').expect(401);
 		});
 
-		test(`should NOT be able to access POST ${SamlUrls.config}`, async () => {
-			await testServer.authlessAgent.post(`/sso/saml${SamlUrls.config}`).expect(401);
+		test('should NOT be able to access POST /sso/saml/config', async () => {
+			await testServer.authlessAgent.post('/sso/saml/config').expect(401);
 		});
 
-		test(`should NOT be able to access POST ${SamlUrls.configToggleEnabled}`, async () => {
-			await testServer.authlessAgent.post(`/sso/saml${SamlUrls.configToggleEnabled}`).expect(401);
+		test('should NOT be able to access POST /sso/saml/config/toggle', async () => {
+			await testServer.authlessAgent.post('/sso/saml/config/toggle').expect(401);
 		});
 
-		test(`should be able to access GET ${SamlUrls.acs}`, async () => {
+		test('should be able to access GET /sso/saml/acs', async () => {
 			// Note that 401 here is coming from the missing SAML object,
 			// not from not being able to access the endpoint, so this is expected!
-			const response = await testServer.authlessAgent.get(`/sso/saml${SamlUrls.acs}`).expect(401);
+			const response = await testServer.authlessAgent.get('/sso/saml/acs').expect(401);
 			expect(response.text).toContain('SAML Authentication failed');
 		});
 
-		test(`should be able to access POST ${SamlUrls.acs}`, async () => {
+		test('should be able to access POST /sso/saml/acs', async () => {
 			// Note that 401 here is coming from the missing SAML object,
 			// not from not being able to access the endpoint, so this is expected!
-			const response = await testServer.authlessAgent.post(`/sso/saml${SamlUrls.acs}`).expect(401);
+			const response = await testServer.authlessAgent.post('/sso/saml/acs').expect(401);
 			expect(response.text).toContain('SAML Authentication failed');
 		});
 
-		test(`should be able to access GET ${SamlUrls.initSSO}`, async () => {
-			const response = await testServer.authlessAgent
-				.get(`/sso/saml${SamlUrls.initSSO}`)
-				.expect(200);
+		test('should be able to access GET /sso/saml/initsso', async () => {
+			await testServer.authlessAgent.get('/sso/saml/initsso').expect(200);
 		});
 
-		test(`should NOT be able to access GET ${SamlUrls.configTest}`, async () => {
-			await testServer.authlessAgent.get(`/sso/saml${SamlUrls.configTest}`).expect(401);
+		test('should NOT be able to access GET /sso/saml/config/test', async () => {
+			await testServer.authlessAgent.get('/sso/saml/config/test').expect(401);
 		});
 	});
 });
@@ -304,7 +303,7 @@ describe('SAML login flow', () => {
 				return;
 			},
 		);
-		const response = await authOwnerAgent.post(`/sso/saml${SamlUrls.acs}`).expect(302);
+		await authOwnerAgent.post('/sso/saml/acs').expect(302);
 		expect(mockedHookOnUserLoginSuccess).toBeCalled();
 		mockedHookOnUserLoginSuccess.mockRestore();
 		mockedHandleSamlLogin.mockRestore();
@@ -346,7 +345,7 @@ describe('SAML login flow', () => {
 				return;
 			},
 		);
-		const response = await authOwnerAgent.post(`/sso/saml${SamlUrls.acs}`).expect(401);
+		await authOwnerAgent.post('/sso/saml/acs').expect(401);
 		expect(mockedHookOnUserLoginFailed).toBeCalled();
 		mockedHookOnUserLoginFailed.mockRestore();
 		mockedHandleSamlLogin.mockRestore();