Merge pull request #3227 from overleaf/jel-security-email-alerts

Move security alert handling to private function

GitOrigin-RevId: a59b6b0802986b2caa9e9715d80225eb11b163a9

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

@@ -1,5 +1,4 @@
 const APP_ROOT = '../../../../app/src'
-const OError = require('@overleaf/o-error')
 const EmailHandler = require(`${APP_ROOT}/Features/Email/EmailHandler`)
 const Errors = require('../Errors/Errors')
 const _ = require('lodash')
@@ -62,6 +61,7 @@ function link(
   callback,
   retry
 ) {
+  const accountLinked = true
   if (!oauthProviders[providerId]) {
     return callback(new Error('Not a valid provider'))
   }
@@ -88,21 +88,8 @@ function link(
     } else if (err != null) {
       callback(err)
     } else if (res) {
-      const providerName = oauthProviders[providerId].name
-      const indefiniteArticle = _getIndefiniteArticle(providerName)
-      const emailOptions = {
-        to: res.email,
-        action: `${providerName} account linked`,
-        actionDescribed: `${indefiniteArticle} ${providerName} account was linked to your account ${
-          res.email
-        }`
-      }
-      EmailHandler.sendEmail('securityAlert', emailOptions, error => {
-        if (error != null) {
-          logger.warn(OError.tag(error))
-        }
-        return callback(null, res)
-      })
+      _sendSecurityAlert(accountLinked, providerId, res, userId)
+      callback(null, res)
     } else if (retry) {
       // if already retried then throw error
       callback(new Error('update failed'))
@@ -126,6 +113,7 @@ function link(
 }
 
 function unlink(userId, providerId, callback) {
+  const accountLinked = false
   if (!oauthProviders[providerId]) {
     return callback(new Error('Not a valid provider'))
   }
@@ -146,21 +134,9 @@ function unlink(userId, providerId, callback) {
     } else if (!res) {
       callback(new Error('update failed'))
     } else {
-      const providerName = oauthProviders[providerId].name
-      const indefiniteArticle = _getIndefiniteArticle(providerName)
-      const emailOptions = {
-        to: res.email,
-        action: `${providerName} account no longer linked`,
-        actionDescribed: `${indefiniteArticle} ${providerName} account is no longer linked to your account ${
-          res.email
-        }`
-      }
-      EmailHandler.sendEmail('securityAlert', emailOptions, error => {
-        if (error != null) {
-          logger.warn(error)
-        }
-        return callback(null, res)
-      })
+      // no need to wait, errors are logged and not passed back
+      _sendSecurityAlert(accountLinked, providerId, res, userId)
+      callback(null, res)
     }
   })
 }
@@ -175,6 +151,28 @@ function _getUserQuery(providerId, externalUserId) {
   return query
 }
 
+function _sendSecurityAlert(accountLinked, providerId, user, userId) {
+  const operation = accountLinked ? 'linked' : 'no longer linked'
+  const tense = accountLinked ? 'was' : 'is'
+  const providerName = oauthProviders[providerId].name
+  const indefiniteArticle = _getIndefiniteArticle(providerName)
+  const emailOptions = {
+    to: user.email,
+    action: `${providerName} account ${operation}`,
+    actionDescribed: `${indefiniteArticle} ${providerName} account ${tense} ${operation} to your account ${
+      user.email
+    }`
+  }
+  EmailHandler.sendEmail('securityAlert', emailOptions, error => {
+    if (error) {
+      logger.error(
+        { error, userId },
+        `could not send security alert email when ${providerName} ${operation}`
+      )
+    }
+  })
+}
+
 function _thirdPartyIdentifierUpdate(
   user,
   providerId,

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

@@ -21,6 +21,44 @@ const UrlHelper = require('../Helpers/UrlHelper')
 const { promisify } = require('util')
 const { expressify } = require('../../util/promises')
 
+async function _sendSecurityAlertClearedSessions(user) {
+  const emailOptions = {
+    to: user.email,
+    actionDescribed: `active sessions were cleared on your account ${
+      user.email
+    }`,
+    action: 'active sessions cleared'
+  }
+  try {
+    await EmailHandler.promises.sendEmail('securityAlert', emailOptions)
+  } catch (error) {
+    // log error when sending security alert email but do not pass back
+    logger.error(
+      { error, userId: user._id },
+      'could not send security alert email when sessions cleared'
+    )
+  }
+}
+
+function _sendSecurityAlertPasswordChanged(user) {
+  const emailOptions = {
+    to: user.email,
+    actionDescribed: `your password has been changed on your account ${
+      user.email
+    }`,
+    action: 'password changed'
+  }
+  EmailHandler.sendEmail('securityAlert', emailOptions, error => {
+    if (error) {
+      // log error when sending security alert email but do not pass back
+      logger.error(
+        { error, userId: user._id },
+        'could not send security alert email when password changed'
+      )
+    }
+  })
+}
+
 async function _ensureAffiliation(userId, emailData) {
   if (emailData.samlProviderId) {
     await UserUpdater.promises.confirmEmail(userId, emailData.email)
@@ -67,19 +105,8 @@ async function changePassword(req, res, next) {
     req.body.newPassword1
   )
 
-  const emailOptions = {
-    to: user.email,
-    actionDescribed: `your password has been changed on your account ${
-      user.email
-    }`,
-    action: 'password changed'
-  }
-  EmailHandler.sendEmail('securityAlert', emailOptions, error => {
-    if (error) {
-      // log error when sending security alert email but do not pass back
-      logger.error({ err: error })
-    }
-  })
+  // no need to wait, errors are logged and not passed back
+  _sendSecurityAlertPasswordChanged(user)
 
   await UserSessionsManager.promises.revokeAllUserSessions(user, [
     req.sessionID
@@ -96,7 +123,8 @@ async function changePassword(req, res, next) {
 
 async function clearSessions(req, res, next) {
   metrics.inc('user.clear-sessions')
-  const user = AuthenticationController.getSessionUser(req)
+  const userId = AuthenticationController.getLoggedInUserId(req)
+  const user = await UserGetter.promises.getUser(userId, { email: 1 })
   const sessions = await UserSessionsManager.promises.getAllUserSessions(user, [
     req.sessionID
   ])
@@ -110,22 +138,8 @@ async function clearSessions(req, res, next) {
   await UserSessionsManager.promises.revokeAllUserSessions(user, [
     req.sessionID
   ])
-  const emailOptions = {
-    to: user.email,
-    actionDescribed: `active sessions were cleared on your account ${
-      user.email
-    }`,
-    action: 'active sessions cleared'
-  }
 
-  try {
-    await EmailHandler.promises.sendEmail('securityAlert', emailOptions)
-  } catch (error) {
-    logger.error(
-      { userId: user._id },
-      'could not send security alert email when sessions cleared'
-    )
-  }
+  await _sendSecurityAlertClearedSessions(user)
 
   res.sendStatus(201)
 }

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

@@ -19,6 +19,26 @@ const NewsletterManager = require('../Newsletter/NewsletterManager')
 const RecurlyWrapper = require('../Subscription/RecurlyWrapper')
 const UserAuditLogHandler = require('./UserAuditLogHandler')
 
+async function _sendSecurityAlertPrimaryEmailChanged(userId, oldEmail, email) {
+  // send email to both old and new primary email
+  const emailOptions = {
+    actionDescribed: `the primary email address on your account was changed to ${email}`,
+    action: 'change of primary email address'
+  }
+  const toOld = Object.assign({}, emailOptions, { to: oldEmail })
+  const toNew = Object.assign({}, emailOptions, { to: email })
+
+  try {
+    await EmailHandler.promises.sendEmail('securityAlert', toOld)
+    await EmailHandler.promises.sendEmail('securityAlert', toNew)
+  } catch (error) {
+    logger.error(
+      { error, userId },
+      'could not send security alert email when primary email changed'
+    )
+  }
+}
+
 async function addEmailAddress(userId, newEmail, affiliationOptions, auditLog) {
   newEmail = EmailHelper.parseEmail(newEmail)
   if (!newEmail) {
@@ -114,22 +134,8 @@ async function setDefaultEmailAddress(
   }
 
   if (sendSecurityAlert) {
-    // send email to both old and new primary email
-    const emailOptions = {
-      actionDescribed: `the primary email address on your account was changed to ${email}`,
-      action: 'change of primary email address'
-    }
-    const toOld = Object.assign({}, emailOptions, { to: oldEmail })
-    const toNew = Object.assign({}, emailOptions, { to: email })
-    try {
-      await EmailHandler.promises.sendEmail('securityAlert', toOld)
-      await EmailHandler.promises.sendEmail('securityAlert', toNew)
-    } catch (error) {
-      logger.error(
-        { err: error, userId },
-        'could not send security alert email when primary email changed'
-      )
-    }
+    // no need to wait, errors are logged and not passed back
+    _sendSecurityAlertPrimaryEmailChanged(userId, oldEmail, email)
   }
 
   try {

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

@@ -26,6 +26,9 @@ describe('ThirdPartyIdentityManager', function() {
           ThirdPartyIdentityExistsError: sinon.stub(),
           ThirdPartyUserNotFoundError: sinon.stub()
         }),
+        'logger-sharelatex': (this.logger = {
+          error: sinon.stub()
+        }),
         '../../../../app/src/models/User': {
           User: (this.User = {
             findOneAndUpdate: sinon.stub().yields(undefined, this.user),
@@ -59,6 +62,34 @@ describe('ThirdPartyIdentityManager', function() {
         'a Google account was linked'
       )
     })
+    describe('errors', function() {
+      const anError = new Error('oops')
+      describe('EmailHandler', function() {
+        beforeEach(function() {
+          this.EmailHandler.sendEmail.yields(anError)
+        })
+        it('should log but not return the error', function(done) {
+          this.ThirdPartyIdentityManager.link(
+            this.userId,
+            'google',
+            this.externalUserId,
+            this.externalData,
+            error => {
+              expect(error).to.not.exist
+              const loggerCall = this.logger.error.getCall(0)
+              expect(loggerCall.args[0]).to.deep.equal({
+                error: anError,
+                userId: this.userId
+              })
+              expect(loggerCall.args[1]).to.contain(
+                'could not send security alert email when Google linked'
+              )
+              done()
+            }
+          )
+        })
+      })
+    })
   })
   describe('unlink', function() {
     it('should send email alert', async function() {
@@ -69,5 +100,31 @@ describe('ThirdPartyIdentityManager', function() {
         'an Orcid account is no longer linked'
       )
     })
+    describe('errors', function() {
+      const anError = new Error('oops')
+      describe('EmailHandler', function() {
+        beforeEach(function() {
+          this.EmailHandler.sendEmail.yields(anError)
+        })
+        it('should log but not return the error', function(done) {
+          this.ThirdPartyIdentityManager.unlink(
+            this.userId,
+            'google',
+            error => {
+              expect(error).to.not.exist
+              const loggerCall = this.logger.error.getCall(0)
+              expect(loggerCall.args[0]).to.deep.equal({
+                error: anError,
+                userId: this.userId
+              })
+              expect(loggerCall.args[1]).to.contain(
+                'could not send security alert email when Google no longer linked'
+              )
+              done()
+            }
+          )
+        })
+      })
+    })
   })
 })

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

@@ -596,6 +596,16 @@ describe('UserController', function() {
       it('sends a security alert email', function(done) {
         this.res.sendStatus.callsFake(status => {
           this.EmailHandler.promises.sendEmail.callCount.should.equal(1)
+          const expectedArg = {
+            to: this.user.email,
+            actionDescribed: `active sessions were cleared on your account ${
+              this.user.email
+            }`,
+            action: 'active sessions cleared'
+          }
+          const emailCall = this.EmailHandler.promises.sendEmail.lastCall
+          expect(emailCall.args[0]).to.equal('securityAlert')
+          expect(emailCall.args[1]).to.deep.equal(expectedArg)
           done()
         })
 
@@ -639,11 +649,20 @@ describe('UserController', function() {
       })
 
       describe('when EmailHandler produces an error', function() {
+        const anError = new Error('oops')
         it('send a 201 response but log error', function(done) {
-          this.EmailHandler.promises.sendEmail.rejects(new Error('oops'))
+          this.EmailHandler.promises.sendEmail.rejects(anError)
           this.res.sendStatus.callsFake(status => {
             status.should.equal(201)
             this.logger.error.callCount.should.equal(1)
+            const loggerCall = this.logger.error.getCall(0)
+            expect(loggerCall.args[0]).to.deep.equal({
+              error: anError,
+              userId: this.user_id
+            })
+            expect(loggerCall.args[1]).to.contain(
+              'could not send security alert email when sessions cleared'
+            )
             done()
           })
           this.UserController.clearSessions(this.req, this.res)
@@ -793,6 +812,7 @@ describe('UserController', function() {
       })
 
       describe('EmailHandler error', function() {
+        const anError = new Error('oops')
         beforeEach(function() {
           this.AuthenticationManager.promises.authenticate.resolves(this.user)
           this.AuthenticationManager.promises.setUserPassword.resolves()
@@ -800,12 +820,19 @@ describe('UserController', function() {
             newPassword1: 'newpass',
             newPassword2: 'newpass'
           }
-          this.EmailHandler.sendEmail.yields(new Error('oops'))
+          this.EmailHandler.sendEmail.yields(anError)
         })
         it('should not return error but should log it', function(done) {
           this.res.json.callsFake(result => {
             expect(result.message.type).to.equal('success')
             this.logger.error.callCount.should.equal(1)
+            expect(this.logger.error).to.have.been.calledWithExactly(
+              {
+                error: anError,
+                userId: this.user_id
+              },
+              'could not send security alert email when password changed'
+            )
             done()
           })
           this.UserController.changePassword(this.req, this.res)

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

@@ -669,6 +669,38 @@ describe('UserUpdater', function() {
           }
         )
       })
+      describe('errors', function() {
+        const anError = new Error('oops')
+        describe('EmailHandler', function() {
+          beforeEach(function() {
+            this.EmailHandler.promises.sendEmail.rejects(anError)
+            this.UserUpdater.promises.updateUser = sinon
+              .stub()
+              .resolves({ n: 1 })
+          })
+          it('should log but not pass back the error', function(done) {
+            this.UserUpdater.setDefaultEmailAddress(
+              this.stubbedUser._id,
+              this.newEmail,
+              false,
+              this.auditLog,
+              true,
+              error => {
+                expect(error).to.not.exist
+                const loggerCall = this.logger.error.firstCall
+                expect(loggerCall.args[0]).to.deep.equal({
+                  error: anError,
+                  userId: this.stubbedUser._id
+                })
+                expect(loggerCall.args[1]).to.contain(
+                  'could not send security alert email when primary email changed'
+                )
+                done()
+              }
+            )
+          })
+        })
+      })
     })
   })