From 5f8db6ee23a69fffbf551654809fc0cb5c9b887a Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Fri, 12 Apr 2024 12:04:00 +0200 Subject: [PATCH] Merge pull request #17799 from overleaf/ab-account-deletion-audit-log [web] Add audit log when user account is deleted or recovered GitOrigin-RevId: 3d5f99705fbd6192ccae430e040be4b7fcb3f740 --- .../web/app/src/Features/User/UserDeleter.js | 7 ++ .../test/unit/src/User/UserDeleterTests.js | 108 ++++++++++++++---- 2 files changed, 94 insertions(+), 21 deletions(-) diff --git a/services/web/app/src/Features/User/UserDeleter.js b/services/web/app/src/Features/User/UserDeleter.js index f88e05d341..6f739bfba0 100644 --- a/services/web/app/src/Features/User/UserDeleter.js +++ b/services/web/app/src/Features/User/UserDeleter.js @@ -12,6 +12,7 @@ const SubscriptionUpdater = require('../Subscription/SubscriptionUpdater') const SubscriptionLocator = require('../Subscription/SubscriptionLocator') const UserMembershipsHandler = require('../UserMembership/UserMembershipsHandler') const UserSessionsManager = require('./UserSessionsManager') +const UserAuditLogHandler = require('./UserAuditLogHandler') const InstitutionsAPI = require('../Institutions/InstitutionsAPI') const Modules = require('../../infrastructure/Modules') const Errors = require('../Errors/Errors') @@ -47,6 +48,12 @@ async function deleteUser(userId, options) { await ensureCanDeleteUser(user) await _cleanupUser(user) await Modules.promises.hooks.fire('deleteUser', userId) + await UserAuditLogHandler.promises.addEntry( + userId, + 'delete-account', + options.deleterUser ? options.deleterUser._id : userId, + options.ipAddress + ) await _createDeletedUser(user, options) await ProjectDeleter.promises.deleteUsersProjects(user._id) await _sendDeleteEmail(user) diff --git a/services/web/test/unit/src/User/UserDeleterTests.js b/services/web/test/unit/src/User/UserDeleterTests.js index 093a36cfcc..dba3b71317 100644 --- a/services/web/test/unit/src/User/UserDeleterTests.js +++ b/services/web/test/unit/src/User/UserDeleterTests.js @@ -15,6 +15,7 @@ describe('UserDeleter', function () { tk.freeze(Date.now()) this.userId = new ObjectId() + this.ipAddress = '1.2.3.4' this.UserMock = sinon.mock(User) this.DeletedUserMock = sinon.mock(DeletedUser) @@ -106,6 +107,12 @@ describe('UserDeleter', function () { }, } + this.UserAuditLogHandler = { + promises: { + addEntry: sinon.stub().resolves(), + }, + } + this.UserDeleter = SandboxedModule.require(modulePath, { requires: { '../../models/User': { User }, @@ -122,6 +129,7 @@ describe('UserDeleter', function () { '../../models/UserAuditLogEntry': { UserAuditLogEntry: this.UserAuditLogEntry, }, + './UserAuditLogHandler': this.UserAuditLogHandler, '../../infrastructure/Modules': this.Modules, '../OnboardingDataCollection/OnboardingDataCollectionManager': this.OnboardingDataCollectionManager, @@ -151,7 +159,7 @@ describe('UserDeleter', function () { deleterData: { deletedAt: new Date(), deletedUserId: this.userId, - deleterIpAddress: undefined, + deleterIpAddress: this.ipAddress, deleterId: undefined, deletedUserLastLoggedIn: this.user.lastLoggedIn, deletedUserSignUpDate: this.user.signUpDate, @@ -164,7 +172,7 @@ describe('UserDeleter', function () { } }) - describe('when no options are passed', function () { + describe('when only the ip address is passed', function () { beforeEach(function () { this.DeletedUserMock.expects('updateOne') .withArgs( @@ -185,40 +193,52 @@ describe('UserDeleter', function () { }) it('should find and the user in mongo by its id', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) this.UserMock.verify() }) it('should delete the user from mailchimp', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect( this.NewsletterManager.promises.unsubscribe ).to.have.been.calledWith(this.user, { delete: true }) }) it('should delete all the projects of a user', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect( this.ProjectDeleter.promises.deleteUsersProjects ).to.have.been.calledWith(this.userId) }) it("should cancel the user's subscription", async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect( this.SubscriptionHandler.promises.cancelSubscription ).to.have.been.calledWith(this.user) }) it('should delete user affiliations', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect( this.InstitutionsApi.promises.deleteAffiliations ).to.have.been.calledWith(this.userId) }) it('should fire the deleteUser hook for modules', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect(this.Modules.promises.hooks.fire).to.have.been.calledWith( 'deleteUser', this.userId @@ -226,21 +246,27 @@ describe('UserDeleter', function () { }) it('should stop the user sessions', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect( this.UserSessionsManager.promises.revokeAllUserSessions ).to.have.been.calledWith(this.userId, []) }) it('should remove user from group subscriptions', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect( this.SubscriptionUpdater.promises.removeUserFromAllGroups ).to.have.been.calledWith(this.userId) }) it('should remove user memberships', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) expect( this.UserMembershipsHandler.promises.removeUserFromAllEntities ).to.have.been.calledWith(this.userId) @@ -255,12 +281,16 @@ describe('UserDeleter', function () { }) it('should create a deletedUser', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) this.DeletedUserMock.verify() }) it('should email the user', async function () { - await this.UserDeleter.promises.deleteUser(this.userId, {}) + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) const emailOptions = { to: 'bob@bob.com', action: 'account deleted', @@ -270,6 +300,20 @@ describe('UserDeleter', function () { this.EmailHandler.promises.sendEmail ).to.have.been.calledWith('securityAlert', emailOptions) }) + + it('should add an audit log entry', async function () { + await this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.userId, + 'delete-account', + this.userId, + this.ipAddress + ) + }) }) describe('when unsubscribing from mailchimp fails', function () { @@ -280,8 +324,11 @@ describe('UserDeleter', function () { }) it('should return an error and not delete the user', async function () { - await expect(this.UserDeleter.promises.deleteUser(this.userId, {})) - .to.be.rejected + await expect( + this.UserDeleter.promises.deleteUser(this.userId, { + ipAddress: this.ipAddress, + }) + ).to.be.rejected this.UserMock.verify() }) }) @@ -295,12 +342,16 @@ describe('UserDeleter', function () { }) it('should delete the user', function (done) { - this.UserDeleter.deleteUser(this.userId, {}, err => { - expect(err).not.to.exist - this.UserMock.verify() - this.DeletedUserMock.verify() - done() - }) + this.UserDeleter.deleteUser( + this.userId, + { ipAddress: this.ipAddress }, + err => { + expect(err).not.to.exist + this.UserMock.verify() + this.DeletedUserMock.verify() + done() + } + ) }) }) }) @@ -335,6 +386,21 @@ describe('UserDeleter', function () { this.DeletedUserMock.verify() }) + it('should add an audit log entry', async function () { + await this.UserDeleter.promises.deleteUser(this.userId, { + deleterUser: { _id: this.deleterId }, + ipAddress: this.ipAddress, + }) + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.userId, + 'delete-account', + this.deleterId, + this.ipAddress + ) + }) + describe('when called as a callback', function () { it('should delete the user', function (done) { this.UserDeleter.deleteUser(