diff --git a/services/web/app/src/Features/User/UserAuditLogHandler.js b/services/web/app/src/Features/User/UserAuditLogHandler.js index f43be7f150..5e926758b9 100644 --- a/services/web/app/src/Features/User/UserAuditLogHandler.js +++ b/services/web/app/src/Features/User/UserAuditLogHandler.js @@ -15,6 +15,19 @@ const MAX_AUDIT_LOG_ENTRIES = 200 * - 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, + initiatorId, + ipAddress + }) + + if (!initiatorId && operation !== 'reset-password') + throw new OError('missing initiatorId for audit log', { + operation, + ipAddress + }) + const timestamp = new Date() const entry = { operation, diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index f34e916a64..9bea98e205 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -338,7 +338,11 @@ const UserController = { res.sendStatus(400) } else { // update the user email - UserUpdater.changeEmailAddress(userId, newEmail, err => { + const auditLog = { + initiatorId: userId, + ipAddress: req.ip + } + UserUpdater.changeEmailAddress(userId, newEmail, auditLog, err => { if (err) { if (err instanceof Errors.EmailExistsError) { const translation = req.i18n.translate( diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index b79b261d00..f3ae4116b3 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/services/web/app/src/Features/User/UserEmailsController.js @@ -121,7 +121,11 @@ const UserEmailsController = { if (!email) { return res.sendStatus(422) } - UserUpdater.setDefaultEmailAddress(userId, email, false, err => { + const auditLog = { + initiatorId: userId, + ipAddress: req.ip + } + UserUpdater.setDefaultEmailAddress(userId, email, false, auditLog, err => { if (err) { return UserEmailsController._handleEmailError(err, req, res, next) } diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 204322bfcb..bbe57298ef 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/services/web/app/src/Features/User/UserUpdater.js @@ -17,8 +17,14 @@ const EmailHelper = require('../Helpers/EmailHelper') const Errors = require('../Errors/Errors') const NewsletterManager = require('../Newsletter/NewsletterManager') const RecurlyWrapper = require('../Subscription/RecurlyWrapper') +const UserAuditLogHandler = require('./UserAuditLogHandler') -async function setDefaultEmailAddress(userId, email, allowUnconfirmed) { +async function setDefaultEmailAddress( + userId, + email, + allowUnconfirmed, + auditLog +) { email = EmailHelper.parseEmail(email) if (email == null) { throw new Error('invalid email') @@ -41,6 +47,17 @@ async function setDefaultEmailAddress(userId, email, allowUnconfirmed) { throw new Errors.UnconfirmedEmailError() } + await UserAuditLogHandler.promises.addEntry( + userId, + 'change-primary-email', + auditLog.initiatorId, + auditLog.ipAddress, + { + newPrimaryEmail: email, + oldPrimaryEmail: oldEmail + } + ) + const query = { _id: userId, 'emails.email': email } const update = { $set: { email } } const res = await UserUpdater.promises.updateUser(query, update) @@ -117,7 +134,7 @@ const UserUpdater = { // default email and removing the old email. Prefer manipulating multiple // emails and the default rather than calling this method directly // - changeEmailAddress(userId, newEmail, callback) { + changeEmailAddress(userId, newEmail, auditLog, callback) { newEmail = EmailHelper.parseEmail(newEmail) if (newEmail == null) { return callback(new Error('invalid email')) @@ -132,7 +149,14 @@ const UserUpdater = { cb(error) }), cb => UserUpdater.addEmailAddress(userId, newEmail, cb), - cb => UserUpdater.setDefaultEmailAddress(userId, newEmail, true, cb), + cb => + UserUpdater.setDefaultEmailAddress( + userId, + newEmail, + true, + auditLog, + cb + ), cb => UserUpdater.removeEmailAddress(userId, oldEmail, cb) ], callback diff --git a/services/web/test/acceptance/src/UserEmailsTests.js b/services/web/test/acceptance/src/UserEmailsTests.js index 2595706e0c..300afe60b1 100644 --- a/services/web/test/acceptance/src/UserEmailsTests.js +++ b/services/web/test/acceptance/src/UserEmailsTests.js @@ -774,6 +774,69 @@ describe('UserEmails', function() { done ) }) + + describe('audit log', function() { + const originalEmail = 'original@overleaf.com' + let otherEmail, response, userHelper, user, userId + beforeEach(async function() { + otherEmail = 'other@overleaf.com' + userHelper = new UserHelper() + userHelper = await UserHelper.createUser({ + email: originalEmail + }) + userHelper = await UserHelper.loginUser({ + email: originalEmail, + password: userHelper.getDefaultPassword() + }) + userId = userHelper.user._id + response = await userHelper.request.post({ + form: { + email: otherEmail + }, + simple: false, + uri: '/user/emails' + }) + expect(response.statusCode).to.equal(204) + const token = await new Promise(resolve => { + db.tokens.findOne( + { + 'data.user_id': userId.toString(), + 'data.email': otherEmail + }, + (error, tokenData) => { + expect(error).to.not.exist + resolve(tokenData.token) + } + ) + }) + response = await userHelper.request.post(`/user/emails/confirm`, { + form: { + token + }, + simple: false + }) + expect(response.statusCode).to.equal(200) + response = await userHelper.request.post('/user/emails/default', { + form: { + email: otherEmail + }, + simple: false + }) + expect(response.statusCode).to.equal(200) + userHelper = await UserHelper.getUser(userId) + user = userHelper.user + }) + it('should be updated', function() { + const entry = user.auditLog[user.auditLog.length - 1] + expect(typeof entry.initiatorId).to.equal('object') + expect(entry.initiatorId).to.deep.equal(user._id) + expect(entry.ipAddress).to.equal('127.0.0.1') + expect(entry.info).to.deep.equal({ + newPrimaryEmail: otherEmail, + oldPrimaryEmail: originalEmail + }) + }) + }) }) describe('when not logged in', function() { diff --git a/services/web/test/unit/src/User/UserAuditLogHandlerTests.js b/services/web/test/unit/src/User/UserAuditLogHandlerTests.js index 5e67452edb..6bc3704e2c 100644 --- a/services/web/test/unit/src/User/UserAuditLogHandlerTests.js +++ b/services/web/test/unit/src/User/UserAuditLogHandlerTests.js @@ -40,10 +40,12 @@ describe('UserAuditLogHandler', function() { describe('addEntry', function() { describe('success', function() { - beforeEach(async function() { + beforeEach(function() { this.dbUpdate = this.UserMock.expects('updateOne') .chain('exec') .resolves({ nModified: 1 }) + }) + it('writes a log', async function() { await this.UserAuditLogHandler.promises.addEntry( this.userId, this.action.operation, @@ -51,30 +53,80 @@ describe('UserAuditLogHandler', function() { this.action.ip, this.action.info ) + this.UserMock.verify() }) - it('writes a log', async function() { + it('updates the log for password reset operation witout a initiatorId', async function() { + await expect( + this.UserAuditLogHandler.promises.addEntry( + this.userId, + 'reset-password', + undefined, + this.action.ip, + this.action.info + ) + ) this.UserMock.verify() }) }) - describe('when the user does not exist', function() { - beforeEach(function() { - this.UserMock.expects('updateOne') - .chain('exec') - .resolves({ nModified: 0 }) + describe('errors', function() { + describe('when the user does not exist', function() { + beforeEach(function() { + this.UserMock.expects('updateOne') + .chain('exec') + .resolves({ nModified: 0 }) + }) + + it('throws an error', async function() { + await expect( + this.UserAuditLogHandler.promises.addEntry( + this.userId, + this.action.operation, + this.action.initiatorId, + this.action.ip, + this.action.info + ) + ).to.be.rejected + }) }) - it('throws an error', async function() { - await expect( - this.UserAuditLogHandler.promises.addEntry( - this.userId, - this.action.operation, - this.action.initiatorId, - this.action.ip, - this.action.info - ) - ).to.be.rejected + describe('missing parameters', function() { + it('throws an error when no operation', async function() { + await expect( + this.UserAuditLogHandler.promises.addEntry( + this.userId, + undefined, + this.action.initiatorId, + this.action.ip, + this.action.info + ) + ).to.be.rejected + }) + + it('throws an error when no IP', async function() { + await expect( + this.UserAuditLogHandler.promises.addEntry( + this.userId, + this.action.operation, + this.action.initiatorId, + undefined, + this.action.info + ) + ).to.be.rejected + }) + + it('throws an error when no initiatorId and not a password reset operation', async function() { + await expect( + this.UserAuditLogHandler.promises.addEntry( + this.userId, + this.action.operation, + undefined, + this.action.ip, + this.action.info + ) + ).to.be.rejected + }) }) }) }) diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 4b34d8e8dc..1982e9308f 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -303,6 +303,7 @@ describe('UserController', function() { describe('updateUserSettings', function() { beforeEach(function() { + this.auditLog = { initiatorId: this.user_id, ipAddress: this.req.ip } this.newEmail = 'hello@world.com' this.req.externalAuthenticationSystemUsed = sinon.stub().returns(false) }) @@ -381,11 +382,11 @@ describe('UserController', function() { it('should call the user updater with the new email and user _id', function(done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(2) + this.UserUpdater.changeEmailAddress.callsArgWith(3) this.res.sendStatus = code => { code.should.equal(200) this.UserUpdater.changeEmailAddress - .calledWith(this.user_id, this.newEmail) + .calledWith(this.user_id, this.newEmail, this.auditLog) .should.equal(true) done() } @@ -394,7 +395,7 @@ describe('UserController', function() { it('should update the email on the session', function(done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(2) + this.UserUpdater.changeEmailAddress.callsArgWith(3) let callcount = 0 this.User.findById = (id, cb) => { if (++callcount === 2) { @@ -418,7 +419,7 @@ describe('UserController', function() { it('should call populateTeamInvites', function(done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(2) + this.UserUpdater.changeEmailAddress.callsArgWith(3) this.res.sendStatus = code => { code.should.equal(200) this.UserHandler.populateTeamInvites @@ -432,7 +433,7 @@ describe('UserController', function() { describe('when changeEmailAddress yields an error', function() { it('should pass on an error and not send a success status', function(done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(2, new OError()) + this.UserUpdater.changeEmailAddress.callsArgWith(3, new OError()) this.HttpErrorHandler.legacyInternal = sinon.spy( (req, res, message, error) => { expect(req).to.exist @@ -454,7 +455,7 @@ describe('UserController', function() { }) this.req.body.email = this.newEmail.toUpperCase() this.UserUpdater.changeEmailAddress.callsArgWith( - 2, + 3, new Errors.EmailExistsError() ) this.UserController.updateUserSettings(this.req, this.res) diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index 41221873b0..4068a2e54c 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/services/web/test/unit/src/User/UserUpdaterTests.js @@ -68,7 +68,12 @@ describe('UserUpdater', function() { 'settings-sharelatex': (this.settings = {}), request: (this.request = {}), '../Newsletter/NewsletterManager': this.NewsletterManager, - '../Subscription/RecurlyWrapper': this.RecurlyWrapper + '../Subscription/RecurlyWrapper': this.RecurlyWrapper, + './UserAuditLogHandler': (this.UserAuditLogHandler = { + promises: { + addEntry: sinon.stub().resolves() + } + }) } }) @@ -125,6 +130,10 @@ describe('UserUpdater', function() { describe('changeEmailAddress', function() { beforeEach(function() { + this.auditLog = { + initiatorId: 'abc123', + ipAddress: '0:0:0:0' + } this.UserGetter.getUserEmail.callsArgWith(1, null, this.stubbedUser.email) this.UserUpdater.addEmailAddress = sinon.stub().callsArgWith(2) this.UserUpdater.setDefaultEmailAddress = sinon.stub().yields() @@ -135,6 +144,7 @@ describe('UserUpdater', function() { this.UserUpdater.changeEmailAddress( this.stubbedUser._id, this.newEmail, + this.auditLog, err => { should.not.exist(err) this.UserUpdater.addEmailAddress @@ -152,10 +162,15 @@ describe('UserUpdater', function() { }) it('validates email', function(done) { - this.UserUpdater.changeEmailAddress(this.stubbedUser._id, 'foo', err => { - should.exist(err) - done() - }) + this.UserUpdater.changeEmailAddress( + this.stubbedUser._id, + 'foo', + this.auditLog, + err => { + should.exist(err) + done() + } + ) }) it('handle error', function(done) { @@ -163,6 +178,7 @@ describe('UserUpdater', function() { this.UserUpdater.changeEmailAddress( this.stubbedUser._id, this.newEmail, + this.auditLog, err => { should.exist(err) done() @@ -350,6 +366,10 @@ describe('UserUpdater', function() { describe('setDefaultEmailAddress', function() { beforeEach(function() { + this.auditLog = { + initiatorId: this.stubbedUser, + ipAddress: '0:0:0:0' + } this.stubbedUser.emails = [ { email: this.newEmail, @@ -368,6 +388,7 @@ describe('UserUpdater', function() { this.stubbedUser._id, this.newEmail, false, + this.auditLog, err => { should.not.exist(err) this.UserUpdater.promises.updateUser @@ -388,6 +409,7 @@ describe('UserUpdater', function() { this.stubbedUser._id, this.newEmail, false, + this.auditLog, err => { should.not.exist(err) this.NewsletterManager.promises.changeEmail @@ -408,6 +430,7 @@ describe('UserUpdater', function() { this.stubbedUser._id, this.newEmail, false, + this.auditLog, err => { should.exist(err) done() @@ -422,6 +445,7 @@ describe('UserUpdater', function() { this.stubbedUser._id, this.newEmail, false, + this.auditLog, err => { should.exist(err) done() @@ -434,6 +458,7 @@ describe('UserUpdater', function() { this.stubbedUser._id, '.edu', false, + this.auditLog, err => { should.exist(err) done() @@ -441,6 +466,49 @@ describe('UserUpdater', function() { ) }) + it('updates audit log', function(done) { + this.UserUpdater.promises.updateUser = sinon.stub().resolves({ n: 1 }) + + this.UserUpdater.setDefaultEmailAddress( + this.stubbedUser._id, + this.newEmail, + false, + this.auditLog, + error => { + expect(error).to.not.exist + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledWith( + this.stubbedUser._id, + 'change-primary-email', + this.auditLog.initiatorId, + this.auditLog.ipAddress, + { + newPrimaryEmail: this.newEmail, + oldPrimaryEmail: this.stubbedUser.email + } + ) + done() + } + ) + }) + + it('blocks email update if audit log returns an error', function(done) { + this.UserUpdater.promises.updateUser = sinon.stub() + this.UserAuditLogHandler.promises.addEntry.rejects(new Error('oops')) + this.UserUpdater.setDefaultEmailAddress( + this.stubbedUser._id, + this.newEmail, + false, + this.auditLog, + error => { + expect(error).to.exist + expect(this.UserUpdater.promises.updateUser).to.not.have.been.called + done() + } + ) + }) + describe('when email not confirmed', function() { beforeEach(function() { this.stubbedUser.emails = [ @@ -457,6 +525,7 @@ describe('UserUpdater', function() { this.stubbedUser._id, this.newEmail, false, + this.auditLog, error => { expect(error).to.exist expect(error.name).to.equal('UnconfirmedEmailError') @@ -481,6 +550,7 @@ describe('UserUpdater', function() { this.stubbedUser._id, this.newEmail, false, + this.auditLog, error => { expect(error).to.exist expect(error.name).to.equal('Error')