fix: Allow disabling MFA with recovery codes (#12014)

Co-authored-by: Tomi Turtiainen <[email protected]>
n8n-io · Dec 4, 2024 · 95d56fe · 95d56fe
1 parent 2b6a72f
commit 95d56fe
Show file tree

Hide file tree

Showing 12 changed files with 98 additions and 25 deletions.
diff --git a/cypress/e2e/27-two-factor-authentication.cy.ts b/cypress/e2e/27-two-factor-authentication.cy.ts
@@ -68,16 +68,28 @@ describe('Two-factor authentication', { disableAutoLogin: true }, () => {
 		mainSidebar.actions.signout();
 	});
 
-	it('Should be able to disable MFA in account with MFA code ', () => {
+	it('Should be able to disable MFA in account with MFA code', () => {
 		const { email, password } = user;
 		signinPage.actions.loginWithEmailAndPassword(email, password);
 		personalSettingsPage.actions.enableMfa();
 		mainSidebar.actions.signout();
-		const loginToken = generateOTPToken(user.mfaSecret);
-		mfaLoginPage.actions.loginWithMfaCode(email, password, loginToken);
+		const mfaCode = generateOTPToken(user.mfaSecret);
+		mfaLoginPage.actions.loginWithMfaCode(email, password, mfaCode);
 		const disableToken = generateOTPToken(user.mfaSecret);
 		personalSettingsPage.actions.disableMfa(disableToken);
 		personalSettingsPage.getters.enableMfaButton().should('exist');
 		mainSidebar.actions.signout();
 	});
+
+	it('Should be able to disable MFA in account with recovery code', () => {
+		const { email, password } = user;
+		signinPage.actions.loginWithEmailAndPassword(email, password);
+		personalSettingsPage.actions.enableMfa();
+		mainSidebar.actions.signout();
+		const mfaCode = generateOTPToken(user.mfaSecret);
+		mfaLoginPage.actions.loginWithMfaCode(email, password, mfaCode);
+		personalSettingsPage.actions.disableMfa(user.mfaRecoveryCodes[0]);
+		personalSettingsPage.getters.enableMfaButton().should('exist');
+		mainSidebar.actions.signout();
+	});
 });
diff --git a/packages/cli/src/controllers/mfa.controller.ts b/packages/cli/src/controllers/mfa.controller.ts
@@ -86,13 +86,24 @@ export class MFAController {
 	@Post('/disable', { rateLimit: true })
 	async disableMFA(req: MFA.Disable) {
 		const { id: userId } = req.user;
-		const { mfaCode = null } = req.body;
 
-		if (typeof mfaCode !== 'string' || !mfaCode) {
-			throw new BadRequestError('Token is required to disable MFA feature');
+		const { mfaCode, mfaRecoveryCode } = req.body;
+
+		const mfaCodeDefined = mfaCode && typeof mfaCode === 'string';
+
+		const mfaRecoveryCodeDefined = mfaRecoveryCode && typeof mfaRecoveryCode === 'string';
+
+		if (!mfaCodeDefined === !mfaRecoveryCodeDefined) {
+			throw new BadRequestError(
+				'Either MFA code or recovery code is required to disable MFA feature',
+			);
 		}
 
-		await this.mfaService.disableMfa(userId, mfaCode);
+		if (mfaCodeDefined) {
+			await this.mfaService.disableMfaWithMfaCode(userId, mfaCode);
+		} else if (mfaRecoveryCodeDefined) {
+			await this.mfaService.disableMfaWithRecoveryCode(userId, mfaRecoveryCode);
+		}
 	}
 
 	@Post('/verify', { rateLimit: true })

diff --git a/packages/cli/src/errors/response-errors/invalid-mfa-recovery-code-error.ts b/packages/cli/src/errors/response-errors/invalid-mfa-recovery-code-error.ts
@@ -0,0 +1,7 @@
+import { ForbiddenError } from './forbidden.error';
+
+export class InvalidMfaRecoveryCodeError extends ForbiddenError {
+	constructor(hint?: string) {
+		super('Invalid MFA recovery code', hint);
+	}
+}
diff --git a/packages/cli/src/mfa/mfa.service.ts b/packages/cli/src/mfa/mfa.service.ts
@@ -4,6 +4,7 @@ import { v4 as uuid } from 'uuid';
 
 import { AuthUserRepository } from '@/databases/repositories/auth-user.repository';
 import { InvalidMfaCodeError } from '@/errors/response-errors/invalid-mfa-code.error';
+import { InvalidMfaRecoveryCodeError } from '@/errors/response-errors/invalid-mfa-recovery-code-error';
 
 import { TOTPService } from './totp.service';
 
@@ -85,12 +86,27 @@ export class MfaService {
 		return await this.authUserRepository.save(user);
 	}
 
-	async disableMfa(userId: string, mfaCode: string) {
+	async disableMfaWithMfaCode(userId: string, mfaCode: string) {
 		const isValidToken = await this.validateMfa(userId, mfaCode, undefined);
+
 		if (!isValidToken) {
 			throw new InvalidMfaCodeError();
 		}
 
+		await this.disableMfaForUser(userId);
+	}
+
+	async disableMfaWithRecoveryCode(userId: string, recoveryCode: string) {
+		const isValidToken = await this.validateMfa(userId, undefined, recoveryCode);
+
+		if (!isValidToken) {
+			throw new InvalidMfaRecoveryCodeError();
+		}
+
+		await this.disableMfaForUser(userId);
+	}
+
+	private async disableMfaForUser(userId: string) {
 		await this.authUserRepository.update(userId, {
 			mfaEnabled: false,
 			mfaSecret: null,

diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts
@@ -318,7 +318,7 @@ export type LoginRequest = AuthlessRequest<
 export declare namespace MFA {
 	type Verify = AuthenticatedRequest<{}, {}, { mfaCode: string }, {}>;
 	type Activate = AuthenticatedRequest<{}, {}, { mfaCode: string }, {}>;
-	type Disable = AuthenticatedRequest<{}, {}, { mfaCode: string }, {}>;
+	type Disable = AuthenticatedRequest<{}, {}, { mfaCode?: string; mfaRecoveryCode?: string }, {}>;
 	type Config = AuthenticatedRequest<{}, {}, { login: { enabled: boolean } }, {}>;
 	type ValidateRecoveryCode = AuthenticatedRequest<
 		{},

diff --git a/packages/cli/test/integration/mfa/mfa.api.test.ts b/packages/cli/test/integration/mfa/mfa.api.test.ts
@@ -184,7 +184,19 @@ describe('Disable MFA setup', () => {
 		expect(dbUser.mfaRecoveryCodes.length).toBe(0);
 	});
 
-	test('POST /disable should fail if invalid mfaCode is given', async () => {
+	test('POST /disable should fail if invalid MFA recovery code is given', async () => {
+		const { user } = await createUserWithMfaEnabled();
+
+		await testServer
+			.authAgentFor(user)
+			.post('/mfa/disable')
+			.send({
+				mfaRecoveryCode: 'invalid token',
+			})
+			.expect(403);
+	});
+
+	test('POST /disable should fail if invalid MFA code is given', async () => {
 		const { user } = await createUserWithMfaEnabled();
 
 		await testServer
@@ -195,6 +207,12 @@ describe('Disable MFA setup', () => {
 			})
 			.expect(403);
 	});
+
+	test('POST /disable should fail if neither MFA code nor recovery code is sent', async () => {
+		const { user } = await createUserWithMfaEnabled();
+
+		await testServer.authAgentFor(user).post('/mfa/disable').send({ anotherParam: '' }).expect(400);
+	});
 });
 
 describe('Change password with MFA enabled', () => {

diff --git a/packages/editor-ui/src/api/mfa.ts b/packages/editor-ui/src/api/mfa.ts
@@ -26,7 +26,8 @@ export async function verifyMfaCode(
 }
 
 export type DisableMfaParams = {
-	mfaCode: string;
+	mfaCode?: string;
+	mfaRecoveryCode?: string;
 };
 
 export async function disableMfa(context: IRestApiContext, data: DisableMfaParams): Promise<void> {

diff --git a/packages/editor-ui/src/components/PromptMfaCodeModal/PromptMfaCodeModal.vue b/packages/editor-ui/src/components/PromptMfaCodeModal/PromptMfaCodeModal.vue
@@ -6,6 +6,7 @@ import { useI18n } from '@/composables/useI18n';
 import { promptMfaCodeBus } from '@/event-bus';
 import type { IFormInputs } from '@/Interface';
 import { createFormEventBus } from 'n8n-design-system';
+import { validate as validateUuid } from 'uuid';
 
 const i18n = useI18n();
 
@@ -14,21 +15,27 @@ const readyToSubmit = ref(false);
 
 const formFields: IFormInputs = [
 	{
-		name: 'mfaCode',
+		name: 'mfaCodeOrMfaRecoveryCode',
 		initialValue: '',
 		properties: {
-			label: i18n.baseText('mfa.code.input.label'),
-			placeholder: i18n.baseText('mfa.code.input.placeholder'),
+			label: i18n.baseText('mfa.code.recovery.input.label'),
+			placeholder: i18n.baseText('mfa.code.recovery.input.placeholder'),
 			focusInitially: true,
 			capitalize: true,
 			required: true,
 		},
 	},
 ];
 
-function onSubmit(values: { mfaCode: string }) {
+function onSubmit(values: { mfaCodeOrMfaRecoveryCode: string }) {
+	if (validateUuid(values.mfaCodeOrMfaRecoveryCode)) {
+		promptMfaCodeBus.emit('close', {
+			mfaRecoveryCode: values.mfaCodeOrMfaRecoveryCode,
+		});
+		return;
+	}
 	promptMfaCodeBus.emit('close', {
-		mfaCode: values.mfaCode,
+		mfaCode: values.mfaCodeOrMfaRecoveryCode,
 	});
 }
 
@@ -43,7 +50,7 @@ function onFormReady(isReady: boolean) {
 
 <template>
 	<Modal
-		width="460px"
+		width="500px"
 		height="300px"
 		max-height="640px"
 		:title="i18n.baseText('mfa.prompt.code.modal.title')"

diff --git a/packages/editor-ui/src/event-bus/mfa.ts b/packages/editor-ui/src/event-bus/mfa.ts
@@ -3,7 +3,8 @@ import { createEventBus } from 'n8n-design-system/utils';
 export const mfaEventBus = createEventBus();
 
 export interface MfaModalClosedEventPayload {
-	mfaCode: string;
+	mfaCode?: string;
+	mfaRecoveryCode?: string;
 }
 
 export interface MfaModalEvents {

diff --git a/packages/editor-ui/src/plugins/i18n/locales/en.json b/packages/editor-ui/src/plugins/i18n/locales/en.json
@@ -2596,7 +2596,9 @@
 	"mfa.button.back": "Back",
 	"mfa.code.input.label": "Two-factor code",
 	"mfa.code.input.placeholder": "e.g. 123456",
-	"mfa.recovery.input.label": "Recovery Code",
+	"mfa.code.recovery.input.label": "Two-factor code or recovery code",
+	"mfa.code.recovery.input.placeholder": "e.g. 123456 or c79f9c02-7b2e-44...",
+	"mfa.recovery.input.label": "Recovery code",
 	"mfa.recovery.input.placeholder": "e.g c79f9c02-7b2e-44...",
 	"mfa.code.invalid": "This code is invalid, try again or",
 	"mfa.recovery.invalid": "This code is invalid or was already used, try again",
@@ -2620,7 +2622,7 @@
 	"mfa.setup.step2.toast.setupFinished.message": "Two-factor authentication enabled",
 	"mfa.setup.step2.toast.setupFinished.error.message": "Error enabling two-factor authentication",
 	"mfa.setup.step2.toast.tokenExpired.error.message": "MFA token expired. Close the modal and enable MFA again",
-	"mfa.prompt.code.modal.title": "Two-factor code required",
+	"mfa.prompt.code.modal.title": "Two-factor code or recovery code required",
 	"settings.personal.mfa.section.title": "Two-factor authentication (2FA)",
 	"settings.personal.personalisation": "Personalisation",
 	"settings.personal.theme": "Theme",

diff --git a/packages/editor-ui/src/stores/users.store.ts b/packages/editor-ui/src/stores/users.store.ts
@@ -331,10 +331,8 @@ export const useUsersStore = defineStore(STORES.USERS, () => {
 		}
 	};
 
-	const disableMfa = async (mfaCode: string) => {
-		await mfaApi.disableMfa(rootStore.restApiContext, {
-			mfaCode,
-		});
+	const disableMfa = async (data: mfaApi.DisableMfaParams) => {
+		await mfaApi.disableMfa(rootStore.restApiContext, data);
 
 		if (currentUser.value) {
 			currentUser.value.mfaEnabled = false;

diff --git a/packages/editor-ui/src/views/SettingsPersonalView.vue b/packages/editor-ui/src/views/SettingsPersonalView.vue
@@ -226,7 +226,7 @@ async function disableMfa(payload: MfaModalEvents['closed']) {
 	}
 
 	try {
-		await usersStore.disableMfa(payload.mfaCode);
+		await usersStore.disableMfa(payload);
 
 		showToast({
 			title: i18n.baseText('settings.personal.mfa.toast.disabledMfa.title'),