feat(otp): update OTP verification to include IP address

Author: adriangohjw

src/server/modules/auth/LoginController.ts

@@ -68,7 +68,7 @@ export class LoginController {
     const { email, otp }: VerifyOtpRequest = req.body
 
     try {
-      const user = await this.authService.verifyOtp(email, otp)
+      const user = await this.authService.verifyOtp(email, otp, getIp(req))
       req.session!.user = user
       res.ok(jsonMessage('OTP hash verification ok.'))
       dogstatsd.increment(OTP_VERIFY_SUCCESS, 1, 1)

src/server/modules/auth/__tests__/LoginController.test.ts

@@ -1,7 +1,7 @@
 import httpMocks from 'node-mocks-http'
 import {
   createRequestWithEmail,
-  createRequestWithEmailAndOtp,
+  createRequestWithEmailAndIpAndOtp,
   createRequestWithUser,
   userModelMock,
 } from '../../../../../test/server/api/util'
@@ -116,6 +116,7 @@ describe('LoginController', () => {
     const mailJobFailure = jest.fn()
     const mailJobSuccess = jest.fn()
 
+    const getRedisKey = jest.fn()
     const deleteOtpByEmail = jest.fn()
     const setOtpForEmail = jest.fn()
     const getOtpForEmail = jest.fn()
@@ -124,7 +125,7 @@ describe('LoginController', () => {
     const authService = new AuthService(
       { hash, compare },
       { mailOTP, initMailer, mailJobFailure, mailJobSuccess },
-      { deleteOtpByEmail, setOtpForEmail, getOtpForEmail },
+      { getRedisKey, deleteOtpByEmail, setOtpForEmail, getOtpForEmail },
       new UserRepository(new UserMapper(urlMapper), urlMapper),
     )
     const controller = new LoginController(authService)
@@ -152,6 +153,7 @@ describe('LoginController', () => {
       expect(res.ok).toHaveBeenCalled()
       expect(setOtpForEmail).toHaveBeenCalledWith(
         email,
+        ip,
         expect.objectContaining({
           hashedOtp: otp,
           retries: expect.any(Number),
@@ -190,6 +192,7 @@ describe('LoginController', () => {
   describe('verifyOtp tests', () => {
     const email = '[email protected]'
     const otp = '1'
+    const ip = '1.1.1.1' // This should match the IP set in createRequestWithEmailAndIpAndOtp
 
     const hash = jest.fn()
     const compare = jest.fn()
@@ -199,6 +202,7 @@ describe('LoginController', () => {
     const mailJobFailure = jest.fn()
     const mailJobSuccess = jest.fn()
 
+    const getRedisKey = jest.fn()
     const deleteOtpByEmail = jest.fn()
     const setOtpForEmail = jest.fn()
     const getOtpForEmail = jest.fn()
@@ -217,7 +221,7 @@ describe('LoginController', () => {
     const authService = new AuthService(
       { hash, compare },
       { mailOTP, initMailer, mailJobFailure, mailJobSuccess },
-      { deleteOtpByEmail, setOtpForEmail, getOtpForEmail },
+      { getRedisKey, deleteOtpByEmail, setOtpForEmail, getOtpForEmail },
       userRepository,
     )
 
@@ -243,54 +247,54 @@ describe('LoginController', () => {
       test('valid email and otp', async () => {
         const user = { id: 1, email }
 
-        getOtpForEmail.mockImplementation((e) =>
+        getOtpForEmail.mockImplementation((e, i) =>
           Promise.resolve(
-            e === email
+            e === email && i === ip
               ? {
                   hashedOtp: otp,
                   retries: 100,
                 }
               : null,
           ),
         )
-        const req = createRequestWithEmailAndOtp(email, otp)
+        const req = createRequestWithEmailAndIpAndOtp(email, ip, otp)
         const res = getMockResponse()
         findOrCreateWithEmail.mockResolvedValue(user)
 
         await controller.verifyOtp(req, res)
 
-        expect(deleteOtpByEmail).toHaveBeenCalledWith(email)
+        expect(deleteOtpByEmail).toHaveBeenCalledWith(email, ip)
         expect(req.session!.user).toStrictEqual(user)
         expect(res.ok).toHaveBeenCalled()
       })
 
       test('valid email, wrong otp and expiring', async () => {
         const badOtp = '0'
 
-        getOtpForEmail.mockImplementation((e) =>
+        getOtpForEmail.mockImplementation((e, i) =>
           Promise.resolve(
-            e === email
+            e === email && i === ip
               ? {
                   hashedOtp: otp,
                   retries: 1,
                 }
               : null,
           ),
         )
-        const req = createRequestWithEmailAndOtp(email, badOtp)
+        const req = createRequestWithEmailAndIpAndOtp(email, ip, badOtp)
         const res = getMockResponse()
 
         await controller.verifyOtp(req, res)
 
-        expect(deleteOtpByEmail).toHaveBeenCalledWith(email)
+        expect(deleteOtpByEmail).toHaveBeenCalledWith(email, ip)
         expect(req.session!.user).toBeUndefined()
         expect(res.unauthorized).toHaveBeenCalled()
       })
 
       test('valid email and no otp in cache', async () => {
         getOtpForEmail.mockResolvedValue(null)
 
-        const req = createRequestWithEmailAndOtp(email, otp)
+        const req = createRequestWithEmailAndIpAndOtp(email, ip, otp)
         const res = getMockResponse()
 
         await controller.verifyOtp(req, res)
@@ -302,9 +306,9 @@ describe('LoginController', () => {
 
       test('valid email and wrong otp with retries left', async () => {
         const badOtp = '0'
-        getOtpForEmail.mockImplementation((e) =>
+        getOtpForEmail.mockImplementation((e, i) =>
           Promise.resolve(
-            e === email
+            e === email && i === ip
               ? {
                   hashedOtp: otp,
                   retries: 100,
@@ -313,12 +317,12 @@ describe('LoginController', () => {
           ),
         )
 
-        const req = createRequestWithEmailAndOtp(email, badOtp)
+        const req = createRequestWithEmailAndIpAndOtp(email, ip, badOtp)
         const res = getMockResponse()
 
         await controller.verifyOtp(req, res)
 
-        expect(setOtpForEmail).toHaveBeenCalledWith(email, {
+        expect(setOtpForEmail).toHaveBeenCalledWith(email, ip, {
           hashedOtp: otp,
           retries: 99,
         })
@@ -327,17 +331,17 @@ describe('LoginController', () => {
       })
 
       test('no email and has valid otp in request', async () => {
-        getOtpForEmail.mockImplementation((e) =>
+        getOtpForEmail.mockImplementation((e, i) =>
           Promise.resolve(
-            e === email
+            e === email && i === ip
               ? {
                   hashedOtp: otp,
                   retries: 100,
                 }
               : null,
           ),
         )
-        const req = createRequestWithEmailAndOtp(undefined, '1')
+        const req = createRequestWithEmailAndIpAndOtp(undefined, ip, otp)
         const res = getMockResponse()
 
         await controller.verifyOtp(req, res)
@@ -351,7 +355,7 @@ describe('LoginController', () => {
 
     test('cache down', async () => {
       getOtpForEmail.mockRejectedValue(new Error())
-      const req = createRequestWithEmailAndOtp(email, otp)
+      const req = createRequestWithEmailAndIpAndOtp(email, ip, otp)
       const res = getMockResponse()
 
       await controller.verifyOtp(req, res)
@@ -363,9 +367,9 @@ describe('LoginController', () => {
     })
 
     test('db down', async () => {
-      getOtpForEmail.mockImplementation((e) =>
+      getOtpForEmail.mockImplementation((e, i) =>
         Promise.resolve(
-          e === email
+          e === email && i === ip
             ? {
                 hashedOtp: otp,
                 retries: 100,
@@ -375,7 +379,7 @@ describe('LoginController', () => {
       )
       findOrCreateWithEmail.mockRejectedValue(new Error())
 
-      const req = createRequestWithEmailAndOtp(email, otp)
+      const req = createRequestWithEmailAndIpAndOtp(email, ip, otp)
       const res = getMockResponse()
 
       await controller.verifyOtp(req, res)

src/server/modules/auth/interfaces/AuthService.ts

@@ -4,19 +4,21 @@ export interface AuthService {
   /**
    * Generates and stores a random otp for the input email.
    * @param  {string} email The email of the user to generate an otp for.
+   * @param  {string} ip The IP address of the user.
    * @returns Promise that resolves to void.
    */
   generateOtp(email: string, ip: string): Promise<void>
 
   /**
-   * Verifies that the input otp matches the stored otp for the input email.
+   * Verifies that the input otp matches the stored otp for the input email and IP.
    * Throws an error if the otp is different or there is no otp generated
-   * for the input email.
+   * for the input email and IP combination.
    * @param  {string} email Email of the user.
    * @param  {string} otp Otp to verify.
+   * @param  {string} ip IP address of the user.
    * @returns Promise that resolves to the user if the otp is valid.
    */
-  verifyOtp(email: string, otp: string): Promise<StorableUser>
+  verifyOtp(email: string, otp: string, ip: string): Promise<StorableUser>
 
   /**
    * Generates the user with the officer email provided, if the

src/server/modules/auth/interfaces/OtpRepository.ts

@@ -2,24 +2,35 @@ import { StorableOtp } from '../../../repositories/types'
 
 export interface OtpRepository {
   /**
-   * Delete the otp associated with the user with the input email.
+   * Get the redis key for the otp associated with the user with the input email and IP.
    * @param  {string} email Email of the user.
+   * @param  {string} ip IP address of the user.
+   * @returns Promise that resolves to the redis key.
+   */
+  getRedisKey(email: string, ip: string): string
+
+  /**
+   * Delete the otp associated with the user with the input email and IP.
+   * @param  {string} email Email of the user.
+   * @param  {string} ip IP address of the user.
    * @returns Promise that resolves to void.
    */
-  deleteOtpByEmail(email: string): Promise<void>
+  deleteOtpByEmail(email: string, ip: string): Promise<void>
 
   /**
-   * Sets or replaces the otp associated with the user with the input email.
+   * Sets or replaces the otp associated with the user with the input email and IP.
    * @param  {string} email Email of the user.
+   * @param  {string} ip IP address of the user.
    * @param  {StorableOtp} otp The new otp to be associated.
    * @returns Promise that resolves to void.
    */
-  setOtpForEmail(email: string, otp: StorableOtp): Promise<void>
+  setOtpForEmail(email: string, ip: string, otp: StorableOtp): Promise<void>
 
   /**
-   * Retrieves the otp associated with the user with the input email.
+   * Retrieves the otp associated with the user with the input email and IP.
    * @param  {string} email Email of the user.
+   * @param  {string} ip IP address of the user.
    * @returns Promise that resolves the otp or null if it does not exist.
    */
-  getOtpForEmail(email: string): Promise<StorableOtp | null>
+  getOtpForEmail(email: string, ip: string): Promise<StorableOtp | null>
 }

src/server/modules/auth/repositories/OtpRepository.ts

@@ -19,9 +19,17 @@ export class OtpRepository implements interfaces.OtpRepository {
     this.otpMapper = otpMapper
   }
 
-  public deleteOtpByEmail: (email: string) => Promise<void> = (email) => {
+  private getRedisKey(email: string, ip: string): string {
+    return `${email}:${ip}`
+  }
+
+  public deleteOtpByEmail: (email: string, ip: string) => Promise<void> = (
+    email,
+    ip,
+  ) => {
     return new Promise((resolve, reject) => {
-      otpClient.del(email, (redisDelError, resdisDelResponse) => {
+      const key = this.getRedisKey(email, ip)
+      otpClient.del(key, (redisDelError, resdisDelResponse) => {
         if (redisDelError || resdisDelResponse !== 1) {
           reject(redisDelError)
           return
@@ -32,13 +40,15 @@ export class OtpRepository implements interfaces.OtpRepository {
     })
   }
 
-  public setOtpForEmail: (email: string, otp: StorableOtp) => Promise<void> = (
-    email,
-    otp,
-  ) => {
+  public setOtpForEmail: (
+    email: string,
+    ip: string,
+    otp: StorableOtp,
+  ) => Promise<void> = (email, ip, otp) => {
     return new Promise((resolve, reject) => {
+      const key = this.getRedisKey(email, ip)
       otpClient.set(
-        email,
+        key,
         this.otpMapper.dtoToPersistence(otp),
         'EX',
         otpExpiry,
@@ -54,11 +64,13 @@ export class OtpRepository implements interfaces.OtpRepository {
     })
   }
 
-  public getOtpForEmail: (email: string) => Promise<StorableOtp | null> = (
-    email,
-  ) => {
+  public getOtpForEmail: (
+    email: string,
+    ip: string,
+  ) => Promise<StorableOtp | null> = (email, ip) => {
     return new Promise((resolve, reject) => {
-      otpClient.get(email, (redisError, string) => {
+      const key = this.getRedisKey(email, ip)
+      otpClient.get(key, (redisError, string) => {
         if (redisError) {
           reject(redisError)
           return