From c0e60be36d3c84781ea4f6250aa0974c2c64aee6 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Fri, 9 Aug 2024 18:12:13 +0300 Subject: [PATCH] fix: Require mfa code to disable mfa Continuation of #10341 Also enables rate limiting for the `/mfa/disable` endpoint. For the FE: - adds a typed event emitter - a mechanism to use Modal event emitter to provide data back from the modal --- 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/design-system/src/utils/event-bus.ts | 48 ++--------- .../src/utils/typed-event-bus.ts | 36 ++++++++ packages/editor-ui/src/api/mfa.ts | 8 +- packages/editor-ui/src/components/Modal.vue | 7 +- 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 | 22 ++++- .../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 +++++++-- 19 files changed, 260 insertions(+), 87 deletions(-) create mode 100644 packages/cli/src/errors/response-errors/invalid-mfa-code.error.ts create mode 100644 packages/design-system/src/utils/typed-event-bus.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 43b1d3438b22bd..aa6fddb5a93ce8 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 df22c377bca9b6..3ff4c5bbe08604 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 4089cd35233107..179076d25d2243 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 4d3c6c0d9f50c2..6c228df47e90e2 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 00000000000000..cc00976a84ef5a --- /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 5301351c770df8..565020447c91d3 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 7755ad4e3ceab1..792ad1cb8876ca 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/design-system/src/utils/event-bus.ts b/packages/design-system/src/utils/event-bus.ts index 1b7cb10182a48c..23f5e5f02acf8e 100644 --- a/packages/design-system/src/utils/event-bus.ts +++ b/packages/design-system/src/utils/event-bus.ts @@ -2,51 +2,17 @@ export type CallbackFn = Function; export type UnregisterFn = () => void; -export type Listener = (payload: Payload) => void; - -export type Payloads = { - [E in keyof ListenerMap]: unknown; -}; - -// TODO: Fix all usages of `createEventBus` and convert `any` to `unknown` -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export interface EventBus = Record> { - on: ( - eventName: EventName, - fn: Listener, - ) => UnregisterFn; - - once: ( - eventName: EventName, - fn: Listener, - ) => UnregisterFn; - - off: ( - eventName: EventName, - fn: Listener, - ) => void; - - emit: ( - eventName: EventName, - event?: ListenerMap[EventName], - ) => void; +export interface EventBus { + on: (eventName: string, fn: CallbackFn) => UnregisterFn; + once: (eventName: string, fn: CallbackFn) => UnregisterFn; + off: (eventName: string, fn: CallbackFn) => void; + emit: (eventName: string, event?: T) => void; } /** - * Creates an event bus with the given listener map. - * - * @example - * ```ts - * const eventBus = createEventBus<{ - * 'user-logged-in': { username: string }; - * 'user-logged-out': never; - * }>(); + * @deprecated Use the typed version instead from `typed-event-bus.ts` */ -export function createEventBus< - // TODO: Fix all usages of `createEventBus` and convert `any` to `unknown` - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ListenerMap extends Payloads = Record, ->(): EventBus { +export function createEventBus(): EventBus { const handlers = new Map(); function off(eventName: string, fn: CallbackFn) { diff --git a/packages/design-system/src/utils/typed-event-bus.ts b/packages/design-system/src/utils/typed-event-bus.ts new file mode 100644 index 00000000000000..8290b68fb2a8a2 --- /dev/null +++ b/packages/design-system/src/utils/typed-event-bus.ts @@ -0,0 +1,36 @@ +import type { UnregisterFn } from './event-bus'; +import { createEventBus as createUntypedEventBus } from './event-bus'; + +export type Listener = (payload: Payload) => void; + +export type Payloads = { + [E in keyof ListenerMap]: unknown; +}; + +export interface TypedEventBus> { + on: ( + eventName: EventName, + fn: Listener, + ) => UnregisterFn; + + once: ( + eventName: EventName, + fn: Listener, + ) => UnregisterFn; + + off: ( + eventName: EventName, + fn: Listener, + ) => void; + + emit: ( + eventName: EventName, + event?: ListenerMap[EventName], + ) => void; +} + +export function createTypedEventBus< + ListenerMap extends Payloads, +>(): TypedEventBus { + return createUntypedEventBus() as TypedEventBus; +} diff --git a/packages/editor-ui/src/api/mfa.ts b/packages/editor-ui/src/api/mfa.ts index dcc5d2131399e7..09cfb84df40bfc 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 7a26314ad0dc83..7f4a8334af6fad 100644 --- a/packages/editor-ui/src/components/Modal.vue +++ b/packages/editor-ui/src/components/Modal.vue @@ -1,7 +1,7 @@