Merge pull request #18533 from mozilla/FXA-11111

task(recovery-phone): Standardize SMS code validity on resend

Author: dschom

libs/accounts/recovery-phone/README.md

@@ -12,6 +12,8 @@ Run `nx test-unit accounts-recovery-phone` to execute the unit tests via [Jest](
 
 ## Running integration tests
 
+Make sure local infra (ie databases) are spun up by checking status. `yarn pm2 status` should show redis and mysql instances running. If not, run `yarn start infrastructure`.
+
 Run `nx test-integration accounts-recovery-phone` to execute the integration tests via [Jest](https://jestjs.io).
 
 ## Testing webhook callbacks for message status updates

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

@@ -1,5 +1,6 @@
 import {
   PhoneNumberLookupData,
+  RECOVERY_PHONE_REDIS_PREFIX,
   RecoveryPhoneManager,
 } from './recovery-phone.manager';
 import {
@@ -9,10 +10,12 @@ import {
   RecoveryPhoneFactory,
 } from '@fxa/shared/db/mysql/account';
 import { Test } from '@nestjs/testing';
+import Redis from 'ioredis';
 
 describe('RecoveryPhoneManager', () => {
   let recoveryPhoneManager: RecoveryPhoneManager;
   let db: AccountDatabase;
+  let redis: Redis.Redis;
   const dateMock = jest.spyOn(global.Date, 'now');
 
   // Taken from: https://www.twilio.com/docs/lookup/v2-api#code-lookup-with-data-packages
@@ -38,19 +41,29 @@ describe('RecoveryPhoneManager', () => {
     callForwarding: {},
   };
 
-  const mockRedis = {
-    set: jest.fn(),
-    get: jest.fn(),
-    del: jest.fn(),
-  };
+  async function clearRedisSmsKeys() {
+    const addedKeys = await redis.keys(RECOVERY_PHONE_REDIS_PREFIX);
+    for (const key of addedKeys) {
+      await redis.del(key);
+    }
+  }
 
   beforeAll(async () => {
     dateMock.mockImplementation(() => 1739227529776);
+
     db = await testAccountDatabaseSetup([
       'accounts',
       'recoveryPhones',
       'recoveryCodes',
     ]);
+
+    // Since this is an integration we can expect that
+    // infrastructure is running.
+    redis = new Redis('localhost');
+
+    // Make sure sms key state is clean
+    await clearRedisSmsKeys();
+
     const moduleRef = await Test.createTestingModule({
       providers: [
         RecoveryPhoneManager,
@@ -60,7 +73,7 @@ describe('RecoveryPhoneManager', () => {
         },
         {
           provide: 'RecoveryPhoneRedis',
-          useValue: mockRedis,
+          useValue: redis,
         },
       ],
     }).compile();
@@ -69,6 +82,7 @@ describe('RecoveryPhoneManager', () => {
   });
 
   afterAll(async () => {
+    await clearRedisSmsKeys();
     await db.destroy();
     dateMock.mockReset();
   });
@@ -192,48 +206,63 @@ describe('RecoveryPhoneManager', () => {
     insertIntoSpy.mockRestore();
   });
 
-  it('should store unconfirmed phone number data in Redis', async () => {
+  it('should store and retrieve unconfirmed phone number data in Redis', async () => {
     const mockPhone = RecoveryPhoneFactory();
     const { uid, phoneNumber } = mockPhone;
-    const code = '123456';
     const isSetup = true;
     await recoveryPhoneManager.storeUnconfirmed(
       uid.toString('hex'),
-      code,
+      '111111',
       phoneNumber,
       isSetup,
       mockLookUpData
     );
-
-    const redisKey = `sms-attempt:${uid.toString('hex')}:${code}`;
-
-    expect(mockRedis.set).toHaveBeenCalledWith(
-      redisKey,
-      expect.any(String),
-      'EX',
-      600
+    await recoveryPhoneManager.storeUnconfirmed(
+      uid.toString('hex'),
+      '222222',
+      phoneNumber,
+      isSetup
     );
 
-    const expectedData = expect.objectContaining({
-      createdAt: expect.any(Number),
+    // Store one item for different accounts to make sure lookup is filtering
+    // on uid
+    await recoveryPhoneManager.storeUnconfirmed(
+      uid.toString('hex').replace(/./g, '1'),
+      '333333',
       phoneNumber,
-      isSetup,
-      lookupData: JSON.stringify(mockLookUpData),
-    });
+      isSetup
+    );
+
+    const firstUnconfirmedCode = await recoveryPhoneManager.getUnconfirmed(
+      uid.toString('hex'),
+      '111111'
+    );
+    expect(firstUnconfirmedCode).toBeDefined();
+    expect(firstUnconfirmedCode?.lookupData).toBeDefined();
+    expect(firstUnconfirmedCode?.lookupData).toEqual(
+      JSON.stringify(mockLookUpData)
+    );
 
-    const storedData = mockRedis.set.mock.calls[0][1];
-    expect(() => JSON.parse(storedData)).not.toThrow();
-    const parsedData = JSON.parse(mockRedis.set.mock.calls[0][1]);
-    expect(parsedData).toEqual(expectedData);
+    const secondUnconfirmedCode = await recoveryPhoneManager.getUnconfirmed(
+      uid.toString('hex'),
+      '222222'
+    );
+    expect(secondUnconfirmedCode).toBeDefined();
+    expect(secondUnconfirmedCode?.lookupData).toEqual(null);
+
+    const allUnconfirmedCodes =
+      await recoveryPhoneManager.getAllUnconfirmedCodes(uid.toString('hex'));
+    expect(allUnconfirmedCodes.length).toEqual(2);
+    expect(allUnconfirmedCodes).toContain('111111');
+    expect(allUnconfirmedCodes).toContain('222222');
+    expect(allUnconfirmedCodes).not.toContain('333333');
   });
 
   it('should return null if no unconfirmed phone number data is found in Redis', async () => {
     const mockPhone = RecoveryPhoneFactory();
     const { uid } = mockPhone;
     const code = '123456';
 
-    mockRedis.get.mockResolvedValue(null);
-
     const result = await recoveryPhoneManager.getUnconfirmed(
       uid.toString('hex'),
       code

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

@@ -31,10 +31,13 @@ export type PhoneNumberLookupData = ReturnType<
   typeof PhoneNumberInstance.prototype.toJSON
 >;
 
+/**
+ * Standard prefix for all recovery phone entries in redis.
+ */
+export const RECOVERY_PHONE_REDIS_PREFIX = 'recovery-phone:sms-attempt';
+
 @Injectable()
 export class RecoveryPhoneManager {
-  private readonly redisPrefix = 'sms-attempt';
-
   constructor(
     @Inject(AccountDbProvider) private readonly db: AccountDatabase,
     @Inject('RecoveryPhoneRedis') private readonly redisClient: Redis
@@ -128,7 +131,7 @@ export class RecoveryPhoneManager {
     isSetup: boolean,
     lookupData?: PhoneNumberLookupData
   ): Promise<void> {
-    const redisKey = `${this.redisPrefix}:${uid}:${code}`;
+    const redisKey = `${RECOVERY_PHONE_REDIS_PREFIX}:${uid}:${code}`;
     const data = {
       createdAt: Date.now(),
       phoneNumber,
@@ -159,7 +162,7 @@ export class RecoveryPhoneManager {
     isSetup: boolean;
     lookupData: Record<string, any> | null;
   } | null> {
-    const redisKey = `${this.redisPrefix}:${uid}:${code}`;
+    const redisKey = `${RECOVERY_PHONE_REDIS_PREFIX}:${uid}:${code}`;
     const data = await this.redisClient.get(redisKey);
 
     if (!data) {
@@ -174,9 +177,28 @@ export class RecoveryPhoneManager {
    *
    * @param uid
    */
-  async getAllUnconfirmed(uid: string): Promise<string[]> {
-    const redisKey = `${this.redisPrefix}:${uid}:*`;
-    return await this.redisClient.keys(redisKey);
+  private async getAllUnconfirmedKeys(uid: string): Promise<string[]> {
+    const pattern = `${RECOVERY_PHONE_REDIS_PREFIX}:${uid}:*`;
+    let cursor = '0';
+    let keys: string[] = [];
+
+    do {
+      const reply = await this.redisClient.scan(cursor, 'MATCH', pattern);
+      cursor = reply[0];
+      keys = keys.concat(reply[1]);
+    } while (cursor !== '0');
+
+    return keys;
+  }
+
+  /**
+   * Returns codes sent out for all unconfirmed phone numbers for a user.
+   * @param uid
+   * @returns
+   */
+  async getAllUnconfirmedCodes(uid: string): Promise<string[]> {
+    const keys = await this.getAllUnconfirmedKeys(uid);
+    return keys.map((x) => x.split(':').pop() || '').filter((x) => !!x);
   }
 
   /**
@@ -187,7 +209,7 @@ export class RecoveryPhoneManager {
    * @returns
    */
   async removeCode(uid: string, code: string) {
-    const redisKey = `${this.redisPrefix}:${uid}:${code}`;
+    const redisKey = `${RECOVERY_PHONE_REDIS_PREFIX}:${uid}:${code}`;
     const count = await this.redisClient.del(redisKey);
     return count > 0;
   }

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

@@ -20,7 +20,7 @@ export const RecoveryPhoneRedisProvider = {
   provide: 'RecoveryPhoneRedis',
   useFactory: (config: ConfigService) => {
     const baseRedisConfig = config.get('redis');
-    const recoveryPhoneRedisConfig = config.get('recoveryPhone.redis');
+    const recoveryPhoneRedisConfig = config.get('redis.recoveryPhone');
     return new Redis({
       ...baseRedisConfig,
       ...recoveryPhoneRedisConfig,

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

@@ -53,7 +53,7 @@ describe('RecoveryPhoneService', () => {
   const mockRecoveryPhoneManager = {
     storeUnconfirmed: jest.fn(),
     getUnconfirmed: jest.fn(),
-    getAllUnconfirmed: jest.fn(),
+    getAllUnconfirmedCodes: jest.fn(),
     registerPhoneNumber: jest.fn(),
     removePhoneNumber: jest.fn(),
     getConfirmedPhoneNumber: jest.fn(),
@@ -118,7 +118,7 @@ describe('RecoveryPhoneService', () => {
       };
     });
     mockRecoveryPhoneManager.hasRecoveryCodes.mockResolvedValue(true);
-    mockRecoveryPhoneManager.getAllUnconfirmed.mockResolvedValue([]);
+    mockRecoveryPhoneManager.getAllUnconfirmedCodes.mockResolvedValue([]);
 
     const module: TestingModule = await Test.createTestingModule({
       providers: [
@@ -178,15 +178,15 @@ describe('RecoveryPhoneService', () => {
       phoneNumber,
       true
     );
-    expect(mockRecoveryPhoneManager.getAllUnconfirmed).toBeCalledWith(uid);
+    expect(mockRecoveryPhoneManager.getAllUnconfirmedCodes).toBeCalledWith(uid);
     expect(result).toBeTruthy();
   });
 
   it('Should send new code to set up a phone number', async () => {
     mockOtpManager.generateCode.mockReturnValue(code);
-    mockRecoveryPhoneManager.getAllUnconfirmed.mockResolvedValue([
-      'this:is:the:code123',
-      'this:is:the:code456',
+    mockRecoveryPhoneManager.getAllUnconfirmedCodes.mockResolvedValue([
+      'code123',
+      'code456',
     ]);
 
     const result = await service.setupPhoneNumber(
@@ -210,7 +210,7 @@ describe('RecoveryPhoneService', () => {
       phoneNumber,
       true
     );
-    expect(mockRecoveryPhoneManager.getAllUnconfirmed).toBeCalledWith(uid);
+    expect(mockRecoveryPhoneManager.getAllUnconfirmedCodes).toBeCalledWith(uid);
   });
 
   it('handles message template when provided to set up phone number', async () => {
@@ -235,7 +235,7 @@ describe('RecoveryPhoneService', () => {
       phoneNumber,
       true
     );
-    expect(mockRecoveryPhoneManager.getAllUnconfirmed).toBeCalledWith(uid);
+    expect(mockRecoveryPhoneManager.getAllUnconfirmedCodes).toBeCalledWith(uid);
   });
 
   it('Will reject a phone number that is not part of launch', async () => {
@@ -601,9 +601,9 @@ describe('RecoveryPhoneService', () => {
 
     it('Should send new code for setup phone number', async () => {
       mockOtpManager.generateCode.mockReturnValue(code);
-      mockRecoveryPhoneManager.getAllUnconfirmed.mockResolvedValue([
-        'this:is:the:code123',
-        'this:is:the:code456',
+      mockRecoveryPhoneManager.getAllUnconfirmedCodes.mockResolvedValue([
+        'code123',
+        'code456',
       ]);
 
       mockRecoveryPhoneManager.getConfirmedPhoneNumber.mockResolvedValueOnce({
@@ -639,7 +639,9 @@ describe('RecoveryPhoneService', () => {
         uid,
         'code456'
       );
-      expect(mockRecoveryPhoneManager.getAllUnconfirmed).toBeCalledWith(uid);
+      expect(mockRecoveryPhoneManager.getAllUnconfirmedCodes).toBeCalledWith(
+        uid
+      );
     });
   });
 

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

@@ -134,14 +134,10 @@ export class RecoveryPhoneService {
     }
 
     // Invalidate and remove any or all previous unconfirmed code entries
-    const unconfirmedKeys = await this.recoveryPhoneManager.getAllUnconfirmed(
-      uid
-    );
-    for (const key of unconfirmedKeys) {
-      const code = key.split(':').pop();
-      if (code) {
-        await this.recoveryPhoneManager.removeCode(uid, code);
-      }
+    const unconfirmedCodes =
+      await this.recoveryPhoneManager.getAllUnconfirmedCodes(uid);
+    for (const code of unconfirmedCodes) {
+      await this.recoveryPhoneManager.removeCode(uid, code);
     }
 
     // Rejects the phone number if it has been registered for too many accounts
@@ -456,14 +452,10 @@ export class RecoveryPhoneService {
     }
 
     // Invalidate and remove any or all previous unconfirmed code entries
-    const unconfirmedKeys = await this.recoveryPhoneManager.getAllUnconfirmed(
-      uid
-    );
-    for (const key of unconfirmedKeys) {
-      const oldCode = key.split(':').pop();
-      if (oldCode) {
-        await this.recoveryPhoneManager.removeCode(uid, oldCode);
-      }
+    const unconfirmedCodes =
+      await this.recoveryPhoneManager.getAllUnconfirmedCodes(uid);
+    for (const oldCode of unconfirmedCodes) {
+      await this.recoveryPhoneManager.removeCode(uid, oldCode);
     }
 
     // Generate a new otp code, and store it as unconfirmed for later validation

libs/accounts/recovery-phone/src/lib/twilio.provider.ts

@@ -46,9 +46,22 @@ export const TwilioFactory: Provider<Twilio> = {
     if (credentialMode === 'apiKeys' && accountSid && apiKey && apiSecret) {
       return new Twilio(apiKey, apiSecret, { accountSid });
     }
-
     throw new Error(
-      'Invalid configuration state. Check docs for TwilioConfig, a value is probably missing.'
+      `Invalid configuration state for credential mode ${credentialMode}.\n ${JSON.stringify(
+        {
+          credentialMode,
+          has: {
+            testAccountSid: !!testAccountSid,
+            testAuthToken: !!testAuthToken,
+            accountSid: !!accountSid,
+            authToken: !!authToken,
+            apiKey: !!apiKey,
+            apiSecret: !!apiKey,
+          },
+        },
+        null,
+        ''
+      )}`
     );
   },
   inject: [TwilioConfig],