Merge pull request #18391 from mozilla/FXA-11085

feat(recovery phone): Limit number of accounts that can use the same recovery phone number

Author: vpomerleau

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

@@ -74,3 +74,13 @@ export class RecoveryPhoneNotEnabled extends RecoveryPhoneError {
     super('Recovery phone not enabled.', {}, cause);
   }
 }
+
+export class RecoveryPhoneRegistrationLimitReached extends RecoveryPhoneError {
+  constructor(phoneNumber: string, cause?: Error) {
+    super(
+      'Maximum registrations reached for this phone number.',
+      { phoneNumber },
+      cause
+    );
+  }
+}

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

@@ -127,6 +127,7 @@ describe('RecoveryPhoneManager', () => {
   it('should throw if recovery phone already exists', async () => {
     const mockPhone = RecoveryPhoneFactory();
     const { uid, phoneNumber } = mockPhone;
+
     await recoveryPhoneManager.registerPhoneNumber(
       uid.toString('hex'),
       phoneNumber,
@@ -264,4 +265,37 @@ describe('RecoveryPhoneManager', () => {
 
     expect(result).toBe(true);
   });
+
+  it('should return number of accounts associated with a phone number', async () => {
+    const mockPhone = RecoveryPhoneFactory();
+    const mockPhone2 = RecoveryPhoneFactory();
+    const { uid, phoneNumber } = mockPhone;
+    const { uid: uid2 } = mockPhone2;
+
+    await db
+      .insertInto('recoveryPhones')
+      .values({
+        uid: uid,
+        phoneNumber: phoneNumber,
+        createdAt: Date.now(),
+        lastConfirmed: Date.now(),
+      })
+      .execute();
+
+    await db
+      .insertInto('recoveryPhones')
+      .values({
+        uid: uid2,
+        phoneNumber: phoneNumber,
+        createdAt: Date.now(),
+        lastConfirmed: Date.now(),
+      })
+      .execute();
+
+    const result = await recoveryPhoneManager.getCountByPhoneNumber(
+      phoneNumber
+    );
+
+    expect(result).toBe(2);
+  });
 });

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

@@ -9,6 +9,7 @@ import {
 import { Inject, Injectable } from '@nestjs/common';
 import {
   getConfirmedPhoneNumber,
+  getCountByPhoneNumber,
   hasRecoveryCodes,
   registerPhoneNumber,
   removePhoneNumber,
@@ -200,4 +201,8 @@ export class RecoveryPhoneManager {
   async hasRecoveryCodes(uid: string): Promise<boolean> {
     return hasRecoveryCodes(this.db, Buffer.from(uid, 'hex'));
   }
+
+  async getCountByPhoneNumber(phoneNumber: string) {
+    return getCountByPhoneNumber(this.db, phoneNumber);
+  }
 }

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

@@ -63,3 +63,16 @@ export async function hasRecoveryCodes(
 
   return result.length > 0;
 }
+
+export async function getCountByPhoneNumber(
+  db: AccountDatabase,
+  phoneNumber: string
+): Promise<number> {
+  const result = await db
+    .selectFrom('recoveryPhones')
+    .where('phoneNumber', '=', phoneNumber)
+    .select('uid')
+    .execute();
+
+  return result.length;
+}

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

@@ -19,6 +19,9 @@ export class RecoveryPhoneConfig {
   @IsArray()
   public allowedRegions?: Array<string>;
 
+  @IsNumber()
+  public maxRegistrationsPerNumber?: number;
+
   @IsObject()
   public sms?: RecoveryPhoneServiceConfig;
 }

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

@@ -13,6 +13,7 @@ import {
   RecoveryNumberNotSupportedError,
   RecoveryPhoneNotEnabled,
   RecoveryNumberRemoveMissingBackupCodes,
+  RecoveryPhoneRegistrationLimitReached,
 } from './recovery-phone.errors';
 import { LOGGER_PROVIDER } from '@fxa/shared/log';
 import { StatsDService } from '@fxa/shared/metrics/statsd';
@@ -49,6 +50,7 @@ describe('RecoveryPhoneService', () => {
     getConfirmedPhoneNumber: jest.fn(),
     hasRecoveryCodes: jest.fn(),
     removeCode: jest.fn(),
+    getCountByPhoneNumber: jest.fn(),
   };
 
   const mockOtpManager = {
@@ -58,6 +60,7 @@ describe('RecoveryPhoneService', () => {
   const mockRecoveryPhoneConfig: RecoveryPhoneConfig = {
     enabled: true,
     allowedRegions: ['US'],
+    maxRegistrationsPerNumber: 5,
     sms: {
       validNumberPrefixes: ['+1500'],
       smsPumpingRiskThreshold: 75,
@@ -116,7 +119,7 @@ describe('RecoveryPhoneService', () => {
     expect(service).toBeInstanceOf(RecoveryPhoneService);
   });
 
-  it('Should setup a phone number', async () => {
+  it('Should set up a phone number', async () => {
     mockOtpManager.generateCode.mockReturnValue(code);
 
     const result = await service.setupPhoneNumber(uid, phoneNumber);
@@ -134,10 +137,9 @@ describe('RecoveryPhoneService', () => {
       true
     );
     expect(mockRecoveryPhoneManager.getAllUnconfirmed).toBeCalledWith(uid);
-    expect(result).toBeTruthy();
   });
 
-  it('Should send new code for setup phone number', async () => {
+  it('Should send new code to set up a phone number', async () => {
     mockOtpManager.generateCode.mockReturnValue(code);
     mockRecoveryPhoneManager.getAllUnconfirmed.mockResolvedValue([
       'this:is:the:code123',
@@ -163,7 +165,7 @@ describe('RecoveryPhoneService', () => {
     expect(mockRecoveryPhoneManager.getAllUnconfirmed).toBeCalledWith(uid);
   });
 
-  it('handles message template when provided to setup phone number', async () => {
+  it('handles message template when provided to set up phone number', async () => {
     mockOtpManager.generateCode.mockReturnValue(code);
     const getFormattedMessage = jest.fn().mockResolvedValue('message');
 
@@ -195,6 +197,14 @@ describe('RecoveryPhoneService', () => {
     );
   });
 
+  it('Will reject a phone number if it has been used for too many accounts', async () => {
+    mockRecoveryPhoneManager.getCountByPhoneNumber.mockReturnValue(5);
+
+    await expect(service.setupPhoneNumber(uid, phoneNumber)).rejects.toEqual(
+      new RecoveryPhoneRegistrationLimitReached(phoneNumber)
+    );
+  });
+
   it('Throws error during send sms', async () => {
     mockSmsManager.sendSMS.mockRejectedValueOnce(mockError);
     await expect(service.setupPhoneNumber(uid, phoneNumber)).rejects.toEqual(
@@ -311,6 +321,19 @@ describe('RecoveryPhoneService', () => {
       );
     });
 
+    it('rejects phone number that has been used for too many accounts', async () => {
+      const phoneNumber = '+15005550000';
+      mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue({
+        isSetup: true,
+        phoneNumber,
+      });
+      mockRecoveryPhoneManager.getCountByPhoneNumber.mockResolvedValue(5);
+
+      await expect(service.confirmSetupCode(uid, code)).rejects.toThrow(
+        new RecoveryPhoneRegistrationLimitReached(phoneNumber)
+      );
+    });
+
     it('can handle failed lookup by throwing PhoneNumberNotSupported error', async () => {
       mockRecoveryPhoneManager.getUnconfirmed.mockReturnValue({
         isSetup: true,

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

@@ -15,6 +15,7 @@ import {
   RecoveryNumberNotSupportedError,
   RecoveryPhoneNotEnabled,
   RecoveryNumberRemoveMissingBackupCodes,
+  RecoveryPhoneRegistrationLimitReached,
 } from './recovery-phone.errors';
 import { MessageInstance } from 'twilio/lib/rest/api/v2010/account/message';
 import { LOGGER_PROVIDER } from '@fxa/shared/log';
@@ -97,6 +98,17 @@ export class RecoveryPhoneService {
       }
     }
 
+    // Rejects the phone number if it has been registered for too many accounts
+    const countByPhoneNumber =
+      await this.recoveryPhoneManager.getCountByPhoneNumber(phoneNumber);
+
+    if (
+      this.config.maxRegistrationsPerNumber &&
+      countByPhoneNumber >= this.config.maxRegistrationsPerNumber
+    ) {
+      throw new RecoveryPhoneRegistrationLimitReached(phoneNumber);
+    }
+
     const code = await this.otpCode.generateCode();
 
     await this.recoveryPhoneManager.storeUnconfirmed(
@@ -156,6 +168,17 @@ export class RecoveryPhoneService {
       return false;
     }
 
+    // Rejects the phone number if it has been registered for too many accounts
+    const countByPhoneNumber =
+      await this.recoveryPhoneManager.getCountByPhoneNumber(data.phoneNumber);
+
+    if (
+      this.config.maxRegistrationsPerNumber &&
+      countByPhoneNumber >= this.config.maxRegistrationsPerNumber
+    ) {
+      throw new RecoveryPhoneRegistrationLimitReached(data.phoneNumber);
+    }
+
     // If this was for a setup operation. Register the phone number to the uid.
     const lookupData: PhoneNumberLookupData = await (async () => {
       try {

packages/fxa-auth-server/config/index.ts

@@ -2168,7 +2168,7 @@ const convictConf = convict({
       format: Boolean,
     },
     allowedRegions: {
-      default: ['US'],
+      default: ['CA', 'US'],
       doc: 'Allowed regions for recovery phone',
       env: 'RECOVERY_PHONE__ALLOWED_REGIONS',
       format: Array,
@@ -2187,6 +2187,12 @@ const convictConf = convict({
         format: Number,
       },
     },
+    maxRegistrationsPerNumber: {
+      default: 5,
+      doc: 'Max number of uids that can be associated to the same phone number.',
+      env: 'RECOVERY_PHONE__MAX_UID_PER_NUMBER',
+      format: Number,
+    },
     redis: {},
     sms: {
       from: {

packages/fxa-auth-server/lib/error.js

@@ -613,6 +613,16 @@ AppError.smsSendRateLimitExceeded = () => {
   });
 };
 
+AppError.recoveryPhoneRegistrationLimitReached = () => {
+  return new AppError({
+    code: 400,
+    error: 'Bad Request',
+    errno: ERRNO.RECOVERY_PHONE_REGISTRATION_LIMIT_REACHED,
+    message:
+      'Limit reached for number off accounts that can be associated with phone number.',
+  });
+};
+
 AppError.invalidRegion = (region) => {
   return new AppError(
     {

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

@@ -14,6 +14,7 @@ import {
   RecoveryNumberNotExistsError,
   SmsSendRateLimitExceededError,
   RecoveryNumberRemoveMissingBackupCodes,
+  RecoveryPhoneRegistrationLimitReached,
   TwilioMessageStatus,
 } from '@fxa/accounts/recovery-phone';
 import {
@@ -230,6 +231,10 @@ class RecoveryPhoneHandler {
         throw AppError.smsSendRateLimitExceeded();
       }
 
+      if (error instanceof RecoveryPhoneRegistrationLimitReached) {
+        throw AppError.recoveryPhoneRegistrationLimitReached();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'setupPhoneNumber',
@@ -291,6 +296,10 @@ class RecoveryPhoneHandler {
         throw AppError.recoveryPhoneNumberAlreadyExists();
       }
 
+      if (error instanceof RecoveryPhoneRegistrationLimitReached) {
+        throw AppError.recoveryPhoneRegistrationLimitReached();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'confirmCode',