code review comments

mozilla · Feb 4, 2025 · f713205 · f713205
1 parent 14416c8
commit f713205
Show file tree

Hide file tree

Showing 12 changed files with 122 additions and 95 deletions.
diff --git a/libs/accounts/recovery-phone/README.md b/libs/accounts/recovery-phone/README.md
@@ -12,4 +12,4 @@ Run `nx test-unit recovery-phone` to execute the unit tests via [Jest](https://j
 
 ## Running integration tests
 
-Run `nx test-integration recovery-phone` to execute the unit tests via [Jest](https://jestjs.io).
+Run `nx test-integration recovery-phone` to execute the integration tests via [Jest](https://jestjs.io).
diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.manager.in.spec.ts
@@ -6,9 +6,9 @@ import {
   AccountDatabase,
   AccountDbProvider,
   testAccountDatabaseSetup,
+  RecoveryPhoneFactory,
 } from '@fxa/shared/db/mysql/account';
 import { Test } from '@nestjs/testing';
-import { RecoveryPhoneFactory } from '../../../../shared/db/mysql/account/src/lib/factories';
 
 describe('RecoveryPhoneManager', () => {
   let recoveryPhoneManager: RecoveryPhoneManager;
@@ -205,21 +205,22 @@ describe('RecoveryPhoneManager', () => {
 
     expect(mockRedis.set).toHaveBeenCalledWith(
       redisKey,
-      expect.stringMatching(/.*/), // Ensures it's a valid JSON string
+      expect.any(String),
       'EX',
       600
     );
 
     const expectedData = expect.objectContaining({
-      createdAt: expect.any(Number), // Matches any number, we just care that date is included
+      createdAt: expect.any(Number),
       phoneNumber,
       isSetup,
       lookupData: JSON.stringify(mockLookUpData),
     });
 
-    // Parse the actual stored data and compare as an object
-    const storedData = JSON.parse(mockRedis.set.mock.calls[0][1]); // Extract actual data from mock call
-    expect(storedData).toEqual(expectedData);
+    const storedData = mockRedis.set.mock.calls[0][1];
+    expect(() => JSON.parse(storedData)).not.toThrow(); // checks if storedData is a valid JSON
+    const parsedData = JSON.parse(mockRedis.set.mock.calls[0][1]);
+    expect(parsedData).toEqual(expectedData);
   });
 
   it('should return null if no unconfirmed phone number data is found in Redis', async () => {

diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts
@@ -122,24 +122,21 @@ describe('RecoveryPhoneService', () => {
     expect(mockRecoveryPhoneManager.getAllUnconfirmed).toBeCalledWith(uid);
   });
 
-  it('handles localized message body when provided to setup phone number', async () => {
+  it('handles message template when provided to setup phone number', async () => {
     mockOtpManager.generateCode.mockReturnValue(code);
-    const localizedMessageBody = {
-      part1: 'part1',
-      part2: 'part2',
-    };
+    const getFormattedMessage = jest.fn().mockResolvedValue('message');
 
     const result = await service.setupPhoneNumber(
       uid,
       phoneNumber,
-      localizedMessageBody
+      getFormattedMessage
     );
 
     expect(result).toBeTruthy();
     expect(mockOtpManager.generateCode).toBeCalled();
     expect(mockSmsManager.sendSMS).toBeCalledWith({
       to: phoneNumber,
-      body: `${localizedMessageBody.part1} ${code} ${localizedMessageBody.part2}`,
+      body: 'message',
     });
     expect(mockRecoveryPhoneManager.storeUnconfirmed).toBeCalledWith(
       uid,

diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts
@@ -60,7 +60,7 @@ export class RecoveryPhoneService {
   public async setupPhoneNumber(
     uid: string,
     phoneNumber: string,
-    localizedMessageBody?: { part1: string; part2: string }
+    getFormattedMessage?: (code: string) => Promise<string>
   ) {
     if (!this.config.enabled) {
       throw new RecoveryPhoneNotEnabled();
@@ -96,13 +96,13 @@ export class RecoveryPhoneService {
       true
     );
 
-    const smsBody = localizedMessageBody
-      ? `${localizedMessageBody.part1} ${code} ${localizedMessageBody.part2}`
-      : `${code}`;
+    const formattedSMSbody = getFormattedMessage
+      ? await getFormattedMessage(code)
+      : undefined;
 
-    let msg = {} as MessageInstance;
+    const smsBody = formattedSMSbody || `${code}`;
 
-    msg = await this.smsManager.sendSMS({
+    const msg = await this.smsManager.sendSMS({
       to: phoneNumber,
       body: smsBody,
     });
@@ -286,12 +286,12 @@ export class RecoveryPhoneService {
   /**
    * Sends an totp code to a user
    * @param uid Account id
-   * @param localizedMessageBody Optional localized message body
+   * @param getFormattedMessage Optional template function to format the message
    * @returns True if message didn't fail to send.
    */
   public async sendCode(
     uid: string,
-    localizedMessageBody?: { part1: string; part2: string }
+    getFormattedMessage?: (code: string) => Promise<string>
   ) {
     if (!this.config.enabled) {
       throw new RecoveryPhoneNotEnabled();
@@ -307,8 +307,8 @@ export class RecoveryPhoneService {
       false
     );
 
-    const smsBody = localizedMessageBody
-      ? `${localizedMessageBody.part1} ${code} ${localizedMessageBody.part2}`
+    const smsBody = getFormattedMessage
+      ? await getFormattedMessage(code)
       : `${code}`;
 
     let msg = {} as MessageInstance;

diff --git a/libs/shared/db/mysql/account/src/index.ts b/libs/shared/db/mysql/account/src/index.ts
@@ -9,6 +9,7 @@ export {
   AccountFactory,
   AccountCustomerFactory,
   PaypalCustomerFactory,
+  RecoveryPhoneFactory,
 } from './lib/factories';
 export { setupAccountDatabase, AccountDbProvider } from './lib/setup';
 export { testAccountDatabaseSetup } from './lib/tests';

diff --git a/packages/functional-tests/tests/settings/recoveryPhone.spec.ts b/packages/functional-tests/tests/settings/recoveryPhone.spec.ts
@@ -103,7 +103,7 @@ test.describe('severity-1 #smoke', () => {
       await settings.disconnectTotp();
     });
 
-    test('can sign-in settings with recovery phone', async ({
+    test('can sign-in to settings with recovery phone', async ({
       target,
       pages: {
         page,
@@ -147,7 +147,7 @@ test.describe('severity-1 #smoke', () => {
         target,
       });
 
-      await page.waitForURL(/settings/);
+      await expect(await settings.settingsHeading).toBeVisible();
 
       // Remove totp so account can be deleted
       await settings.disconnectTotp();

diff --git a/packages/fxa-auth-server/lib/l10n/index.ts b/packages/fxa-auth-server/lib/l10n/index.ts
@@ -7,9 +7,16 @@ import { FluentBundle, FluentResource } from '@fluent/bundle';
 import { determineLocale, parseAcceptLanguage } from '@fxa/shared/l10n';
 import { ILocalizerBindings } from './interfaces/ILocalizerBindings';
 
+/**
+ * Represents a Fluent (FTL) message
+ * @param id - unique identifier for the message
+ * @param message - a fallback message in case the localized string cannot be found
+ * @param vars - optional arguments to be interpolated into the localized string
+ */
 export interface FtlIdMsg {
   id: string;
   message: string;
+  vars?: Record<string, string>;
 }
 
 interface LocalizedStrings {
@@ -118,11 +125,11 @@ class Localizer {
 
     const localizedFtlIdMsgs = await Promise.all(
       ftlIdMsgs.map(async (ftlIdMsg) => {
-        const { id, message } = ftlIdMsg;
+        const { id, message, vars } = ftlIdMsg;
         let localizedMessage;
         try {
-          localizedMessage = (await l10n.formatValue(id, message)) || message;
-        } catch {
+          localizedMessage = (await l10n.formatValue(id, vars)) || message;
+        } catch (e) {
           localizedMessage = message;
         }
         return Promise.resolve({

diff --git a/packages/fxa-auth-server/lib/l10n/server.ftl b/packages/fxa-auth-server/lib/l10n/server.ftl
@@ -3,12 +3,10 @@
 session-verify-send-push-title-2 = Logging in to your { -product-mozilla-account }?
 session-verify-send-push-body-2 = Click here to confirm it’s you
 
-# this message is sent by SMS and is followed by a 6-digit code used to register a recovery phone
-# sms character length is limited, please use shortest possible translation
-recovery-phone-setup-sms-body-part-1 = Mozilla verification code:
-# this message is sent by SMS and is followed by a 6-digit code used as two-step-authentication during sign-in
-# sms character length is limited, please use shortest possible translation
-recovery-phone-signin-sms-body-part-1 = Mozilla recovery code:
-# this is the second part of an SMS sent to either register or use a recovery phone
-# sms character length is limited, please use shortest possible translation
-recovery-phone-sms-body-part-2 = Expires in 5 minutes. Do not share this code.
+# Message sent by SMS with limited character length, please use shortest possible translation
+# $code  - 6 digit code used to verify phone ownership when registering a recovery phone
+recovery-phone-setup-sms-body = Mozilla verification code: { $code }. Expires in 5 minutes. Do not share this code.
+
+# Message sent by SMS with limited character length, please use shortest possible translation
+# $code  - 6 digit code used to sign in with a recovery phone as backup for two-step authentication
+recovery-phone-signin-sms-body = Mozilla recovery code: { $code }. Expires in 5 minutes. Do not share this code.
diff --git a/packages/fxa-auth-server/lib/routes/recovery-phone.ts b/packages/fxa-auth-server/lib/routes/recovery-phone.ts
@@ -52,6 +52,34 @@ class RecoveryPhoneHandler {
     this.localizer = new Localizer(new NodeRendererBindings());
   }
 
+  getLocalizedStrings = async (
+    request: AuthRequest,
+    code: string,
+    type: 'setup' | 'signin'
+  ) => {
+    const id =
+      type === 'setup'
+        ? 'recovery-phone-setup-sms-body'
+        : 'recovery-phone-signin-sms-body';
+
+    const fallbackMessage =
+      type === 'setup'
+        ? `Mozilla verification code: ${code}. Expires in 5 minutes. Do not share this code.`
+        : `Mozilla recovery code: ${code}. Expires in 5 minutes. Do not share this code.`;
+
+    const localizedStrings = await this.localizer.localizeStrings(
+      request.app.locale,
+      [
+        {
+          id,
+          message: fallbackMessage,
+          vars: { code },
+        },
+      ]
+    );
+    return localizedStrings;
+  };
+
   async sendCode(request: AuthRequest) {
     const { uid, email } = request.auth
       .credentials as SessionTokenAuthCredential;
@@ -62,31 +90,15 @@ class RecoveryPhoneHandler {
 
     await this.customs.check(request, email, 'recoveryPhoneSendCode');
 
-    const ftlIdMsgs = [
-      {
-        id: 'recovery-phone-signin-sms-body-part-1',
-        message: 'Mozilla recovery code:',
-      },
-      {
-        id: 'recovery-phone-sms-body-part-2',
-        message: 'Expires in 5 minutes. Do not share this code.',
-      },
-    ];
-    const localizedStrings = await this.localizer.localizeStrings(
-      request.app.locale,
-      ftlIdMsgs
-    );
-
-    const localizedMessageBody = {
-      part1: localizedStrings['recovery-phone-signin-sms-body-part-1'],
-      part2: localizedStrings['recovery-phone-sms-body-part-2'],
+    const getFormattedMessage = async (code: string) => {
+      return this.getLocalizedStrings(request, code, 'signin')[0];
     };
 
     let success = false;
     try {
       success = await this.recoveryPhoneService.sendCode(
         uid,
-        localizedMessageBody
+        getFormattedMessage
       );
     } catch (error) {
       if (error instanceof RecoveryNumberNotExistsError) {
@@ -132,31 +144,20 @@ class RecoveryPhoneHandler {
     }
     await this.customs.check(request, email, 'recoveryPhoneCreate');
 
-    const ftlIdMsgs = [
-      {
-        id: 'recovery-phone-setup-sms-body-part-1',
-        message: 'Mozilla recovery code:',
-      },
-      {
-        id: 'recovery-phone-sms-body-part-2',
-        message: 'Expires in 5 minutes. Do not share this code.',
-      },
-    ];
-    const localizedStrings = await this.localizer.localizeStrings(
-      request.app.locale,
-      ftlIdMsgs
-    );
-
-    const localizedMessageBody = {
-      part1: localizedStrings['recovery-phone-setup-sms-body-part-1'],
-      part2: localizedStrings['recovery-phone-sms-body-part-2'],
+    const getFormattedMessage = async (code: string) => {
+      const localizedMessage = await this.getLocalizedStrings(
+        request,
+        code,
+        'setup'
+      )[0];
+      return localizedMessage;
     };
 
     try {
       const success = await this.recoveryPhoneService.setupPhoneNumber(
         uid,
         phoneNumber,
-        localizedMessageBody
+        getFormattedMessage
       );
       if (success) {
         this.log.info('account.recoveryPhone.setupPhoneNumber.success', {
@@ -491,7 +492,7 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneStartSetup');
+        log.begin('recoveryPhoneStartSetup', request);
         return recoveryPhoneHandler.setupPhoneNumber(request);
       },
     },
@@ -504,7 +505,7 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneAvailable');
+        log.begin('recoveryPhoneAvailable', request);
         return recoveryPhoneHandler.available(request);
       },
     },
@@ -522,7 +523,7 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneConfirmSetup');
+        log.begin('recoveryPhoneConfirmSetup', request);
         return recoveryPhoneHandler.confirmCode(request, true);
       },
     },
@@ -535,7 +536,7 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneSigninSendCode');
+        log.begin('recoveryPhoneSigninSendCode', request);
         return recoveryPhoneHandler.sendCode(request);
       },
     },
@@ -548,7 +549,7 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneSigninConfirmCode');
+        log.begin('recoveryPhoneSigninConfirmCode', request);
         return recoveryPhoneHandler.confirmCode(request, false);
       },
     },
@@ -561,7 +562,7 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneRemove');
+        log.begin('recoveryPhoneRemove', request);
         return recoveryPhoneHandler.destroy(request);
       },
     },
@@ -574,7 +575,7 @@ export const recoveryPhoneRoutes = (
         },
       },
       handler: function (request: AuthRequest) {
-        log.begin('recoveryPhoneInfo');
+        log.begin('recoveryPhoneInfo', request);
         return recoveryPhoneHandler.exists(request);
       },
     },

diff --git a/packages/fxa-auth-server/test/client/api.js b/packages/fxa-auth-server/test/client/api.js
@@ -944,7 +944,7 @@ module.exports = (config) => {
     });
   };
 
-  ClientApi.prototype.recoveryPhoneCreate = async function (
+  ClientApi.prototype.recoveryPhoneNumberCreate = async function (
     sessionTokenHex,
     phoneNumber
   ) {
Original file line number	Diff line number	Diff line change
Expand Up		@@ -12,4 +12,4 @@ Run `nx test-unit recovery-phone` to execute the unit tests via [Jest](https://j

		## Running integration tests

		Run `nx test-integration recovery-phone` to execute the unit tests via [Jest](https://jestjs.io).
		Run `nx test-integration recovery-phone` to execute the integration tests via [Jest](https://jestjs.io).