diff --git a/services/web/app/src/Features/User/UserGetter.js b/services/web/app/src/Features/User/UserGetter.js index 3b211278e6..de3ec8c8f1 100644 --- a/services/web/app/src/Features/User/UserGetter.js +++ b/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 diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index cd06e42521..23a09aee8a 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/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', diff --git a/services/web/test/unit/src/User/UserGetterTests.js b/services/web/test/unit/src/User/UserGetterTests.js index bbe237e75b..fecb8731b2 100644 --- a/services/web/test/unit/src/User/UserGetterTests.js +++ b/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() diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index f94ac14b03..f1a0366cfd 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/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() })