fix(core): Set the authentication methad to email during startup if…

… the SAML configuration in the database has been corrupted (#11600) Co-authored-by: Iván Ovejero <[email protected]>
n8n-io · Nov 7, 2024 · 6439291 · 6439291
1 parent ddbb263
commit 6439291
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 15 deletions.
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
@@ -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<IdentityProviderInstance>());
@@ -41,14 +48,82 @@ describe('SamlService', () => {
 				],
 			});
 
-			//
 			// ACT & ASSERT
-			//
 			await expect(
 				samlService.getAttributesFromLoginResponse(mock<express.Request>(), 'post'),
 			).rejects.toThrowError(
 				'SAML Authentication failed. Invalid SAML response (missing attributes: http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress, http://schemas.xmlsoap.org/ws/2005/05/identity/claims/firstname, http://schemas.xmlsoap.org/ws/2005/05/identity/claims/lastname, http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn).',
 			);
 		});
 	});
+
+	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
@@ -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
@@ -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<void> {
-		// 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
@@ -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,