fix(auth): make login emails case-insensitive (#2125)

* fix(auth): lowercase auth emails through Joi * test: add tests for email case-insensitivity * refactor: move lowercase after email validation message * test: fix POST endpoint in new tests * test: fix remaining tests
opengovsg · Jun 10, 2021 · 3cf26b8 · 3cf26b8
1 parent c5e2931
commit 3cf26b8
Show file tree

Hide file tree

Showing 6 changed files with 142 additions and 10 deletions.
diff --git a/src/app/modules/auth/__tests__/auth.routes.spec.ts b/src/app/modules/auth/__tests__/auth.routes.spec.ts
@@ -97,6 +97,23 @@ describe('auth.routes', () => {
       expect(response.text).toEqual('OK')
     })
 
+    it('should return 200 when domain of body.email has a case-insensitive match in Agency collection', async () => {
+      // Arrange
+      // Insert agency
+      const validDomain = 'example.com'
+      const validEmail = `test@${validDomain}`
+      await dbHandler.insertAgency({ mailDomain: validDomain })
+
+      // Act
+      const response = await request
+        .post('/auth/checkuser')
+        .send({ email: validEmail.toUpperCase() })
+
+      // Assert
+      expect(response.status).toEqual(200)
+      expect(response.text).toEqual('OK')
+    })
+
     it('should return 500 when validating domain returns a database error', async () => {
       // Arrange
       // Insert agency
@@ -251,6 +268,23 @@ describe('auth.routes', () => {
       expect(response.status).toEqual(200)
       expect(response.body).toEqual(`OTP sent to ${VALID_EMAIL}`)
     })
+
+    it('should return 200 when otp is sent successfully and email is non-lowercase', async () => {
+      // Arrange
+      const sendLoginOtpSpy = jest
+        .spyOn(MailService, 'sendLoginOtp')
+        .mockReturnValueOnce(okAsync(true))
+
+      // Act
+      const response = await request
+        .post('/auth/sendotp')
+        .send({ email: VALID_EMAIL.toUpperCase() })
+
+      // Assert
+      expect(sendLoginOtpSpy).toHaveBeenCalled()
+      expect(response.status).toEqual(200)
+      expect(response.body).toEqual(`OTP sent to ${VALID_EMAIL}`)
+    })
   })
 
   describe('POST /auth/verifyotp', () => {
@@ -476,6 +510,33 @@ describe('auth.routes', () => {
       expect(sessionCookie).toBeDefined()
     })
 
+    it('should return 200 with user object when body.otp is a valid OTP and body.email is non-lowercase', async () => {
+      // Arrange
+      // Request for OTP so the hash exists.
+      await requestForOtp(VALID_EMAIL)
+
+      // Act
+      const response = await request
+        .post('/auth/verifyotp')
+        .send({ email: VALID_EMAIL.toUpperCase(), otp: MOCK_VALID_OTP })
+
+      // Assert
+      expect(response.status).toEqual(200)
+      // Body should be an user object.
+      expect(response.body).toMatchObject({
+        // Required since that's how the data is sent out from the application.
+        agency: jsonParseStringify(defaultAgency.toObject()),
+        _id: expect.any(String),
+        created: expect.any(String),
+        email: VALID_EMAIL,
+      })
+      // Should have session cookie returned.
+      const sessionCookie = request.cookies.find(
+        (cookie) => cookie.name === 'connect.sid',
+      )
+      expect(sessionCookie).toBeDefined()
+    })
+
     it('should return 500 when upserting user document fails', async () => {
       // Arrange
       // Request for OTP so the hash exists.

diff --git a/src/app/modules/auth/auth.routes.ts b/src/app/modules/auth/auth.routes.ts
@@ -23,7 +23,8 @@ AuthRouter.post(
       email: Joi.string()
         .required()
         .email()
-        .message('Please enter a valid email'),
+        .message('Please enter a valid email')
+        .lowercase(),
     }),
   }),
   AuthController.handleCheckUser,
@@ -49,7 +50,8 @@ AuthRouter.post(
       email: Joi.string()
         .required()
         .email()
-        .message('Please enter a valid email'),
+        .message('Please enter a valid email')
+        .lowercase(),
     }),
   }),
   AuthController.handleLoginSendOtp,
@@ -75,7 +77,8 @@ AuthRouter.post(
       email: Joi.string()
         .required()
         .email()
-        .message('Please enter a valid email'),
+        .message('Please enter a valid email')
+        .lowercase(),
       otp: Joi.string()
         .required()
         .regex(/^\d{6}$/)

diff --git a/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts b/src/app/routes/api/v3/admin/forms/__tests__/admin-forms.settings.routes.spec.ts
@@ -301,7 +301,9 @@ describe('admin-form.settings.routes', () => {
       })
       const session = await createAuthedSession(fakeUser.email, request)
       const expectedResponse = jsonParseStringify({
-        message: `User ${fakeUser.email} not authorized to perform write operation on Form ${form._id} with title: ${form.title}.`,
+        message: `User ${fakeUser.email.toLowerCase()} not authorized to perform write operation on Form ${
+          form._id
+        } with title: ${form.title}.`,
       })
 
       // Act
@@ -431,12 +433,14 @@ describe('admin-form.settings.routes', () => {
         },
       })
       const fakeUser = await dbHandler.insertUser({
-        mailName: 'fakeUser',
+        mailName: 'userWithoutReadPermissions',
         agencyId: new ObjectId(),
       })
       const session = await createAuthedSession(fakeUser.email, request)
       const expectedResponse = jsonParseStringify({
-        message: `User ${fakeUser.email} not authorized to perform read operation on Form ${form._id} with title: ${form.title}.`,
+        message: `User ${fakeUser.email.toLowerCase()} not authorized to perform read operation on Form ${
+          form._id
+        } with title: ${form.title}.`,
       })
 
       // Act

diff --git a/src/app/routes/api/v3/auth/__tests__/auth.routes.spec.ts b/src/app/routes/api/v3/auth/__tests__/auth.routes.spec.ts
@@ -96,6 +96,23 @@ describe('auth.routes', () => {
       expect(response.text).toEqual('OK')
     })
 
+    it('should return 200 when domain of body.email has a case-insensitive match in Agency collection', async () => {
+      // Arrange
+      // Insert agency
+      const validDomain = 'example.com'
+      const validEmail = `test@${validDomain}`
+      await dbHandler.insertAgency({ mailDomain: validDomain })
+
+      // Act
+      const response = await request
+        .post('/auth/email/validate')
+        .send({ email: validEmail.toUpperCase() })
+
+      // Assert
+      expect(response.status).toEqual(200)
+      expect(response.text).toEqual('OK')
+    })
+
     it('should return 500 when validating domain returns a database error', async () => {
       // Arrange
       // Insert agency
@@ -250,6 +267,23 @@ describe('auth.routes', () => {
       expect(response.status).toEqual(200)
       expect(response.body).toEqual(`OTP sent to ${VALID_EMAIL}`)
     })
+
+    it('should return 200 when otp is sent successfully and email is non-lowercase', async () => {
+      // Arrange
+      const sendLoginOtpSpy = jest
+        .spyOn(MailService, 'sendLoginOtp')
+        .mockReturnValueOnce(okAsync(true))
+
+      // Act
+      const response = await request
+        .post('/auth/otp/generate')
+        .send({ email: VALID_EMAIL.toUpperCase() })
+
+      // Assert
+      expect(sendLoginOtpSpy).toHaveBeenCalled()
+      expect(response.status).toEqual(200)
+      expect(response.body).toEqual(`OTP sent to ${VALID_EMAIL}`)
+    })
   })
 
   describe('POST /auth/otp/verify', () => {
@@ -475,6 +509,33 @@ describe('auth.routes', () => {
       expect(sessionCookie).toBeDefined()
     })
 
+    it('should return 200 with user object when body.otp is a valid OTP and body.email is non-lowercase', async () => {
+      // Arrange
+      // Request for OTP so the hash exists.
+      await requestForOtp(VALID_EMAIL)
+
+      // Act
+      const response = await request
+        .post('/auth/otp/verify')
+        .send({ email: VALID_EMAIL.toUpperCase(), otp: MOCK_VALID_OTP })
+
+      // Assert
+      expect(response.status).toEqual(200)
+      // Body should be an user object.
+      expect(response.body).toMatchObject({
+        // Required since that's how the data is sent out from the application.
+        agency: JSON.parse(JSON.stringify(defaultAgency.toObject())),
+        _id: expect.any(String),
+        created: expect.any(String),
+        email: VALID_EMAIL,
+      })
+      // Should have session cookie returned.
+      const sessionCookie = request.cookies.find(
+        (cookie) => cookie.name === 'connect.sid',
+      )
+      expect(sessionCookie).toBeDefined()
+    })
+
     it('should return 500 when upserting user document fails', async () => {
       // Arrange
       // Request for OTP so the hash exists.

diff --git a/src/app/routes/api/v3/auth/auth.routes.ts b/src/app/routes/api/v3/auth/auth.routes.ts
@@ -21,7 +21,8 @@ AuthRouter.post(
       email: Joi.string()
         .required()
         .email()
-        .message('Please enter a valid email'),
+        .message('Please enter a valid email')
+        .lowercase(),
     }),
   }),
   AuthController.handleCheckUser,
@@ -47,7 +48,8 @@ AuthRouter.post(
       email: Joi.string()
         .required()
         .email()
-        .message('Please enter a valid email'),
+        .message('Please enter a valid email')
+        .lowercase(),
     }),
   }),
   AuthController.handleLoginSendOtp,
@@ -73,7 +75,8 @@ AuthRouter.post(
       email: Joi.string()
         .required()
         .email()
-        .message('Please enter a valid email'),
+        .message('Please enter a valid email')
+        .lowercase(),
       otp: Joi.string()
         .required()
         .regex(/^\d{6}$/)

diff --git a/tests/integration/helpers/express-auth.ts b/tests/integration/helpers/express-auth.ts
@@ -35,7 +35,7 @@ export const createAuthedSession = async (
 
   const sendOtpResponse = await request.post('/auth/sendotp').send({ email })
   expect(sendOtpResponse.status).toEqual(200)
-  expect(sendOtpResponse.body).toEqual(`OTP sent to ${email}`)
+  expect(sendOtpResponse.body).toEqual(`OTP sent to ${email.toLowerCase()}`)
 
   // Act
   await request.post('/auth/verifyotp').send({ email, otp: MOCK_VALID_OTP })