From 5736b8adf40b69bc9b86822d44ea8d40eab3f5ae Mon Sep 17 00:00:00 2001 From: Jessica Lawshe <5312836+lawshe@users.noreply.github.com> Date: Tue, 27 May 2025 09:39:21 -0500 Subject: [PATCH] Merge pull request #25556 from overleaf/jel-group-audit-log-remove-from-group [web] Log when user leaves or is removed from group GitOrigin-RevId: 8a5042b21cbf4eb622d5ca35cc095d94fe5a8539 --- .../SubscriptionGroupController.mjs | 8 ++- .../Subscription/SubscriptionGroupHandler.js | 9 ++- .../Subscription/SubscriptionUpdater.js | 72 ++++++++++++------- .../SubscriptionGroupControllerTests.mjs | 11 ++- .../SubscriptionGroupHandlerTests.js | 10 ++- .../types/group-management/group-audit-log.ts | 7 ++ 6 files changed, 83 insertions(+), 34 deletions(-) create mode 100644 services/web/types/group-management/group-audit-log.ts diff --git a/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs b/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs index ce1207cded..90ecd51091 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs +++ b/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs @@ -108,10 +108,16 @@ async function _removeUserFromGroup( }) } + const groupAuditLog = { + initiatorId: loggedInUserId, + ipAddress: req.ip, + } + try { await SubscriptionGroupHandler.promises.removeUserFromGroup( subscriptionId, - userToRemoveId + userToRemoveId, + groupAuditLog ) } catch (error) { logger.err( diff --git a/services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js b/services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js index 5772946b8a..c717b2eec6 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js +++ b/services/web/app/src/Features/Subscription/SubscriptionGroupHandler.js @@ -22,10 +22,11 @@ const { const EmailHelper = require('../Helpers/EmailHelper') const { InvalidEmailError } = require('../Errors/Errors') -async function removeUserFromGroup(subscriptionId, userIdToRemove) { +async function removeUserFromGroup(subscriptionId, userIdToRemove, auditLog) { await SubscriptionUpdater.promises.removeUserFromGroup( subscriptionId, - userIdToRemove + userIdToRemove, + auditLog ) } @@ -463,7 +464,9 @@ async function updateGroupMembersBulk( ) } for (const user of membersToRemove) { - await removeUserFromGroup(subscription._id, user._id) + await removeUserFromGroup(subscription._id, user._id, { + initiatorId: inviterId, + }) } } diff --git a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js index 7b57e32619..15f61b6160 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js +++ b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js @@ -18,8 +18,31 @@ const Modules = require('../../infrastructure/Modules') /** * @typedef {import('../../../../types/subscription/dashboard/subscription').Subscription} Subscription * @typedef {import('../../../../types/subscription/dashboard/subscription').PaymentProvider} PaymentProvider + * @typedef {import('../../../../types/group-management/group-audit-log').GroupAuditLog} GroupAuditLog */ +/** + * + * @param {GroupAuditLog} auditLog + */ +async function subscriptionUpdateWithAuditLog(dbFilter, dbUpdate, auditLog) { + const session = await mongoose.startSession() + + try { + await session.withTransaction(async () => { + await Subscription.updateOne(dbFilter, dbUpdate, { session }).exec() + + await Modules.promises.hooks.fire( + 'addGroupAuditLogEntry', + auditLog, + session + ) + }) + } finally { + await session.endSession() + } +} + /** * Change the admin of the given subscription. * @@ -68,8 +91,6 @@ async function syncSubscription( } async function addUserToGroup(subscriptionId, userId, auditLog) { - const session = await mongoose.startSession() - await UserAuditLogHandler.promises.addEntry( userId, 'join-group-subscription', @@ -78,28 +99,16 @@ async function addUserToGroup(subscriptionId, userId, auditLog) { { subscriptionId } ) - try { - await session.withTransaction(async () => { - await Subscription.updateOne( - { _id: subscriptionId }, - { $addToSet: { member_ids: userId } }, - { session } - ).exec() - - await Modules.promises.hooks.fire( - 'addGroupAuditLogEntry', - { - initiatorId: auditLog?.initiatorId, - ipAddress: auditLog?.ipAddress, - groupId: subscriptionId, - operation: 'join-group', - }, - session - ) - }) - } finally { - await session.endSession() - } + await subscriptionUpdateWithAuditLog( + { _id: subscriptionId }, + { $addToSet: { member_ids: userId } }, + { + initiatorId: auditLog?.initiatorId, + ipAddress: auditLog?.ipAddress, + groupId: subscriptionId, + operation: 'join-group', + } + ) await FeaturesUpdater.promises.refreshFeatures(userId, 'add-to-group') await _sendUserGroupPlanCodeUserProperty(userId) @@ -110,7 +119,7 @@ async function addUserToGroup(subscriptionId, userId, auditLog) { ) } -async function removeUserFromGroup(subscriptionId, userId) { +async function removeUserFromGroup(subscriptionId, userId, auditLog) { await UserAuditLogHandler.promises.addEntry( userId, 'leave-group-subscription', @@ -118,6 +127,19 @@ async function removeUserFromGroup(subscriptionId, userId) { undefined, { subscriptionId } ) + + await subscriptionUpdateWithAuditLog( + { _id: subscriptionId }, + { $pull: { member_ids: userId } }, + { + initiatorId: auditLog?.initiatorId, + ipAddress: auditLog?.ipAddress, + groupId: subscriptionId, + operation: 'leave-group', + info: { userIdRemoved: userId }, + } + ) + await Subscription.updateOne( { _id: subscriptionId }, { $pull: { member_ids: userId } } diff --git a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.mjs b/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.mjs index 4376e752e7..c1ce6733ca 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.mjs +++ b/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.mjs @@ -185,7 +185,10 @@ describe('SubscriptionGroupController', function () { const res = { sendStatus: () => { this.SubscriptionGroupHandler.promises.removeUserFromGroup - .calledWith(this.subscriptionId, userIdToRemove) + .calledWith(this.subscriptionId, userIdToRemove, { + initiatorId: this.req.session.user._id, + ipAddress: this.req.ip, + }) .should.equal(true) done() }, @@ -277,7 +280,11 @@ describe('SubscriptionGroupController', function () { sinon.assert.calledWith( this.SubscriptionGroupHandler.promises.removeUserFromGroup, this.subscriptionId, - memberUserIdToremove + memberUserIdToremove, + { + initiatorId: this.req.session.user._id, + ipAddress: this.req.ip, + } ) done() }, diff --git a/services/web/test/unit/src/Subscription/SubscriptionGroupHandlerTests.js b/services/web/test/unit/src/Subscription/SubscriptionGroupHandlerTests.js index 0c47db3e14..1c314458da 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionGroupHandlerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionGroupHandlerTests.js @@ -233,13 +233,15 @@ describe('SubscriptionGroupHandler', function () { describe('removeUserFromGroup', function () { it('should call the subscription updater to remove the user', async function () { + const auditLog = { ipAddress: '0:0:0:0', initiatorId: this.user._id } await this.Handler.promises.removeUserFromGroup( this.adminUser_id, - this.user._id + this.user._id, + auditLog ) this.SubscriptionUpdater.promises.removeUserFromGroup - .calledWith(this.adminUser_id, this.user._id) + .calledWith(this.adminUser_id, this.user._id, auditLog) .should.equal(true) }) }) @@ -1149,7 +1151,9 @@ describe('SubscriptionGroupHandler', function () { expect( this.SubscriptionUpdater.promises.removeUserFromGroup - ).to.have.been.calledWith(this.subscription._id, members[2]._id) + ).to.have.been.calledWith(this.subscription._id, members[2]._id, { + initiatorId: inviterId, + }) expect( this.TeamInvitesHandler.promises.createInvite.callCount diff --git a/services/web/types/group-management/group-audit-log.ts b/services/web/types/group-management/group-audit-log.ts new file mode 100644 index 0000000000..c96c12e7cd --- /dev/null +++ b/services/web/types/group-management/group-audit-log.ts @@ -0,0 +1,7 @@ +export type GroupAuditLog = { + groupId: string + operation: string + ipAddress?: string + initiatorId?: string + info?: object +}