Merge pull request #7293 from overleaf/td-email-change-notifications

Send primary email address change notification to latest confirmed addresses

GitOrigin-RevId: ba4aba38a2d8785ee24156449c612ff05cd66fc7

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

@@ -288,6 +288,11 @@ const decorateFullEmails = (
 
     emailData.emailHasInstitutionLicence =
       InstitutionsHelper.emailHasLicence(emailData)
+
+    const lastConfirmedAtStr = emailData.reconfirmedAt || emailData.confirmedAt
+    emailData.lastConfirmedAt = lastConfirmedAtStr
+      ? moment(lastConfirmedAtStr).toDate()
+      : null
   })
 
   return emailsData

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

@@ -19,25 +19,51 @@ const NewsletterManager = require('../Newsletter/NewsletterManager')
 const RecurlyWrapper = require('../Subscription/RecurlyWrapper')
 const UserAuditLogHandler = require('./UserAuditLogHandler')
 const AnalyticsManager = require('../Analytics/AnalyticsManager')
+const _ = require('lodash')
 
 async function _sendSecurityAlertPrimaryEmailChanged(userId, oldEmail, email) {
-  // send email to both old and new primary email
+  // Send email to the following:
+  // - the old primary
+  // - the new primary
+  // - for all other current (confirmed or recently-enough reconfirmed) email addresses, group by institution if we
+  //   have it, or domain if we don’t, and for each group send to the most recently reconfirmed (or confirmed if never
+  //   reconfirmed) address in that group.
+  // See #6101.
   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 sendToRecipients(recipients) {
+    // On failure, log the error and carry on so that one email failing does not prevent other emails sending
+    for await (const recipient of recipients) {
+      try {
+        const opts = Object.assign({}, emailOptions, { to: recipient })
+        await EmailHandler.promises.sendEmail('securityAlert', opts)
+      } catch (error) {
+        logger.error(
+          { error, userId },
+          'could not send security alert email when primary email changed'
+        )
+      }
+    }
   }
+
+  // First, send notification to the old and new primary emails before getting other emails from v1 to ensure that these
+  // are still sent in the event of not being able to reach v1
+  const oldAndNewPrimaryEmails = [oldEmail, email]
+  await sendToRecipients(oldAndNewPrimaryEmails)
+
+  // Next, get extra recipients with affiliation data
+  const emailsData = await UserGetter.promises.getUserFullEmails(userId)
+  const extraRecipients =
+    UserUpdater.securityAlertPrimaryEmailChangedExtraRecipients(
+      emailsData,
+      oldEmail,
+      email
+    )
+
+  await sendToRecipients(extraRecipients)
 }
 
 async function addEmailAddress(userId, newEmail, affiliationOptions, auditLog) {
@@ -389,6 +415,46 @@ const UserUpdater = {
       error => callback(error)
     )
   },
+
+  securityAlertPrimaryEmailChangedExtraRecipients(emailsData, oldEmail, email) {
+    // Group by institution if we have it, or domain if we don’t, and for each group send to the most recently
+    // reconfirmed (or confirmed if never reconfirmed) address in that group. We also remove the original and new
+    // primary email addresses because they are emailed separately
+    // See #6101.
+    function sortEmailsByConfirmation(emails) {
+      return emails.sort((e1, e2) => e2.lastConfirmedAt - e1.lastConfirmedAt)
+    }
+
+    const recipients = new Set()
+    const emailsToIgnore = new Set([oldEmail, email])
+
+    // Remove non-confirmed emails
+    const confirmedEmails = emailsData.filter(email => !!email.lastConfirmedAt)
+
+    // Group other emails by institution, separating out those with no institution and grouping them instead by domain.
+    // The keys for each group are not used for anything other than the grouping, so can have a slightly paranoid format
+    // to avoid any potential clash
+    const groupedEmails = _.groupBy(confirmedEmails, emailData => {
+      if (!emailData.affiliation || !emailData.affiliation.institution) {
+        return `domain:${EmailHelper.getDomain(emailData.email)}`
+      }
+      return `institution_id:${emailData.affiliation.institution.id}`
+    })
+
+    // For each group of emails, order the emails by (re-)confirmation date and pick the first
+    for (const emails of Object.values(groupedEmails)) {
+      // Sort by confirmation and pick the first
+      sortEmailsByConfirmation(emails)
+
+      // Ignore original and new primary email addresses
+      const recipient = emails[0].email
+      if (!emailsToIgnore.has(recipient)) {
+        recipients.add(emails[0].email)
+      }
+    }
+
+    return Array.from(recipients)
+  },
 }
 ;[
   'updateUser',

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

@@ -17,6 +17,7 @@ const {
 
 describe('UserGetter', function () {
   beforeEach(function () {
+    const confirmedAt = new Date()
     this.fakeUser = {
       _id: '12390i',
       email: 'email2@foo.bar',
@@ -24,7 +25,8 @@ describe('UserGetter', function () {
         {
           email: 'email1@foo.bar',
           reversedHostname: 'rab.oof',
-          confirmedAt: new Date(),
+          confirmedAt: confirmedAt,
+          lastConfirmedAt: confirmedAt,
         },
         { email: 'email2@foo.bar', reversedHostname: 'rab.oof' },
       ],
@@ -152,6 +154,7 @@ describe('UserGetter', function () {
               email: 'email1@foo.bar',
               reversedHostname: 'rab.oof',
               confirmedAt: this.fakeUser.emails[0].confirmedAt,
+              lastConfirmedAt: this.fakeUser.emails[0].lastConfirmedAt,
               emailHasInstitutionLicence: false,
               default: false,
             },
@@ -160,6 +163,7 @@ describe('UserGetter', function () {
               reversedHostname: 'rab.oof',
               emailHasInstitutionLicence: false,
               default: true,
+              lastConfirmedAt: null,
             },
           ])
           done()
@@ -199,6 +203,7 @@ describe('UserGetter', function () {
               email: 'email1@foo.bar',
               reversedHostname: 'rab.oof',
               confirmedAt: this.fakeUser.emails[0].confirmedAt,
+              lastConfirmedAt: this.fakeUser.emails[0].lastConfirmedAt,
               default: false,
               emailHasInstitutionLicence: true,
               affiliation: {
@@ -223,6 +228,7 @@ describe('UserGetter', function () {
               reversedHostname: 'rab.oof',
               emailHasInstitutionLicence: false,
               default: true,
+              lastConfirmedAt: null,
             },
           ])
           done()
@@ -248,6 +254,7 @@ describe('UserGetter', function () {
               email: 'email1@foo.bar',
               reversedHostname: 'rab.oof',
               confirmedAt: this.fakeUser.emails[0].confirmedAt,
+              lastConfirmedAt: this.fakeUser.emails[0].lastConfirmedAt,
               default: false,
               emailHasInstitutionLicence: false,
               samlProviderId: 'saml_id',
@@ -258,6 +265,7 @@ describe('UserGetter', function () {
               reversedHostname: 'rab.oof',
               emailHasInstitutionLicence: false,
               default: true,
+              lastConfirmedAt: null,
             },
           ])
           done()

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

@@ -25,6 +25,7 @@ describe('UserUpdater', function () {
         ensureUniqueEmailAddress: sinon.stub(),
         getUser: sinon.stub(),
         getUserByMainEmail: sinon.stub(),
+        getUserFullEmails: sinon.stub(),
       },
     }
     this.addAffiliation = sinon.stub().yields()
@@ -522,17 +523,24 @@ describe('UserUpdater', function () {
   })
 
   describe('setDefaultEmailAddress', function () {
+    function setStubbedUserEmails(test, emails) {
+      test.stubbedUser.emails = emails
+      test.UserGetter.promises.getUserFullEmails.resolves(
+        test.stubbedUser.emails
+      )
+    }
+
     beforeEach(function () {
       this.auditLog = {
         initiatorId: this.stubbedUser,
         ipAddress: '0:0:0:0',
       }
-      this.stubbedUser.emails = [
+      setStubbedUserEmails(this, [
         {
           email: this.newEmail,
           confirmedAt: new Date(),
         },
-      ]
+      ])
       this.UserGetter.promises.getUser.resolves(this.stubbedUser)
       this.NewsletterManager.promises.changeEmail.callsArgWith(2, null)
       this.RecurlyWrapper.promises.updateAccountEmailAddress.resolves()
@@ -681,12 +689,12 @@ describe('UserUpdater', function () {
 
     describe('when email not confirmed', function () {
       beforeEach(function () {
-        this.stubbedUser.emails = [
+        setStubbedUserEmails(this, [
           {
             email: this.newEmail,
             confirmedAt: null,
           },
-        ]
+        ])
         this.UserUpdater.promises.updateUser = sinon.stub()
       })
 
@@ -708,9 +716,142 @@ describe('UserUpdater', function () {
       })
     })
 
+    describe('securityAlertPrimaryEmailChangedExtraRecipients', function () {
+      it('should be empty for unaffiliated user with single email', function () {
+        const recipients =
+          this.UserUpdater.securityAlertPrimaryEmailChangedExtraRecipients(
+            this.stubbedUser.emails,
+            this.stubbedUser.email,
+            this.newEmail
+          )
+        expect(recipients).to.have.same.members([])
+      })
+
+      it('should be most recently (re-)confirmed emails grouped by institution and by domain for unaffiliated emails as recipients', function () {
+        setStubbedUserEmails(this, [
+          {
+            email: '1@a1.uni',
+            confirmedAt: new Date(2020, 0, 1),
+            reConfirmedAt: new Date(2021, 2, 11),
+            lastConfirmedAt: new Date(2021, 2, 11),
+            default: false,
+            affiliation: {
+              institution: {
+                id: 123,
+                name: 'A1 University',
+              },
+              cachedConfirmedAt: '2020-01-01T18:25:01.639Z',
+              cachedReconfirmedAt: '2021-03-11T18:25:01.639Z',
+            },
+          },
+          {
+            email: '2@a1.uni',
+            confirmedAt: new Date(2019, 0, 1),
+            reConfirmedAt: new Date(2022, 2, 11),
+            lastConfirmedAt: new Date(2022, 2, 11),
+            default: false,
+            affiliation: {
+              institution: {
+                id: 123,
+                name: 'A1 University',
+              },
+              cachedConfirmedAt: '2019-01-01T18:25:01.639Z',
+              cachedReconfirmedAt: '2022-03-11T18:25:01.639Z',
+            },
+          },
+          {
+            email: '2020@foo.bar',
+            confirmedAt: new Date(2020, 6, 1),
+            lastConfirmedAt: new Date(2020, 6, 1),
+          },
+          {
+            email: '2021@foo.bar',
+            confirmedAt: new Date(2021, 6, 1),
+            lastConfirmedAt: new Date(2021, 6, 1),
+          },
+          {
+            email: this.stubbedUser.email,
+            confirmedAt: new Date(2021, 6, 1),
+            lastConfirmedAt: new Date(2021, 6, 1),
+          },
+        ])
+
+        const recipients =
+          this.UserUpdater.securityAlertPrimaryEmailChangedExtraRecipients(
+            this.stubbedUser.emails,
+            this.stubbedUser.email,
+            this.newEmail
+          )
+        expect(recipients).to.have.same.members(['2@a1.uni', '2021@foo.bar'])
+      })
+
+      it('should be most recently (re-)confirmed emails grouped by institution and by domain for unaffiliated emails as recipients (multiple institutions and unaffiliated email domains)', function () {
+        setStubbedUserEmails(this, [
+          {
+            email: '1@a1.uni',
+            confirmedAt: new Date(2020, 0, 1),
+            reConfirmedAt: new Date(2021, 2, 11),
+            lastConfirmedAt: new Date(2021, 2, 11),
+            default: false,
+            affiliation: {
+              institution: {
+                id: 123,
+                name: 'A1 University',
+              },
+              cachedConfirmedAt: '2020-01-01T18:25:01.639Z',
+              cachedReconfirmedAt: '2021-03-11T18:25:01.639Z',
+            },
+          },
+          {
+            email: '1@b2.uni',
+            confirmedAt: new Date(2019, 0, 1),
+            reConfirmedAt: new Date(2022, 2, 11),
+            lastConfirmedAt: new Date(2022, 2, 11),
+            default: false,
+            affiliation: {
+              institution: {
+                id: 234,
+                name: 'B2 University',
+              },
+              cachedConfirmedAt: '2019-01-01T18:25:01.639Z',
+              cachedReconfirmedAt: '2022-03-11T18:25:01.639Z',
+            },
+          },
+          {
+            email: '2020@foo.bar',
+            confirmedAt: new Date(2020, 6, 1),
+            lastConfirmedAt: new Date(2020, 6, 1),
+          },
+          {
+            email: '2021@bar.foo',
+            confirmedAt: new Date(2021, 6, 1),
+            lastConfirmedAt: new Date(2021, 6, 1),
+          },
+          {
+            email: this.stubbedUser.email,
+            confirmedAt: new Date(2021, 6, 1),
+            lastConfirmedAt: new Date(2021, 6, 1),
+          },
+        ])
+
+        const recipients =
+          this.UserUpdater.securityAlertPrimaryEmailChangedExtraRecipients(
+            this.stubbedUser.emails,
+            this.stubbedUser.email,
+            this.newEmail
+          )
+        expect(recipients).to.have.same.members([
+          '1@a1.uni',
+          '1@b2.uni',
+          '2020@foo.bar',
+          '2021@bar.foo',
+        ])
+      })
+    })
+
     describe('when email does not belong to user', function () {
       beforeEach(function () {
-        this.stubbedUser.emails = []
+        setStubbedUserEmails(this, [])
         this.UserGetter.promises.getUser.resolves(this.stubbedUser)
         this.UserUpdater.promises.updateUser = sinon.stub()
       })