Merge pull request #3713 from overleaf/jpa-login-event-drop-pii

[AuthenticationController] do not include PII as part of login event

GitOrigin-RevId: 274378b3a21945637dc33d2cfb39a53e9aaad9b7

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

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

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