diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 4bde56200d..5d472f457c 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -17,6 +17,7 @@ const NotificationsBuilder = require('../Notifications/NotificationsBuilder') const UrlHelper = require('../Helpers/UrlHelper') const AsyncFormHelper = require('../Helpers/AsyncFormHelper') const _ = require('lodash') +const UserAuditLogHandler = require('../User/UserAuditLogHandler') const { acceptsJson } = require('../../infrastructure/RequestContentTypeDetection') @@ -98,12 +99,18 @@ const AuthenticationController = { const redir = AuthenticationController._getRedirectFromSession(req) || '/project' _loginAsyncHandlers(req, user) - _afterLoginSessionSetup(req, user, function(err) { + const userId = user._id + UserAuditLogHandler.addEntry(userId, 'login', userId, req.ip, err => { if (err) { return next(err) } - AuthenticationController._clearRedirectFromSession(req) - AsyncFormHelper.redirect(req, res, redir) + _afterLoginSessionSetup(req, user, function(err) { + if (err) { + return next(err) + } + AuthenticationController._clearRedirectFromSession(req) + AsyncFormHelper.redirect(req, res, redir) + }) }) }) }, @@ -508,7 +515,7 @@ function _loginAsyncHandlers(req, user) { LoginRateLimiter.recordSuccessfulLogin(user.email) AuthenticationController._recordSuccessfulLogin(user._id) AuthenticationController.ipMatchCheck(req, user) - Analytics.recordEvent(user._id, 'user-logged-in', { ip: req.ip }) + Analytics.recordEvent(user._id, 'user-logged-in') Analytics.identifyUser(user._id, req.sessionID) logger.log( { email: user.email, user_id: user._id.toString() }, diff --git a/services/web/test/acceptance/src/AuthenticationTests.js b/services/web/test/acceptance/src/AuthenticationTests.js new file mode 100644 index 0000000000..fd2df6dbc9 --- /dev/null +++ b/services/web/test/acceptance/src/AuthenticationTests.js @@ -0,0 +1,37 @@ +const { expect } = require('chai') +const { ObjectId } = require('mongodb') +const User = require('./helpers/User').promises + +describe('Authentication', function() { + let user + beforeEach('init vars', function() { + user = new User() + }) + + describe('login', function() { + beforeEach('doLogin', async function() { + await user.login() + }) + + it('should log the user in', async function() { + const { + response: { statusCode } + } = await user.doRequest('GET', '/project') + expect(statusCode).to.equal(200) + }) + + it('should emit an user auditLog entry for the login', async function() { + const { + auditLog: [auditLogEntry] + } = await user.get() + expect(auditLogEntry).to.exist + expect(auditLogEntry.timestamp).to.exist + delete auditLogEntry.timestamp + expect(auditLogEntry).to.deep.equal({ + operation: 'login', + ipAddress: '127.0.0.1', + initiatorId: ObjectId(user.id) + }) + }) + }) +}) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index ac15897ef1..221bc8f762 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -23,6 +23,9 @@ describe('AuthenticationController', function() { console: console }, requires: { + '../User/UserAuditLogHandler': (this.UserAuditLogHandler = { + addEntry: sinon.stub().yields(null) + }), '../Helpers/AsyncFormHelper': (this.AsyncFormHelper = { redirect: sinon.stub() }), @@ -1188,6 +1191,48 @@ describe('AuthenticationController', function() { }) }) + describe('UserAuditLog', function() { + it('should add an audit log entry', function() { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + expect(this.UserAuditLogHandler.addEntry).to.have.been.calledWith( + this.user._id, + 'login', + this.user._id, + '42.42.42.42' + ) + }) + + it('should add an audit log entry before logging the user in', function() { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + expect(this.UserAuditLogHandler.addEntry).to.have.been.calledBefore( + this.req.login + ) + }) + + it('should not log the user in without an audit log entry', function() { + const theError = new Error() + this.UserAuditLogHandler.addEntry.yields(theError) + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + expect(this.next).to.have.been.calledWith(theError) + expect(this.req.login).to.not.have.been.called + }) + }) + describe('_afterLoginSessionSetup', function() { beforeEach(function() {})