From 6439291738dec16261979d6d835acbc63743d51a Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Thu, 7 Nov 2024 14:16:47 +0100 Subject: [PATCH] fix(core): Set the authentication methad to `email` during startup if the SAML configuration in the database has been corrupted (#11600) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Iván Ovejero --- .../saml/__tests__/saml.service.ee.test.ts | 83 ++++++++++++++++++- .../errors/invalid-saml-metadata.error.ts | 7 ++ packages/cli/src/sso/saml/saml.service.ee.ts | 37 +++++++-- packages/cli/src/sso/sso-helpers.ts | 8 +- 4 files changed, 120 insertions(+), 15 deletions(-) create mode 100644 packages/cli/src/sso/saml/errors/invalid-saml-metadata.error.ts diff --git a/packages/cli/src/sso/saml/__tests__/saml.service.ee.test.ts b/packages/cli/src/sso/saml/__tests__/saml.service.ee.test.ts index 5dda04dc18..9692cb9ec7 100644 --- a/packages/cli/src/sso/saml/__tests__/saml.service.ee.test.ts +++ b/packages/cli/src/sso/saml/__tests__/saml.service.ee.test.ts @@ -2,22 +2,29 @@ import type express from 'express'; import { mock } from 'jest-mock-extended'; import type { IdentityProviderInstance, ServiceProviderInstance } from 'samlify'; +import { SettingsRepository } from '@/databases/repositories/settings.repository'; import { Logger } from '@/logging/logger.service'; import { UrlService } from '@/services/url.service'; import * as samlHelpers from '@/sso/saml/saml-helpers'; import { SamlService } from '@/sso/saml/saml.service.ee'; import { mockInstance } from '@test/mocking'; +import { SAML_PREFERENCES_DB_KEY } from '../constants'; +import { InvalidSamlMetadataError } from '../errors/invalid-saml-metadata.error'; + describe('SamlService', () => { const logger = mockInstance(Logger); const urlService = mockInstance(UrlService); const samlService = new SamlService(logger, urlService); + const settingsRepository = mockInstance(SettingsRepository); + + beforeEach(() => { + jest.restoreAllMocks(); + }); describe('getAttributesFromLoginResponse', () => { test('throws when any attribute is missing', async () => { - // // ARRANGE - // jest .spyOn(samlService, 'getIdentityProviderInstance') .mockReturnValue(mock()); @@ -41,9 +48,7 @@ describe('SamlService', () => { ], }); - // // ACT & ASSERT - // await expect( samlService.getAttributesFromLoginResponse(mock(), 'post'), ).rejects.toThrowError( @@ -51,4 +56,74 @@ describe('SamlService', () => { ); }); }); + + describe('init', () => { + test('calls `reset` if an InvalidSamlMetadataError is thrown', async () => { + // ARRANGE + jest + .spyOn(samlService, 'loadFromDbAndApplySamlPreferences') + .mockRejectedValue(new InvalidSamlMetadataError()); + jest.spyOn(samlService, 'reset'); + + // ACT + await samlService.init(); + + // ASSERT + expect(samlService.reset).toHaveBeenCalledTimes(1); + }); + + test('calls `reset` if a SyntaxError is thrown', async () => { + // ARRANGE + jest + .spyOn(samlService, 'loadFromDbAndApplySamlPreferences') + .mockRejectedValue(new SyntaxError()); + jest.spyOn(samlService, 'reset'); + + // ACT + await samlService.init(); + + // ASSERT + expect(samlService.reset).toHaveBeenCalledTimes(1); + }); + + test('does not call reset and rethrows if another error is thrown', async () => { + // ARRANGE + jest + .spyOn(samlService, 'loadFromDbAndApplySamlPreferences') + .mockRejectedValue(new TypeError()); + jest.spyOn(samlService, 'reset'); + + // ACT & ASSERT + await expect(samlService.init()).rejects.toThrowError(TypeError); + expect(samlService.reset).toHaveBeenCalledTimes(0); + }); + + test('does not call reset if no error is trown', async () => { + // ARRANGE + jest.spyOn(samlService, 'reset'); + + // ACT + await samlService.init(); + + // ASSERT + expect(samlService.reset).toHaveBeenCalledTimes(0); + }); + }); + + describe('reset', () => { + test('disables saml login and deletes the saml `features.saml` key in the db', async () => { + // ARRANGE + jest.spyOn(samlHelpers, 'setSamlLoginEnabled'); + jest.spyOn(settingsRepository, 'delete'); + + // ACT + await samlService.reset(); + + // ASSERT + expect(samlHelpers.setSamlLoginEnabled).toHaveBeenCalledTimes(1); + expect(samlHelpers.setSamlLoginEnabled).toHaveBeenCalledWith(false); + expect(settingsRepository.delete).toHaveBeenCalledTimes(1); + expect(settingsRepository.delete).toHaveBeenCalledWith({ key: SAML_PREFERENCES_DB_KEY }); + }); + }); }); diff --git a/packages/cli/src/sso/saml/errors/invalid-saml-metadata.error.ts b/packages/cli/src/sso/saml/errors/invalid-saml-metadata.error.ts new file mode 100644 index 0000000000..3d2ceaddb6 --- /dev/null +++ b/packages/cli/src/sso/saml/errors/invalid-saml-metadata.error.ts @@ -0,0 +1,7 @@ +import { ApplicationError } from 'n8n-workflow'; + +export class InvalidSamlMetadataError extends ApplicationError { + constructor() { + super('Invalid SAML metadata', { level: 'warning' }); + } +} diff --git a/packages/cli/src/sso/saml/saml.service.ee.ts b/packages/cli/src/sso/saml/saml.service.ee.ts index 6b07919730..ce69a39e6d 100644 --- a/packages/cli/src/sso/saml/saml.service.ee.ts +++ b/packages/cli/src/sso/saml/saml.service.ee.ts @@ -16,6 +16,7 @@ import { Logger } from '@/logging/logger.service'; import { UrlService } from '@/services/url.service'; import { SAML_PREFERENCES_DB_KEY } from './constants'; +import { InvalidSamlMetadataError } from './errors/invalid-saml-metadata.error'; import { createUserFromSamlAttributes, getMappedSamlAttributesFromFlowResult, @@ -81,11 +82,24 @@ export class SamlService { ) {} async init(): Promise { - // load preferences first but do not apply so as to not load samlify unnecessarily - await this.loadFromDbAndApplySamlPreferences(false); - if (isSamlLicensedAndEnabled()) { - await this.loadSamlify(); - await this.loadFromDbAndApplySamlPreferences(true); + try { + // load preferences first but do not apply so as to not load samlify unnecessarily + await this.loadFromDbAndApplySamlPreferences(false); + if (isSamlLicensedAndEnabled()) { + await this.loadSamlify(); + await this.loadFromDbAndApplySamlPreferences(true); + } + } catch (error) { + // If the SAML configuration has been corrupted in the database we'll + // delete the corrupted configuration and enable email logins again. + if (error instanceof InvalidSamlMetadataError || error instanceof SyntaxError) { + this.logger.warn( + `SAML initialization failed because of invalid metadata in database: ${error.message}. IMPORTANT: Disabling SAML and switching to email-based login for all users. Please review your configuration and re-enable SAML.`, + ); + await this.reset(); + } else { + throw error; + } } } @@ -98,7 +112,7 @@ export class SamlService { validate: async (response: string) => { const valid = await validateResponse(response); if (!valid) { - throw new ApplicationError('Invalid SAML response'); + throw new InvalidSamlMetadataError(); } }, }); @@ -230,7 +244,7 @@ export class SamlService { } else if (prefs.metadata) { const validationResult = await validateMetadata(prefs.metadata); if (!validationResult) { - throw new ApplicationError('Invalid SAML metadata'); + throw new InvalidSamlMetadataError(); } } this.getIdentityProviderInstance(true); @@ -371,4 +385,13 @@ export class SamlService { } return attributes; } + + /** + * Disables SAML, switches to email based logins and deletes the SAML + * configuration from the database. + */ + async reset() { + await setSamlLoginEnabled(false); + await Container.get(SettingsRepository).delete({ key: SAML_PREFERENCES_DB_KEY }); + } } diff --git a/packages/cli/src/sso/sso-helpers.ts b/packages/cli/src/sso/sso-helpers.ts index 925c7a1eb1..f600e72cfd 100644 --- a/packages/cli/src/sso/sso-helpers.ts +++ b/packages/cli/src/sso/sso-helpers.ts @@ -5,10 +5,10 @@ import type { AuthProviderType } from '@/databases/entities/auth-identity'; import { SettingsRepository } from '@/databases/repositories/settings.repository'; /** - * Only one authentication method can be active at a time. This function sets the current authentication method - * and saves it to the database. - * SSO methods should only switch to email and then to another method. Email can switch to any method. - * @param authenticationMethod + * Only one authentication method can be active at a time. This function sets + * the current authentication method and saves it to the database. + * SSO methods should only switch to email and then to another method. Email + * can switch to any method. */ export async function setCurrentAuthenticationMethod( authenticationMethod: AuthProviderType,