fix(otp): increment instead of update for concurrency (#1186)

* fix(otp): increment instead of update for concurrency

* ref(otp): migrate to use neverthrow instead

* chore: remove unneeded check for empty string

src/integration/Users.spec.ts

@@ -377,12 +377,10 @@ describe("Users Router", () => {
 
         if (i <= maxNumOfOtpAttempts) {
           expect(otpEntry?.attempts).toBe(i)
-          expect(actual.body.error.message).toBe("OTP is not valid")
+          expect(actual.body.message).toBe("OTP is not valid")
         } else {
           expect(otpEntry?.attempts).toBe(maxNumOfOtpAttempts)
-          expect(actual.body.error.message).toBe(
-            "Max number of attempts reached"
-          )
+          expect(actual.body.message).toBe("Max number of attempts reached")
         }
       }
     })
@@ -645,12 +643,10 @@ describe("Users Router", () => {
 
         if (i <= maxNumOfOtpAttempts) {
           expect(otpEntry?.attempts).toBe(i)
-          expect(actual.body.error.message).toBe("OTP is not valid")
+          expect(actual.body.message).toBe("OTP is not valid")
         } else {
           expect(otpEntry?.attempts).toBe(maxNumOfOtpAttempts)
-          expect(actual.body.error.message).toBe(
-            "Max number of attempts reached"
-          )
+          expect(actual.body.message).toBe("Max number of attempts reached")
         }
       }
     })

src/routes/v2/authenticated/users.ts

@@ -1,5 +1,6 @@
 import autoBind from "auto-bind"
 import express from "express"
+import { ResultAsync } from "neverthrow"
 import validator from "validator"
 
 import logger from "@logger/logger"
@@ -10,6 +11,7 @@ import { attachReadRouteHandlerWrapper } from "@middleware/routeHandler"
 
 import UserSessionData from "@classes/UserSessionData"
 
+import DatabaseError from "@root/errors/DatabaseError"
 import { isError, RequestHandler } from "@root/types"
 import UsersService from "@services/identity/UsersService"
 
@@ -70,13 +72,30 @@ export class UsersRouter {
     const userId = userSessionData.isomerUserId
     const parsedEmail = email.toLowerCase()
 
-    const isOtpValid = await this.usersService.verifyEmailOtp(parsedEmail, otp)
-    if (!isOtpValid) {
-      throw new BadRequestError("Invalid OTP")
-    }
-
-    await this.usersService.updateUserByIsomerId(userId, { email: parsedEmail })
-    res.sendStatus(200)
+    return this.usersService
+      .verifyEmailOtp(parsedEmail, otp)
+      .andThen(() =>
+        ResultAsync.fromPromise(
+          this.usersService.updateUserByIsomerId(userId, {
+            email: parsedEmail,
+          }),
+          (error) => {
+            logger.error(
+              `Error updating user email by Isomer ID: ${JSON.stringify(error)}`
+            )
+            return new DatabaseError(
+              "An error occurred when updating the database"
+            )
+          }
+        )
+      )
+      .map(() => res.sendStatus(200))
+      .mapErr((error) => {
+        if (error instanceof BadRequestError) {
+          return res.status(400).json({ message: error.message })
+        }
+        return res.status(500).json({ message: error.message })
+      })
   }
 
   sendMobileNumberOtp: RequestHandler<
@@ -104,15 +123,32 @@ export class UsersRouter {
     const { userSessionData } = res.locals
     const userId = userSessionData.isomerUserId
 
-    const isOtpValid = await this.usersService.verifyMobileOtp(mobile, otp)
-    if (!isOtpValid) {
-      throw new BadRequestError("Invalid OTP")
-    }
-
-    await this.usersService.updateUserByIsomerId(userId, {
-      contactNumber: mobile,
-    })
-    return res.sendStatus(200)
+    return this.usersService
+      .verifyMobileOtp(mobile, otp)
+      .andThen(() =>
+        ResultAsync.fromPromise(
+          this.usersService.updateUserByIsomerId(userId, {
+            contactNumber: mobile,
+          }),
+          (error) => {
+            logger.error(
+              `Error updating user contact number by Isomer ID: ${JSON.stringify(
+                error
+              )}`
+            )
+            return new DatabaseError(
+              "An error occurred when updating the database"
+            )
+          }
+        )
+      )
+      .map(() => res.sendStatus(200))
+      .mapErr((error) => {
+        if (error instanceof BadRequestError) {
+          return res.status(400).json({ message: error.message })
+        }
+        return res.status(500).json({ message: error.message })
+      })
   }
 
   getRouter() {

src/services/identity/UsersService.ts

@@ -1,11 +1,14 @@
-import { Op, ModelStatic } from "sequelize"
+import { ResultAsync, errAsync, okAsync } from "neverthrow"
+import { Op, ModelStatic, Transaction } from "sequelize"
 import { Sequelize } from "sequelize-typescript"
 import { RequireAtLeastOne } from "type-fest"
 
 import { config } from "@config/config"
 
 import { Otp, Repo, Site, User, Whitelist, SiteMember } from "@database/models"
 import { BadRequestError } from "@root/errors/BadRequestError"
+import DatabaseError from "@root/errors/DatabaseError"
+import logger from "@root/logger/logger"
 import { milliSecondsToMinutes } from "@root/utils/time-utils"
 import SmsClient from "@services/identity/SmsClient"
 import MailClient from "@services/utilServices/MailClient"
@@ -230,56 +233,201 @@ class UsersService {
     await this.smsClient.sendSms(mobileNumber, message)
   }
 
-  private async verifyOtp(otpEntry: Otp | null, otp: string) {
+  private verifyOtp(
+    otpEntry: Otp | null,
+    otp: string,
+    transaction: Transaction
+  ) {
     // TODO: Change all the following to use AuthError after FE fix
-    if (!otp || otp === "") {
-      throw new BadRequestError("Empty OTP provided")
-    }
-
-    if (!otpEntry) {
-      throw new BadRequestError("OTP not found")
-    }
-
-    if (otpEntry.attempts >= MAX_NUM_OTP_ATTEMPTS) {
-      throw new BadRequestError("Max number of attempts reached")
-    }
-
-    if (!otpEntry?.hashedOtp) {
-      await otpEntry.destroy()
-      throw new BadRequestError("Hashed OTP not found")
-    }
+    return okAsync(otp)
+      .andThen((otp) => {
+        if (!otp) {
+          return errAsync(new BadRequestError("Empty OTP provided"))
+        }
 
-    // increment attempts
-    await otpEntry.update({ attempts: otpEntry.attempts + 1 })
-
-    const isValidOtp = await this.otpService.verifyOtp(otp, otpEntry.hashedOtp)
-    if (!isValidOtp) {
-      throw new BadRequestError("OTP is not valid")
-    }
+        return okAsync(otp)
+      })
+      .andThen(() => {
+        if (!otpEntry) {
+          return errAsync(new BadRequestError("OTP not found"))
+        }
 
-    if (isValidOtp && otpEntry.expiresAt < new Date()) {
-      await otpEntry.destroy()
-      throw new BadRequestError("OTP has expired")
-    }
+        return okAsync(otpEntry)
+      })
+      .andThen((otpDbEntry) => {
+        if (otpDbEntry.attempts >= MAX_NUM_OTP_ATTEMPTS) {
+          return errAsync(new BadRequestError("Max number of attempts reached"))
+        }
 
-    // destroy otp before returning true since otp has been "used"
-    await otpEntry.destroy()
-    return true
+        return okAsync(otpDbEntry)
+      })
+      .andThen((otpDbEntry) => {
+        if (!otpDbEntry.hashedOtp) {
+          return ResultAsync.fromPromise(
+            otpDbEntry.destroy({ transaction }),
+            (error) => {
+              logger.error(
+                `Error destroying OTP entry: ${JSON.stringify(error)}`
+              )
+
+              return new DatabaseError("Error destroying OTP entry in database")
+            }
+          ).andThen(() => errAsync(new BadRequestError("Hashed OTP not found")))
+        }
+
+        return okAsync(otpDbEntry)
+      })
+      .andThen((otpDbEntry) =>
+        // increment attempts
+        ResultAsync.fromPromise(
+          this.otpRepository.increment("attempts", {
+            where: { id: otpDbEntry.id },
+            transaction,
+          }),
+          (error) => {
+            logger.error(
+              `Error incrementing OTP attempts: ${JSON.stringify(error)}`
+            )
+
+            return new DatabaseError("Error incrementing OTP attempts")
+          }
+        ).map(() => otpDbEntry)
+      )
+      .andThen((otpDbEntry) =>
+        ResultAsync.combine([
+          okAsync(otpDbEntry),
+          ResultAsync.fromPromise(
+            this.otpService.verifyOtp(otp, otpDbEntry.hashedOtp),
+            (error) => {
+              logger.error(`Error verifying OTP: ${JSON.stringify(error)}`)
+
+              return new BadRequestError("Error verifying OTP")
+            }
+          ),
+        ])
+      )
+      .andThen(([otpDbEntry, isValidOtp]) => {
+        if (!isValidOtp) {
+          return errAsync(new BadRequestError("OTP is not valid"))
+        }
+
+        if (isValidOtp && otpDbEntry.expiresAt < new Date()) {
+          return ResultAsync.fromPromise(
+            otpDbEntry.destroy({ transaction }),
+            (error) => {
+              logger.error(
+                `Error destroying OTP entry: ${JSON.stringify(error)}`
+              )
+
+              return new DatabaseError("Error destroying OTP entry in database")
+            }
+          ).andThen(() => errAsync(new BadRequestError("OTP has expired")))
+        }
+
+        return okAsync(otpDbEntry)
+      })
+      .andThen((otpDbEntry) =>
+        // destroy otp before returning true since otp has been "used"
+        ResultAsync.fromPromise(
+          otpDbEntry.destroy({ transaction }),
+          (error) => {
+            logger.error(`Error destroying OTP entry: ${JSON.stringify(error)}`)
+
+            return new DatabaseError("Error destroying OTP entry in database")
+          }
+        )
+          .andThen(() =>
+            ResultAsync.fromPromise(transaction.commit(), (txError) => {
+              logger.error(
+                `Error committing transaction: ${JSON.stringify(txError)}`
+              )
+              return new DatabaseError("Error committing transaction")
+            })
+          )
+          .map(() => true)
+      )
+      .orElse((error) =>
+        ResultAsync.fromPromise(transaction.commit(), (txError) => {
+          logger.error(
+            `Error committing transaction: ${JSON.stringify(txError)}`
+          )
+          return new DatabaseError("Error committing transaction")
+        }).andThen(() => errAsync(error))
+      )
   }
 
-  async verifyEmailOtp(email: string, otp: string) {
+  verifyEmailOtp(email: string, otp: string) {
     const parsedEmail = email.toLowerCase()
-    const otpEntry = await this.otpRepository.findOne({
-      where: { email: parsedEmail },
+
+    return ResultAsync.fromPromise(this.sequelize.transaction(), (error) => {
+      logger.error(
+        `Error starting database transaction: ${JSON.stringify(error)}`
+      )
+
+      return new BadRequestError("Error starting database transaction")
     })
-    return this.verifyOtp(otpEntry, otp)
+      .andThen((transaction) =>
+        ResultAsync.combine([
+          ResultAsync.fromPromise(
+            this.otpRepository.findOne({
+              where: { email: parsedEmail },
+              lock: true,
+              transaction,
+            }),
+            (error) => {
+              logger.error(
+                `Error finding OTP entry when verifying email OTP: ${JSON.stringify(
+                  error
+                )}`
+              )
+
+              return new BadRequestError(
+                "Error finding OTP entry when verifying email OTP"
+              )
+            }
+          ),
+          okAsync(transaction),
+        ])
+      )
+      .andThen(([otpEntry, transaction]) =>
+        this.verifyOtp(otpEntry, otp, transaction)
+      )
   }
 
-  async verifyMobileOtp(mobileNumber: string, otp: string) {
-    const otpEntry = await this.otpRepository.findOne({
-      where: { mobileNumber },
+  verifyMobileOtp(mobileNumber: string, otp: string) {
+    return ResultAsync.fromPromise(this.sequelize.transaction(), (error) => {
+      logger.error(
+        `Error starting database transaction: ${JSON.stringify(error)}`
+      )
+
+      return new BadRequestError("Error starting database transaction")
     })
-    return this.verifyOtp(otpEntry, otp)
+      .andThen((transaction) =>
+        ResultAsync.combine([
+          ResultAsync.fromPromise(
+            this.otpRepository.findOne({
+              where: { mobileNumber },
+              lock: true,
+              transaction,
+            }),
+            (error) => {
+              logger.error(
+                `Error finding OTP entry when verifying mobile OTP: ${JSON.stringify(
+                  error
+                )}`
+              )
+
+              return new BadRequestError(
+                "Error finding OTP entry when verifying mobile OTP"
+              )
+            }
+          ),
+          okAsync(transaction),
+        ])
+      )
+      .andThen(([otpEntry, transaction]) =>
+        this.verifyOtp(otpEntry, otp, transaction)
+      )
   }
 
   private getOtpExpiry() {