From f8f54f896b0b8d530288d38fd7ddeae3fbb37542 Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Mon, 18 Aug 2025 11:40:35 +0200 Subject: [PATCH] chore(core): SSO logins are considered MFA logins (#18347) --- .../__tests__/oidc.controller.ee.test.ts | 146 ++++++++++++++++++ .../sso.ee/oidc/routes/oidc.controller.ee.ts | 2 +- .../__tests__/saml.controller.ee.test.ts | 103 +++++++++++- .../sso.ee/saml/routes/saml.controller.ee.ts | 2 +- .../frontend/@n8n/i18n/src/locales/en.json | 8 +- 5 files changed, 251 insertions(+), 10 deletions(-) create mode 100644 packages/cli/src/sso.ee/oidc/routes/__tests__/oidc.controller.ee.test.ts diff --git a/packages/cli/src/sso.ee/oidc/routes/__tests__/oidc.controller.ee.test.ts b/packages/cli/src/sso.ee/oidc/routes/__tests__/oidc.controller.ee.test.ts new file mode 100644 index 0000000000..5c5ae96c33 --- /dev/null +++ b/packages/cli/src/sso.ee/oidc/routes/__tests__/oidc.controller.ee.test.ts @@ -0,0 +1,146 @@ +import type { User } from '@n8n/db'; +import { type Request, type Response } from 'express'; +import { mock } from 'jest-mock-extended'; + +import type { AuthService } from '@/auth/auth.service'; +import type { UrlService } from '@/services/url.service'; + +import type { OidcService } from '../../oidc.service.ee'; +import { OidcController } from '../oidc.controller.ee'; + +const authService = mock(); +const oidcService = mock(); +const urlService = mock(); +const controller = new OidcController(oidcService, authService, urlService); + +const user = mock({ + id: '456', + email: 'oidc-user@example.com', + firstName: 'OIDC', + lastName: 'User', + password: 'password', + authIdentities: [], + role: 'global:member', +}); + +describe('OidcController', () => { + beforeEach(() => { + jest.clearAllMocks(); + + // Mock URL service + urlService.getInstanceBaseUrl.mockReturnValue('http://localhost:5678'); + }); + + describe('callbackHandler', () => { + test('Should issue cookie with MFA flag set to true on successful OIDC login', async () => { + const req = mock({ + originalUrl: '/sso/oidc/callback?code=auth_code&state=state_value', + }); + const res = mock(); + + const expectedCallbackUrl = new URL( + 'http://localhost:5678/sso/oidc/callback?code=auth_code&state=state_value', + ); + + // Mock successful OIDC login + oidcService.loginUser.mockResolvedValueOnce(user); + + await controller.callbackHandler(req, res); + + // Verify that loginUser was called with the correct callback URL + expect(oidcService.loginUser).toHaveBeenCalledWith(expectedCallbackUrl); + + // Verify that issueCookie was called with MFA flag set to true + expect(authService.issueCookie).toHaveBeenCalledWith(res, user, true); + + // Verify redirect to home page + expect(res.redirect).toHaveBeenCalledWith('/'); + }); + + test('Should handle callback URL with different query parameters', async () => { + const req = mock({ + originalUrl: + '/sso/oidc/callback?code=different_code&state=different_state&session_state=session123', + }); + const res = mock(); + + const expectedCallbackUrl = new URL( + 'http://localhost:5678/sso/oidc/callback?code=different_code&state=different_state&session_state=session123', + ); + + oidcService.loginUser.mockResolvedValueOnce(user); + + await controller.callbackHandler(req, res); + + expect(oidcService.loginUser).toHaveBeenCalledWith(expectedCallbackUrl); + expect(authService.issueCookie).toHaveBeenCalledWith(res, user, true); + expect(res.redirect).toHaveBeenCalledWith('/'); + }); + + test('Should handle callback URL with no query parameters', async () => { + const req = mock({ + originalUrl: '/sso/oidc/callback', + }); + const res = mock(); + + const expectedCallbackUrl = new URL('http://localhost:5678/sso/oidc/callback'); + + oidcService.loginUser.mockResolvedValueOnce(user); + + await controller.callbackHandler(req, res); + + expect(oidcService.loginUser).toHaveBeenCalledWith(expectedCallbackUrl); + expect(authService.issueCookie).toHaveBeenCalledWith(res, user, true); + expect(res.redirect).toHaveBeenCalledWith('/'); + }); + + test('Should propagate errors from OIDC service', async () => { + const req = mock({ + originalUrl: '/sso/oidc/callback?code=auth_code&state=state_value', + }); + const res = mock(); + + const loginError = new Error('OIDC login failed'); + oidcService.loginUser.mockRejectedValueOnce(loginError); + + await expect(controller.callbackHandler(req, res)).rejects.toThrow('OIDC login failed'); + + // Verify that issueCookie was not called when login fails + expect(authService.issueCookie).not.toHaveBeenCalled(); + expect(res.redirect).not.toHaveBeenCalled(); + }); + }); + + describe('redirectToAuthProvider', () => { + test('Should redirect to generated authorization URL', async () => { + const req = mock(); + const res = mock(); + + const mockAuthUrl = new URL( + 'https://provider.com/auth?client_id=123&redirect_uri=http://localhost:5678/callback', + ); + oidcService.generateLoginUrl.mockResolvedValueOnce(mockAuthUrl); + + await controller.redirectToAuthProvider(req, res); + + expect(oidcService.generateLoginUrl).toHaveBeenCalled(); + expect(res.redirect).toHaveBeenCalledWith( + 'https://provider.com/auth?client_id=123&redirect_uri=http://localhost:5678/callback', + ); + }); + + test('Should propagate errors from OIDC service during URL generation', async () => { + const req = mock(); + const res = mock(); + + const urlGenerationError = new Error('Failed to generate authorization URL'); + oidcService.generateLoginUrl.mockRejectedValueOnce(urlGenerationError); + + await expect(controller.redirectToAuthProvider(req, res)).rejects.toThrow( + 'Failed to generate authorization URL', + ); + + expect(res.redirect).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/cli/src/sso.ee/oidc/routes/oidc.controller.ee.ts b/packages/cli/src/sso.ee/oidc/routes/oidc.controller.ee.ts index c1af2196c6..efec7fb79d 100644 --- a/packages/cli/src/sso.ee/oidc/routes/oidc.controller.ee.ts +++ b/packages/cli/src/sso.ee/oidc/routes/oidc.controller.ee.ts @@ -57,7 +57,7 @@ export class OidcController { const user = await this.oidcService.loginUser(callbackUrl); - this.authService.issueCookie(res, user, false); + this.authService.issueCookie(res, user, true); res.redirect('/'); } diff --git a/packages/cli/src/sso.ee/saml/routes/__tests__/saml.controller.ee.test.ts b/packages/cli/src/sso.ee/saml/routes/__tests__/saml.controller.ee.test.ts index 126255746e..e88c2a5ce3 100644 --- a/packages/cli/src/sso.ee/saml/routes/__tests__/saml.controller.ee.test.ts +++ b/packages/cli/src/sso.ee/saml/routes/__tests__/saml.controller.ee.test.ts @@ -2,15 +2,29 @@ import type { User } from '@n8n/db'; import { type Response } from 'express'; import { mock } from 'jest-mock-extended'; +import type { AuthService } from '@/auth/auth.service'; +import type { EventService } from '@/events/event.service'; import type { AuthlessRequest } from '@/requests'; +import type { UrlService } from '@/services/url.service'; import type { SamlService } from '../../saml.service.ee'; import { getServiceProviderConfigTestReturnUrl } from '../../service-provider.ee'; import type { SamlUserAttributes } from '../../types'; import { SamlController } from '../saml.controller.ee'; +// Mock the saml-helpers module +jest.mock('../../saml-helpers', () => ({ + isConnectionTestRequest: jest.fn(), + isSamlLicensedAndEnabled: jest.fn(), +})); + +import { isConnectionTestRequest, isSamlLicensedAndEnabled } from '../../saml-helpers'; + +const authService = mock(); const samlService = mock(); -const controller = new SamlController(mock(), samlService, mock(), mock()); +const urlService = mock(); +const eventService = mock(); +const controller = new SamlController(authService, samlService, urlService, eventService); const user = mock({ id: '123', @@ -29,9 +43,17 @@ const attributes: SamlUserAttributes = { describe('Test views', () => { const RelayState = getServiceProviderConfigTestReturnUrl(); + beforeEach(() => { + // Mock the helper functions for test connection flow + (isConnectionTestRequest as jest.Mock).mockReturnValue(true); + }); + test('Should render success with template', async () => { const req = mock(); - const res = mock(); + const res = mock({ + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + }); samlService.handleSamlLogin.mockResolvedValueOnce({ authenticatedUser: user, @@ -46,7 +68,10 @@ describe('Test views', () => { test('Should render failure with template', async () => { const req = mock(); - const res = mock(); + const res = mock({ + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + }); samlService.handleSamlLogin.mockResolvedValueOnce({ authenticatedUser: undefined, @@ -61,7 +86,10 @@ describe('Test views', () => { test('Should render error with template', async () => { const req = mock(); - const res = mock(); + const res = mock({ + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + }); samlService.handleSamlLogin.mockRejectedValueOnce(new Error('Test Error')); @@ -70,3 +98,70 @@ describe('Test views', () => { expect(res.render).toBeCalledWith('saml-connection-test-failed', { message: 'Test Error' }); }); }); + +describe('SAML Login Flow', () => { + beforeEach(() => { + jest.clearAllMocks(); + // Mock the helper functions for actual login flow (not test connections) + (isConnectionTestRequest as jest.Mock).mockReturnValue(false); + (isSamlLicensedAndEnabled as jest.Mock).mockReturnValue(true); + + // Mock URL service + urlService.getInstanceBaseUrl.mockReturnValue('http://localhost:5678'); + }); + + test('Should issue cookie with MFA flag set to true on successful SAML login', async () => { + const req = mock({ browserId: 'test-browser-id' }); + const res = mock(); + + samlService.handleSamlLogin.mockResolvedValueOnce({ + authenticatedUser: user, + attributes, + onboardingRequired: false, + }); + + await controller.acsPost(req, res, { RelayState: '/' }); + + // Verify that issueCookie was called with MFA flag set to true + expect(authService.issueCookie).toHaveBeenCalledWith(res, user, true, 'test-browser-id'); + expect(eventService.emit).toHaveBeenCalledWith('user-logged-in', { + user, + authenticationMethod: 'saml', + }); + expect(res.redirect).toHaveBeenCalledWith('http://localhost:5678/'); + }); + + test('Should issue cookie with MFA flag set to true when onboarding is required', async () => { + const req = mock({ browserId: 'test-browser-id' }); + const res = mock(); + + samlService.handleSamlLogin.mockResolvedValueOnce({ + authenticatedUser: user, + attributes, + onboardingRequired: true, + }); + + await controller.acsPost(req, res, { RelayState: '/' }); + + // Verify that issueCookie was called with MFA flag set to true + expect(authService.issueCookie).toHaveBeenCalledWith(res, user, true, 'test-browser-id'); + expect(res.redirect).toHaveBeenCalledWith('http://localhost:5678/saml/onboarding'); + }); + + test('Should respect custom RelayState redirect URL', async () => { + const req = mock({ browserId: 'test-browser-id' }); + const res = mock(); + const customRelayState = '/custom/redirect'; + + samlService.handleSamlLogin.mockResolvedValueOnce({ + authenticatedUser: user, + attributes, + onboardingRequired: false, + }); + + await controller.acsPost(req, res, { RelayState: customRelayState }); + + expect(authService.issueCookie).toHaveBeenCalledWith(res, user, true, 'test-browser-id'); + expect(res.redirect).toHaveBeenCalledWith('http://localhost:5678/custom/redirect'); + }); +}); diff --git a/packages/cli/src/sso.ee/saml/routes/saml.controller.ee.ts b/packages/cli/src/sso.ee/saml/routes/saml.controller.ee.ts index 2cc1ad3f27..ff665acda7 100644 --- a/packages/cli/src/sso.ee/saml/routes/saml.controller.ee.ts +++ b/packages/cli/src/sso.ee/saml/routes/saml.controller.ee.ts @@ -127,7 +127,7 @@ export class SamlController { // Only sign in user if SAML is enabled, otherwise treat as test connection if (isSamlLicensedAndEnabled()) { - this.authService.issueCookie(res, loginResult.authenticatedUser, false, req.browserId); + this.authService.issueCookie(res, loginResult.authenticatedUser, true, req.browserId); if (loginResult.onboardingRequired) { return res.redirect(this.urlService.getInstanceBaseUrl() + '/saml/onboarding'); } else { diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index f1b8ebed14..e2c48ec95b 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3127,15 +3127,15 @@ "settings.personal.mfa.toast.disabledMfa.error.message": "Error disabling two-factor authentication", "settings.personal.mfa.toast.canEnableMfa.title": "MFA pre-requisite failed", "settings.personal.mfa.enforced": "The settings on this instance require you to set up 2FA. Please enable it to continue working in this instance.", - "settings.personal.mfa.enforce.message": "Enforces 2FA for all users on this instance.", - "settings.personal.mfa.enforce.unlicensed_tooltip": "You can enforce 2FA for all users on this instance when you upgrade your plan. {action}", + "settings.personal.mfa.enforce.message": "Enforces 2FA for all users on this instance authenticating with email and password logins.", + "settings.personal.mfa.enforce.unlicensed_tooltip": "You can enforce 2FA for all users on this instance authenticating with email and password logins when you upgrade your plan. {action}", "settings.personal.mfa.enforce.unlicensed_tooltip.link": "View plans", "settings.personal.mfa.enforce.title": "Enforce two-factor authentication", "settings.personal.mfa.enforce.error": "Cannot enforce 2FA for all users", "settings.personal.mfa.enforce.enabled.title": "2FA Enforced", - "settings.personal.mfa.enforce.enabled.message": "Two-factor authentication is now required for all users on this instance.", + "settings.personal.mfa.enforce.enabled.message": "Two-factor authentication is now required for all users on this instance authenticating with email and password logins.", "settings.personal.mfa.enforce.disabled.title": "2FA No Longer Enforced", - "settings.personal.mfa.enforce.disabled.message": "Two-factor authentication is no longer mandatory for users on this instance.", + "settings.personal.mfa.enforce.disabled.message": "Two-factor authentication is no longer mandatory for users on this instance authenticating with email and password logins.", "settings.mfa.toast.noRecoveryCodeLeft.title": "No 2FA recovery codes remaining", "settings.mfa.toast.noRecoveryCodeLeft.message": "You have used all of your recovery codes. Disable then re-enable two-factor authentication to generate new codes. Open settings", "sso.login.divider": "or",