diff --git a/services/web/app/src/Features/Subscription/SubscriptionGroupController.js b/services/web/app/src/Features/Subscription/SubscriptionGroupController.js index 7e9a3a2579..238fcd9d80 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionGroupController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionGroupController.js @@ -3,27 +3,44 @@ 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' ) - SubscriptionGroupHandler.removeUserFromGroup( - subscription._id, + UserAuditLogHandler.addEntry( userToRemoveId, - function (error) { - if (error) { - OError.tag(error, 'error removing user from group', { + '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, - userToRemove_id: userToRemoveId, }) - return next(error) + return next(auditLogError) } - - res.sendStatus(200) + SubscriptionGroupHandler.removeUserFromGroup( + subscription._id, + userToRemoveId, + function (error) { + if (error) { + OError.tag(error, 'error removing user from group', { + subscriptionId: subscription._id, + userToRemove_id: userToRemoveId, + }) + return next(error) + } + res.sendStatus(200) + } + ) } ) } @@ -38,18 +55,34 @@ function removeSelfFromGroup(req, res, next) { return next(error) } - SubscriptionGroupHandler.removeUserFromGroup( - subscription._id, + UserAuditLogHandler.addEntry( userToRemoveId, - function (error) { - if (error) { - logger.err( - { err: error, userToRemoveId, subscriptionId }, - 'error removing self from group' - ) - return res.sendStatus(500) + '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) } - res.sendStatus(200) + SubscriptionGroupHandler.removeUserFromGroup( + subscription._id, + userToRemoveId, + function (error) { + if (error) { + logger.err( + { err: error, userToRemoveId, subscriptionId }, + 'error removing self from group' + ) + return res.sendStatus(500) + } + res.sendStatus(200) + } + ) } ) } diff --git a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js b/services/web/app/src/Features/Subscription/SubscriptionUpdater.js index 13d72044a2..0b7284e5ba 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionUpdater.js +++ b/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 diff --git a/services/web/app/src/Features/Subscription/TeamInvitesController.js b/services/web/app/src/Features/Subscription/TeamInvitesController.js index 48a0cf9dff..aef4cd853c 100644 --- a/services/web/app/src/Features/Subscription/TeamInvitesController.js +++ b/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 }) } diff --git a/services/web/app/src/Features/User/UserAuditLogHandler.js b/services/web/app/src/Features/User/UserAuditLogHandler.js index 366ea814cd..a02ebe70cf 100644 --- a/services/web/app/src/Features/User/UserAuditLogHandler.js +++ b/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', { diff --git a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js index f944870abb..ff855e7534 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionGroupControllerTests.js +++ b/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) + }) }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js b/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js index d689857fbc..b606c7462e 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionUpdaterTests.js +++ b/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 () { diff --git a/services/web/test/unit/src/Subscription/TeamInvitesControllerTests.js b/services/web/test/unit/src/Subscription/TeamInvitesControllerTests.js new file mode 100644 index 0000000000..a65560133f --- /dev/null +++ b/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) + }) + }) +}) diff --git a/services/web/test/unit/src/User/UserAuditLogHandlerTests.js b/services/web/test/unit/src/User/UserAuditLogHandlerTests.js index 4f39a1fd71..86bd07d141 100644 --- a/services/web/test/unit/src/User/UserAuditLogHandlerTests.js +++ b/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,