fix(core): Improve saml endpoints and audit events (#6107)

* update saml endpoints and login audit

* fix(core): Skip auth for controllers/routes that don't use the `Authorized` decorator

* fix linting

* lint fix

* add tests and fix endpoint permission

* add hook test

---------

Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <aditya@netroy.in>

packages/cli/src/Interfaces.ts

@@ -337,6 +337,7 @@ export interface IDiagnosticInfo {
 	n8n_multi_user_allowed: boolean;
 	smtp_set_up: boolean;
 	ldap_allowed: boolean;
+	saml_enabled: boolean;
 }
 
 export interface ITelemetryUserDeletionData {

packages/cli/src/InternalHooks.ts

@@ -5,6 +5,7 @@ import { Service } from 'typedi';
 import { snakeCase } from 'change-case';
 import { BinaryDataManager } from 'n8n-core';
 import type {
+	AuthenticationMethod,
 	ExecutionStatus,
 	INodesGraphResult,
 	IRun,
@@ -80,6 +81,7 @@ export class InternalHooks implements IInternalHooksClass {
 			n8n_multi_user_allowed: diagnosticInfo.n8n_multi_user_allowed,
 			smtp_set_up: diagnosticInfo.smtp_set_up,
 			ldap_allowed: diagnosticInfo.ldap_allowed,
+			saml_enabled: diagnosticInfo.saml_enabled,
 		};
 
 		return Promise.all([
@@ -732,6 +734,38 @@ export class InternalHooks implements IInternalHooksClass {
 		]);
 	}
 
+	async onUserLoginSuccess(userLoginData: {
+		user: User;
+		authenticationMethod: AuthenticationMethod;
+	}): Promise<void> {
+		void Promise.all([
+			eventBus.sendAuditEvent({
+				eventName: 'n8n.audit.user.login.success',
+				payload: {
+					authenticationMethod: userLoginData.authenticationMethod,
+					...userToPayload(userLoginData.user),
+				},
+			}),
+		]);
+	}
+
+	async onUserLoginFailed(userLoginData: {
+		user: string;
+		authenticationMethod: AuthenticationMethod;
+		reason?: string;
+	}): Promise<void> {
+		void Promise.all([
+			eventBus.sendAuditEvent({
+				eventName: 'n8n.audit.user.login.failed',
+				payload: {
+					authenticationMethod: userLoginData.authenticationMethod,
+					user: userLoginData.user,
+					reason: userLoginData.reason,
+				},
+			}),
+		]);
+	}
+
 	/**
 	 * Credentials
 	 */

packages/cli/src/Server.ts

@@ -159,7 +159,11 @@ import { SamlService } from './sso/saml/saml.service.ee';
 import { variablesController } from './environments/variables/variables.controller';
 import { LdapManager } from './Ldap/LdapManager.ee';
 import { getVariablesLimit, isVariablesEnabled } from '@/environments/variables/enviromentHelpers';
-import { getCurrentAuthenticationMethod } from './sso/ssoHelpers';
+import {
+	getCurrentAuthenticationMethod,
+	isLdapCurrentAuthenticationMethod,
+	isSamlCurrentAuthenticationMethod,
+} from './sso/ssoHelpers';
 import { isVersionControlLicensed } from '@/environments/versionControl/versionControlHelper';
 import { VersionControlService } from '@/environments/versionControl/versionControl.service.ee';
 import { VersionControlController } from '@/environments/versionControl/versionControl.controller.ee';
@@ -1419,7 +1423,8 @@ export async function start(): Promise<void> {
 		binaryDataMode: binaryDataConfig.mode,
 		n8n_multi_user_allowed: isUserManagementEnabled(),
 		smtp_set_up: config.getEnv('userManagement.emails.mode') === 'smtp',
-		ldap_allowed: isLdapEnabled(),
+		ldap_allowed: isLdapCurrentAuthenticationMethod(),
+		saml_enabled: isSamlCurrentAuthenticationMethod(),
 	};
 
 	// Set up event handling

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

@@ -19,10 +19,13 @@ import type {
 import { handleEmailLogin, handleLdapLogin } from '@/auth';
 import type { PostHogClient } from '@/posthog';
 import {
+	getCurrentAuthenticationMethod,
 	isLdapCurrentAuthenticationMethod,
 	isSamlCurrentAuthenticationMethod,
 } from '@/sso/ssoHelpers';
 import type { UserRepository } from '@db/repositories';
+import { InternalHooks } from '../InternalHooks';
+import Container from 'typedi';
 
 @RestController()
 export class AuthController {
@@ -67,12 +70,15 @@ export class AuthController {
 
 		let user: User | undefined;
 
+		let usedAuthenticationMethod = getCurrentAuthenticationMethod();
+
 		if (isSamlCurrentAuthenticationMethod()) {
 			// attempt to fetch user data with the credentials, but don't log in yet
 			const preliminaryUser = await handleEmailLogin(email, password);
 			// if the user is an owner, continue with the login
 			if (preliminaryUser?.globalRole?.name === 'owner') {
 				user = preliminaryUser;
+				usedAuthenticationMethod = 'email';
 			} else {
 				throw new AuthError('SAML is enabled, please log in with SAML');
 			}
@@ -83,9 +89,17 @@ export class AuthController {
 		}
 		if (user) {
 			await issueCookie(res, user);
+			void Container.get(InternalHooks).onUserLoginSuccess({
+				user,
+				authenticationMethod: usedAuthenticationMethod,
+			});
 			return withFeatureFlags(this.postHog, sanitizeUser(user));
 		}
-
+		void Container.get(InternalHooks).onUserLoginFailed({
+			user: email,
+			authenticationMethod: usedAuthenticationMethod,
+			reason: 'wrong credentials',
+		});
 		throw new AuthError('Wrong username or password. Do you have caps lock on?');
 	}
 

packages/cli/src/eventbus/EventMessageClasses/index.ts

@@ -11,6 +11,8 @@ export const eventNamesWorkflow = [
 ] as const;
 export const eventNamesNode = ['n8n.node.started', 'n8n.node.finished'] as const;
 export const eventNamesAudit = [
+	'n8n.audit.user.login.success',
+	'n8n.audit.user.login.failed',
 	'n8n.audit.user.signedup',
 	'n8n.audit.user.updated',
 	'n8n.audit.user.deleted',

packages/cli/src/sso/saml/routes/saml.controller.ee.ts

@@ -1,6 +1,6 @@
 import express from 'express';
 import { getInstanceBaseUrl } from '@/UserManagement/UserManagementHelper';
-import { Authorized, Get, Post, RestController } from '@/decorators';
+import { Authorized, Get, NoAuthRequired, Post, RestController } from '@/decorators';
 import { SamlUrls } from '../constants';
 import {
 	samlLicensedAndEnabledMiddleware,
@@ -23,11 +23,14 @@ import {
 } from '../serviceProvider.ee';
 import { getSamlConnectionTestSuccessView } from '../views/samlConnectionTestSuccess';
 import { getSamlConnectionTestFailedView } from '../views/samlConnectionTestFailed';
+import Container from 'typedi';
+import { InternalHooks } from '@/InternalHooks';
 
 @RestController('/sso/saml')
 export class SamlController {
 	constructor(private samlService: SamlService) {}
 
+	@NoAuthRequired()
 	@Get(SamlUrls.metadata)
 	async getServiceProviderMetadata(req: express.Request, res: express.Response) {
 		return res
@@ -39,7 +42,7 @@ export class SamlController {
 	 * GET /sso/saml/config
 	 * Return SAML config
 	 */
-	@Authorized(['global', 'owner'])
+	@Authorized('any')
 	@Get(SamlUrls.config, { middlewares: [samlLicensedMiddleware] })
 	async configGet() {
 		const prefs = this.samlService.samlPreferences;
@@ -87,6 +90,7 @@ export class SamlController {
 	 * GET /sso/saml/acs
 	 * Assertion Consumer Service endpoint
 	 */
+	@NoAuthRequired()
 	@Get(SamlUrls.acs, { middlewares: [samlLicensedMiddleware] })
 	async acsGet(req: SamlConfiguration.AcsRequest, res: express.Response) {
 		return this.acsHandler(req, res, 'redirect');
@@ -96,6 +100,7 @@ export class SamlController {
 	 * POST /sso/saml/acs
 	 * Assertion Consumer Service endpoint
 	 */
+	@NoAuthRequired()
 	@Post(SamlUrls.acs, { middlewares: [samlLicensedMiddleware] })
 	async acsPost(req: SamlConfiguration.AcsRequest, res: express.Response) {
 		return this.acsHandler(req, res, 'post');
@@ -122,6 +127,10 @@ export class SamlController {
 				}
 			}
 			if (loginResult.authenticatedUser) {
+				void Container.get(InternalHooks).onUserLoginSuccess({
+					user: loginResult.authenticatedUser,
+					authenticationMethod: 'saml',
+				});
 				// Only sign in user if SAML is enabled, otherwise treat as test connection
 				if (isSamlLicensedAndEnabled()) {
 					await issueCookie(res, loginResult.authenticatedUser);
@@ -134,11 +143,19 @@ export class SamlController {
 					return res.status(202).send(loginResult.attributes);
 				}
 			}
+			void Container.get(InternalHooks).onUserLoginFailed({
+				user: loginResult.attributes.email ?? 'unknown',
+				authenticationMethod: 'saml',
+			});
 			throw new AuthError('SAML Authentication failed');
 		} catch (error) {
 			if (isConnectionTestRequest(req)) {
 				return res.send(getSamlConnectionTestFailedView((error as Error).message));
 			}
+			void Container.get(InternalHooks).onUserLoginFailed({
+				user: 'unknown',
+				authenticationMethod: 'saml',
+			});
 			throw new AuthError('SAML Authentication failed: ' + (error as Error).message);
 		}
 	}
@@ -148,6 +165,7 @@ export class SamlController {
 	 * Access URL for implementing SP-init SSO
 	 * This endpoint is available if SAML is licensed and enabled
 	 */
+	@NoAuthRequired()
 	@Get(SamlUrls.initSSO, { middlewares: [samlLicensedAndEnabledMiddleware] })
 	async initSsoGet(req: express.Request, res: express.Response) {
 		return this.handleInitSSO(res);

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

@@ -3,13 +3,21 @@ import type { SuperAgentTest } from 'supertest';
 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 { License } from '@/License';
 import { randomEmail, randomName, randomValidPassword } from '../shared/random';
 import * as testDb from '../shared/testDb';
 import * as utils from '../shared/utils';
 import { sampleConfig } from './sampleMetadata';
+import { InternalHooks } from '@/InternalHooks';
+import { SamlService } from '@/sso/saml/saml.service.ee';
+import { SamlUserAttributes } from '@/sso/saml/types/samlUserAttributes';
+import { AuthenticationMethod } from 'n8n-workflow';
 
+let someUser: User;
 let owner: User;
+let noAuthMemberAgent: SuperAgentTest;
+let authMemberAgent: SuperAgentTest;
 let authOwnerAgent: SuperAgentTest;
 
 async function enableSaml(enable: boolean) {
@@ -20,7 +28,10 @@ beforeAll(async () => {
 	Container.get(License).isSamlEnabled = () => true;
 	const app = await utils.initTestServer({ endpointGroups: ['me', 'saml'] });
 	owner = await testDb.createOwner();
+	someUser = await testDb.createUser();
 	authOwnerAgent = utils.createAuthAgent(app)(owner);
+	authMemberAgent = utils.createAgent(app, { auth: true, user: someUser });
+	noAuthMemberAgent = utils.createAgent(app, { auth: false, user: someUser });
 });
 
 afterAll(async () => {
@@ -129,6 +140,216 @@ describe('Instance owner', () => {
 				.expect(500);
 
 			expect(getCurrentAuthenticationMethod()).toBe('ldap');
+			await setCurrentAuthenticationMethod('saml');
 		});
 	});
 });
+
+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 ${SamlUrls.config}`, async () => {
+			await authOwnerAgent.get(`/sso/saml${SamlUrls.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 ${SamlUrls.configToggleEnabled}`, async () => {
+			await authOwnerAgent.post(`/sso/saml${SamlUrls.configToggleEnabled}`).expect(400);
+		});
+
+		test(`should be able to access GET ${SamlUrls.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);
+			expect(response.text).toContain('SAML Authentication failed');
+		});
+
+		test(`should be able to access POST ${SamlUrls.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);
+			expect(response.text).toContain('SAML Authentication failed');
+		});
+
+		test(`should be able to access GET ${SamlUrls.initSSO}`, async () => {
+			const response = await authOwnerAgent.get(`/sso/saml${SamlUrls.initSSO}`).expect(200);
+		});
+
+		test(`should be able to access GET ${SamlUrls.configTest}`, async () => {
+			await authOwnerAgent.get(`/sso/saml${SamlUrls.configTest}`).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 ${SamlUrls.config}`, async () => {
+			await authMemberAgent.get(`/sso/saml${SamlUrls.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 ${SamlUrls.configToggleEnabled}`, async () => {
+			await authMemberAgent.post(`/sso/saml${SamlUrls.configToggleEnabled}`).expect(403);
+		});
+
+		test(`should be able to access GET ${SamlUrls.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);
+			expect(response.text).toContain('SAML Authentication failed');
+		});
+
+		test(`should be able to access POST ${SamlUrls.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);
+			expect(response.text).toContain('SAML Authentication failed');
+		});
+
+		test(`should be able to access GET ${SamlUrls.initSSO}`, async () => {
+			const response = await authMemberAgent.get(`/sso/saml${SamlUrls.initSSO}`).expect(200);
+		});
+
+		test(`should NOT be able to access GET ${SamlUrls.configTest}`, async () => {
+			await authMemberAgent.get(`/sso/saml${SamlUrls.configTest}`).expect(403);
+		});
+	});
+	describe('Non-Authenticated User', () => {
+		test(`should be able to access ${SamlUrls.metadata}`, async () => {
+			await noAuthMemberAgent.get(`/sso/saml${SamlUrls.metadata}`).expect(200);
+		});
+
+		test(`should NOT be able to access GET ${SamlUrls.config}`, async () => {
+			await noAuthMemberAgent.get(`/sso/saml${SamlUrls.config}`).expect(401);
+		});
+
+		test(`should NOT be able to access POST ${SamlUrls.config}`, async () => {
+			await noAuthMemberAgent.post(`/sso/saml${SamlUrls.config}`).expect(401);
+		});
+
+		test(`should NOT be able to access POST ${SamlUrls.configToggleEnabled}`, async () => {
+			await noAuthMemberAgent.post(`/sso/saml${SamlUrls.configToggleEnabled}`).expect(401);
+		});
+
+		test(`should be able to access GET ${SamlUrls.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 noAuthMemberAgent.get(`/sso/saml${SamlUrls.acs}`).expect(401);
+			expect(response.text).toContain('SAML Authentication failed');
+		});
+
+		test(`should be able to access POST ${SamlUrls.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 noAuthMemberAgent.post(`/sso/saml${SamlUrls.acs}`).expect(401);
+			expect(response.text).toContain('SAML Authentication failed');
+		});
+
+		test(`should be able to access GET ${SamlUrls.initSSO}`, async () => {
+			const response = await noAuthMemberAgent.get(`/sso/saml${SamlUrls.initSSO}`).expect(200);
+		});
+
+		test(`should NOT be able to access GET ${SamlUrls.configTest}`, async () => {
+			await noAuthMemberAgent.get(`/sso/saml${SamlUrls.configTest}`).expect(401);
+		});
+	});
+});
+
+describe('SAML login flow', () => {
+	beforeEach(async () => {
+		await enableSaml(true);
+	});
+
+	test('should trigger onUserLoginSuccess hook', async () => {
+		const mockedHandleSamlLogin = jest.spyOn(Container.get(SamlService), 'handleSamlLogin');
+
+		mockedHandleSamlLogin.mockImplementation(
+			async (): Promise<{
+				authenticatedUser: User;
+				attributes: SamlUserAttributes;
+				onboardingRequired: false;
+			}> => {
+				return {
+					authenticatedUser: someUser,
+					attributes: {
+						email: someUser.email,
+						firstName: someUser.firstName,
+						lastName: someUser.lastName,
+						userPrincipalName: someUser.email,
+					},
+					onboardingRequired: false,
+				};
+			},
+		);
+
+		const mockedHookOnUserLoginSuccess = jest.spyOn(
+			Container.get(InternalHooks),
+			'onUserLoginSuccess',
+		);
+		mockedHookOnUserLoginSuccess.mockImplementation(
+			async (userLoginData: { user: User; authenticationMethod: AuthenticationMethod }) => {
+				expect(userLoginData.authenticationMethod).toEqual('saml');
+				return;
+			},
+		);
+		const response = await authOwnerAgent.post(`/sso/saml${SamlUrls.acs}`).expect(302);
+		expect(mockedHookOnUserLoginSuccess).toBeCalled();
+		mockedHookOnUserLoginSuccess.mockRestore();
+		mockedHandleSamlLogin.mockRestore();
+	});
+
+	test('should trigger onUserLoginFailed hook', async () => {
+		const mockedHandleSamlLogin = jest.spyOn(Container.get(SamlService), 'handleSamlLogin');
+
+		mockedHandleSamlLogin.mockImplementation(
+			async (): Promise<{
+				authenticatedUser: User | undefined;
+				attributes: SamlUserAttributes;
+				onboardingRequired: false;
+			}> => {
+				return {
+					authenticatedUser: undefined,
+					attributes: {
+						email: someUser.email,
+						firstName: someUser.firstName,
+						lastName: someUser.lastName,
+						userPrincipalName: someUser.email,
+					},
+					onboardingRequired: false,
+				};
+			},
+		);
+
+		const mockedHookOnUserLoginFailed = jest.spyOn(
+			Container.get(InternalHooks),
+			'onUserLoginFailed',
+		);
+		mockedHookOnUserLoginFailed.mockImplementation(
+			async (userLoginData: {
+				user: string;
+				authenticationMethod: AuthenticationMethod;
+				reason?: string;
+			}) => {
+				expect(userLoginData.authenticationMethod).toEqual('saml');
+				return;
+			},
+		);
+		const response = await authOwnerAgent.post(`/sso/saml${SamlUrls.acs}`).expect(401);
+		expect(mockedHookOnUserLoginFailed).toBeCalled();
+		mockedHookOnUserLoginFailed.mockRestore();
+		mockedHandleSamlLogin.mockRestore();
+	});
+});