diff --git a/src/app/modules/bounce/__tests__/bounce.controller.spec.ts b/src/app/modules/bounce/__tests__/bounce.controller.spec.ts index 75244e8e47..85e91c8e29 100644 --- a/src/app/modules/bounce/__tests__/bounce.controller.spec.ts +++ b/src/app/modules/bounce/__tests__/bounce.controller.spec.ts @@ -1,9 +1,10 @@ import { ObjectId } from 'bson' import mongoose from 'mongoose' -import { errAsync, okAsync } from 'neverthrow' +import { errAsync, ok, okAsync } from 'neverthrow' import { mocked } from 'ts-jest/utils' import getFormModel from 'src/app/models/form.server.model' +import { handleSns } from 'src/app/modules/bounce/bounce.controller' import getBounceModel from 'src/app/modules/bounce/bounce.model' import * as BounceService from 'src/app/modules/bounce/bounce.service' import * as FormService from 'src/app/modules/form/form.service' @@ -19,6 +20,7 @@ import dbHandler from 'tests/unit/backend/helpers/jest-db' import expressHandler from 'tests/unit/backend/helpers/jest-express' import { DatabaseError } from '../../core/core.errors' +import { InvalidNotificationError } from '../bounce.errors' const Bounce = getBounceModel(mongoose) const FormModel = getFormModel(mongoose) @@ -39,8 +41,6 @@ jest.doMock('mongoose', () => ({ VersionError: MockVersionError, }, })) -// eslint-disable-next-line import/first -import { handleSns } from 'src/app/modules/bounce/bounce.controller' const MOCK_NOTIFICATION = { notificationType: 'Bounce' } as IEmailNotification const MOCK_REQ = expressHandler.mockRequest({ @@ -100,49 +100,39 @@ describe('handleSns', () => { beforeEach(() => { jest.resetAllMocks() // Default mocks - MockBounceService.isValidSnsRequest.mockResolvedValue(true) + MockBounceService.validateSnsRequest.mockReturnValue(okAsync(true)) + MockBounceService.safeParseNotification.mockReturnValue( + ok(MOCK_NOTIFICATION), + ) MockBounceService.extractEmailType.mockReturnValue(EmailType.AdminResponse) - MockBounceService.getUpdatedBounceDoc.mockResolvedValue(mockBounceDoc) + MockBounceService.getUpdatedBounceDoc.mockReturnValue( + okAsync(mockBounceDoc), + ) MockFormService.retrieveFullFormById.mockResolvedValue(okAsync(mockForm)) mockBounceDoc.isCriticalBounce.mockReturnValue(true) - MockBounceService.getEditorsWithContactNumbers.mockResolvedValue( - MOCK_CONTACTS, + MockBounceService.getEditorsWithContactNumbers.mockReturnValue( + okAsync(MOCK_CONTACTS), ) mockBounceDoc.hasNotified.mockReturnValue(false) - MockBounceService.notifyAdminsOfBounce.mockResolvedValue({ - emailRecipients: MOCK_EMAIL_RECIPIENTS, - smsRecipients: MOCK_CONTACTS, - }) + MockBounceService.sendEmailBounceNotification.mockReturnValue( + okAsync(MOCK_EMAIL_RECIPIENTS), + ) + MockBounceService.sendSmsBounceNotification.mockReturnValue( + okAsync(MOCK_CONTACTS), + ) + MockBounceService.saveBounceDoc.mockReturnValue(okAsync(mockBounceDoc)) // Note that this is true to simulate permanent bounce mockBounceDoc.areAllPermanentBounces.mockReturnValue(true) }) afterEach(async () => await dbHandler.clearDatabase()) - it('should return immediately when requests are invalid', async () => { - MockBounceService.isValidSnsRequest.mockResolvedValueOnce(false) - await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( - MOCK_REQ.body, + it('should return 401 when requests are invalid', async () => { + MockBounceService.validateSnsRequest.mockReturnValueOnce( + errAsync(new InvalidNotificationError()), ) - expect(MockBounceService.logEmailNotification).not.toHaveBeenCalled() - expect(MockFormService.retrieveFullFormById).not.toHaveBeenCalled() - expect( - MockBounceService.getEditorsWithContactNumbers, - ).not.toHaveBeenCalled() - expect(MockBounceService.notifyAdminsOfBounce).not.toHaveBeenCalled() - expect(MockFormService.deactivateForm).not.toHaveBeenCalled() - expect(MockBounceService.notifyAdminsOfDeactivation).not.toHaveBeenCalled() - expect(MockBounceService.logCriticalBounce).not.toHaveBeenCalled() - expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(403) - }) - - it('should return 400 when errors are thrown in isValidSnsRequest', async () => { - MockBounceService.isValidSnsRequest.mockImplementation(() => { - throw new Error() - }) await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( + expect(MockBounceService.validateSnsRequest).toHaveBeenCalledWith( MOCK_REQ.body, ) expect(MockBounceService.logEmailNotification).not.toHaveBeenCalled() @@ -150,11 +140,12 @@ describe('handleSns', () => { expect( MockBounceService.getEditorsWithContactNumbers, ).not.toHaveBeenCalled() - expect(MockBounceService.notifyAdminsOfBounce).not.toHaveBeenCalled() + expect(MockBounceService.sendEmailBounceNotification).not.toHaveBeenCalled() + expect(MockBounceService.sendSmsBounceNotification).not.toHaveBeenCalled() expect(MockFormService.deactivateForm).not.toHaveBeenCalled() expect(MockBounceService.notifyAdminsOfDeactivation).not.toHaveBeenCalled() expect(MockBounceService.logCriticalBounce).not.toHaveBeenCalled() - expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(400) + expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(401) }) it('should call services correctly for permanent critical bounces', async () => { @@ -162,7 +153,7 @@ describe('handleSns', () => { // to mocks are needed await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( + expect(MockBounceService.validateSnsRequest).toHaveBeenCalledWith( MOCK_REQ.body, ) expect(MockBounceService.logEmailNotification).toHaveBeenCalledWith( @@ -181,7 +172,11 @@ describe('handleSns', () => { expect(MockBounceService.getEditorsWithContactNumbers).toHaveBeenCalledWith( mockForm, ) - expect(MockBounceService.notifyAdminsOfBounce).toHaveBeenCalledWith( + expect(MockBounceService.sendEmailBounceNotification).toHaveBeenCalledWith( + mockBounceDoc, + mockForm, + ) + expect(MockBounceService.sendSmsBounceNotification).toHaveBeenCalledWith( mockBounceDoc, mockForm, MOCK_CONTACTS, @@ -205,7 +200,7 @@ describe('handleSns', () => { autoSmsRecipients: MOCK_CONTACTS, hasDeactivated: true, }) - expect(mockBounceDoc.save).toHaveBeenCalled() + expect(MockBounceService.saveBounceDoc).toHaveBeenCalledWith(mockBounceDoc) expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(200) }) @@ -215,7 +210,7 @@ describe('handleSns', () => { await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( + expect(MockBounceService.validateSnsRequest).toHaveBeenCalledWith( MOCK_REQ.body, ) expect(MockBounceService.logEmailNotification).toHaveBeenCalledWith( @@ -234,7 +229,11 @@ describe('handleSns', () => { expect(MockBounceService.getEditorsWithContactNumbers).toHaveBeenCalledWith( mockForm, ) - expect(MockBounceService.notifyAdminsOfBounce).toHaveBeenCalledWith( + expect(MockBounceService.sendEmailBounceNotification).toHaveBeenCalledWith( + mockBounceDoc, + mockForm, + ) + expect(MockBounceService.sendSmsBounceNotification).toHaveBeenCalledWith( mockBounceDoc, mockForm, MOCK_CONTACTS, @@ -254,16 +253,16 @@ describe('handleSns', () => { autoSmsRecipients: MOCK_CONTACTS, hasDeactivated: false, }) - expect(mockBounceDoc.save).toHaveBeenCalled() + expect(MockBounceService.saveBounceDoc).toHaveBeenCalledWith(mockBounceDoc) expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(200) }) - it('should return 400 when errors are thrown in getUpdatedBounceDoc', async () => { - MockBounceService.getUpdatedBounceDoc.mockImplementationOnce(() => { - throw new Error() - }) + it('should return 200 even when errors are thrown in getUpdatedBounceDoc', async () => { + MockBounceService.getUpdatedBounceDoc.mockReturnValueOnce( + errAsync(new DatabaseError()), + ) await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( + expect(MockBounceService.validateSnsRequest).toHaveBeenCalledWith( MOCK_REQ.body, ) expect(MockBounceService.logEmailNotification).toHaveBeenCalledWith( @@ -275,7 +274,7 @@ describe('handleSns', () => { expect(MockBounceService.getUpdatedBounceDoc).toHaveBeenCalledWith( MOCK_NOTIFICATION, ) - expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(400) + expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(200) }) it('should return 500 early if error occurs while retrieving form', async () => { @@ -285,7 +284,7 @@ describe('handleSns', () => { await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( + expect(MockBounceService.validateSnsRequest).toHaveBeenCalledWith( MOCK_REQ.body, ) expect(MockBounceService.logEmailNotification).toHaveBeenCalledWith( @@ -303,19 +302,20 @@ describe('handleSns', () => { expect( MockBounceService.getEditorsWithContactNumbers, ).not.toHaveBeenCalled() - expect(MockBounceService.notifyAdminsOfBounce).not.toHaveBeenCalled() + expect(MockBounceService.sendEmailBounceNotification).not.toHaveBeenCalled() + expect(MockBounceService.sendSmsBounceNotification).not.toHaveBeenCalled() expect(MockBounceService.notifyAdminsOfDeactivation).not.toHaveBeenCalled() expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(500) }) - it('should return 400 when errors are thrown in deactivateForm', async () => { - MockFormService.deactivateForm.mockImplementationOnce(() => { - throw new Error() - }) + it('should return 200 even when errors are returned from deactivateForm', async () => { + MockFormService.deactivateForm.mockReturnValueOnce( + errAsync(new DatabaseError()), + ) await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( + expect(MockBounceService.validateSnsRequest).toHaveBeenCalledWith( MOCK_REQ.body, ) expect(MockBounceService.logEmailNotification).toHaveBeenCalledWith( @@ -338,41 +338,7 @@ describe('handleSns', () => { expect(MockFormService.deactivateForm).toHaveBeenCalledWith( mockBounceDoc.formId, ) - expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(400) - }) - - it('should return 400 when errors are thrown in notifyAdminsOfBounce', async () => { - MockBounceService.notifyAdminsOfBounce.mockImplementationOnce(() => { - throw new Error() - }) - - await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( - MOCK_REQ.body, - ) - expect(MockBounceService.logEmailNotification).toHaveBeenCalledWith( - MOCK_NOTIFICATION, - ) - expect(MockBounceService.extractEmailType).toHaveBeenCalledWith( - MOCK_NOTIFICATION, - ) - expect(MockBounceService.getUpdatedBounceDoc).toHaveBeenCalledWith( - MOCK_NOTIFICATION, - ) - expect(MockFormService.retrieveFullFormById).toHaveBeenCalledWith( - mockBounceDoc.formId, - ) - expect(mockBounceDoc.isCriticalBounce).toHaveBeenCalled() - expect(MockBounceService.getEditorsWithContactNumbers).toHaveBeenCalledWith( - mockForm, - ) - expect(MockBounceService.notifyAdminsOfBounce).toHaveBeenCalledWith( - mockBounceDoc, - mockForm, - MOCK_CONTACTS, - ) - expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(400) + expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(200) }) it('should return 200 when VersionErrors are thrown', async () => { @@ -382,7 +348,7 @@ describe('handleSns', () => { await handleSns(MOCK_REQ, MOCK_RES, jest.fn()) - expect(MockBounceService.isValidSnsRequest).toHaveBeenCalledWith( + expect(MockBounceService.validateSnsRequest).toHaveBeenCalledWith( MOCK_REQ.body, ) expect(MockBounceService.logEmailNotification).toHaveBeenCalledWith( @@ -405,7 +371,11 @@ describe('handleSns', () => { expect(MockFormService.deactivateForm).toHaveBeenCalledWith( mockBounceDoc.formId, ) - expect(MockBounceService.notifyAdminsOfBounce).toHaveBeenCalledWith( + expect(MockBounceService.sendEmailBounceNotification).toHaveBeenCalledWith( + mockBounceDoc, + mockForm, + ) + expect(MockBounceService.sendSmsBounceNotification).toHaveBeenCalledWith( mockBounceDoc, mockForm, MOCK_CONTACTS, @@ -421,7 +391,7 @@ describe('handleSns', () => { autoSmsRecipients: MOCK_CONTACTS, hasDeactivated: true, }) - expect(mockBounceDoc.save).toHaveBeenCalled() + expect(MockBounceService.saveBounceDoc).toHaveBeenCalledWith(mockBounceDoc) expect(MOCK_RES.sendStatus).toHaveBeenCalledWith(200) }) }) diff --git a/src/app/modules/bounce/__tests__/bounce.service.spec.ts b/src/app/modules/bounce/__tests__/bounce.service.spec.ts index f29fb5306f..8e5e773660 100644 --- a/src/app/modules/bounce/__tests__/bounce.service.spec.ts +++ b/src/app/modules/bounce/__tests__/bounce.service.spec.ts @@ -25,7 +25,7 @@ import dbHandler from 'tests/unit/backend/helpers/jest-db' import getMockLogger from 'tests/unit/backend/helpers/jest-logger' import { DatabaseError } from '../../core/core.errors' -import { UserWithContactNumber } from '../bounce.types' +import { UserWithContactNumber } from '../../user/user.types' import { makeBounceNotification, MOCK_SNS_BODY } from './bounce-test-helpers' @@ -52,21 +52,17 @@ MockLoggerModule.createLoggerWithLabel.mockReturnValue(mockLogger) // Import modules which depend on config last so that mocks get imported correctly import getBounceModel from 'src/app/modules/bounce/bounce.model' -import { - extractEmailType, - getEditorsWithContactNumbers, - getUpdatedBounceDoc, - isValidSnsRequest, - logCriticalBounce, - logEmailNotification, - notifyAdminsOfBounce, - notifyAdminsOfDeactivation, -} from 'src/app/modules/bounce/bounce.service' +import * as BounceService from 'src/app/modules/bounce/bounce.service' import { InvalidNumberError, SmsSendError, } from 'src/app/services/sms/sms.errors' +import { + InvalidNotificationError, + MissingEmailHeadersError, +} from '../bounce.errors' + const Form = getFormModel(mongoose) const Bounce = getBounceModel(mongoose) @@ -87,6 +83,10 @@ const MOCK_SUBMISSION_ID = new ObjectId() describe('BounceService', () => { beforeAll(async () => await dbHandler.connect()) + beforeEach(() => { + MockMailService.sendBounceNotification.mockReturnValue(okAsync(true)) + }) + afterEach(async () => { await dbHandler.clearDatabase() jest.clearAllMocks() @@ -99,7 +99,9 @@ describe('BounceService', () => { const notification = makeBounceNotification({ emailType: EmailType.AdminResponse, }) - expect(extractEmailType(notification)).toBe(EmailType.AdminResponse) + expect(BounceService.extractEmailType(notification)).toBe( + EmailType.AdminResponse, + ) }) }) @@ -112,7 +114,7 @@ describe('BounceService', () => { await dbHandler.clearDatabase() }) - it('should return null when there is no form ID', async () => { + it('should return MissingEmailHeadersError when there is no form ID', async () => { const notification = makeBounceNotification() const header = notification.mail.headers.find( (header) => header.name === EMAIL_HEADERS.formId, @@ -121,8 +123,8 @@ describe('BounceService', () => { if (header) { header.value = '' } - const result = await getUpdatedBounceDoc(notification) - expect(result).toBeNull() + const result = await BounceService.getUpdatedBounceDoc(notification) + expect(result._unsafeUnwrapErr()).toEqual(new MissingEmailHeadersError()) }) it('should call updateBounceInfo if the document exists', async () => { @@ -133,8 +135,8 @@ describe('BounceService', () => { const notification = makeBounceNotification({ formId: MOCK_FORM_ID, }) - const result = await getUpdatedBounceDoc(notification) - expect(result?.toObject()).toEqual( + const result = await BounceService.getUpdatedBounceDoc(notification) + expect(result._unsafeUnwrap().toObject()).toEqual( bounceDoc.updateBounceInfo(notification).toObject(), ) }) @@ -144,8 +146,8 @@ describe('BounceService', () => { const notification = makeBounceNotification({ formId: MOCK_FORM_ID, }) - const result = await getUpdatedBounceDoc(notification) - const actual = pick(result?.toObject(), [ + const result = await BounceService.getUpdatedBounceDoc(notification) + const actual = pick(result._unsafeUnwrap().toObject(), [ 'formId', 'bounces', 'hasAutoEmailed', @@ -178,7 +180,7 @@ describe('BounceService', () => { bounceType: BounceType.Transient, emailType: EmailType.EmailConfirmation, }) - logEmailNotification(notification) + BounceService.logEmailNotification(notification) expect(mockLogger.info).not.toHaveBeenCalled() expect(mockLogger.warn).not.toHaveBeenCalled() expect(mockShortTermLogger.info).toHaveBeenCalledWith(notification) @@ -195,7 +197,7 @@ describe('BounceService', () => { bounceType: BounceType.Transient, emailType: EmailType.AdminResponse, }) - logEmailNotification(notification) + BounceService.logEmailNotification(notification) expect(mockLogger.info).toHaveBeenCalledWith({ message: 'Email notification', meta: { @@ -218,7 +220,7 @@ describe('BounceService', () => { bounceType: BounceType.Transient, emailType: EmailType.LoginOtp, }) - logEmailNotification(notification) + BounceService.logEmailNotification(notification) expect(mockLogger.info).toHaveBeenCalledWith({ message: 'Email notification', meta: { @@ -241,7 +243,7 @@ describe('BounceService', () => { bounceType: BounceType.Transient, emailType: EmailType.AdminBounce, }) - logEmailNotification(notification) + BounceService.logEmailNotification(notification) expect(mockLogger.info).toHaveBeenCalledWith({ message: 'Email notification', meta: { @@ -264,14 +266,14 @@ describe('BounceService', () => { bounceType: BounceType.Transient, emailType: EmailType.VerificationOtp, }) - logEmailNotification(notification) + BounceService.logEmailNotification(notification) expect(mockLogger.info).not.toHaveBeenCalled() expect(mockLogger.warn).not.toHaveBeenCalled() expect(mockShortTermLogger.info).toHaveBeenCalledWith(notification) }) }) - describe('notifyAdminsOfBounce', () => { + describe('sendEmailBounceNotification', () => { const MOCK_FORM_TITLE = 'FormTitle' let testUser: IUserSchema @@ -282,10 +284,6 @@ describe('BounceService', () => { testUser = user }) - beforeEach(async () => { - jest.resetAllMocks() - }) - it('should auto-email when admin is not email recipient', async () => { const form = (await new Form({ admin: testUser._id, @@ -300,7 +298,10 @@ describe('BounceService', () => { ], }) - const notifiedRecipients = await notifyAdminsOfBounce(bounceDoc, form, []) + const notifiedRecipients = await BounceService.sendEmailBounceNotification( + bounceDoc, + form, + ) expect(MockMailService.sendBounceNotification).toHaveBeenCalledWith({ emailRecipients: [testUser.email], @@ -309,7 +310,7 @@ describe('BounceService', () => { formTitle: form.title, formId: form._id, }) - expect(notifiedRecipients.emailRecipients).toEqual([testUser.email]) + expect(notifiedRecipients._unsafeUnwrap()).toEqual([testUser.email]) }) it('should auto-email when any collaborator is not email recipient', async () => { @@ -328,7 +329,10 @@ describe('BounceService', () => { ], }) - const notifiedRecipients = await notifyAdminsOfBounce(bounceDoc, form, []) + const notifiedRecipients = await BounceService.sendEmailBounceNotification( + bounceDoc, + form, + ) expect(MockMailService.sendBounceNotification).toHaveBeenCalledWith({ emailRecipients: [collabEmail], @@ -337,7 +341,7 @@ describe('BounceService', () => { formTitle: form.title, formId: form._id, }) - expect(notifiedRecipients.emailRecipients).toEqual([collabEmail]) + expect(notifiedRecipients._unsafeUnwrap()).toEqual([collabEmail]) }) it('should not auto-email when admin is email recipient', async () => { @@ -354,10 +358,13 @@ describe('BounceService', () => { ], }) - const notifiedRecipients = await notifyAdminsOfBounce(bounceDoc, form, []) + const notifiedRecipients = await BounceService.sendEmailBounceNotification( + bounceDoc, + form, + ) expect(MockMailService.sendBounceNotification).not.toHaveBeenCalled() - expect(notifiedRecipients.emailRecipients).toEqual([]) + expect(notifiedRecipients._unsafeUnwrap()).toEqual([]) }) it('should not auto-email when all collabs are email recipients', async () => { @@ -377,10 +384,29 @@ describe('BounceService', () => { ], }) - const notifiedRecipients = await notifyAdminsOfBounce(bounceDoc, form, []) + const notifiedRecipients = await BounceService.sendEmailBounceNotification( + bounceDoc, + form, + ) expect(MockMailService.sendBounceNotification).not.toHaveBeenCalled() - expect(notifiedRecipients.emailRecipients).toEqual([]) + expect(notifiedRecipients._unsafeUnwrap()).toEqual([]) + }) + }) + + describe('sendSmsBounceNotification', () => { + const MOCK_FORM_TITLE = 'FormTitle' + let testUser: IUserSchema + + beforeEach(async () => { + const { user } = await dbHandler.insertFormCollectionReqs({ + userId: MOCK_ADMIN_ID, + }) + testUser = user + }) + + beforeEach(async () => { + jest.resetAllMocks() }) it('should send text for all SMS recipients and return successful ones', async () => { @@ -396,10 +422,11 @@ describe('BounceService', () => { }) MockSmsFactory.sendBouncedSubmissionSms.mockReturnValue(okAsync(true)) - const notifiedRecipients = await notifyAdminsOfBounce(bounceDoc, form, [ - MOCK_CONTACT, - MOCK_CONTACT_2, - ]) + const notifiedRecipients = await BounceService.sendSmsBounceNotification( + bounceDoc, + form, + [MOCK_CONTACT, MOCK_CONTACT_2], + ) expect(MockSmsFactory.sendBouncedSubmissionSms).toHaveBeenCalledTimes(2) expect(MockSmsFactory.sendBouncedSubmissionSms).toHaveBeenCalledWith({ @@ -418,7 +445,7 @@ describe('BounceService', () => { recipient: MOCK_CONTACT_2.contact, recipientEmail: MOCK_CONTACT_2.email, }) - expect(notifiedRecipients.smsRecipients).toEqual([ + expect(notifiedRecipients._unsafeUnwrap()).toEqual([ MOCK_CONTACT, MOCK_CONTACT_2, ]) @@ -439,10 +466,11 @@ describe('BounceService', () => { .mockReturnValueOnce(okAsync(true)) .mockReturnValueOnce(errAsync(new InvalidNumberError())) - const notifiedRecipients = await notifyAdminsOfBounce(bounceDoc, form, [ - MOCK_CONTACT, - MOCK_CONTACT_2, - ]) + const notifiedRecipients = await BounceService.sendSmsBounceNotification( + bounceDoc, + form, + [MOCK_CONTACT, MOCK_CONTACT_2], + ) expect(MockSmsFactory.sendBouncedSubmissionSms).toHaveBeenCalledTimes(2) expect(MockSmsFactory.sendBouncedSubmissionSms).toHaveBeenCalledWith({ @@ -461,7 +489,7 @@ describe('BounceService', () => { recipient: MOCK_CONTACT_2.contact, recipientEmail: MOCK_CONTACT_2.email, }) - expect(notifiedRecipients.smsRecipients).toEqual([MOCK_CONTACT]) + expect(notifiedRecipients._unsafeUnwrap()).toEqual([MOCK_CONTACT]) }) }) @@ -488,7 +516,7 @@ describe('BounceService', () => { }) const autoEmailRecipients = [MOCK_EMAIL, MOCK_EMAIL_2] const autoSmsRecipients = [MOCK_CONTACT, MOCK_CONTACT_2] - logCriticalBounce({ + BounceService.logCriticalBounce({ bounceDoc, notification: snsInfo, autoEmailRecipients, @@ -533,7 +561,7 @@ describe('BounceService', () => { }) const autoEmailRecipients: string[] = [] const autoSmsRecipients = [MOCK_CONTACT, MOCK_CONTACT_2] - logCriticalBounce({ + BounceService.logCriticalBounce({ bounceDoc, notification: snsInfo, autoEmailRecipients, @@ -578,7 +606,7 @@ describe('BounceService', () => { }) const autoEmailRecipients: string[] = [] const autoSmsRecipients = [MOCK_CONTACT, MOCK_CONTACT_2] - logCriticalBounce({ + BounceService.logCriticalBounce({ bounceDoc, notification: snsInfo, autoEmailRecipients, @@ -620,7 +648,7 @@ describe('BounceService', () => { }) const autoEmailRecipients: string[] = [] const autoSmsRecipients: UserWithContactNumber[] = [] - logCriticalBounce({ + BounceService.logCriticalBounce({ bounceDoc, notification: snsInfo, autoEmailRecipients, @@ -662,7 +690,7 @@ describe('BounceService', () => { }) const autoEmailRecipients: string[] = [] const autoSmsRecipients: UserWithContactNumber[] = [] - logCriticalBounce({ + BounceService.logCriticalBounce({ bounceDoc, notification: snsInfo, autoEmailRecipients, @@ -690,7 +718,7 @@ describe('BounceService', () => { }) }) - describe('isValidSnsRequest', () => { + describe('validateSnsRequest', () => { const keys = crypto.generateKeyPairSync('rsa', { modulusLength: 2048, publicKeyEncoding: { @@ -712,30 +740,43 @@ describe('BounceService', () => { }) }) - it('should gracefully reject when input is empty', () => { - return expect(isValidSnsRequest(undefined!)).resolves.toBe(false) + it('should gracefully reject when input is empty', async () => { + const result = await BounceService.validateSnsRequest(undefined!) + + expect(result._unsafeUnwrapErr()).toEqual(new InvalidNotificationError()) }) - it('should reject requests when their structure is invalid', () => { + it('should reject requests when their structure is invalid', async () => { const invalidBody = omit(cloneDeep(body), 'Type') as ISnsNotification - return expect(isValidSnsRequest(invalidBody)).resolves.toBe(false) + + const result = await BounceService.validateSnsRequest(invalidBody) + + expect(result._unsafeUnwrapErr()).toEqual(new InvalidNotificationError()) }) - it('should reject requests when their certificate URL is invalid', () => { + it('should reject requests when their certificate URL is invalid', async () => { body.SigningCertURL = 'http://www.example.com' - return expect(isValidSnsRequest(body)).resolves.toBe(false) + + const result = await BounceService.validateSnsRequest(body) + + expect(result._unsafeUnwrapErr()).toEqual(new InvalidNotificationError()) }) - it('should reject requests when their signature version is invalid', () => { + it('should reject requests when their signature version is invalid', async () => { body.SignatureVersion = 'wrongSignatureVersion' - return expect(isValidSnsRequest(body)).resolves.toBe(false) + + const result = await BounceService.validateSnsRequest(body) + + expect(result._unsafeUnwrapErr()).toEqual(new InvalidNotificationError()) }) - it('should reject requests when their signature is invalid', () => { - return expect(isValidSnsRequest(body)).resolves.toBe(false) + it('should reject requests when their signature is invalid', async () => { + const result = await BounceService.validateSnsRequest(body) + + expect(result._unsafeUnwrapErr()).toEqual(new InvalidNotificationError()) }) - it('should accept when requests are valid', () => { + it('should accept when requests are valid', async () => { const signer = crypto.createSign('RSA-SHA1') const baseString = dedent`Message @@ -751,7 +792,10 @@ describe('BounceService', () => { ` + '\n' signer.write(baseString) body.Signature = signer.sign(keys.privateKey, 'base64') - return expect(isValidSnsRequest(body)).resolves.toBe(true) + + const result = await BounceService.validateSnsRequest(body) + + expect(result._unsafeUnwrap()).toBe(true) }) }) @@ -785,13 +829,13 @@ describe('BounceService', () => { okAsync([MOCK_CONTACT]), ) - const result = await getEditorsWithContactNumbers(form) + const result = await BounceService.getEditorsWithContactNumbers(form) expect(MockUserService.findContactsForEmails).toHaveBeenCalledWith([ form.admin.email, MOCK_EMAIL, ]) - expect(result).toEqual([MOCK_CONTACT]) + expect(result._unsafeUnwrap()).toEqual([MOCK_CONTACT]) }) it('should filter out collaborators without contact numbers', async () => { @@ -809,13 +853,13 @@ describe('BounceService', () => { okAsync([omit(MOCK_CONTACT, 'contact'), MOCK_CONTACT_2]), ) - const result = await getEditorsWithContactNumbers(form) + const result = await BounceService.getEditorsWithContactNumbers(form) expect(MockUserService.findContactsForEmails).toHaveBeenCalledWith([ form.admin.email, MOCK_EMAIL, ]) - expect(result).toEqual([MOCK_CONTACT_2]) + expect(result._unsafeUnwrap()).toEqual([MOCK_CONTACT_2]) }) it('should return empty array when UserService returns error', async () => { @@ -833,13 +877,13 @@ describe('BounceService', () => { errAsync(new DatabaseError()), ) - const result = await getEditorsWithContactNumbers(form) + const result = await BounceService.getEditorsWithContactNumbers(form) expect(MockUserService.findContactsForEmails).toHaveBeenCalledWith([ form.admin.email, MOCK_EMAIL, ]) - expect(result).toEqual([]) + expect(result._unsafeUnwrap()).toEqual([]) }) }) @@ -867,12 +911,12 @@ describe('BounceService', () => { .execPopulate()) as IPopulatedForm MockSmsFactory.sendFormDeactivatedSms.mockReturnValue(okAsync(true)) - const result = await notifyAdminsOfDeactivation(form, [ + const result = await BounceService.notifyAdminsOfDeactivation(form, [ MOCK_CONTACT, MOCK_CONTACT_2, ]) - expect(result).toEqual(true) + expect(result._unsafeUnwrap()).toEqual(true) expect(MockSmsFactory.sendFormDeactivatedSms).toHaveBeenCalledTimes(2) expect(MockSmsFactory.sendFormDeactivatedSms).toHaveBeenCalledWith({ adminEmail: form.admin.email, @@ -903,12 +947,12 @@ describe('BounceService', () => { .mockReturnValueOnce(okAsync(true)) .mockReturnValueOnce(errAsync(new SmsSendError())) - const result = await notifyAdminsOfDeactivation(form, [ + const result = await BounceService.notifyAdminsOfDeactivation(form, [ MOCK_CONTACT, MOCK_CONTACT_2, ]) - expect(result).toEqual(true) + expect(result._unsafeUnwrap()).toEqual(true) expect(MockSmsFactory.sendFormDeactivatedSms).toHaveBeenCalledTimes(2) expect(MockSmsFactory.sendFormDeactivatedSms).toHaveBeenCalledWith({ adminEmail: form.admin.email, diff --git a/src/app/modules/bounce/bounce.controller.ts b/src/app/modules/bounce/bounce.controller.ts index 160cbff392..074be5ff54 100644 --- a/src/app/modules/bounce/bounce.controller.ts +++ b/src/app/modules/bounce/bounce.controller.ts @@ -1,121 +1,123 @@ import { RequestHandler } from 'express' -import { ParamsDictionary } from 'express-serve-static-core' import { StatusCodes } from 'http-status-codes' -import mongoose from 'mongoose' import { createLoggerWithLabel } from '../../../config/logger' -import { IEmailNotification, ISnsNotification } from '../../../types' +import { ISnsNotification } from '../../../types' import { EmailType } from '../../services/mail/mail.constants' +import { DatabaseConflictError } from '../core/core.errors' import * as FormService from '../form/form.service' import * as BounceService from './bounce.service' -import { AdminNotificationResult } from './bounce.types' const logger = createLoggerWithLabel(module) + /** * Validates that a request came from Amazon SNS, then updates the Bounce * collection. Also informs form admins and collaborators if their form responses - * bounced. + * bounced. Note that the response code is meaningless as it goes back to AWS. * @param req Express request object * @param res - Express response object */ export const handleSns: RequestHandler< - ParamsDictionary, + unknown, never, ISnsNotification > = async (req, res) => { - // Since this function is for a public endpoint, catch all possible errors - // so we never fail on malformed input. The response code is meaningless since - // it is meant to go back to AWS. - try { - const isValid = await BounceService.isValidSnsRequest(req.body) - if (!isValid) return res.sendStatus(StatusCodes.FORBIDDEN) - - const notification: IEmailNotification = JSON.parse(req.body.Message) - BounceService.logEmailNotification(notification) - if ( - BounceService.extractEmailType(notification) !== EmailType.AdminResponse - ) { - return res.sendStatus(StatusCodes.OK) - } - const bounceDoc = await BounceService.getUpdatedBounceDoc(notification) - // Missing headers in notification - if (!bounceDoc) return res.sendStatus(StatusCodes.OK) - - const formResult = await FormService.retrieveFullFormById(bounceDoc.formId) - if (formResult.isErr()) { - // Either database error occurred or the formId saved in the bounce collection - // doesn't exist, so something went wrong. - logger.error({ - message: 'Failed to retrieve form corresponding to bounced formId', - meta: { - action: 'handleSns', - formId: bounceDoc.formId, - }, - }) - return res.sendStatus(StatusCodes.INTERNAL_SERVER_ERROR) - } - const form = formResult.value - - if (bounceDoc.isCriticalBounce()) { - // Get contact numbers - const possibleSmsRecipients = await BounceService.getEditorsWithContactNumbers( - form, - ) - - // Notify admin and collaborators - let notificationRecipients: AdminNotificationResult = { - emailRecipients: [], - smsRecipients: [], - } - if (!bounceDoc.hasNotified()) { - notificationRecipients = await BounceService.notifyAdminsOfBounce( - bounceDoc, - form, - possibleSmsRecipients, - ) - bounceDoc.setNotificationState( - notificationRecipients.emailRecipients, - notificationRecipients.smsRecipients, - ) - } - - // Deactivate if all bounces are permanent - const shouldDeactivate = bounceDoc.areAllPermanentBounces() - if (shouldDeactivate) { - await FormService.deactivateForm(bounceDoc.formId) - await BounceService.notifyAdminsOfDeactivation( - form, - possibleSmsRecipients, - ) - } + const notificationResult = await BounceService.validateSnsRequest( + req.body, + ).andThen(() => BounceService.safeParseNotification(req.body.Message)) + if (notificationResult.isErr()) { + logger.warn({ + message: 'Unable to parse email notification request', + meta: { + action: 'handleSns', + }, + error: notificationResult.error, + }) + return res.sendStatus(StatusCodes.UNAUTHORIZED) + } + const notification = notificationResult.value - // Important log message for user follow-ups - BounceService.logCriticalBounce({ - bounceDoc, - notification, - autoEmailRecipients: notificationRecipients.emailRecipients, - autoSmsRecipients: notificationRecipients.smsRecipients, - hasDeactivated: shouldDeactivate, - }) - } - await bounceDoc.save() + BounceService.logEmailNotification(notification) + // If not admin response, no more action to be taken + if ( + BounceService.extractEmailType(notification) !== EmailType.AdminResponse + ) { return res.sendStatus(StatusCodes.OK) - } catch (err) { + } + + const bounceDocResult = await BounceService.getUpdatedBounceDoc(notification) + if (bounceDocResult.isErr()) { logger.warn({ - message: 'Error updating bounces', + message: 'Error while retrieving or creating new bounce doc', meta: { action: 'handleSns', }, - error: err, + error: bounceDocResult.error, }) - // Accept the risk that there might be concurrency problems - // when multiple server instances try to access the same - // document, due to notifications arriving asynchronously. - if (err instanceof mongoose.Error.VersionError) { - return res.sendStatus(StatusCodes.OK) + return res.sendStatus(StatusCodes.OK) + } + const bounceDoc = bounceDocResult.value + + const formResult = await FormService.retrieveFullFormById(bounceDoc.formId) + if (formResult.isErr()) { + // Either database error occurred or the formId saved in the bounce collection + // doesn't exist, so something went wrong. + logger.error({ + message: 'Failed to retrieve form corresponding to bounced formId', + meta: { + action: 'handleSns', + formId: bounceDoc.formId, + }, + }) + return res.sendStatus(StatusCodes.INTERNAL_SERVER_ERROR) + } + const form = formResult.value + + if (bounceDoc.isCriticalBounce()) { + // Send notifications and deactivate form on best-effort basis, ignore errors + const possibleSmsRecipients = await BounceService.getEditorsWithContactNumbers( + form, + ).unwrapOr([]) + const emailRecipients = await BounceService.sendEmailBounceNotification( + bounceDoc, + form, + ).unwrapOr([]) + const smsRecipients = await BounceService.sendSmsBounceNotification( + bounceDoc, + form, + possibleSmsRecipients, + ).unwrapOr([]) + bounceDoc.setNotificationState(emailRecipients, smsRecipients) + + const shouldDeactivate = bounceDoc.areAllPermanentBounces() + if (shouldDeactivate) { + await FormService.deactivateForm(bounceDoc.formId) + await BounceService.notifyAdminsOfDeactivation( + form, + possibleSmsRecipients, + ) } - // Malformed request, could not be parsed - return res.sendStatus(StatusCodes.BAD_REQUEST) + + // Important log message for user follow-ups + BounceService.logCriticalBounce({ + bounceDoc, + notification, + autoEmailRecipients: emailRecipients, + autoSmsRecipients: smsRecipients, + hasDeactivated: shouldDeactivate, + }) } + + return BounceService.saveBounceDoc(bounceDoc) + .map(() => res.sendStatus(StatusCodes.OK)) + .mapErr((error) => { + // Accept the risk that there might be concurrency problems + // when multiple server instances try to access the same + // document, due to notifications arriving asynchronously. + if (error instanceof DatabaseConflictError) + return res.sendStatus(StatusCodes.OK) + // Otherwise internal database error + return res.sendStatus(StatusCodes.INTERNAL_SERVER_ERROR) + }) } diff --git a/src/app/modules/bounce/bounce.errors.ts b/src/app/modules/bounce/bounce.errors.ts new file mode 100644 index 0000000000..e97ef45168 --- /dev/null +++ b/src/app/modules/bounce/bounce.errors.ts @@ -0,0 +1,63 @@ +import { InvalidNumberError, SmsSendError } from '../../services/sms/sms.errors' +import { ApplicationError } from '../core/core.errors' + +/** + * Error while retrieving the public key from the certificate URL provided + * in the SNS notification body. + */ +export class RetrieveAwsCertError extends ApplicationError { + constructor(message = 'Error while retrieving AWS signing public key') { + super(message) + } +} + +/** + * Unexpected shape of request body. + */ +export class InvalidNotificationError extends ApplicationError { + constructor(message = 'Notification from AWS could not be validated') { + super(message) + } +} + +/** + * Error while sending bounce-related notification to form admins via SMS. + */ +export class SendBounceSmsNotificationError extends ApplicationError { + meta: { + contact: string + originalError: SmsSendError | InvalidNumberError + } + + constructor( + originalError: SmsSendError | InvalidNumberError, + contact: string, + message = 'Error while sending bounce notification via SMS', + ) { + super(message) + this.meta = { + contact, + originalError, + } + } +} + +/** + * Email headers are missing the custom headers containing form and submission IDs. + */ +export class MissingEmailHeadersError extends ApplicationError { + constructor( + message = 'Email is missing custom header containing form or submission ID', + ) { + super(message) + } +} + +/** + * Error while parsing notification + */ +export class ParseNotificationError extends ApplicationError { + constructor(message = 'Could not parse SNS notification') { + super(message) + } +} diff --git a/src/app/modules/bounce/bounce.service.ts b/src/app/modules/bounce/bounce.service.ts index 93a9418dc9..78d9173bb6 100644 --- a/src/app/modules/bounce/bounce.service.ts +++ b/src/app/modules/bounce/bounce.service.ts @@ -2,6 +2,13 @@ import axios from 'axios' import crypto from 'crypto' import { difference, isEmpty } from 'lodash' import mongoose from 'mongoose' +import { + combineWithAllErrors, + errAsync, + okAsync, + Result, + ResultAsync, +} from 'neverthrow' import { createCloudWatchLogger, @@ -17,14 +24,24 @@ import { import { EMAIL_HEADERS, EmailType } from '../../services/mail/mail.constants' import MailService from '../../services/mail/mail.service' import { SmsFactory } from '../../services/sms/sms.factory' +import { transformMongoError } from '../../utils/handle-mongo-error' +import { hasProp } from '../../utils/has-prop' +import { PossibleDatabaseError } from '../core/core.errors' import { getCollabEmailsWithPermission } from '../form/form.utils' import * as UserService from '../user/user.service' +import { UserWithContactNumber } from '../user/user.types' +import { isUserWithContactNumber } from '../user/user.utils' +import { + InvalidNotificationError, + MissingEmailHeadersError, + ParseNotificationError, + RetrieveAwsCertError, + SendBounceSmsNotificationError, +} from './bounce.errors' import getBounceModel from './bounce.model' -import { AdminNotificationResult, UserWithContactNumber } from './bounce.types' import { extractHeader, - extractSmsErrors, extractSuccessfulSmsRecipients, isBounceNotification, } from './bounce.util' @@ -54,8 +71,8 @@ const AWS_HOSTNAME = '.amazonaws.com' * @param body body from Express request object * @returns true if all required keys are present */ -const hasRequiredKeys = (body: any): body is ISnsNotification => { - return !isEmpty(body) && snsKeys.every((keyObj) => body[keyObj.key]) +const hasRequiredKeys = (body: unknown): body is ISnsNotification => { + return !isEmpty(body) && snsKeys.every((keyObj) => hasProp(body, keyObj.key)) } /** @@ -96,13 +113,29 @@ const getSnsBasestring = (body: ISnsNotification): string => { * @param body body from Express request object * @returns true if signature is valid */ -const isValidSnsSignature = async ( +const isValidSnsSignature = ( body: ISnsNotification, -): Promise => { - const { data: cert } = await axios.get(body.SigningCertURL) - const verifier = crypto.createVerify('RSA-SHA1') - verifier.update(getSnsBasestring(body), 'utf8') - return verifier.verify(cert, body.Signature, 'base64') +): ResultAsync => { + return ResultAsync.fromPromise( + axios.get(body.SigningCertURL), + (error) => { + logger.warn({ + message: 'Error while retrieving AWS signing certificate', + meta: { + action: 'isValidSnsSignature', + signingCertUrl: body.SigningCertURL, + }, + error, + }) + return new RetrieveAwsCertError() + }, + ).andThen(({ data: cert }) => { + const verifier = crypto.createVerify('RSA-SHA1') + verifier.update(getSnsBasestring(body), 'utf8') + return verifier.verify(cert, body.Signature, 'base64') + ? okAsync(true) + : errAsync(new InvalidNotificationError()) + }) } /** @@ -111,15 +144,16 @@ const isValidSnsSignature = async ( * @param body Body of Express request object * @returns true if request shape and signature are valid */ -export const isValidSnsRequest = async ( +export const validateSnsRequest = ( body: ISnsNotification, -): Promise => { - const isValid = - hasRequiredKeys(body) && - body.SignatureVersion === '1' && // We only check for SHA1-RSA signatures - isValidCertUrl(body.SigningCertURL) && - (await isValidSnsSignature(body)) - return isValid +): ResultAsync => { + if ( + !hasRequiredKeys(body) || + body.SignatureVersion !== '1' || + !isValidCertUrl(body.SigningCertURL) + ) + return errAsync(new InvalidNotificationError()) + return isValidSnsSignature(body) } /** @@ -197,35 +231,63 @@ const computeValidEmails = ( } /** - * Notifies admin and collaborators via email and SMS that response was lost. + * Notifies admin and collaborators via email that response was lost. * @param bounceDoc Document from Bounce collection * @param form Form corresponding to the formId from bounceDoc - * @param possibleSmsRecipients Contact details of recipients to attempt to SMS - * @returns contact details for email and SMSes which were successfully sent. Note that - * this doesn't mean the emails and SMSes were received, only that they were delivered + * @returns contact details for emails which were successfully sent. Note that + * this doesn't mean the emails were received, only that they were delivered * to the mail server/carrier. */ -export const notifyAdminsOfBounce = async ( +export const sendEmailBounceNotification = ( bounceDoc: IBounceSchema, form: IPopulatedForm, - possibleSmsRecipients: UserWithContactNumber[], -): Promise => { + // Returns no errors. If emails fail, returns + // empty array as list of recipients. +): ResultAsync => { // Email all collaborators const emailRecipients = computeValidEmails(form, bounceDoc) - if (emailRecipients.length > 0) { - await MailService.sendBounceNotification({ - emailRecipients, - bouncedRecipients: bounceDoc.getEmails(), - bounceType: bounceDoc.areAllPermanentBounces() - ? BounceType.Permanent - : BounceType.Transient, - formTitle: form.title, - formId: bounceDoc.formId, + if (emailRecipients.length === 0) return okAsync([]) + return MailService.sendBounceNotification({ + emailRecipients, + bouncedRecipients: bounceDoc.getEmails(), + bounceType: bounceDoc.areAllPermanentBounces() + ? BounceType.Permanent + : BounceType.Transient, + formTitle: form.title, + formId: bounceDoc.formId, + }) + .map(() => emailRecipients) + .orElse((error) => { + // Log error, then return empty array as email was sent + logger.warn({ + message: 'Failed to send some bounce notification emails', + meta: { + action: 'notifyAdminOfBounce', + formId: form._id, + }, + error, + }) + return okAsync([]) }) - } +} - // Sms given recipients - const smsPromises = possibleSmsRecipients.map((recipient) => +/** + * Notifies admin and collaborators via SMS that response was lost. + * @param bounceDoc Document from Bounce collection + * @param form Form corresponding to the formId from bounceDoc + * @param possibleSmsRecipients Contact details of recipients to attempt to SMS + * @returns contact details for SMSes which were successfully sent. Note that + * this doesn't mean and SMSes were received, only that they were delivered + * to the carrier. + */ +export const sendSmsBounceNotification = ( + bounceDoc: IBounceSchema, + form: IPopulatedForm, + possibleSmsRecipients: UserWithContactNumber[], + // Returns no errors. If SMSes fail, returns + // empty array as list of recipients. +): ResultAsync => { + const smsResults = possibleSmsRecipients.map((recipient) => SmsFactory.sendBouncedSubmissionSms({ adminEmail: form.admin.email, adminId: form.admin._id, @@ -233,26 +295,30 @@ export const notifyAdminsOfBounce = async ( formTitle: form.title, recipient: recipient.contact, recipientEmail: recipient.email, - }), + }) + .map(() => recipient) + .mapErr( + (error) => new SendBounceSmsNotificationError(error, recipient.contact), + ), ) - - // neverthrow#combine is not used since we do not want to short circuit on the first error. - const smsResults = await Promise.all(smsPromises) - const successfulSmsRecipients = extractSuccessfulSmsRecipients( - smsResults, - possibleSmsRecipients, + return ( + combineWithAllErrors(smsResults) + // All succeeded + .map(() => possibleSmsRecipients) + .orElse((errors) => { + logger.warn({ + message: 'Failed to send some bounce notification SMSes', + meta: { + action: 'notifyAdminOfBounce', + formId: form._id, + errors, + }, + }) + return okAsync( + extractSuccessfulSmsRecipients(errors, possibleSmsRecipients), + ) + }) ) - if (successfulSmsRecipients.length < possibleSmsRecipients.length) { - logger.warn({ - message: 'Failed to send some bounce notification SMSes', - meta: { - action: 'notifyAdminOfBounce', - formId: form._id, - reasons: extractSmsErrors(smsResults), - }, - }) - } - return { emailRecipients, smsRecipients: successfulSmsRecipients } } /** @@ -290,15 +356,31 @@ export const logEmailNotification = ( * @param body The request body of the notification * @return the updated document from the Bounce collection or null if there are missing headers. */ -export const getUpdatedBounceDoc = async ( +export const getUpdatedBounceDoc = ( notification: IEmailNotification, -): Promise => { +): ResultAsync< + IBounceSchema, + MissingEmailHeadersError | PossibleDatabaseError +> => { const formId = extractHeader(notification, EMAIL_HEADERS.formId) - if (!formId) return null - const oldBounces = await Bounce.findOne({ formId }) - return oldBounces - ? oldBounces.updateBounceInfo(notification) - : Bounce.fromSnsNotification(notification, formId) + if (!formId) return errAsync(new MissingEmailHeadersError()) + return ResultAsync.fromPromise(Bounce.findOne({ formId }).exec(), (error) => { + logger.error({ + message: 'Error while retrieving Bounce document', + meta: { + action: 'getUpdatedBounceDoc', + formId, + }, + }) + return transformMongoError(error) + }).map((bounceDoc) => { + // Doc already exists for this form, so update it with latest info + if (bounceDoc) { + return bounceDoc.updateBounceInfo(notification) + } + // Create new doc from scratch + return Bounce.fromSnsNotification(notification, formId) + }) } /** @@ -319,30 +401,27 @@ export const extractEmailType = ( * @returns The contact details, filtered for the emails which have verified * contact numbers in the database */ -export const getEditorsWithContactNumbers = async ( +export const getEditorsWithContactNumbers = ( form: IPopulatedForm, -): Promise => { + // Never return an error. If database query fails, return empty array. +): ResultAsync => { const possibleEditors = [ form.admin.email, ...getCollabEmailsWithPermission(form.permissionList, true), ] - const smsRecipientsResult = await UserService.findContactsForEmails( - possibleEditors, - ) - if (smsRecipientsResult.isOk()) { - return smsRecipientsResult.value.filter( - (r) => !!r.contact, - ) as UserWithContactNumber[] - } else { - logger.warn({ - message: 'Failed to retrieve contact numbers for form editors', - meta: { - action: 'getEditorsWithContactNumbers', - formId: form._id, - }, + return UserService.findContactsForEmails(possibleEditors) + .map((editors) => editors.filter(isUserWithContactNumber)) + .orElse((error) => { + logger.warn({ + message: 'Failed to retrieve contact numbers for form editors', + meta: { + action: 'getEditorsWithContactNumbers', + formId: form._id, + }, + error, + }) + return okAsync([]) }) - return [] - } } /** @@ -352,11 +431,12 @@ export const getEditorsWithContactNumbers = async ( * @param possibleSmsRecipients Recipients to attempt to notify * @returns true regardless of the outcome */ -export const notifyAdminsOfDeactivation = async ( +export const notifyAdminsOfDeactivation = ( form: IPopulatedForm, possibleSmsRecipients: UserWithContactNumber[], -): Promise => { - const smsPromises = possibleSmsRecipients.map((recipient) => + // Best-effort attempt to send SMSes, don't propagate error upwards +): ResultAsync => { + const smsResults = possibleSmsRecipients.map((recipient) => SmsFactory.sendFormDeactivatedSms({ adminEmail: form.admin.email, adminId: form.admin._id, @@ -366,19 +446,65 @@ export const notifyAdminsOfDeactivation = async ( recipientEmail: recipient.email, }), ) - - // neverthrow#combine is not used since we do not want to short circuit on the first error. - const smsResults = await Promise.all(smsPromises) - const smsErrors = extractSmsErrors(smsResults) - if (smsErrors.length > 0) { - logger.warn({ - message: 'Failed to send some form deactivation notification SMSes', - meta: { - action: 'notifyAdminsOfDeactivation', - formId: form._id, - reasons: smsErrors, - }, + return combineWithAllErrors(smsResults) + .map(() => true as const) + .orElse((errors) => { + logger.warn({ + message: 'Failed to send some form deactivation notification SMSes', + meta: { + action: 'notifyAdminsOfDeactivation', + formId: form._id, + errors, + }, + }) + return okAsync(true) }) - } - return true +} + +/** + * Saves a document to the database. + * @param bounceDoc Bounce document + * @returns The saved document + */ +export const saveBounceDoc = ( + bounceDoc: IBounceSchema, +): ResultAsync => { + return ResultAsync.fromPromise(bounceDoc.save(), (error) => { + // Accept the risk that there might be concurrency problems + // when multiple server instances try to access the same + // document, due to notifications arriving asynchronously. + // Hence avoid logging so logs do not get polluted + if (!(error instanceof mongoose.Error.VersionError)) { + logger.warn({ + message: 'Error while saving Bounce document', + meta: { + action: 'saveBounceDoc', + formId: bounceDoc.formId, + }, + }) + } + return transformMongoError(error) + }) +} + +/** + * Safely parses an SNS notification. + * @param message Content of notification + */ +export const safeParseNotification = ( + message: string, +): Result => { + return Result.fromThrowable( + () => JSON.parse(message), + (error) => { + logger.warn({ + message: 'Unable to parse SNS notification', + meta: { + action: 'safeParseNotification', + }, + error, + }) + return new ParseNotificationError() + }, + )() } diff --git a/src/app/modules/bounce/bounce.util.ts b/src/app/modules/bounce/bounce.util.ts index fb88898427..f378275c6f 100644 --- a/src/app/modules/bounce/bounce.util.ts +++ b/src/app/modules/bounce/bounce.util.ts @@ -1,13 +1,12 @@ -import { Result } from 'neverthrow' - import { IBounceNotification, IDeliveryNotification, IEmailNotification, } from '../../../types' -import { InvalidNumberError, SmsSendError } from '../../services/sms/sms.errors' +import { UserWithContactNumber } from '../user/user.types' + +import { SendBounceSmsNotificationError } from './bounce.errors' -import { UserWithContactNumber } from './bounce.types' /** * Extracts custom headers which we send with all emails, such as form ID, submission ID * and email type (admin response, email confirmation OTP etc). @@ -70,39 +69,17 @@ export const isDeliveryNotification = ( /** * Filters the given SMS recipients to only those which were sent succesfully - * @param smsResults Array of sms results - * @param smsRecipients Recipients who were SMSed. This array must correspond - * exactly to smsResults, i.e. the result at smsResults[i] corresponds - * to the result of the attempt to SMS smsRecipients[i] + * @param smsErrors Array of sms errors + * @param smsRecipients Recipients who were SMSed * @returns the contact details of SMSes sent successfully */ export const extractSuccessfulSmsRecipients = ( - smsResults: Result[], + smsErrors: SendBounceSmsNotificationError[], smsRecipients: UserWithContactNumber[], ): UserWithContactNumber[] => { - return smsResults.reduce((acc, result, index) => { - if (result.isOk()) { - acc.push(smsRecipients[index]) - } - return acc - }, []) -} - -/** - * Extracts the errors from results of attempting to send SMSes - * @param smsResults Array of Promise.allSettled results - * @returns Array of errors - */ -export const extractSmsErrors = ( - smsResults: Result[], -): (SmsSendError | InvalidNumberError)[] => { - return smsResults.reduce<(SmsSendError | InvalidNumberError)[]>( - (acc, result) => { - if (result.isErr()) { - acc.push(result.error) - } - return acc - }, - [], + // Get recipients which errored + const failedRecipients = smsErrors.map((error) => error.meta.contact) + return smsRecipients.filter( + (recipient) => !failedRecipients.includes(recipient.contact), ) } diff --git a/src/app/modules/bounce/bounce.types.ts b/src/app/modules/user/user.types.ts similarity index 59% rename from src/app/modules/bounce/bounce.types.ts rename to src/app/modules/user/user.types.ts index 0246f4c1e1..8800101055 100644 --- a/src/app/modules/bounce/bounce.types.ts +++ b/src/app/modules/user/user.types.ts @@ -3,8 +3,3 @@ import { SetRequired } from 'type-fest' import { UserContactView } from '../../../types' export type UserWithContactNumber = SetRequired - -export interface AdminNotificationResult { - emailRecipients: string[] - smsRecipients: UserWithContactNumber[] -} diff --git a/src/app/modules/user/user.utils.ts b/src/app/modules/user/user.utils.ts index 5b09487b5c..d9026359e6 100644 --- a/src/app/modules/user/user.utils.ts +++ b/src/app/modules/user/user.utils.ts @@ -1,12 +1,14 @@ import { StatusCodes } from 'http-status-codes' import { createLoggerWithLabel } from '../../../config/logger' +import { UserContactView } from '../../../types' import * as SmsErrors from '../../services/sms/sms.errors' import { HashingError } from '../../utils/hash' import * as CoreErrors from '../core/core.errors' import { ErrorResponseData } from '../core/core.types' import * as UserErrors from './user.errors' +import { UserWithContactNumber } from './user.types' const logger = createLoggerWithLabel(module) /** @@ -58,3 +60,15 @@ export const mapRouteError = ( } } } + +/** + * Checks for presence of contact number in a user's contact + * details. Type guard. + * @param userDetails Contact view of user + * @returns True if user has a contact number + */ +export const isUserWithContactNumber = ( + userDetails: UserContactView, +): userDetails is UserWithContactNumber => { + return !!userDetails.contact +}