test(otp): add OTP IP-based storage tests to prevent denial of service attacks

Author: adriangohjw

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

@@ -389,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,
+      })
+    })
+  })
 })