[web] Add audit logs for `clear_sessions_set_must_reconfirm` script, "must-reset-password-set" and "must-reset-password-unset" (#21776)

* Promisify clear_sessions_set_must_reconfirm.mjs

* Add test on PasswordResetTests.mjs

* Add `must-reset-password-unset` audit log

* Add `must-reset-password-set` audit log

* Add test ClearSessionsSetMustReconfirmTests.mjs

* Fixup bad copy-paste in test: `must-reset-password-set` -> `must-reset-password-unset`

* Check `must_reconfirm` before calling `removeReconfirmFlag`

Co-authored-by: Jakob Ackermann <jakob.ackermann@overleaf.com>

* Fix unit test

* Use `promiseMapWithLimit`

* Add `{ script: true }` to AuditLog. Also use `undefined` instead of `null` for consistency

---------

Co-authored-by: Jakob Ackermann <jakob.ackermann@overleaf.com>
GitOrigin-RevId: 522026c82196d263c196503d899b8c57b05b31dd

services/web/app/src/Features/PasswordReset/PasswordResetController.mjs

@@ -44,7 +44,7 @@ async function setNewUserPassword(req, res, next) {
       password,
       auditLog
     )
-    const { found, reset, userId } = result
+    const { found, reset, userId, mustReconfirm } = result
     if (!found) {
       return res.status(404).json({
         message: {
@@ -58,7 +58,9 @@ async function setNewUserPassword(req, res, next) {
       })
     }
     await UserSessionsManager.promises.removeSessionsFromRedis({ _id: userId })
-    await UserUpdater.promises.removeReconfirmFlag(userId)
+    if (mustReconfirm) {
+      await UserUpdater.promises.removeReconfirmFlag(userId, auditLog)
+    }
     if (!req.session.doLoginAfterPasswordReset) {
       return res.sendStatus(200)
     }

services/web/app/src/Features/PasswordReset/PasswordResetHandler.mjs

@@ -71,6 +71,7 @@ async function getUserForPasswordResetToken(token) {
     _id: 1,
     'overleaf.id': 1,
     email: 1,
+    must_reconfirm: 1,
   })
 
   await assertUserPermissions(user, ['change-password'])
@@ -117,7 +118,12 @@ async function setNewUserPassword(token, password, auditLog) {
 
   await PasswordResetHandler.promises.expirePasswordResetToken(token)
 
-  return { found: true, reset, userId: user._id }
+  return {
+    found: true,
+    reset,
+    userId: user._id,
+    mustReconfirm: user.must_reconfirm,
+  }
 }
 
 const PasswordResetHandler = {

services/web/app/src/Features/User/UserAuditLogHandler.js

@@ -5,6 +5,7 @@ const { callbackify } = require('util')
 function _canHaveNoIpAddressId(operation) {
   if (operation === 'join-group-subscription') return true
   if (operation === 'leave-group-subscription') return true
+  if (operation === 'must-reset-password-set') return true
   return false
 }
 
@@ -17,6 +18,8 @@ function _canHaveNoInitiatorId(operation, info) {
   if (operation === 'remove-email' && info.script) return true
   if (operation === 'join-group-subscription') return true
   if (operation === 'leave-group-subscription') return true
+  if (operation === 'must-reset-password-set') return true
+  if (operation === 'must-reset-password-unset') return true
 }
 
 /**

services/web/app/src/Features/User/UserUpdater.js

@@ -494,7 +494,18 @@ async function changeEmailAddress(userId, newEmail, auditLog) {
   await removeEmailAddress(userId, oldEmail, auditLog)
 }
 
-async function removeReconfirmFlag(userId) {
+/**
+ * @param {string} userId
+ * @param {{initiatorId: string, ip: string}} auditLog
+ * @returns {Promise<void>}
+ */
+async function removeReconfirmFlag(userId, auditLog) {
+  await UserAuditLogHandler.promises.addEntry(
+    userId.toString(),
+    'must-reset-password-unset',
+    auditLog.initiatorId,
+    auditLog.ip
+  )
   await updateUser(userId.toString(), { $set: { must_reconfirm: false } })
 }
 

services/web/scripts/clear_sessions_set_must_reconfirm.mjs

@@ -1,8 +1,9 @@
 import fs from 'node:fs'
 import { ObjectId } from '../app/src/infrastructure/mongodb.js'
-import async from 'async'
 import UserUpdater from '../app/src/Features/User/UserUpdater.js'
 import UserSessionsManager from '../app/src/Features/User/UserSessionsManager.js'
+import UserAuditLogHandler from '../app/src/Features/User/UserAuditLogHandler.js'
+import { promiseMapWithLimit } from '@overleaf/promise-utils'
 
 const ASYNC_LIMIT = 10
 
@@ -30,37 +31,45 @@ function _validateUserIdList(userIds) {
   })
 }
 
-function _handleUser(userId, callback) {
-  UserUpdater.updateUser(userId, { $set: { must_reconfirm: true } }, error => {
-    if (error) {
-      console.log(`Failed to set must_reconfirm ${userId}`, error)
-      processLogger.failedSet.push(userId)
-      return callback()
-    } else {
-      UserSessionsManager.removeSessionsFromRedis(
-        { _id: userId },
-        null,
-        error => {
-          if (error) {
-            console.log(`Failed to clear sessions for ${userId}`, error)
-            processLogger.failedClear.push(userId)
-          } else {
-            processLogger.success.push(userId)
-          }
-          return callback()
-        }
-      )
-    }
-  })
+async function _handleUser(userId) {
+  try {
+    await UserUpdater.promises.updateUser(userId, {
+      $set: { must_reconfirm: true },
+    })
+  } catch (error) {
+    console.log(`Failed to set must_reconfirm ${userId}`, error)
+    processLogger.failedSet.push(userId)
+    return
+  }
+
+  try {
+    await UserAuditLogHandler.promises.addEntry(
+      userId,
+      'must-reset-password-set',
+      undefined,
+      undefined,
+      { script: true }
+    )
+  } catch (error) {
+    console.log(`Failed to create audit log for ${userId}`, error)
+    // don't block the process if audit log fails
+  }
+
+  try {
+    await UserSessionsManager.promises.removeSessionsFromRedis(
+      { _id: userId },
+      null
+    )
+  } catch (error) {
+    console.log(`Failed to clear sessions for ${userId}`, error)
+    processLogger.failedClear.push(userId)
+    return
+  }
+  processLogger.success.push(userId)
 }
 
 async function _loopUsers(userIds) {
-  await new Promise((resolve, reject) => {
-    async.eachLimit(userIds, ASYNC_LIMIT, _handleUser, error => {
-      if (error) return reject(error)
-      resolve()
-    })
-  })
+  return promiseMapWithLimit(ASYNC_LIMIT, userIds, _handleUser)
 }
 
 const fileName = process.argv[2]

services/web/test/acceptance/src/ClearSessionsSetMustReconfirmTests.mjs

@@ -0,0 +1,155 @@
+import { exec } from 'node:child_process'
+import { promisify } from 'util'
+import { expect } from 'chai'
+import logger from '@overleaf/logger'
+import { ObjectId, db } from '../../../app/src/infrastructure/mongodb.js'
+import fs from 'fs/promises'
+import UserHelper from './helpers/User.js'
+import UserGetter from '../../../app/src/Features/User/UserGetter.js'
+
+const User = UserHelper.promises
+const TEST_FILE_PATH = '/tmp/test-users.txt'
+
+describe('ClearSessionsSetMustReconfirm', function () {
+  let user1, user2, user3, user4, usersMustReconfirm, usersMustNotReconfirm
+
+  beforeEach('create test users', async function () {
+    user1 = new User()
+    user2 = new User() // not in the file
+    user3 = new User() // not in the file
+    user4 = new User()
+    await user1.login()
+    await user2.login()
+    await user3.login()
+    await user4.login()
+    usersMustReconfirm = [user1, user4]
+    usersMustNotReconfirm = [user2, user3]
+  })
+
+  beforeEach('create test file', async function () {
+    await fs.writeFile(
+      TEST_FILE_PATH,
+      usersMustReconfirm.map(user => user._id.toString()).join('\n')
+    )
+  })
+
+  afterEach('cleanup test file', async function () {
+    try {
+      await fs.unlink(TEST_FILE_PATH)
+    } catch (err) {
+      // Ignore error if file doesn't exist
+    }
+  })
+
+  async function runScript(filePath = TEST_FILE_PATH) {
+    let result
+    try {
+      result = await promisify(exec)(
+        ['VERBOSE_LOGGING=true']
+          .concat(['node', 'scripts/clear_sessions_set_must_reconfirm.mjs'])
+          .concat([filePath])
+          .join(' ')
+      )
+    } catch (error) {
+      logger.error({ error }, 'script failed')
+      throw error
+    }
+    const { stdout: stdOut } = result
+    expect(stdOut).to.include('DONE.')
+    return result
+  }
+
+  describe('processing users', function () {
+    it('should process all users successfully', async function () {
+      const { stdout } = await runScript()
+      expect(stdout).to.include(`${usersMustReconfirm.length} successful`)
+      expect(stdout).to.include('0 failed to clear sessions')
+      expect(stdout).to.include('0 failed to set must_reconfirm')
+      for (const user of usersMustReconfirm) {
+        const updatedUser = await UserGetter.promises.getUser({
+          _id: user._id,
+        })
+        expect(updatedUser.must_reconfirm).to.be.true
+      }
+      for (const user of usersMustNotReconfirm) {
+        const updatedUser = await UserGetter.promises.getUser({
+          _id: user._id,
+        })
+        expect(updatedUser.must_reconfirm).to.be.false
+      }
+    })
+
+    it('should handle invalid user IDs in file', async function () {
+      await fs.writeFile(
+        TEST_FILE_PATH,
+        [
+          'invalid-id',
+          ...usersMustReconfirm.map(user => user._id.toString()).join('\n'),
+        ].join('\n')
+      )
+      try {
+        await runScript()
+        expect.fail('Should have thrown error')
+      } catch (error) {
+        expect(error.message).to.include('user ID not valid')
+      }
+    })
+
+    it('should process large number of users with concurrency limit', async function () {
+      const manyUserIds = Array.from({ length: 15 }, () =>
+        new ObjectId().toString()
+      )
+      await fs.writeFile(TEST_FILE_PATH, manyUserIds.join('\n'))
+      const { stdout } = await runScript()
+      expect(stdout).to.include('15 successful')
+    })
+  })
+
+  describe('error handling', function () {
+    beforeEach('ensure test file exists', async function () {
+      await fs.writeFile(
+        TEST_FILE_PATH,
+        usersMustReconfirm.map(user => user._id.toString()).join('\n')
+      )
+    })
+
+    it('should report failed user updates', async function () {
+      await db.users.updateOne(
+        { _id: user1._id },
+        { $set: { must_reconfirm: null } }
+      )
+      const { stdout } = await runScript()
+      expect(stdout).to.include('failed to set must_reconfirm')
+    })
+
+    it('should handle missing input file', async function () {
+      try {
+        await runScript(['/non/existent/file'])
+        expect.fail('Should have thrown error')
+      } catch (error) {
+        expect(error.message).to.include('ENOENT')
+      }
+    })
+  })
+
+  describe('audit logging', function () {
+    it('should create audit log entries for processed users', async function () {
+      await runScript()
+      for (const user of usersMustReconfirm) {
+        const auditLogEntry = await user.getAuditLog()
+        expect(auditLogEntry).to.exist
+        expect(auditLogEntry[0].operation).to.equal('login')
+        expect(auditLogEntry[1].operation).to.equal('must-reset-password-set')
+        expect(auditLogEntry[1].initiatorId).to.be.undefined
+        expect(auditLogEntry[1].ipAddress).to.be.undefined
+        expect(auditLogEntry[1].info).to.deep.equal({ script: true })
+      }
+      for (const user of usersMustNotReconfirm) {
+        const auditLogEntry = await user.getAuditLog()
+        expect(auditLogEntry).to.exist
+        expect(auditLogEntry[0].operation).to.equal('login')
+        expect(auditLogEntry[1]).to.be.undefined
+      }
+    })
+  })
+})

services/web/test/acceptance/src/PasswordResetTests.mjs

@@ -450,4 +450,53 @@ describe('PasswordReset', function () {
       expect(response.status).to.equal(400)
     })
   })
+
+  describe('reconfirm flag', function () {
+    const getReconfirmAuditLogEntry = async function (email) {
+      const userHelper = await UserHelper.getUser({ email })
+      const auditLog = userHelper.getAuditLogWithoutNoise()
+      return auditLog.find(
+        entry => entry.operation === 'must-reset-password-unset'
+      )
+    }
+    it('should add audit log entry when flag changes from true to false', async function () {
+      // Set must_reconfirm to true
+      await db.users.updateOne(
+        { _id: user._id },
+        { $set: { must_reconfirm: true } }
+      )
+      response = await userHelper.fetch('/user/password/set', {
+        method: 'POST',
+        body: new URLSearchParams({
+          passwordResetToken: token,
+          password: 'a-password',
+        }),
+      })
+      expect(response.status).to.equal(200)
+
+      const reconfirmEntry = await getReconfirmAuditLogEntry(email)
+      expect(reconfirmEntry).to.exist
+      expect(reconfirmEntry.ipAddress).to.equal('127.0.0.1')
+      expect(reconfirmEntry.timestamp).to.exist
+    })
+
+    it('should not add audit log entry when flag was already false', async function () {
+      await db.users.updateOne(
+        { _id: user._id },
+        { $set: { must_reconfirm: false } }
+      )
+
+      response = await userHelper.fetch('/user/password/set', {
+        method: 'POST',
+        body: new URLSearchParams({
+          passwordResetToken: token,
+          password: 'a-password',
+        }),
+      })
+      expect(response.status).to.equal(200)
+
+      const reconfirmEntry = await getReconfirmAuditLogEntry(email)
+      expect(reconfirmEntry).to.not.exist
+    })
+  })
 })

services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.mjs

@@ -35,9 +35,12 @@ describe('PasswordResetController', function () {
       generateAndEmailResetToken: sinon.stub(),
       promises: {
         generateAndEmailResetToken: sinon.stub(),
-        setNewUserPassword: sinon
-          .stub()
-          .resolves({ found: true, reset: true, userID: this.user_id }),
+        setNewUserPassword: sinon.stub().resolves({
+          found: true,
+          reset: true,
+          userID: this.user_id,
+          mustReconfirm: true,
+        }),
         getUserForPasswordResetToken: sinon
           .stub()
           .withArgs(this.token)
@@ -271,7 +274,7 @@ describe('PasswordResetController', function () {
       this.PasswordResetController.setNewUserPassword(this.req, this.res)
     })
 
-    it('should call removeReconfirmFlag', function (done) {
+    it('should call removeReconfirmFlag if user.must_reconfirm', function (done) {
       this.res.sendStatus = code => {
         this.UserUpdater.promises.removeReconfirmFlag.callCount.should.equal(1)
         done()