feat: enforce max 10 OTP requests for verified field (#3952)

* feat: enforce max 10 OTP requests for verified field

* test: update tests

* chore: update error message

shared/utils/verification.ts

@@ -5,6 +5,8 @@ export const SALT_ROUNDS = 10
 export const TRANSACTION_EXPIRE_AFTER_SECONDS = 14400 // 4 hours
 export const HASH_EXPIRE_AFTER_SECONDS = 60 * 30 // 30 minutes
 export const WAIT_FOR_OTP_SECONDS = 30
+export const MAX_OTP_REQUESTS = 10
+
 /**
  * WAIT_FOR_OTP_SECONDS tolerance. Server allows OTPs to be requested every
  * (WAIT_FOR_OTP_SECONDS - WAIT_FOR_OTP_TOLERANCE_SECONDS) seconds.

src/app/modules/verification/__tests__/verification.model.spec.ts

@@ -77,6 +77,7 @@ describe('Verification Model', () => {
         hashedOtp: 'hashedOtp',
         hashCreatedAt: new Date(),
         hashRetries: 5,
+        otpRequests: 6,
       }
       const vfnParams = merge({}, VFN_PARAMS, { fields: [field] })
       const verification = new VerificationModel(vfnParams)
@@ -457,6 +458,7 @@ describe('Verification Model', () => {
           hashedOtp: 'mockHashedOtp',
           hashCreatedAt: new Date(),
           hashRetries: 3,
+          otpRequests: 3,
         }
         const transaction = await VerificationModel.create({
           ...VFN_PARAMS,
@@ -477,6 +479,7 @@ describe('Verification Model', () => {
         expect(result!.fields[0].hashCreatedAt).not.toEqual(field.hashCreatedAt)
         expect(result!.fields[0].signedData).toEqual(updateParams.signedData)
         expect(result!.fields[0].hashedOtp).toEqual(updateParams.hashedOtp)
+        expect(result!.fields[0].otpRequests).toEqual(field.otpRequests + 1)
 
         expect(result!.formId).toEqual(transaction.formId)
       })
@@ -488,13 +491,15 @@ describe('Verification Model', () => {
           hashedOtp: 'mockHashedOtp',
           hashCreatedAt: new Date(),
           hashRetries: 3,
+          otpRequests: 1,
         }
         const field2 = {
           ...generateFieldParams(),
           signedData: 'mockSignedData2',
           hashedOtp: 'mockHashedOtp2',
           hashCreatedAt: new Date(),
           hashRetries: 2,
+          otpRequests: 3,
         }
         const transaction = await VerificationModel.create({
           ...VFN_PARAMS,
@@ -517,12 +522,14 @@ describe('Verification Model', () => {
         )
         expect(result!.fields[0].signedData).toEqual(updateParams.signedData)
         expect(result!.fields[0].hashedOtp).toEqual(updateParams.hashedOtp)
+        expect(result!.fields[0].otpRequests).toEqual(field1.otpRequests + 1)
 
         // field2 should remain completely unchanged
         expect(result!.fields[1].hashRetries).toEqual(field2.hashRetries)
         expect(result!.fields[1].signedData).toEqual(field2.signedData)
         expect(result!.fields[1].hashedOtp).toEqual(field2.hashedOtp)
         expect(result!.fields[1].hashCreatedAt).toEqual(field2.hashCreatedAt)
+        expect(result!.fields[1].otpRequests).toEqual(field2.otpRequests)
 
         expect(result!.formId).toEqual(transaction.formId)
       })

src/app/modules/verification/__tests__/verification.service.spec.ts

@@ -18,7 +18,10 @@ MockLoggerModule.createLoggerWithLabel.mockReturnValue(mockLogger)
 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 { OtpRequestError } from 'src/app/modules/verification/verification.errors'
+import {
+  OtpRequestCountExceededError,
+  OtpRequestError,
+} from 'src/app/modules/verification/verification.errors'
 import {
   MailGenerationError,
   MailSendError,
@@ -442,6 +445,36 @@ describe('Verification service', () => {
       expect(result._unsafeUnwrapErr()).toEqual(new WaitForOtpError())
     })
 
+    it('should return OtpRequestCountExceededError when OTP max requests are exceeded', async () => {
+      const maxExceededOtpField = generateFieldParams({
+        otpRequests: 11,
+      })
+      const maxExceededOtpTransaction = await VerificationModel.create({
+        formId: mockFormId,
+        // Expire 1 hour in future
+        expireAt: addHours(new Date(), 1),
+        fields: [maxExceededOtpField],
+      })
+
+      const result = await VerificationService.sendNewOtp({
+        transactionId: maxExceededOtpTransaction._id,
+        fieldId: maxExceededOtpField._id,
+        hashedOtp: MOCK_HASHED_OTP,
+        otp: MOCK_OTP,
+        recipient: MOCK_RECIPIENT,
+      })
+
+      expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled()
+      expect(MockSmsFactory.sendVerificationOtp).not.toHaveBeenCalled()
+      expect(
+        MockFormsgSdk.verification.generateSignature,
+      ).not.toHaveBeenCalled()
+      expect(updateHashSpy).not.toHaveBeenCalled()
+      expect(result._unsafeUnwrapErr()).toEqual(
+        new OtpRequestCountExceededError(),
+      )
+    })
+
     it('should forward errors returned by MailService.sendVerificationOtp', async () => {
       const error = new MailSendError()
       MockMailService.sendVerificationOtp.mockReturnValueOnce(errAsync(error))

src/app/modules/verification/__tests__/verification.test.helpers.ts

@@ -13,6 +13,7 @@ export const VFN_FIELD_DEFAULTS = {
   hashedOtp: null,
   hashCreatedAt: null,
   hashRetries: 0,
+  otpRequests: 0,
 }
 
 export const generateFieldParams = (

src/app/modules/verification/verification.errors.ts

@@ -72,6 +72,15 @@ export class WaitForOtpError extends ApplicationError {
   }
 }
 
+/**
+ * Max OTP request count exceeded
+ */
+export class OtpRequestCountExceededError extends ApplicationError {
+  constructor(message = 'Max OTP request count exceeded') {
+    super(message)
+  }
+}
+
 /**
  * Unsupported field type for OTP verification
  */

src/app/modules/verification/verification.model.ts

@@ -30,7 +30,10 @@ const VerificationFieldSchema = new Schema<IVerificationFieldSchema>({
   // No ttl index is applied on hashCreatedAt, as we do not want to delete the
   // entire document when a hash expires.
   hashCreatedAt: { type: Date, default: null },
+  // Number of retries attempted for a given OTP
   hashRetries: { type: Number, default: 0 },
+  // Number of OTPs requested for this field
+  otpRequests: { type: Number, default: 0 },
 })
 
 const compileVerificationModel = (db: Mongoose): IVerificationModel => {
@@ -165,6 +168,9 @@ const compileVerificationModel = (db: Mongoose): IVerificationModel => {
           'fields.$.signedData': updateData.signedData,
           'fields.$.hashRetries': 0,
         },
+        $inc: {
+          'fields.$.otpRequests': 1,
+        },
       },
       {
         new: true,

src/app/modules/verification/verification.service.ts

@@ -40,6 +40,7 @@ import {
   MissingHashDataError,
   NonVerifiedFieldTypeError,
   OtpExpiredError,
+  OtpRequestCountExceededError,
   OtpRequestError,
   OtpRetryExceededError,
   TransactionExpiredError,
@@ -52,6 +53,7 @@ import { SendOtpParams } from './verification.types'
 import {
   hasAdminExceededFreeSmsLimit,
   isOtpExpired,
+  isOtpRequestCountExceeded,
   isOtpWaitTimeElapsed,
   isTransactionExpired,
 } from './verification.util'
@@ -278,6 +280,7 @@ export const resetFieldForTransaction = (
  * @returns err(TransactionExpiredError) when transaction is expired
  * @returns err(FieldNotFoundInTransactionError) when field does not exist
  * @returns err(WaitForOtpError) when waiting time for new OTP has not elapsed
+ * @returns err(OtpRequestCountExceededError) when max OTP requests has been exceeded
  * @returns err(MalformedParametersError) when form data to send SMS OTP cannot be retrieved
  * @returns err(SmsSendError) when attempt to send OTP SMS fails
  * @returns err(InvalidNumberError) when SMS recipient is invalid
@@ -298,6 +301,7 @@ export const sendNewOtp = ({
   | FieldNotFoundInTransactionError
   | TransactionExpiredError
   | WaitForOtpError
+  | OtpRequestCountExceededError
   | MalformedParametersError
   | SmsSendError
   | InvalidNumberError
@@ -322,6 +326,14 @@ export const sendNewOtp = ({
           return errAsync(new WaitForOtpError())
         }
 
+        if (isOtpRequestCountExceeded(field.otpRequests)) {
+          logger.warn({
+            message: 'Max OTP request count exceeded',
+            meta: logMeta,
+          })
+          return errAsync(new OtpRequestCountExceededError())
+        }
+
         return sendOtpForField(transaction.formId, field, recipient, otp)
       })
       .andThen(() => {

src/app/modules/verification/verification.util.ts

@@ -2,6 +2,7 @@ import { StatusCodes } from 'http-status-codes'
 
 import {
   HASH_EXPIRE_AFTER_SECONDS,
+  MAX_OTP_REQUESTS,
   VERIFIED_FIELDTYPES,
   WAIT_FOR_OTP_SECONDS,
   WAIT_FOR_OTP_TOLERANCE_SECONDS,
@@ -14,6 +15,7 @@ import {
 import { smsConfig } from '../../config/features/sms.config'
 import { createLoggerWithLabel } from '../../config/logger'
 import {
+  OtpRequestCountExceededError,
   OtpRequestError,
   SmsLimitExceededError,
 } from '../../modules/verification/verification.errors'
@@ -124,6 +126,16 @@ export const isOtpWaitTimeElapsed = (hashCreatedAt: Date | null): boolean => {
   return currentDate > elapseAt
 }
 
+/**
+ * Computes whether the number of OTPs requested has exceeded the maximum allowed.
+ * @param otpRequests Number of OTPs already requested
+ * @returns true if the number of OTPs has exceeded the maximum allowed
+ */
+export const isOtpRequestCountExceeded = (otpRequests: number): boolean => {
+  // Use >= because otpRequests is the number of OTPs previously requested
+  return otpRequests >= MAX_OTP_REQUESTS
+}
+
 export const mapRouteError: MapRouteError = (
   error,
   coreErrorMsg = 'Sorry, something went wrong. Please refresh and try again.',
@@ -155,6 +167,11 @@ export const mapRouteError: MapRouteError = (
         errorMessage: `You must wait for ${WAIT_FOR_OTP_SECONDS} seconds between each OTP request.`,
         statusCode: StatusCodes.UNPROCESSABLE_ENTITY,
       }
+    case OtpRequestCountExceededError:
+      return {
+        errorMessage: `You have requested too many OTPs. Please refresh and try again.`,
+        statusCode: StatusCodes.BAD_REQUEST,
+      }
     case InvalidNumberError:
       return {
         errorMessage:

src/types/verification.ts

@@ -11,6 +11,7 @@ export interface IVerificationField {
   hashedOtp: string | null
   hashCreatedAt: Date | null
   hashRetries: number
+  otpRequests: number
 }
 
 export interface IVerificationFieldSchema