Skip to content

Commit 234a670

Browse files
committed
task(settings): Add MFA guard to remove 2FA SMS backup
Because: - We want to protect this action with MFA This Commit: - Wraps the PageRecoveryPhoneRemove component with an MfaGuard
1 parent 0836a94 commit 234a670

9 files changed

Lines changed: 68 additions & 24 deletions

File tree

packages/functional-tests/tests/settings/recoveryPhone.spec.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ test.describe('severity-1 #smoke', () => {
9999
await expect(settings.alertBar).toHaveText('Recovery phone added');
100100

101101
await settings.totp.removeRecoveryPhoneButton.click();
102+
102103
await page.waitForURL(/recovery_phone\/remove/);
103104
await page.getByRole('button', { name: 'Remove phone number' }).click();
104105

packages/fxa-auth-client/lib/client.ts

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1531,14 +1531,16 @@ export default class AuthClient {
15311531
} = {},
15321532
headers?: Headers
15331533
): Promise<SignedInAccountData> {
1534-
1535-
const oldCredentials = await this.sessionReauth(sessionToken, options.reauthEmail || email, oldPassword, {
1536-
keys: true
1537-
}, headers);
1538-
const oldCredentialsAuth = await crypto.getCredentials(
1539-
email,
1540-
oldPassword
1534+
const oldCredentials = await this.sessionReauth(
1535+
sessionToken,
1536+
options.reauthEmail || email,
1537+
oldPassword,
1538+
{
1539+
keys: true,
1540+
},
1541+
headers
15411542
);
1543+
const oldCredentialsAuth = await crypto.getCredentials(email, oldPassword);
15421544
const oldAuthPW = oldCredentialsAuth.authPW;
15431545

15441546
const keys = await this.accountKeys(
@@ -1547,10 +1549,7 @@ export default class AuthClient {
15471549
headers
15481550
);
15491551

1550-
const newCredentials = await crypto.getCredentials(
1551-
email,
1552-
newPassword
1553-
);
1552+
const newCredentials = await crypto.getCredentials(email, newPassword);
15541553

15551554
const wrapKb = crypto.unwrapKB(keys.kB, newCredentials.unwrapBKey);
15561555
const authPW = newCredentials.authPW;
@@ -1600,8 +1599,8 @@ export default class AuthClient {
16001599
if (
16011600
error &&
16021601
error.email &&
1603-
error.errno === ERRORS.INCORRECT_EMAIL_CASE
1604-
&& !options.skipCaseError
1602+
error.errno === ERRORS.INCORRECT_EMAIL_CASE &&
1603+
!options.skipCaseError
16051604
) {
16061605
options.skipCaseError = true;
16071606
options.reauthEmail = email;
@@ -2979,7 +2978,7 @@ export default class AuthClient {
29792978
}
29802979

29812980
/**
2982-
* Removes a recovery phone from the user's account
2981+
* @deprecated Use recoveryPhoneDeleteWithJwt instead
29832982
*
29842983
* @param sessionToken The user's current session token
29852984
* @param headers
@@ -2988,6 +2987,17 @@ export default class AuthClient {
29882987
return this.sessionDelete('/recovery_phone', sessionToken, {}, headers);
29892988
}
29902989

2990+
/**
2991+
* Disables 2FA Protection on the account.
2992+
*
2993+
* @param jwt - required, must be a verified session token
2994+
* @param headers - Optional additional headers for the request
2995+
* @returns A promise that resolves when the 2FA has been removed
2996+
*/
2997+
async recoveryPhoneDeleteWithJwt(jwt: string, headers?: Headers) {
2998+
return this.jwtDelete('/mfa/recovery_phone', jwt, {}, headers);
2999+
}
3000+
29913001
/**
29923002
* Gets status of the recovery phone on the users account.
29933003
* @param sessionToken The user's current session token

packages/fxa-auth-server/lib/routes/recovery-phone.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,6 +1193,26 @@ export const recoveryPhoneRoutes = (
11931193
return recoveryPhoneHandler.destroy(request);
11941194
},
11951195
},
1196+
{
1197+
method: 'DELETE',
1198+
path: '/mfa/recovery_phone',
1199+
options: {
1200+
auth: {
1201+
strategy: 'mfa',
1202+
scope: ['mfa:2fa'],
1203+
payload: false,
1204+
},
1205+
response: {},
1206+
},
1207+
handler: async function (request: AuthRequest) {
1208+
return routes
1209+
.find(
1210+
(route) =>
1211+
route.path === '/v1/recovery_phone' && route.method === 'DELETE'
1212+
)
1213+
?.handler(request);
1214+
},
1215+
},
11961216
{
11971217
method: 'GET',
11981218
path: '/recovery_phone',

packages/fxa-settings/src/components/Settings/PageRecoveryPhoneRemove/index.stories.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
import React from 'react';
66
import { Meta } from '@storybook/react';
77
import { withLocalization } from 'fxa-react/lib/storybooks';
8-
import PageRecoveryPhoneRemove from '.';
8+
import { PageRecoveryPhoneRemove } from '.';
99
import { LocationProvider } from '@reach/router';
1010
import { mockAppContext, MOCK_ACCOUNT } from '../../../models/mocks';
1111
import { Account, AppContext } from '../../../models';

packages/fxa-settings/src/components/Settings/PageRecoveryPhoneRemove/index.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

55
import React from 'react';
6-
import PageRecoveryPhoneRemove from '.';
6+
import { PageRecoveryPhoneRemove } from '.';
77
import { act, screen } from '@testing-library/react';
88
import userEvent from '@testing-library/user-event';
99
import { Account, AppContext } from '../../../models';

packages/fxa-settings/src/components/Settings/PageRecoveryPhoneRemove/index.tsx

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import FlowContainer from '../FlowContainer';
1515
import { getLocalizedErrorMessage } from '../../../lib/error-utils';
1616
import GleanMetrics from '../../../lib/glean';
1717
import { useNavigateWithQuery } from '../../../lib/hooks/useNavigateWithQuery';
18+
import { MfaGuard } from '../MfaGuard';
1819

1920
// TODO, update this link with #section-heading once the SUMO article is updated (FXA-10918)
2021
const sumoTwoStepLink = (
@@ -26,7 +27,7 @@ const sumoTwoStepLink = (
2627
</LinkExternal>
2728
);
2829

29-
const PageRecoveryPhoneRemove = (props: RouteComponentProps) => {
30+
export const PageRecoveryPhoneRemove = (props: RouteComponentProps) => {
3031
const navigateWithQuery = useNavigateWithQuery();
3132
const account = useAccount();
3233
const alertBar = useAlertBar();
@@ -140,4 +141,10 @@ const PageRecoveryPhoneRemove = (props: RouteComponentProps) => {
140141
);
141142
};
142143

143-
export default PageRecoveryPhoneRemove;
144+
export const PageMfaGuardRecoveryPhoneRemove = (props: RouteComponentProps) => {
145+
return (
146+
<MfaGuard requiredScope="2fa">
147+
<PageRecoveryPhoneRemove {...props} />
148+
</MfaGuard>
149+
);
150+
};

packages/fxa-settings/src/components/Settings/index.test.tsx

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,9 +197,9 @@ describe('Settings App', () => {
197197
});
198198
it('routes to PageChangePassword', async () => {
199199
// Suppress MFA guard for this test
200-
mockMfaGuard.mockImplementationOnce(({ children }: { children: ReactNode }) => (
201-
<>{children}</>
202-
));
200+
mockMfaGuard.mockImplementationOnce(
201+
({ children }: { children: ReactNode }) => <>{children}</>
202+
);
203203

204204
// Mock the JWT token cache to suppress MFA guard
205205
const mockJwtTokenCache = {
@@ -373,6 +373,11 @@ describe('Settings App', () => {
373373
route: '/change_password',
374374
hasPassword: true,
375375
},
376+
{
377+
pageName: 'PageRecoveryPhoneRemove',
378+
route: '/recovery_phone/remove',
379+
hasPassword: true,
380+
},
376381
];
377382

378383
it.each(guardedRoutes)(

packages/fxa-settings/src/components/Settings/index.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ import { currentAccount } from '../../lib/cache';
3333
import { hasAccount, setCurrentAccount } from '../../lib/storage-utils';
3434
import GleanMetrics from '../../lib/glean';
3535
import Head from 'fxa-react/components/Head';
36-
import PageRecoveryPhoneRemove from './PageRecoveryPhoneRemove';
36+
import { PageMfaGuardRecoveryPhoneRemove } from './PageRecoveryPhoneRemove';
3737
import { SettingsIntegration } from './interfaces';
3838
import { useNavigateWithQuery } from '../../lib/hooks/useNavigateWithQuery';
3939
import MfaGuardPage2faChange from './Page2faChange';
@@ -197,7 +197,7 @@ export const Settings = ({
197197
<Redirect from="/avatar/change" to="/settings/avatar/" noThrow />
198198

199199
<MfaGuardPageRecoveryPhoneSetup path="/recovery_phone/setup" />
200-
<PageRecoveryPhoneRemove path="/recovery_phone/remove" />
200+
<PageMfaGuardRecoveryPhoneRemove path="/recovery_phone/remove" />
201201

202202
<PageMfaGuardTestWithAuthClient path="/mfa_guard/test/auth_client" />
203203
<PageMfaGuardTestWithGql path="/mfa_guard/test/gql" />

packages/fxa-settings/src/models/Account.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1566,8 +1566,9 @@ export class Account implements AccountData {
15661566
}
15671567

15681568
async removeRecoveryPhone() {
1569+
const jwt = this.getCachedJwtByScope('2fa');
15691570
const result = await this.withLoadingStatus(
1570-
this.authClient.recoveryPhoneDelete(sessionToken()!)
1571+
this.authClient.recoveryPhoneDeleteWithJwt(jwt)
15711572
);
15721573
return result;
15731574
}

0 commit comments

Comments
 (0)