feat(sms): Set up flow to add recovery phone from Settings by LZoog · Pull Request #18276 · mozilla/fxa

LZoog · 2025-01-23T19:36:14Z

Because:

We want users to be able to add a recovery phone

This commit:

Enables the "add" page if the feature flag is on and availability from GQL is true
Returns 'availability' with other recovery phone info in the GQL resolver
Displays recovery phone info in recovery phone row,
Hides 'change' and 'delete' buttons if user doesn't have a recovery phone set up, displays SIM swap link only if user doesn't have a recovery phone

You will need these env vars set:

RECOVERY_PHONE__TWILIO__ACCOUNT_SID=updateMe
RECOVERY_PHONE__TWILIO__AUTH_TOKEN=updateMe
GEODB_LOCATION_OVERRIDE={ "location": { "countryCode": "US", "postalCode": "85001"} }

The magic number 4159929960 works for this flow.

LZoog · 2025-01-23T20:44:32Z

    }

    const code = await this.otpCode.generateCode();
+    await this.recoveryPhoneManager.storeUnconfirmed(


Not sure why but I wasn't seeing this show up in Redis, it looked like there was an entry but nothing to expand/look at. When I'm done with this PR I'll circle back and see if it works removing the change in this file.

vpomerleau

Left some comments from first pass, will have another look once rebased on main!

vpomerleau · 2025-01-27T21:15:37Z

    nullable: true,
-    description: 'The registered recovery phone number',
+    description:
+      'The registered recovery phone number. If the user does not have a verified session, this field will return the last 4 digits of the phone number with a mask on the rest.',


Does it return with mask, or just return the last 4 digits?

Currently it returns with a mask, but I think it makes sense for it to return with only the last 4 digits. I'll file one issue for the Twilio national_format bit + this since they're related.

vpomerleau · 2025-01-27T21:50:36Z

-    />
-  </AppContext.Provider>
-);
+// Recovery phone not shown since `available` is based on region and recovery codes


Should these stories be re-enabled or deleted?

They're all there I believe, they were just refactored to use createSubject (and I added a couple new ones).

vpomerleau · 2025-01-27T21:52:26Z

+    ).toContain(MOCK_MASKED_PHONE_NUMBER);
+    expect(
+      screen.queryByTestId('backup-authentication-codes-sub-row')
+    ).toBeInTheDocument();


Check that recovery phone delete button is unavailable, or tested in sub-component?

vpomerleau · 2025-01-27T21:53:03Z

+    ).toContain(MOCK_MASKED_PHONE_NUMBER);
+    expect(
+      screen.getByTestId('backup-authentication-codes-sub-row').textContent
+    ).toContain('3');


test that delete available for phone number?

vpomerleau · 2025-01-27T21:56:32Z

+        config.featureFlags?.enableAdding2FABackupPhone === true &&
+        (recoveryPhone.available === true || recoveryPhone.exists === true)


Suggested change

config.featureFlags?.enableAdding2FABackupPhone === true &&

(recoveryPhone.available === true || recoveryPhone.exists === true)

(config.featureFlags?.enableAdding2FABackupPhone === true &&

recoveryPhone.available === true) || recoveryPhone.exists === true

I think the row should always be shown if a user has added a recovery phone, regardless of whether adding or using a phone is enabled. They should have the option of removing the phone number from their account even if we've had to disable actually using the feature.

Which makes me think that the "remove phone" route option should depend on having a phone configured and not on the feature flags... hm...

Oh yeah, good catch. When I first wrote this logic, I don't think I realized this would hide the remove option for users if the feature flag was off.

Which makes me think that the "remove phone" route option should depend on having a phone configured and not on the feature flags... hm...

I guess both? The "manage" flag should be on and the user should have a phone number.

vpomerleau

Thanks for the updates! This is looking great 🚀 Approved pending CI passes and your final run of manual testing 🤘

vpomerleau · 2025-01-28T05:27:08Z

+    // Just retry adding the number and another code will send. Note that more than
+    // one code can be valid at the same time if the user clicks “resend code” to
+    // account for SMS transmission delay.
+    // try/catch is in the component that calls this function


Noting that @vbudhram recommends invalidating the previous code when a new one is sent one. There's a thread up for discussion in the Slack channel for this project

Was the try-catch comment meant to stay in, or a self-review note?

Sure - sounds like we have a separate issue filed for that, I can link it here since I need another push anyway. 👍

I left that in because at first I wrapped this in the try/catch since usually our account.whatever methods have a try/catch, and then realized we didn't need it. Can remove if you want though.

vpomerleau · 2025-01-28T05:28:52Z

+              navigate(`${SETTINGS_PATH}/recovery_phone/setup`);
+            }}
+            onDeleteClick={() => {
+              navigate(`${SETTINGS_PATH}/recovery_phone/remove`);


vpomerleau · 2025-01-28T05:30:50Z

@@ -1,3 +1,4 @@
+import { existsSync } from 'fs';


unused import causing build failure on CI

Ahhh dumb auto import, thanks.

vbudhram · 2025-01-28T17:28:45Z

Looks like removeRecoveryPhone in Account.ts got reverted. It should be this.

  async removeRecoveryPhone() {
    const result = await this.withLoadingStatus(
      this.authClient.recoveryPhoneDelete(sessionToken()!)
    );

    const cache = this.apolloClient.cache;
    cache.modify({
      id: cache.identify({ __typename: 'Account' }),
      fields: {
        recoveryPhone() {
          return {
            exists: false,
            phoneNumber: null,
            available: true,
          };
        },
      },
    });

    return result;
  }

vbudhram

@LZoog Did some manual testing and just noticed the one thing. Thanks!

LZoog · 2025-01-28T18:33:15Z

@vbudhram ahh thank you, there was a big ol' merge conflict there and I resolved with my changes and totally forgot to add yours back in!

Because: * We want users to be able to add a recovery phone This commit: * Enables the "add" page if the feature flag is on and availability from GQL is true * Returns 'availability' with other recovery phone info in the GQL resolver * Displays recovery phone info in recovery phone row, * Hides 'change' and 'delete' buttons if user doesn't have a recovery phone set up, displays SIM swap link only if user doesn't have a recovery phone closes FXA-10370

LZoog commented Jan 23, 2025

View reviewed changes

LZoog commented Jan 24, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/components/App/gql.ts Outdated

LZoog force-pushed the FXA-10370 branch from a474395 to d2b7f11 Compare January 24, 2025 23:51

vpomerleau mentioned this pull request Jan 27, 2025

feat(fxa-settings): Implement using recovery phone to sign in #18255

Merged

4 tasks

LZoog force-pushed the FXA-10370 branch from d2b7f11 to 0328d1b Compare January 27, 2025 20:10

vpomerleau reviewed Jan 27, 2025

View reviewed changes

vpomerleau reviewed Jan 28, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/components/Settings/UnitRowTwoStepAuth/index.tsx Outdated

LZoog force-pushed the FXA-10370 branch 3 times, most recently from 868adab to 5e7431d Compare January 28, 2025 01:54

LZoog marked this pull request as ready for review January 28, 2025 01:54

LZoog requested review from a team as code owners January 28, 2025 01:54

vpomerleau approved these changes Jan 28, 2025

View reviewed changes

vbudhram approved these changes Jan 28, 2025

View reviewed changes

LZoog force-pushed the FXA-10370 branch from 5e7431d to 71816af Compare January 28, 2025 19:00

LZoog force-pushed the FXA-10370 branch from 71816af to 6dd1055 Compare January 28, 2025 19:04

bcolsson approved these changes Jan 28, 2025

View reviewed changes

vpomerleau merged commit de6b0bf into main Jan 28, 2025

vpomerleau deleted the FXA-10370 branch January 28, 2025 19:44

		config.featureFlags?.enableAdding2FABackupPhone === true &&
		(recoveryPhone.available === true \|\| recoveryPhone.exists === true)

Conversation

LZoog commented Jan 23, 2025 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vpomerleau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LZoog Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LZoog Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vpomerleau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LZoog Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbudhram commented Jan 28, 2025

Uh oh!

vbudhram left a comment

Choose a reason for hiding this comment

Uh oh!

LZoog commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LZoog commented Jan 23, 2025 •

edited by atlassian Bot

Loading

LZoog Jan 28, 2025 •

edited

Loading

LZoog Jan 28, 2025 •

edited

Loading

LZoog Jan 28, 2025 •

edited

Loading