From 4e6bc5e81c71d366c2a0ecfb2536e8b2d133b441 Mon Sep 17 00:00:00 2001 From: Timothee Groleau Date: Thu, 18 Apr 2024 11:06:40 +0800 Subject: [PATCH] Fix + Refactor: improve verify otp flows (#1295) * 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 --- src/services/identity/UsersService.ts | 298 +++++++++++--------------- 1 file changed, 126 insertions(+), 172 deletions(-) diff --git a/src/services/identity/UsersService.ts b/src/services/identity/UsersService.ts index c30c29430..e5055bd3b 100644 --- a/src/services/identity/UsersService.ts +++ b/src/services/identity/UsersService.ts @@ -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" @@ -23,6 +25,8 @@ enum OtpType { Mobile = "MOBILE", } +type Class = new (...args: any[]) => T + interface UsersServiceProps { mailer: MailClient smsClient: SmsClient @@ -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( + ErrorClass: Class, + 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() {