diff --git a/src/server/modules/auth/LoginController.ts b/src/server/modules/auth/LoginController.ts index 5fe863c2a..0e4700bc6 100644 --- a/src/server/modules/auth/LoginController.ts +++ b/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) diff --git a/src/server/modules/auth/__tests__/LoginController.test.ts b/src/server/modules/auth/__tests__/LoginController.test.ts index 70433dede..db901d245 100644 --- a/src/server/modules/auth/__tests__/LoginController.test.ts +++ b/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 = 'aa@open.test.sg' 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,9 +245,9 @@ 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, @@ -253,13 +255,13 @@ describe('LoginController', () => { : 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() }) @@ -267,9 +269,9 @@ describe('LoginController', () => { 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, @@ -277,12 +279,12 @@ describe('LoginController', () => { : 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() }) @@ -290,7 +292,7 @@ describe('LoginController', () => { 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,9 +329,9 @@ 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, @@ -337,7 +339,7 @@ describe('LoginController', () => { : 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 = 'test@open.gov.sg' + 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, + }) + }) + }) }) diff --git a/src/server/modules/auth/interfaces/AuthService.ts b/src/server/modules/auth/interfaces/AuthService.ts index 9218ae7ea..a53a1acd4 100644 --- a/src/server/modules/auth/interfaces/AuthService.ts +++ b/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 /** - * 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 + verifyOtp(email: string, otp: string, ip: string): Promise /** * Generates the user with the officer email provided, if the diff --git a/src/server/modules/auth/interfaces/OtpRepository.ts b/src/server/modules/auth/interfaces/OtpRepository.ts index 3637976a8..d0042271d 100644 --- a/src/server/modules/auth/interfaces/OtpRepository.ts +++ b/src/server/modules/auth/interfaces/OtpRepository.ts @@ -2,24 +2,27 @@ import { StorableOtp } from '../../../repositories/types' export interface OtpRepository { /** - * Delete the otp associated with the user with the input email. + * 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 + deleteOtpByEmail(email: string, ip: string): Promise /** - * 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 + setOtpForEmail(email: string, ip: string, otp: StorableOtp): Promise /** - * 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 + getOtpForEmail(email: string, ip: string): Promise } diff --git a/src/server/modules/auth/repositories/OtpRepository.ts b/src/server/modules/auth/repositories/OtpRepository.ts index c0f2fa00d..850091fb2 100644 --- a/src/server/modules/auth/repositories/OtpRepository.ts +++ b/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 = (email) => { + private getRedisKey(email: string, ip: string): string { + return `${email}:${ip}` + } + + public deleteOtpByEmail: (email: string, ip: string) => Promise = ( + 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 = ( - email, - otp, - ) => { + public setOtpForEmail: ( + email: string, + ip: string, + otp: StorableOtp, + ) => Promise = (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 = ( - email, - ) => { + public getOtpForEmail: ( + email: string, + ip: string, + ) => Promise = (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 diff --git a/src/server/modules/auth/repositories/__tests__/OtpRepository.test.ts b/src/server/modules/auth/repositories/__tests__/OtpRepository.test.ts index 50c55c793..de21ff171 100644 --- a/src/server/modules/auth/repositories/__tests__/OtpRepository.test.ts +++ b/src/server/modules/auth/repositories/__tests__/OtpRepository.test.ts @@ -16,6 +16,8 @@ const otp = { hashedOtp: 'aaa', retries: 1000, } +const email = 'aaa@aa.com' +const ip = '1.1.1.1' describe('otp cache redis test', () => { beforeEach(async () => { @@ -31,35 +33,43 @@ describe('otp cache redis test', () => { }) test('deleteOtpByEmail test', async () => { - redisMockClient.set('aaa@aa.com', 'aa') - await cache.deleteOtpByEmail('aaa@aa.com') + // Arrange + const key = `${email}:${ip}` + redisMockClient.set(key, 'aa') + + // Act + await cache.deleteOtpByEmail(email, ip) + + // Assert expect(redisMockClient.del).toBeCalledTimes(1) - expect(redisMockClient.del).toBeCalledWith( - 'aaa@aa.com', - expect.any(Function), - ) + expect(redisMockClient.del).toBeCalledWith(key, expect.any(Function)) }) test('getOtpByEmail test', async () => { - redisMockClient.set('aaa@aa.com', JSON.stringify(otp)) - await expect(cache.getOtpForEmail('aaa@aa.com')).resolves.toStrictEqual(otp) - expect(redisMockClient.get).toBeCalledWith( - 'aaa@aa.com', - expect.any(Function), - ) + // Arrange + const key = `${email}:${ip}` + redisMockClient.set(key, JSON.stringify(otp)) + + // Act + await expect(cache.getOtpForEmail(email, ip)).resolves.toStrictEqual(otp) + + // Assert + expect(redisMockClient.get).toBeCalledWith(key, expect.any(Function)) }) test('getOtpByEmail null test', async () => { - await expect(cache.getOtpForEmail('aaa@aa.com')).resolves.toStrictEqual( - null, - ) + // Arrange + await expect(cache.getOtpForEmail(email, ip)).resolves.toStrictEqual(null) + + // Assert expect(redisMockClient.get).toBeCalledWith( - 'aaa@aa.com', + `${email}:${ip}`, expect.any(Function), ) }) test('getOtpByEmail throws test', async () => { + // Arrange const originalGet = redisMockClient.get redisMockClient.get = (_, callback) => { if (callback == null) { @@ -68,15 +78,23 @@ describe('otp cache redis test', () => { callback(Error(), '') return true } - redisMockClient.set('aaa@aa.com', JSON.stringify(otp)) - await expect(cache.getOtpForEmail('aaa@aa.com')).rejects.toThrowError() + const key = `${email}:${ip}` + redisMockClient.set(key, JSON.stringify(otp)) + + // Act + await expect(cache.getOtpForEmail(email, ip)).rejects.toThrowError() + + // Assert redisMockClient.get = originalGet }) test('setOtpByEmail test', async () => { - await cache.setOtpForEmail('aaa@aa.com', otp) + // Arrange + await cache.setOtpForEmail(email, ip, otp) + + // Assert expect(redisMockClient.set).toBeCalledWith( - 'aaa@aa.com', + `${email}:${ip}`, JSON.stringify(otp), 'EX', 10, @@ -85,11 +103,16 @@ describe('otp cache redis test', () => { }) test('setOtpByEmail throws test', async () => { + // Arrange const originalSet = redisMockClient.set redisMockClient.set = () => { throw Error() } - await expect(cache.setOtpForEmail('aaa@aa.com', otp)).rejects.toThrowError() + + // Act + await expect(cache.setOtpForEmail(email, ip, otp)).rejects.toThrowError() + + // Assert redisMockClient.set = originalSet }) }) diff --git a/src/server/modules/auth/services/AuthService.ts b/src/server/modules/auth/services/AuthService.ts index 8331da907..521e7dcda 100644 --- a/src/server/modules/auth/services/AuthService.ts +++ b/src/server/modules/auth/services/AuthService.ts @@ -48,7 +48,7 @@ export class AuthService implements interfaces.AuthService { } try { - await this.otpRepository.setOtpForEmail(email, otpObject) + await this.otpRepository.setOtpForEmail(email, ip, otpObject) } catch (saveError) { logger.error(`Could not save OTP hash:\t${saveError}`) throw new Error('Could not save OTP hash.') @@ -68,63 +68,66 @@ export class AuthService implements interfaces.AuthService { } } - public verifyOtp: (email: string, otp: string) => Promise = - async (email, otp) => { - let retrievedOtp: StorableOtp | null = null - // Retrieve hash from cache - try { - retrievedOtp = await this.otpRepository.getOtpForEmail(email) - } catch (error) { - logger.error(`Error verifying OTP for ${email}:\t${error}`) - throw error - } + public verifyOtp: ( + email: string, + otp: string, + ip: string, + ) => Promise = async (email, otp, ip) => { + let retrievedOtp: StorableOtp | null = null + // Retrieve hash from cache + try { + retrievedOtp = await this.otpRepository.getOtpForEmail(email, ip) + } catch (error) { + logger.error(`Error verifying OTP for ${email}:\t${error}`) + throw error + } - if (!retrievedOtp) { - throw new NotFoundError('Missing otp') - } + if (!retrievedOtp) { + throw new NotFoundError('Missing otp') + } - const isOtpMatch = await this.cryptography.compare( - otp, - retrievedOtp.hashedOtp, - ) + const isOtpMatch = await this.cryptography.compare( + otp, + retrievedOtp.hashedOtp, + ) - if (!isOtpMatch) { - const modifiedOtp = { - ...retrievedOtp, - retries: retrievedOtp.retries - 1, - } + if (!isOtpMatch) { + const modifiedOtp = { + ...retrievedOtp, + retries: retrievedOtp.retries - 1, + } - if (modifiedOtp.retries > 0) { - try { - await this.otpRepository.setOtpForEmail(email, modifiedOtp) - } catch (error) { - logger.error(`OTP retry could not be decremented:\t${error}`) - } - } else { - try { - await this.otpRepository.deleteOtpByEmail(email) - } catch (error) { - logger.error( - `Could not delete OTP after reaching retry limit:\t${error}`, - ) - } + if (modifiedOtp.retries > 0) { + try { + await this.otpRepository.setOtpForEmail(email, ip, modifiedOtp) + } catch (error) { + logger.error(`OTP retry could not be decremented:\t${error}`) + } + } else { + try { + await this.otpRepository.deleteOtpByEmail(email, ip) + } catch (error) { + logger.error( + `Could not delete OTP after reaching retry limit:\t${error}`, + ) } - throw new InvalidOtpError(modifiedOtp.retries) } + throw new InvalidOtpError(modifiedOtp.retries) + } - try { - const dbUser = await this.userRepository.findOrCreateWithEmail(email) + try { + const dbUser = await this.userRepository.findOrCreateWithEmail(email) - this.otpRepository.deleteOtpByEmail(email).catch((error) => { - logger.error(`OTP could not be expired:\t${error}`) - }) + this.otpRepository.deleteOtpByEmail(email, ip).catch((error) => { + logger.error(`OTP could not be expired:\t${error}`) + }) - return dbUser - } catch (error) { - logger.error(`Error creating user:\t ${email}, ${error}`) - throw new Error('Error creating user.') - } + return dbUser + } catch (error) { + logger.error(`Error creating user:\t ${email}, ${error}`) + throw new Error('Error creating user.') } + } public genDBUserWithOfficerEmail: ( officerEmail: string, diff --git a/test/server/api/LoginRoute.test.ts b/test/server/api/LoginRoute.test.ts index 337e11738..4efe97c9b 100644 --- a/test/server/api/LoginRoute.test.ts +++ b/test/server/api/LoginRoute.test.ts @@ -56,7 +56,7 @@ describe('POST /api/login/otp', () => { describe('POST /api/login/verify', () => { test('verify the OTP', async (done) => { // Prime cache - getOtpCache().setOtpForEmail('otpgo.gov@open.test.sg', { + getOtpCache().setOtpForEmail('otpgo.gov@open.test.sg', '127.0.0.1', { hashedOtp: '1', retries: 100, }) diff --git a/test/server/api/util.ts b/test/server/api/util.ts index 6e7f1b44c..0e52a44d9 100644 --- a/test/server/api/util.ts +++ b/test/server/api/util.ts @@ -87,13 +87,18 @@ export function createRequestWithEmail(email: any): Request { * @param {any} user * @returns A mock Request with the input email and otp. */ -export function createRequestWithEmailAndOtp(email: any, otp: any): Request { +export function createRequestWithEmailAndIpAndOtp( + email: any, + ip: string, + otp: any, +): Request { return httpMocks.createRequest({ session: {}, body: { email, otp, }, + ip, }) } diff --git a/test/server/mocks/repositories/OtpRepository.ts b/test/server/mocks/repositories/OtpRepository.ts index 8a137ac7b..f6ebaee4e 100644 --- a/test/server/mocks/repositories/OtpRepository.ts +++ b/test/server/mocks/repositories/OtpRepository.ts @@ -7,41 +7,56 @@ import { OtpRepository } from '../../../../src/server/modules/auth/interfaces/Ot export class OtpRepositoryMock implements OtpRepository { cache = new Map() - deleteOtpByEmail = (email: string) => { - this.cache.delete(email) + private getCacheKey(email: string, ip: string): string { + return `${email}:${ip}` + } + + getRedisKey(email: string, ip: string): string { + return this.getCacheKey(email, ip) + } + + deleteOtpByEmail = (email: string, ip: string) => { + const key = this.getCacheKey(email, ip) + this.cache.delete(key) return Promise.resolve() } - setOtpForEmail = (email: string, otp: StorableOtp) => { - this.cache.set(email, otp) + setOtpForEmail = (email: string, ip: string, otp: StorableOtp) => { + const key = this.getCacheKey(email, ip) + this.cache.set(key, otp) return Promise.resolve() } - getOtpForEmail = (email: string) => { - if (!this.cache.has(email)) { + getOtpForEmail = (email: string, ip: string) => { + const key = this.getCacheKey(email, ip) + if (!this.cache.has(key)) { return Promise.resolve(null) } - return Promise.resolve(this.cache.get(email)!) + return Promise.resolve(this.cache.get(key)!) } } @injectable() export class OtpRepositoryMockDown implements OtpRepository { - deleteOtpByEmail(_: string): Promise { + getRedisKey(email: string, ip: string): string { + return `${email}:${ip}` + } + + deleteOtpByEmail(_: string, __: string): Promise { return Promise.reject(Error()) } - setOtpForEmail(_: string, __: StorableOtp): Promise { + setOtpForEmail(_: string, __: string, ___: StorableOtp): Promise { return Promise.reject(Error()) } - getOtpForEmail(_: string): Promise { + getOtpForEmail(_: string, __: string): Promise { return Promise.reject(Error()) } } export class OtpRepositoryMockNoWrite extends OtpRepositoryMock { - deleteOtpByEmail = (_: string) => Promise.reject() + deleteOtpByEmail = (_: string, __: string) => Promise.reject() - setOtpForEmail = (__: string, _: StorableOtp) => Promise.reject() + setOtpForEmail = (_: string, __: string, ___: StorableOtp) => Promise.reject() }