Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
23 changes: 23 additions & 0 deletions packages/functional-tests/pages/settings/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,38 @@ export class SettingsPage extends SettingsLayout {
}, creds.uid);
}

/**
* Removes 2FA from the account by clicking the 'disable' button on the 2FA row.
*/
async disconnectTotp() {
await this.totp.disableButton.click();

// Obtain the MFA JWT if necessary
if (await this.isMfaGuardVisible()) {
Comment thread
dschom marked this conversation as resolved.
const email = await this.primaryEmail.status.textContent();
if (!email) {
throw new Error('Could not determine primary email!');
}
await this.confirmMfaGuard(email);
}

await this.modalConfirmButton.click();

await expect(this.settingsHeading).toBeVisible();
await expect(this.alertBar).toHaveText('Two-step authentication disabled');
await expect(this.totp.status).toHaveText('Disabled');
}

/**
* Indicates that the MFA guard's modal dialog is currently displayed.
* @returns true if the MFA modal is open
*/
async isMfaGuardVisible() {
return await this.page
.getByRole('heading', { name: 'Enter confirmation code' })
Comment thread
dschom marked this conversation as resolved.
.isVisible();
}

/**
* Confirms the MFA modal (MfaGuard) by retrieving the code from the inbox
* and submitting it. Use when an action triggers the protected modal.
Expand Down
9 changes: 1 addition & 8 deletions packages/functional-tests/tests/oauth/totp.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,7 @@ test.describe('severity-1 #smoke', () => {
await expect(settings.alertBar).toHaveText(
'Two-step authentication has been enabled'
);
await settings.totp.disableButton.click();
await settings.clickModalConfirm();

await expect(settings.modalConfirmButton).toBeHidden();
await expect(settings.totp.status).toHaveText('Disabled');
await expect(settings.alertBar).toHaveText(
'Two-step authentication disabled'
);
await settings.disconnectTotp();

await relier.goto();
await relier.clickEmailFirst();
Expand Down
20 changes: 20 additions & 0 deletions packages/fxa-auth-client/lib/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1871,10 +1871,30 @@ export default class AuthClient {
return this.jwtPost('/mfa/totp/replace/confirm', jwt, { code }, headers);
}

/**
* @deprecated Use deleteTotpTokenWithJwt instead
*
* Disables 2FA Protection on the account.
*
* @param sessionToken - required, must be a verified session token
* @param headers - Optional additional headers for the request
* @returns A promise that resolves when the 2FA has been removed
*/
async deleteTotpToken(sessionToken: hexstring, headers?: Headers) {
return this.sessionPost('/totp/destroy', sessionToken, {}, headers);
}

/**
* Disables 2FA Protection on the account.
*
* @param jwt - required, must be a verified session token
* @param headers - Optional additional headers for the request
* @returns A promise that resolves when the 2FA has been removed
*/
async deleteTotpTokenWithJwt(jwt: string, headers?: Headers) {
return this.jwtPost('/mfa/totp/destroy', jwt, {}, headers);
}

async checkTotpTokenExists(
sessionToken: hexstring,
headers?: Headers
Expand Down
12 changes: 12 additions & 0 deletions packages/fxa-auth-server/docs/swagger/totp-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ const TOTP_DESTROY_POST = {
`,
],
};
const MFA_TOTP_DESTROY_POST = {
...TAGS_TOTP,
description: '/mfa/totp/destroy',
notes: [
dedent`
🔒 Authenticated with MFA JWT (scope: mfa:2fa)

Deletes the current TOTP token for the user. The underlying session needs to have been verified by TOTP to remove it. It does not bypass that requirement.
`,
],
};

const TOTP_EXISTS_GET = {
...TAGS_TOTP,
Expand Down Expand Up @@ -158,6 +169,7 @@ const API_DOCS = {
SESSION_VERIFY_TOTP_POST,
TOTP_CREATE_POST,
TOTP_DESTROY_POST,
MFA_TOTP_DESTROY_POST,
TOTP_EXISTS_GET,
TOTP_VERIFY_POST,
TOTP_VERIFY_RECOVERY_CODE_POST,
Expand Down
25 changes: 24 additions & 1 deletion packages/fxa-auth-server/lib/routes/totp.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ module.exports = (
}
}

return [
const routes = [
{
method: 'POST',
path: '/totp/create',
Expand Down Expand Up @@ -588,6 +588,27 @@ module.exports = (
}
},
},
{
method: 'POST',
path: '/mfa/totp/destroy',
options: {
...TOTP_DOCS.MFA_TOTP_DESTROY_POST,
auth: {
strategy: 'mfa',
scope: ['mfa:2fa'],
payload: false,
},
response: {},
},
handler: async function (request) {
return routes
.find(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MagentaManifold @nshirley @vpomerleau I like this approach for the simple reason that it doesn't corrupt the git history of the previous route handler and makes the diff nicer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, neat! 🤯

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

(route) =>
route.path === '/v1/totp/destroy' && route.method === 'POST'
)
.handler(request);
},
},
{
method: 'POST',
path: '/totp/destroy',
Expand Down Expand Up @@ -1163,4 +1184,6 @@ module.exports = (
handler: handleTotpReplaceConfirm,
},
];

return routes;
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
MOCK_NATIONAL_FORMAT_PHONE_NUMBER,
} from '../../../pages/mocks';
import GleanMetrics from '../../../lib/glean';
import { JwtTokenCache } from '../../../lib/cache';

jest.mock('../../../models/AlertBarInfo');

Expand All @@ -21,7 +22,32 @@ jest.mock('../../../lib/glean', () => ({
},
}));

jest.mock('../../../models', () => ({
...jest.requireActual('../../../models'),
useAuthClient: () => ({
mfaRequestOtp: jest.fn(),
}),
}));

// Mocks to suppress MFA Guard
jest.mock('../../../lib/cache', () => ({
...jest.requireActual('../../../lib/cache'),
JwtTokenCache: {
hasToken: jest.fn(),
getToken: jest.fn(),
getSnapshot: jest.fn(),
subscribe: jest.fn(),
},
sessionToken: () => 'session-123',
}));

describe('UnitRowTwoStepAuth', () => {
beforeEach(() => {
(JwtTokenCache.hasToken as jest.Mock).mockReturnValue(true);
(JwtTokenCache.getToken as jest.Mock).mockReturnValue('jwt-123');
(JwtTokenCache.getSnapshot as jest.Mock).mockReturnValue(true);
});

it('renders when two-step authentication is enabled', async () => {
renderWithRouter(createSubject());

Expand Down Expand Up @@ -129,6 +155,17 @@ describe('UnitRowTwoStepAuth', () => {
);
});

it('renders MFAGuard when the disable totp modal is rendered and jwt is missing', async () => {
(JwtTokenCache.hasToken as jest.Mock).mockReturnValue(false);
const user = userEvent.setup();

renderWithRouter(createSubject());

await user.click(screen.getByRole('button', { name: 'Disable' }));

expect(screen.getByText('Enter confirmation code')).toBeInTheDocument();
});

it('renders with no backup codes and no recovery phone', () => {
renderWithRouter(
createSubject({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import React, { useCallback, useEffect } from 'react';
import * as Sentry from '@sentry/browser';
import { useErrorHandler } from 'react-error-boundary';
import LinkExternal from 'fxa-react/components/LinkExternal';
import { useBooleanState } from 'fxa-react/lib/hooks';
import Modal from '../Modal';
Expand All @@ -22,6 +24,8 @@ import { useNavigateWithQuery } from '../../../lib/hooks/useNavigateWithQuery';
import { formatPhoneNumber } from '../../../lib/recovery-phone-utils';
import { RecoveryPhoneSetupReason } from '../../../lib/types';
import ModalVerifySession from '../ModalVerifySession';
import { MfaGuard } from '../MfaGuard';
import { isInvalidJwtError } from '../../../lib/mfa-guard-utils';

const route = `${SETTINGS_PATH}/two_step_authentication`;
const replaceCodesRoute = `${route}/replace_codes`;
Expand Down Expand Up @@ -209,7 +213,14 @@ export const UnitRowTwoStepAuth = () => {
/>
)}
{disable2FAModalRevealed && (
<DisableTwoStepAuthModal {...{ hideDisable2FAModal }} />
<MfaGuard
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nshirley This seems to be all that's needed to wrap a button. Note that in this scenario the onDismissCallback is needed. This was recently added in #19443.

requiredScope={'2fa'}
onDismissCallback={async () => {
hideDisable2FAModal();
}}
>
<DisableTwoStepAuthModal {...{ hideDisable2FAModal }} />
</MfaGuard>
)}
</UnitRow>
</>
Expand All @@ -221,6 +232,7 @@ const DisableTwoStepAuthModal = ({
}: {
hideDisable2FAModal: () => void;
}) => {
const errorHandler = useErrorHandler();
const alertBar = useAlertBar();
const account = useAccount();
const ftlMsgResolver = useFtlMsgResolver();
Expand All @@ -243,15 +255,23 @@ const DisableTwoStepAuthModal = ({
() => GleanMetrics.accountPref.twoStepAuthDisableSuccessView()
);
} catch (e) {
if (isInvalidJwtError(e)) {
// JWT invalid/expired.
errorHandler(e);
return;
}

hideDisable2FAModal();
alertBar.error(
ftlMsgResolver.getMsg(
'tfa-row-cannot-disable-2',
'Two-step authentication could not be disabled'
)
);

Sentry.captureException(e);
}
}, [account, hideDisable2FAModal, alertBar, ftlMsgResolver]);
}, [account, hideDisable2FAModal, alertBar, ftlMsgResolver, errorHandler]);

return (
<VerifiedSessionGuard
Expand Down
2 changes: 1 addition & 1 deletion packages/fxa-settings/src/models/Account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1126,7 +1126,7 @@ export class Account implements AccountData {

async disableTwoStepAuth() {
await this.withLoadingStatus(
this.authClient.deleteTotpToken(sessionToken()!)
this.authClient.deleteTotpTokenWithJwt(this.getCachedJwtByScope('2fa'))
);

const cache = this.apolloClient.cache;
Expand Down