Merge pull request #3040 from overleaf/jel-email-added-alert

Secondary email added alert

GitOrigin-RevId: 6bfe8eb54110a522281b32490b0107db8890ab64

services/web/app/src/Features/Email/EmailBuilder.js

@@ -603,10 +603,22 @@ templates.securityAlert = NoCTAEmailTemplate({
       `${settings.siteUrl}/learn/how-to/Keeping_your_account_secure`,
       isPlainText
     )
+
+    const actionDescribed = EmailMessageHelper.cleanHTML(
+      opts.actionDescribed,
+      isPlainText
+    )
+
+    if (!opts.message) {
+      opts.message = []
+    }
+    const message = opts.message.map(m => {
+      return EmailMessageHelper.cleanHTML(m, isPlainText)
+    })
+
     return [
-      `We are writing to let you know that ${
-        opts.actionDescribed
-      } on ${dateFormatted} at ${timeFormatted} GMT.`,
+      `We are writing to let you know that ${actionDescribed} on ${dateFormatted} at ${timeFormatted} GMT.`,
+      ...message,
       `If this was you, you can ignore this email.`,
       `If this was not you, we recommend getting in touch with our support team at ${
         settings.adminEmail

services/web/app/src/Features/Email/EmailMessageHelper.js

@@ -1,7 +1,27 @@
+const sanitizeHtml = require('sanitize-html')
+const sanitizeOptions = {
+  html: {
+    allowedTags: ['span', 'b', 'br', 'i'],
+    allowedAttributes: {
+      span: ['style', 'class']
+    }
+  },
+  plainText: {
+    allowedTags: [],
+    allowedAttributes: {}
+  }
+}
+
+function cleanHTML(text, isPlainText) {
+  if (!isPlainText) return sanitizeHtml(text, sanitizeOptions.html)
+  return sanitizeHtml(text, sanitizeOptions.plainText)
+}
+
 function displayLink(text, url, isPlainText) {
   return isPlainText ? `${text} (${url})` : `<a href="${url}">${text}</a>`
 }
 
 module.exports = {
+  cleanHTML,
   displayLink
 }

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

@@ -5,46 +5,49 @@ const settings = require('settings-sharelatex')
 const Errors = require('../Errors/Errors')
 const UserUpdater = require('./UserUpdater')
 const UserGetter = require('./UserGetter')
+const { promisify } = require('util')
 
 const ONE_YEAR_IN_S = 365 * 24 * 60 * 60
 
-const UserEmailsConfirmationHandler = {
-  sendConfirmationEmail(userId, email, emailTemplate, callback) {
-    if (arguments.length === 3) {
-      callback = emailTemplate
-      emailTemplate = 'confirmEmail'
-    }
+function sendConfirmationEmail(userId, email, emailTemplate, callback) {
+  if (arguments.length === 3) {
+    callback = emailTemplate
+    emailTemplate = 'confirmEmail'
+  }
 
-    // when force-migrating accounts to v2 from v1, we don't want to send confirmation messages -
-    // setting this env var allows us to turn this behaviour off
-    if (process.env['SHARELATEX_NO_CONFIRMATION_MESSAGES'] != null) {
-      return callback(null)
-    }
+  // when force-migrating accounts to v2 from v1, we don't want to send confirmation messages -
+  // setting this env var allows us to turn this behaviour off
+  if (process.env['SHARELATEX_NO_CONFIRMATION_MESSAGES'] != null) {
+    return callback(null)
+  }
 
-    email = EmailHelper.parseEmail(email)
-    if (!email) {
-      return callback(new Error('invalid email'))
-    }
-    const data = { user_id: userId, email }
-    OneTimeTokenHandler.getNewToken(
-      'email_confirmation',
-      data,
-      { expiresIn: ONE_YEAR_IN_S },
-      function(err, token) {
-        if (err) {
-          return callback(err)
-        }
-        const emailOptions = {
-          to: email,
-          confirmEmailUrl: `${
-            settings.siteUrl
-          }/user/emails/confirm?token=${token}`,
-          sendingUser_id: userId
-        }
-        EmailHandler.sendEmail(emailTemplate, emailOptions, callback)
+  email = EmailHelper.parseEmail(email)
+  if (!email) {
+    return callback(new Error('invalid email'))
+  }
+  const data = { user_id: userId, email }
+  OneTimeTokenHandler.getNewToken(
+    'email_confirmation',
+    data,
+    { expiresIn: ONE_YEAR_IN_S },
+    function(err, token) {
+      if (err) {
+        return callback(err)
       }
-    )
-  },
+      const emailOptions = {
+        to: email,
+        confirmEmailUrl: `${
+          settings.siteUrl
+        }/user/emails/confirm?token=${token}`,
+        sendingUser_id: userId
+      }
+      EmailHandler.sendEmail(emailTemplate, emailOptions, callback)
+    }
+  )
+}
+
+const UserEmailsConfirmationHandler = {
+  sendConfirmationEmail,
 
   confirmEmailFromToken(token, callback) {
     OneTimeTokenHandler.getValueFromTokenAndExpire(
@@ -83,4 +86,8 @@ const UserEmailsConfirmationHandler = {
   }
 }
 
+UserEmailsConfirmationHandler.promises = {
+  sendConfirmationEmail: promisify(sendConfirmationEmail)
+}
+
 module.exports = UserEmailsConfirmationHandler

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

@@ -1,40 +1,55 @@
-let UserEmailsController
 const AuthenticationController = require('../Authentication/AuthenticationController')
 const UserGetter = require('./UserGetter')
 const UserUpdater = require('./UserUpdater')
+const EmailHandler = require('../Email/EmailHandler')
 const EmailHelper = require('../Helpers/EmailHelper')
 const UserEmailsConfirmationHandler = require('./UserEmailsConfirmationHandler')
 const { endorseAffiliation } = require('../Institutions/InstitutionsAPI')
 const Errors = require('../Errors/Errors')
 const HttpErrorHandler = require('../Errors/HttpErrorHandler')
+const { expressify } = require('../../util/promises')
 
-function add(req, res, next) {
+async function add(req, res, next) {
   const userId = AuthenticationController.getLoggedInUserId(req)
   const email = EmailHelper.parseEmail(req.body.email)
   if (!email) {
     return res.sendStatus(422)
   }
+  const user = await UserGetter.promises.getUser(userId, { email: 1 })
 
   const affiliationOptions = {
     university: req.body.university,
     role: req.body.role,
     department: req.body.department
   }
-  UserUpdater.addEmailAddress(userId, email, affiliationOptions, function(
-    error
-  ) {
-    if (error) {
-      return UserEmailsController._handleEmailError(error, req, res, next)
-    }
-    UserEmailsConfirmationHandler.sendConfirmationEmail(userId, email, function(
-      error
-    ) {
-      if (error) {
-        return next(error)
-      }
-      res.sendStatus(204)
-    })
-  })
+
+  try {
+    await UserUpdater.promises.addEmailAddress(
+      userId,
+      email,
+      affiliationOptions
+    )
+  } catch (error) {
+    return UserEmailsController._handleEmailError(error, req, res, next)
+  }
+
+  const emailOptions = {
+    to: user.email,
+    actionDescribed: `a secondary email address has been added to your account ${
+      user.email
+    }`,
+    message: [
+      `<span style="display:inline-block;padding: 0 20px;width:100%;">Added: <br/><b>${email}</b></span>`
+    ],
+    action: 'secondary email address added'
+  }
+  await EmailHandler.promises.sendEmail('securityAlert', emailOptions)
+  await UserEmailsConfirmationHandler.promises.sendConfirmationEmail(
+    userId,
+    email
+  )
+
+  res.sendStatus(204)
 }
 
 function resendConfirmation(req, res, next) {
@@ -61,7 +76,7 @@ function resendConfirmation(req, res, next) {
   })
 }
 
-module.exports = UserEmailsController = {
+const UserEmailsController = {
   list(req, res, next) {
     const userId = AuthenticationController.getLoggedInUserId(req)
     UserGetter.getUserFullEmails(userId, function(error, fullEmails) {
@@ -72,7 +87,7 @@ module.exports = UserEmailsController = {
     })
   },
 
-  add,
+  add: expressify(add),
 
   remove(req, res, next) {
     const userId = AuthenticationController.getLoggedInUserId(req)
@@ -169,3 +184,9 @@ module.exports = UserEmailsController = {
     next(error)
   }
 }
+
+UserEmailsController.promises = {
+  add
+}
+
+module.exports = UserEmailsController

services/web/test/unit/src/Email/EmailBuilderTests.js

@@ -104,13 +104,25 @@ describe('EmailBuilder', function() {
   describe('templates', function() {
     describe('securityAlert', function() {
       before(function() {
-        this.email = 'example@overleaf.com'
+        this.message = 'more details about the action'
+        this.messageHTML = `<br /><span style="text-align:center" class="a-class"><b><i>${
+          this.message
+        }</i></b></span>`
+        this.messageNotAllowedHTML = `<div></div>${this.messageHTML}`
+
+        this.actionDescribed = 'an action described'
+        this.actionDescribedHTML = `<br /><span style="text-align:center" class="a-class"><b><i>${
+          this.actionDescribed
+        }</i></b>`
+        this.actionDescribedNotAllowedHTML = `<div></div>${
+          this.actionDescribedHTML
+        }`
+
         this.opts = {
           to: this.email,
-          actionDescribed: `an Institutional SSO account at Overleaf University was linked to your account ${
-            this.email
-          }`,
-          action: 'institutional SSO account linked'
+          actionDescribed: this.actionDescribedNotAllowedHTML,
+          action: 'an action',
+          message: [this.messageNotAllowedHTML]
         }
         this.email = this.EmailBuilder.buildEmail('securityAlert', this.opts)
       })
@@ -119,6 +131,30 @@ describe('EmailBuilder', function() {
         expect(this.email.html != null).to.equal(true)
         expect(this.email.text != null).to.equal(true)
       })
+
+      describe('HTML email', function() {
+        it('should clean HTML in opts.actionDescribed', function() {
+          expect(this.email.html).to.not.contain(
+            this.actionDescribedNotAllowedHTML
+          )
+          expect(this.email.html).to.contain(this.actionDescribedHTML)
+        })
+        it('should clean HTML in opts.message', function() {
+          expect(this.email.html).to.not.contain(this.messageNotAllowedHTML)
+          expect(this.email.html).to.contain(this.messageHTML)
+        })
+      })
+
+      describe('plain text email', function() {
+        it('should remove all HTML in opts.actionDescribed', function() {
+          expect(this.email.text).to.not.contain(this.actionDescribedHTML)
+          expect(this.email.text).to.contain(this.actionDescribed)
+        })
+        it('should remove all HTML in opts.message', function() {
+          expect(this.email.text).to.not.contain(this.messageHTML)
+          expect(this.email.text).to.contain(this.message)
+        })
+      })
     })
   })
 })

services/web/test/unit/src/Email/EmailMessageHelperTests.js

@@ -0,0 +1,39 @@
+const SandboxedModule = require('sandboxed-module')
+const path = require('path')
+const { expect } = require('chai')
+
+const MODULE_PATH = path.join(
+  __dirname,
+  '../../../../app/src/Features/Email/EmailMessageHelper'
+)
+
+describe('EmailMessageHelper', function() {
+  beforeEach(function() {
+    this.EmailMessageHelper = SandboxedModule.require(MODULE_PATH, {
+      globals: {
+        console: console
+      }
+    })
+  })
+  describe('cleanHTML', function() {
+    beforeEach(function() {
+      this.text = 'a message'
+      this.span = `<span style="text-align:center">${this.text}</span>`
+      this.fullMessage = `${this.span}<div></div>`
+    })
+    it('should remove HTML for plainText version', function() {
+      const processed = this.EmailMessageHelper.cleanHTML(
+        this.fullMessage,
+        true
+      )
+      expect(processed).to.equal(this.text)
+    })
+    it('should keep HTML for HTML version but remove tags not allowed', function() {
+      const processed = this.EmailMessageHelper.cleanHTML(
+        this.fullMessage,
+        false
+      )
+      expect(processed).to.equal(this.span)
+    })
+  })
+})

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

@@ -13,9 +13,15 @@ describe('UserEmailsController', function() {
   beforeEach(function() {
     this.req = new MockRequest()
     this.res = new MockResponse()
-    this.user = { _id: 'mock-user-id' }
+    this.next = sinon.stub()
+    this.user = { _id: 'mock-user-id', email: 'example@overleaf.com' }
 
-    this.UserGetter = { getUserFullEmails: sinon.stub() }
+    this.UserGetter = {
+      getUserFullEmails: sinon.stub(),
+      promises: {
+        getUser: sinon.stub().resolves(this.user)
+      }
+    }
     this.AuthenticationController = {
       getLoggedInUserId: sinon.stub().returns(this.user._id),
       setInSessionUser: sinon.stub()
@@ -27,7 +33,10 @@ describe('UserEmailsController', function() {
       addEmailAddress: sinon.stub(),
       removeEmailAddress: sinon.stub(),
       setDefaultEmailAddress: sinon.stub(),
-      updateV1AndSetDefaultEmailAddress: sinon.stub()
+      updateV1AndSetDefaultEmailAddress: sinon.stub(),
+      promises: {
+        addEmailAddress: sinon.stub().resolves()
+      }
     }
     this.EmailHelper = { parseEmail: sinon.stub() }
     this.endorseAffiliation = sinon.stub().yields()
@@ -51,8 +60,17 @@ describe('UserEmailsController', function() {
         '../../infrastructure/Features': this.Features,
         './UserGetter': this.UserGetter,
         './UserUpdater': this.UserUpdater,
+        '../Email/EmailHandler': (this.EmailHandler = {
+          promises: {
+            sendEmail: sinon.stub().resolves()
+          }
+        }),
         '../Helpers/EmailHelper': this.EmailHelper,
-        './UserEmailsConfirmationHandler': (this.UserEmailsConfirmationHandler = {}),
+        './UserEmailsConfirmationHandler': (this.UserEmailsConfirmationHandler = {
+          promises: {
+            sendConfirmationEmail: sinon.stub().resolves()
+          }
+        }),
         '../Institutions/InstitutionsAPI': this.InstitutionsAPI,
         '../Errors/HttpErrorHandler': this.HttpErrorHandler,
         '../Errors/Errors': Errors,
@@ -96,69 +114,94 @@ describe('UserEmailsController', function() {
       this.UserEmailsConfirmationHandler.sendConfirmationEmail = sinon
         .stub()
         .yields()
-      this.UserUpdater.addEmailAddress.callsArgWith(3, null)
     })
 
     it('adds new email', function(done) {
-      this.UserEmailsController.add(this.req, {
-        sendStatus: code => {
-          code.should.equal(204)
-          assertCalledWith(this.EmailHelper.parseEmail, this.newEmail)
-          assertCalledWith(
-            this.UserUpdater.addEmailAddress,
-            this.user._id,
-            this.newEmail
-          )
+      this.UserEmailsController.add(
+        this.req,
+        {
+          sendStatus: code => {
+            code.should.equal(204)
+            assertCalledWith(this.EmailHelper.parseEmail, this.newEmail)
+            assertCalledWith(
+              this.UserUpdater.promises.addEmailAddress,
+              this.user._id,
+              this.newEmail
+            )
 
-          const affiliationOptions = this.UserUpdater.addEmailAddress.lastCall
-            .args[2]
-          Object.keys(affiliationOptions).length.should.equal(3)
-          affiliationOptions.university.should.equal(this.req.body.university)
-          affiliationOptions.department.should.equal(this.req.body.department)
-          affiliationOptions.role.should.equal(this.req.body.role)
+            const affiliationOptions = this.UserUpdater.promises.addEmailAddress
+              .lastCall.args[2]
+            Object.keys(affiliationOptions).length.should.equal(3)
+            affiliationOptions.university.should.equal(this.req.body.university)
+            affiliationOptions.department.should.equal(this.req.body.department)
+            affiliationOptions.role.should.equal(this.req.body.role)
 
-          done()
-        }
-      })
+            done()
+          }
+        },
+        this.next
+      )
+    })
+
+    it('sends a security alert email', async function() {
+      await this.UserEmailsController.promises.add(
+        this.req,
+        this.res,
+        this.next
+      )
+      const emailCall = this.EmailHandler.promises.sendEmail.getCall(0)
+      emailCall.args[0].should.to.equal('securityAlert')
+      emailCall.args[1].to.should.equal(this.user.email)
+      emailCall.args[1].actionDescribed.should.contain(
+        'a secondary email address'
+      )
+      emailCall.args[1].to.should.equal(this.user.email)
+      emailCall.args[1].message[0].should.contain(this.newEmail)
     })
 
     it('sends an email confirmation', function(done) {
-      this.UserEmailsController.add(this.req, {
-        sendStatus: code => {
-          code.should.equal(204)
-          assertCalledWith(
-            this.UserEmailsConfirmationHandler.sendConfirmationEmail,
-            this.user._id,
-            this.newEmail
-          )
-          done()
-        }
-      })
+      this.UserEmailsController.add(
+        this.req,
+        {
+          sendStatus: code => {
+            code.should.equal(204)
+            assertCalledWith(
+              this.UserEmailsConfirmationHandler.promises.sendConfirmationEmail,
+              this.user._id,
+              this.newEmail
+            )
+            done()
+          }
+        },
+        this.next
+      )
     })
 
     it('handles email parse error', function(done) {
       this.EmailHelper.parseEmail.returns(null)
-      this.UserEmailsController.add(this.req, {
-        sendStatus: code => {
-          code.should.equal(422)
-          assertNotCalled(this.UserUpdater.addEmailAddress)
-          done()
-        }
-      })
+      this.UserEmailsController.add(
+        this.req,
+        {
+          sendStatus: code => {
+            code.should.equal(422)
+            assertNotCalled(this.UserUpdater.promises.addEmailAddress)
+            done()
+          }
+        },
+        this.next
+      )
     })
 
     it('should pass the error to the next handler when adding the email fails', function(done) {
-      this.UserUpdater.addEmailAddress.callsArgWith(3, new Error())
-      this.next = sinon.spy(error => {
-        expect(error).instanceOf(Error)
+      this.UserUpdater.promises.addEmailAddress.rejects(new Error())
+      this.UserEmailsController.add(this.req, this.res, error => {
+        expect(error).to.be.instanceof(Error)
         done()
       })
-      this.UserEmailsController.add(this.req, this.res, this.next)
     })
 
     it('should call the HTTP conflict handler when the email already exists', function(done) {
-      this.UserUpdater.addEmailAddress.callsArgWith(
-        3,
+      this.UserUpdater.promises.addEmailAddress.rejects(
         new Errors.EmailExistsError()
       )
       this.HttpErrorHandler.conflict = sinon.spy((req, res, message) => {
@@ -167,12 +210,11 @@ describe('UserEmailsController', function() {
         message.should.equal('email_already_registered')
         done()
       })
-      this.UserEmailsController.add(this.req, this.res)
+      this.UserEmailsController.add(this.req, this.res, this.next)
     })
 
     it("should call the HTTP conflict handler when there's a domain matching error", function(done) {
-      this.UserUpdater.addEmailAddress.callsArgWith(
-        3,
+      this.UserUpdater.promises.addEmailAddress.rejects(
         new Error('422: Email does not belong to university')
       )
       this.HttpErrorHandler.conflict = sinon.spy((req, res, message) => {
@@ -181,7 +223,7 @@ describe('UserEmailsController', function() {
         message.should.equal('email_does_not_belong_to_university')
         done()
       })
-      this.UserEmailsController.add(this.req, this.res)
+      this.UserEmailsController.add(this.req, this.res, this.next)
     })
   })