Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Allow disabling MFA using recovery code #11908

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions cypress/e2e/27-two-factor-authentication.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,28 @@ describe('Two-factor authentication', { disableAutoLogin: true }, () => {
mainSidebar.actions.signout();
});

it('Should be able to disable MFA in account', () => {
it('Should be able to disable MFA in account with MFA token ', () => {
const { email, password } = user;
signinPage.actions.loginWithEmailAndPassword(email, password);
personalSettingsPage.actions.enableMfa();
mainSidebar.actions.signout();
const token = generateOTPToken(user.mfaSecret);
mfaLoginPage.actions.loginWithMfaToken(email, password, token);
personalSettingsPage.actions.disableMfa();
const loginToken = generateOTPToken(user.mfaSecret);
mfaLoginPage.actions.loginWithMfaToken(email, password, loginToken);
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 loginToken = generateOTPToken(user.mfaSecret);
mfaLoginPage.actions.loginWithMfaToken(email, password, loginToken);
personalSettingsPage.actions.disableMfa(user.mfaRecoveryCodes[0]);
personalSettingsPage.getters.enableMfaButton().should('exist');
mainSidebar.actions.signout();
});
});
6 changes: 5 additions & 1 deletion cypress/pages/settings-personal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ export class PersonalSettingsPage extends BasePage {
saveSettingsButton: () => cy.getByTestId('save-settings-button'),
enableMfaButton: () => cy.getByTestId('enable-mfa-button'),
disableMfaButton: () => cy.getByTestId('disable-mfa-button'),
mfaCodeInput: () => cy.getByTestId('mfa-code-input'),
mfaSaveButton: () => cy.getByTestId('mfa-save-button'),
themeSelector: () => cy.getByTestId('theme-select'),
selectOptionsVisible: () => cy.get('.el-select-dropdown:visible .el-select-dropdown__item'),
};
Expand Down Expand Up @@ -83,9 +85,11 @@ export class PersonalSettingsPage extends BasePage {
mfaSetupModal.getters.saveButton().click();
});
},
disableMfa: () => {
disableMfa: (code: string) => {
cy.visit(this.url);
this.getters.disableMfaButton().click();
this.getters.mfaCodeInput().type(code);
this.getters.mfaSaveButton().click();
},
};
}
12 changes: 4 additions & 8 deletions packages/cli/src/controllers/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export class AuthController {
/** Log in a user */
@Post('/login', { skipAuth: true, rateLimit: true })
async login(req: LoginRequest, res: Response): Promise<PublicUser | undefined> {
const { email, password, mfaToken, mfaRecoveryCode } = req.body;
const { email, password, mfaCode, mfaRecoveryCode } = req.body;
if (!email) throw new ApplicationError('Email is required to log in');
if (!password) throw new ApplicationError('Password is required to log in');

Expand Down Expand Up @@ -75,16 +75,12 @@ export class AuthController {

if (user) {
if (user.mfaEnabled) {
if (!mfaToken && !mfaRecoveryCode) {
if (!mfaCode && !mfaRecoveryCode) {
throw new AuthError('MFA Error', 998);
}

const isMFATokenValid = await this.mfaService.validateMfa(
user.id,
mfaToken,
mfaRecoveryCode,
);
if (!isMFATokenValid) {
const isMFACodeValid = await this.mfaService.validateMfa(user.id, mfaCode, mfaRecoveryCode);
if (!isMFACodeValid) {
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved
throw new AuthError('Invalid mfa token or recovery code');
}
}
Expand Down
8 changes: 4 additions & 4 deletions packages/cli/src/controllers/me.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ export class MeController {
throw new BadRequestError('Two-factor code is required to change email');
}

const isMfaTokenValid = await this.mfaService.validateMfa(userId, payload.mfaCode, undefined);
if (!isMfaTokenValid) {
const isMfaCodeValid = await this.mfaService.validateMfa(userId, payload.mfaCode, undefined);
if (!isMfaCodeValid) {
throw new InvalidMfaCodeError();
}
}
Expand Down Expand Up @@ -142,8 +142,8 @@ export class MeController {
throw new BadRequestError('Two-factor code is required to change password.');
}

const isMfaTokenValid = await this.mfaService.validateMfa(user.id, mfaCode, undefined);
if (!isMfaTokenValid) {
const isMfaCodeValid = await this.mfaService.validateMfa(user.id, mfaCode, undefined);
if (!isMfaCodeValid) {
throw new InvalidMfaCodeError();
}
}
Expand Down
25 changes: 13 additions & 12 deletions packages/cli/src/controllers/mfa.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,54 +59,55 @@ export class MFAController {

@Post('/enable', { rateLimit: true })
async activateMFA(req: MFA.Activate) {
const { token = null } = req.body;
const { mfaCode = null } = req.body;
const { id, mfaEnabled } = req.user;

await this.externalHooks.run('mfa.beforeSetup', [req.user]);

const { decryptedSecret: secret, decryptedRecoveryCodes: recoveryCodes } =
await this.mfaService.getSecretAndRecoveryCodes(id);

if (!token) throw new BadRequestError('Token is required to enable MFA feature');
if (!mfaCode) throw new BadRequestError('MFA code is required to enable MFA feature');

if (mfaEnabled) throw new BadRequestError('MFA already enabled');

if (!secret || !recoveryCodes.length) {
throw new BadRequestError('Cannot enable MFA without generating secret and recovery codes');
}

const verified = this.mfaService.totp.verifySecret({ secret, token, window: 10 });
const verified = this.mfaService.totp.verifySecret({ secret, mfaCode, window: 10 });

if (!verified)
throw new BadRequestError('MFA token expired. Close the modal and enable MFA again', 997);
throw new BadRequestError('MFA code expired. Close the modal and enable MFA again', 997);

await this.mfaService.enableMfa(id);
}

@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');
}
const { mfaCode, mfaRecoveryCode } = req.body;

await this.mfaService.disableMfa(userId, token);
if (mfaCode && typeof mfaCode === 'string') {
await this.mfaService.disableMfaWithMfaCode(userId, mfaCode);
} else if (mfaRecoveryCode && typeof mfaRecoveryCode === 'string') {
await this.mfaService.disableMfaWithRecoveryCode(userId, mfaRecoveryCode);
}
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved
}

@Post('/verify', { rateLimit: true })
async verifyMFA(req: MFA.Verify) {
const { id } = req.user;
const { token } = req.body;
const { mfaCode } = req.body;

const { decryptedSecret: secret } = await this.mfaService.getSecretAndRecoveryCodes(id);

if (!token) throw new BadRequestError('Token is required to enable MFA feature');
if (!mfaCode) throw new BadRequestError('MFA code is required to enable MFA feature');

if (!secret) throw new BadRequestError('No MFA secret se for this user');

const verified = this.mfaService.totp.verifySecret({ secret, token });
const verified = this.mfaService.totp.verifySecret({ secret, mfaCode });

if (!verified) throw new BadRequestError('MFA secret could not be verified');
}
Expand Down
6 changes: 3 additions & 3 deletions packages/cli/src/controllers/password-reset.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export class PasswordResetController {
*/
@Post('/change-password', { skipAuth: true })
async changePassword(req: PasswordResetRequest.NewPassword, res: Response) {
const { token, password, mfaToken } = req.body;
const { token, password, mfaCode } = req.body;

if (!token || !password) {
this.logger.debug(
Expand All @@ -189,11 +189,11 @@ export class PasswordResetController {
if (!user) throw new NotFoundError('');

if (user.mfaEnabled) {
if (!mfaToken) throw new BadRequestError('If MFA enabled, mfaToken is required.');
if (!mfaCode) throw new BadRequestError('If MFA enabled, mfaCode is required.');

const { decryptedSecret: secret } = await this.mfaService.getSecretAndRecoveryCodes(user.id);

const validToken = this.mfaService.totp.verifySecret({ secret, token: mfaToken });
const validToken = this.mfaService.totp.verifySecret({ secret, mfaCode });

if (!validToken) throw new BadRequestError('Invalid MFA token.');
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { ForbiddenError } from './forbidden.error';

export class InvalidMfaRecoveryCodeError extends ForbiddenError {
constructor(hint?: string) {
super('Invalid MFA recovery code', hint);
}
}
26 changes: 21 additions & 5 deletions packages/cli/src/mfa/mfa.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -56,13 +57,13 @@ export class MfaService {

async validateMfa(
userId: string,
mfaToken: string | undefined,
mfaCode: string | undefined,
mfaRecoveryCode: string | undefined,
) {
const user = await this.authUserRepository.findOneByOrFail({ id: userId });
if (mfaToken) {
if (mfaCode) {
const decryptedSecret = this.cipher.decrypt(user.mfaSecret!);
return this.totp.verifySecret({ secret: decryptedSecret, token: mfaToken });
return this.totp.verifySecret({ secret: decryptedSecret, mfaCode });
}

if (mfaRecoveryCode) {
Expand All @@ -85,12 +86,27 @@ export class MfaService {
return await this.authUserRepository.save(user);
}

async disableMfa(userId: string, mfaToken: string) {
const isValidToken = await this.validateMfa(userId, mfaToken, undefined);
async disableMfaWithMfaCode(userId: string, mfaCode: string) {
const isValidToken = await this.validateMfa(userId, mfaCode, '');
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved

if (!isValidToken) {
throw new InvalidMfaCodeError();
}

await this.disableMfaForUser(userId);
}

async disableMfaWithRecoveryCode(userId: string, recoveryCode: string) {
const isValidToken = await this.validateMfa(userId, '', recoveryCode);
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved

if (!isValidToken) {
throw new InvalidMfaRecoveryCodeError();
}

await this.disableMfaForUser(userId);
}

private async disableMfaForUser(userId: string) {
await this.authUserRepository.update(userId, {
mfaEnabled: false,
mfaSecret: null,
Expand Down
8 changes: 6 additions & 2 deletions packages/cli/src/mfa/totp.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,14 @@ export class TOTPService {
}).toString();
}

verifySecret({ secret, token, window = 2 }: { secret: string; token: string; window?: number }) {
verifySecret({
secret,
mfaCode,
window = 2,
}: { secret: string; mfaCode: string; window?: number }) {
return new OTPAuth.TOTP({
secret: OTPAuth.Secret.fromBase32(secret),
}).validate({ token, window }) === null
}).validate({ token: mfaCode, window }) === null
? false
: true;
}
Expand Down
10 changes: 5 additions & 5 deletions packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ export declare namespace PasswordResetRequest {
export type NewPassword = AuthlessRequest<
{},
{},
Pick<PublicUser, 'password'> & { token?: string; userId?: string; mfaToken?: string }
Pick<PublicUser, 'password'> & { token?: string; userId?: string; mfaCode?: string }
>;
}

Expand Down Expand Up @@ -306,7 +306,7 @@ export type LoginRequest = AuthlessRequest<
{
email: string;
password: string;
mfaToken?: string;
mfaCode?: string;
mfaRecoveryCode?: string;
}
>;
Expand All @@ -316,9 +316,9 @@ export type LoginRequest = AuthlessRequest<
// ----------------------------------

export declare namespace MFA {
type Verify = AuthenticatedRequest<{}, {}, { token: string }, {}>;
type Activate = AuthenticatedRequest<{}, {}, { token: string }, {}>;
type Disable = AuthenticatedRequest<{}, {}, { token: string }, {}>;
type Verify = AuthenticatedRequest<{}, {}, { mfaCode: string }, {}>;
type Activate = AuthenticatedRequest<{}, {}, { mfaCode: string }, {}>;
type Disable = AuthenticatedRequest<{}, {}, { mfaCode?: string; mfaRecoveryCode?: string }, {}>;
type Config = AuthenticatedRequest<{}, {}, { login: { enabled: boolean } }, {}>;
type ValidateRecoveryCode = AuthenticatedRequest<
{},
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/test/integration/auth.api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ describe('POST /login', () => {
const response = await testServer.authlessAgent.post('/login').send({
email: owner.email,
password: ownerPassword,
mfaToken: mfaService.totp.generateTOTP(secret),
mfaCode: mfaService.totp.generateTOTP(secret),
});

expect(response.statusCode).toBe(200);
Expand Down
Loading