Merge pull request #2400 from opengovsg/adriangohjw/isom-2003-vapt-active-denial-of-service-of-login-to-gogovsg

isom-2003 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'
@@ -152,6 +152,7 @@ describe('LoginController', () => {
       expect(res.ok).toHaveBeenCalled()
       expect(setOtpForEmail).toHaveBeenCalledWith(
         email,
+        ip,
         expect.objectContaining({
           hashedOtp: otp,
           retries: expect.any(Number),
@@ -190,6 +191,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()
@@ -243,54 +245,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 +304,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 +315,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 +329,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 +353,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 +365,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 +377,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)
@@ -387,4 +389,221 @@ describe('LoginController', () => {
       expect(logger.error).toBeCalled()
     })
   })
+
+  // Ensure no active denial of service attacks can be performed on an email address
+  // by requesting OTPs for the same email address from different IPs
+  describe('OTP IP-based Storage Tests', () => {
+    const email = '[email protected]'
+    const otpA = '123456'
+    const otpB = '654321'
+    const ip1 = '192.168.1.1'
+    const ip2 = '192.168.1.2'
+
+    const hash = jest.fn()
+    const compare = jest.fn()
+    const mailOTP = jest.fn()
+    const initMailer = jest.fn()
+    const mailJobFailure = jest.fn()
+    const mailJobSuccess = jest.fn()
+    const deleteOtpByEmail = jest.fn()
+    const setOtpForEmail = jest.fn()
+    const getOtpForEmail = jest.fn()
+
+    const urlMapper = new UrlMapper()
+    const userRepository = new UserRepository(
+      new UserMapper(urlMapper),
+      urlMapper,
+    )
+    const findOrCreateWithEmail = jest.spyOn(
+      userRepository,
+      'findOrCreateWithEmail',
+    )
+
+    const authService = new AuthService(
+      { hash, compare },
+      { mailOTP, initMailer, mailJobFailure, mailJobSuccess },
+      { deleteOtpByEmail, setOtpForEmail, getOtpForEmail },
+      userRepository,
+    )
+    const controller = new LoginController(authService)
+
+    beforeEach(() => {
+      hash.mockClear()
+      compare.mockClear()
+      mailOTP.mockClear()
+      initMailer.mockClear()
+      deleteOtpByEmail.mockClear()
+      setOtpForEmail.mockClear()
+      getOtpForEmail.mockClear()
+      findOrCreateWithEmail.mockClear()
+
+      compare.mockImplementation((data, encrypted) =>
+        Promise.resolve(data === encrypted),
+      )
+      deleteOtpByEmail.mockResolvedValue(undefined)
+      hash.mockResolvedValue('hashedOtp')
+    })
+
+    // For cases where a user has multiple devices, we want to ensure that they can login from any of them
+    // This is more of a basic test to ensure that OTP is also dependent on the IP and not just the email address
+    test('Same user from different IPs can both complete login successfully', async () => {
+      const user = { id: 1, email }
+      findOrCreateWithEmail.mockResolvedValue(user)
+
+      // Set up OTPs for both IPs
+      getOtpForEmail.mockImplementation((e, i) => {
+        if (e === email && i === ip1) {
+          return Promise.resolve({ hashedOtp: otpA, retries: 3 })
+        }
+        if (e === email && i === ip2) {
+          return Promise.resolve({ hashedOtp: otpB, retries: 3 })
+        }
+        return Promise.resolve(null)
+      })
+
+      // User logs in from mobile (IP1)
+      const req1 = createRequestWithEmailAndIpAndOtp(email, ip1, otpA)
+      const res1 = getMockResponse()
+      await controller.verifyOtp(req1, res1)
+
+      // User logs in from desktop (IP2)
+      const req2 = createRequestWithEmailAndIpAndOtp(email, ip2, otpB)
+      const res2 = getMockResponse()
+      await controller.verifyOtp(req2, res2)
+
+      // Both should succeed
+      expect(res1.ok).toHaveBeenCalled()
+      expect(res2.ok).toHaveBeenCalled()
+      expect(deleteOtpByEmail).toHaveBeenCalledWith(email, ip1)
+      expect(deleteOtpByEmail).toHaveBeenCalledWith(email, ip2)
+    })
+
+    test('User A cannot verify with User B OTP', async () => {
+      // Set up OTPs for both users with different values
+      getOtpForEmail.mockImplementation((e, i) => {
+        if (e === email && i === ip1) {
+          return Promise.resolve({ hashedOtp: otpA, retries: 3 })
+        }
+        if (e === email && i === ip2) {
+          return Promise.resolve({ hashedOtp: otpB, retries: 3 })
+        }
+        return Promise.resolve(null)
+      })
+
+      // User A tries to verify with User B's OTP
+      const req = createRequestWithEmailAndIpAndOtp(email, ip1, otpB)
+      const res = getMockResponse()
+      await controller.verifyOtp(req, res)
+
+      // Should fail
+      expect(res.unauthorized).toHaveBeenCalled()
+      expect(deleteOtpByEmail).not.toHaveBeenCalled()
+    })
+
+    // Note: we use the same OTP to ensure OTP deletion is not dependent on the OTP value
+    test('Only User A OTP is deleted after successful verification, User B OTP remains', async () => {
+      const user = { id: 1, email }
+      findOrCreateWithEmail.mockResolvedValue(user)
+
+      // Set up OTPs for both users
+      getOtpForEmail.mockImplementation((e, i) => {
+        if (e === email && i === ip1) {
+          return Promise.resolve({ hashedOtp: otpA, retries: 3 })
+        }
+        if (e === email && i === ip2) {
+          return Promise.resolve({ hashedOtp: otpA, retries: 3 })
+        }
+        return Promise.resolve(null)
+      })
+
+      // User A verifies OTP successfully
+      const req = createRequestWithEmailAndIpAndOtp(email, ip1, otpA)
+      const res = getMockResponse()
+      await controller.verifyOtp(req, res)
+
+      // Only User A's OTP should be deleted
+      expect(deleteOtpByEmail).toHaveBeenCalledWith(email, ip1)
+      expect(deleteOtpByEmail).not.toHaveBeenCalledWith(email, ip2)
+      expect(deleteOtpByEmail).toHaveBeenCalledTimes(1)
+    })
+
+    test('Different IPs create different Redis keys for same email', async () => {
+      // User A requests OTP
+      const req1 = createRequestWithEmail(email)
+      req1.ip = ip1
+      const res1 = getMockResponse()
+      await controller.generateOtp(req1, res1)
+
+      // User B requests OTP
+      const req2 = createRequestWithEmail(email)
+      req2.ip = ip2
+      const res2 = getMockResponse()
+      await controller.generateOtp(req2, res2)
+
+      // Verify different keys were used
+      expect(setOtpForEmail).toHaveBeenCalledWith(
+        email,
+        ip1,
+        expect.any(Object),
+      )
+      expect(setOtpForEmail).toHaveBeenCalledWith(
+        email,
+        ip2,
+        expect.any(Object),
+      )
+      expect(setOtpForEmail).toHaveBeenCalledTimes(2)
+    })
+
+    test('User A locked out after 3 wrong attempts, User B can still try', async () => {
+      compare.mockResolvedValue(false) // Wrong OTP
+
+      // Set up OTPs for both users with mutable retry counts
+      let userARetries = 3
+      let userBRetries = 3
+
+      getOtpForEmail.mockImplementation((e, i) => {
+        if (e === email && i === ip1) {
+          return Promise.resolve({ hashedOtp: otpA, retries: userARetries })
+        }
+        if (e === email && i === ip2) {
+          return Promise.resolve({ hashedOtp: otpA, retries: userBRetries })
+        }
+        return Promise.resolve(null)
+      })
+
+      // Mock setOtpForEmail to update retry counts
+      setOtpForEmail.mockImplementation((e, i, otpData) => {
+        if (e === email && i === ip1) {
+          userARetries = otpData.retries
+        }
+        if (e === email && i === ip2) {
+          userBRetries = otpData.retries
+        }
+        return Promise.resolve()
+      })
+
+      // User A enters wrong OTP 3 times
+      for (let i = 0; i < 3; i += 1) {
+        const req = createRequestWithEmailAndIpAndOtp(email, ip1, 'wrong')
+        const res = getMockResponse()
+        // eslint-disable-next-line no-await-in-loop
+        await controller.verifyOtp(req, res)
+        expect(res.unauthorized).toHaveBeenCalled()
+      }
+
+      // User A should be locked out (OTP deleted)
+      expect(deleteOtpByEmail).toHaveBeenCalledWith(email, ip1)
+
+      // User B should still be able to try
+      const req2 = createRequestWithEmailAndIpAndOtp(email, ip2, 'wrong')
+      const res2 = getMockResponse()
+      await controller.verifyOtp(req2, res2)
+
+      // User B should still have retries left
+      expect(setOtpForEmail).toHaveBeenCalledWith(email, ip2, {
+        hashedOtp: otpA,
+        retries: 2,
+      })
+    })
+  })
 })