Merge pull request #18221 from mozilla/FXA-10973

task(recovery-phone): Support rotating from phone number

Author: dschom

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

@@ -7,6 +7,7 @@ import { BaseError } from '@fxa/shared/error';
 // See full list of codes here, https://www.twilio.com/docs/api/errors
 export const TwilioErrorCodes = {
   ['INVALID_TO_PHONE_NUMBER']: 21211,
+  ['SMS_SEND_RATE_LIMIT_EXCEEDED']: 14107,
 };
 
 export class RecoveryPhoneError extends BaseError {
@@ -42,3 +43,18 @@ export class RecoveryNumberNotSupportedError extends RecoveryPhoneError {
     super('Phone number not supported.', { phoneNumber }, cause);
   }
 }
+
+export class SmsSendRateLimitExceededError extends RecoveryPhoneError {
+  constructor(
+    uid: string,
+    toPhoneNumber: string,
+    fromPhoneNumber: string,
+    cause?: Error
+  ) {
+    super(
+      'Too many SMS are currently being sent. Try again later.',
+      { uid, toPhoneNumber, fromPhoneNumber },
+      cause
+    );
+  }
+}

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

@@ -8,10 +8,11 @@ import { Test, TestingModule } from '@nestjs/testing';
 import { SmsManager } from './sms.manager';
 import { SmsManagerConfig } from './sms.manger.config';
 import { TwilioProvider } from './twilio.provider';
+import { TwilioErrorCodes } from './recovery-phone.errors';
 
 describe('SmsManager', () => {
   const to = '+15005551111';
-  const from = '+15005550000';
+  const from = ['+15005550000', '+15005550001'];
   const mockTwilioSmsClient = {
     messages: {
       create: jest.fn(),
@@ -27,6 +28,7 @@ describe('SmsManager', () => {
     from,
     validNumberPrefixes: ['+1'],
     maxMessageLength: 160,
+    maxRetries: 2,
   };
 
   let manager: SmsManager;
@@ -68,7 +70,7 @@ describe('SmsManager', () => {
     expect(msg).toBeDefined();
     expect(mockTwilioSmsClient.messages.create).toBeCalledWith({
       to,
-      from,
+      from: from[0],
       body,
     });
     expect(msg?.status).toEqual('sent');
@@ -105,6 +107,62 @@ describe('SmsManager', () => {
     );
   });
 
+  it('Retries if send rate limit exceeded.', async () => {
+    const mockError = new Error();
+    (mockError as any).code = TwilioErrorCodes.SMS_SEND_RATE_LIMIT_EXCEEDED;
+    mockTwilioSmsClient.messages.create.mockRejectedValueOnce(mockError);
+    mockTwilioSmsClient.messages.create.mockReturnValueOnce({
+      sid: 'foo',
+      status: 'sent',
+    });
+    const body = 'test success';
+    const msg = await manager.sendSMS({
+      to,
+      body,
+    });
+
+    expect(msg).toBeDefined();
+    expect(mockTwilioSmsClient.messages.create).toBeCalledWith({
+      to,
+      from: from[0],
+      body,
+    });
+    expect(mockTwilioSmsClient.messages.create).toBeCalledWith({
+      to,
+      from: from[0],
+      body,
+    });
+    expect(mockTwilioSmsClient.messages.create).toBeCalledTimes(2);
+    expect(msg?.status).toEqual('sent');
+    expect(mockLog.log).toBeCalledTimes(1);
+    expect(mockLog.log).toBeCalledWith('SMS sent', {
+      sid: 'foo',
+      status: 'sent',
+    });
+    expect(mockMetrics.increment).toBeCalledTimes(1);
+    expect(mockMetrics.increment).toBeCalledWith('sms.send.sent');
+  });
+
+  it('Retries but eventually fails if send rate limit exceeded.', async () => {
+    const mockError = new Error();
+    (mockError as any).code = TwilioErrorCodes.SMS_SEND_RATE_LIMIT_EXCEEDED;
+
+    mockTwilioSmsClient.messages.create.mockRejectedValue(mockError);
+    const body = 'test success';
+    await expect(
+      manager.sendSMS({
+        to,
+        body,
+      })
+    ).rejects.toThrow(
+      'Too many SMS are currently being sent. Try again later.'
+    );
+
+    expect(mockLog.log).toBeCalledTimes(0);
+    expect(mockMetrics.increment).toBeCalledTimes(1);
+    expect(mockTwilioSmsClient.messages.create).toBeCalledTimes(3); // initial call + config.maxRetries.
+  });
+
   it('Records failure', async () => {
     const boom = new Error('Boom');
     mockTwilioSmsClient.messages.create.mockRejectedValue(boom);
@@ -120,4 +178,13 @@ describe('SmsManager', () => {
     expect(mockLog.log).toBeCalledTimes(0);
     expect(mockMetrics.increment).toBeCalledTimes(1);
   });
+
+  it('Rotates numbers', () => {
+    const number1 = manager.rotateFromNumber();
+    const number2 = manager.rotateFromNumber();
+
+    expect(number1).toBeDefined();
+    expect(number2).toBeDefined();
+    expect(number1).not.toEqual(number2);
+  });
 });

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

@@ -7,16 +7,20 @@ import { StatsDService } from '@fxa/shared/metrics/statsd';
 import { Inject, Injectable, LoggerService } from '@nestjs/common';
 import { StatsD } from 'hot-shots';
 import { Twilio } from 'twilio';
+import { MessageInstance } from 'twilio/lib/rest/api/v2010/account/message';
 import { SmsManagerConfig } from './sms.manger.config';
 import { TwilioProvider } from './twilio.provider';
 import {
   RecoveryNumberInvalidFormatError,
   RecoveryNumberNotSupportedError,
+  SmsSendRateLimitExceededError,
   TwilioErrorCodes,
 } from './recovery-phone.errors';
 
 @Injectable()
 export class SmsManager {
+  private currentPhoneNumber = 0;
+
   constructor(
     @Inject(TwilioProvider) private readonly client: Twilio,
     @Inject(StatsDService) private readonly metrics: StatsD,
@@ -56,12 +60,20 @@ export class SmsManager {
       }
     }
 
+    return await this._sendSMS(to, body, uid, 0);
+  }
+
+  private async _sendSMS(
+    to: string,
+    body: string,
+    uid: string | undefined,
+    retryCount: number
+  ): Promise<MessageInstance> {
+    const from = this.rotateFromNumber();
     try {
       const msg = await this.client.messages.create({
         to,
-        // TODO: Should this just be passed in every time or set from config?
-        //       Will we always send from the same number?
-        from: this.config.from,
+        from,
         body,
       });
       // Typically the message will be in queued status. The following metric and log
@@ -73,12 +85,31 @@ export class SmsManager {
       });
       return msg;
     } catch (err) {
+      if (err.code === TwilioErrorCodes.SMS_SEND_RATE_LIMIT_EXCEEDED) {
+        if (retryCount < this.config.maxRetries) {
+          // Exp back off and try again
+          await new Promise((r) =>
+            setTimeout(r, Math.pow(2, retryCount++) * 1000)
+          );
+          return await this._sendSMS(to, body, uid, retryCount);
+        }
+      }
+
       this.metrics.increment('sms.send.error');
 
       if (err.code === TwilioErrorCodes.INVALID_TO_PHONE_NUMBER) {
         throw new RecoveryNumberInvalidFormatError(uid || '', to, err);
       }
+      if (err.code === TwilioErrorCodes.SMS_SEND_RATE_LIMIT_EXCEEDED) {
+        throw new SmsSendRateLimitExceededError(uid || '', to, from, err);
+      }
       throw err;
     }
   }
+
+  public rotateFromNumber() {
+    return this.config.from[
+      this.currentPhoneNumber++ % this.config.from.length
+    ];
+  }
 }

libs/accounts/recovery-phone/src/lib/sms.manger.config.ts

@@ -9,11 +9,14 @@ export class SmsManagerConfig {
    * The number that SMS are sent from. This should be our number.
    * */
   @IsString()
-  public readonly from!: string;
+  public readonly from!: string[];
 
   @IsNumber()
   public readonly maxMessageLength!: number;
 
+  @IsNumber()
+  public readonly maxRetries!: number;
+
   @IsArray()
   public readonly validNumberPrefixes!: Array<string>;
 }

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

@@ -2168,10 +2168,10 @@ const convictConf = convict({
     redis: {},
     sms: {
       from: {
-        default: '15005550006',
+        default: ['15005550006'],
         doc: 'The twilio number messages are sent from.',
         env: 'RECOVERY_PHONE__SMS__FROM',
-        format: String,
+        format: Array,
       },
       maxMessageLength: {
         default: 60,
@@ -2185,6 +2185,12 @@ const convictConf = convict({
         env: 'RECOVERY_PHONE__SMS__VALID_NUMBER_PREFIXES',
         format: Array,
       },
+      maxRetries: {
+        default: 3,
+        doc: 'Number of times an sms message can be retried in the event a sms sending rate limit is encountered. Note that we use an exponential back off here. e.g. 3 would mean up to a 1s, 2s and then 4s retry.',
+        env: 'RECOVERY_PHONE__SMS__MAX_RETRIES',
+        format: Number,
+      },
     },
   },
   twilio: {

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

@@ -594,6 +594,15 @@ AppError.recoveryPhoneNumberDoesNotExist = () => {
   });
 };
 
+AppError.smsSendRateLimitExceeded = () => {
+  return new AppError({
+    code: 429,
+    error: 'Too many requests',
+    errno: ERRNO.SMS_SEND_RATE_LIMIT_EXCEEDED,
+    message: 'Client has sent too many requests',
+  });
+};
+
 AppError.invalidRegion = (region) => {
   return new AppError(
     {

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

@@ -8,6 +8,7 @@ import {
   RecoveryNumberInvalidFormatError,
   RecoveryNumberAlreadyExistsError,
   RecoveryNumberNotExistsError,
+  SmsSendRateLimitExceededError,
 } from '@fxa/accounts/recovery-phone';
 import {
   AccountManager,
@@ -61,6 +62,10 @@ class RecoveryPhoneHandler {
         throw AppError.recoveryPhoneNumberDoesNotExist();
       }
 
+      if (error instanceof SmsSendRateLimitExceededError) {
+        throw AppError.smsSendRateLimitExceeded();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'sendCode',
@@ -113,6 +118,10 @@ class RecoveryPhoneHandler {
         throw AppError.invalidPhoneNumber();
       }
 
+      if (error instanceof SmsSendRateLimitExceededError) {
+        throw AppError.smsSendRateLimitExceeded();
+      }
+
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',
         'setupPhoneNumber',

packages/fxa-auth-server/test/local/routes/recovery-phone.js

@@ -11,6 +11,7 @@ const assert = { ...sinon.assert, ...chai.assert };
 const { recoveryPhoneRoutes } = require('../../../lib/routes/recovery-phone');
 import {
   RecoveryNumberNotSupportedError,
+  SmsSendRateLimitExceededError,
   RecoveryPhoneService,
 } from '@fxa/accounts/recovery-phone';
 
@@ -203,6 +204,25 @@ describe('/recovery_phone', () => {
       assert.equal(mockGlean.twoStepAuthPhoneCode.sendError.callCount, 1);
     });
 
+    it('indicates too many requests when sms rate limit is exceeded', async () => {
+      mockRecoveryPhoneService.setupPhoneNumber = sinon.fake.returns(
+        Promise.reject(
+          new SmsSendRateLimitExceededError(uid, phoneNumber, '+495550005555')
+        )
+      );
+
+      const promise = makeRequest({
+        method: 'POST',
+        path: '/recovery_phone/create',
+        credentials: { uid, email },
+        payload: { phoneNumber: '+495550005555' },
+      });
+
+      await assert.isRejected(promise, 'Client has sent too many requests');
+      assert.equal(mockGlean.twoStepAuthPhoneCode.sent.callCount, 0);
+      assert.equal(mockGlean.twoStepAuthPhoneCode.sendError.callCount, 1);
+    });
+
     it('handles unexpected backend error', async () => {
       mockRecoveryPhoneService.setupPhoneNumber = sinon.fake.returns(
         Promise.reject(new Error('BOOM'))

packages/fxa-shared/lib/errors.ts

@@ -131,6 +131,8 @@ export const AUTH_SERVER_ERRNOS = {
   RECOVERY_PHONE_NUMBER_ALREADY_EXISTS: 214,
   RECOVERY_PHONE_NUMBER_DOES_NOT_EXIST: 215,
 
+  SMS_SEND_RATE_LIMIT_EXCEEDED: 216,
+
   INTERNAL_VALIDATION_ERROR: 998,
   UNEXPECTED_ERROR: 999,
 };