Skip to content

Commit

Permalink
fix: Require mfa code to disable mfa (#10345)
Browse files Browse the repository at this point in the history
  • Loading branch information
tomi authored Aug 13, 2024
1 parent e950df0 commit 3384f52
Show file tree
Hide file tree
Showing 17 changed files with 215 additions and 44 deletions.
18 changes: 12 additions & 6 deletions packages/cli/src/Mfa/mfa.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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: [],
});
}
}
8 changes: 3 additions & 5 deletions packages/cli/src/controllers/__tests__/me.controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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<MeRequest.Password>({
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 () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/controllers/me.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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_';

Expand Down Expand Up @@ -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();
}
}

Expand Down
15 changes: 10 additions & 5 deletions packages/cli/src/controllers/mfa.controller.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 })
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { ForbiddenError } from './forbidden.error';

export class InvalidMfaCodeError extends ForbiddenError {
constructor(hint?: string) {
super('Invalid two-factor code.', hint);
}
}
1 change: 1 addition & 0 deletions packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<
{},
Expand Down
26 changes: 23 additions & 3 deletions packages/cli/test/integration/mfa/mfa.api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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 },
Expand All @@ -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', () => {
Expand Down
8 changes: 6 additions & 2 deletions packages/editor-ui/src/api/mfa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ export async function verifyMfaToken(
return await makeRestApiRequest(context, 'POST', '/mfa/verify', data);
}

export async function disableMfa(context: IRestApiContext): Promise<void> {
return await makeRestApiRequest(context, 'DELETE', '/mfa/disable');
export type DisableMfaParams = {
token: string;
};

export async function disableMfa(context: IRestApiContext, data: DisableMfaParams): Promise<void> {
return await makeRestApiRequest(context, 'POST', '/mfa/disable', data);
}
10 changes: 7 additions & 3 deletions packages/editor-ui/src/components/Modal.vue
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<template>
<el-dialog
:model-value="uiStore.modalsById[name].open"
:before-close="closeDialog"
:before-close="onCloseDialog"
:class="{
'dialog-wrapper': true,
scrollable: scrollable,
Expand Down Expand Up @@ -34,7 +34,7 @@
class="modal-content"
@keydown.stop
@keydown.enter="handleEnter"
@keydown.esc="closeDialog"
@keydown.esc="onCloseDialog"
>
<slot v-if="!loading" name="content" />
<div v-else :class="$style.loader">
Expand Down Expand Up @@ -182,7 +182,10 @@ export default defineComponent({
this.$emit('enter');
}
},
async closeDialog() {
async onCloseDialog() {
await this.closeDialog();
},
async closeDialog(returnData?: unknown) {
if (this.beforeClose) {
const shouldClose = await this.beforeClose();
if (shouldClose === false) {
Expand All @@ -191,6 +194,7 @@ export default defineComponent({
}
}
this.uiStore.closeModal(this.name);
this.eventBus?.emit('closed', returnData);
},
getCustomClass() {
let classes = this.customClass || '';
Expand Down
6 changes: 6 additions & 0 deletions packages/editor-ui/src/components/Modals.vue
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
SETUP_CREDENTIALS_MODAL_KEY,
PROJECT_MOVE_RESOURCE_MODAL,
PROJECT_MOVE_RESOURCE_CONFIRM_MODAL,
PROMPT_MFA_CODE_MODAL_KEY,
} from '@/constants';
import AboutModal from '@/components/AboutModal.vue';
Expand Down Expand Up @@ -63,6 +64,7 @@ import WorkflowHistoryVersionRestoreModal from '@/components/WorkflowHistory/Wor
import SetupWorkflowCredentialsModal from '@/components/SetupWorkflowCredentialsModal/SetupWorkflowCredentialsModal.vue';
import ProjectMoveResourceModal from '@/components/Projects/ProjectMoveResourceModal.vue';
import ProjectMoveResourceConfirmModal from '@/components/Projects/ProjectMoveResourceConfirmModal.vue';
import PromptMfaCodeModal from './PromptMfaCodeModal/PromptMfaCodeModal.vue';
</script>

<template>
Expand Down Expand Up @@ -144,6 +146,10 @@ import ProjectMoveResourceConfirmModal from '@/components/Projects/ProjectMoveRe
<MfaSetupModal />
</ModalRoot>

<ModalRoot :name="PROMPT_MFA_CODE_MODAL_KEY">
<PromptMfaCodeModal />
</ModalRoot>

<ModalRoot :name="WORKFLOW_SHARE_MODAL_KEY">
<template #default="{ modalName, active, data }">
<WorkflowShareModal :data="data" :is-active="active" :modal-name="modalName" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
<template>
<Modal
width="460px"
height="300px"
max-height="640px"
:title="i18n.baseText('mfa.prompt.code.modal.title')"
:event-bus="promptMfaCodeBus"
:name="PROMPT_MFA_CODE_MODAL_KEY"
:center="true"
>
<template #content>
<div :class="[$style.formContainer]">
<n8n-form-inputs
data-test-id="mfa-code-form"
:inputs="formFields"
:event-bus="formBus"
@submit="onSubmit"
@ready="onFormReady"
/>
</div>
</template>
<template #footer>
<div>
<n8n-button
float="right"
:disabled="!readyToSubmit"
:label="i18n.baseText('settings.personal.save')"
size="large"
data-test-id="mfa-save-button"
@click="onClickSave"
/>
</div>
</template>
</Modal>
</template>

<script setup lang="ts">
import { ref } from 'vue';
import Modal from '../Modal.vue';
import { PROMPT_MFA_CODE_MODAL_KEY } from '@/constants';
import { useI18n } from '@/composables/useI18n';
import { promptMfaCodeBus } from '@/event-bus';
import type { IFormInputs } from '@/Interface';
import { createFormEventBus } from 'n8n-design-system';
const i18n = useI18n();
const formBus = ref(createFormEventBus());
const readyToSubmit = ref(false);
const formFields: IFormInputs = [
{
name: 'mfaCode',
initialValue: '',
properties: {
label: i18n.baseText('mfa.code.input.label'),
placeholder: i18n.baseText('mfa.code.input.placeholder'),
focusInitially: true,
capitalize: true,
required: true,
},
},
];
function onSubmit(values: { mfaCode: string }) {
promptMfaCodeBus.emit('close', {
mfaCode: values.mfaCode,
});
}
function onClickSave() {
formBus.value.emit('submit');
}
function onFormReady(isReady: boolean) {
readyToSubmit.value = isReady;
}
</script>

<style lang="scss" module>
.formContainer {
padding-bottom: var(--spacing-xl);
}
</style>
1 change: 1 addition & 0 deletions packages/editor-ui/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export const SOURCE_CONTROL_PUSH_MODAL_KEY = 'sourceControlPush';
export const SOURCE_CONTROL_PULL_MODAL_KEY = 'sourceControlPull';
export const DEBUG_PAYWALL_MODAL_KEY = 'debugPaywall';
export const MFA_SETUP_MODAL_KEY = 'mfaSetup';
export const PROMPT_MFA_CODE_MODAL_KEY = 'promptMfaCode';
export const WORKFLOW_HISTORY_VERSION_RESTORE = 'workflowHistoryVersionRestore';
export const SETUP_CREDENTIALS_MODAL_KEY = 'setupCredentials';
export const PROJECT_MOVE_RESOURCE_MODAL = 'projectMoveResourceModal';
Expand Down
15 changes: 15 additions & 0 deletions packages/editor-ui/src/event-bus/mfa.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
import { createEventBus } from 'n8n-design-system/utils';

export const mfaEventBus = createEventBus();

export interface MfaModalClosedEventPayload {
mfaCode: string;
}

export interface MfaModalEvents {
close: MfaModalClosedEventPayload | undefined;

closed: MfaModalClosedEventPayload | undefined;
}

/**
* Event bus for transmitting the MFA code from a modal back to the view
*/
export const promptMfaCodeBus = createEventBus<MfaModalEvents>();
1 change: 1 addition & 0 deletions packages/editor-ui/src/plugins/i18n/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -2521,6 +2521,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",
"settings.personal.mfa.section.title": "Two-factor authentication (2FA)",
"settings.personal.personalisation": "Personalisation",
"settings.personal.theme": "Theme",
Expand Down
4 changes: 3 additions & 1 deletion packages/editor-ui/src/stores/ui.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
SETUP_CREDENTIALS_MODAL_KEY,
PROJECT_MOVE_RESOURCE_MODAL,
PROJECT_MOVE_RESOURCE_CONFIRM_MODAL,
PROMPT_MFA_CODE_MODAL_KEY,
} from '@/constants';
import type {
CloudUpdateLinkSourceType,
Expand Down Expand Up @@ -114,6 +115,7 @@ export const useUIStore = defineStore(STORES.UI, () => {
WORKFLOW_ACTIVE_MODAL_KEY,
COMMUNITY_PACKAGE_INSTALL_MODAL_KEY,
MFA_SETUP_MODAL_KEY,
PROMPT_MFA_CODE_MODAL_KEY,
SOURCE_CONTROL_PUSH_MODAL_KEY,
SOURCE_CONTROL_PULL_MODAL_KEY,
EXTERNAL_SECRETS_PROVIDER_MODAL_KEY,
Expand Down Expand Up @@ -696,7 +698,7 @@ export const useUIStore = defineStore(STORES.UI, () => {
});

/**
* Helper function for listening to credential changes in the store
* Helper function for listening to model opening and closings in the store
*/
export const listenForModalChanges = (opts: {
store: UiStore;
Expand Down
Loading

0 comments on commit 3384f52

Please sign in to comment.