fix: Allow disabling MFA using recovery code #11908

RicardoE105 · 2024-11-26T22:07:00Z

Summary

1 - Allow users to disable MFA also using the recovery code
2 - Standardize how we refer to MFA code and recovery code

Before

Now

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/ADO-2936/can-not-disable-mfa-with-recovery-code

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

tomi · 2024-11-27T08:46:40Z

packages/cli/src/errors/response-errors/invalid-mfa-code.error.ts

@@ -2,6 +2,6 @@ import { ForbiddenError } from './forbidden.error';

 export class InvalidMfaCodeError extends ForbiddenError {


We should change the name of this error class as well, if it's not only about mfa code anymore. Is this used in other places where the error might now be incorrect?

tomi · 2024-11-27T08:49:33Z

packages/editor-ui/src/stores/users.store.ts

@@ -331,9 +331,10 @@ export const useUsersStore = defineStore(STORES.USERS, () => {
 		}
 	};

-	const disableMfa = async (mfaCode: string) => {
+	const disableMfa = async (mfaCode: string, recoveryCode: string) => {


Shouldn't this be:

Suggested change

const disableMfa = async (mfaCode: string, recoveryCode: string) => {

const disableMfa = async (mfaCode?: string, recoveryCode?: string) => {

?

tomi · 2024-11-27T08:49:47Z

packages/editor-ui/src/plugins/i18n/locales/en.json

@@ -2608,7 +2609,8 @@
 	"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",
+	"mfa.prompt.code.modal.divider": "Or",


Suggested change

"mfa.prompt.code.modal.divider": "Or",

"mfa.prompt.code.modal.divider": "or",

tomi · 2024-11-27T08:52:01Z

packages/editor-ui/src/components/PromptMfaCodeModal/PromptMfaCodeModal.vue

+	promptMfaCodeBus.emit('close', {
+		mfaCode: mfaCode.value,
+		recoveryCode: recoveryCode.value,
+	});


I think we should map this to an object that only has one or the other, not both.

tomi · 2024-11-27T08:53:23Z

packages/editor-ui/src/api/mfa.ts

 export type DisableMfaParams = {
 	token: string;
+	recoveryCode: string;
 };


The type is incorrect, as only one or the other is required. Would this work?

Suggested change

export type DisableMfaParams = {

token: string;

recoveryCode: string;

};

export type DisableMfaParams = { token: string; } | { recoveryCode: string; }

or this?

Suggested change

export type DisableMfaParams = {

token: string;

recoveryCode: string;

};

export type DisableMfaParams = {

token?: string;

recoveryCode?: string;

};

I always send both because in the backend we have a function that check both and return true/false depending on whether one of the them is valid. It's the same function to login with MFA. The alternative would have been to check in the FE, and in the BE for which one we are sending and validate always only the one we send, and that would have been some IF's conditions that I wanted to avoid.

But yeah, I see where you are coming from that is kind of consuming that you need to send both. Will address

tomi · 2024-11-27T08:57:17Z

The form now allows entering both MFA code and recovery code. I think it prefers the MFA code in that case. Would probably make more sense to allow user to input only one or the other, and have the TS types match that

RicardoE105 · 2024-11-27T13:37:41Z

The form now allows entering both MFA code and recovery code. I think it prefers the MFA code in that case. Would probably make more sense to allow user to input only one or the other, and have the TS types match that

We could:

1 - Add an info box saying that if you send both, the token takes priority.
2 - Disable the save button if both are set, and show an error messages "you can only send one"
3 - Add a select which two options -> MFA token and recovery code, and show the correct field depending on the that.
4 - Suggested by Adi -> maybe we should combine the two input boxes in the future, and figure out based on the string format if it's a TOTP code, or a recovery code.

tomi · 2024-11-27T14:04:26Z

We could:

1 - Add an info box saying that if you send both, the token takes priority. 2 - Disable the save button if both are set, and show an error messages "you can only send one" 3 - Add a select which two options -> MFA token and recovery code, and show the correct field depending on the that. 4 - Suggested by Adi -> maybe we should combine the two input boxes in the future, and figure out based on the string format if it's a TOTP code, or a recovery code.

Yeah I think option #4 would be the best one 👍

Also updates the UI to allow the use of recovery codes when disabling MFA

codecov · 2024-11-27T16:50:30Z

Codecov Report

Attention: Patch coverage is 65.65657% with 34 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...mponents/PromptMfaCodeModal/PromptMfaCodeModal.vue	0.00%	14 Missing ⚠️
packages/editor-ui/src/api/mfa.ts	0.00%	6 Missing ⚠️
...ackages/editor-ui/src/views/ChangePasswordView.vue	0.00%	3 Missing ⚠️
packages/editor-ui/src/api/users.ts	0.00%	2 Missing ⚠️
...ackages/editor-ui/src/components/MfaSetupModal.vue	0.00%	2 Missing ⚠️
packages/editor-ui/src/stores/users.store.ts	71.42%	2 Missing ⚠️
packages/editor-ui/src/views/SigninView.vue	60.00%	2 Missing ⚠️
...s/cli/src/controllers/password-reset.controller.ts	66.66%	1 Missing ⚠️
packages/cli/src/mfa/mfa.service.ts	91.66%	1 Missing ⚠️
...kages/editor-ui/src/views/SettingsPersonalView.vue	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

tomi

Nice work 👌 Few comments. Also I would prefer if the separated the renaming refactor into a separate PR. That way the changes stay separate and are easier to for example revert if ever needed

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

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

packages/cli/src/mfa/mfa.service.ts

packages/editor-ui/src/components/PromptMfaCodeModal/PromptMfaCodeModal.vue

packages/editor-ui/src/views/MfaView.vue

RicardoE105 · 2024-11-28T13:25:38Z

@tomi 🙏 Thanks for the feedback. Applied it all and whenever I have time I will split this PR into two as you suggested.

…-code

Allow disabling MFA using recovery code

cd853e2

RicardoE105 changed the title ~~Allow disabling MFA using recovery code~~ fix: Allow disabling MFA using recovery code Nov 26, 2024

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Nov 26, 2024

fix test

f6e4118

RicardoE105 requested a review from MiloradFilipovic November 26, 2024 23:00

tomi reviewed Nov 27, 2024

View reviewed changes

standardize how we refer to mfa code and recovery code

f2a540d

Also updates the UI to allow the use of recovery codes when disabling MFA

RicardoE105 requested a review from tomi November 27, 2024 16:28

fix linting issue

a0afd32

tomi reviewed Nov 28, 2024

View reviewed changes

Apply feedback

afe75f3

RicardoE105 added 3 commits November 28, 2024 08:27

fix small overlooked

e473be5

refactor condition on endpoint to disable MFA

ce16e30

Merge branch 'master' into ado-2936-can-not-disable-mfa-with-recovery…

a33e9f2

…-code

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Allow disabling MFA using recovery code #11908

fix: Allow disabling MFA using recovery code #11908

RicardoE105 commented Nov 26, 2024 •

edited

Loading

tomi Nov 27, 2024

tomi Nov 27, 2024

tomi Nov 27, 2024

tomi Nov 27, 2024

tomi Nov 27, 2024

RicardoE105 Nov 27, 2024

RicardoE105 Nov 27, 2024

tomi commented Nov 27, 2024

RicardoE105 commented Nov 27, 2024 •

edited

Loading

tomi commented Nov 27, 2024

codecov bot commented Nov 27, 2024 •

edited

Loading

tomi left a comment

RicardoE105 commented Nov 28, 2024

		@@ -2,6 +2,6 @@ import { ForbiddenError } from './forbidden.error';

		export class InvalidMfaCodeError extends ForbiddenError {

	const disableMfa = async (mfaCode: string, recoveryCode: string) => {
	const disableMfa = async (mfaCode?: string, recoveryCode?: string) => {

	"mfa.prompt.code.modal.divider": "Or",
	"mfa.prompt.code.modal.divider": "or",

fix: Allow disabling MFA using recovery code #11908

Are you sure you want to change the base?

fix: Allow disabling MFA using recovery code #11908

Conversation

RicardoE105 commented Nov 26, 2024 • edited Loading

Summary

Before

Now

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

tomi Nov 27, 2024

Choose a reason for hiding this comment

tomi Nov 27, 2024

Choose a reason for hiding this comment

tomi Nov 27, 2024

Choose a reason for hiding this comment

tomi Nov 27, 2024

Choose a reason for hiding this comment

tomi Nov 27, 2024

Choose a reason for hiding this comment

RicardoE105 Nov 27, 2024

Choose a reason for hiding this comment

RicardoE105 Nov 27, 2024

Choose a reason for hiding this comment

tomi commented Nov 27, 2024

RicardoE105 commented Nov 27, 2024 • edited Loading

tomi commented Nov 27, 2024

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

tomi left a comment

Choose a reason for hiding this comment

RicardoE105 commented Nov 28, 2024

RicardoE105 commented Nov 26, 2024 •

edited

Loading

RicardoE105 commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading