Skip to content

Commit

Permalink
Refactor TwilioSmsProvider into two separate modules (messaging, veri…
Browse files Browse the repository at this point in the history
…fy) (#8871)

## Description

Closes #8755

Builds on the earlier [configurable verify PR](#8783).

Note: the mocking is SUPER ugly and feels almost useless here; very open to ideas on structuring the tests better, but I think this is another arg in favor of refactoring the SMS module cc @emilhotkowski 😅

## Changes
- Splits `TwilioSmsProvider` into two providers: `TwilioVerifyProvider` and `TwilioMessagingProvider`; providers can now be configured by country `SMS_PROVIDERS_X=twiliomessaging,twilioverify,...`, and Verify/Messaging will be by default randomized (instead of preferring Verify as previously). (I'm really not attached to the env var names, if someone has a better idea that's less clunky, I'm all 👂's!)
- `twilio` provider in env variable setting is now syntactic sugar for both `twilioverify` and `twiliomessaging`, to keep backwards compatibility of env vars
- Adds unit tests for the `sendSms` function (where the above changes are located)
  - Mocking of the twilio module to make this test possible ^
- Adds a test script to really test out the `sendSms` function (i.e. using the real twilio module), usable by adding the `TEST_SMS_RECIPIENT` parameter in `.env.development` (and making sure the necessary twilio SID + AUTH vars are also set there, just as needed for running the AS locally)
- Splits out `SmsFields` from `AttestationModel` interface to make it possible to write the above two types of tests. It isn't a super clean break right now (makes sense to revisit with the full refactor of the SMS module), but it roughly separates into: `SmsFields` = fields required to produce an actual SMS, `AttestationModel` = metadata regarding production of the SMS (i.e. provider, attempt) + underlying data model

Relevant docs PR: celo-org/docs#211

## Testing
- [x] used the two test scripts
- [x] tested this locally
- [x] deployed to Alfajores
- [x] tested verification flows on Alfajores -- able to switch on/off for DE (my phone number), other flows work as expected
  • Loading branch information
eelanagaraj authored Nov 5, 2021
1 parent 26b63eb commit 7d47b0b
Show file tree
Hide file tree
Showing 16 changed files with 481 additions and 179 deletions.
2 changes: 1 addition & 1 deletion .env.alfajores
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ ATTESTATION_SERVICE_DOCKER_IMAGE_TAG="attestation-service-alfajores"

CELO_PROVIDER_BACKUPS=https://alfajores-forno.celo-testnet.org
TELEKOM_FROM="+14157389085"
SMS_PROVIDERS=twilio,nexmo,telekom
SMS_PROVIDERS=twilioverify,twiliomessaging,nexmo,telekom
SMS_PROVIDERS_RANDOMIZED="1"

NEXMO_APPLICATIONS=0e8afe80-29f8-4e88-af03-5b5066161944,775fb58b-331b-4040-b1eb-7aed298b5685,07bd882c-5565-4387-a9e8-48fa2de3b3cc,93a3355e-ebed-4eeb-bb93-61f0c752add9,7a194b22-6f3c-488a-b174-933f7e875875,06a4573b-406d-48f6-97db-ef5652ce98c4,90b0915b-eb5f-4d27-942b-7d90d137ec1f,9c572771-f37d-4b02-b783-69c7934fe5fb,d93e193d-367f-4315-95a5-6dfb108288e5,adfeb89a-7dfb-49b8-9e3e-2a1284719986
Expand Down
2 changes: 1 addition & 1 deletion .env.baklava
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ ATTESTATION_SERVICE_DOCKER_IMAGE_REPOSITORY="us.gcr.io/celo-testnet/celo-monorep
ATTESTATION_SERVICE_DOCKER_IMAGE_TAG="attestation-service-baklava"

CELO_PROVIDER_BACKUPS=https://baklava-forno.celo-testnet.org
SMS_PROVIDERS=twilio,nexmo
SMS_PROVIDERS=twilioverify,twiliomessaging,nexmo
SMS_PROVIDERS_RANDOMIZED="1"

# We only configured 15 nexmo applications. So delivery receipts from the second 15 will always go to the wrong place for now
Expand Down
15 changes: 13 additions & 2 deletions packages/attestation-service/config/.env.development
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,16 @@ SMS_PROVIDERS=twilio

# Optional: set a different list or order of providers for a specific country.
# (They are tried from first to last, regardless of SMS_PROVIDERS_RANDOMIZED)
# SMS_PROVIDERS_US=twilio,nexmo
# SMS_PROVIDERS_MX=nexmo
# We recommend the following country-specific settings (1.5.0+):
SMS_PROVIDERS_CN=twiliomessaging
SMS_PROVIDERS_VN=messagebird,twilioverify
SMS_PROVIDERS_TR=twilioverify
SMS_PROVIDERS_BR=messagebird,twilioverify,twiliomessaging
SMS_PROVIDERS_IN=messagebird,twilioverify,twiliomessaging
SMS_PROVIDERS_VE=messagebird,twilioverify,twiliomessaging
SMS_PROVIDERS_GH=messagebird,twilioverify,twiliomessaging
SMS_PROVIDERS_PH=messagebird,twilioverify,twiliomessaging,nexmo
SMS_PROVIDERS_DE=messagebird,twilioverify,twiliomessaging

# Service requests on this port
PORT=3000
Expand Down Expand Up @@ -78,3 +86,6 @@ LOG_FORMAT= human
LOG_LEVEL=info

APP_SIGNATURE=x

# Phone number (prefixed with "+") for test-send-sms-twilio script
# TEST_SMS_RECIPIENT=x
5 changes: 3 additions & 2 deletions packages/attestation-service/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@celo/attestation-service",
"version": "1.4.0",
"version": "1.5.0-dev",
"description": "Issues attestation messages for Celo's identity protocol",
"main": "./lib/index.js",
"types": "./lib/index.d.ts",
Expand All @@ -25,7 +25,8 @@
"db:migrate:undo": "sequelize db:migrate:undo",
"db:migrate:dev": "sequelize db:migrate",
"dev": "NODE_ENV=dev CONFIG=config/.env.development nodemon",
"lint": "tslint -c tslint.json --project ."
"lint": "tslint -c tslint.json --project .",
"test-send-sms-twilio": "TS_NODE_FILES=true CONFIG=config/.env.development ts-node scripts/test-send-sms-twilio.ts"
},
"dependencies": {
"@celo/contractkit": "1.3.1-dev",
Expand Down
104 changes: 104 additions & 0 deletions packages/attestation-service/scripts/test-send-sms-twilio.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/**
* Script for testing TwilioSmsProvider.sendSms
* (Verify & Messaging Services) using the real twilio API.
* Uses `.env.development` file for sensitive info; set your phone number
* as TEST_SMS_RECIPIENT to receive the messages and check against
* expected output (logged in console).
* Comment out cases under `testCases` as desired.
*/

import { fetchEnv, fetchEnvOrDefault } from '../src/env'
import { SmsFields } from '../src/models/attestation'
import { readUnsupportedRegionsFromEnv } from '../src/sms/base'
import { TwilioSmsProvider } from '../src/sms/twilio'
import { TwilioMessagingProvider } from '../src/sms/twilioMessaging'
import { TwilioVerifyProvider } from '../src/sms/twilioVerify'
;(async function main() {
const twilioSid = fetchEnv('TWILIO_ACCOUNT_SID')
const messagingServiceSid = fetchEnv('TWILIO_MESSAGING_SERVICE_SID')
const verifyServiceSid = fetchEnvOrDefault('TWILIO_VERIFY_SERVICE_SID', '')
const twilioAuthToken = fetchEnv('TWILIO_AUTH_TOKEN')
const unsupportedRegionCodes = readUnsupportedRegionsFromEnv(
'TWILIO_UNSUPPORTED_REGIONS',
'TWILIO_BLACKLIST'
)
const testPhoneNumber = fetchEnv('TEST_SMS_RECIPIENT')
const countryCode = 'DE'

enum SendMethod {
MESSAGE_SERVICE = 'message service',
VERIFY = 'verify API',
}

type TestCase = {
id: string
expectedSendMethod: SendMethod
}

const testCases: TestCase[] = [
{
id: '000000',
expectedSendMethod: SendMethod.MESSAGE_SERVICE,
},
{
id: '111111',
expectedSendMethod: SendMethod.VERIFY,
},
]

testCases.map(async (testCase: TestCase) => {
const attestation: SmsFields = {
account: '0x123',
identifier: '0x456',
issuer: '0x789',
countryCode: countryCode,
phoneNumber: testPhoneNumber,
message: '',
securityCode: '',
attestationCode: '123',
appSignature: undefined,
language: 'en',
}

let twilioSmsProvider: TwilioSmsProvider
let expectedMsg: string
let expectedSidStart: string

switch (testCase.expectedSendMethod) {
case SendMethod.MESSAGE_SERVICE:
twilioSmsProvider = new TwilioMessagingProvider(
twilioSid,
twilioAuthToken,
unsupportedRegionCodes,
messagingServiceSid
)
expectedMsg = attestation.message = 'via-message:' + testCase.id
expectedSidStart = 'SM'
break
case SendMethod.VERIFY:
twilioSmsProvider = new TwilioVerifyProvider(
twilioSid,
twilioAuthToken,
unsupportedRegionCodes,
verifyServiceSid
)
expectedMsg = 'Your Celo verification code is: ' + testCase.id
attestation.securityCode = testCase.id
expectedSidStart = 'VE'
break
}

try {
await twilioSmsProvider.initialize()
const messageSID = await twilioSmsProvider.sendSms(attestation)
const messageSIDStart = messageSID.substring(0, 2)
console.log(`Message SID for id ${testCase.id}: ${messageSID}`)
console.log(`SMS should match: ${expectedMsg}`)
if (messageSIDStart !== expectedSidStart) {
throw new Error(`Returned message SID did not match expected starting letters`)
}
} catch (e) {
console.error(e)
}
})
})()
16 changes: 11 additions & 5 deletions packages/attestation-service/src/models/attestation.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import { E164Number } from '@celo/utils/lib/io'
import { BuildOptions, DataTypes, Model, Sequelize } from 'sequelize'

export interface AttestationModel extends Model {
readonly id: number
// Split out SmsFields from the underlying data model;
// Contains fields relevant to the message itself, not to meta-info
// about how/when the message was sent.
export interface SmsFields {
account: string
identifier: string
issuer: string
countryCode: string
phoneNumber: E164Number
message: string
securityCode: string | null
securityCodeAttempt: number
attestationCode: string | null
appSignature: string | undefined
language: string | undefined
}

export interface AttestationModel extends Model, SmsFields {
readonly id: number
securityCodeAttempt: number
ongoingDeliveryId: string | null
providers: string
attempt: number
Expand All @@ -24,8 +32,6 @@ export interface AttestationModel extends Model {
recordError: (error: string) => void
failure: () => boolean
currentError: () => string | undefined
appSignature: string | undefined
language: string | undefined
}

export interface AttestationKey {
Expand Down
6 changes: 4 additions & 2 deletions packages/attestation-service/src/sms/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { E164Number } from '@celo/utils/lib/io'
import Logger from 'bunyan'
import express from 'express'
import { fetchEnvOrDefault } from '../env'
import { AttestationModel } from '../models/attestation'
import { SmsFields } from '../models/attestation'

export abstract class SmsProvider {
abstract type: SmsProviderType
Expand All @@ -12,7 +12,7 @@ export abstract class SmsProvider {
return !this.unsupportedRegionCodes.includes(countryCode.toUpperCase())
}
// Should throw Error when unsuccesful, return if successful
abstract sendSms(attestation: AttestationModel): Promise<string>
abstract sendSms(attestation: SmsFields): Promise<string>

// if this provider supports delivery status updates to an endpoint delivery_<providername>/, should return 'GET' or 'POST'
abstract deliveryStatusMethod(): string | null
Expand All @@ -27,6 +27,8 @@ export enum SmsProviderType {
NEXMO = 'nexmo',
UNKNOWN = 'unknown',
TWILIO = 'twilio',
TWILIO_MESSAGING = 'twiliomessaging',
TWILIO_VERIFY = 'twilioverify',
MESSAGEBIRD = 'messagebird',
TELEKOM = 'telekom',
}
Expand Down
53 changes: 36 additions & 17 deletions packages/attestation-service/src/sms/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { obfuscateNumber, SmsProvider, SmsProviderType } from './base'
import { MessageBirdSmsProvider } from './messagebird'
import { NexmoSmsProvider } from './nexmo'
import { TelekomSmsProvider } from './telekom'
import { TwilioSmsProvider } from './twilio'
import { TwilioMessagingProvider, TwilioVerifyProvider } from './twilio'

// Maximum delivery attempts (including first) regardless of provider
const maxDeliveryAttempts = parseInt(
Expand Down Expand Up @@ -69,13 +69,23 @@ function phoneNumberTypeToString(t: PhoneNumberType): string {
}
}

function providerNamesToList(providers: string) {
return (
providers
// Backwards compatibility: 'twilio' as syntactic sugar for 'twilioverify,twiliomessaging'
.replace(
new RegExp(`\\b(${SmsProviderType.TWILIO})\\b`, 'g'),
`${SmsProviderType.TWILIO_VERIFY},${SmsProviderType.TWILIO_MESSAGING}`
)
.split(',')
.filter((t) => t != null && t !== '')
)
}

export async function initializeSmsProviders(
deliveryStatusURLForProviderType: (type: string) => string
) {
const smsProvidersToConfigure = fetchEnv('SMS_PROVIDERS')
.split(',')
.filter((t) => t != null && t !== '') as Array<SmsProviderType | string>

const smsProvidersToConfigure = providerNamesToList(fetchEnv('SMS_PROVIDERS'))
if (smsProvidersToConfigure.length === 0) {
throw new Error('You have to specify at least one sms provider')
}
Expand Down Expand Up @@ -105,11 +115,21 @@ export async function initializeSmsProviders(
smsProviders.push(nexmoProvider)
smsProvidersByType[SmsProviderType.NEXMO] = nexmoProvider
break
case SmsProviderType.TWILIO:
const twilioProvider = TwilioSmsProvider.fromEnv()
await twilioProvider.initialize(deliveryStatusURLForProviderType(configuredSmsProvider))
smsProviders.push(twilioProvider)
smsProvidersByType[SmsProviderType.TWILIO] = twilioProvider
case SmsProviderType.TWILIO_VERIFY:
const twilioVerifyProvider = TwilioVerifyProvider.fromEnv()
await twilioVerifyProvider.initialize(
deliveryStatusURLForProviderType(configuredSmsProvider)
)
smsProviders.push(twilioVerifyProvider)
smsProvidersByType[SmsProviderType.TWILIO_VERIFY] = twilioVerifyProvider
break
case SmsProviderType.TWILIO_MESSAGING:
const twilioMessagingProvider = TwilioMessagingProvider.fromEnv()
await twilioMessagingProvider.initialize(
deliveryStatusURLForProviderType(configuredSmsProvider)
)
smsProviders.push(twilioMessagingProvider)
smsProvidersByType[SmsProviderType.TWILIO_MESSAGING] = twilioMessagingProvider
break
default:
throw new Error(`Unknown sms provider type specified: ${configuredSmsProvider}`)
Expand All @@ -124,9 +144,9 @@ function smsProvidersFor(
phoneNumber: E164Number,
logger: Logger
): SmsProvider[] {
const providersForRegion = fetchEnvOrDefault(`SMS_PROVIDERS_${countryCode}`, '')
.split(',')
.filter((t) => t != null && t !== '')
const providersForRegion = providerNamesToList(
fetchEnvOrDefault(`SMS_PROVIDERS_${countryCode}`, '')
)
let providers =
providersForRegion.length > 0
? providersForRegion.map((name) => smsProvidersByType[name])
Expand Down Expand Up @@ -184,10 +204,9 @@ function providersToCsv(providers: SmsProvider[]) {
// but cannot require providers not configured or unsupported for this country code.
function getProvidersFor(attestation: AttestationModel, logger: Logger) {
const validProviders = smsProvidersFor(attestation.countryCode, attestation.phoneNumber, logger)
const attestationProviders = attestation.providers
.split(',')
.filter((t) => t != null && t !== '')
.map((name) => smsProvidersByType[name])
const attestationProviders = providerNamesToList(attestation.providers).map(
(name) => smsProvidersByType[name]
)

for (const p of attestationProviders) {
if (!validProviders.find((v) => p.type === v.type)) {
Expand Down
4 changes: 2 additions & 2 deletions packages/attestation-service/src/sms/messagebird.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import initMB, { MessageBird } from 'messagebird'
import fetch from 'node-fetch'
import util from 'util'
import { fetchEnv } from '../env'
import { AttestationModel, AttestationStatus } from '../models/attestation'
import { AttestationStatus, SmsFields } from '../models/attestation'
import { readUnsupportedRegionsFromEnv, SmsProvider, SmsProviderType } from './base'
import { receivedDeliveryReport } from './index'

Expand Down Expand Up @@ -97,7 +97,7 @@ export class MessageBirdSmsProvider extends SmsProvider {
}
}

async sendSms(attestation: AttestationModel) {
async sendSms(attestation: SmsFields) {
const reference = randomBytes(8).toString('hex')
const m = await util.promisify(this.messagebird.messages.create)({
originator: 'Celo',
Expand Down
4 changes: 2 additions & 2 deletions packages/attestation-service/src/sms/nexmo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Nexmo from 'nexmo'
import { receivedDeliveryReport } from '.'
import { fetchEnv, fetchEnvOrDefault, isYes } from '../env'
import { Gauges } from '../metrics'
import { AttestationModel, AttestationStatus } from '../models/attestation'
import { AttestationStatus, SmsFields } from '../models/attestation'
import { readUnsupportedRegionsFromEnv, SmsProvider, SmsProviderType } from './base'

const phoneUtil = PhoneNumberUtil.getInstance()
Expand Down Expand Up @@ -106,7 +106,7 @@ export class NexmoSmsProvider extends SmsProvider {

deliveryStatusHandlers = () => [bodyParser.json()]

async sendSms(attestation: AttestationModel) {
async sendSms(attestation: SmsFields) {
const nexmoNumber = this.getMatchingNumber(attestation.countryCode)
return new Promise<string>((resolve, reject) => {
this.client.message.sendSms(
Expand Down
4 changes: 2 additions & 2 deletions packages/attestation-service/src/sms/telekom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import express from 'express'
import fetch from 'node-fetch'
import { receivedDeliveryReport } from '.'
import { fetchEnv, fetchEnvOrDefault } from '../env'
import { AttestationModel, AttestationStatus } from '../models/attestation'
import { AttestationStatus, SmsFields } from '../models/attestation'
import { readUnsupportedRegionsFromEnv, SmsProvider, SmsProviderType } from './base'

export class TelekomSmsProvider extends SmsProvider {
Expand Down Expand Up @@ -71,7 +71,7 @@ export class TelekomSmsProvider extends SmsProvider {
this.deliveryStatusURL = deliveryStatusURL
}

async sendSms(attestation: AttestationModel) {
async sendSms(attestation: SmsFields) {
const response = await fetch(this.serviceURL, {
method: 'POST',
headers: {
Expand Down
Loading

0 comments on commit 7d47b0b

Please sign in to comment.