Merge pull request #18913 from mozilla/FXA-10372

feat(settings): Enable changing recovery phone

Author: vpomerleau

libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts

@@ -186,10 +186,10 @@ describe('RecoveryPhoneManager', () => {
     expect(deleteFromSpy).toBeCalledWith('recoveryPhones');
   });
 
-  it('should replace a recovery phone', async () => {
+  it('should change a recovery phone', async () => {
     const mockPhone = RecoveryPhoneFactory();
 
-    const res = await recoveryPhoneManager.replacePhoneNumber(
+    const res = await recoveryPhoneManager.changePhoneNumber(
       mockPhone.uid.toString('hex'),
       mockPhone.phoneNumber,
       mockLookUpData

libs/accounts/recovery-phone/src/lib/recovery-phone.manager.ts

@@ -13,7 +13,7 @@ import {
   hasRecoveryCodes,
   registerPhoneNumber,
   removePhoneNumber,
-  replacePhoneNumber,
+  changePhoneNumber,
 } from './recovery-phone.repository';
 import {
   RecoveryNumberAlreadyExistsError,
@@ -124,14 +124,14 @@ export class RecoveryPhoneManager {
    * @param phoneNumber The new phone number to replace the existing one
    * @param lookupData Lookup data for twilio cross-check
    */
-  async replacePhoneNumber(
+  async changePhoneNumber(
     uid: string,
     phoneNumber: string,
     lookupData: PhoneNumberLookupData
   ): Promise<boolean> {
     const uidBuffer = Buffer.from(uid, 'hex');
     const now = Date.now();
-    const results = await replacePhoneNumber(this.db, {
+    const results = await changePhoneNumber(this.db, {
       uid: uidBuffer,
       phoneNumber,
       lastConfirmed: now,

libs/accounts/recovery-phone/src/lib/recovery-phone.repository.ts

@@ -42,7 +42,7 @@ export async function registerPhoneNumber(
  *
  * @returns The number of rows updated
  */
-export async function replacePhoneNumber(
+export async function changePhoneNumber(
   db: AccountDatabase,
   recoveryPhone: RecoveryPhone
 ): Promise<number> {

libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts

@@ -60,7 +60,7 @@ describe('RecoveryPhoneService', () => {
     hasRecoveryCodes: jest.fn(),
     removeCode: jest.fn(),
     getCountByPhoneNumber: jest.fn(),
-    replacePhoneNumber: jest.fn(),
+    changePhoneNumber: jest.fn(),
   };
 
   const mockOtpManager = {
@@ -922,32 +922,32 @@ describe('RecoveryPhoneService', () => {
 
   describe('replace phone number', () => {
     it('replaces code successfully', async () => {
-      mockRecoveryPhoneManager.replacePhoneNumber.mockResolvedValue(true);
+      mockRecoveryPhoneManager.changePhoneNumber.mockResolvedValue(true);
       mockRecoveryPhoneManager.getUnconfirmed.mockResolvedValueOnce({
         isSetup: true,
         phoneNumber,
       });
-      const result = await service.replacePhoneNumber(uid, code);
+      const result = await service.changePhoneNumber(uid, code);
 
       expect(result).toBe(true);
     });
 
     it('returns false if there is no existing unconfirmed code', async () => {
-      mockRecoveryPhoneManager.replacePhoneNumber.mockResolvedValue(true);
+      mockRecoveryPhoneManager.changePhoneNumber.mockResolvedValue(true);
       mockRecoveryPhoneManager.getUnconfirmed.mockResolvedValueOnce({});
 
-      const result = await service.replacePhoneNumber(uid, code);
+      const result = await service.changePhoneNumber(uid, code);
 
       expect(result).toBe(false);
     });
 
     it('returns false if existing unconfirmed code is not for setup', async () => {
-      mockRecoveryPhoneManager.replacePhoneNumber.mockResolvedValue(true);
+      mockRecoveryPhoneManager.changePhoneNumber.mockResolvedValue(true);
       mockRecoveryPhoneManager.getUnconfirmed.mockResolvedValueOnce({
         isSetup: false,
         phoneNumber,
       });
-      const result = await service.replacePhoneNumber(uid, code);
+      const result = await service.changePhoneNumber(uid, code);
 
       expect(result).toBe(false);
     });

libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts

@@ -345,7 +345,7 @@ export class RecoveryPhoneService {
    * @param code
    * @returns
    */
-  public async replacePhoneNumber(uid: string, code: string): Promise<boolean> {
+  public async changePhoneNumber(uid: string, code: string): Promise<boolean> {
     const unconfirmedCode = await this.recoveryPhoneManager.getUnconfirmed(
       uid,
       code
@@ -363,7 +363,7 @@ export class RecoveryPhoneService {
 
     const lookupData = await this.getPhoneNumberLookupData(phoneNumber);
     await this.validateLookupDataForSmsPumping(lookupData);
-    await this.recoveryPhoneManager.replacePhoneNumber(
+    await this.recoveryPhoneManager.changePhoneNumber(
       uid,
       phoneNumber,
       lookupData
@@ -562,7 +562,7 @@ export class RecoveryPhoneService {
    * Validates that the provided code is for setup and not for sign in.
    *
    * No further actions are taken; the code is left in redis for completing the
-   * setup process. This is used in `recovery_phone/replace` and we need
+   * setup process. This is used in `recovery_phone/change` and we need
    * to ensure the code is valid before removing the existing phone number.
    */
   public async validateSetupCode(uid: string, code: string) {

packages/functional-tests/pages/settings/components/unitRow.ts

@@ -6,16 +6,17 @@ import { Page } from '@playwright/test';
 import { ConnectedService } from './connectedService';
 
 export class UnitRow {
-  constructor(readonly page: Page, readonly id: string) {}
+  constructor(
+    readonly page: Page,
+    readonly id: string
+  ) {}
 
   get status() {
     return this.page.getByTestId(`${this.id}-unit-row-header-value`);
   }
 
   async screenshot() {
-    const el = await this.page.waitForSelector(
-      `[data-testid=${this.id}-unit-row]`
-    );
+    const el = this.page.getByTestId(`${this.id}-unit-row`);
     return el.screenshot();
   }
 }
@@ -93,12 +94,20 @@ export class TotpRow extends UnitRow {
     return this.page.getByTestId('two-step-disable-button-unit-row-modal');
   }
 
-  get changeButton() {
-    return this.page.getByRole('button', { name: 'Create new codes' });
+  get getNewBackupCodesButton() {
+    return this.page.getByTestId('backup-codes-get-new-button');
+  }
+
+  get addCodesButton() {
+    return this.page.getByTestId('backup-codes-add-button');
   }
 
   get addRecoveryPhoneButton() {
-    return this.page.getByRole('button', { name: 'Add' });
+    return this.page.getByTestId('recovery-phone-add-button');
+  }
+
+  get changeRecoveryPhoneButton() {
+    return this.page.getByTestId('recovery-phone-change-button');
   }
 
   get removeRecoveryPhoneButton() {

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

@@ -91,7 +91,9 @@ test.describe('severity-1 #smoke', () => {
 
       await recoveryPhone.enterPhoneNumber('1234567890');
       await recoveryPhone.clickSendCode();
-      await expect(recoveryPhone.addErrorBanner).toHaveText(/invalid phone number/i);
+      await expect(recoveryPhone.addErrorBanner).toHaveText(
+        /invalid phone number/i
+      );
 
       await settings.goto();
       await settings.disconnectTotp();
@@ -159,6 +161,55 @@ test.describe('severity-1 #smoke', () => {
       await settings.disconnectTotp();
     });
 
+    test('can change recovery phone', async ({
+      target,
+      pages: { page, settings, signin, recoveryPhone, totp },
+      testAccountTracker,
+    }) => {
+      const credentials = await signInAccount(
+        target,
+        page,
+        settings,
+        signin,
+        testAccountTracker
+      );
+
+      await settings.goto();
+      await setupRecoveryPhone({
+        settings,
+        totp,
+        recoveryPhone,
+        page,
+        credentials,
+        target,
+      });
+
+      await settings.totp.changeRecoveryPhoneButton.click();
+      await page.waitForURL(/recovery_phone\/setup/);
+
+      await expect(page.getByText('Change recovery phone')).toBeVisible();
+
+      await expect(recoveryPhone.addHeader()).toBeVisible();
+
+      await recoveryPhone.enterPhoneNumber(getPhoneNumber(target.name));
+      await recoveryPhone.clickSendCode();
+
+      await expect(recoveryPhone.confirmHeader).toBeVisible();
+
+      const code = await target.smsClient.getCode(
+        getPhoneNumber(target.name),
+        credentials.uid
+      );
+
+      await recoveryPhone.enterCode(code);
+      await recoveryPhone.clickConfirm();
+
+      await page.waitForURL(/settings/);
+      await expect(settings.alertBar).toHaveText('Recovery phone changed');
+
+      await settings.disconnectTotp();
+    });
+
     test('can sign-in to settings with recovery phone', async ({
       target,
       pages: {

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

@@ -87,7 +87,7 @@ test.describe('severity-1 #smoke', () => {
       await settings.disconnectTotp();
     });
 
-    test('can change backup authentication codes', async ({
+    test('can get new backup authentication codes', async ({
       target,
       pages: {
         page,
@@ -109,7 +109,7 @@ test.describe('severity-1 #smoke', () => {
 
       await settings.goto();
       const { recoveryCodes } = await addTotp(settings, totp);
-      await settings.totp.changeButton.click();
+      await settings.totp.getNewBackupCodesButton.click();
 
       const newCodes = await totp.getRecoveryCodes();
       expect(newCodes.some((c) => recoveryCodes.includes(c))).toBe(false);

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

@@ -2211,13 +2211,13 @@ export default class AuthClient {
    * @param code The otp code sent to the user's phone
    * @param headers
    */
-    async recoveryPhoneReplace(
+    async recoveryPhoneChange(
       sessionToken: string,
       code: string,
       headers?: Headers
     ): Promise<{ nationalFormat?: string }> {
       return this.sessionPost(
-        '/recovery_phone/replace',
+        '/recovery_phone/change',
         sessionToken,
         {
           code,

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

@@ -526,10 +526,9 @@ class RecoveryPhoneHandler {
     throw AppError.invalidOrExpiredOtpCode();
   }
 
-  async replacePhoneNumber(request: AuthRequest) {
+  async changePhoneNumber(request: AuthRequest) {
     // need to check first that there is an existing phone number
-    const { uid } = request.auth
-      .credentials as SessionTokenAuthCredential;
+    const { uid } = request.auth.credentials as SessionTokenAuthCredential;
 
     const { code } = request.payload as unknown as {
       code: string;
@@ -556,7 +555,7 @@ class RecoveryPhoneHandler {
     // replace the existing phone number with the new one associated with the code.
     let replacedSuccess = false;
     try {
-      replacedSuccess = await this.recoveryPhoneService.replacePhoneNumber(
+      replacedSuccess = await this.recoveryPhoneService.changePhoneNumber(
         uid,
         code
       );
@@ -579,7 +578,7 @@ class RecoveryPhoneHandler {
 
     if (!replacedSuccess) {
       await this.glean.twoStepAuthPhoneReplace.failure(request);
-      this.statsd.increment('account.recoveryPhone.replacePhoneNumber.failure');
+      this.statsd.increment('account.recoveryPhone.changePhoneNumber.failure');
       recordSecurityEvent('account.recovery_phone_replace_failed', {
         db: this.db,
         request,
@@ -590,7 +589,7 @@ class RecoveryPhoneHandler {
     }
 
     await this.glean.twoStepAuthPhoneReplace.success(request);
-    this.statsd.increment('account.recoveryPhone.replacePhoneNumber.success');
+    this.statsd.increment('account.recoveryPhone.changePhoneNumber.success');
 
     const { phoneNumber, nationalFormat } =
       await this.recoveryPhoneService.hasConfirmed(uid);
@@ -1009,7 +1008,7 @@ export const recoveryPhoneRoutes = (
     },
     {
       method: 'POST',
-      path: '/recovery_phone/replace',
+      path: '/recovery_phone/change',
       options: {
         pre: [{ method: featureEnabledCheck }],
         auth: {
@@ -1022,8 +1021,8 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneReplace', request);
-        return recoveryPhoneHandler.replacePhoneNumber(request);
+        log.begin('recoveryPhoneChange', request);
+        return recoveryPhoneHandler.changePhoneNumber(request);
       },
     },
     {