Merge pull request #15672 from overleaf/mj-group-subscription-audit-revival

[web] Add audit logs when user joins or leaves group subscription

GitOrigin-RevId: d64425f5a2434c60c89c297c9a51acae3b96c31e

services/web/app/src/Features/Subscription/SubscriptionGroupController.js

@@ -3,14 +3,30 @@ const OError = require('@overleaf/o-error')
 const logger = require('@overleaf/logger')
 const SubscriptionLocator = require('./SubscriptionLocator')
 const SessionManager = require('../Authentication/SessionManager')
+const UserAuditLogHandler = require('../User/UserAuditLogHandler')
 
 function removeUserFromGroup(req, res, next) {
   const subscription = req.entity
   const userToRemoveId = req.params.user_id
+  const loggedInUserId = SessionManager.getLoggedInUserId(req.session)
   logger.debug(
     { subscriptionId: subscription._id, userToRemoveId },
     'removing user from group subscription'
   )
+  UserAuditLogHandler.addEntry(
+    userToRemoveId,
+    'remove-from-group-subscription',
+    loggedInUserId,
+    req.ip,
+    { subscriptionId: subscription._id },
+    function (auditLogError) {
+      if (auditLogError) {
+        OError.tag(auditLogError, 'error adding audit log entry', {
+          userToRemoveId,
+          subscriptionId: subscription._id,
+        })
+        return next(auditLogError)
+      }
       SubscriptionGroupHandler.removeUserFromGroup(
         subscription._id,
         userToRemoveId,
@@ -22,11 +38,12 @@ function removeUserFromGroup(req, res, next) {
             })
             return next(error)
           }
-
           res.sendStatus(200)
         }
       )
     }
+  )
+}
 
 function removeSelfFromGroup(req, res, next) {
   const subscriptionId = req.query.subscriptionId
@@ -38,6 +55,20 @@ function removeSelfFromGroup(req, res, next) {
         return next(error)
       }
 
+      UserAuditLogHandler.addEntry(
+        userToRemoveId,
+        'remove-from-group-subscription',
+        userToRemoveId,
+        req.ip,
+        { subscriptionId: subscription._id },
+        function (auditLogError) {
+          if (auditLogError) {
+            OError.tag(auditLogError, 'error adding audit log entry', {
+              userToRemoveId,
+              subscriptionId,
+            })
+            return next(auditLogError)
+          }
           SubscriptionGroupHandler.removeUserFromGroup(
             subscription._id,
             userToRemoveId,
@@ -55,6 +86,8 @@ function removeSelfFromGroup(req, res, next) {
         }
       )
     }
+  )
+}
 
 module.exports = {
   removeUserFromGroup,

services/web/app/src/Features/Subscription/SubscriptionUpdater.js

@@ -9,6 +9,7 @@ const AnalyticsManager = require('../Analytics/AnalyticsManager')
 const { DeletedSubscription } = require('../../models/DeletedSubscription')
 const logger = require('@overleaf/logger')
 const Features = require('../../infrastructure/Features')
+const UserAuditLogHandler = require('../User/UserAuditLogHandler')
 
 /**
  * Change the admin of the given subscription.
@@ -59,6 +60,13 @@ async function syncSubscription(
 }
 
 async function addUserToGroup(subscriptionId, userId) {
+  await UserAuditLogHandler.promises.addEntry(
+    userId,
+    'join-group-subscription',
+    undefined,
+    undefined,
+    { subscriptionId }
+  )
   await Subscription.updateOne(
     { _id: subscriptionId },
     { $addToSet: { member_ids: userId } }
@@ -73,6 +81,13 @@ async function addUserToGroup(subscriptionId, userId) {
 }
 
 async function removeUserFromGroup(subscriptionId, userId) {
+  await UserAuditLogHandler.promises.addEntry(
+    userId,
+    'leave-group-subscription',
+    undefined,
+    undefined,
+    { subscriptionId }
+  )
   await Subscription.updateOne(
     { _id: subscriptionId },
     { $pull: { member_ids: userId } }
@@ -97,6 +112,17 @@ async function removeUserFromAllGroups(userId) {
   }
   const subscriptionIds = subscriptions.map(sub => sub._id)
   const removeOperation = { $pull: { member_ids: userId } }
+
+  for (const subscriptionId of subscriptionIds) {
+    await UserAuditLogHandler.promises.addEntry(
+      userId,
+      'leave-group-subscription',
+      undefined,
+      undefined,
+      { subscriptionId }
+    )
+  }
+
   await Subscription.updateMany(
     { _id: subscriptionIds },
     removeOperation

services/web/app/src/Features/Subscription/TeamInvitesController.js

@@ -13,6 +13,7 @@ const PermissionsManager = require('../Authorization/PermissionsManager')
 const EmailHandler = require('../Email/EmailHandler')
 const { RateLimiter } = require('../../infrastructure/RateLimiter')
 const Modules = require('../../infrastructure/Modules')
+const UserAuditLogHandler = require('../User/UserAuditLogHandler')
 
 const rateLimiters = {
   resendGroupInvite: new RateLimiter('resend-group-invite', {
@@ -198,6 +199,21 @@ async function acceptInvite(req, res, next) {
     await Modules.promises.hooks.fire('hasGroupSSOEnabled', subscription)
   )?.[0]
 
+  try {
+    await UserAuditLogHandler.promises.addEntry(
+      userId,
+      'accept-group-invitation',
+      userId,
+      req.ip,
+      { subscriptionId: subscription._id }
+    )
+  } catch (e) {
+    logger.error(
+      { err: e, userId, subscriptionId: subscription._id },
+      'error adding audit log entry'
+    )
+  }
+
   res.json({ groupSSOActive })
 }
 

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

@@ -2,12 +2,20 @@ const OError = require('@overleaf/o-error')
 const { UserAuditLogEntry } = require('../../models/UserAuditLogEntry')
 const { callbackify } = require('util')
 
+function _canHaveNoIpAddressId(operation) {
+  if (operation === 'join-group-subscription') return true
+  if (operation === 'leave-group-subscription') return true
+  return false
+}
+
 function _canHaveNoInitiatorId(operation, info) {
   if (operation === 'reset-password') return true
   if (operation === 'unlink-sso' && info.providerId === 'collabratec')
     return true
   if (operation === 'unlink-institution-sso-not-migrated') return true
   if (operation === 'remove-email' && info.script) return true
+  if (operation === 'join-group-subscription') return true
+  if (operation === 'leave-group-subscription') return true
 }
 
 /**
@@ -22,12 +30,19 @@ function _canHaveNoInitiatorId(operation, info) {
  * - info: an object detailing what happened
  */
 async function addEntry(userId, operation, initiatorId, ipAddress, info = {}) {
-  if (!operation || !ipAddress)
-    throw new OError('missing required audit log data', {
-      operation,
+  if (!operation) {
+    throw new OError('missing operation for audit log', {
       initiatorId,
       ipAddress,
     })
+  }
+
+  if (!ipAddress && !_canHaveNoIpAddressId(operation, info)) {
+    throw new OError('missing ipAddress for audit log', {
+      operation,
+      initiatorId,
+    })
+  }
 
   if (!initiatorId && !_canHaveNoInitiatorId(operation, info)) {
     throw new OError('missing initiatorId for audit log', {

services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js

@@ -41,11 +41,16 @@ describe('SubscriptionGroupController', function () {
       },
     }
 
+    this.UserAuditLogHandler = {
+      addEntry: sinon.stub().callsArgWith(5),
+    }
+
     this.Controller = SandboxedModule.require(modulePath, {
       requires: {
         './SubscriptionGroupHandler': this.GroupHandler,
         './SubscriptionLocator': this.SubscriptionLocator,
         '../Authentication/SessionManager': this.SessionManager,
+        '../User/UserAuditLogHandler': this.UserAuditLogHandler,
       },
     })
   })
@@ -66,6 +71,27 @@ describe('SubscriptionGroupController', function () {
       }
       this.Controller.removeUserFromGroup(this.req, res)
     })
+
+    it('should log that the user has been removed', function (done) {
+      const userIdToRemove = '31231'
+      this.req.params = { user_id: userIdToRemove }
+      this.req.entity = this.subscription
+
+      const res = {
+        sendStatus: () => {
+          sinon.assert.calledWith(
+            this.UserAuditLogHandler.addEntry,
+            userIdToRemove,
+            'remove-from-group-subscription',
+            this.adminUserId,
+            this.req.ip,
+            { subscriptionId: this.subscriptionId }
+          )
+          done()
+        },
+      }
+      this.Controller.removeUserFromGroup(this.req, res)
+    })
   })
 
   describe('removeSelfFromGroup', function () {
@@ -90,5 +116,26 @@ describe('SubscriptionGroupController', function () {
       }
       this.Controller.removeSelfFromGroup(this.req, res)
     })
+
+    it('should log that the user has left the subscription', function (done) {
+      this.req.query = { subscriptionId: this.subscriptionId }
+      const memberUserIdToremove = 123456789
+      this.req.session.user._id = memberUserIdToremove
+
+      const res = {
+        sendStatus: () => {
+          sinon.assert.calledWith(
+            this.UserAuditLogHandler.addEntry,
+            memberUserIdToremove,
+            'remove-from-group-subscription',
+            memberUserIdToremove,
+            this.req.ip,
+            { subscriptionId: this.subscriptionId }
+          )
+          done()
+        },
+      }
+      this.Controller.removeSelfFromGroup(this.req, res)
+    })
   })
 })

services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js

@@ -147,6 +147,12 @@ describe('SubscriptionUpdater', function () {
       hasFeature: sinon.stub().returns(false),
     }
 
+    this.UserAuditLogHandler = {
+      promises: {
+        addEntry: sinon.stub().resolves(),
+      },
+    }
+
     this.SubscriptionUpdater = SandboxedModule.require(modulePath, {
       requires: {
         '../../models/Subscription': {
@@ -162,6 +168,7 @@ describe('SubscriptionUpdater', function () {
         },
         '../Analytics/AnalyticsManager': this.AnalyticsManager,
         '../../infrastructure/Features': this.Features,
+        '../User/UserAuditLogHandler': this.UserAuditLogHandler,
       },
     })
   })
@@ -487,6 +494,23 @@ describe('SubscriptionUpdater', function () {
         'better_group_subscription'
       )
     })
+
+    it('should add an entry to the user audit log when joining a group', async function () {
+      await this.SubscriptionUpdater.promises.addUserToGroup(
+        this.subscription._id,
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.UserAuditLogHandler.promises.addEntry,
+        this.otherUserId,
+        'join-group-subscription',
+        undefined,
+        undefined,
+        {
+          subscriptionId: this.subscription._id,
+        }
+      )
+    })
   })
 
   describe('removeUserFromGroup', function () {
@@ -554,6 +578,23 @@ describe('SubscriptionUpdater', function () {
         .calledWith(this.otherUserId)
         .should.equal(true)
     })
+
+    it('should add an audit log when a user leaves a group', async function () {
+      await this.SubscriptionUpdater.promises.removeUserFromGroup(
+        this.subscription._id,
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.UserAuditLogHandler.promises.addEntry,
+        this.otherUserId,
+        'leave-group-subscription',
+        undefined,
+        undefined,
+        {
+          subscriptionId: this.subscription._id,
+        }
+      )
+    })
   })
 
   describe('removeUserFromAllGroups', function () {
@@ -638,6 +679,32 @@ describe('SubscriptionUpdater', function () {
         }
       )
     })
+
+    it('should add an audit log entry for each group the user leaves', async function () {
+      await this.SubscriptionUpdater.promises.removeUserFromAllGroups(
+        this.otherUserId
+      )
+      sinon.assert.calledWith(
+        this.UserAuditLogHandler.promises.addEntry,
+        this.otherUserId,
+        'leave-group-subscription',
+        undefined,
+        undefined,
+        {
+          subscriptionId: 'fake-id-1',
+        }
+      )
+      sinon.assert.calledWith(
+        this.UserAuditLogHandler.promises.addEntry,
+        this.otherUserId,
+        'leave-group-subscription',
+        undefined,
+        undefined,
+        {
+          subscriptionId: 'fake-id-2',
+        }
+      )
+    })
   })
 
   describe('deleteSubscription', function () {

services/web/test/unit/src/Subscription/TeamInvitesControllerTests.js

@@ -0,0 +1,102 @@
+const SandboxedModule = require('sandboxed-module')
+const sinon = require('sinon')
+const modulePath =
+  '../../../../app/src/Features/Subscription/TeamInvitesController'
+
+describe('TeamInvitesController', function () {
+  beforeEach(function () {
+    this.user = { _id: '!@312431', email: 'user@email.com' }
+    this.adminUserId = '123jlkj'
+    this.subscriptionId = '123434325412'
+    this.user_email = 'bob@gmail.com'
+    this.req = {
+      session: {
+        user: {
+          _id: this.adminUserId,
+          email: this.user_email,
+        },
+      },
+      params: {},
+      query: {},
+      ip: '0.0.0.0',
+    }
+
+    this.subscription = {
+      _id: this.subscriptionId,
+    }
+
+    this.TeamInvitesHandler = {
+      promises: { acceptInvite: sinon.stub().resolves(this.subscription) },
+    }
+
+    this.SubscriptionLocator = {
+      promises: {
+        hasSSOEnabled: sinon.stub().resolves(true),
+      },
+    }
+    this.ErrorController = { notFound: sinon.stub() }
+
+    this.SessionManager = {
+      getLoggedInUserId(session) {
+        return session.user._id
+      },
+      getSessionUser(session) {
+        return session.user
+      },
+    }
+
+    this.UserAuditLogHandler = {
+      promises: {
+        addEntry: sinon.stub().resolves(),
+      },
+    }
+    this.UserGetter = {
+      promises: {
+        getUser: sinon.stub().resolves(this.user),
+        getUserByMainEmail: sinon.stub().resolves(this.user),
+        getUserByAnyEmail: sinon.stub().resolves(this.user),
+      },
+    }
+    this.EmailHandler = {
+      sendDeferredEmail: sinon.stub().resolves(),
+    }
+
+    this.RateLimiter = {
+      RateLimiter: class {},
+    }
+
+    this.Controller = SandboxedModule.require(modulePath, {
+      requires: {
+        './TeamInvitesHandler': this.TeamInvitesHandler,
+        '../Authentication/SessionManager': this.SessionManager,
+        './SubscriptionLocator': this.SubscriptionLocator,
+        '../User/UserAuditLogHandler': this.UserAuditLogHandler,
+        '../Errors/ErrorController': this.ErrorController,
+        '../User/UserGetter': this.UserGetter,
+        '../Email/EmailHandler': this.EmailHandler,
+        '../../infrastructure/RateLimiter': this.RateLimiter,
+      },
+    })
+  })
+
+  describe('acceptInvite', function () {
+    it('should add an audit log entry', function (done) {
+      this.req.params.token = 'foo'
+      this.req.session.user = this.user
+      const res = {
+        json: () => {
+          sinon.assert.calledWith(
+            this.UserAuditLogHandler.promises.addEntry,
+            this.user._id,
+            'accept-group-invitation',
+            this.user._id,
+            this.req.ip,
+            { subscriptionId: this.subscriptionId }
+          )
+          done()
+        },
+      }
+      this.Controller.acceptInvite(this.req, res)
+    })
+  })
+})

services/web/test/unit/src/User/UserAuditLogHandlerTests.js

@@ -81,6 +81,18 @@ describe('UserAuditLogHandler', function () {
         )
         this.UserAuditLogEntryMock.verify()
       })
+
+      it('updates the log when no ip address or initiatorId is specified for a group join event', async function () {
+        this.UserAuditLogHandler.promises.addEntry(
+          this.userId,
+          'join-group-subscription',
+          undefined,
+          undefined,
+          {
+            subscriptionId: 'foo',
+          }
+        )
+      })
     })
 
     describe('errors', function () {
@@ -97,7 +109,7 @@ describe('UserAuditLogHandler', function () {
           ).to.be.rejected
         })
 
-        it('throws an error when no IP', async function () {
+        it('throws an error when no IP and not excempt', async function () {
           await expect(
             this.UserAuditLogHandler.promises.addEntry(
               this.userId,