Merge pull request #9951 from overleaf/jk-audit-failed-login-attempts

[web] Audit failed login attempts

GitOrigin-RevId: 19325f808f77584891e1e12b5ed7aaa16aa6aec9

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) {

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,

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')
     })
   })
 })

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,

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)
         })
       })