Modify db-uniqueness constraint for ongoingDeliveryId

packages/attestation-service/migrations/20211109200901-attestation-v8.js

@@ -0,0 +1,68 @@
+'use strict'
+
+module.exports = {
+  up: async (queryInterface, Sequelize) => {
+    const transaction = await queryInterface.sequelize.transaction()
+
+    try {
+      await queryInterface.removeIndex(
+        'Attestations',
+        ['ongoingDeliveryId'],
+        { fields: ['ongoingDeliveryId'], unique: true },
+        { transaction }
+      )
+      await queryInterface.addIndex(
+        'Attestations',
+        ['ongoingDeliveryId'],
+        { fields: ['ongoingDeliveryId'], unique: false },
+        { transaction }
+      )
+      await transaction.commit()
+    } catch (error) {
+      await transaction.rollback()
+      throw error
+    }
+  },
+  down: async (queryInterface, Sequelize) => {
+    const transaction = await queryInterface.sequelize.transaction()
+    try {
+      await queryInterface.removeIndex(
+        'Attestations',
+        ['ongoingDeliveryId'],
+        { fields: ['ongoingDeliveryId'], unique: false },
+        { transaction }
+      )
+      await queryInterface.addIndex(
+        'Attestations',
+        ['ongoingDeliveryId'],
+        { fields: ['ongoingDeliveryId'], unique: true },
+        { transaction }
+      )
+    } catch (error) {
+      await transaction.rollback()
+      if (
+        error.errors.length == 1 &&
+        error.errors[0].path == 'ongoingDeliveryId' &&
+        error.errors[0].type == 'unique violation'
+      ) {
+        try {
+          console.warn('Duplicates in ongoingDeliveryId; nulling out column and retrying.')
+          await queryInterface.removeColumn('Attestations', 'ongoingDeliveryId')
+          await queryInterface.addColumn('Attestations', 'ongoingDeliveryId', {
+            type: Sequelize.STRING,
+            allowNull: true,
+          })
+          await queryInterface.addIndex(
+            'Attestations',
+            ['ongoingDeliveryId'],
+            { fields: ['ongoingDeliveryId'], unique: true },
+            { transaction }
+          )
+        } catch (error) {
+          await transaction.rollback()
+          throw error
+        }
+      }
+    }
+  },
+}

packages/attestation-service/src/db.ts

@@ -320,10 +320,15 @@ export async function findAttestationByDeliveryId(
   ongoingDeliveryId: string,
   options: FindOptions = {}
 ): Promise<AttestationModel | null> {
-  return (await getAttestationTable()).findOne({
+  // For testing only, delete
+  const val = (await getAttestationTable()).findOne({
     where: { ongoingDeliveryId },
+    // Ensure deterministic result, since deliveryId uniqueness is not enforced
+    order: ['createdAt', 'desc'],
     ...options,
   })
+  rootLogger.warn(`In findAttestationByDeliveryId, deliveryId: ${ongoingDeliveryId}, val: ${val} `)
+  return val
 }
 
 export async function findOrCreateAttestation(

packages/attestation-service/src/models/attestation.ts

@@ -15,14 +15,14 @@ export interface SmsFields {
   attestationCode: string | null
   appSignature: string | undefined
   language: string | undefined
-  attempt: number
 }
 
 export interface AttestationModel extends Model, SmsFields {
   readonly id: number
   securityCodeAttempt: number
   ongoingDeliveryId: string | null
   providers: string
+  attempt: number
   status: AttestationStatus
   errors: string | null
   createdAt: Date

packages/attestation-service/src/sms/twilioVerify.ts

@@ -109,38 +109,14 @@ export class TwilioVerifyProvider extends TwilioSmsProvider {
       }
     }
 
-    // If there's a previous verification
-    // if (attestation.attempt) {
-    // console.log('hellooooo')
-    // const y = new Date()
-    // try {
-    //   // const x = await this.client.verify
-    //   await this.client.verify
-    //     .services(this.verifyServiceSid)
-    //     .verifications(attestation.phoneNumber)
-    //     .update({ status: 'canceled' })
-    //   // console.log(x)
-    // } catch (e) {
-    //   // let errorMsg = 'no error message'
-    //   // if (e instanceof Error) {
-    //   //   errorMsg = e.message
-    //   // }
-    //   // throw new Error(`Unable to cancel previous verification: ${errorMsg}`)
-    // }
-    // const z = new Date()
-    // console.log('total time: ', z.getTime() - y.getTime())
-    // // }
-
-    let deliveryId: string
-
     // Note: SID returned by Verify API is not unique for a phone number (within a 10 min interval)
     // i.e. re-requests to the same phone number in <10 min will return an exisiting SID
 
     try {
       const m = await this.client.verify
         .services(this.verifyServiceSid)
         .verifications.create(requestParams)
-      deliveryId = m.sid
+      return m.sid
     } catch (e) {
       // Verify landlines using voice
       if (
@@ -152,23 +128,10 @@ export class TwilioVerifyProvider extends TwilioSmsProvider {
         const m = await this.client.verify
           .services(this.verifyServiceSid)
           .verifications.create(requestParams)
-        deliveryId = m.sid
+        return m.sid
       } else {
         throw e
       }
     }
-    try {
-      // Change status of verification to ensure unique delivery IDs are created
-      await this.client.verify
-        .services(this.verifyServiceSid)
-        .verifications(deliveryId)
-        .update({ status: 'canceled' })
-    } catch {
-      // This shouldn't throw a hard error though as this is to prevent a tiny edge case:
-      // 2 Verify requests for the same issuer + phone number in <10 min.
-      // At this point, the text has already been sent.
-    } finally {
-      return deliveryId
-    }
   }
 }

packages/attestation-service/test/sms/twilio.test.ts

@@ -1,6 +1,6 @@
 import { SmsFields } from '../../src/models/attestation'
 import { TwilioMessagingProvider, TwilioVerifyProvider } from '../../src/sms/twilio'
-import { mockMessagesCreate, mockVerifyCreate, mockVerifyUpdate } from '../__mocks__/twilio'
+import { mockMessagesCreate, mockVerifyCreate } from '../__mocks__/twilio'
 
 jest.mock('../__mocks__/twilio')
 
@@ -25,48 +25,30 @@ describe('TwilioSmsProvider tests', () => {
       attestationCode: '56789',
       appSignature: undefined,
       language: 'en',
-      attempt: 0,
     }
   })
-  describe('TwilioVerifyProvider tests', () => {
-    it('should initialize and send SMS', async () => {
-      const twilioVerifyProvider = new TwilioVerifyProvider(
-        twilioSid,
-        twilioAuthToken,
-        unsupportedRegionCodes,
-        verifyServiceSid
-      )
-      await twilioVerifyProvider.initialize('fake-delivery-status-url')
-      await twilioVerifyProvider.sendSms(attestation)
-      expect(mockVerifyCreate).toBeCalledTimes(1)
-      expect(mockMessagesCreate).not.toBeCalled()
-    })
-    it('should create new SID on second attempt', async () => {
-      const twilioVerifyProvider = new TwilioVerifyProvider(
-        twilioSid,
-        twilioAuthToken,
-        unsupportedRegionCodes,
-        verifyServiceSid
-      )
-      await twilioVerifyProvider.initialize('fake-delivery-status-url')
-      await twilioVerifyProvider.sendSms(attestation)
-      expect(mockVerifyCreate).toBeCalledTimes(1)
-      expect(mockVerifyUpdate).toBeCalledTimes(1)
-      expect(mockMessagesCreate).not.toBeCalled()
-    })
+  it('should initialize and send SMS via TwilioVerifyProvider', async () => {
+    const twilioVerifyProvider = new TwilioVerifyProvider(
+      twilioSid,
+      twilioAuthToken,
+      unsupportedRegionCodes,
+      verifyServiceSid
+    )
+    await twilioVerifyProvider.initialize('fake-delivery-status-url')
+    await twilioVerifyProvider.sendSms(attestation)
+    expect(mockVerifyCreate).toBeCalledTimes(1)
+    expect(mockMessagesCreate).not.toBeCalled()
   })
-  describe('TwilioMessagingProvider tests', () => {
-    it('should initialize and send SMS', async () => {
-      const twilioMessagingProvider = new TwilioMessagingProvider(
-        twilioSid,
-        twilioAuthToken,
-        unsupportedRegionCodes,
-        messagingServiceSid
-      )
-      await twilioMessagingProvider.initialize('fake-delivery-status-url')
-      await twilioMessagingProvider.sendSms(attestation)
-      expect(mockMessagesCreate).toBeCalledTimes(1)
-      expect(mockVerifyCreate).not.toBeCalled()
-    })
+  it('should initialize and send SMS via TwilioMessagingProvider', async () => {
+    const twilioMessagingProvider = new TwilioMessagingProvider(
+      twilioSid,
+      twilioAuthToken,
+      unsupportedRegionCodes,
+      messagingServiceSid
+    )
+    await twilioMessagingProvider.initialize('fake-delivery-status-url')
+    await twilioMessagingProvider.sendSms(attestation)
+    expect(mockMessagesCreate).toBeCalledTimes(1)
+    expect(mockVerifyCreate).not.toBeCalled()
   })
 })