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

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)

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(