From 3384f52a35b835ba1d8633dc94bab0ad6e7023b3 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Tue, 13 Aug 2024 15:56:54 +0300 Subject: [PATCH] fix: Require mfa code to disable mfa (#10345) --- packages/cli/src/Mfa/mfa.service.ts | 18 ++-- .../__tests__/me.controller.test.ts | 8 +- packages/cli/src/controllers/me.controller.ts | 4 +- .../cli/src/controllers/mfa.controller.ts | 15 ++-- .../response-errors/invalid-mfa-code.error.ts | 7 ++ packages/cli/src/requests.ts | 1 + .../cli/test/integration/mfa/mfa.api.test.ts | 26 +++++- packages/editor-ui/src/api/mfa.ts | 8 +- packages/editor-ui/src/components/Modal.vue | 10 ++- packages/editor-ui/src/components/Modals.vue | 6 ++ .../PromptMfaCodeModal/PromptMfaCodeModal.vue | 84 +++++++++++++++++++ packages/editor-ui/src/constants.ts | 1 + packages/editor-ui/src/event-bus/mfa.ts | 15 ++++ .../src/plugins/i18n/locales/en.json | 1 + packages/editor-ui/src/stores/ui.store.ts | 4 +- packages/editor-ui/src/stores/users.store.ts | 13 +-- .../src/views/SettingsPersonalView.vue | 38 +++++++-- 17 files changed, 215 insertions(+), 44 deletions(-) create mode 100644 packages/cli/src/errors/response-errors/invalid-mfa-code.error.ts create mode 100644 packages/editor-ui/src/components/PromptMfaCodeModal/PromptMfaCodeModal.vue diff --git a/packages/cli/src/Mfa/mfa.service.ts b/packages/cli/src/Mfa/mfa.service.ts index 43b1d3438b22b..aa6fddb5a93ce 100644 --- a/packages/cli/src/Mfa/mfa.service.ts +++ b/packages/cli/src/Mfa/mfa.service.ts @@ -3,6 +3,7 @@ import { Service } from 'typedi'; import { Cipher } from 'n8n-core'; import { AuthUserRepository } from '@db/repositories/authUser.repository'; import { TOTPService } from './totp.service'; +import { InvalidMfaCodeError } from '@/errors/response-errors/invalid-mfa-code.error'; @Service() export class MfaService { @@ -82,11 +83,16 @@ export class MfaService { return await this.authUserRepository.save(user); } - async disableMfa(userId: string) { - const user = await this.authUserRepository.findOneByOrFail({ id: userId }); - user.mfaEnabled = false; - user.mfaSecret = null; - user.mfaRecoveryCodes = []; - return await this.authUserRepository.save(user); + async disableMfa(userId: string, mfaToken: string) { + const isValidToken = await this.validateMfa(userId, mfaToken, undefined); + if (!isValidToken) { + throw new InvalidMfaCodeError(); + } + + await this.authUserRepository.update(userId, { + mfaEnabled: false, + mfaSecret: null, + mfaRecoveryCodes: [], + }); } } diff --git a/packages/cli/src/controllers/__tests__/me.controller.test.ts b/packages/cli/src/controllers/__tests__/me.controller.test.ts index df22c377bca9b..3ff4c5bbe0860 100644 --- a/packages/cli/src/controllers/__tests__/me.controller.test.ts +++ b/packages/cli/src/controllers/__tests__/me.controller.test.ts @@ -17,7 +17,7 @@ import { badPasswords } from '@test/testData'; import { mockInstance } from '@test/mocking'; import { AuthUserRepository } from '@/databases/repositories/authUser.repository'; import { MfaService } from '@/Mfa/mfa.service'; -import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; +import { InvalidMfaCodeError } from '@/errors/response-errors/invalid-mfa-code.error'; const browserId = 'test-browser-id'; @@ -230,16 +230,14 @@ describe('MeController', () => { ); }); - it('should throw ForbiddenError if invalid mfa code is given', async () => { + it('should throw InvalidMfaCodeError if invalid mfa code is given', async () => { const req = mock({ user: mock({ password: passwordHash, mfaEnabled: true }), body: { currentPassword: 'old_password', newPassword: 'NewPassword123', mfaCode: '123' }, }); mockMfaService.validateMfa.mockResolvedValue(false); - await expect(controller.updatePassword(req, mock())).rejects.toThrowError( - new ForbiddenError('Invalid two-factor code.'), - ); + await expect(controller.updatePassword(req, mock())).rejects.toThrow(InvalidMfaCodeError); }); it('should succeed when mfa code is correct', async () => { diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 4089cd3523310..179076d25d224 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/packages/cli/src/controllers/me.controller.ts @@ -24,7 +24,7 @@ import { UserRepository } from '@/databases/repositories/user.repository'; import { isApiEnabled } from '@/PublicApi'; import { EventService } from '@/events/event.service'; import { MfaService } from '@/Mfa/mfa.service'; -import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; +import { InvalidMfaCodeError } from '@/errors/response-errors/invalid-mfa-code.error'; export const API_KEY_PREFIX = 'n8n_api_'; @@ -155,7 +155,7 @@ export class MeController { const isMfaTokenValid = await this.mfaService.validateMfa(user.id, mfaCode, undefined); if (!isMfaTokenValid) { - throw new ForbiddenError('Invalid two-factor code.'); + throw new InvalidMfaCodeError(); } } diff --git a/packages/cli/src/controllers/mfa.controller.ts b/packages/cli/src/controllers/mfa.controller.ts index 4d3c6c0d9f50c..6c228df47e90e 100644 --- a/packages/cli/src/controllers/mfa.controller.ts +++ b/packages/cli/src/controllers/mfa.controller.ts @@ -1,4 +1,4 @@ -import { Delete, Get, Post, RestController } from '@/decorators'; +import { Get, Post, RestController } from '@/decorators'; import { AuthenticatedRequest, MFA } from '@/requests'; import { MfaService } from '@/Mfa/mfa.service'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -71,11 +71,16 @@ export class MFAController { await this.mfaService.enableMfa(id); } - @Delete('/disable') - async disableMFA(req: AuthenticatedRequest) { - const { id } = req.user; + @Post('/disable', { rateLimit: true }) + async disableMFA(req: MFA.Disable) { + const { id: userId } = req.user; + const { token = null } = req.body; + + if (typeof token !== 'string' || !token) { + throw new BadRequestError('Token is required to disable MFA feature'); + } - await this.mfaService.disableMfa(id); + await this.mfaService.disableMfa(userId, token); } @Post('/verify', { rateLimit: true }) diff --git a/packages/cli/src/errors/response-errors/invalid-mfa-code.error.ts b/packages/cli/src/errors/response-errors/invalid-mfa-code.error.ts new file mode 100644 index 0000000000000..cc00976a84ef5 --- /dev/null +++ b/packages/cli/src/errors/response-errors/invalid-mfa-code.error.ts @@ -0,0 +1,7 @@ +import { ForbiddenError } from './forbidden.error'; + +export class InvalidMfaCodeError extends ForbiddenError { + constructor(hint?: string) { + super('Invalid two-factor code.', hint); + } +} diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 5301351c770df..565020447c91d 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -353,6 +353,7 @@ export type LoginRequest = AuthlessRequest< export declare namespace MFA { type Verify = AuthenticatedRequest<{}, {}, { token: string }, {}>; type Activate = AuthenticatedRequest<{}, {}, { token: string }, {}>; + type Disable = AuthenticatedRequest<{}, {}, { token: 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 index 7755ad4e3ceab..792ad1cb8876c 100644 --- a/packages/cli/test/integration/mfa/mfa.api.test.ts +++ b/packages/cli/test/integration/mfa/mfa.api.test.ts @@ -48,7 +48,8 @@ describe('Enable MFA setup', () => { secondCall.body.data.recoveryCodes.join(''), ); - await testServer.authAgentFor(owner).delete('/mfa/disable').expect(200); + const token = new TOTPService().generateTOTP(firstCall.body.data.secret); + await testServer.authAgentFor(owner).post('/mfa/disable').send({ token }).expect(200); const thirdCall = await testServer.authAgentFor(owner).get('/mfa/qr').expect(200); @@ -135,9 +136,16 @@ describe('Enable MFA setup', () => { describe('Disable MFA setup', () => { test('POST /disable should disable login with MFA', async () => { - const { user } = await createUserWithMfaEnabled(); + const { user, rawSecret } = await createUserWithMfaEnabled(); + const token = new TOTPService().generateTOTP(rawSecret); - await testServer.authAgentFor(user).delete('/mfa/disable').expect(200); + await testServer + .authAgentFor(user) + .post('/mfa/disable') + .send({ + token, + }) + .expect(200); const dbUser = await Container.get(AuthUserRepository).findOneOrFail({ where: { id: user.id }, @@ -147,6 +155,18 @@ describe('Disable MFA setup', () => { expect(dbUser.mfaSecret).toBe(null); expect(dbUser.mfaRecoveryCodes.length).toBe(0); }); + + test('POST /disable should fail if invalid token is given', async () => { + const { user } = await createUserWithMfaEnabled(); + + await testServer + .authAgentFor(user) + .post('/mfa/disable') + .send({ + token: 'invalid token', + }) + .expect(403); + }); }); describe('Change password with MFA enabled', () => { diff --git a/packages/editor-ui/src/api/mfa.ts b/packages/editor-ui/src/api/mfa.ts index dcc5d2131399e..09cfb84df40bf 100644 --- a/packages/editor-ui/src/api/mfa.ts +++ b/packages/editor-ui/src/api/mfa.ts @@ -18,6 +18,10 @@ export async function verifyMfaToken( return await makeRestApiRequest(context, 'POST', '/mfa/verify', data); } -export async function disableMfa(context: IRestApiContext): Promise { - return await makeRestApiRequest(context, 'DELETE', '/mfa/disable'); +export type DisableMfaParams = { + token: string; +}; + +export async function disableMfa(context: IRestApiContext, data: DisableMfaParams): Promise { + return await makeRestApiRequest(context, 'POST', '/mfa/disable', data); } diff --git a/packages/editor-ui/src/components/Modal.vue b/packages/editor-ui/src/components/Modal.vue index 7a26314ad0dc8..141e93a521472 100644 --- a/packages/editor-ui/src/components/Modal.vue +++ b/packages/editor-ui/src/components/Modal.vue @@ -1,7 +1,7 @@