diff --git a/services/web/app/src/Features/User/UserAuditLogHandler.js b/services/web/app/src/Features/User/UserAuditLogHandler.js index d2f7f85670..1284603e89 100644 --- a/services/web/app/src/Features/User/UserAuditLogHandler.js +++ b/services/web/app/src/Features/User/UserAuditLogHandler.js @@ -9,6 +9,7 @@ function _canHaveNoInitiatorId(operation, info) { 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 } /** diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index f4ef8f78c9..f1c2acd2b2 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/services/web/app/src/Features/User/UserEmailsController.js @@ -164,8 +164,11 @@ const UserEmailsController = { if (!email) { return res.sendStatus(422) } - - UserUpdater.removeEmailAddress(userId, email, function (error) { + const auditLog = { + initiatorId: userId, + ipAddress: req.ip, + } + UserUpdater.removeEmailAddress(userId, email, auditLog, function (error) { if (error) { return next(error) } diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index b7fc030ed8..ee77bd9c08 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/services/web/app/src/Features/User/UserUpdater.js @@ -310,7 +310,12 @@ async function maybeCreateRedundantSubscriptionNotification(userId, email) { .create() } -async function removeEmailAddress(userId, email, skipParseEmail = false) { +async function removeEmailAddress( + userId, + email, + auditLog, + skipParseEmail = false +) { // remove one of the user's email addresses. The email cannot be the user's // default email address if (!skipParseEmail) { @@ -330,6 +335,18 @@ async function removeEmailAddress(userId, email, skipParseEmail = false) { throw new Error('cannot remove primary email') } + await UserAuditLogHandler.promises.addEntry( + userId, + 'remove-email', + auditLog.initiatorId, + auditLog.ipAddress, + { + removedEmail: email, + // Add optional extra info + ...(auditLog.extraInfo || {}), + } + ) + try { await InstitutionsAPI.promises.removeAffiliation(userId, email) } catch (error) { @@ -406,7 +423,7 @@ async function changeEmailAddress(userId, newEmail, auditLog) { const oldEmail = await UserGetter.promises.getUserEmail(userId) await addEmailAddress(userId, newEmail, {}, auditLog) await setDefaultEmailAddress(userId, newEmail, true, auditLog, true) - await removeEmailAddress(userId, oldEmail) + await removeEmailAddress(userId, oldEmail, auditLog) } async function removeReconfirmFlag(userId) { diff --git a/services/web/scripts/remove_email.js b/services/web/scripts/remove_email.js index a10ab686c5..53cc4dff36 100644 --- a/services/web/scripts/remove_email.js +++ b/services/web/scripts/remove_email.js @@ -47,6 +47,19 @@ async function removeEmail() { ) } + const auditLog = { + initiatorId: undefined, + ipAddress: '0.0.0.0', + extraInfo: { + script: true, + }, + } + const skipParseEmail = true - await UserUpdater.promises.removeEmailAddress(userId, email, skipParseEmail) + await UserUpdater.promises.removeEmailAddress( + userId, + email, + auditLog, + skipParseEmail + ) } diff --git a/services/web/test/unit/src/User/UserAuditLogHandlerTests.js b/services/web/test/unit/src/User/UserAuditLogHandlerTests.js index a2c6c95d5a..f711a50fcc 100644 --- a/services/web/test/unit/src/User/UserAuditLogHandlerTests.js +++ b/services/web/test/unit/src/User/UserAuditLogHandlerTests.js @@ -65,6 +65,22 @@ describe('UserAuditLogHandler', function () { ) this.UserMock.verify() }) + + it('updates the log for a email removal via script', async function () { + await expect( + this.UserAuditLogHandler.promises.addEntry( + this.userId, + 'remove-email', + undefined, + this.action.ip, + { + removedEmail: 'foo', + script: true, + } + ) + ) + this.UserMock.verify() + }) }) describe('errors', function () { @@ -124,6 +140,20 @@ describe('UserAuditLogHandler', function () { ) ).to.be.rejected }) + + it('throws an error when remove-email is not from a script, but has no initiatorId', async function () { + await expect( + this.UserAuditLogHandler.promises.addEntry( + this.userId, + 'remove-email', + undefined, + this.action.ip, + { + removedEmail: 'foo', + } + ) + ).to.be.rejected + }) }) }) }) diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index c58585dc6a..2542a65b6b 100644 --- a/services/web/test/unit/src/User/UserEmailsControllerTests.js +++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js @@ -244,7 +244,11 @@ describe('UserEmailsController', function () { }) it('removes email', function (done) { - this.UserUpdater.removeEmailAddress.callsArgWith(2, null) + const auditLog = { + initiatorId: this.user._id, + ipAddress: this.req.ip, + } + this.UserUpdater.removeEmailAddress.callsArgWith(3, null) this.UserEmailsController.remove(this.req, { sendStatus: code => { @@ -253,7 +257,8 @@ describe('UserEmailsController', function () { assertCalledWith( this.UserUpdater.removeEmailAddress, this.user._id, - this.email + this.email, + auditLog ) done() }, diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index bff3909d8e..0635abc857 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/services/web/test/unit/src/User/UserUpdaterTests.js @@ -376,10 +376,14 @@ describe('UserUpdater', function () { }) describe('removeEmailAddress', function () { + this.beforeEach(function () { + this.auditLog = { initiatorId: this.user._id, ipAddress: '127:0:0:0' } + }) it('removes the email', async function () { await this.UserUpdater.promises.removeEmailAddress( this.user._id, - this.newEmail + this.newEmail, + this.auditLog ) expect(this.db.users.updateOne).to.have.been.calledWith( { _id: this.user._id, email: { $ne: this.newEmail } }, @@ -390,7 +394,8 @@ describe('UserUpdater', function () { it('removes the affiliation', async function () { await this.UserUpdater.promises.removeEmailAddress( this.user._id, - this.newEmail + this.newEmail, + this.auditLog ) expect(this.InstitutionsAPI.promises.removeAffiliation).to.have.been .calledOnce @@ -402,7 +407,8 @@ describe('UserUpdater', function () { it('refreshes features', async function () { await this.UserUpdater.promises.removeEmailAddress( this.user._id, - this.newEmail + this.newEmail, + this.auditLog ) sinon.assert.calledWith( this.FeaturesUpdater.promises.refreshFeatures, @@ -417,7 +423,8 @@ describe('UserUpdater', function () { await expect( this.UserUpdater.promises.removeEmailAddress( this.user._id, - this.newEmail + this.newEmail, + this.auditLog ) ).to.be.rejected expect(this.FeaturesUpdater.promises.refreshFeatures).not.to.have.been @@ -430,7 +437,8 @@ describe('UserUpdater', function () { await expect( this.UserUpdater.promises.removeEmailAddress( this.user._id, - this.newEmail + this.newEmail, + this.auditLog ) ).to.be.rejectedWith('Cannot remove email') expect(this.FeaturesUpdater.promises.refreshFeatures).not.to.have.been @@ -443,7 +451,8 @@ describe('UserUpdater', function () { await expect( this.UserUpdater.promises.removeEmailAddress( this.user._id, - this.newEmail + this.newEmail, + this.auditLog ) ).to.be.rejected expect(this.db.users.updateOne).not.to.have.been.called @@ -455,7 +464,8 @@ describe('UserUpdater', function () { await expect( this.UserUpdater.promises.removeEmailAddress( this.user._id, - this.user.email + this.user.email, + this.auditLog ) ).to.be.rejectedWith('cannot remove primary email') expect(this.db.users.updateOne).not.to.have.been.called @@ -465,7 +475,11 @@ describe('UserUpdater', function () { it('validates the email', function () { expect( - this.UserUpdater.promises.removeEmailAddress(this.user._id, 'baz') + this.UserUpdater.promises.removeEmailAddress( + this.user._id, + 'baz', + this.auditLog + ) ).to.be.rejectedWith('invalid email') }) @@ -474,6 +488,7 @@ describe('UserUpdater', function () { await this.UserUpdater.promises.removeEmailAddress( this.user._id, 'baz', + this.auditLog, skipParseEmail ) }) @@ -484,10 +499,57 @@ describe('UserUpdater', function () { this.UserUpdater.promises.removeEmailAddress( this.user._id, 1, + this.auditLog, skipParseEmail ) ).to.be.rejectedWith('email must be a string') }) + + it('logs the removal to the audit log', async function () { + await this.UserUpdater.promises.removeEmailAddress( + this.user._id, + this.newEmail, + this.auditLog + ) + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.user._id, + 'remove-email', + this.auditLog.initiatorId, + this.auditLog.ipAddress, + { + removedEmail: this.newEmail, + } + ) + }) + + it('logs the removal from script to the audit log', async function () { + this.auditLog = { + initiatorId: undefined, + ipAddress: '0.0.0.0', + extraInfo: { + script: true, + }, + } + await this.UserUpdater.promises.removeEmailAddress( + this.user._id, + this.newEmail, + this.auditLog + ) + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.user._id, + 'remove-email', + this.auditLog.initiatorId, + this.auditLog.ipAddress, + { + removedEmail: this.newEmail, + script: true, + } + ) + }) }) describe('setDefaultEmailAddress', function () {