From 049377448949f35332166129edd163f29dc398f4 Mon Sep 17 00:00:00 2001 From: tshuli <63710093+tshuli@users.noreply.github.com> Date: Tue, 7 Jun 2022 09:08:26 +0800 Subject: [PATCH] feat: log ip address on callback from twilio (#3937) * feat: log mobile number when otp generated * Revert "feat: log mobile number when otp generated" This reverts commit 58b4bc54c99199e0f9237dacc86a0b8dfc85e39c. * feat: pass senderIp as query param to twilio status callback * chore: update tests * chore: use node url api * chore: pass in full url to URL api * chore: add try catch block, fix tests --- .../__tests__/twilio.controller.spec.ts | 16 +++++++++++++ src/app/modules/twilio/twilio.controller.ts | 20 ++++++++++++++++ .../user/__tests__/user.controller.spec.ts | 1 + src/app/modules/user/user.controller.ts | 5 +++- .../__tests__/verification.controller.spec.ts | 22 +++++++++++++++++ .../__tests__/verification.service.spec.ts | 12 ++++++++++ .../__tests__/verification.test.helpers.ts | 1 + .../verification/verification.controller.ts | 8 ++++++- .../verification/verification.service.ts | 13 ++++++++-- .../verification/verification.types.ts | 1 + .../sms/__tests__/sms.factory.spec.ts | 2 ++ .../sms/__tests__/sms.service.spec.ts | 24 +++++++++++++++++++ src/app/services/sms/sms.factory.ts | 10 ++++---- src/app/services/sms/sms.service.ts | 16 +++++++++++-- 14 files changed, 141 insertions(+), 10 deletions(-) diff --git a/src/app/modules/twilio/__tests__/twilio.controller.spec.ts b/src/app/modules/twilio/__tests__/twilio.controller.spec.ts index 240c6d7f03..27291c1ac8 100644 --- a/src/app/modules/twilio/__tests__/twilio.controller.spec.ts +++ b/src/app/modules/twilio/__tests__/twilio.controller.spec.ts @@ -48,6 +48,13 @@ describe('twilio.controller', () => { it('should return 200 when successfully delivered message is sent', async () => { const mockReq = expressHandler.mockRequest({ body: MOCK_SUCCESSFUL_MESSAGE, + others: { + protocol: 'https', + host: 'webhook-endpoint.gov.sg', + url: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`, + originalUrl: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`, + get: () => 'webhook-endpoint.gov.sg', + }, }) const mockRes = expressHandler.mockResponse() await twilioSmsUpdates(mockReq, mockRes, jest.fn()) @@ -57,6 +64,7 @@ describe('twilio.controller', () => { meta: { action: 'twilioSmsUpdates', body: MOCK_SUCCESSFUL_MESSAGE, + senderIp: '200.0.0.0', }, }) expect(mockLogger.error).not.toBeCalled() @@ -66,6 +74,13 @@ describe('twilio.controller', () => { it('should return 200 when failed delivered message is sent', async () => { const mockReq = expressHandler.mockRequest({ body: MOCK_FAILED_MESSAGE, + others: { + protocol: 'https', + host: 'webhook-endpoint.gov.sg', + url: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`, + originalUrl: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`, + get: () => 'webhook-endpoint.gov.sg', + }, }) const mockRes = expressHandler.mockResponse() await twilioSmsUpdates(mockReq, mockRes, jest.fn()) @@ -76,6 +91,7 @@ describe('twilio.controller', () => { meta: { action: 'twilioSmsUpdates', body: MOCK_FAILED_MESSAGE, + senderIp: '200.0.0.0', }, }) expect(mockRes.sendStatus).toBeCalledWith(200) diff --git a/src/app/modules/twilio/twilio.controller.ts b/src/app/modules/twilio/twilio.controller.ts index a727dd70d9..de5bd23340 100644 --- a/src/app/modules/twilio/twilio.controller.ts +++ b/src/app/modules/twilio/twilio.controller.ts @@ -54,6 +54,24 @@ export const twilioSmsUpdates: ControllerHandler< * Example: https://www.twilio.com/docs/usage/webhooks/sms-webhooks. */ + // Extract public sender's ip address which was passed to twilio as a query param in the status callback + let senderIp = null + try { + const url = new URL( + req.protocol + '://' + req.get('host') + req.originalUrl, + ) + senderIp = url.searchParams.get('senderIp') + } catch { + logger.error({ + message: 'Error occurred when extracting senderIp', + meta: { + action: 'twilioSmsUpdates', + body: req.body, + originalUrl: req.originalUrl, + }, + }) + } + const ddTags: TwilioSmsStatsdTags = { // msgSrvcSid not included to limit tag cardinality (for now?) smsstatus: req.body.SmsStatus, @@ -70,6 +88,7 @@ export const twilioSmsUpdates: ControllerHandler< meta: { action: 'twilioSmsUpdates', body: req.body, + senderIp, }, }) } else { @@ -78,6 +97,7 @@ export const twilioSmsUpdates: ControllerHandler< meta: { action: 'twilioSmsUpdates', body: req.body, + senderIp, }, }) } diff --git a/src/app/modules/user/__tests__/user.controller.spec.ts b/src/app/modules/user/__tests__/user.controller.spec.ts index f4714dc90e..881c02eefc 100644 --- a/src/app/modules/user/__tests__/user.controller.spec.ts +++ b/src/app/modules/user/__tests__/user.controller.spec.ts @@ -63,6 +63,7 @@ describe('user.controller', () => { MOCK_REQ.body.contact, expectedOtp, MOCK_REQ.body.userId, + 'MOCK_IP', ) expect(mockRes.sendStatus).toBeCalledWith(200) }) diff --git a/src/app/modules/user/user.controller.ts b/src/app/modules/user/user.controller.ts index e953b0047f..7f672a4ae3 100644 --- a/src/app/modules/user/user.controller.ts +++ b/src/app/modules/user/user.controller.ts @@ -40,10 +40,12 @@ export const _handleContactSendOtp: ControllerHandler< return res.status(StatusCodes.UNAUTHORIZED).json('User is unauthorized.') } + const senderIp = getRequestIp(req) + const logMeta = { action: 'handleContactSendOtp', userId, - ip: getRequestIp(req), + ip: senderIp, } // Step 1: Create OTP for contact verification. @@ -67,6 +69,7 @@ export const _handleContactSendOtp: ControllerHandler< contact, otp, userId, + senderIp, ) // Error sending OTP. diff --git a/src/app/modules/verification/__tests__/verification.controller.spec.ts b/src/app/modules/verification/__tests__/verification.controller.spec.ts index ede7c25097..611f810b5f 100644 --- a/src/app/modules/verification/__tests__/verification.controller.spec.ts +++ b/src/app/modules/verification/__tests__/verification.controller.spec.ts @@ -391,6 +391,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.sendStatus).toHaveBeenCalledWith(StatusCodes.CREATED) }) @@ -409,6 +410,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -428,6 +430,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -447,6 +450,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -481,6 +485,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith( StatusCodes.UNPROCESSABLE_ENTITY, @@ -502,6 +507,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -521,6 +527,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -540,6 +547,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -559,6 +567,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -578,6 +587,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) }) @@ -597,6 +607,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith( StatusCodes.INTERNAL_SERVER_ERROR, @@ -664,6 +675,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect( MockVerificationService.disableVerifiedFieldsIfRequired, @@ -702,6 +714,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -734,6 +747,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -766,6 +780,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -799,6 +814,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -832,6 +848,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -886,6 +903,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -944,6 +962,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -976,6 +995,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND) expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) @@ -1008,6 +1028,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith( StatusCodes.UNPROCESSABLE_ENTITY, @@ -1067,6 +1088,7 @@ describe('Verification controller', () => { otp: MOCK_OTP, hashedOtp: MOCK_HASHED_OTP, recipient: MOCK_ANSWER, + senderIp: 'MOCK_IP', }) expect(mockRes.status).toHaveBeenCalledWith( StatusCodes.INTERNAL_SERVER_ERROR, diff --git a/src/app/modules/verification/__tests__/verification.service.spec.ts b/src/app/modules/verification/__tests__/verification.service.spec.ts index c88535d0a3..0a7a015218 100644 --- a/src/app/modules/verification/__tests__/verification.service.spec.ts +++ b/src/app/modules/verification/__tests__/verification.service.spec.ts @@ -66,6 +66,7 @@ import { MOCK_HASHED_OTP, MOCK_OTP, MOCK_RECIPIENT, + MOCK_SENDER_IP, MOCK_SIGNED_DATA, } from './verification.test.helpers' @@ -327,6 +328,7 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) // Default mock params has fieldType: 'mobile' @@ -334,6 +336,7 @@ describe('Verification service', () => { MOCK_RECIPIENT, MOCK_OTP, mockTransaction.formId, + MOCK_SENDER_IP, ) expect(MockFormsgSdk.verification.generateSignature).toHaveBeenCalledWith( { @@ -360,6 +363,7 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled() @@ -384,6 +388,7 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled() @@ -403,6 +408,7 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled() @@ -434,6 +440,7 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled() @@ -492,6 +499,7 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) expect(MockMailService.sendVerificationOtp).toHaveBeenCalledWith( @@ -526,12 +534,14 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) expect(MockSmsFactory.sendVerificationOtp).toHaveBeenCalledWith( MOCK_RECIPIENT, MOCK_OTP, new ObjectId(mockFormId), + MOCK_SENDER_IP, ) expect( MockFormsgSdk.verification.generateSignature, @@ -551,6 +561,7 @@ describe('Verification service', () => { hashedOtp: MOCK_HASHED_OTP, otp: MOCK_OTP, recipient: MOCK_RECIPIENT, + senderIp: MOCK_SENDER_IP, }) // Mock params default to mobile @@ -558,6 +569,7 @@ describe('Verification service', () => { MOCK_RECIPIENT, MOCK_OTP, new ObjectId(mockFormId), + MOCK_SENDER_IP, ) expect(MockFormsgSdk.verification.generateSignature).toHaveBeenCalledWith( { diff --git a/src/app/modules/verification/__tests__/verification.test.helpers.ts b/src/app/modules/verification/__tests__/verification.test.helpers.ts index 3533d815b1..a97bba1a34 100644 --- a/src/app/modules/verification/__tests__/verification.test.helpers.ts +++ b/src/app/modules/verification/__tests__/verification.test.helpers.ts @@ -7,6 +7,7 @@ export const MOCK_SIGNED_DATA = 'mockSignedData' export const MOCK_HASHED_OTP = 'mockHashedOtp' export const MOCK_OTP = '123456' export const MOCK_RECIPIENT = '81234567' +export const MOCK_SENDER_IP = '200.0.0.0' export const VFN_FIELD_DEFAULTS = { signedData: null, diff --git a/src/app/modules/verification/verification.controller.ts b/src/app/modules/verification/verification.controller.ts index 11315f960a..d9ddae5713 100644 --- a/src/app/modules/verification/verification.controller.ts +++ b/src/app/modules/verification/verification.controller.ts @@ -5,7 +5,7 @@ import { ErrorDto } from '../../../../shared/types' import { SALT_ROUNDS } from '../../../../shared/utils/verification' import { createLoggerWithLabel } from '../../config/logger' import { generateOtpWithHash } from '../../utils/otp' -import { createReqMeta } from '../../utils/request' +import { createReqMeta, getRequestIp } from '../../utils/request' import { ControllerHandler } from '../core/core.types' import * as FormService from '../form/form.service' @@ -143,6 +143,8 @@ export const handleGetOtp: ControllerHandler< > = async (req, res) => { const { transactionId } = req.params const { answer, fieldId } = req.body + const senderIp = getRequestIp(req) + const logMeta = { action: 'handleGetOtp', transactionId, @@ -157,6 +159,7 @@ export const handleGetOtp: ControllerHandler< otp, recipient: answer, transactionId, + senderIp, }), ) .map(() => res.sendStatus(StatusCodes.CREATED)) @@ -200,6 +203,8 @@ export const _handleGenerateOtp: ControllerHandler< > = async (req, res) => { const { transactionId, formId, fieldId } = req.params const { answer } = req.body + const senderIp = getRequestIp(req) + const logMeta = { action: 'handleGenerateOtp', transactionId, @@ -220,6 +225,7 @@ export const _handleGenerateOtp: ControllerHandler< otp, recipient: answer, transactionId, + senderIp, }), ) // Return the required data for next steps. diff --git a/src/app/modules/verification/verification.service.ts b/src/app/modules/verification/verification.service.ts index 4f83cb0495..3efa458552 100644 --- a/src/app/modules/verification/verification.service.ts +++ b/src/app/modules/verification/verification.service.ts @@ -294,6 +294,7 @@ export const sendNewOtp = ({ recipient, otp, hashedOtp, + senderIp, }: SendOtpParams): ResultAsync< IVerificationSchema, | TransactionNotFoundError @@ -334,7 +335,13 @@ export const sendNewOtp = ({ return errAsync(new OtpRequestCountExceededError()) } - return sendOtpForField(transaction.formId, field, recipient, otp) + return sendOtpForField( + transaction.formId, + field, + recipient, + otp, + senderIp, + ) }) .andThen(() => { const signedData = formsgSdk.verification.generateSignature({ @@ -504,12 +511,14 @@ export const verifyOtp = ( * @param field * @param recipient * @param otp + * @param senderIp */ const sendOtpForField = ( formId: string, field: IVerificationFieldSchema, recipient: string, otp: string, + senderIp: string, ): ResultAsync< true, | DatabaseError @@ -529,7 +538,7 @@ const sendOtpForField = ( .andThen((form) => shouldGenerateMobileOtp(form, fieldId)) // call sms - it should validate the recipient .andThen(() => - SmsFactory.sendVerificationOtp(recipient, otp, formId), + SmsFactory.sendVerificationOtp(recipient, otp, formId, senderIp), ) : errAsync(new MalformedParametersError('Field id not present')) case BasicField.Email: diff --git a/src/app/modules/verification/verification.types.ts b/src/app/modules/verification/verification.types.ts index 46148cd864..9605681606 100644 --- a/src/app/modules/verification/verification.types.ts +++ b/src/app/modules/verification/verification.types.ts @@ -13,4 +13,5 @@ export type SendOtpParams = { recipient: string otp: string hashedOtp: string + senderIp: string } diff --git a/src/app/services/sms/__tests__/sms.factory.spec.ts b/src/app/services/sms/__tests__/sms.factory.spec.ts index b66f56b57b..f1103cc5d8 100644 --- a/src/app/services/sms/__tests__/sms.factory.spec.ts +++ b/src/app/services/sms/__tests__/sms.factory.spec.ts @@ -56,6 +56,7 @@ describe('sms.factory', () => { 'mockRecipient', 'mockOtp', 'mockFormId', + 'mockSenderIp', ] // Act @@ -77,6 +78,7 @@ describe('sms.factory', () => { 'mockRecipient', 'mockOtp', 'mockUserId', + 'mockSenderIp', ] // Act diff --git a/src/app/services/sms/__tests__/sms.service.spec.ts b/src/app/services/sms/__tests__/sms.service.spec.ts index 6a0286b97f..93d3aae235 100644 --- a/src/app/services/sms/__tests__/sms.service.spec.ts +++ b/src/app/services/sms/__tests__/sms.service.spec.ts @@ -34,6 +34,7 @@ const MOCK_ADMIN_EMAIL = 'adminEmail@email.com' const MOCK_ADMIN_ID = new ObjectId().toHexString() const MOCK_FORM_ID = new ObjectId().toHexString() const MOCK_FORM_TITLE = 'formTitle' +const MOCK_SENDER_IP = '200.000.000.000' const MOCK_TWILIO_WEBHOOK_ROUTE = '/api/v3/notifications/twilio' @@ -103,6 +104,11 @@ describe('sms.service', () => { forceDelivery: true, statusCallback: expect.stringContaining(MOCK_TWILIO_WEBHOOK_ROUTE), }) + + expect(twilioSuccessSpy.mock.calls[0][0].statusCallback).toEqual( + expect.not.stringContaining('?senderIp'), + ) + expect(smsCountSpy).toHaveBeenCalledWith({ smsData: { form: MOCK_FORM_ID, @@ -185,6 +191,11 @@ describe('sms.service', () => { forceDelivery: true, statusCallback: expect.stringContaining(MOCK_TWILIO_WEBHOOK_ROUTE), }) + + expect(twilioSuccessSpy.mock.calls[0][0].statusCallback).toEqual( + expect.not.stringContaining('?senderIp'), + ) + expect(smsCountSpy).toHaveBeenCalledWith({ smsData: { form: MOCK_FORM_ID, @@ -275,6 +286,7 @@ describe('sms.service', () => { /* recipient= */ TWILIO_TEST_NUMBER, /* otp= */ '111111', /* formId= */ testForm._id, + /* senderIp= */ MOCK_SENDER_IP, /* defaultConfig= */ MOCK_VALID_CONFIG, ) @@ -295,10 +307,15 @@ describe('sms.service', () => { /* recipient= */ TWILIO_TEST_NUMBER, /* otp= */ '111111', /* formId= */ testForm._id, + /* senderIp= */ MOCK_SENDER_IP, /* defaultConfig= */ MOCK_VALID_CONFIG, ) // Assert + expect(twilioSuccessSpy.mock.calls[0][0].statusCallback).toEqual( + expect.stringContaining('?senderIp'), + ) + expect(actualResult._unsafeUnwrap()).toEqual(true) // Logging should also have happened. const expectedLogParams = { @@ -319,6 +336,7 @@ describe('sms.service', () => { /* recipient= */ TWILIO_TEST_NUMBER, /* otp= */ '111111', /* formId= */ testForm._id, + /* senderIp= */ MOCK_SENDER_IP, /* defaultConfig= */ MOCK_INVALID_CONFIG, ) @@ -342,10 +360,15 @@ describe('sms.service', () => { /* recipient= */ TWILIO_TEST_NUMBER, /* otp= */ '111111', /* userId= */ testUser._id, + /* senderIp= */ MOCK_SENDER_IP, /* defaultConfig= */ MOCK_VALID_CONFIG, ) // Assert + expect(twilioSuccessSpy.mock.calls[0][0].statusCallback).toEqual( + expect.stringContaining('?senderIp'), + ) + // Should resolve to true expect(actualResult.isOk()).toEqual(true) expect(actualResult._unsafeUnwrap()).toEqual(true) @@ -398,6 +421,7 @@ describe('sms.service', () => { /* recipient= */ TWILIO_TEST_NUMBER, /* otp= */ '111111', /* userId= */ testUser._id, + /* senderIp= */ MOCK_SENDER_IP, /* defaultConfig= */ MOCK_INVALID_CONFIG, ) diff --git a/src/app/services/sms/sms.factory.ts b/src/app/services/sms/sms.factory.ts index 3d76c73937..f65f971058 100644 --- a/src/app/services/sms/sms.factory.ts +++ b/src/app/services/sms/sms.factory.ts @@ -15,11 +15,13 @@ interface ISmsFactory { recipient: string, otp: string, formId: string, + senderIp: string, ) => ReturnType sendAdminContactOtp: ( recipient: string, otp: string, userId: string, + senderIp: string, ) => ReturnType /** * Informs recipient that the given form was deactivated. Rejects if SMS feature @@ -65,10 +67,10 @@ export const createSmsFactory = (smsConfig: ISms): ISmsFactory => { } return { - sendVerificationOtp: (recipient, otp, formId) => - sendVerificationOtp(recipient, otp, formId, twilioConfig), - sendAdminContactOtp: (recipient, otp, userId) => - sendAdminContactOtp(recipient, otp, userId, twilioConfig), + sendVerificationOtp: (recipient, otp, formId, senderIp) => + sendVerificationOtp(recipient, otp, formId, senderIp, twilioConfig), + sendAdminContactOtp: (recipient, otp, userId, senderIp) => + sendAdminContactOtp(recipient, otp, userId, senderIp, twilioConfig), sendFormDeactivatedSms: (params) => sendFormDeactivatedSms(params, twilioConfig), sendBouncedSubmissionSms: (params) => diff --git a/src/app/services/sms/sms.service.ts b/src/app/services/sms/sms.service.ts index 5ac1796318..71a00b9752 100644 --- a/src/app/services/sms/sms.service.ts +++ b/src/app/services/sms/sms.service.ts @@ -166,6 +166,7 @@ const logSmsSend = (logParams: LogSmsParams) => { * @param smsData The data for logging smsCount * @param recipient The mobile number of the recipient * @param message The message to send + * @param senderIp The ip address of the person triggering the SMS */ const sendSms = ( twilioConfig: TwilioConfig, @@ -173,6 +174,7 @@ const sendSms = ( recipient: string, message: string, smsType: SmsType, + senderIp?: string, ): ResultAsync => { if (!isPhoneNumber(recipient)) { logger.warn({ @@ -194,13 +196,19 @@ const sendSms = ( const statusCallbackRoute = '/api/v3/notifications/twilio' + const statusCallback = senderIp + ? `${config.app.appUrl}${statusCallbackRoute}?${encodeURI( + `senderIp=${senderIp}`, + )}` + : `${config.app.appUrl}${statusCallbackRoute}` + return ResultAsync.fromPromise( client.messages.create({ to: recipient, body: message, from: msgSrvcSid, forceDelivery: true, - statusCallback: config.app.appUrl + statusCallbackRoute, + statusCallback, }), (error) => { logger.error({ @@ -290,12 +298,13 @@ const sendSms = ( * @param recipient The phone number to send to * @param otp The OTP to send * @param formId Form id for retrieving otp data. - * + * @param senderIp The ip address of the person triggering the SMS */ export const sendVerificationOtp = ( recipient: string, otp: string, formId: string, + senderIp: string, defaultConfig: TwilioConfig, ): ResultAsync< true, @@ -348,6 +357,7 @@ export const sendVerificationOtp = ( recipient, message, SmsType.Verification, + senderIp, ) }) }) @@ -357,6 +367,7 @@ export const sendAdminContactOtp = ( recipient: string, otp: string, userId: string, + senderIp: string, defaultConfig: TwilioConfig, ): ResultAsync => { logger.info({ @@ -379,6 +390,7 @@ export const sendAdminContactOtp = ( recipient, message, SmsType.AdminContact, + senderIp, ) }