Skip to content

Commit

Permalink
refactor: Standardize MFA code and recovery code naming across code b…
Browse files Browse the repository at this point in the history
…ase (#12011)
  • Loading branch information
RicardoE105 authored Dec 3, 2024
1 parent f16de4d commit 70706d8
Show file tree
Hide file tree
Showing 22 changed files with 150 additions and 137 deletions.
20 changes: 11 additions & 9 deletions cypress/e2e/27-two-factor-authentication.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,33 +49,35 @@ describe('Two-factor authentication', { disableAutoLogin: true }, () => {
cy.intercept('GET', '/rest/mfa/qr').as('getMfaQrCode');
});

it('Should be able to login with MFA token', () => {
it('Should be able to login with MFA code', () => {
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);
const mfaCode = generateOTPToken(user.mfaSecret);
mfaLoginPage.actions.loginWithMfaCode(email, password, mfaCode);
mainSidebar.actions.signout();
});

it('Should be able to login with recovery code', () => {
it('Should be able to login with MFA recovery code', () => {
const { email, password } = user;
signinPage.actions.loginWithEmailAndPassword(email, password);
personalSettingsPage.actions.enableMfa();
mainSidebar.actions.signout();
mfaLoginPage.actions.loginWithRecoveryCode(email, password, user.mfaRecoveryCodes[0]);
mfaLoginPage.actions.loginWithMfaRecoveryCode(email, password, user.mfaRecoveryCodes[0]);
mainSidebar.actions.signout();
});

it('Should be able to disable MFA in account', () => {
it('Should be able to disable MFA in account with MFA code ', () => {
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.loginWithMfaCode(email, password, loginToken);
const disableToken = generateOTPToken(user.mfaSecret);
personalSettingsPage.actions.disableMfa(disableToken);
personalSettingsPage.getters.enableMfaButton().should('exist');
mainSidebar.actions.signout();
});
});
16 changes: 8 additions & 8 deletions cypress/pages/mfa-login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@ export class MfaLoginPage extends BasePage {

getters = {
form: () => cy.getByTestId('mfa-login-form'),
token: () => cy.getByTestId('token'),
recoveryCode: () => cy.getByTestId('recoveryCode'),
mfaCode: () => cy.getByTestId('mfaCode'),
mfaRecoveryCode: () => cy.getByTestId('mfaRecoveryCode'),
enterRecoveryCodeButton: () => cy.getByTestId('mfa-enter-recovery-code-button'),
};

actions = {
loginWithMfaToken: (email: string, password: string, mfaToken: string) => {
loginWithMfaCode: (email: string, password: string, mfaCode: string) => {
const signinPage = new SigninPage();
const workflowsPage = new WorkflowsPage();

cy.session(
[mfaToken],
[mfaCode],
() => {
cy.visit(signinPage.url);

Expand All @@ -30,7 +30,7 @@ export class MfaLoginPage extends BasePage {
});

this.getters.form().within(() => {
this.getters.token().type(mfaToken);
this.getters.mfaCode().type(mfaCode);
});

// we should be redirected to /workflows
Expand All @@ -43,12 +43,12 @@ export class MfaLoginPage extends BasePage {
},
);
},
loginWithRecoveryCode: (email: string, password: string, recoveryCode: string) => {
loginWithMfaRecoveryCode: (email: string, password: string, mfaRecoveryCode: string) => {
const signinPage = new SigninPage();
const workflowsPage = new WorkflowsPage();

cy.session(
[recoveryCode],
[mfaRecoveryCode],
() => {
cy.visit(signinPage.url);

Expand All @@ -61,7 +61,7 @@ export class MfaLoginPage extends BasePage {
this.getters.enterRecoveryCodeButton().click();

this.getters.form().within(() => {
this.getters.recoveryCode().type(recoveryCode);
this.getters.mfaRecoveryCode().type(mfaRecoveryCode);
});

// we should be redirected to /workflows
Expand Down
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'),
mfaCodeOrMfaRecoveryCodeInput: () => cy.getByTestId('mfa-code-or-recovery-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: (mfaCodeOrRecoveryCode: string) => {
cy.visit(this.url);
this.getters.disableMfaButton().click();
this.getters.mfaCodeOrMfaRecoveryCodeInput().type(mfaCodeOrRecoveryCode);
this.getters.mfaSaveButton().click();
},
};
}
10 changes: 5 additions & 5 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,16 @@ 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(
const isMfaCodeOrMfaRecoveryCodeValid = await this.mfaService.validateMfa(
user.id,
mfaToken,
mfaCode,
mfaRecoveryCode,
);
if (!isMFATokenValid) {
if (!isMfaCodeOrMfaRecoveryCodeValid) {
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
20 changes: 10 additions & 10 deletions packages/cli/src/controllers/mfa.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,54 +59,54 @@ 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('Token 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;
const { mfaCode = null } = req.body;

if (typeof token !== 'string' || !token) {
if (typeof mfaCode !== 'string' || !mfaCode) {
throw new BadRequestError('Token is required to disable MFA feature');
}

await this.mfaService.disableMfa(userId, token);
await this.mfaService.disableMfa(userId, mfaCode);
}

@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
10 changes: 5 additions & 5 deletions packages/cli/src/mfa/mfa.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,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,8 +85,8 @@ export class MfaService {
return await this.authUserRepository.save(user);
}

async disableMfa(userId: string, mfaToken: string) {
const isValidToken = await this.validateMfa(userId, mfaToken, undefined);
async disableMfa(userId: string, mfaCode: string) {
const isValidToken = await this.validateMfa(userId, mfaCode, undefined);
if (!isValidToken) {
throw new InvalidMfaCodeError();
}
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 }, {}>;
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

0 comments on commit 70706d8

Please sign in to comment.