From 9403657e469358e90b912279ac605b6d7c83250d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 10 Apr 2024 10:55:49 +0200 Subject: [PATCH] refactor(core): Remove unnecessary indirection in SAML code (no-changelog) (#9103) --- packages/cli/src/sso/saml/constants.ts | 28 ----- .../saml/middleware/samlEnabledMiddleware.ts | 4 +- .../src/sso/saml/routes/saml.controller.ee.ts | 32 ++---- .../cli/src/sso/saml/serviceProvider.ee.ts | 8 +- .../test/integration/saml/saml.api.test.ts | 105 +++++++++--------- 5 files changed, 70 insertions(+), 107 deletions(-) diff --git a/packages/cli/src/sso/saml/constants.ts b/packages/cli/src/sso/saml/constants.ts index b2dd3f2a9b..4fbf8536dc 100644 --- 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 index 69015838d7..e386541de0 100644 --- a/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts +++ b/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts @@ -1,7 +1,7 @@ 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 { @@ -9,7 +9,7 @@ export const samlLicensedAndEnabledMiddleware: RequestHandler = (req, res, next) } }; -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 index 632a4db90b..38de44c230 100644 --- 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,7 +38,7 @@ 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') @@ -47,10 +46,9 @@ export class SamlController { } /** - * 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 index 372d277b30..3f4b3aec62 100644 --- 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 index 768c03fb24..c12f8da175 100644 --- 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();