diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index ec3e1fffd2..7448c64549 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -196,9 +196,14 @@ const AuthenticationController = { status: 429, }) } + const auditLog = { + ipAddress: req.ip, + info: { method: 'Password login' }, + } AuthenticationManager.authenticate( { email }, password, + auditLog, function (error, user) { if (error != null) { if (error instanceof ParallelLoginError) { diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 32a1cd61fc..1c1b17ee80 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -11,6 +11,8 @@ const { } = require('./AuthenticationErrors') const util = require('util') const HaveIBeenPwned = require('./HaveIBeenPwned') +const UserAuditLogHandler = require('../User/UserAuditLogHandler') +const logger = require('@overleaf/logger') const BCRYPT_ROUNDS = Settings.security.bcryptRounds || 12 const BCRYPT_MINOR_VERSION = Settings.security.bcryptMinorVersion || 'a' @@ -56,7 +58,11 @@ const AuthenticationManager = { }) }, - authenticate(query, password, callback) { + authenticate(query, password, auditLog, callback) { + if (typeof callback === 'undefined') { + callback = auditLog + auditLog = null + } AuthenticationManager._checkUserPassword( query, password, @@ -83,7 +89,26 @@ const AuthenticationManager = { return callback(new ParallelLoginError()) } if (!match) { - return callback(null, null) + if (!auditLog) { + return callback(null, null) + } else { + return UserAuditLogHandler.addEntry( + user._id, + 'failed-password-match', + user._id, + auditLog.ipAddress, + auditLog.info, + err => { + if (err) { + logger.error( + { userId: user._id, err, info: auditLog.info }, + 'Error while adding AuditLog entry for failed-password-match' + ) + } + callback(null, null) + } + ) + } } AuthenticationManager.checkRounds( user, diff --git a/services/web/test/acceptance/src/AuthenticationTests.js b/services/web/test/acceptance/src/AuthenticationTests.js index 87a902191b..22a38bbc20 100644 --- a/services/web/test/acceptance/src/AuthenticationTests.js +++ b/services/web/test/acceptance/src/AuthenticationTests.js @@ -76,9 +76,11 @@ describe('Authentication', function () { describe('failed login', function () { beforeEach('fetchCsrfToken', async function () { + await user.login() + await user.logout() await user.getCsrfToken() }) - it('should return a 401', async function () { + it('should return a 401, and add an entry to the audit log', async function () { const { response: { statusCode }, } = await user.doRequest('POST', { @@ -90,6 +92,17 @@ describe('Authentication', function () { }, }) expect(statusCode).to.equal(401) + const auditLog = await user.getAuditLog() + const auditLogEntry = auditLog.pop() + expect(auditLogEntry).to.exist + expect(auditLogEntry.timestamp).to.exist + expect(auditLogEntry.initiatorId).to.deep.equal(ObjectId(user.id)) + expect(auditLogEntry.userId).to.deep.equal(ObjectId(user.id)) + expect(auditLogEntry.operation).to.equal('failed-password-match') + expect(auditLogEntry.info).to.deep.equal({ + method: 'Password login', + }) + expect(auditLogEntry.ipAddress).to.equal('127.0.0.1') }) }) }) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index a1a5f08a3c..72aa800a73 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -333,7 +333,7 @@ describe('AuthenticationController', function () { this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) this.AuthenticationManager.authenticate = sinon .stub() - .callsArgWith(2, null, this.user) + .callsArgWith(3, null, this.user) this.req.sessionID = Math.random() }) @@ -361,7 +361,7 @@ describe('AuthenticationController', function () { beforeEach(function () { this.AuthenticationManager.authenticate = sinon .stub() - .callsArgWith(2, new AuthenticationErrors.ParallelLoginError()) + .callsArgWith(3, new AuthenticationErrors.ParallelLoginError()) this.AuthenticationController.doPassportLogin( this.req, this.req.body.email, @@ -440,7 +440,7 @@ describe('AuthenticationController', function () { this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) this.AuthenticationManager.authenticate = sinon .stub() - .callsArgWith(2, null, null) + .callsArgWith(3, null, null) this.cb = sinon.stub() this.AuthenticationController.doPassportLogin( this.req, diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 3eae2000f6..a93e8cb8b1 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -30,6 +30,9 @@ describe('AuthenticationManager', function () { './HaveIBeenPwned': { checkPasswordForReuseInBackground: sinon.stub(), }, + '../User/UserAuditLogHandler': (this.UserAuditLogHandler = { + addEntry: sinon.stub().callsArgWith(5, null), + }), }, }) this.callback = sinon.stub() @@ -270,6 +273,8 @@ describe('AuthenticationManager', function () { describe('when the encrypted passwords do not match', function () { beforeEach(function () { + this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' + this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) this.AuthenticationManager.authenticate( { email: this.email }, this.unencryptedPassword, @@ -279,6 +284,35 @@ describe('AuthenticationManager', function () { it('should not return the user', function () { this.callback.calledWith(null, null).should.equal(true) + this.UserAuditLogHandler.addEntry.callCount.should.equal(0) + }) + }) + + describe('when the encrypted passwords do not match, with auditLog', function () { + beforeEach(function () { + this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' + this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) + this.auditLog = { ipAddress: 'ip', info: { method: 'foo' } } + this.AuthenticationManager.authenticate( + { email: this.email }, + this.unencryptedPassword, + this.auditLog, + this.callback + ) + }) + + it('should not return the user, but add entry to audit log', function () { + this.callback.calledWith(null, null).should.equal(true) + this.UserAuditLogHandler.addEntry.callCount.should.equal(1) + this.UserAuditLogHandler.addEntry + .calledWith( + this.user._id, + 'failed-password-match', + this.user._id, + this.auditLog.ipAddress, + this.auditLog.info + ) + .should.equal(true) }) })