From e483cf1e6f343bf5a11245979663edb4fa704dc1 Mon Sep 17 00:00:00 2001 From: seaerchin Date: Fri, 13 Aug 2021 14:30:04 +0800 Subject: [PATCH 1/2] fix(verification): prevents otp request when limit exceeded --- .../__tests__/verification.controller.spec.ts | 25 +++++- .../__tests__/verification.service.spec.ts | 79 +++++++++++++++++++ .../verification/verification.controller.ts | 6 +- .../verification/verification.service.ts | 19 +++++ .../modules/verification/verification.util.ts | 7 ++ 5 files changed, 133 insertions(+), 3 deletions(-) diff --git a/src/app/modules/verification/__tests__/verification.controller.spec.ts b/src/app/modules/verification/__tests__/verification.controller.spec.ts index f2e8528396..9d9e8f9633 100644 --- a/src/app/modules/verification/__tests__/verification.controller.spec.ts +++ b/src/app/modules/verification/__tests__/verification.controller.spec.ts @@ -28,6 +28,7 @@ import { NonVerifiedFieldTypeError, OtpExpiredError, OtpRetryExceededError, + SmsLimitExceededError, TransactionExpiredError, TransactionNotFoundError, WaitForOtpError, @@ -613,7 +614,6 @@ describe('Verification controller', () => { fieldId: MOCK_FIELD_ID, }, }) - const MOCK_FORM = { admin: { _id: new ObjectId(), @@ -640,6 +640,7 @@ describe('Verification controller', () => { MockVerificationService.sendNewOtp.mockReturnValue( okAsync(mockTransaction), ) + MockVerificationService.shouldGenerateOtp.mockReturnValue(okAsync(true)) }) it('should return 201 when params are valid', async () => { @@ -835,6 +836,28 @@ describe('Verification controller', () => { expect(mockRes.json).toHaveBeenCalledWith(expectedResponse) }) + it('should return 400 when sms limit has been exceeded and the form is not onboarded', async () => { + // Arrange + MockVerificationService.shouldGenerateOtp.mockReturnValueOnce( + errAsync(new SmsLimitExceededError()), + ) + const expected = { + message: + 'Sorry, this form is outdated. Please refresh your browser to get the latest version of the form', + } + + // Act + await VerificationController._handleGenerateOtp( + MOCK_REQ, + mockRes, + jest.fn(), + ) + + // Assert + expect(mockRes.status).toHaveBeenCalledWith(400) + expect(mockRes.json).toBeCalledWith(expected) + }) + it('should return 400 when field type is not verifiable', async () => { // Arrange MockVerificationService.sendNewOtp.mockReturnValueOnce( diff --git a/src/app/modules/verification/__tests__/verification.service.spec.ts b/src/app/modules/verification/__tests__/verification.service.spec.ts index 74818f2bec..886c0b2275 100644 --- a/src/app/modules/verification/__tests__/verification.service.spec.ts +++ b/src/app/modules/verification/__tests__/verification.service.spec.ts @@ -7,6 +7,7 @@ import { mocked } from 'ts-jest/utils' import { smsConfig } from 'src/app/config/features/sms.config' import formsgSdk from 'src/app/config/formsg-sdk' import * as FormService from 'src/app/modules/form/form.service' +import { SmsLimitExceededError } from 'src/app/modules/verification/verification.errors' import { MailGenerationError, MailSendError, @@ -839,4 +840,82 @@ describe('Verification service', () => { expect(retrievalSpy).toBeCalledWith(String(MOCK_FORM.admin._id)) }) }) + + describe('shouldGenerateOtp', () => { + it('should return true when sms counts is less than limit', async () => { + // Arrange + const MOCK_FORM = { + admin: { + _id: 'something', + }, + } + const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') + retrieveSpy.mockReturnValueOnce( + okAsync(smsConfig.smsVerificationLimit - 1), + ) + + // Act + const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM) + + // Assert + expect(actual._unsafeUnwrap()).toBe(true) + expect(retrieveSpy).toBeCalledWith(MOCK_FORM.admin._id) + }) + + it('should return true when the form is onboarded', async () => { + // Arrange + const MOCK_FORM = { + msgSrvcName: 'somename', + admin: { + _id: 'something', + }, + } + const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') + + // Act + const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM) + + // Assert + expect(actual._unsafeUnwrap()).toBe(true) + expect(retrieveSpy).not.toBeCalled() + }) + + it('should return SmsLimitExceeded error when sms counts exceeds limit and form is not onboarded', async () => { + // Arrange + const MOCK_FORM = { + admin: { + _id: 'something', + }, + } + const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') + retrieveSpy.mockReturnValueOnce( + okAsync(smsConfig.smsVerificationLimit + 1), + ) + + // Act + const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM) + + // Assert + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(SmsLimitExceededError) + expect(retrieveSpy).toBeCalledWith(MOCK_FORM.admin._id) + }) + + it('should propagate any errors during sms counts retrieval', async () => { + // Arrange + const MOCK_FORM = { + admin: { + _id: 'something', + }, + } + const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') + retrieveSpy.mockReturnValueOnce(errAsync(new DatabaseError())) + + // Act + const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM) + + // Assert + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(DatabaseError) + expect(retrieveSpy).toBeCalledWith(MOCK_FORM.admin._id) + }) + }) }) diff --git a/src/app/modules/verification/verification.controller.ts b/src/app/modules/verification/verification.controller.ts index 0741fd4a4f..0ba3ab7eeb 100644 --- a/src/app/modules/verification/verification.controller.ts +++ b/src/app/modules/verification/verification.controller.ts @@ -209,10 +209,12 @@ export const _handleGenerateOtp: ControllerHandler< // Step 1: Ensure that the form for the specified transaction exists return ( FormService.retrieveFormById(formId) - // Step 2: Generate hash and otp + // Step 2: Check if we should allow public user to request for OTP + .andThen((form) => VerificationService.shouldGenerateOtp(form)) + // Step 3: Generate hash and otp .andThen(() => generateOtpWithHash(logMeta, SALT_ROUNDS)) .andThen(({ otp, hashedOtp }) => - // Step 3: Send otp + // Step 4: Send otp VerificationService.sendNewOtp({ fieldId, hashedOtp, diff --git a/src/app/modules/verification/verification.service.ts b/src/app/modules/verification/verification.service.ts index c34383d0f9..2af5428bf9 100644 --- a/src/app/modules/verification/verification.service.ts +++ b/src/app/modules/verification/verification.service.ts @@ -7,6 +7,7 @@ import { } from '../../../../shared/utils/verification' import { BasicField, + IFormSchema, IPopulatedForm, IVerificationFieldSchema, IVerificationSchema, @@ -40,6 +41,7 @@ import { NonVerifiedFieldTypeError, OtpExpiredError, OtpRetryExceededError, + SmsLimitExceededError, TransactionExpiredError, TransactionNotFoundError, WaitForOtpError, @@ -546,3 +548,20 @@ const checkSmsCountAndPerformAction = ( return okAsync(true) } + +export const shouldGenerateOtp = ({ + msgSrvcName, + admin, +}: Pick): ResultAsync< + true, + SmsLimitExceededError | PossibleDatabaseError +> => { + if (msgSrvcName) { + return okAsync(true) + } + return SmsService.retrieveFreeSmsCounts(admin._id).andThen((counts) => + hasAdminExceededFreeSmsLimit(counts) + ? errAsync(new SmsLimitExceededError()) + : okAsync(true), + ) +} diff --git a/src/app/modules/verification/verification.util.ts b/src/app/modules/verification/verification.util.ts index b47478817c..58bf0073f9 100644 --- a/src/app/modules/verification/verification.util.ts +++ b/src/app/modules/verification/verification.util.ts @@ -13,6 +13,7 @@ import { } from '../../../types' import { smsConfig } from '../../config/features/sms.config' import { createLoggerWithLabel } from '../../config/logger' +import { SmsLimitExceededError } from '../../modules/verification/verification.errors' import { MailSendError } from '../../services/mail/mail.errors' import { InvalidNumberError, SmsSendError } from '../../services/sms/sms.errors' import { HashingError } from '../../utils/hash' @@ -191,6 +192,12 @@ export const mapRouteError: MapRouteError = ( errorMessage: coreErrorMsg, statusCode: StatusCodes.NOT_FOUND, } + case SmsLimitExceededError: + return { + errorMessage: + 'Sorry, this form is outdated. Please refresh your browser to get the latest version of the form', + statusCode: StatusCodes.BAD_REQUEST, + } case HashingError: case DatabaseError: return { From 4e32036dacc64b7a878d8d38ce90d6e742fc37b3 Mon Sep 17 00:00:00 2001 From: seaerchin Date: Fri, 13 Aug 2021 15:00:11 +0800 Subject: [PATCH 2/2] refactor(verification): updated check to see if verifiable mobile fields exists --- .../__tests__/verification.service.spec.ts | 32 ++++++++++++++++++- .../verification/verification.errors.ts | 11 +++++++ .../verification/verification.service.ts | 18 ++++++++++- 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/app/modules/verification/__tests__/verification.service.spec.ts b/src/app/modules/verification/__tests__/verification.service.spec.ts index 886c0b2275..8fe55ed5e4 100644 --- a/src/app/modules/verification/__tests__/verification.service.spec.ts +++ b/src/app/modules/verification/__tests__/verification.service.spec.ts @@ -7,7 +7,10 @@ import { mocked } from 'ts-jest/utils' import { smsConfig } from 'src/app/config/features/sms.config' import formsgSdk from 'src/app/config/formsg-sdk' import * as FormService from 'src/app/modules/form/form.service' -import { SmsLimitExceededError } from 'src/app/modules/verification/verification.errors' +import { + OtpRequestError, + SmsLimitExceededError, +} from 'src/app/modules/verification/verification.errors' import { MailGenerationError, MailSendError, @@ -26,6 +29,7 @@ import { UpdateFieldData, } from 'src/types' +import { generateDefaultField } from 'tests/unit/backend/helpers/generate-form-data' import dbHandler from 'tests/unit/backend/helpers/jest-db' import { SMS_WARNING_TIERS } from '../../../../../shared/utils/verification' @@ -848,6 +852,9 @@ describe('Verification service', () => { admin: { _id: 'something', }, + form_fields: [ + generateDefaultField(BasicField.Mobile, { isVerifiable: true }), + ], } const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') retrieveSpy.mockReturnValueOnce( @@ -886,6 +893,9 @@ describe('Verification service', () => { admin: { _id: 'something', }, + form_fields: [ + generateDefaultField(BasicField.Mobile, { isVerifiable: true }), + ], } const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') retrieveSpy.mockReturnValueOnce( @@ -900,12 +910,32 @@ describe('Verification service', () => { expect(retrieveSpy).toBeCalledWith(MOCK_FORM.admin._id) }) + it('should return OtpRequestError when an OTP is requested on a form without OTP enabled', async () => { + // Arrange + const MOCK_FORM = { + admin: { + _id: 'something', + }, + } + const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') + + // Act + const actual = await VerificationService.shouldGenerateOtp(MOCK_FORM) + + // Assert + expect(actual._unsafeUnwrapErr()).toBeInstanceOf(OtpRequestError) + expect(retrieveSpy).not.toBeCalled() + }) + it('should propagate any errors during sms counts retrieval', async () => { // Arrange const MOCK_FORM = { admin: { _id: 'something', }, + form_fields: [ + generateDefaultField(BasicField.Mobile, { isVerifiable: true }), + ], } const retrieveSpy = jest.spyOn(SmsService, 'retrieveFreeSmsCounts') retrieveSpy.mockReturnValueOnce(errAsync(new DatabaseError())) diff --git a/src/app/modules/verification/verification.errors.ts b/src/app/modules/verification/verification.errors.ts index 555add4134..0061add700 100644 --- a/src/app/modules/verification/verification.errors.ts +++ b/src/app/modules/verification/verification.errors.ts @@ -93,3 +93,14 @@ export class SmsLimitExceededError extends ApplicationError { super(message) } } + +/** + * Public user attempts to request for an OTP on a form without OTP enabled. + */ +export class OtpRequestError extends ApplicationError { + constructor( + message = 'Please ensure that the form you are trying to request OTP for has the feature enabled.', + ) { + super(message) + } +} diff --git a/src/app/modules/verification/verification.service.ts b/src/app/modules/verification/verification.service.ts index 2af5428bf9..4e1fbc754a 100644 --- a/src/app/modules/verification/verification.service.ts +++ b/src/app/modules/verification/verification.service.ts @@ -40,6 +40,7 @@ import { MissingHashDataError, NonVerifiedFieldTypeError, OtpExpiredError, + OtpRequestError, OtpRetryExceededError, SmsLimitExceededError, TransactionExpiredError, @@ -552,13 +553,28 @@ const checkSmsCountAndPerformAction = ( export const shouldGenerateOtp = ({ msgSrvcName, admin, -}: Pick): ResultAsync< + form_fields, +}: Pick): ResultAsync< true, SmsLimitExceededError | PossibleDatabaseError > => { + // This check is here to ensure that programmatic pings are rejected. + // If the check is solely on whether the form is onboarded, + // Pings to form that are not onboarded will go through + // Even if the form has no mobile field to verify. + const hasVerifiableMobileFields = form_fields?.filter( + ({ fieldType, isVerifiable }) => + fieldType === BasicField.Mobile && isVerifiable, + ) + if (msgSrvcName) { return okAsync(true) } + + if (!hasVerifiableMobileFields) { + return errAsync(new OtpRequestError()) + } + return SmsService.retrieveFreeSmsCounts(admin._id).andThen((counts) => hasAdminExceededFreeSmsLimit(counts) ? errAsync(new SmsLimitExceededError())