From 072a1dd82555145da1b5d98b608cc5d864a2bc1c Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Tue, 6 Jan 2026 10:38:30 +0100 Subject: [PATCH] fix(core): Fix redirection of user missing MFA to personal settings (#23881) --- packages/@n8n/db/src/entities/types-db.ts | 2 + packages/cli/src/auth/auth.service.ts | 9 ++ packages/cli/src/server.ts | 2 +- .../__tests__/frontend.service.test.ts | 34 +++++- packages/cli/src/services/frontend.service.ts | 13 ++- .../cli/test/integration/mfa/mfa.api.test.ts | 100 ++++++++++++++++++ .../src/app/stores/settings.store.ts | 6 +- 7 files changed, 160 insertions(+), 6 deletions(-) diff --git a/packages/@n8n/db/src/entities/types-db.ts b/packages/@n8n/db/src/entities/types-db.ts index a5f425f0308..7ee450ceb70 100644 --- a/packages/@n8n/db/src/entities/types-db.ts +++ b/packages/@n8n/db/src/entities/types-db.ts @@ -391,6 +391,8 @@ export type APIRequest< export type AuthenticationInformation = { usedMfa: boolean; + // Indicates the user is logged in but hasn't completed required MFA enrollment + mfaEnrollmentRequired?: boolean; }; export type AuthenticatedRequest< diff --git a/packages/cli/src/auth/auth.service.ts b/packages/cli/src/auth/auth.service.ts index ccf72bd1049..255aa9a04bc 100644 --- a/packages/cli/src/auth/auth.service.ts +++ b/packages/cli/src/auth/auth.service.ts @@ -114,7 +114,16 @@ export class AuthService { // If the user has MFA enforced, but did not use it during authentication, we need to throw an error throw new AuthError('MFA not used during authentication'); } else { + // User doesn't have MFA enabled, but MFA is enforced + // They need to set up MFA before accessing most endpoints if (allowUnauthenticated) { + // Don't set req.user to avoid giving full access to semi-authenticated users + // Instead, set a flag in authInfo to indicate MFA enrollment is required + // This allows endpoints to handle this state appropriately (e.g., return public settings) + req.authInfo = { + usedMfa, + mfaEnrollmentRequired: true, + }; return next(); } diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index e237016a263..ec5e2e8f2f0 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -489,7 +489,7 @@ export class Server extends AbstractServer { ResponseHelper.send(async (req: AuthenticatedRequest) => { return req.user ? await frontendService.getSettings() - : await frontendService.getPublicSettings(); + : await frontendService.getPublicSettings(!!req.authInfo?.mfaEnrollmentRequired); }), ); } diff --git a/packages/cli/src/services/__tests__/frontend.service.test.ts b/packages/cli/src/services/__tests__/frontend.service.test.ts index fb61af87fdb..318f82f6c95 100644 --- a/packages/cli/src/services/__tests__/frontend.service.test.ts +++ b/packages/cli/src/services/__tests__/frontend.service.test.ts @@ -230,7 +230,39 @@ describe('FrontendService', () => { }; const { service } = createMockService(); - const settings = await service.getPublicSettings(); + const settings = await service.getPublicSettings(false); + + expect(settings).toEqual(expectedPublicSettings); + }); + + it('should return public settings with mfa', async () => { + const expectedPublicSettings = { + settingsMode: 'public', + defaultLocale: 'en', + userManagement: { + smtpSetup: false, + showSetupOnFirstLoad: true, + authenticationMethod: 'email', + }, + sso: { + saml: { loginEnabled: false }, + ldap: { loginEnabled: false, loginLabel: '' }, + oidc: { + loginEnabled: false, + loginUrl: 'http://localhost:5678/rest/sso/oidc/login', + }, + }, + authCookie: { secure: false }, + previewMode: false, + enterprise: { saml: false, ldap: false, oidc: false }, + mfa: { + enabled: false, + enforced: false, + }, + }; + + const { service } = createMockService(); + const settings = await service.getPublicSettings(true); expect(settings).toEqual(expectedPublicSettings); }); diff --git a/packages/cli/src/services/frontend.service.ts b/packages/cli/src/services/frontend.service.ts index e04aefe43bb..2d9ef13b270 100644 --- a/packages/cli/src/services/frontend.service.ts +++ b/packages/cli/src/services/frontend.service.ts @@ -20,11 +20,11 @@ import { getLdapLoginLabel } from '@/ldap.ee/helpers.ee'; import { License } from '@/license'; import { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; import { MfaService } from '@/mfa/mfa.service'; -import { OwnershipService } from '@/services/ownership.service'; import { CommunityPackagesConfig } from '@/modules/community-packages/community-packages.config'; import type { CommunityPackagesService } from '@/modules/community-packages/community-packages.service'; import { isApiEnabled } from '@/public-api'; import { PushConfig } from '@/push/push.config'; +import { OwnershipService } from '@/services/ownership.service'; import { getSamlLoginLabel } from '@/sso.ee/saml/saml-helpers'; import { getCurrentAuthenticationMethod } from '@/sso.ee/sso-helpers'; import { UserManagementMailer } from '@/user-management/email'; @@ -93,6 +93,11 @@ export type PublicFrontendSettings = { loginUrl: FrontendSettings['sso']['oidc']['loginUrl']; }; }; + + mfa?: { + enabled: boolean; + enforced: boolean; + }; }; @Service() @@ -519,7 +524,7 @@ export class FrontendService { * Only add settings that are absolutely necessary for non-authenticated pages * @returns Public settings for unauthenticated users */ - async getPublicSettings(): Promise { + async getPublicSettings(includeMfaSettings: boolean): Promise { // Get full settings to ensure all required properties are initialized const { defaultLocale, @@ -528,6 +533,7 @@ export class FrontendService { authCookie, previewMode, enterprise: { saml, ldap, oidc }, + mfa, } = await this.getSettings(); const publicSettings: PublicFrontendSettings = { @@ -548,6 +554,9 @@ export class FrontendService { previewMode, enterprise: { saml, ldap, oidc }, }; + if (includeMfaSettings) { + publicSettings.mfa = mfa; + } return publicSettings; } diff --git a/packages/cli/test/integration/mfa/mfa.api.test.ts b/packages/cli/test/integration/mfa/mfa.api.test.ts index 65c45f798ca..990b6380d30 100644 --- a/packages/cli/test/integration/mfa/mfa.api.test.ts +++ b/packages/cli/test/integration/mfa/mfa.api.test.ts @@ -468,4 +468,104 @@ describe('Enforce MFA', () => { key: MFA_ENFORCE_SETTING, }); }); + + test('User without MFA should be able to access MFA setup endpoints when enforcement is enabled', async () => { + const settingsRepository = Container.get(SettingsRepository); + + // Enable MFA enforcement as owner with MFA + owner.mfaEnabled = true; + await testServer + .authAgentFor(owner) + .post('/mfa/enforce-mfa') + .send({ enforce: true }) + .expect(200); + owner.mfaEnabled = false; + + // Create a regular user without MFA + const user = await createUser(); + + // User should be able to access /mfa/qr to get QR code and secret + const qrResponse = await testServer.authAgentFor(user).get('/mfa/qr').expect(200); + + const { secret } = qrResponse.body.data; + expect(secret).toBeDefined(); + + // User should be able to verify MFA code + const mfaCode = new TOTPService().generateTOTP(secret); + await testServer.authAgentFor(user).post('/mfa/verify').send({ mfaCode }).expect(200); + + // User should be able to enable MFA + await testServer.authAgentFor(user).post('/mfa/enable').send({ mfaCode }).expect(200); + + // Verify MFA was enabled for the user + const updatedUser = await Container.get(UserRepository).findOneOrFail({ + where: { id: user.id }, + }); + expect(updatedUser.mfaEnabled).toBe(true); + + // Clean up + await settingsRepository.delete({ + key: MFA_ENFORCE_SETTING, + }); + }); + + test('User without MFA should be blocked from regular endpoints when enforcement is enabled', async () => { + const settingsRepository = Container.get(SettingsRepository); + + // Enable MFA enforcement + owner.mfaEnabled = true; + await testServer + .authAgentFor(owner) + .post('/mfa/enforce-mfa') + .send({ enforce: true }) + .expect(200); + owner.mfaEnabled = false; + + // Create a regular user without MFA + const user = await createUser(); + + // User should be blocked from accessing change password endpoint + const response = await testServer + .authAgentFor(user) + .patch('/me/password') + .send({ currentPassword: 'password', newPassword: 'newPassword123!' }) + .expect(401); + + expect(response.body.message).toBe('Unauthorized'); + expect(response.body.mfaRequired).toBe(true); + + // Clean up + await settingsRepository.delete({ + key: MFA_ENFORCE_SETTING, + }); + }); + + test('User with MFA enabled but not used should be blocked when enforcement is enabled', async () => { + const settingsRepository = Container.get(SettingsRepository); + + // Enable MFA enforcement + owner.mfaEnabled = true; + await testServer + .authAgentFor(owner) + .post('/mfa/enforce-mfa') + .send({ enforce: true }) + .expect(200); + owner.mfaEnabled = false; + + // Create a user with MFA enabled + const { user, rawPassword } = await createUserWithMfaEnabled(); + + // Login without MFA code should fail with error code 998 + const loginResponse = await testServer.authlessAgent + .post('/login') + .send({ emailOrLdapLoginId: user.email, password: rawPassword }) + .expect(401); + + expect(loginResponse.body.code).toBe(998); + + // Clean up + await settingsRepository.delete({ + key: MFA_ENFORCE_SETTING, + }); + }); }); diff --git a/packages/frontend/editor-ui/src/app/stores/settings.store.ts b/packages/frontend/editor-ui/src/app/stores/settings.store.ts index 30664609212..ed583030cc7 100644 --- a/packages/frontend/editor-ui/src/app/stores/settings.store.ts +++ b/packages/frontend/editor-ui/src/app/stores/settings.store.ts @@ -239,6 +239,10 @@ export const useSettingsStore = defineStore(STORES.SETTINGS, () => { setSettings(fetchedSettings); rootStore.setDefaultLocale(fetchedSettings.defaultLocale); + // Set MFA enforced state even for public settings mode + // as it is needed to determine if the MFA setup page should be shown + isMFAEnforced.value = settings.value.mfa?.enforced ?? false; + if (fetchedSettings.settingsMode === 'public') { // public settings mode is typically used for unauthenticated users // when public settings are returned we can skip the rest of the setup @@ -255,8 +259,6 @@ export const useSettingsStore = defineStore(STORES.SETTINGS, () => { setSaveDataProgressExecution(fetchedSettings.saveExecutionProgress); setSaveManualExecutions(fetchedSettings.saveManualExecutions); - isMFAEnforced.value = settings.value.mfa?.enforced ?? false; - rootStore.setUrlBaseWebhook(fetchedSettings.urlBaseWebhook); rootStore.setUrlBaseEditor(fetchedSettings.urlBaseEditor); rootStore.setEndpointForm(fetchedSettings.endpointForm);