diff --git a/package-lock.json b/package-lock.json index 673d48081f..692b8d1640 100644 --- a/package-lock.json +++ b/package-lock.json @@ -19187,6 +19187,11 @@ "resolved": "https://registry.npmjs.org/netmask/-/netmask-1.0.6.tgz", "integrity": "sha1-ICl+idhvb2QA8lDZ9Pa0wZRfzTU=" }, + "neverthrow": { + "version": "2.7.1", + "resolved": "https://registry.npmjs.org/neverthrow/-/neverthrow-2.7.1.tgz", + "integrity": "sha512-tk7vBuxlBcVU6GIXrAOpez2ZpvJxVW1xl2OjVufdVd87g61SjvekY0VBOuTh1EdUf6/gbgQcQujpxXlap0bQ0A==" + }, "ng-infinite-scroll": { "version": "1.3.0", "resolved": "https://registry.npmjs.org/ng-infinite-scroll/-/ng-infinite-scroll-1.3.0.tgz", diff --git a/package.json b/package.json index 84f27eaa98..25ada01da2 100644 --- a/package.json +++ b/package.json @@ -134,6 +134,7 @@ "mongodb-uri": "^0.9.7", "mongoose": "^5.10.0", "multiparty": ">=4.1.3", + "neverthrow": "^2.7.1", "ng-infinite-scroll": "^1.3.0", "ng-table": "^3.0.1", "ngclipboard": "^2.0.0", diff --git a/src/app/models/user.server.model.ts b/src/app/models/user.server.model.ts index 32aaad4d37..7b98277af4 100644 --- a/src/app/models/user.server.model.ts +++ b/src/app/models/user.server.model.ts @@ -98,7 +98,10 @@ const compileUserModel = (db: Mongoose) => { runValidators: true, setDefaultsOnInsert: true, }, - ) + ).populate({ + path: 'agency', + model: AGENCY_SCHEMA_ID, + }) } return db.model<IUserSchema, IUserModel>(USER_SCHEMA_ID, UserSchema) diff --git a/src/app/modules/auth/__tests__/auth.controller.spec.ts b/src/app/modules/auth/__tests__/auth.controller.spec.ts index 21ba412ef6..cde65ca3fc 100644 --- a/src/app/modules/auth/__tests__/auth.controller.spec.ts +++ b/src/app/modules/auth/__tests__/auth.controller.spec.ts @@ -1,12 +1,15 @@ +import { errAsync, okAsync } from 'neverthrow' import expressHandler from 'tests/unit/backend/helpers/jest-express' import { mocked } from 'ts-jest/utils' import MailService from 'src/app/services/mail.service' import { IAgencySchema, IUserSchema } from 'src/types' +import { ApplicationError, DatabaseError } from '../../core/core.errors' +import { MailSendError } from '../../mail/mail.errors' import * as UserService from '../../user/user.service' import * as AuthController from '../auth.controller' -import { InvalidOtpError } from '../auth.errors' +import { InvalidDomainError, InvalidOtpError } from '../auth.errors' import * as AuthService from '../auth.service' const VALID_EMAIL = 'test@example.com' @@ -25,20 +28,39 @@ describe('auth.controller', () => { }) describe('handleCheckUser', () => { - it('should return 200', async () => { + const MOCK_REQ = expressHandler.mockRequest({ + body: { email: 'test@example.com' }, + }) + + it('should return 200 when domain is valid', async () => { // Arrange const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(<IAgencySchema>{}), + ) // Act - await AuthController.handleCheckUser( - expressHandler.mockRequest(), - mockRes, - jest.fn(), - ) + await AuthController.handleCheckUser(MOCK_REQ, mockRes, jest.fn()) // Assert expect(mockRes.sendStatus).toBeCalledWith(200) }) + + it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => { + // Arrange + const expectedError = new InvalidDomainError() + const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + errAsync(expectedError), + ) + + // Act + await AuthController.handleCheckUser(MOCK_REQ, mockRes, jest.fn()) + + // Assert + expect(mockRes.status).toBeCalledWith(expectedError.status) + expect(mockRes.send).toBeCalledWith(expectedError.message) + }) }) describe('handleLoginSendOtp', () => { @@ -51,8 +73,11 @@ describe('auth.controller', () => { // Arrange const mockRes = expressHandler.mockResponse() // Mock AuthService and MailService to return without errors - MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP) - MockMailService.sendLoginOtp.mockResolvedValueOnce(true) + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(<IAgencySchema>{}), + ) + MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP)) + MockMailService.sendLoginOtp.mockReturnValueOnce(okAsync(true)) // Act await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn()) @@ -65,12 +90,31 @@ describe('auth.controller', () => { expect(MockMailService.sendLoginOtp).toHaveBeenCalledTimes(1) }) + it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => { + // Arrange + const expectedError = new InvalidDomainError() + const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + errAsync(expectedError), + ) + + // Act + await AuthController.handleLoginSendOtp(MOCK_REQ, mockRes, jest.fn()) + + // Assert + expect(mockRes.status).toBeCalledWith(expectedError.status) + expect(mockRes.send).toBeCalledWith(expectedError.message) + }) + it('should return 500 when there is an error generating login OTP', async () => { // Arrange const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(<IAgencySchema>{}), + ) // Mock createLoginOtp failure - MockAuthService.createLoginOtp.mockRejectedValueOnce( - new Error('otp creation error'), + MockAuthService.createLoginOtp.mockReturnValueOnce( + errAsync(new DatabaseError('otp creation error')), ) // Act @@ -89,10 +133,13 @@ describe('auth.controller', () => { it('should return 500 when there is an error sending login OTP', async () => { // Arrange const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(<IAgencySchema>{}), + ) // Mock createLoginOtp success but sendLoginOtp failure. - MockAuthService.createLoginOtp.mockResolvedValueOnce(MOCK_OTP) - MockMailService.sendLoginOtp.mockRejectedValueOnce( - new Error('send error'), + MockAuthService.createLoginOtp.mockReturnValueOnce(okAsync(MOCK_OTP)) + MockMailService.sendLoginOtp.mockReturnValueOnce( + errAsync(new MailSendError('send error')), ) // Act @@ -101,7 +148,7 @@ describe('auth.controller', () => { // Assert expect(mockRes.status).toBeCalledWith(500) expect(mockRes.send).toBeCalledWith( - 'Error sending OTP. Please try again later and if the problem persists, contact us.', + 'Failed to send login OTP. Please try again later and if the problem persists, contact us.', ) // Services should have been invoked. expect(MockAuthService.createLoginOtp).toHaveBeenCalledTimes(1) @@ -122,36 +169,49 @@ describe('auth.controller', () => { const mockUser = { toObject: () => ({ id: 'imagine this is a user document from the db' }), } as IUserSchema - // Add agency into locals due to precondition. - const mockRes = expressHandler.mockResponse({ - locals: { agency: MOCK_AGENCY }, - }) + const mockRes = expressHandler.mockResponse() // Mock all service success. - MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true) - MockUserService.retrieveUser.mockResolvedValueOnce(mockUser) + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(MOCK_AGENCY), + ) + MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true)) + MockUserService.retrieveUser.mockReturnValueOnce(okAsync(mockUser)) // Act await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn()) // Assert expect(mockRes.status).toBeCalledWith(200) - expect(mockRes.send).toBeCalledWith({ - ...mockUser.toObject(), - agency: MOCK_AGENCY, - }) + expect(mockRes.send).toBeCalledWith(mockUser.toObject()) }) - it('should return 422 when verifying login OTP throws an InvalidOtpError', async () => { + it('should return with ApplicationError status and message when retrieving agency returns an ApplicationError', async () => { // Arrange - // Add agency into locals due to precondition. - const mockRes = expressHandler.mockResponse({ - locals: { agency: MOCK_AGENCY }, - }) + const expectedError = new InvalidDomainError() + const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + errAsync(expectedError), + ) + + // Act + await AuthController.handleLoginVerifyOtp(MOCK_REQ, mockRes, jest.fn()) + + // Assert + expect(mockRes.status).toBeCalledWith(expectedError.status) + expect(mockRes.send).toBeCalledWith(expectedError.message) + }) + + it('should return 422 when verifying login OTP returns an InvalidOtpError', async () => { + // Arrange + const mockRes = expressHandler.mockResponse() const expectedInvalidOtpError = new InvalidOtpError() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(MOCK_AGENCY), + ) // Mock error from verifyLoginOtp. - MockAuthService.verifyLoginOtp.mockRejectedValueOnce( - expectedInvalidOtpError, + MockAuthService.verifyLoginOtp.mockReturnValueOnce( + errAsync(expectedInvalidOtpError), ) // Act @@ -165,15 +225,15 @@ describe('auth.controller', () => { expect(MockUserService.retrieveUser).not.toHaveBeenCalled() }) - it('should return 500 when verifying login OTP throws a non-InvalidOtpError', async () => { + it('should return 500 when verifying login OTP returns a non-InvalidOtpError', async () => { // Arrange - // Add agency into locals due to precondition. - const mockRes = expressHandler.mockResponse({ - locals: { agency: MOCK_AGENCY }, - }) + const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(MOCK_AGENCY), + ) // Mock generic error from verifyLoginOtp. - MockAuthService.verifyLoginOtp.mockRejectedValueOnce( - new Error('generic error'), + MockAuthService.verifyLoginOtp.mockReturnValueOnce( + errAsync(new ApplicationError('generic error')), ) // Act @@ -182,22 +242,22 @@ describe('auth.controller', () => { // Assert expect(mockRes.status).toBeCalledWith(500) expect(mockRes.send).toBeCalledWith( - 'Failed to validate OTP. Please try again later and if the problem persists, contact us.', + expect.stringContaining('Failed to process OTP.'), ) // Check that the correct services have been called or not called. expect(MockAuthService.verifyLoginOtp).toHaveBeenCalledTimes(1) expect(MockUserService.retrieveUser).not.toHaveBeenCalled() }) - it('should return 500 when an error is thrown while upserting user', async () => { + it('should return 500 when an error is returned while upserting user', async () => { // Arrange - // Add agency into locals due to precondition. - const mockRes = expressHandler.mockResponse({ - locals: { agency: MOCK_AGENCY }, - }) - MockAuthService.verifyLoginOtp.mockResolvedValueOnce(true) - MockUserService.retrieveUser.mockRejectedValueOnce( - new Error('upsert error'), + const mockRes = expressHandler.mockResponse() + MockAuthService.validateEmailDomain.mockReturnValueOnce( + okAsync(MOCK_AGENCY), + ) + MockAuthService.verifyLoginOtp.mockReturnValueOnce(okAsync(true)) + MockUserService.retrieveUser.mockReturnValueOnce( + errAsync(new DatabaseError()), ) // Act @@ -207,9 +267,7 @@ describe('auth.controller', () => { expect(mockRes.status).toBeCalledWith(500) expect(mockRes.send).toBeCalledWith( // Use stringContaining here due to dynamic text and out of test scope. - expect.stringContaining( - 'User signin failed. Please try again later and if the problem persists', - ), + expect.stringContaining('Failed to process OTP.'), ) // Check that the correct services have been called or not called. expect(MockAuthService.verifyLoginOtp).toHaveBeenCalledTimes(1) @@ -257,7 +315,7 @@ describe('auth.controller', () => { expect(mockRes.sendStatus).toBeCalledWith(400) }) - it('should return 500 when error is thrown when destroying session', async () => { + it('should return 500 when error is returned when destroying session', async () => { // Arrange const mockDestroyWithErr = jest .fn() diff --git a/src/app/modules/auth/__tests__/auth.middlewares.spec.ts b/src/app/modules/auth/__tests__/auth.middlewares.spec.ts deleted file mode 100644 index 4c3bb824ba..0000000000 --- a/src/app/modules/auth/__tests__/auth.middlewares.spec.ts +++ /dev/null @@ -1,71 +0,0 @@ -import expressHandler from 'tests/unit/backend/helpers/jest-express' -import { mocked } from 'ts-jest/utils' - -import { IAgencySchema } from 'src/types' - -import { InvalidDomainError } from '../auth.errors' -import * as AuthMiddleware from '../auth.middlewares' -import * as AuthService from '../auth.service' - -jest.mock('../auth.service') -const MockAuthService = mocked(AuthService) - -describe('auth.middleware', () => { - describe('validateDomain', () => { - const MOCK_REQ = expressHandler.mockRequest({ - body: { email: 'test@example.com' }, - }) - - it('should continue without error when domain is valid', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - const mockNext = jest.fn() - MockAuthService.getAgencyWithEmail.mockResolvedValueOnce( - {} as IAgencySchema, - ) - - // Act - await AuthMiddleware.validateDomain(MOCK_REQ, mockRes, mockNext) - - // Assert - expect(mockNext).toBeCalled() - }) - - it('should return 500 when retrieving agency throws non ApplicationError', async () => { - // Arrange - const mockRes = expressHandler.mockResponse() - const mockNext = jest.fn() - MockAuthService.getAgencyWithEmail.mockRejectedValueOnce( - new Error('some error'), - ) - - // Act - await AuthMiddleware.validateDomain(MOCK_REQ, mockRes, mockNext) - - // Assert - expect(mockRes.status).toBeCalledWith(500) - expect(mockRes.send).toBeCalledWith( - expect.stringContaining( - 'Unable to validate email domain. If this issue persists, please submit a Support Form', - ), - ) - expect(mockNext).not.toBeCalled() - }) - - it('should return with ApplicationError status and message when retrieving agency throws ApplicationError', async () => { - // Arrange - const expectedError = new InvalidDomainError() - const mockRes = expressHandler.mockResponse() - const mockNext = jest.fn() - MockAuthService.getAgencyWithEmail.mockRejectedValueOnce(expectedError) - - // Act - await AuthMiddleware.validateDomain(MOCK_REQ, mockRes, mockNext) - - // Assert - expect(mockRes.status).toBeCalledWith(expectedError.status) - expect(mockRes.send).toBeCalledWith(expectedError.message) - expect(mockNext).not.toBeCalled() - }) - }) -}) diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts index fa3e69b9f6..5c01e21040 100644 --- a/src/app/modules/auth/__tests__/auth.routes.spec.ts +++ b/src/app/modules/auth/__tests__/auth.routes.spec.ts @@ -1,4 +1,5 @@ import { pick } from 'lodash' +import { errAsync, okAsync } from 'neverthrow' import supertest from 'supertest' import { CookieStore, setupApp } from 'tests/integration/helpers/express-setup' import dbHandler from 'tests/unit/backend/helpers/jest-db' @@ -8,6 +9,8 @@ import MailService from 'src/app/services/mail.service' import * as OtpUtils from 'src/app/utils/otp' import { IAgencySchema } from 'src/types' +import { ApplicationError, DatabaseError } from '../../core/core.errors' +import { MailSendError } from '../../mail/mail.errors' import * as UserService from '../../user/user.service' import { AuthRouter } from '../auth.routes' import * as AuthService from '../auth.service' @@ -82,16 +85,17 @@ describe('auth.routes', () => { expect(response.text).toEqual('OK') }) - it('should return 500 when validating domain throws an unknown error', async () => { + it('should return 500 when validating domain returns a database error', async () => { // Arrange // Insert agency const validDomain = 'example.com' const validEmail = `test@${validDomain}` + const mockErrorString = 'Unable to validate email domain.' await dbHandler.insertDefaultAgency({ mailDomain: validDomain }) const getAgencySpy = jest - .spyOn(AuthService, 'getAgencyWithEmail') - .mockRejectedValueOnce(new Error('some error occured')) + .spyOn(AuthService, 'validateEmailDomain') + .mockReturnValueOnce(errAsync(new DatabaseError(mockErrorString))) // Act const response = await request @@ -101,9 +105,7 @@ describe('auth.routes', () => { // Assert expect(getAgencySpy).toBeCalled() expect(response.status).toEqual(500) - expect(response.text).toEqual( - expect.stringContaining('Unable to validate email domain.'), - ) + expect(response.text).toEqual(mockErrorString) }) }) @@ -160,7 +162,7 @@ describe('auth.routes', () => { // Arrange const createLoginOtpSpy = jest .spyOn(AuthService, 'createLoginOtp') - .mockRejectedValueOnce(new Error('some error')) + .mockReturnValueOnce(errAsync(new ApplicationError())) // Act const response = await request @@ -179,7 +181,7 @@ describe('auth.routes', () => { // Arrange const sendLoginOtpSpy = jest .spyOn(MailService, 'sendLoginOtp') - .mockRejectedValueOnce(new Error('some error')) + .mockReturnValueOnce(errAsync(new MailSendError('some error'))) // Act const response = await request @@ -190,15 +192,15 @@ describe('auth.routes', () => { expect(sendLoginOtpSpy).toHaveBeenCalled() expect(response.status).toEqual(500) expect(response.text).toEqual( - 'Error sending OTP. Please try again later and if the problem persists, contact us.', + 'Failed to send login OTP. Please try again later and if the problem persists, contact us.', ) }) - it('should return 500 when validating domain throws an unknown error', async () => { + it('should return 500 when validating domain returns a database error', async () => { // Arrange const getAgencySpy = jest - .spyOn(AuthService, 'getAgencyWithEmail') - .mockRejectedValueOnce(new Error('some error occured')) + .spyOn(AuthService, 'validateEmailDomain') + .mockReturnValueOnce(errAsync(new DatabaseError())) // Act const response = await request @@ -209,7 +211,7 @@ describe('auth.routes', () => { expect(getAgencySpy).toBeCalled() expect(response.status).toEqual(500) expect(response.text).toEqual( - expect.stringContaining('Unable to validate email domain.'), + 'Failed to send login OTP. Please try again later and if the problem persists, contact us.', ) }) @@ -217,7 +219,7 @@ describe('auth.routes', () => { // Arrange const sendLoginOtpSpy = jest .spyOn(MailService, 'sendLoginOtp') - .mockResolvedValueOnce(true) + .mockReturnValueOnce(okAsync(true)) // Act const response = await request @@ -323,11 +325,11 @@ describe('auth.routes', () => { ) }) - it('should return 500 when validating domain throws an unknown error', async () => { + it('should return 500 when validating domain returns a database error', async () => { // Arrange const getAgencySpy = jest - .spyOn(AuthService, 'getAgencyWithEmail') - .mockRejectedValueOnce(new Error('some error occured')) + .spyOn(AuthService, 'validateEmailDomain') + .mockReturnValueOnce(errAsync(new DatabaseError())) // Act const response = await request @@ -337,9 +339,7 @@ describe('auth.routes', () => { // Assert expect(getAgencySpy).toBeCalled() expect(response.status).toEqual(500) - expect(response.text).toEqual( - expect.stringContaining('Unable to validate email domain.'), - ) + expect(response.text).toEqual('Something went wrong. Please try again.') }) it('should return 422 when hash does not exist for body.otp', async () => { @@ -443,10 +443,10 @@ describe('auth.routes', () => { // Request for OTP so the hash exists. await requestForOtp(VALID_EMAIL) - // Mock error thrown when creating user + // Mock error returned when creating user const upsertSpy = jest .spyOn(UserService, 'retrieveUser') - .mockRejectedValueOnce(new Error('some error')) + .mockReturnValueOnce(errAsync(new DatabaseError('some error'))) // Act const response = await request @@ -458,7 +458,7 @@ describe('auth.routes', () => { expect(upsertSpy).toBeCalled() expect(response.status).toEqual(500) expect(response.text).toEqual( - expect.stringContaining('User signin failed. Please try again later'), + expect.stringContaining('Failed to process OTP.'), ) }) }) @@ -512,7 +512,7 @@ describe('auth.routes', () => { // Helper functions const requestForOtp = async (email: string) => { // Set that so no real mail is sent. - jest.spyOn(MailService, 'sendLoginOtp').mockResolvedValue(true) + jest.spyOn(MailService, 'sendLoginOtp').mockReturnValue(okAsync(true)) const response = await request.post('/auth/sendotp').send({ email }) expect(response.text).toEqual(`OTP sent to ${email}!`) diff --git a/src/app/modules/auth/__tests__/auth.service.spec.ts b/src/app/modules/auth/__tests__/auth.service.spec.ts index 540e0ed17d..77d9c8c3c4 100644 --- a/src/app/modules/auth/__tests__/auth.service.spec.ts +++ b/src/app/modules/auth/__tests__/auth.service.spec.ts @@ -36,35 +36,40 @@ describe('auth.service', () => { afterAll(async () => await dbHandler.closeDatabase()) - describe('getAgencyWithEmail', () => { + describe('validateEmailDomain', () => { it('should retrieve agency successfully when email is valid and domain is in Agency collection', async () => { // Act - const actual = await AuthService.getAgencyWithEmail(VALID_EMAIL) + const actual = await AuthService.validateEmailDomain(VALID_EMAIL) // Assert - expect(actual.toObject()).toEqual(defaultAgency.toObject()) + expect(actual.isOk()).toBe(true) + expect(actual._unsafeUnwrap().toObject()).toEqual( + defaultAgency.toObject(), + ) }) - it('should throw InvalidDomainError when email is invalid', async () => { + it('should return InvalidDomainError error result when email is invalid', async () => { // Arrange const notAnEmail = 'not an email' // Act - const actualPromise = AuthService.getAgencyWithEmail(notAnEmail) + const actual = await AuthService.validateEmailDomain(notAnEmail) // Assert - await expect(actualPromise).rejects.toThrowError(InvalidDomainError) + expect(actual.isErr()).toBe(true) + expect(actual._unsafeUnwrapErr()).toEqual(new InvalidDomainError()) }) - it('should throw InvalidDomainError when valid email domain is not in Agency collection', async () => { + it('should return InvalidDomainError error result when valid email domain is not in Agency collection', async () => { // Arrange const invalidEmail = 'invalid@example.com' // Act - const actualPromise = AuthService.getAgencyWithEmail(invalidEmail) + const actual = await AuthService.validateEmailDomain(invalidEmail) // Assert - await expect(actualPromise).rejects.toThrowError(InvalidDomainError) + expect(actual.isErr()).toBe(true) + expect(actual._unsafeUnwrapErr()).toEqual(new InvalidDomainError()) }) }) @@ -75,23 +80,25 @@ describe('auth.service', () => { await expect(TokenModel.countDocuments()).resolves.toEqual(0) // Act - const actualOtp = await AuthService.createLoginOtp(VALID_EMAIL) + const actualResult = await AuthService.createLoginOtp(VALID_EMAIL) // Assert - expect(actualOtp).toEqual(MOCK_OTP) + expect(actualResult.isOk()).toBe(true) + expect(actualResult._unsafeUnwrap()).toEqual(MOCK_OTP) // Should have new token document inserted. await expect(TokenModel.countDocuments()).resolves.toEqual(1) }) - it('should throw InvalidDomainError when email is invalid', async () => { + it('should return with InvalidDomainError when email is invalid', async () => { // Arrange const notAnEmail = 'not an email' // Act - const actualPromise = AuthService.createLoginOtp(notAnEmail) + const actualResult = await AuthService.createLoginOtp(notAnEmail) // Assert - await expect(actualPromise).rejects.toThrowError(InvalidDomainError) + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(InvalidDomainError) }) }) @@ -103,31 +110,39 @@ describe('auth.service', () => { await expect(TokenModel.countDocuments()).resolves.toEqual(1) // Act - const actual = await AuthService.verifyLoginOtp(MOCK_OTP, VALID_EMAIL) + const actualResult = await AuthService.verifyLoginOtp( + MOCK_OTP, + VALID_EMAIL, + ) // Assert // Resolves successfully. - expect(actual).toEqual(true) + expect(actualResult.isOk()).toBe(true) + expect(actualResult._unsafeUnwrap()).toEqual(true) // Token document should be removed. await expect(TokenModel.countDocuments()).resolves.toEqual(0) }) - it('should throw InvalidOtpError when Token document cannot be retrieved', async () => { + it('should return with InvalidOtpError when Token document cannot be retrieved', async () => { // Arrange // No OTP requested; should have no documents prior to acting. await expect(TokenModel.countDocuments()).resolves.toEqual(0) // Act - const verifyPromise = AuthService.verifyLoginOtp(MOCK_OTP, VALID_EMAIL) + const actualResult = await AuthService.verifyLoginOtp( + MOCK_OTP, + VALID_EMAIL, + ) // Assert const expectedError = new InvalidOtpError( 'OTP has expired. Please request for a new OTP.', ) - await expect(verifyPromise).rejects.toThrowError(expectedError) + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) }) - it('should throw InvalidOtpError when verification has been attempted too many times', async () => { + it('should return with InvalidOtpError when verification has been attempted too many times', async () => { // Arrange // Add a Token document to verify against. await AuthService.createLoginOtp(VALID_EMAIL) @@ -138,29 +153,37 @@ describe('auth.service', () => { ) // Act - const verifyPromise = AuthService.verifyLoginOtp(MOCK_OTP, VALID_EMAIL) + const actualResult = await AuthService.verifyLoginOtp( + MOCK_OTP, + VALID_EMAIL, + ) // Assert const expectedError = new InvalidOtpError( 'You have hit the max number of attempts. Please request for a new OTP.', ) - await expect(verifyPromise).rejects.toThrowError(expectedError) + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) }) - it('should throw InvalidOtpError when the OTP hash does not match', async () => { + it('should return with InvalidOtpError when the OTP hash does not match', async () => { // Arrange // Add a Token document to verify against. await AuthService.createLoginOtp(VALID_EMAIL) const invalidOtp = '654321' // Act - const verifyPromise = AuthService.verifyLoginOtp(invalidOtp, VALID_EMAIL) + const actualResult = await AuthService.verifyLoginOtp( + invalidOtp, + VALID_EMAIL, + ) // Assert const expectedError = new InvalidOtpError( 'OTP is invalid. Please try again.', ) - await expect(verifyPromise).rejects.toThrowError(expectedError) + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toEqual(expectedError) }) }) }) diff --git a/src/app/modules/auth/auth.controller.ts b/src/app/modules/auth/auth.controller.ts index 4fa51633a4..c5907ff358 100644 --- a/src/app/modules/auth/auth.controller.ts +++ b/src/app/modules/auth/auth.controller.ts @@ -1,5 +1,4 @@ -import to from 'await-to-js' -import { Request, RequestHandler } from 'express' +import { RequestHandler } from 'express' import { ParamsDictionary } from 'express-serve-static-core' import { StatusCodes } from 'http-status-codes' import { isEmpty } from 'lodash' @@ -8,171 +7,188 @@ import { createLoggerWithLabel } from '../../../config/logger' import { LINKS } from '../../../shared/constants' import MailService from '../../services/mail.service' import { getRequestIp } from '../../utils/request' -import { ApplicationError } from '../core/core.errors' import * as UserService from '../user/user.service' import * as AuthService from './auth.service' -import { ResponseAfter, SessionUser } from './auth.types' +import { SessionUser } from './auth.types' +import { mapRouteError } from './auth.utils' const logger = createLoggerWithLabel(module) /** - * Precondition: AuthMiddlewares.validateDomain must precede this handler. - * @returns 200 regardless, assumed to have passed domain validation. + * Handler for GET /auth/checkuser endpoint. + * @returns 500 when there was an error validating body.email + * @returns 401 when domain of body.email is invalid + * @returns 200 if domain of body.email is valid */ -export const handleCheckUser: RequestHandler = async ( - _req: Request, - res: ResponseAfter['validateDomain'], -) => { - return res.sendStatus(StatusCodes.OK) +export const handleCheckUser: RequestHandler< + ParamsDictionary, + string, + { email: string } +> = async (req, res) => { + // Joi validation ensures existence. + const { email } = req.body + + return AuthService.validateEmailDomain(email) + .map(() => res.sendStatus(StatusCodes.OK)) + .mapErr((error) => { + logger.error({ + message: 'Domain validation error', + meta: { + action: 'handleCheckUser', + ip: getRequestIp(req), + email, + }, + error, + }) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).send(errorMessage) + }) } /** - * Precondition: AuthMiddlewares.validateDomain must precede this handler. + * Handler for POST /auth/sendotp endpoint. + * @return 200 when OTP has been been successfully sent + * @return 401 when email domain is invalid + * @return 500 when unknown errors occurs during generate OTP, or create/send the email that delivers the OTP to the user's email address */ -export const handleLoginSendOtp: RequestHandler = async ( - req: Request<ParamsDictionary, string, { email: string }>, - res: ResponseAfter['validateDomain'], -) => { +export const handleLoginSendOtp: RequestHandler< + ParamsDictionary, + string, + { email: string } +> = async (req, res) => { // Joi validation ensures existence. const { email } = req.body const requestIp = getRequestIp(req) const logMeta = { - action: 'handleSendLoginOtp', + action: 'handleLoginSendOtp', email, ip: requestIp, } - // Create OTP. - const [otpErr, otp] = await to(AuthService.createLoginOtp(email)) - - if (otpErr || !otp) { - logger.error({ - message: 'Error generating OTP', - meta: logMeta, - error: otpErr ?? undefined, - }) - return res - .status(StatusCodes.INTERNAL_SERVER_ERROR) - .send( - 'Failed to send login OTP. Please try again later and if the problem persists, contact us.', + return ( + // Step 1: Validate email domain. + AuthService.validateEmailDomain(email) + // Step 2: Create login OTP. + .andThen(() => AuthService.createLoginOtp(email)) + // Step 3: Send login OTP to email address. + .andThen((otp) => + MailService.sendLoginOtp({ + recipient: email, + otp, + ipAddress: requestIp, + }), ) - } - - // Send OTP. - const [sendErr] = await to( - MailService.sendLoginOtp({ - recipient: email, - otp, - ipAddress: requestIp, - }), + // Step 4a: Successfully sent login otp. + .map(() => { + logger.info({ + message: 'Login OTP sent successfully', + meta: logMeta, + }) + + return res.status(StatusCodes.OK).send(`OTP sent to ${email}!`) + }) + // Step 4b: Error occurred whilst sending otp. + .mapErr((error) => { + logger.error({ + message: 'Error sending login OTP', + meta: logMeta, + error, + }) + const { errorMessage, statusCode } = mapRouteError( + error, + /* coreErrorMessage=*/ 'Failed to send login OTP. Please try again later and if the problem persists, contact us.', + ) + return res.status(statusCode).send(errorMessage) + }) ) - if (sendErr) { - logger.error({ - message: 'Error mailing OTP', - meta: logMeta, - error: sendErr, - }) - - return res - .status(StatusCodes.INTERNAL_SERVER_ERROR) - .send( - 'Error sending OTP. Please try again later and if the problem persists, contact us.', - ) - } - - // Successfully sent login otp. - logger.info({ - message: 'Login OTP sent successfully', - meta: logMeta, - }) - - return res.status(StatusCodes.OK).send(`OTP sent to ${email}!`) } /** - * Precondition: AuthMiddlewares.validateDomain must precede this handler. + * Handler for POST /auth/verifyotp endpoint. + * @returns 200 when user has successfully logged in, with session cookie set + * @returns 401 when the email domain is invalid + * @returns 422 when the OTP is invalid + * @returns 500 when error occurred whilst verifying the OTP */ -export const handleLoginVerifyOtp: RequestHandler = async ( - req: Request< - ParamsDictionary, - string | SessionUser, - { email: string; otp: string } - >, - res: ResponseAfter['validateDomain'], -) => { +export const handleLoginVerifyOtp: RequestHandler< + ParamsDictionary, + string | SessionUser, + { email: string; otp: string } +> = async (req, res) => { // Joi validation ensures existence. const { email, otp } = req.body - // validateDomain middleware will populate agency. - const { agency } = res.locals const logMeta = { action: 'handleLoginVerifyOtp', email, ip: getRequestIp(req), } + const coreErrorMessage = `Failed to process OTP. Please try again later and if the problem persists, submit our Support Form (${LINKS.supportFormLink}).` - const [verifyErr] = await to(AuthService.verifyLoginOtp(otp, email)) - - if (verifyErr) { - logger.warn({ - message: - verifyErr instanceof ApplicationError - ? 'Login OTP is invalid' - : 'Error occurred when trying to validate login OTP', - meta: logMeta, - error: verifyErr, - }) - - if (verifyErr instanceof ApplicationError) { - return res.status(verifyErr.status).send(verifyErr.message) - } - - // Unknown error, return generic error response. - return res - .status(StatusCodes.INTERNAL_SERVER_ERROR) - .send( - 'Failed to validate OTP. Please try again later and if the problem persists, contact us.', - ) - } - - // OTP is valid, proceed to login user. - try { - // TODO (#317): remove usage of non-null assertion - const user = await UserService.retrieveUser(email, agency!) - // Create user object to return to frontend. - const userObj = { ...user.toObject(), agency } - - if (!req.session) { - throw new Error('req.session not found') - } - - // TODO(#212): Should store only userId in session. - // Add user info to session. - req.session.user = userObj as SessionUser - logger.info({ - message: `Successfully logged in user ${user.email}`, - meta: logMeta, - }) - - return res.status(StatusCodes.OK).send(userObj) - } catch (err) { + const validateResult = await AuthService.validateEmailDomain(email) + if (validateResult.isErr()) { + const { error } = validateResult logger.error({ - message: 'Error logging in user', + message: 'Domain validation error', meta: logMeta, - error: err, + error, }) - - return res - .status(StatusCodes.INTERNAL_SERVER_ERROR) - .send( - `User signin failed. Please try again later and if the problem persists, submit our Support Form (${LINKS.supportFormLink}).`, - ) + const { errorMessage, statusCode } = mapRouteError(error) + return res.status(statusCode).send(errorMessage) } + + // Since there is no error, agency is retrieved from validation. + const agency = validateResult.value + + // Step 1: Verify login OTP. + return ( + AuthService.verifyLoginOtp(otp, email) + // Step 2: OTP is valid, retrieve associated user. + .andThen(() => UserService.retrieveUser(email, agency._id)) + // Step 3a: Set session and return user in response. + .map((user) => { + if (!req.session) { + logger.error({ + message: 'Error logging in user; req.session is undefined', + meta: logMeta, + }) + + return res + .status(StatusCodes.INTERNAL_SERVER_ERROR) + .send(coreErrorMessage) + } + + // TODO(#212): Should store only userId in session. + // Add user info to session. + const userObj = user.toObject() as SessionUser + req.session.user = userObj + logger.info({ + message: `Successfully logged in user ${user.email}`, + meta: logMeta, + }) + + return res.status(StatusCodes.OK).send(userObj) + }) + // Step 3b: Error occured in one of the steps. + .mapErr((error) => { + logger.warn({ + message: 'Error occurred when trying to validate login OTP', + meta: logMeta, + error, + }) + + const { errorMessage, statusCode } = mapRouteError( + error, + coreErrorMessage, + ) + return res.status(statusCode).send(errorMessage) + }) + ) } export const handleSignout: RequestHandler = async (req, res) => { - if (isEmpty(req.session)) { + if (!req.session || isEmpty(req.session)) { logger.error({ message: 'Attempted to sign out without a session', meta: { @@ -182,7 +198,7 @@ export const handleSignout: RequestHandler = async (req, res) => { return res.sendStatus(StatusCodes.BAD_REQUEST) } - req.session!.destroy((error) => { + req.session.destroy((error) => { if (error) { logger.error({ message: 'Failed to destroy session', diff --git a/src/app/modules/auth/auth.middlewares.ts b/src/app/modules/auth/auth.middlewares.ts deleted file mode 100644 index 7a19f652e8..0000000000 --- a/src/app/modules/auth/auth.middlewares.ts +++ /dev/null @@ -1,58 +0,0 @@ -import to from 'await-to-js' -import { RequestHandler } from 'express' -import { ParamsDictionary } from 'express-serve-static-core' -import { StatusCodes } from 'http-status-codes' - -import { createLoggerWithLabel } from '../../../config/logger' -import { LINKS } from '../../../shared/constants' -import { getRequestIp } from '../../utils/request' -import { ApplicationError } from '../core/core.errors' - -import * as AuthService from './auth.service' - -const logger = createLoggerWithLabel(module) - -/** - * Middleware to check if domain of email in the body is from a whitelisted - * agency. - * @returns 500 when there was an error validating email - * @returns 401 when email domain is invalid - * @returns sets retrieved agency in `res.locals.agency` and calls next when domain is valid - */ -export const validateDomain: RequestHandler< - ParamsDictionary, - string, - { email: string } -> = async (req, res, next) => { - // Joi validation ensures existence. - const { email } = req.body - - const [validationError, agency] = await to( - AuthService.getAgencyWithEmail(email), - ) - - if (validationError) { - logger.error({ - message: 'Domain validation error', - meta: { - action: 'validateDomain', - ip: getRequestIp(req), - email, - }, - error: validationError, - }) - if (validationError instanceof ApplicationError) { - return res.status(validationError.status).send(validationError.message) - } - return res - .status(StatusCodes.INTERNAL_SERVER_ERROR) - .send( - `Unable to validate email domain. If this issue persists, please submit a Support Form at (${LINKS.supportFormLink}).`, - ) - } - - // Pass down agency to next handler. - res.locals.agency = agency - - return next() -} diff --git a/src/app/modules/auth/auth.routes.ts b/src/app/modules/auth/auth.routes.ts index 8584040a04..de7babc84e 100644 --- a/src/app/modules/auth/auth.routes.ts +++ b/src/app/modules/auth/auth.routes.ts @@ -2,7 +2,6 @@ import { celebrate, Joi, Segments } from 'celebrate' import { Router } from 'express' import * as AuthController from './auth.controller' -import * as AuthMiddlewares from './auth.middlewares' export const AuthRouter = Router() @@ -24,7 +23,6 @@ AuthRouter.post( .message('Please enter a valid email'), }), }), - AuthMiddlewares.validateDomain, AuthController.handleCheckUser, ) @@ -50,7 +48,6 @@ AuthRouter.post( .message('Please enter a valid email'), }), }), - AuthMiddlewares.validateDomain, AuthController.handleLoginSendOtp, ) @@ -81,7 +78,6 @@ AuthRouter.post( .message('Please enter a valid otp'), }), }), - AuthMiddlewares.validateDomain, AuthController.handleLoginVerifyOtp, ) diff --git a/src/app/modules/auth/auth.service.ts b/src/app/modules/auth/auth.service.ts index ac6d34f4b6..9478d5fbaa 100644 --- a/src/app/modules/auth/auth.service.ts +++ b/src/app/modules/auth/auth.service.ts @@ -1,14 +1,21 @@ import bcrypt from 'bcrypt' import mongoose from 'mongoose' +import { errAsync, okAsync, ResultAsync } from 'neverthrow' import validator from 'validator' +import { IAgencySchema, ITokenSchema } from 'src/types' + import config from '../../../config/config' +import { createLoggerWithLabel } from '../../../config/logger' +import { LINKS } from '../../../shared/constants' import getAgencyModel from '../../models/agency.server.model' import getTokenModel from '../../models/token.server.model' import { generateOtp } from '../../utils/otp' +import { ApplicationError, DatabaseError } from '../core/core.errors' import { InvalidDomainError, InvalidOtpError } from './auth.errors' +const logger = createLoggerWithLabel(module) const TokenModel = getTokenModel(mongoose) const AgencyModel = getAgencyModel(mongoose) @@ -19,46 +26,80 @@ export const MAX_OTP_ATTEMPTS = 10 * Validates the domain of the given email. A domain is valid if it exists in * the Agency collection in the database. * @param email the email to validate the domain for - * @returns the agency document with the domain of the email only if it is valid. - * @throws error if database query fails or if agency cannot be found. + * @returns ok(the agency document) with the domain of the email if the domain is valid + * @returns err(InvalidDomainError) if the agency document cannot be found + * @returns err(DatabaseError) if database query fails */ -export const getAgencyWithEmail = async (email: string) => { +export const validateEmailDomain = ( + email: string, +): ResultAsync<IAgencySchema, InvalidDomainError | DatabaseError> => { // Extra guard even if Joi validation has already checked. if (!validator.isEmail(email)) { - throw new InvalidDomainError() + return errAsync(new InvalidDomainError()) } const emailDomain = email.split('@').pop() - const agency = await AgencyModel.findOne({ emailDomain }) - if (!agency) { - throw new InvalidDomainError() - } + return ResultAsync.fromPromise( + AgencyModel.findOne({ emailDomain }).exec(), + (error) => { + logger.error({ + message: 'Database error when retrieving agency', + meta: { + action: 'validateEmailDomain', + emailDomain, + }, + error, + }) - return agency + return new DatabaseError( + `Unable to validate email domain. If this issue persists, please submit a Support Form at (${LINKS.supportFormLink})`, + ) + }, + ).andThen((agency) => { + if (!agency) { + const noAgencyError = new InvalidDomainError() + logger.warn({ + message: 'Agency not found', + meta: { + action: 'retrieveAgency', + emailDomain, + }, + error: noAgencyError, + }) + return errAsync(noAgencyError) + } + return okAsync(agency) + }) } /** * Creates a login OTP and saves its hash into the Token collection. * @param email the email to link the generated otp to - * @returns the generated OTP if saving into DB is successful - * @throws {InvalidDomainError} the given email is invalid - * @throws {Error} if any error occur whilst creating the OTP or insertion of OTP into the database. + * @returns ok(the generated OTP) if saving into DB is successful + * @returns err(InvalidDomainError) if the given email is invalid + * @returns err(ApplicationError) if any error occur whilst hashing the OTP + * @returns err(DatabaseError) if error occurs during upsertion of hashed OTP into the database. */ -export const createLoginOtp = async (email: string) => { +export const createLoginOtp = ( + email: string, +): ResultAsync< + string, + ApplicationError | DatabaseError | InvalidDomainError +> => { if (!validator.isEmail(email)) { - throw new InvalidDomainError() + return errAsync(new InvalidDomainError()) } const otp = generateOtp() - const hashedOtp = await bcrypt.hash(otp, DEFAULT_SALT_ROUNDS) - - await TokenModel.upsertOtp({ - email, - hashedOtp, - expireAt: new Date(Date.now() + config.otpLifeSpan), - }) - return otp + return ( + // Step 1: Hash OTP. + hashOtp(otp, { email }) + // Step 2: Upsert otp hash into database. + .andThen((hashedOtp) => upsertOtp(email, hashedOtp)) + // Step 3: Return generated OTP. + .map(() => otp) + ) } /** @@ -69,37 +110,179 @@ export const createLoginOtp = async (email: string) => { * returned. Else, the document is kept in the database and an error is thrown. * @param otpToVerify the OTP to verify with the hashed counterpart * @param email the email used to retrieve the document from the database - * @returns true on success - * @throws {InvalidOtpError} if the OTP is invalid or expired. - * @throws {Error} if any errors occur whilst retrieving from database or comparing hashes. + * @returns ok(true) on success + * @returns err(InvalidOtpError) if the OTP is invalid or expired + * @returns err(ApplicationError) if any error occurs whilst comparing hashes + * @returns err(DatabaseError) if any errors occur whilst querying the database */ -export const verifyLoginOtp = async (otpToVerify: string, email: string) => { - const updatedDocument = await TokenModel.incrementAttemptsByEmail(email) +export const verifyLoginOtp = ( + otpToVerify: string, + email: string, +): ResultAsync<true, InvalidOtpError | DatabaseError | ApplicationError> => { + return ( + // Step 1: Increment login attempts. + incrementLoginAttempts(email) + // Step 2: Compare otp with saved hash. + .andThen(({ hashedOtp }) => compareOtpHash(otpToVerify, hashedOtp)) + // Step 3: Remove token document from collection since hash matches. + .andThen(() => removeTokenOnSuccess(email)) + // Step 4: Return true (as success). + .map(() => true) + ) +} - // Does not exist, return expired error message. - if (!updatedDocument) { - throw new InvalidOtpError('OTP has expired. Please request for a new OTP.') - } +// Private helper functions +/** + * Hashes the given otp. + * @param otpToHash the otp to hash + * @param logMeta additional metadata for logging, if available + * @returns ok(hashed otp) if the hashing was successful + * @returns err(ApplicationError) if hashing error occurs + */ +const hashOtp = (otpToHash: string, logMeta: Record<string, unknown> = {}) => { + return ResultAsync.fromPromise( + bcrypt.hash(otpToHash, DEFAULT_SALT_ROUNDS), + (error) => { + logger.error({ + message: 'bcrypt hash otp error', + meta: { + action: 'hashOtp', + ...logMeta, + }, + error, + }) - // Too many attempts. - if (updatedDocument.numOtpAttempts! > MAX_OTP_ATTEMPTS) { - throw new InvalidOtpError( - 'You have hit the max number of attempts. Please request for a new OTP.', - ) - } + return new ApplicationError() + }, + ) +} + +/** + * Compares otp with a given hash. + * @param otpToVerify The unhashed OTP to check match for + * @param hashedOtp The hashed OTP to check match for + * @param logMeta additional metadata for logging, if available + * @returns ok(true) if the hash matches + * @returns err(ApplicationError) if error occurs whilst comparing hashes + * @returns err(InvalidOtpError) if OTP hashes do not match + */ +const compareOtpHash = ( + otpToVerify: string, + hashedOtp: string, + logMeta: Record<string, unknown> = {}, +): ResultAsync<true, ApplicationError | InvalidOtpError> => { + return ResultAsync.fromPromise( + bcrypt.compare(otpToVerify, hashedOtp), + (error) => { + logger.error({ + message: 'bcrypt compare otp error', + meta: { + action: 'compareHash', + ...logMeta, + }, + error, + }) - // Compare otp with saved hash. - const isOtpMatch = await bcrypt.compare( - otpToVerify, - updatedDocument.hashedOtp, + return new ApplicationError() + }, + ).andThen((isOtpMatch) => { + if (!isOtpMatch) { + return errAsync(new InvalidOtpError('OTP is invalid. Please try again.')) + } + + return okAsync(isOtpMatch) + }) +} + +/** + * Upserts the given otp hash into the document keyed by the given email + * @param email the email to retrieve the current Token document for + * @param hashedOtp the otp hash to upsert + * @returns ok(upserted Token document) if upsert is successful + * @returns err(DatabaseError) if upsert fails + */ +const upsertOtp = ( + email: string, + hashedOtp: string, +): ResultAsync<ITokenSchema, DatabaseError> => { + return ResultAsync.fromPromise( + TokenModel.upsertOtp({ + email, + hashedOtp, + expireAt: new Date(Date.now() + config.otpLifeSpan), + }), + (error) => { + logger.error({ + message: 'Database upsert OTP error', + meta: { + action: 'upsertOtp', + email, + }, + error, + }) + + return new DatabaseError() + }, ) +} - if (!isOtpMatch) { - throw new InvalidOtpError('OTP is invalid. Please try again.') - } +/** + * Increment login attempts for the given email + * @param email the email to increment login attempts for + * @returns ok(updated document) if increment succeeds + * @returns err(DatabaseError) if any database error occurs whilst updating attempts + * @returns err(InvalidOtpError) if login has expired or if max number of attempts are reached + */ +const incrementLoginAttempts = ( + email: string, +): ResultAsync<ITokenSchema, InvalidOtpError | DatabaseError> => { + return ResultAsync.fromPromise( + TokenModel.incrementAttemptsByEmail(email), + (error) => { + logger.error({ + message: 'Database increment OTP error', + meta: { + action: 'incrementAttempts', + email, + }, + error, + }) + + return new DatabaseError() + }, + ).andThen((upsertedDoc) => { + // Document does not exist, return expired error message. + if (!upsertedDoc || !upsertedDoc.numOtpAttempts) { + return errAsync( + new InvalidOtpError('OTP has expired. Please request for a new OTP.'), + ) + } + if (upsertedDoc.numOtpAttempts > MAX_OTP_ATTEMPTS) { + return errAsync( + new InvalidOtpError( + 'You have hit the max number of attempts. Please request for a new OTP.', + ), + ) + } + + return okAsync(upsertedDoc) + }) +} - // Hashed OTP matches, remove from collection. - await TokenModel.findOneAndRemove({ email }) - // Finally return true (as success). - return true +const removeTokenOnSuccess = (email: string) => { + return ResultAsync.fromPromise( + TokenModel.findOneAndRemove({ email }).exec(), + (error) => { + logger.error({ + message: 'Database remove Token document error', + meta: { + action: 'removeTokenOnSuccess', + email, + }, + error, + }) + + return new DatabaseError() + }, + ) } diff --git a/src/app/modules/auth/auth.types.ts b/src/app/modules/auth/auth.types.ts index 861c0e7a98..ba7e1a50ee 100644 --- a/src/app/modules/auth/auth.types.ts +++ b/src/app/modules/auth/auth.types.ts @@ -1,13 +1,3 @@ -import { IAgencySchema, IPopulatedUser } from 'src/types' - -import { ResponseWithLocals } from '../core/core.types' - -/** - * Meta typing for the shape of the Express.Response object after various - * middlewares for /auth. - */ -export type ResponseAfter = { - validateDomain: ResponseWithLocals<{ agency?: IAgencySchema }> -} +import { IPopulatedUser } from 'src/types' export type SessionUser = IPopulatedUser diff --git a/src/app/modules/auth/auth.utils.ts b/src/app/modules/auth/auth.utils.ts new file mode 100644 index 0000000000..366717de21 --- /dev/null +++ b/src/app/modules/auth/auth.utils.ts @@ -0,0 +1,58 @@ +import { StatusCodes } from 'http-status-codes' + +import { createLoggerWithLabel } from '../../../config/logger' +import { ApplicationError, DatabaseError } from '../core/core.errors' +import { MailSendError } from '../mail/mail.errors' + +import { InvalidDomainError, InvalidOtpError } from './auth.errors' + +const logger = createLoggerWithLabel(module) + +type ErrorResponseData = { + statusCode: StatusCodes + errorMessage: string +} + +/** + * Handler to map ApplicationErrors to their correct status code and error + * messages. + * @param error The error to retrieve the status codes and error messages + * @param coreErrorMessage Any error message to return instead of the default core error message, if any + */ +export const mapRouteError = ( + error: ApplicationError, + coreErrorMessage?: string, +): ErrorResponseData => { + switch (error.constructor) { + case InvalidDomainError: + return { + statusCode: StatusCodes.UNAUTHORIZED, + errorMessage: error.message, + } + case InvalidOtpError: + return { + statusCode: StatusCodes.UNPROCESSABLE_ENTITY, + errorMessage: error.message, + } + case MailSendError: + case ApplicationError: + case DatabaseError: + return { + statusCode: StatusCodes.INTERNAL_SERVER_ERROR, + errorMessage: coreErrorMessage ?? error.message, + } + default: + logger.error({ + message: 'Unknown route error observed', + meta: { + action: 'mapRouteError', + }, + error, + }) + + return { + statusCode: StatusCodes.INTERNAL_SERVER_ERROR, + errorMessage: 'Something went wrong. Please try again.', + } + } +} diff --git a/src/app/modules/core/core.errors.ts b/src/app/modules/core/core.errors.ts index 5c66fdfacb..d6f665ff1f 100644 --- a/src/app/modules/core/core.errors.ts +++ b/src/app/modules/core/core.errors.ts @@ -2,7 +2,7 @@ * A custom base error class that encapsulates the name, message, status code, * and logging meta string (if any) for the error. */ -export abstract class ApplicationError extends Error { +export class ApplicationError extends Error { /** * Http status code for the error to be returned in the response. */ @@ -26,3 +26,9 @@ export abstract class ApplicationError extends Error { this.meta = meta } } + +export class DatabaseError extends ApplicationError { + constructor(message?: string) { + super(message) + } +} diff --git a/src/app/modules/core/core.types.ts b/src/app/modules/core/core.types.ts deleted file mode 100644 index 9c1011c90c..0000000000 --- a/src/app/modules/core/core.types.ts +++ /dev/null @@ -1,3 +0,0 @@ -import { Response } from 'express' - -export type ResponseWithLocals<T> = Omit<Response, 'locals'> & { locals: T } diff --git a/src/app/modules/mail/mail.errors.ts b/src/app/modules/mail/mail.errors.ts new file mode 100644 index 0000000000..a9726ead37 --- /dev/null +++ b/src/app/modules/mail/mail.errors.ts @@ -0,0 +1,9 @@ +import { ApplicationError } from '../core/core.errors' + +export class MailSendError extends ApplicationError { + constructor( + message = 'Error sending OTP. Please try again later and if the problem persists, contact us.', + ) { + super(message) + } +} diff --git a/src/app/modules/user/user.service.ts b/src/app/modules/user/user.service.ts index 358953227a..799e24a902 100644 --- a/src/app/modules/user/user.service.ts +++ b/src/app/modules/user/user.service.ts @@ -1,6 +1,7 @@ import to from 'await-to-js' import bcrypt from 'bcrypt' import mongoose from 'mongoose' +import { errAsync, ResultAsync } from 'neverthrow' import validator from 'validator' import getAdminVerificationModel from '../../../app/models/admin_verification.server.model' @@ -8,11 +9,15 @@ import { AGENCY_SCHEMA_ID } from '../../../app/models/agency.server.model' import getUserModel from '../../../app/models/user.server.model' import { generateOtp } from '../../../app/utils/otp' import config from '../../../config/config' +import { createLoggerWithLabel } from '../../../config/logger' import { IAgency, IPopulatedUser, IUserSchema } from '../../../types' import { InvalidDomainError } from '../auth/auth.errors' +import { DatabaseError } from '../core/core.errors' import { InvalidOtpError, MalformedOtpError } from './user.errors' +const logger = createLoggerWithLabel(module) + const AdminVerificationModel = getAdminVerificationModel(mongoose) const UserModel = getUserModel(mongoose) @@ -161,23 +166,35 @@ export const getPopulatedUserById = async ( * new user document is created and returned. * @param email the email of the user to retrieve * @param agency the agency document to associate with the user - * @returns the upserted user document - * @throws {InvalidDomainError} on invalid email - * @throws {Error} on upsert failure + * @returns ok(upserted user document) populated with agency details + * @returns err(InvalidDomainError) when given email is invalid + * @returns err(DatabaseError) on upsert failure */ -export const retrieveUser = async (email: string, agency: IAgency) => { +export const retrieveUser = ( + email: string, + agencyId: IAgency['_id'], +): ResultAsync<IPopulatedUser, InvalidDomainError | DatabaseError> => { if (!validator.isEmail(email)) { - throw new InvalidDomainError() + return errAsync(new InvalidDomainError()) } - const admin = await UserModel.upsertUser({ - email, - agency: agency._id, - }) - - if (!admin) { - throw new Error('Failed to upsert user') - } - - return admin + return ResultAsync.fromPromise( + UserModel.upsertUser({ + email, + agency: agencyId, + }), + (error) => { + logger.error({ + message: 'Database error when upserting user', + meta: { + action: 'retrieveUser', + email, + agencyId, + }, + error, + }) + + return new DatabaseError() + }, + ) } diff --git a/src/app/services/mail.service.ts b/src/app/services/mail.service.ts index 4803b4853b..f325d2f536 100644 --- a/src/app/services/mail.service.ts +++ b/src/app/services/mail.service.ts @@ -1,5 +1,6 @@ import { get, inRange, isEmpty } from 'lodash' import moment from 'moment-timezone' +import { ResultAsync } from 'neverthrow' import Mail from 'nodemailer/lib/mailer' import promiseRetry from 'promise-retry' import { OperationOptions } from 'retry' @@ -16,6 +17,7 @@ import { ISubmissionSchema, } from '../../types' import { EMAIL_HEADERS, EmailType } from '../constants/mail' +import { MailSendError } from '../modules/mail/mail.errors' import { generateAutoreplyHtml, generateAutoreplyPdf, @@ -302,9 +304,10 @@ export class MailService { * Sends a login otp email to a valid email * @param recipient the recipient email address * @param otp the OTP to send - * @throws error if mail fails, to be handled by the caller + * @returns ok(never) if sending of mail succeeds + * @returns err(MailSendError) if sending of mail fails, to be handled by the caller */ - sendLoginOtp = async ({ + sendLoginOtp = ({ recipient, otp, ipAddress, @@ -312,23 +315,39 @@ export class MailService { recipient: string otp: string ipAddress: string - }) => { - const mail: MailOptions = { - to: recipient, - from: this.#senderFromString, - subject: `One-Time Password (OTP) for ${this.#appName}`, - html: await generateLoginOtpHtml({ - appName: this.#appName, - appUrl: this.#appUrl, - ipAddress: ipAddress, - otp, - }), - headers: { - [EMAIL_HEADERS.emailType]: EmailType.LoginOtp, - }, - } + }): ResultAsync<any, MailSendError> => { + return generateLoginOtpHtml({ + appName: this.#appName, + appUrl: this.#appUrl, + ipAddress: ipAddress, + otp, + }).andThen((loginHtml) => { + const mail: MailOptions = { + to: recipient, + from: this.#senderFromString, + subject: `One-Time Password (OTP) for ${this.#appName}`, + html: loginHtml, + headers: { + [EMAIL_HEADERS.emailType]: EmailType.LoginOtp, + }, + } - return this.#sendNodeMail(mail, { mailId: 'OTP' }) + return ResultAsync.fromPromise( + this.#sendNodeMail(mail, { mailId: 'OTP' }), + (error) => { + logger.error({ + message: 'Error sending login OTP to email', + meta: { + action: 'sendLoginOtp', + recipient, + }, + error, + }) + + return new MailSendError() + }, + ) + }) } /** diff --git a/src/app/utils/mail.ts b/src/app/utils/mail.ts index 87e25f420d..f8179ed539 100644 --- a/src/app/utils/mail.ts +++ b/src/app/utils/mail.ts @@ -1,21 +1,39 @@ import dedent from 'dedent-js' import ejs from 'ejs' import { flattenDeep } from 'lodash' +import { ResultAsync } from 'neverthrow' import puppeteer from 'puppeteer-core' import validator from 'validator' import { IFormSchema, ISubmissionSchema } from 'src/types' import config from '../../config/config' +import { createLoggerWithLabel } from '../../config/logger' +import { MailSendError } from '../modules/mail/mail.errors' + +const logger = createLoggerWithLabel(module) export const generateLoginOtpHtml = (htmlData: { otp: string appName: string appUrl: string ipAddress: string -}): Promise<string> => { +}): ResultAsync<string, MailSendError> => { const pathToTemplate = `${process.cwd()}/src/app/views/templates/otp-email.server.view.html` - return ejs.renderFile(pathToTemplate, htmlData) + return ResultAsync.fromPromise( + ejs.renderFile(pathToTemplate, htmlData), + (error) => { + logger.error({ + message: 'Error occurred whilst rendering login otp ejs', + meta: { + action: 'generateLoginOtpHtml', + }, + error, + }) + + return new MailSendError() + }, + ) } export const generateVerificationOtpHtml = ({ diff --git a/src/config/logger.ts b/src/config/logger.ts index bf4ef0e4b3..2254a39f3e 100644 --- a/src/config/logger.ts +++ b/src/config/logger.ts @@ -20,7 +20,7 @@ type CustomLoggerParams = { action: string [other: string]: any } - error?: Error + error?: unknown } // A variety of helper functions to make winston logging like console logging, diff --git a/src/types/token.ts b/src/types/token.ts index f808a833d7..55d4332d71 100644 --- a/src/types/token.ts +++ b/src/types/token.ts @@ -18,5 +18,7 @@ export interface ITokenModel extends Model<ITokenSchema> { params: Omit<IToken, '_id' | 'numOtpSent'>, ) => Promise<ITokenSchema> - incrementAttemptsByEmail: (email: IToken['email']) => Promise<ITokenSchema> + incrementAttemptsByEmail: ( + email: IToken['email'], + ) => Promise<ITokenSchema | null> } diff --git a/src/types/user.ts b/src/types/user.ts index 01ab753ab1..ed8a7f8762 100644 --- a/src/types/user.ts +++ b/src/types/user.ts @@ -20,9 +20,16 @@ export interface IUserSchema extends IUser, Document { } export interface IUserModel extends Model<IUserSchema> { + /** + * Upsert into User collection with given email and agencyId. + * If user with given email does not exist, a new document will be created. + * If user with given email already exists, the user's agency will be updated + * and populated before being returned. + * @returns the user document after upsert with populated agency details + */ upsertUser: ( upsertParams: Pick<IUser, 'email' | 'agency'>, - ) => Promise<IUserSchema> + ) => Promise<IPopulatedUser> } export interface IPopulatedUser extends IUserSchema { diff --git a/tests/unit/backend/helpers/jest-express.ts b/tests/unit/backend/helpers/jest-express.ts index ed91e6fed2..051dc86f88 100644 --- a/tests/unit/backend/helpers/jest-express.ts +++ b/tests/unit/backend/helpers/jest-express.ts @@ -10,7 +10,7 @@ const mockRequest = <P extends Record<string, string>, B>({ body?: B session?: any secure?: boolean -} = {}) => { +} = {}): Request<P, any, B> => { return { body: body ?? {}, params: params ?? {}, diff --git a/tests/unit/backend/modules/user/user.service.spec.ts b/tests/unit/backend/modules/user/user.service.spec.ts index d14e0a8447..3f3ac51c01 100644 --- a/tests/unit/backend/modules/user/user.service.spec.ts +++ b/tests/unit/backend/modules/user/user.service.spec.ts @@ -3,13 +3,16 @@ import mongoose from 'mongoose' import { ImportMock } from 'ts-mock-imports' import getAdminVerificationModel from 'src/app/models/admin_verification.server.model' +import getUserModel from 'src/app/models/user.server.model' +import { InvalidDomainError } from 'src/app/modules/auth/auth.errors' import * as UserService from 'src/app/modules/user/user.service' import * as OtpUtils from 'src/app/utils/otp' -import { IAgencySchema, IUserSchema } from 'src/types' +import { IAgencySchema, IPopulatedUser, IUserSchema } from 'src/types' import dbHandler from '../../helpers/jest-db' const AdminVerification = getAdminVerificationModel(mongoose) +const UserModel = getUserModel(mongoose) describe('user.service', () => { // Obtained from Twilio's @@ -17,27 +20,23 @@ describe('user.service', () => { const MOCK_CONTACT = '+15005550006' const MOCK_OTP = '123456' const USER_ID = new ObjectID() + const ALLOWED_DOMAIN = 'test.gov.sg' let defaultAgency: IAgencySchema let defaultUser: IUserSchema - beforeAll(async () => { - await dbHandler.connect() - + beforeAll(async () => await dbHandler.connect()) + beforeEach(async () => { // Insert user into collections. const { agency, user } = await dbHandler.insertFormCollectionReqs({ userId: USER_ID, + mailDomain: ALLOWED_DOMAIN, }) defaultAgency = agency.toObject() defaultUser = user.toObject() }) - beforeEach( - async () => - await dbHandler.clearCollection( - AdminVerification.collection.collectionName, - ), - ) + afterEach(async () => await dbHandler.clearDatabase()) afterAll(async () => await dbHandler.closeDatabase()) describe('createContactOtp', () => { @@ -232,4 +231,71 @@ describe('user.service', () => { await expect(userPromise).resolves.toBeNull() }) }) + + describe('retrieveUser', () => { + it('should return InvalidDomainError on invalid email', async () => { + // Arrange + const notAnEmail = 'not an email' + + // Act + const actualResult = await UserService.retrieveUser( + notAnEmail, + defaultAgency._id, + ) + + // Assert + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(InvalidDomainError) + }) + + it('should return new User document when user does not yet exist in the collection', async () => { + // Arrange + const newUserEmail = `newUser@${ALLOWED_DOMAIN}` + // Should have the default user document in collection. + await expect(UserModel.countDocuments()).resolves.toEqual(1) + + // Act + const actualResult = await UserService.retrieveUser( + newUserEmail, + defaultAgency._id, + ) + + // Assert + const expectedUser: Partial<IPopulatedUser> = { + agency: defaultAgency, + email: newUserEmail, + } + expect(actualResult.isOk()).toBe(true) + // Should now have 2 user documents + await expect(UserModel.countDocuments()).resolves.toEqual(2) + expect(actualResult._unsafeUnwrap().toObject()).toEqual( + expect.objectContaining(expectedUser), + ) + }) + + it('should return existing User document when user already exists', async () => { + // Arrange + // Should have the default user document in collection. + await expect(UserModel.countDocuments()).resolves.toEqual(1) + const userEmail = defaultUser.email + + // Act + const actualResult = await UserService.retrieveUser( + userEmail, + defaultAgency._id, + ) + + // Assert + const expectedUser: Partial<IPopulatedUser> = { + ...defaultUser, + agency: defaultAgency, + } + expect(actualResult.isOk()).toBe(true) + // Should still only have 1 user document. + await expect(UserModel.countDocuments()).resolves.toEqual(1) + expect(actualResult._unsafeUnwrap().toObject()).toEqual( + expect.objectContaining(expectedUser), + ) + }) + }) }) diff --git a/tests/unit/backend/services/mail.service.spec.ts b/tests/unit/backend/services/mail.service.spec.ts index 5193414b88..798022b4ea 100644 --- a/tests/unit/backend/services/mail.service.spec.ts +++ b/tests/unit/backend/services/mail.service.spec.ts @@ -3,6 +3,7 @@ import moment from 'moment-timezone' import Mail, { Attachment } from 'nodemailer/lib/mailer' import { ImportMock } from 'ts-mock-imports' +import { MailSendError } from 'src/app/modules/mail/mail.errors' import { AutoreplySummaryRenderData, MailOptions, @@ -48,7 +49,7 @@ describe('mail.service', () => { }) describe('Constructor', () => { - it('should throw error when invalid senderMail param is passed ', () => { + it('should throw error when invalid senderMail param is passed', () => { // Arrange const invalidParams = { transporter: mockTransporter, @@ -211,12 +212,14 @@ describe('mail.service', () => { to: MOCK_VALID_EMAIL, from: MOCK_SENDER_STRING, subject: `One-Time Password (OTP) for ${MOCK_APP_NAME}`, - html: await MailUtils.generateLoginOtpHtml({ - otp: MOCK_OTP, - appName: MOCK_APP_NAME, - appUrl: MOCK_APP_URL, - ipAddress: MOCK_IP, - }), + html: ( + await MailUtils.generateLoginOtpHtml({ + otp: MOCK_OTP, + appName: MOCK_APP_NAME, + appUrl: MOCK_APP_URL, + ipAddress: MOCK_IP, + }) + )._unsafeUnwrap(), headers: { // Hardcode in tests in case something changes this. 'X-Formsg-Email-Type': 'Login OTP', @@ -233,31 +236,33 @@ describe('mail.service', () => { const expectedArgument = await generateExpectedArg() // Act - const pendingSend = mailService.sendLoginOtp({ + const actualResult = await mailService.sendLoginOtp({ recipient: MOCK_VALID_EMAIL, otp: MOCK_OTP, ipAddress: MOCK_IP, }) // Assert - await expect(pendingSend).resolves.toEqual(mockedResponse) + expect(actualResult.isOk()).toBe(true) + expect(actualResult._unsafeUnwrap()).toEqual(mockedResponse) // Check arguments passed to sendNodeMail expect(sendMailSpy).toHaveBeenCalledTimes(1) expect(sendMailSpy).toHaveBeenCalledWith(expectedArgument) }) - it('should reject with error when email is invalid', async () => { + it('should return a MailSendError when email is invalid', async () => { // Arrange const invalidEmail = 'notAnEmail' // Act - const pendingSend = mailService.sendLoginOtp({ + const actualResult = await mailService.sendLoginOtp({ recipient: invalidEmail, otp: MOCK_OTP, ipAddress: MOCK_IP, }) // Assert - await expect(pendingSend).rejects.toThrowError('Invalid email error') + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(MailSendError) }) it('should autoretry when 4xx error is thrown by sendNodeMail and pass if second try passes', async () => { @@ -275,14 +280,15 @@ describe('mail.service', () => { const expectedArgument = await generateExpectedArg() // Act - const pendingSendLoginOtp = mailService.sendLoginOtp({ + const actualResult = await mailService.sendLoginOtp({ recipient: MOCK_VALID_EMAIL, otp: MOCK_OTP, ipAddress: MOCK_IP, }) // Assert - await expect(pendingSendLoginOtp).resolves.toEqual(mockedResponse) + expect(actualResult.isOk()).toBe(true) + expect(actualResult._unsafeUnwrap()).toEqual(mockedResponse) // Check arguments passed to sendNodeMail // Should have been called two times since it rejected the first one and // resolved @@ -301,14 +307,15 @@ describe('mail.service', () => { const expectedArgument = await generateExpectedArg() // Act - const pendingSendLoginOtp = mailService.sendLoginOtp({ + const actualResult = await mailService.sendLoginOtp({ recipient: MOCK_VALID_EMAIL, otp: MOCK_OTP, ipAddress: MOCK_IP, }) // Assert - await expect(pendingSendLoginOtp).rejects.toEqual(mock4xxReject) + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(MailSendError) // Check arguments passed to sendNodeMail // Should have been called MOCK_RETRY_COUNT + 1 times expect(sendMailSpy).toHaveBeenCalledTimes(MOCK_RETRY_COUNT + 1) @@ -329,14 +336,15 @@ describe('mail.service', () => { const expectedArgument = await generateExpectedArg() // Act - const pendingSendLoginOtp = mailService.sendLoginOtp({ + const actualResult = await mailService.sendLoginOtp({ recipient: MOCK_VALID_EMAIL, otp: MOCK_OTP, ipAddress: MOCK_IP, }) // Assert - await expect(pendingSendLoginOtp).rejects.toEqual(mockError) + expect(actualResult.isErr()).toBe(true) + expect(actualResult._unsafeUnwrapErr()).toBeInstanceOf(MailSendError) // Check arguments passed to sendNodeMail // Should only invoke two times and stop since the second rejected value // is non-4xx error. @@ -850,7 +858,7 @@ describe('mail.service', () => { html: expectedMailBody, // Attachments should be concatted with mock pdf response attachments: [ - ...MOCK_AUTOREPLY_PARAMS.attachments!, + ...(MOCK_AUTOREPLY_PARAMS.attachments ?? []), { content: MOCK_PDF, filename: 'response.pdf',