mirror of
https://github.com/n8n-io/n8n.git
synced 2026-08-28 17:22:01 +08:00
fix(core): Fix redirection of user missing MFA to personal settings (#23881)
This commit is contained in:
committed by
GitHub
parent
3cc0552cef
commit
072a1dd825
@@ -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<
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
|
||||
@@ -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);
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
@@ -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<PublicFrontendSettings> {
|
||||
async getPublicSettings(includeMfaSettings: boolean): Promise<PublicFrontendSettings> {
|
||||
// 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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user