Merge pull request #3220 from overleaf/jel-link-ieee

Move link/unlink SSO audit log entry

GitOrigin-RevId: 1b912cc58957af7e80628f3f955f01c2a641812d

services/web/app/src/Features/User/ThirdPartyIdentityManager.js

@@ -1,4 +1,5 @@
 const APP_ROOT = '../../../../app/src'
+const UserAuditLogHandler = require(`${APP_ROOT}/Features/User/UserAuditLogHandler`)
 const EmailHandler = require(`${APP_ROOT}/Features/Email/EmailHandler`)
 const Errors = require('../Errors/Errors')
 const _ = require('lodash')
@@ -58,6 +59,7 @@ function link(
   providerId,
   externalUserId,
   externalData,
+  auditLog,
   callback,
   retry
 ) {
@@ -65,80 +67,115 @@ function link(
   if (!oauthProviders[providerId]) {
     return callback(new Error('Not a valid provider'))
   }
-  const query = {
-    _id: userId,
-    'thirdPartyIdentifiers.providerId': {
-      $ne: providerId
-    }
-  }
-  const update = {
-    $push: {
-      thirdPartyIdentifiers: {
-        externalUserId,
-        externalData,
-        providerId
+
+  UserAuditLogHandler.addEntry(
+    userId,
+    'link-sso',
+    auditLog.initiatorId,
+    auditLog.ipAddress,
+    {
+      providerId
+    },
+    error => {
+      if (error) {
+        return callback(error)
       }
-    }
-  }
-  // add new tpi only if an entry for the provider does not exist
-  // projection includes thirdPartyIdentifiers for tests
-  User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => {
-    if (err && err.code === 11000) {
-      callback(new Errors.ThirdPartyIdentityExistsError())
-    } else if (err != null) {
-      callback(err)
-    } else if (res) {
-      _sendSecurityAlert(accountLinked, providerId, res, userId)
-      callback(null, res)
-    } else if (retry) {
-      // if already retried then throw error
-      callback(new Error('update failed'))
-    } else {
-      // attempt to clear existing entry then retry
-      ThirdPartyIdentityManager.unlink(userId, providerId, function(err) {
-        if (err != null) {
-          return callback(err)
+      const query = {
+        _id: userId,
+        'thirdPartyIdentifiers.providerId': {
+          $ne: providerId
+        }
+      }
+      const update = {
+        $push: {
+          thirdPartyIdentifiers: {
+            externalUserId,
+            externalData,
+            providerId
+          }
+        }
+      }
+      // add new tpi only if an entry for the provider does not exist
+      // projection includes thirdPartyIdentifiers for tests
+      User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => {
+        if (err && err.code === 11000) {
+          callback(new Errors.ThirdPartyIdentityExistsError())
+        } else if (err != null) {
+          callback(err)
+        } else if (res) {
+          _sendSecurityAlert(accountLinked, providerId, res, userId)
+          callback(null, res)
+        } else if (retry) {
+          // if already retried then throw error
+          callback(new Error('update failed'))
+        } else {
+          // attempt to clear existing entry then retry
+          ThirdPartyIdentityManager.unlink(
+            userId,
+            providerId,
+            auditLog,
+            function(err) {
+              if (err != null) {
+                return callback(err)
+              }
+              ThirdPartyIdentityManager.link(
+                userId,
+                providerId,
+                externalUserId,
+                externalData,
+                auditLog,
+                callback,
+                true
+              )
+            }
+          )
         }
-        ThirdPartyIdentityManager.link(
-          userId,
-          providerId,
-          externalUserId,
-          externalData,
-          callback,
-          true
-        )
       })
     }
-  })
+  )
 }
 
-function unlink(userId, providerId, callback) {
+function unlink(userId, providerId, auditLog, callback) {
   const accountLinked = false
   if (!oauthProviders[providerId]) {
     return callback(new Error('Not a valid provider'))
   }
-  const query = {
-    _id: userId
-  }
-  const update = {
-    $pull: {
-      thirdPartyIdentifiers: {
-        providerId
+  UserAuditLogHandler.addEntry(
+    userId,
+    'unlink-sso',
+    auditLog.initiatorId,
+    auditLog.ipAddress,
+    {
+      providerId
+    },
+    error => {
+      if (error) {
+        return callback(error)
       }
+      const query = {
+        _id: userId
+      }
+      const update = {
+        $pull: {
+          thirdPartyIdentifiers: {
+            providerId
+          }
+        }
+      }
+      // projection includes thirdPartyIdentifiers for tests
+      User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => {
+        if (err != null) {
+          callback(err)
+        } else if (!res) {
+          callback(new Error('update failed'))
+        } else {
+          // no need to wait, errors are logged and not passed back
+          _sendSecurityAlert(accountLinked, providerId, res, userId)
+          callback(null, res)
+        }
+      })
     }
-  }
-  // projection includes thirdPartyIdentifiers for tests
-  User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => {
-    if (err != null) {
-      callback(err)
-    } else if (!res) {
-      callback(new Error('update failed'))
-    } else {
-      // no need to wait, errors are logged and not passed back
-      _sendSecurityAlert(accountLinked, providerId, res, userId)
-      callback(null, res)
-    }
-  })
+  )
 }
 
 function _getUserQuery(providerId, externalUserId) {

services/web/app/src/Features/User/UserAuditLogHandler.js

@@ -1,8 +1,15 @@
 const OError = require('@overleaf/o-error')
 const { User } = require('../../models/User')
+const { callbackify } = require('util')
 
 const MAX_AUDIT_LOG_ENTRIES = 200
 
+function _canHaveNoInitiatorId(operation, info) {
+  if (operation === 'reset-password') return true
+  if (operation === 'unlink-sso' && info.providerId === 'collabratec')
+    return true
+}
+
 /**
  * Add an audit log entry
  *
@@ -22,11 +29,12 @@ async function addEntry(userId, operation, initiatorId, ipAddress, info = {}) {
       ipAddress
     })
 
-  if (!initiatorId && operation !== 'reset-password')
+  if (!initiatorId && !_canHaveNoInitiatorId(operation, info)) {
     throw new OError('missing initiatorId for audit log', {
       operation,
       ipAddress
     })
+  }
 
   const timestamp = new Date()
   const entry = {
@@ -50,6 +58,7 @@ async function addEntry(userId, operation, initiatorId, ipAddress, info = {}) {
 }
 
 const UserAuditLogHandler = {
+  addEntry: callbackify(addEntry),
   promises: {
     addEntry
   }

services/web/test/acceptance/src/UserThirdPartyIdentityTests.js

@@ -39,6 +39,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.provider,
           this.externalUserId,
           this.externalData,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           done
         )
       })
@@ -99,6 +100,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.provider,
           this.externalUserId,
           this.externalData,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           (err, res) => {
             expect(res.thirdPartyIdentifiers.length).to.equal(1)
             return done()
@@ -114,6 +116,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.provider,
           this.externalUserId,
           this.externalData,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           done
         )
       })
@@ -124,6 +127,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.provider,
           this.externalUserId,
           this.externalData,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           (err, res) => {
             expect(res).to.exist
             done()
@@ -137,6 +141,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.provider,
           this.externalUserId,
           this.externalData,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           (err, user) => {
             expect(user.thirdPartyIdentifiers.length).to.equal(1)
             return done()
@@ -151,6 +156,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.provider,
           this.externalUserId,
           this.externalData,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           (err, user) => {
             expect(user.thirdPartyIdentifiers.length).to.equal(1)
             return done()
@@ -187,6 +193,7 @@ describe('ThirdPartyIdentityManager', function() {
         return ThirdPartyIdentityManager.unlink(
           this.user.id,
           this.provider,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           (err, res) => {
             expect(err).to.be.null
             expect(res.thirdPartyIdentifiers.length).to.equal(0)
@@ -203,6 +210,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.provider,
           this.externalUserId,
           this.externalData,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           done
         )
       })
@@ -211,6 +219,7 @@ describe('ThirdPartyIdentityManager', function() {
         return ThirdPartyIdentityManager.unlink(
           this.user.id,
           this.provider,
+          { initiatorId: this.user.id, ipAddress: '0:0:0:0' },
           (err, user) => {
             expect(user.thirdPartyIdentifiers.length).to.equal(0)
             return done()

services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js

@@ -14,11 +14,15 @@ describe('ThirdPartyIdentityManager', function() {
     }
     this.externalUserId = 'id789'
     this.externalData = {}
+    this.auditLog = { initiatorId: this.userId, ipAddress: '0:0:0:0' }
     this.ThirdPartyIdentityManager = SandboxedModule.require(modulePath, {
       globals: {
         console: console
       },
       requires: {
+        '../../../../app/src/Features/User/UserAuditLogHandler': (this.UserAuditLogHandler = {
+          addEntry: sinon.stub().yields()
+        }),
         '../../../../app/src/Features/Email/EmailHandler': (this.EmailHandler = {
           sendEmail: sinon.stub().yields()
         }),
@@ -54,7 +58,8 @@ describe('ThirdPartyIdentityManager', function() {
         this.userId,
         'google',
         this.externalUserId,
-        this.externalData
+        this.externalData,
+        this.auditLog
       )
       const emailCall = this.EmailHandler.sendEmail.getCall(0)
       expect(emailCall.args[0]).to.equal('securityAlert')
@@ -62,8 +67,43 @@ describe('ThirdPartyIdentityManager', function() {
         'a Google account was linked'
       )
     })
+
+    it('should update user audit log', async function() {
+      await this.ThirdPartyIdentityManager.promises.link(
+        this.userId,
+        'google',
+        this.externalUserId,
+        this.externalData,
+        this.auditLog
+      )
+      expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith(
+        this.userId,
+        'link-sso',
+        this.auditLog.initiatorId,
+        this.auditLog.ipAddress,
+        {
+          providerId: 'google'
+        }
+      )
+    })
     describe('errors', function() {
       const anError = new Error('oops')
+      it('should not unlink if the UserAuditLogHandler throws an error', function(done) {
+        this.UserAuditLogHandler.addEntry.yields(anError)
+        this.ThirdPartyIdentityManager.link(
+          this.userId,
+          'google',
+          this.externalUserId,
+          this.externalData,
+          this.auditLog,
+          error => {
+            expect(error).to.exist
+            expect(error).to.equal(anError)
+            expect(this.User.findOneAndUpdate).to.not.have.been.called
+            done()
+          }
+        )
+      })
       describe('EmailHandler', function() {
         beforeEach(function() {
           this.EmailHandler.sendEmail.yields(anError)
@@ -74,6 +114,7 @@ describe('ThirdPartyIdentityManager', function() {
             'google',
             this.externalUserId,
             this.externalData,
+            this.auditLog,
             error => {
               expect(error).to.not.exist
               const loggerCall = this.logger.error.getCall(0)
@@ -93,15 +134,50 @@ describe('ThirdPartyIdentityManager', function() {
   })
   describe('unlink', function() {
     it('should send email alert', async function() {
-      await this.ThirdPartyIdentityManager.promises.unlink(this.userId, 'orcid')
+      await this.ThirdPartyIdentityManager.promises.unlink(
+        this.userId,
+        'orcid',
+        this.auditLog
+      )
       const emailCall = this.EmailHandler.sendEmail.getCall(0)
       expect(emailCall.args[0]).to.equal('securityAlert')
       expect(emailCall.args[1].actionDescribed).to.contain(
         'an Orcid account is no longer linked'
       )
     })
+    it('should update user audit log', async function() {
+      await this.ThirdPartyIdentityManager.promises.unlink(
+        this.userId,
+        'orcid',
+        this.auditLog
+      )
+      expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith(
+        this.userId,
+        'unlink-sso',
+        this.auditLog.initiatorId,
+        this.auditLog.ipAddress,
+        {
+          providerId: 'orcid'
+        }
+      )
+    })
     describe('errors', function() {
       const anError = new Error('oops')
+      it('should not unlink if the UserAuditLogHandler throws an error', function(done) {
+        this.UserAuditLogHandler.addEntry.yields(anError)
+        this.ThirdPartyIdentityManager.unlink(
+          this.userId,
+          'orcid',
+          this.auditLog,
+          error => {
+            expect(error).to.exist
+            expect(error).to.equal(anError)
+            expect(this.User.findOneAndUpdate).to.not.have.been.called
+            done()
+          }
+        )
+        expect(this.User.findOneAndUpdate).to.not.have.been.called
+      })
       describe('EmailHandler', function() {
         beforeEach(function() {
           this.EmailHandler.sendEmail.yields(anError)
@@ -110,6 +186,7 @@ describe('ThirdPartyIdentityManager', function() {
           this.ThirdPartyIdentityManager.unlink(
             this.userId,
             'google',
+            this.auditLog,
             error => {
               expect(error).to.not.exist
               const loggerCall = this.logger.error.getCall(0)