Merge pull request #18340 from mozilla/FXA-11044

feat(recovery-phone): Display phone number in desired format (masking + Twilio's nationalFormat)

Author: LZoog

libs/accounts/recovery-phone/README.md

@@ -8,8 +8,8 @@ Run `nx build recovery-phone` to build the library.
 
 ## Running unit tests
 
-Run `nx test-unit recovery-phone` to execute the unit tests via [Jest](https://jestjs.io).
+Run `nx test-unit accounts-recovery-phone` to execute the unit tests via [Jest](https://jestjs.io).
 
 ## Running integration tests
 
-Run `nx test-integration recovery-phone` to execute the integration tests via [Jest](https://jestjs.io).
+Run `nx test-integration accounts-recovery-phone` to execute the integration tests via [Jest](https://jestjs.io).

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

@@ -82,9 +82,11 @@ export class RecoveryPhoneManager {
    *
    * @param uid
    */
-  async getConfirmedPhoneNumber(
-    uid: string
-  ): Promise<{ uid: Buffer; phoneNumber: string }> {
+  async getConfirmedPhoneNumber(uid: string): Promise<{
+    uid: Buffer;
+    phoneNumber: string;
+    nationalFormat?: string;
+  }> {
     const uidBuffer = Buffer.from(uid, 'hex');
     const result = await getConfirmedPhoneNumber(this.db, uidBuffer);
     if (!result) {

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

@@ -7,12 +7,27 @@ import { RecoveryPhone } from './recovery-phone.types';
 export async function getConfirmedPhoneNumber(
   db: AccountDatabase,
   uid: Buffer
-): Promise<{ uid: Buffer; phoneNumber: string } | undefined> {
-  return db
+): Promise<
+  { uid: Buffer; phoneNumber: string; nationalFormat?: string } | undefined
+> {
+  const row = await db
     .selectFrom('recoveryPhones')
     .where('uid', '=', uid)
-    .select(['uid', 'phoneNumber'])
+    .select(['uid', 'phoneNumber', 'lookupData'])
     .executeTakeFirst();
+
+  if (!row) {
+    return undefined;
+  }
+  const { uid: userId, phoneNumber, lookupData } = row;
+  const nationalFormat = (lookupData as { nationalFormat?: string })
+    ?.nationalFormat;
+
+  return {
+    uid: userId,
+    phoneNumber,
+    nationalFormat,
+  };
 }
 
 export async function registerPhoneNumber(

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

@@ -208,14 +208,14 @@ describe('RecoveryPhoneService', () => {
       ).toHaveBeenCalledWith(uid);
     });
 
-    it('can return masked phone number', async () => {
+    it('can return stripped phone number', async () => {
       mockRecoveryPhoneManager.getConfirmedPhoneNumber.mockReturnValueOnce({
         phoneNumber,
       });
 
       const result = await service.hasConfirmed(uid, 4);
 
-      expect(result.phoneNumber).toEqual('+•••••••1234');
+      expect(result.phoneNumber).toEqual('1234');
       expect(
         mockRecoveryPhoneManager.getConfirmedPhoneNumber
       ).toHaveBeenCalledWith(uid);
@@ -547,7 +547,7 @@ describe('RecoveryPhoneService', () => {
       expect(service.hasConfirmed(uid)).rejects.toEqual(
         new RecoveryPhoneNotEnabled()
       );
-      expect(() => service.maskPhoneNumber('+15550005555')).toThrow(
+      expect(() => service.stripPhoneNumber('+15550005555')).toThrow(
         new RecoveryPhoneNotEnabled()
       );
       expect(service.removePhoneNumber(uid)).rejects.toEqual(
@@ -562,14 +562,41 @@ describe('RecoveryPhoneService', () => {
     });
   });
 
-  describe('mask phone number', () => {
-    it('can mask number', () => {
+  describe('strip phone number', () => {
+    it('can strip number', () => {
       const phoneNumber = '+123456789';
-      expect(service.maskPhoneNumber(phoneNumber, -1)).toEqual('+•••••••••');
-      expect(service.maskPhoneNumber(phoneNumber, 0)).toEqual('+•••••••••');
-      expect(service.maskPhoneNumber(phoneNumber, 4)).toEqual('+•••••6789');
-      expect(service.maskPhoneNumber(phoneNumber, 9)).toEqual('+123456789');
-      expect(service.maskPhoneNumber(phoneNumber, 12)).toEqual('+123456789');
+
+      expect(service.stripPhoneNumber(phoneNumber)).toEqual(phoneNumber);
+      expect(service.stripPhoneNumber(phoneNumber, -1)).toEqual('');
+      expect(service.stripPhoneNumber(phoneNumber, 0)).toEqual('');
+      expect(service.stripPhoneNumber(phoneNumber, 2)).toEqual('89');
+      expect(service.stripPhoneNumber(phoneNumber, 4)).toEqual('6789');
+      expect(service.stripPhoneNumber(phoneNumber, 9)).toEqual('123456789');
+      expect(service.stripPhoneNumber(phoneNumber, 12)).toEqual('123456789');
+    });
+    it('can strip NANP national_format number and should not display format', () => {
+      const phoneNumber = '(123) 456-7890';
+
+      expect(service.stripPhoneNumber(phoneNumber)).toEqual(phoneNumber);
+      expect(service.stripPhoneNumber(phoneNumber, -1)).toEqual('');
+      expect(service.stripPhoneNumber(phoneNumber, 0)).toEqual('');
+      expect(service.stripPhoneNumber(phoneNumber, 4)).toEqual('7890');
+      expect(service.stripPhoneNumber(phoneNumber, 10)).toEqual('1234567890');
+      expect(service.stripPhoneNumber(phoneNumber, 12)).toEqual('1234567890');
+    });
+    it('can strip non-NANP national_format number and should not display format', () => {
+      const phoneNumber = '+33 9 87 65 43 21';
+
+      expect(service.stripPhoneNumber(phoneNumber)).toEqual(phoneNumber);
+      expect(service.stripPhoneNumber(phoneNumber, -1)).toEqual('');
+      expect(service.stripPhoneNumber(phoneNumber, 0)).toEqual('');
+      expect(service.stripPhoneNumber(phoneNumber, 4)).toEqual('4321');
+      expect(service.stripPhoneNumber(phoneNumber, 9)).toEqual('987654321');
+      expect(service.stripPhoneNumber(phoneNumber, 12)).toEqual('33987654321');
+    });
+    it('can handle being passed an empty string', () => {
+      const phoneNumber = '';
+      expect(service.stripPhoneNumber(phoneNumber, 4)).toEqual('');
     });
   });
 });

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

@@ -117,6 +117,18 @@ export class RecoveryPhoneService {
     return this.isSuccessfulSmsSend(msg);
   }
 
+  public async getNationalFormat(phoneNumber: string) {
+    // When the user _confirms_ their OTP code we also call the lookup endpoint to
+    // store the full data returned in our DB, but we need the national format on the
+    // OTP confirm page before then. "Basic lookups" from Twilio are free, so don't
+    // bother persisting in redis.
+    // https://www.twilio.com/en-us/user-authentication-identity/pricing/lookup
+    const { nationalFormat } = await this.smsManager.phoneNumberLookup(
+      phoneNumber
+    );
+    return nationalFormat;
+  }
+
   /**
    * Confirms a UID code. This will also and finalizes the phone number setup if the code provided was
    * intended for phone number setup.
@@ -261,29 +273,34 @@ export class RecoveryPhoneService {
    */
   public async hasConfirmed(
     uid: string,
-    phoneNumberMask?: number
+    phoneNumberStrip?: number
   ): Promise<{
     exists: boolean;
     phoneNumber?: string;
+    nationalFormat?: string;
   }> {
     if (!this.config.enabled) {
       throw new RecoveryPhoneNotEnabled();
     }
 
     try {
-      const { phoneNumber } =
+      const { phoneNumber, nationalFormat } =
         await this.recoveryPhoneManager.getConfirmedPhoneNumber(uid);
 
       return {
         exists: true,
-        phoneNumber: this.maskPhoneNumber(phoneNumber, phoneNumberMask),
+        phoneNumber: this.stripPhoneNumber(phoneNumber, phoneNumberStrip),
+        nationalFormat: nationalFormat
+          ? this.stripPhoneNumber(nationalFormat, phoneNumberStrip)
+          : undefined,
       };
     } catch (err) {
       if (err instanceof RecoveryNumberNotExistsError) {
         // no-op - we handle the error, and just return false;
         return {
           exists: false,
           phoneNumber: undefined,
+          nationalFormat: undefined,
         };
       }
       // Something unexpected happened...
@@ -296,38 +313,26 @@ export class RecoveryPhoneService {
    *
    * @param phoneNumber The actual phone number
    * @param lastN The last N number of digits to show
-   * @returns A masked number
-   *
-   * @remarks This will not mask a + symbol, since this technically isn't part of the
-   * number. e.g. +15005551234 would be masked as +*******1234 if lastN was 4.
+   * @returns The last N number of digits of the phone number
    */
-  public maskPhoneNumber(phoneNumber: string, lastN?: number) {
+  public stripPhoneNumber(phoneNumber: string, lastN?: number) {
     if (!this.config.enabled) {
       throw new RecoveryPhoneNotEnabled();
     }
-
-    // The + notation can be confusing in a masked number. Don't count it
-    // as a digit.
-    let prefix = '';
-    if (phoneNumber.startsWith('+')) {
-      prefix = '+';
-      phoneNumber = phoneNumber.substring(1);
-    }
-
-    // Clamp lastN between 0 and phoneNumber.length
+    // No stripping needed, session is verified
     if (lastN === undefined) {
-      lastN = phoneNumber.length;
-    } else if (lastN > phoneNumber.length) {
-      lastN = phoneNumber.length;
-    } else if (lastN < 0) {
-      lastN = 0;
+      return phoneNumber;
+    }
+    if (lastN <= 0) {
+      return '';
     }
 
-    // Create mask
-    const maskedPhoneNumber = phoneNumber
-      .substring(phoneNumber.length - lastN)
-      .padStart(phoneNumber.length, '•');
-    return `${prefix}${maskedPhoneNumber}`;
+    const digits = phoneNumber.replace(/\D/g, '');
+    // Clamp lastN between 0 and digits.length
+    if (lastN > digits.length) {
+      lastN = digits.length;
+    }
+    return digits.slice(-lastN);
   }
 
   /**

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

@@ -72,6 +72,7 @@ export class SmsManager {
     const from = this.rotateFromNumber();
 
     try {
+      // Validate the `to` phone number and send the SMS
       const msg = await this.client.messages.create({
         to,
         from,

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

@@ -2171,7 +2171,7 @@ export default class AuthClient {
     sessionToken: string,
     phoneNumber: string,
     headers?: Headers
-  ) {
+  ): Promise<{ nationalFormat?: string; success: boolean }> {
     return this.sessionPost(
       '/recovery_phone/create',
       sessionToken,
@@ -2205,7 +2205,7 @@ export default class AuthClient {
     sessionToken: string,
     code: string,
     headers?: Headers
-  ) {
+  ): Promise<{ nationalFormat?: string }> {
     return this.sessionPost(
       '/recovery_phone/confirm',
       sessionToken,

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

@@ -175,7 +175,18 @@ class RecoveryPhoneHandler {
           uid,
         });
         await this.glean.twoStepAuthPhoneCode.sent(request);
-        return { status: RecoveryPhoneStatus.SUCCESS };
+
+        let nationalFormat: string | null = null;
+        try {
+          nationalFormat = await this.recoveryPhoneService.getNationalFormat(
+            phoneNumber
+          );
+        } catch (e) {
+          // This should not fail since the number was already validated with Twilio so
+          // if it does it's a network problem - just return a null value and don't error out.
+        }
+
+        return { status: RecoveryPhoneStatus.SUCCESS, nationalFormat };
       }
       await this.glean.twoStepAuthPhoneCode.sendError(request);
       return { status: RecoveryPhoneStatus.FAILURE };
@@ -277,17 +288,17 @@ class RecoveryPhoneHandler {
         this.log.info('account.recoveryPhone.phoneAdded.success', { uid });
 
         try {
-          const { phoneNumber } = await this.recoveryPhoneService.hasConfirmed(
-            uid,
-            4
-          );
-
+          const { phoneNumber, nationalFormat } =
+            await this.recoveryPhoneService.hasConfirmed(uid, 4);
           await this.mailer.sendPostAddRecoveryPhoneEmail(
             account.emails,
             account,
             {
               acceptLanguage,
-              maskedLastFourPhoneNumber: phoneNumber?.slice(1),
+              maskedLastFourPhoneNumber: `••••••${this.recoveryPhoneService.stripPhoneNumber(
+                phoneNumber || '',
+                4
+              )}`,
               timeZone: geo.timeZone,
               uaBrowser: ua.browser,
               uaBrowserVersion: ua.browserVersion,
@@ -297,6 +308,11 @@ class RecoveryPhoneHandler {
               uid,
             }
           );
+          return {
+            phoneNumber,
+            nationalFormat,
+            status: RecoveryPhoneStatus.SUCCESS,
+          };
         } catch (error) {
           // log email send error but don't throw
           // user should be allowed to proceed
@@ -448,21 +464,18 @@ class RecoveryPhoneHandler {
   async exists(request: AuthRequest) {
     const { uid, emailVerified, mustVerify, tokenVerified } = request.auth
       .credentials as SessionTokenAuthCredential;
-    const payload = request.payload as unknown as { phoneNumberMask: number };
-    let phoneNumberMask = payload?.phoneNumberMask;
 
     // To ensure no data is leaked, we will never expose the full phone number, if
     // the session is not verified. e.g. The user has entered the correct password,
     // but failed to provide 2FA.
-    if (
-      phoneNumberMask === undefined &&
-      (!emailVerified || (mustVerify && !tokenVerified))
-    ) {
-      phoneNumberMask = 4;
-    }
+    const phoneNumberStrip =
+      !emailVerified || (mustVerify && !tokenVerified) ? 4 : undefined;
 
     try {
-      return await this.recoveryPhoneService.hasConfirmed(uid, phoneNumberMask);
+      return await this.recoveryPhoneService.hasConfirmed(
+        uid,
+        phoneNumberStrip
+      );
     } catch (error) {
       if (error instanceof RecoveryPhoneNotEnabled) {
         throw AppError.featureNotEnabled();

packages/fxa-auth-server/lib/senders/emails/templates/postAddRecoveryPhone/en.ftl

@@ -3,7 +3,7 @@ postAddRecoveryPhone-preview = Account protected by two-step authentication
 postAddRecoveryPhone-title = You created a recovery phone number
 # Variables:
 #  $maskedLastFourPhoneNumber (String) - A bullet point mask with the last four digits of the user's phone number, e.g. ••••••1234
-postAddRecoveryPhone-description = You added { $maskedLastFourPhoneNumber } as your recovery phone
+postAddRecoveryPhone-description-v2 = You added { $maskedLastFourPhoneNumber } as your recovery phone number
 # Links out to a support article about two factor authentication
 postAddRecoveryPhone-how-protect = How this protects your account
 postAddRecoveryPhone-how-protect-plaintext = How this protects your account:

packages/fxa-auth-server/lib/senders/emails/templates/postAddRecoveryPhone/index.mjml

@@ -9,7 +9,7 @@
     </mj-text>
 
     <mj-text css-class="text-body-no-margin">
-      <span dir="ltr" data-l10n-id="postAddRecoveryPhone-description" data-l10n-args="<%= JSON.stringify({ maskedLastFourPhoneNumber }) %>">You added <%- maskedLastFourPhoneNumber %> as your recovery phone</span>
+      <span dir="ltr" data-l10n-id="postAddRecoveryPhone-description-v2" data-l10n-args="<%= JSON.stringify({ maskedLastFourPhoneNumber }) %>">You added <%- maskedLastFourPhoneNumber %> as your recovery phone number</span>
     </mj-text>
     <mj-text css-class="text-body">
       <a class="link-blue" href="<%- twoFactorSupportLink %>">