feat(core): Improve ldap/saml toggle and tests (#5771)

* improve ldap/saml toggle and tests

* import cleanup

* reject regular login users when saml is enabled

* lint fix

packages/cli/src/Ldap/helpers.ts

@@ -26,6 +26,11 @@ import type { ConnectionSecurity, LdapConfig } from './types';
 import { jsonParse, LoggerProxy as Logger } from 'n8n-workflow';
 import { License } from '@/License';
 import { InternalHooks } from '@/InternalHooks';
+import {
+	isEmailCurrentAuthenticationMethod,
+	isLdapCurrentAuthenticationMethod,
+	setCurrentAuthenticationMethod,
+} from '@/sso/ssoHelpers';
 
 /**
  *  Check whether the LDAP feature is disabled in the instance
@@ -50,8 +55,24 @@ export const setLdapLoginLabel = (value: string): void => {
 /**
  * Set the LDAP login enabled to the configuration object
  */
-export const setLdapLoginEnabled = (value: boolean): void => {
+export const setLdapLoginEnabled = async (value: boolean): Promise<void> => {
-	config.set(LDAP_LOGIN_ENABLED, value);
+	if (config.get(LDAP_LOGIN_ENABLED) === value) {
+		return;
+	}
+	// only one auth method can be active at a time, with email being the default
+	if (value && isEmailCurrentAuthenticationMethod()) {
+		// enable ldap login and disable email login, but only if email is the current auth method
+		config.set(LDAP_LOGIN_ENABLED, true);
+		await setCurrentAuthenticationMethod('ldap');
+	} else if (!value && isLdapCurrentAuthenticationMethod()) {
+		// disable ldap login, but only if ldap is the current auth method
+		config.set(LDAP_LOGIN_ENABLED, false);
+		await setCurrentAuthenticationMethod('email');
+	} else {
+		Logger.warn(
+			'Cannot switch LDAP login enabled state when an authentication method other than email is active',
+		);
+	}
 };
 
 /**
@@ -126,8 +147,8 @@ export const getLdapConfig = async (): Promise<LdapConfig> => {
 /**
  * Take the LDAP configuration and set login enabled and login label to the config object
  */
-export const setGlobalLdapConfigVariables = (ldapConfig: LdapConfig): void => {
+export const setGlobalLdapConfigVariables = async (ldapConfig: LdapConfig): Promise<void> => {
-	setLdapLoginEnabled(ldapConfig.loginEnabled);
+	await setLdapLoginEnabled(ldapConfig.loginEnabled);
 	setLdapLoginLabel(ldapConfig.loginLabel);
 };
 
@@ -175,7 +196,7 @@ export const updateLdapConfig = async (ldapConfig: LdapConfig): Promise<void> =>
 		{ key: LDAP_FEATURE_NAME },
 		{ value: JSON.stringify(ldapConfig), loadOnStartup: true },
 	);
-	setGlobalLdapConfigVariables(ldapConfig);
+	await setGlobalLdapConfigVariables(ldapConfig);
 };
 
 /**
@@ -197,7 +218,7 @@ export const handleLdapInit = async (): Promise<void> => {
 
 	const ldapConfig = await getLdapConfig();
 
-	setGlobalLdapConfigVariables(ldapConfig);
+	await setGlobalLdapConfigVariables(ldapConfig);
 
 	// init LDAP manager with the current
 	// configuration

packages/cli/src/controllers/auth.controller.ts

@@ -19,8 +19,10 @@ import type {
 } from '@/Interfaces';
 import { handleEmailLogin, handleLdapLogin } from '@/auth';
 import type { PostHogClient } from '@/posthog';
-import { isSamlCurrentAuthenticationMethod } from '../sso/ssoHelpers';
+import {
-import { SamlUrls } from '../sso/saml/constants';
+	isLdapCurrentAuthenticationMethod,
+	isSamlCurrentAuthenticationMethod,
+} from '@/sso/ssoHelpers';
 
 @RestController()
 export class AuthController {
@@ -73,19 +75,12 @@ export class AuthController {
 			if (preliminaryUser?.globalRole?.name === 'owner') {
 				user = preliminaryUser;
 			} else {
-				// TODO:SAML - uncomment this block when we have a way to redirect users to the SSO flow
+				throw new AuthError('SAML is enabled, please log in with SAML');
-				// if (doRedirectUsersFromLoginToSsoFlow()) {
-				res.redirect(SamlUrls.restInitSSO);
-				return;
-				// return withFeatureFlags(this.postHog, sanitizeUser(preliminaryUser));
-				// } else {
-				// throw new AuthError(
-				// 	'Login with username and password is disabled due to SAML being the default authentication method. Please use SAML to log in.',
-				// );
-				// }
 			}
+		} else if (isLdapCurrentAuthenticationMethod()) {
+			user = await handleLdapLogin(email, password);
 		} else {
-			user = (await handleLdapLogin(email, password)) ?? (await handleEmailLogin(email, password));
+			user = await handleEmailLogin(email, password);
 		}
 		if (user) {
 			await issueCookie(res, user);

packages/cli/src/sso/saml/samlHelpers.ts

@@ -16,6 +16,7 @@ import {
 	isSamlCurrentAuthenticationMethod,
 	setCurrentAuthenticationMethod,
 } from '../ssoHelpers';
+import { LoggerProxy } from 'n8n-workflow';
 /**
  *  Check whether the SAML feature is licensed and enabled in the instance
  */
@@ -29,14 +30,19 @@ export function getSamlLoginLabel(): string {
 
 // can only toggle between email and saml, not directly to e.g. ldap
 export async function setSamlLoginEnabled(enabled: boolean): Promise<void> {
-	if (enabled) {
+	if (config.get(SAML_LOGIN_ENABLED) === enabled) {
-		if (isEmailCurrentAuthenticationMethod()) {
+		return;
+	}
+	if (enabled && isEmailCurrentAuthenticationMethod()) {
 		config.set(SAML_LOGIN_ENABLED, true);
 		await setCurrentAuthenticationMethod('saml');
-		}
+	} else if (!enabled && isSamlCurrentAuthenticationMethod()) {
-	} else {
 		config.set(SAML_LOGIN_ENABLED, false);
 		await setCurrentAuthenticationMethod('email');
+	} else {
+		LoggerProxy.warn(
+			'Cannot switch SAML login enabled state when an authentication method other than email is active',
+		);
 	}
 }
 

packages/cli/src/sso/ssoHelpers.ts

@@ -2,22 +2,12 @@ import config from '@/config';
 import * as Db from '@/Db';
 import type { AuthProviderType } from '@/databases/entities/AuthIdentity';
 
-export function isSamlCurrentAuthenticationMethod(): boolean {
+/**
-	return config.getEnv('userManagement.authenticationMethod') === 'saml';
+ * 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 function isEmailCurrentAuthenticationMethod(): boolean {
+ * @param authenticationMethod
-	return config.getEnv('userManagement.authenticationMethod') === 'email';
+ */
-}
-
-export function isSsoJustInTimeProvisioningEnabled(): boolean {
-	return config.getEnv('sso.justInTimeProvisioning');
-}
-
-export function doRedirectUsersFromLoginToSsoFlow(): boolean {
-	return config.getEnv('sso.redirectLoginToSso');
-}
-
 export async function setCurrentAuthenticationMethod(
 	authenticationMethod: AuthProviderType,
 ): Promise<void> {
@@ -28,3 +18,27 @@ export async function setCurrentAuthenticationMethod(
 		loadOnStartup: true,
 	});
 }
+
+export function getCurrentAuthenticationMethod(): AuthProviderType {
+	return config.getEnv('userManagement.authenticationMethod');
+}
+
+export function isSamlCurrentAuthenticationMethod(): boolean {
+	return getCurrentAuthenticationMethod() === 'saml';
+}
+
+export function isLdapCurrentAuthenticationMethod(): boolean {
+	return getCurrentAuthenticationMethod() === 'ldap';
+}
+
+export function isEmailCurrentAuthenticationMethod(): boolean {
+	return getCurrentAuthenticationMethod() === 'email';
+}
+
+export function isSsoJustInTimeProvisioningEnabled(): boolean {
+	return config.getEnv('sso.justInTimeProvisioning');
+}
+
+export function doRedirectUsersFromLoginToSsoFlow(): boolean {
+	return config.getEnv('sso.redirectLoginToSso');
+}

packages/cli/test/integration/ldap/ldap.api.test.ts

@@ -16,6 +16,7 @@ import { randomEmail, randomName, uniqueId } from './../shared/random';
 import * as testDb from './../shared/testDb';
 import type { AuthAgent } from '../shared/types';
 import * as utils from '../shared/utils';
+import { getCurrentAuthenticationMethod, setCurrentAuthenticationMethod } from '@/sso/ssoHelpers';
 
 jest.mock('@/telemetry');
 jest.mock('@/UserManagement/email/NodeMailer');
@@ -55,6 +56,8 @@ beforeAll(async () => {
 	);
 
 	utils.initConfigFile();
+
+	await setCurrentAuthenticationMethod('email');
 });
 
 beforeEach(async () => {
@@ -174,6 +177,7 @@ describe('PUT /ldap/config', () => {
 		const emailUser = await Db.collections.User.findOneByOrFail({ id: member.id });
 		const localLdapIdentities = await testDb.getLdapIdentities();
 
+		expect(getCurrentAuthenticationMethod()).toBe('email');
 		expect(emailUser.email).toBe(member.email);
 		expect(emailUser.lastName).toBe(member.lastName);
 		expect(emailUser.firstName).toBe(member.firstName);
@@ -190,6 +194,7 @@ test('GET /ldap/config route should retrieve current configuration', async () =>
 
 	let response = await authAgent(owner).put('/ldap/config').send(validPayload);
 	expect(response.statusCode).toBe(200);
+	expect(getCurrentAuthenticationMethod()).toBe('ldap');
 
 	response = await authAgent(owner).get('/ldap/config');
 

packages/cli/test/integration/saml/saml.api.test.ts

@@ -2,10 +2,11 @@ import type { SuperAgentTest } from 'supertest';
 import config from '@/config';
 import type { User } from '@db/entities/User';
 import { setSamlLoginEnabled } from '@/sso/saml/samlHelpers';
-import { setCurrentAuthenticationMethod } from '@/sso/ssoHelpers';
+import { getCurrentAuthenticationMethod, setCurrentAuthenticationMethod } from '@/sso/ssoHelpers';
 import { randomEmail, randomName, randomValidPassword } from '../shared/random';
 import * as testDb from '../shared/testDb';
 import * as utils from '../shared/utils';
+import { sampleConfig } from './sampleMetadata';
 
 let owner: User;
 let authOwnerAgent: SuperAgentTest;
@@ -16,7 +17,7 @@ async function enableSaml(enable: boolean) {
 }
 
 beforeAll(async () => {
-	const app = await utils.initTestServer({ endpointGroups: ['me'] });
+	const app = await utils.initTestServer({ endpointGroups: ['me', 'saml'] });
 	owner = await testDb.createOwner();
 	authOwnerAgent = utils.createAuthAgent(app)(owner);
 });
@@ -67,4 +68,66 @@ describe('Instance owner', () => {
 				});
 		});
 	});
+
+	describe('POST /sso/saml/config', () => {
+		test('should post saml config', async () => {
+			await authOwnerAgent
+				.post('/sso/saml/config')
+				.send({
+					...sampleConfig,
+					loginEnabled: true,
+				})
+				.expect(200);
+			expect(getCurrentAuthenticationMethod()).toBe('saml');
+		});
+	});
+
+	describe('POST /sso/saml/config/toggle', () => {
+		test('should toggle saml as default authentication method', async () => {
+			await enableSaml(true);
+			expect(getCurrentAuthenticationMethod()).toBe('saml');
+
+			await authOwnerAgent
+				.post('/sso/saml/config/toggle')
+				.send({
+					loginEnabled: false,
+				})
+				.expect(200);
+			expect(getCurrentAuthenticationMethod()).toBe('email');
+
+			await authOwnerAgent
+				.post('/sso/saml/config/toggle')
+				.send({
+					loginEnabled: true,
+				})
+				.expect(200);
+			expect(getCurrentAuthenticationMethod()).toBe('saml');
+		});
+	});
+
+	describe('POST /sso/saml/config/toggle', () => {
+		test('should fail enable saml if default authentication is not email', async () => {
+			await enableSaml(true);
+
+			await authOwnerAgent
+				.post('/sso/saml/config/toggle')
+				.send({
+					loginEnabled: false,
+				})
+				.expect(200);
+			expect(getCurrentAuthenticationMethod()).toBe('email');
+
+			await setCurrentAuthenticationMethod('ldap');
+			expect(getCurrentAuthenticationMethod()).toBe('ldap');
+
+			await authOwnerAgent
+				.post('/sso/saml/config/toggle')
+				.send({
+					loginEnabled: true,
+				})
+				.expect(200);
+
+			expect(getCurrentAuthenticationMethod()).toBe('ldap');
+		});
+	});
 });

packages/cli/test/integration/shared/types.d.ts

@@ -22,6 +22,7 @@ type EndpointGroup =
 	| 'publicApi'
 	| 'nodes'
 	| 'ldap'
+	| 'saml'
 	| 'eventBus'
 	| 'license';
 

packages/cli/test/integration/shared/utils.ts

@@ -78,6 +78,9 @@ import { LdapManager } from '@/Ldap/LdapManager.ee';
 import { LDAP_ENABLED } from '@/Ldap/constants';
 import { handleLdapInit } from '@/Ldap/helpers';
 import { Push } from '@/push';
+import { setSamlLoginEnabled } from '@/sso/saml/samlHelpers';
+import { SamlService } from '@/sso/saml/saml.service.ee';
+import { SamlController } from '@/sso/saml/routes/saml.controller.ee';
 
 export const mockInstance = <T>(
 	ctor: new (...args: any[]) => T,
@@ -190,6 +193,11 @@ export async function initTestServer({
 						new LdapController(service, sync, internalHooks),
 					);
 					break;
+				case 'saml':
+					await setSamlLoginEnabled(true);
+					const samlService = Container.get(SamlService);
+					registerController(testServer.app, config, new SamlController(samlService));
+					break;
 				case 'nodes':
 					registerController(
 						testServer.app,