Skip to content

Commit

Permalink
Fix + Refactor: improve verify otp flows (#1295)
Browse files Browse the repository at this point in the history 
* fix: dont start transaction if input is invalid

* refactor(verifyOtp): dont start transaction if not needed + reduce code

* fix: typo in comment

* feat(verifyOtp): stop using transactions

* feat(verifyOtp): remove lock:true from findOne()

* feature(verifyOtp): reduce code by optimizing the query

* fix: ensure incrementing attempts os robust against race conditions
  • Loading branch information
@timotheeg
timotheeg authored Apr 18, 2024
1 parent 57d8188 commit 4e6bc5e
Showing 1 changed file with 126 additions and 172 deletions.
298 changes: 126 additions & 172 deletions src/services/identity/UsersService.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { ResultAsync, errAsync, okAsync } from "neverthrow"
import { Op, ModelStatic, Transaction, QueryTypes } from "sequelize"
import { ResultAsync, errAsync } from "neverthrow"
import { Op, ModelStatic, QueryTypes } from "sequelize"
import { Sequelize } from "sequelize-typescript"
import { RequireAtLeastOne } from "type-fest"

import { config } from "@config/config"

import { BaseIsomerError } from "@errors/BaseError"

import { Otp, Repo, Site, User, Whitelist, SiteMember } from "@database/models"
import { BadRequestError } from "@root/errors/BadRequestError"
import DatabaseError from "@root/errors/DatabaseError"
Expand All @@ -23,6 +25,8 @@ enum OtpType {
Mobile = "MOBILE",
}

type Class<T> = new (...args: any[]) => T

interface UsersServiceProps {
mailer: MailClient
smsClient: SmsClient
Expand Down Expand Up @@ -240,201 +244,151 @@ class UsersService {
await this.smsClient.sendSms(mobileNumber, message)
}

private verifyOtp(
otpEntry: Otp | null,
otp: string,
transaction: Transaction
) {
// TODO: Change all the following to use AuthError after FE fix
return okAsync(otp)
.andThen((otp) => {
if (!otp) {
return errAsync(new BadRequestError("Empty OTP provided"))
}
private otpGetAndLogError<T extends BaseIsomerError>(
ErrorClass: Class<T>,
cause: unknown,
message: string
): T {
logger.error({
error: cause,
message,
})

return okAsync(otp)
})
.andThen(() => {
return new ErrorClass(message)
}

private otpDestroyEntry(otpEntry: Otp) {
return ResultAsync.fromPromise(otpEntry.destroy(), (error) =>
this.otpGetAndLogError(DatabaseError, error, `Error destroying OTP entry`)
)
}

private verifyOtp({
otp,
findConditions,
findErrorMessage,
}: {
otp: string | undefined
findConditions: { email: string } | { mobileNumber: string }
findErrorMessage: string
}) {
if (!otp) {
return errAsync(new BadRequestError("Empty OTP provided"))
}

// local variables that can be referenced for convenience, instead of having the steps of the promise chain carry them each
// time with AsyncResult.combine() wrappers (which makes the code har to read)
let otpEntry: Otp | null = null

// TypeScript can't tell when otpEntry is guaranteed to not be null in the promise chain steps
// So we'll provide non-null assertions ourselves, and we need to tell eslint to leave us alone -_-
/* eslint-disable @typescript-eslint/no-non-null-assertion */
return ResultAsync.fromPromise(
this.otpRepository.findOne({
where: {
...findConditions,
hashedOtp: {
[Op.regexp]: "\\S+", // at least one non-whitespace character (i.e. is truthy!)
},
},
}),
(error) => this.otpGetAndLogError(DatabaseError, error, findErrorMessage)
)
.andThen((_otpEntry: Otp | null) => {
otpEntry = _otpEntry // store otpEntry in outer scope, so it's easier to access it in the local promise chain steps

// verify that otpDbEntry exists
if (!otpEntry) {
return errAsync(new BadRequestError("OTP not found"))
}

return okAsync(otpEntry)
})
.andThen((otpDbEntry) => {
if (otpDbEntry.attempts >= MAX_NUM_OTP_ATTEMPTS) {
return errAsync(new BadRequestError("Max number of attempts reached"))
}
// after this point, otpEntry is guaranteed to be truthy is all promise chain steps

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")))
// verify otpEntry validity

if (otpEntry.expiresAt < new Date()) {
return this.otpDestroyEntry(otpEntry!).andThen(() =>
errAsync(new BadRequestError("OTP has expired"))
)
}

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)}`
)
if (otpEntry.attempts >= MAX_NUM_OTP_ATTEMPTS) {
// should this delete the otpEntry as well?
return errAsync(new BadRequestError("Max number of attempts reached"))
}

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")
// We must successfully be able to increment the otp record attempts before any processing, to prevent brute-force race condition
// Consult GTA-24-012 WP3 for details
return ResultAsync.fromPromise(
this.otpRepository.update(
{
attempts: otpEntry.attempts + 1,
},
{
where: {
id: otpEntry.id,
attempts: otpEntry.attempts, // required to ensure the record has not been modified in parallel
},
}
),
])
)
.andThen(([otpDbEntry, isValidOtp]) => {
if (!isValidOtp) {
return errAsync(new BadRequestError("OTP is not valid"))
(error) =>
this.otpGetAndLogError(
DatabaseError,
error,
"Error incrementing OTP attempts"
)
)
})
.andThen(([numAffectedRows]) => {
if (numAffectedRows <= 0) {
// Record could not be updated. It was likely changed in parallel by another request, we must fail this verification attempt now
return errAsync(
new BadRequestError("Unable to increment OTP attempts")
)
}

if (isValidOtp && otpDbEntry.expiresAt < new Date()) {
return ResultAsync.fromPromise(
otpDbEntry.destroy({ transaction }),
(error) => {
logger.error(
`Error destroying OTP entry: ${JSON.stringify(error)}`
)
// Note/Warning: after this step, otpEntry.attempts does not have the value that is in DB (it is one less!)
// It's OK because we no longer reference otpEntry.attempts in this flow.

return new DatabaseError("Error destroying OTP entry in database")
}
).andThen(() => errAsync(new BadRequestError("OTP has expired")))
return ResultAsync.fromPromise(
this.otpService.verifyOtp(otp, otpEntry!.hashedOtp),
(error) =>
this.otpGetAndLogError(
BadRequestError,
error,
"Error verifying OTP"
)
)
})
.andThen((isValidOtp) => {
if (!isValidOtp) {
return errAsync(new BadRequestError("OTP is not valid"))
}

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 this.otpDestroyEntry(otpEntry!)
})
.map(() => true)

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))
)
/* eslint-enable @typescript-eslint/no-non-null-assertion */
}

verifyEmailOtp(email: string, otp: string) {
const parsedEmail = email.toLowerCase()

return ResultAsync.fromPromise(this.sequelize.transaction(), (error) => {
logger.error(
`Error starting database transaction: ${JSON.stringify(error)}`
)
verifyEmailOtp(email: string, otp: string | undefined) {
const normalizedEmail = email.toLowerCase()

return new BadRequestError("Error starting database transaction")
return this.verifyOtp({
otp,
findConditions: { email: normalizedEmail },
findErrorMessage: "Error finding OTP entry when verifying email 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)
)
}

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")
verifyMobileOtp(mobileNumber: string, otp: string | undefined) {
return this.verifyOtp({
otp,
findConditions: { mobileNumber },
findErrorMessage: "Error finding OTP entry when verifying mobile 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() {
Expand Down

0 comments on commit 4e6bc5e

Please sign in to comment.