Merge pull request #18417 from mozilla/FXA-11133

task(recovery-phone): Add support for message status updates

Author: dschom

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

@@ -16,6 +16,9 @@ import {
 } from './recovery-phone.errors';
 import { LOGGER_PROVIDER } from '@fxa/shared/log';
 import { StatsDService } from '@fxa/shared/metrics/statsd';
+import { MessageStatus } from 'twilio/lib/rest/api/v2010/account/message';
+import { TwilioConfig } from './twilio.config';
+import { getExpectedTwilioSignature } from 'twilio/lib/webhooks/webhooks';
 
 describe('RecoveryPhoneService', () => {
   const phoneNumber = '+15005551234';
@@ -25,14 +28,18 @@ describe('RecoveryPhoneService', () => {
   const mockLogger = {
     error: jest.fn(),
   };
+
   const mockMetrics = {
     gauge: jest.fn(),
     increment: jest.fn(),
   };
+
   const mockSmsManager = {
     sendSMS: jest.fn(),
     phoneNumberLookup: jest.fn(),
+    messageStatus: jest.fn(),
   };
+
   const mockRecoveryPhoneManager = {
     storeUnconfirmed: jest.fn(),
     getUnconfirmed: jest.fn(),
@@ -43,15 +50,27 @@ describe('RecoveryPhoneService', () => {
     hasRecoveryCodes: jest.fn(),
     removeCode: jest.fn(),
   };
-  const mockOtpManager = { generateCode: jest.fn() };
-  const mockRecoveryPhoneConfig = {
+
+  const mockOtpManager = {
+    generateCode: jest.fn(),
+  };
+
+  const mockRecoveryPhoneConfig: RecoveryPhoneConfig = {
     enabled: true,
     allowedRegions: ['US'],
     sms: {
       validNumberPrefixes: ['+1500'],
       smsPumpingRiskThreshold: 75,
     },
-  };
+  } satisfies RecoveryPhoneConfig;
+
+  const mockTwilioConfig: TwilioConfig = {
+    accountSid: 'AC00000000000000000000000000000000',
+    authToken: '00000000000000000000000000000000',
+    webhookUrl: 'http://accounts.firefox.com/recovery-phone/message-status',
+    validateWebhookCalls: true,
+  } satisfies TwilioConfig;
+
   const mockError = new Error('BOOM');
 
   let service: RecoveryPhoneService;
@@ -70,6 +89,10 @@ describe('RecoveryPhoneService', () => {
           provide: RecoveryPhoneConfig,
           useValue: mockRecoveryPhoneConfig,
         },
+        {
+          provide: TwilioConfig,
+          useValue: mockTwilioConfig,
+        },
         {
           provide: LOGGER_PROVIDER,
           useValue: mockLogger,
@@ -588,4 +611,63 @@ describe('RecoveryPhoneService', () => {
       expect(service.stripPhoneNumber(phoneNumber, 4)).toEqual('');
     });
   });
+
+  describe('can handle message status update', () => {
+    it('can handle message status update', async () => {
+      const messageUpdate = {
+        AccountSid: 'AC123',
+        From: '+123456789',
+        MessageSid: 'MS123',
+        MessageStatus: 'delivered' as MessageStatus,
+      };
+      await service.onMessageStatusUpdate(messageUpdate);
+      expect(mockSmsManager.messageStatus).toBeCalledWith(messageUpdate);
+    });
+  });
+
+  describe('verify twilio signature', () => {
+    // This is how Twilio generates the signature, see following doc for more info:
+    // https://www.twilio.com/docs/usage/security#test-the-validity-of-your-webhook-signature
+    const signature = getExpectedTwilioSignature(
+      mockTwilioConfig.authToken,
+      mockTwilioConfig.webhookUrl,
+      {
+        foo: 'bar',
+      }
+    );
+
+    afterEach(() => {
+      mockTwilioConfig.validateWebhookCalls = true;
+    });
+
+    it('can validate twilio signature', () => {
+      const valid = service.validateTwilioSignature(signature, {
+        foo: 'bar',
+      });
+      expect(valid).toBeTruthy();
+    });
+
+    it('can invalidate twilio signature due to bad payload', () => {
+      const valid = service.validateTwilioSignature(signature, {
+        foo: 'bar',
+        bar: 'baz',
+      });
+      expect(valid).toBeFalsy();
+    });
+
+    it('can invalidate twilio signature due to bad signature', () => {
+      const valid = service.validateTwilioSignature(signature + '0', {
+        foo: 'bar',
+      });
+      expect(valid).toBeFalsy();
+    });
+
+    it('will always validate if validateWebhookCalls is false', () => {
+      mockTwilioConfig.validateWebhookCalls = false;
+      const valid = service.validateTwilioSignature(signature + '0', {
+        foo: 'bar',
+      });
+      expect(valid).toBeTruthy();
+    });
+  });
 });

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

@@ -3,7 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import { Inject, Injectable, LoggerService } from '@nestjs/common';
-import { SmsManager } from './sms.manager';
+import { SmsManager, TwilioMessageStatus } from './sms.manager';
 import { OtpManager } from '@fxa/shared/otp';
 import { RecoveryPhoneConfig } from './recovery-phone.service.config';
 import {
@@ -19,6 +19,8 @@ import {
 import { MessageInstance } from 'twilio/lib/rest/api/v2010/account/message';
 import { LOGGER_PROVIDER } from '@fxa/shared/log';
 import { StatsD, StatsDService } from '@fxa/shared/metrics/statsd';
+import { TwilioConfig } from './twilio.config';
+import { validateRequest } from 'twilio';
 
 @Injectable()
 export class RecoveryPhoneService {
@@ -27,6 +29,7 @@ export class RecoveryPhoneService {
     private readonly smsManager: SmsManager,
     private readonly otpCode: OtpManager,
     private readonly config: RecoveryPhoneConfig,
+    private readonly twilioConfig: TwilioConfig,
     @Inject(StatsDService) private readonly metrics: StatsD,
     @Inject(LOGGER_PROVIDER) private readonly log?: LoggerService
   ) {}
@@ -374,6 +377,35 @@ export class RecoveryPhoneService {
     return this.isSuccessfulSmsSend(msg);
   }
 
+  /**
+   * Handles update about message delivery status.
+   * @param messageStatus The message delivery status.
+   */
+  public async onMessageStatusUpdate(messageStatus: TwilioMessageStatus) {
+    await this.smsManager.messageStatus(messageStatus);
+  }
+
+  /**
+   * Produces the signature used to sign requests sent to twilio webhooks
+   * @param params
+   * @returns
+   */
+  public validateTwilioSignature(
+    twilioSignature: string,
+    params: Record<string, any>
+  ) {
+    if (this.twilioConfig.validateWebhookCalls === false) {
+      return true;
+    }
+
+    return validateRequest(
+      this.twilioConfig.authToken,
+      twilioSignature,
+      this.twilioConfig.webhookUrl,
+      params
+    );
+  }
+
   private isSuccessfulSmsSend(msg: MessageInstance) {
     if (
       msg == null ||

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

@@ -14,6 +14,7 @@ describe('SmsManager', () => {
   const to = '+15005551111';
   const from = ['+15005550000', '+15005550001'];
   const mockTwilioSmsClient = {
+    accountSid: 'AC123',
     messages: {
       create: jest.fn(),
     },
@@ -29,6 +30,7 @@ describe('SmsManager', () => {
   };
   const mockLog = {
     log: jest.fn(),
+    warn: jest.fn(),
   };
   const mockConfig = {
     from,
@@ -251,4 +253,92 @@ describe('SmsManager', () => {
       );
     });
   });
+
+  describe('message status updates', () => {
+    it('records message status update for delivered', () => {
+      manager.messageStatus({
+        AccountSid: 'AC123',
+        MessageSid: 'M123',
+        From: '+1234567890',
+        MessageStatus: 'delivered',
+        RawDlrDoneDate: 'TWILIO_DATE_FORMAT',
+      });
+      expect(mockMetrics.increment).toBeCalledTimes(1);
+      expect(mockMetrics.increment).toBeCalledWith(
+        'recovery-phone.message.status.delivered'
+      );
+      expect(mockLog.log).toBeCalledWith(
+        'recovery-phone.message.status.delivered',
+        {
+          From: '+1234567890',
+          MessageSid: 'M123',
+          RawDlrDoneDate: 'TWILIO_DATE_FORMAT',
+        }
+      );
+    });
+
+    it('records message status update for failed', () => {
+      manager.messageStatus({
+        AccountSid: 'AC123',
+        MessageSid: 'M123',
+        From: '+1234567890',
+        MessageStatus: 'failed',
+        ErrorCode: '4000',
+      });
+      expect(mockMetrics.increment).toBeCalledTimes(2);
+      expect(mockMetrics.increment).toBeCalledWith(
+        'recovery-phone.message.status.failed'
+      );
+      expect(mockMetrics.increment).toBeCalledWith(
+        'recovery-phone.message.status.error.4000'
+      );
+      expect(mockLog.log).toBeCalledWith(
+        'recovery-phone.message.status.failed',
+        {
+          From: '+1234567890',
+          MessageSid: 'M123',
+          ErrorCode: '4000',
+        }
+      );
+    });
+
+    it('records message status update for undelivered', () => {
+      manager.messageStatus({
+        AccountSid: 'AC123',
+        MessageSid: 'M123',
+        From: '+1234567890',
+        MessageStatus: 'undelivered',
+        ErrorCode: '4000',
+      });
+      expect(mockMetrics.increment).toBeCalledTimes(2);
+      expect(mockMetrics.increment).toBeCalledWith(
+        'recovery-phone.message.status.undelivered'
+      );
+      expect(mockMetrics.increment).toBeCalledWith(
+        'recovery-phone.message.status.error.4000'
+      );
+      expect(mockLog.log).toBeCalledWith(
+        'recovery-phone.message.status.undelivered',
+        {
+          From: '+1234567890',
+          MessageSid: 'M123',
+          ErrorCode: '4000',
+        }
+      );
+    });
+
+    it('records message status update for unimportant status', () => {
+      manager.messageStatus({
+        AccountSid: 'AC123',
+        MessageSid: 'M123',
+        From: '+1234567890',
+        MessageStatus: 'sending',
+      });
+      expect(mockMetrics.increment).toBeCalledTimes(1);
+      expect(mockMetrics.increment).toBeCalledWith(
+        'recovery-phone.message.status.sending'
+      );
+      expect(mockLog.log).toBeCalledTimes(0);
+    });
+  });
 });

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

@@ -7,7 +7,10 @@ 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 {
+  MessageInstance,
+  MessageStatus,
+} from 'twilio/lib/rest/api/v2010/account/message';
 import { SmsManagerConfig } from './sms.manager.config';
 import { TwilioProvider } from './twilio.provider';
 import {
@@ -17,6 +20,17 @@ import {
   TwilioErrorCodes,
 } from './recovery-phone.errors';
 
+export type TwilioMessageStatus = {
+  AccountSid: string;
+  From: string;
+  MessageSid: string;
+  MessageStatus: MessageStatus;
+  // Only present if status is failed or undelivered
+  ErrorCode?: string;
+  // Probably present if status is delivered or undelivered
+  RawDlrDoneDate?: string;
+};
+
 @Injectable()
 export class SmsManager {
   private currentPhoneNumber = 0;
@@ -149,4 +163,53 @@ export class SmsManager {
       this.currentPhoneNumber++ % this.config.from.length
     ];
   }
+
+  public messageStatus(messageStatus: TwilioMessageStatus) {
+    // Keep tabs on the delivery status
+    this.metrics.increment(
+      `recovery-phone.message.status.${messageStatus.MessageStatus}`
+    );
+
+    // Track specific error codes rates
+    if (messageStatus.ErrorCode) {
+      this.metrics.increment(
+        `recovery-phone.message.status.error.${messageStatus.ErrorCode}`
+      );
+    }
+
+    // Only write log entries for certain statuses.
+    switch (messageStatus.MessageStatus) {
+      case 'queued':
+      case 'sending':
+      case 'sent':
+      case 'receiving':
+      case 'received':
+      case 'accepted':
+      case 'scheduled':
+      case 'read':
+      case 'partially_delivered':
+      case 'canceled':
+        // no-op
+        break;
+
+      case 'failed':
+      case 'undelivered':
+      case 'delivered':
+        const opts: any = {
+          From: messageStatus.From,
+          MessageSid: messageStatus.MessageSid,
+        };
+        if (messageStatus.ErrorCode) {
+          opts.ErrorCode = messageStatus.ErrorCode;
+        }
+        if (messageStatus.RawDlrDoneDate) {
+          opts.RawDlrDoneDate = messageStatus.RawDlrDoneDate;
+        }
+        this.log.log(
+          `recovery-phone.message.status.${messageStatus.MessageStatus}`,
+          opts
+        );
+        break;
+    }
+  }
 }