PLU-341: User last logged in tracking (#801)

## Problem There is no last login tracking for users. Closes https://linear.app/ogp/issue/PLU-341/user-last-logged-in-tracking ## Solution - Create a new `last_login_at` column in the `users` table - Update this column when user logs in successfully via OTP or sgID **Improvements**: - Added test cases for `getOrCreateUser` function ## Before & After Screenshots **BEFORE**: ![Screenshot 2024-11-20 at 3 45 29 PM](https://github.com/user-attachments/assets/5a84c7b0-ae99-4485-877f-7cad43f77a71) **AFTER**: ![Screenshot 2024-11-20 at 3 45 52 PM](https://github.com/user-attachments/assets/34510761-a22e-4d8c-884b-4ad9beb8092b) * OTP Login * Before login ![Screenshot 2024-11-20 at 3 46 28 PM](https://github.com/user-attachments/assets/ba8bdb7f-c668-4751-92b7-50295cbfd02a) * After login https://github.com/user-attachments/assets/c78bd337-3b03-4295-a0ea-687aaa9723c2 ![Screenshot 2024-11-20 at 3 54 50 PM](https://github.com/user-attachments/assets/feccac00-ea1d-43cf-b450-ed5d96008964) * Singpass Login * Before login ![Screenshot 2024-11-20 at 3 55 29 PM](https://github.com/user-attachments/assets/8dee96ff-13fa-44cc-b583-9b17efe028e0) * After login https://github.com/user-attachments/assets/dc7dfe5a-8298-4484-a39a-cbcc44f0ae6d (Singpass was slow in sending the profile details) ![Screenshot 2024-11-20 at 4 01 30 PM](https://github.com/user-attachments/assets/ca8d7d70-16b0-47c0-b29f-75c4271da125) ## Tests - [x] Login via OTP and check that `last_login_at` is updated correctly - [x] Login via Singpass / sgID and check that `last_login_at` is updated correctly - [x] Key in wrong OTP multiple times and check that `last_login_at` is not updated ## Deploy Notes Note, need to run DB migration to add `last_login_at` column before deploying. **New scripts**: - `packages/backend/src/db/migrations/20241119090013_add_user_last_login_at.ts` : DB migration script to create `last_login_at` column in `users` table
opengovsg · Nov 29, 2024 · ee84eee · ee84eee
1 parent 63406ca
commit ee84eee
Show file tree

Hide file tree

Showing 9 changed files with 180 additions and 3 deletions.
diff --git a/packages/backend/src/db/migrations/20241119090013_add_user_last_login_at.ts b/packages/backend/src/db/migrations/20241119090013_add_user_last_login_at.ts
@@ -0,0 +1,13 @@
+import { Knex } from 'knex'
+
+export async function up(knex: Knex): Promise<void> {
+  return knex.schema.table('users', (table) => {
+    table.timestamp('last_login_at').nullable()
+  })
+}
+
+export async function down(knex: Knex): Promise<void> {
+  return knex.schema.table('users', (table) => {
+    table.dropColumn('last_login_at')
+  })
+}
diff --git a/packages/backend/src/graphql/__tests__/mutations/login-with-selected-sgid.test.ts b/packages/backend/src/graphql/__tests__/mutations/login-with-selected-sgid.test.ts
@@ -8,13 +8,15 @@ import type Context from '@/types/express/context'
 const mocks = vi.hoisted(() => ({
   setAuthCookie: vi.fn(),
   getOrCreateUser: vi.fn(),
+  updateLastLogin: vi.fn(),
   logError: vi.fn(),
   verifyJwt: vi.fn(),
 }))
 
 vi.mock('@/helpers/auth', () => ({
   setAuthCookie: mocks.setAuthCookie,
   getOrCreateUser: mocks.getOrCreateUser,
+  updateLastLogin: mocks.updateLastLogin,
 }))
 
 vi.mock('jsonwebtoken', () => ({
@@ -79,6 +81,7 @@ describe('Login with selected SGID', () => {
     expect(mocks.getOrCreateUser).toHaveBeenCalledWith(
       '[email protected]',
     )
+    expect(mocks.updateLastLogin).toHaveBeenCalledWith('abc-def')
     expect(mocks.setAuthCookie).toHaveBeenCalledWith(expect.anything(), {
       userId: 'abc-def',
     })
@@ -118,6 +121,7 @@ describe('Login with selected SGID', () => {
     ).rejects.toThrowError('Invalid work email')
 
     expect(mocks.getOrCreateUser).not.toBeCalled()
+    expect(mocks.updateLastLogin).not.toBeCalled()
     expect(mocks.setAuthCookie).not.toBeCalled()
   })
 

diff --git a/packages/backend/src/graphql/__tests__/mutations/login-with-sgid.test.ts b/packages/backend/src/graphql/__tests__/mutations/login-with-sgid.test.ts
@@ -9,6 +9,7 @@ const mocks = vi.hoisted(() => ({
   sgidUserInfo: vi.fn(),
   setAuthCookie: vi.fn(),
   getOrCreateUser: vi.fn(),
+  updateLastLogin: vi.fn(),
   isWhitelistedEmail: vi.fn(),
   logError: vi.fn(),
   setCookie: vi.fn(),
@@ -44,6 +45,7 @@ vi.mock('@opengovsg/sgid-client', () => ({
 vi.mock('@/helpers/auth', () => ({
   setAuthCookie: mocks.setAuthCookie,
   getOrCreateUser: mocks.getOrCreateUser,
+  updateLastLogin: mocks.updateLastLogin,
 }))
 
 vi.mock('@/models/login-whitelist-entry', () => ({
@@ -89,6 +91,7 @@ describe('Login with SGID', () => {
     expect(mocks.getOrCreateUser).toHaveBeenCalledWith(
       '[email protected]',
     )
+    expect(mocks.updateLastLogin).toHaveBeenCalledWith('abc-def')
     expect(mocks.setAuthCookie).toHaveBeenCalledWith(expect.anything(), {
       userId: 'abc-def',
     })
@@ -139,6 +142,7 @@ describe('Login with SGID', () => {
     const result = await loginWithSgid(null, STUB_PARAMS, STUB_CONTEXT)
 
     expect(mocks.getOrCreateUser).not.toBeCalled()
+    expect(mocks.updateLastLogin).not.toBeCalled()
     expect(mocks.setAuthCookie).not.toBeCalled()
     expect(result.publicOfficerEmployments).toEqual([])
   })
@@ -176,6 +180,7 @@ describe('Login with SGID', () => {
     const result = await loginWithSgid(null, STUB_PARAMS, STUB_CONTEXT)
 
     expect(mocks.getOrCreateUser).toHaveBeenCalledWith('[email protected]')
+    expect(mocks.updateLastLogin).toHaveBeenCalledWith('abc-def')
     expect(mocks.setAuthCookie).toHaveBeenCalledWith(expect.anything(), {
       userId: 'abc-def',
     })
@@ -254,6 +259,7 @@ describe('Login with SGID', () => {
       },
     )
     expect(mocks.getOrCreateUser).not.toBeCalled()
+    expect(mocks.updateLastLogin).not.toBeCalled()
     expect(mocks.setAuthCookie).not.toBeCalled()
   })
 
@@ -270,6 +276,7 @@ describe('Login with SGID', () => {
       event: 'sgid-login-failed-user-info',
     })
     expect(mocks.getOrCreateUser).not.toBeCalled()
+    expect(mocks.updateLastLogin).not.toBeCalled()
     expect(mocks.setAuthCookie).not.toBeCalled()
   })
 

diff --git a/packages/backend/src/graphql/mutations/login-with-selected-sgid.ts b/packages/backend/src/graphql/mutations/login-with-selected-sgid.ts
@@ -2,7 +2,7 @@ import { type Request } from 'express'
 import { verify as verifyJwt } from 'jsonwebtoken'
 
 import appConfig from '@/config/app'
-import { getOrCreateUser, setAuthCookie } from '@/helpers/auth'
+import { getOrCreateUser, setAuthCookie, updateLastLogin } from '@/helpers/auth'
 import logger from '@/helpers/logger'
 import {
   type PublicOfficerEmployment,
@@ -44,6 +44,7 @@ const loginWithSelectedSgid: MutationResolvers['loginWithSelectedSgid'] =
     }
 
     const user = await getOrCreateUser(workEmail)
+    await updateLastLogin(user.id)
     setAuthCookie(context.res, { userId: user.id })
 
     return {

diff --git a/packages/backend/src/graphql/mutations/login-with-sgid.ts b/packages/backend/src/graphql/mutations/login-with-sgid.ts
@@ -3,7 +3,7 @@ import { type Response } from 'express'
 import { sign as signJwt } from 'jsonwebtoken'
 
 import appConfig from '@/config/app'
-import { getOrCreateUser, setAuthCookie } from '@/helpers/auth'
+import { getOrCreateUser, setAuthCookie, updateLastLogin } from '@/helpers/auth'
 import { validateAndParseEmail } from '@/helpers/email-validator'
 import logger from '@/helpers/logger'
 import {
@@ -129,6 +129,7 @@ const loginWithSgid: MutationResolvers['loginWithSgid'] = async (
   // Log user in directly if there is only 1 employment.
   if (publicOfficerEmployments.length === 1) {
     const user = await getOrCreateUser(publicOfficerEmployments[0].workEmail)
+    await updateLastLogin(user.id)
     setAuthCookie(context.res, { userId: user.id })
 
     return {

diff --git a/packages/backend/src/graphql/mutations/verify-otp.ts b/packages/backend/src/graphql/mutations/verify-otp.ts
@@ -56,6 +56,7 @@ const verifyOtp: MutationResolvers['verifyOtp'] = async (
     otpHash: null,
     otpAttempts: 0,
     otpSentAt: null,
+    lastLoginAt: new Date(),
   })
 
   // set auth jwt as cookie

diff --git a/packages/backend/src/helpers/__tests__/auth.test.ts b/packages/backend/src/helpers/__tests__/auth.test.ts
@@ -3,20 +3,35 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
 import appConfig from '@/config/app'
 
-import { getAdminTokenUser, parseAdminToken } from '../auth'
+import {
+  getAdminTokenUser,
+  getOrCreateUser,
+  parseAdminToken,
+  updateLastLogin,
+} from '../auth'
+
+const mockPatchWhere = vi.fn()
 
 const mocks = vi.hoisted(() => ({
   whereUser: vi.fn(() => ({
     first: vi.fn(() => ({
       throwIfNotFound: vi.fn(() => ({ id: 'test-user-id' })),
     })),
   })),
+  findOne: vi.fn(),
+  insertAndFetch: vi.fn(),
+  patch: vi.fn(() => ({
+    where: mockPatchWhere,
+  })),
 }))
 
 vi.mock('@/models/user', () => ({
   default: {
     query: vi.fn(() => ({
       where: mocks.whereUser,
+      findOne: mocks.findOne,
+      insertAndFetch: mocks.insertAndFetch,
+      patch: mocks.patch,
     })),
   },
 }))
@@ -69,4 +84,122 @@ describe('Auth helpers', () => {
       expect(result.id).toEqual('test-user-id')
     })
   })
+
+  describe('getOrCreateUser', () => {
+    afterEach(() => {
+      vi.clearAllMocks() // Clear mocks after each test
+    })
+
+    it('should return an existing user if found', async () => {
+      const email = '[email protected]'
+      const existingUser = { id: 'test-user-id', email }
+
+      mocks.findOne.mockResolvedValueOnce(existingUser)
+
+      const result = await getOrCreateUser(email)
+
+      expect(mocks.findOne).toHaveBeenCalledOnce()
+      expect(mocks.findOne).toHaveBeenCalledWith({ email: email.toLowerCase() })
+
+      expect(mocks.insertAndFetch).not.toHaveBeenCalled() // Ensure no new user was created
+
+      expect(result).toEqual(existingUser)
+    })
+
+    it('should create a new user if none exists', async () => {
+      const email = '[email protected]'
+      const newUser = { id: 'new-user-id', email }
+
+      mocks.findOne.mockResolvedValueOnce(null) // Simulate no user found
+      mocks.insertAndFetch.mockResolvedValueOnce(newUser) // Simulate new user creation
+
+      const user = await getOrCreateUser(email)
+
+      expect(mocks.findOne).toHaveBeenCalledOnce()
+      expect(mocks.findOne).toHaveBeenCalledWith({ email: email.toLowerCase() })
+
+      expect(mocks.insertAndFetch).toHaveBeenCalledOnce()
+      expect(mocks.insertAndFetch).toHaveBeenCalledWith({
+        email: email.toLowerCase(),
+      })
+
+      expect(user).toEqual(newUser)
+    })
+
+    it('should trim and lowercase the email before querying', async () => {
+      const email = '   [email protected]   '
+      const formattedEmail = '[email protected]'
+      const user = { id: 'test-user-id', email: formattedEmail }
+
+      mocks.findOne.mockResolvedValueOnce(user)
+
+      const result = await getOrCreateUser(email)
+
+      expect(mocks.findOne).toHaveBeenCalledOnce()
+      expect(mocks.findOne).toHaveBeenCalledWith({ email: formattedEmail })
+
+      expect(result).toEqual(user)
+    })
+
+    it('should handle errors from User.query().findOne', async () => {
+      const email = '[email protected]'
+
+      mocks.findOne.mockRejectedValueOnce(new Error('Database error'))
+
+      await expect(getOrCreateUser(email)).rejects.toThrowError(
+        'Database error',
+      )
+
+      expect(mocks.findOne).toHaveBeenCalledOnce()
+      expect(mocks.findOne).toHaveBeenCalledWith({ email: email.toLowerCase() })
+
+      expect(mocks.insertAndFetch).not.toHaveBeenCalled() // Ensure no insert attempt was made
+    })
+
+    it('should handle errors from User.query().insertAndFetch', async () => {
+      const email = '[email protected]'
+
+      mocks.findOne.mockResolvedValueOnce(null) // Simulate no user found
+      mocks.insertAndFetch.mockRejectedValueOnce(new Error('Insert error'))
+
+      await expect(getOrCreateUser(email)).rejects.toThrowError('Insert error')
+
+      expect(mocks.findOne).toHaveBeenCalledOnce()
+      expect(mocks.findOne).toHaveBeenCalledWith({ email: email.toLowerCase() })
+
+      expect(mocks.insertAndFetch).toHaveBeenCalledOnce()
+      expect(mocks.insertAndFetch).toHaveBeenCalledWith({
+        email: email.toLowerCase(),
+      })
+    })
+  })
+
+  describe('updateLastLogin', () => {
+    afterEach(() => {
+      vi.clearAllMocks() // Clear mocks after each test
+    })
+
+    it('patch with correct id and date', async () => {
+      const userId = 'test-user-id'
+      mocks.patch().where.mockResolvedValueOnce(1)
+
+      await updateLastLogin(userId)
+
+      expect(mocks.patch).toHaveBeenCalledWith({
+        lastLoginAt: expect.any(Date),
+      })
+      expect(mocks.patch().where).toHaveBeenCalledWith({ id: userId })
+    })
+
+    it('throws error with no user id', async () => {
+      await expect(updateLastLogin('')).rejects.toThrowError('User id required')
+    })
+
+    it('throws error with non-existent user id', async () => {
+      mocks.patch().where.mockReturnValueOnce(Promise.resolve(0))
+      await expect(updateLastLogin('non-existent-id')).rejects.toThrowError(
+        'No user found',
+      )
+    })
+  })
 })
diff --git a/packages/backend/src/helpers/auth.ts b/packages/backend/src/helpers/auth.ts
@@ -61,6 +61,22 @@ export async function getOrCreateUser(email: string): Promise<User> {
   return user
 }
 
+export async function updateLastLogin(id: string) {
+  if (!id) {
+    throw new Error('User id required!')
+  }
+
+  const updatedRows = await User.query()
+    .patch({
+      lastLoginAt: new Date(),
+    })
+    .where({ id })
+
+  if (!updatedRows) {
+    throw new Error('No user found')
+  }
+}
+
 // Admin tokens are more sensitive so we set a low max age of 5 min
 const ADMIN_TOKEN_MAX_AGE_SEC = 5 * 60
 

diff --git a/packages/backend/src/models/user.ts b/packages/backend/src/models/user.ts
@@ -25,6 +25,7 @@ class User extends Base {
   tables?: TableMetadata[]
   sentFlowTransfers?: FlowTransfer[]
   receivedFlowTransfers?: FlowTransfer[]
+  lastLoginAt?: Date
 
   // for typescript support when creating TableCollaborator row in insertGraph
   role?: ITableCollabRole