From b0419a86f2574c9aa3d28446dd9c24c47141e594 Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Thu, 14 Nov 2024 15:55:40 +0100 Subject: [PATCH] [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 * Fix unit test * Use `promiseMapWithLimit` * Add `{ script: true }` to AuditLog. Also use `undefined` instead of `null` for consistency --------- Co-authored-by: Jakob Ackermann GitOrigin-RevId: 522026c82196d263c196503d899b8c57b05b31dd --- .../PasswordReset/PasswordResetController.mjs | 6 +- .../PasswordReset/PasswordResetHandler.mjs | 8 +- .../src/Features/User/UserAuditLogHandler.js | 3 + .../web/app/src/Features/User/UserUpdater.js | 13 +- .../clear_sessions_set_must_reconfirm.mjs | 67 ++++---- .../ClearSessionsSetMustReconfirmTests.mjs | 155 ++++++++++++++++++ .../acceptance/src/PasswordResetTests.mjs | 49 ++++++ .../PasswordResetControllerTests.mjs | 11 +- 8 files changed, 275 insertions(+), 37 deletions(-) create mode 100644 services/web/test/acceptance/src/ClearSessionsSetMustReconfirmTests.mjs diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.mjs b/services/web/app/src/Features/PasswordReset/PasswordResetController.mjs index 92860d05ab..aa4ac057dc 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.mjs +++ b/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) } diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.mjs b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.mjs index b6d837ca23..094f18b95f 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.mjs +++ b/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 = { diff --git a/services/web/app/src/Features/User/UserAuditLogHandler.js b/services/web/app/src/Features/User/UserAuditLogHandler.js index 6f418f0e84..23276c29d9 100644 --- a/services/web/app/src/Features/User/UserAuditLogHandler.js +++ b/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 } /** diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 7f38154edd..abae551f03 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/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} + */ +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 } }) } diff --git a/services/web/scripts/clear_sessions_set_must_reconfirm.mjs b/services/web/scripts/clear_sessions_set_must_reconfirm.mjs index a5f21a2d89..e459a58225 100644 --- a/services/web/scripts/clear_sessions_set_must_reconfirm.mjs +++ b/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] diff --git a/services/web/test/acceptance/src/ClearSessionsSetMustReconfirmTests.mjs b/services/web/test/acceptance/src/ClearSessionsSetMustReconfirmTests.mjs new file mode 100644 index 0000000000..379d231b7a --- /dev/null +++ b/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 + } + }) + }) +}) diff --git a/services/web/test/acceptance/src/PasswordResetTests.mjs b/services/web/test/acceptance/src/PasswordResetTests.mjs index aa99ef6f98..14b98a2d5d 100644 --- a/services/web/test/acceptance/src/PasswordResetTests.mjs +++ b/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 + }) + }) }) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.mjs b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.mjs index bbed3fc452..cd8f27e925 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.mjs +++ b/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()