From 9ca43ebc4eb2845157c3b2e04d90739987c03e5b Mon Sep 17 00:00:00 2001 From: Mathias Jakobsen Date: Mon, 20 Nov 2023 10:22:21 +0000 Subject: [PATCH] Merge pull request #15822 from overleaf/mj-audit-log-tokens [web] Add audit logs for token expiration operations GitOrigin-RevId: 220fe017cf508ead986a4cd2bd9009035418ce43 --- .../PasswordReset/PasswordResetHandler.js | 5 +++- .../User/UserEmailsConfirmationHandler.js | 7 ++++- .../src/Features/User/UserEmailsController.js | 24 +++++++++++++-- .../PasswordResetHandlerTests.js | 30 ++++++++++--------- .../src/User/UserEmailsControllerTests.js | 21 ++++++++++++- 5 files changed, 68 insertions(+), 19 deletions(-) diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index 9c703ea6a8..dee316b0a6 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -6,6 +6,8 @@ const EmailHandler = require('../Email/EmailHandler') const AuthenticationManager = require('../Authentication/AuthenticationManager') const { callbackify, promisify } = require('util') +const AUDIT_LOG_TOKEN_PREFIX_LENGTH = 10 + function generateAndEmailResetToken(email, callback) { UserGetter.getUserByAnyEmail(email, (err, user) => { if (err || !user) { @@ -103,7 +105,8 @@ async function setNewUserPassword(token, password, auditLog) { user._id, 'reset-password', auditLog.initiatorId, - auditLog.ip + auditLog.ip, + { token: token.substring(0, AUDIT_LOG_TOKEN_PREFIX_LENGTH) } ) const reset = await AuthenticationManager.promises.setUserPassword( diff --git a/services/web/app/src/Features/User/UserEmailsConfirmationHandler.js b/services/web/app/src/Features/User/UserEmailsConfirmationHandler.js index e541042b99..48fcbafc9a 100644 --- a/services/web/app/src/Features/User/UserEmailsConfirmationHandler.js +++ b/services/web/app/src/Features/User/UserEmailsConfirmationHandler.js @@ -125,7 +125,12 @@ const UserEmailsConfirmationHandler = { if (!emailExists) { return callback(new Errors.NotFoundError('email missing for user')) } - UserUpdater.confirmEmail(userId, email, callback) + UserUpdater.confirmEmail(userId, email, function (error) { + if (error) { + return callback(error) + } + callback(null, { userId, email }) + }) }) } ) diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index 64a4a4a869..a53e5c07cd 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/services/web/app/src/Features/User/UserEmailsController.js @@ -14,6 +14,9 @@ const { expressify } = require('@overleaf/promise-utils') const AsyncFormHelper = require('../Helpers/AsyncFormHelper') const AnalyticsManager = require('../Analytics/AnalyticsManager') const UserPrimaryEmailCheckHandler = require('../User/UserPrimaryEmailCheckHandler') +const UserAuditLogHandler = require('./UserAuditLogHandler') + +const AUDIT_LOG_TOKEN_PREFIX_LENGTH = 10 async function _sendSecurityAlertEmail(user, email) { const emailOptions = { @@ -267,7 +270,7 @@ const UserEmailsController = { } UserEmailsConfirmationHandler.confirmEmailFromToken( token, - function (error) { + function (error, userData) { if (error) { if (error instanceof Errors.NotFoundError) { res.status(404).json({ @@ -277,7 +280,24 @@ const UserEmailsController = { next(error) } } else { - res.sendStatus(200) + const { userId, email } = userData + const tokenPrefix = token.substring(0, AUDIT_LOG_TOKEN_PREFIX_LENGTH) + UserAuditLogHandler.addEntry( + userId, + 'confirm-email', + userId, + req.ip, + { token: tokenPrefix, email }, + auditLogError => { + if (auditLogError) { + logger.error( + { error: auditLogError, userId, token: tokenPrefix }, + 'failed to add audit log entry' + ) + } + res.sendStatus(200) + } + ) } } ) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index 9f9a941d1a..01194bfbf5 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -290,14 +290,15 @@ describe('PasswordResetHandler', function () { this.auditLog, (error, result) => { const { reset, userId } = result + sinon.assert.calledWith( + this.UserAuditLogHandler.promises.addEntry, + this.user_id, + 'reset-password', + undefined, + this.auditLog.ip, + { token: this.token.substring(0, 10) } + ) expect(error).to.not.exist - const logCall = - this.UserAuditLogHandler.promises.addEntry.lastCall - expect(logCall.args[0]).to.equal(this.user_id) - expect(logCall.args[1]).to.equal('reset-password') - expect(logCall.args[2]).to.equal(undefined) - expect(logCall.args[3]).to.equal(this.auditLog.ip) - expect(logCall.args[4]).to.equal(undefined) done() } ) @@ -344,13 +345,14 @@ describe('PasswordResetHandler', function () { (error, result) => { const { reset, userId } = result expect(error).to.not.exist - const logCall = - this.UserAuditLogHandler.promises.addEntry.lastCall - expect(logCall.args[0]).to.equal(this.user_id) - expect(logCall.args[1]).to.equal('reset-password') - expect(logCall.args[2]).to.equal(this.user_id) - expect(logCall.args[3]).to.equal(this.auditLog.ip) - expect(logCall.args[4]).to.equal(undefined) + sinon.assert.calledWith( + this.UserAuditLogHandler.promises.addEntry, + this.user_id, + 'reset-password', + this.user_id, + this.auditLog.ip, + { token: this.token.substring(0, 10) } + ) done() } ) diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index fad68cd277..bce081cd85 100644 --- a/services/web/test/unit/src/User/UserEmailsControllerTests.js +++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js @@ -56,6 +56,9 @@ describe('UserEmailsController', function () { this.AnalyticsManager = { recordEventForUser: sinon.stub(), } + this.UserAuditLogHandler = { + addEntry: sinon.stub().yields(), + } this.UserEmailsController = SandboxedModule.require(modulePath, { requires: { '../Authentication/SessionManager': this.SessionManager, @@ -79,6 +82,7 @@ describe('UserEmailsController', function () { '../Institutions/InstitutionsAPI': this.InstitutionsAPI, '../Errors/HttpErrorHandler': this.HttpErrorHandler, '../Analytics/AnalyticsManager': this.AnalyticsManager, + './UserAuditLogHandler': this.UserAuditLogHandler, }, }) }) @@ -416,7 +420,7 @@ describe('UserEmailsController', function () { beforeEach(function () { this.UserEmailsConfirmationHandler.confirmEmailFromToken = sinon .stub() - .yields() + .yields(null, { userId: this.user._id, email: this.user.email }) this.res = { sendStatus: sinon.stub(), json: sinon.stub(), @@ -425,6 +429,7 @@ describe('UserEmailsController', function () { this.next = sinon.stub() this.token = 'mock-token' this.req.body = { token: this.token } + this.req.ip = '0.0.0.0' }) describe('successfully', function () { @@ -441,6 +446,20 @@ describe('UserEmailsController', function () { it('should return a 200 status', function () { this.res.sendStatus.calledWith(200).should.equal(true) }) + + it('should log the confirmation to the audit log', function () { + sinon.assert.calledWith( + this.UserAuditLogHandler.addEntry, + this.user._id, + 'confirm-email', + this.user._id, + this.req.ip, + { + token: this.token.substring(0, 10), + email: this.user.email, + } + ) + }) }) describe('without a token', function () {