mudhorn/n8n
1
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2024-12-25 04:34:06 -08:00
Code Issues Actions Packages Projects Releases Wiki Activity

refactor(core): Remove unnecessary indirection in SAML code (no-changelog) (#9103)

Browse source
This commit is contained in:
कारतोफ्फेलस्क्रिप्ट™ 2024-04-10 10:55:49 +02:00 committed by GitHub
parent a7108d14f9
commit 9403657e46
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 70 additions and 107 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
packages/cli/src/sso/saml/constants.ts
View file

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

4
packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts
View file

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

32
packages/cli/src/sso/saml/routes/saml.controller.ee.ts
View file

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

8
packages/cli/src/sso/saml/serviceProvider.ee.ts
View file

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

105
packages/cli/test/integration/saml/saml.api.test.ts
View file

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