From d8b2537f485d5c9c8ea2dcbeb994acc912560c89 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Tue, 21 Jul 2020 09:28:52 -0500 Subject: [PATCH] Merge pull request #2983 from overleaf/jel-security-emails Add security email template and switch institution SSO alerts to use it GitOrigin-RevId: c6f07655165e352527a9efbcfffc5bd2f635405c --- .../app/src/Features/Email/EmailBuilder.js | 65 ++++++++--------- .../src/Features/User/SAMLIdentityManager.js | 32 ++++---- .../User/ThirdPartyIdentityManager.js | 48 +++++++----- .../test/unit/src/Email/EmailBuilderTests.js | 24 +++++- .../unit/src/User/SAMLIdentityManagerTests.js | 28 ++++++- .../User/ThirdPartyIdentityManagerTests.js | 73 +++++++++++++++++++ 6 files changed, 194 insertions(+), 76 deletions(-) create mode 100644 services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js diff --git a/services/web/app/src/Features/Email/EmailBuilder.js b/services/web/app/src/Features/Email/EmailBuilder.js index 7ce3947817..8192debdc4 100644 --- a/services/web/app/src/Features/Email/EmailBuilder.js +++ b/services/web/app/src/Features/Email/EmailBuilder.js @@ -1,6 +1,7 @@ const _ = require('underscore') const settings = require('settings-sharelatex') const marked = require('marked') +const moment = require('moment') const EmailMessageHelper = require('./EmailMessageHelper') const StringHelper = require('../Helpers/StringHelper') const BaseWithHeaderEmailLayout = require(`./Layouts/BaseWithHeaderEmailLayout`) @@ -468,40 +469,6 @@ If you have any questions, you can contact our support team by reply.\ } }) -templates.emailThirdPartyIdentifierLinked = NoCTAEmailTemplate({ - subject(opts) { - return `Your ${settings.appName} account is now linked with ${ - opts.provider - }` - }, - title(opts) { - return `Accounts Linked` - }, - message(opts) { - let message = `We're contacting you to notify you that your ${ - opts.provider - } account is now linked to your ${settings.appName} account.` - return [message] - } -}) - -templates.emailThirdPartyIdentifierUnlinked = NoCTAEmailTemplate({ - subject(opts) { - return `Your ${settings.appName} account is no longer linked with ${ - opts.provider - }` - }, - title(opts) { - return `Accounts No Longer Linked` - }, - message(opts) { - let message = `We're contacting you to notify you that your ${ - opts.provider - } account is no longer linked with your ${settings.appName} account.` - return [message] - } -}) - templates.ownershipTransferConfirmationPreviousOwner = NoCTAEmailTemplate({ subject(opts) { return `Project ownership transfer - ${settings.appName}` @@ -621,6 +588,36 @@ templates.userOnboardingEmail = NoCTAEmailTemplate({ } }) +templates.securityAlert = NoCTAEmailTemplate({ + subject(opts) { + return `Overleaf security note: ${opts.action}` + }, + title(opts) { + return opts.action.charAt(0).toUpperCase() + opts.action.slice(1) + }, + message(opts, isPlainText) { + const dateFormatted = moment().format('dddd D MMMM YYYY') + const timeFormatted = moment().format('HH:mm') + const helpLink = EmailMessageHelper.displayLink( + 'quick guide', + `${settings.siteUrl}/learn/how-to/Keeping_your_account_secure`, + isPlainText + ) + return [ + `We are writing to let you know that ${ + opts.actionDescribed + } on ${dateFormatted} at ${timeFormatted} GMT.`, + `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 + } to report this as potentially suspicious activity on your account.`, + `We also encourage you to read our ${helpLink} to keeping your ${ + settings.appName + } account safe.` + ] + } +}) + function _formatUserNameAndEmail(user, placeholder) { if (user.first_name && user.last_name) { const fullName = `${user.first_name} ${user.last_name}` diff --git a/services/web/app/src/Features/User/SAMLIdentityManager.js b/services/web/app/src/Features/User/SAMLIdentityManager.js index dc24b2e950..ecda8ee8f1 100644 --- a/services/web/app/src/Features/User/SAMLIdentityManager.js +++ b/services/web/app/src/Features/User/SAMLIdentityManager.js @@ -108,33 +108,29 @@ async function _sendLinkedEmail(userId, providerName) { const user = await UserGetter.promises.getUser(userId, { email: 1 }) const emailOptions = { to: user.email, - provider: providerName + actionDescribed: `an Institutional SSO account at ${providerName} was linked to your account ${ + user.email + }`, + action: 'institutional SSO account linked' } - EmailHandler.sendEmail( - 'emailThirdPartyIdentifierLinked', - emailOptions, - error => { - if (error != null) { - logger.warn(error) - } + EmailHandler.sendEmail('securityAlert', emailOptions, error => { + if (error) { + logger.warn({ err: error }) } - ) + }) } function _sendUnlinkedEmail(primaryEmail, providerName) { const emailOptions = { to: primaryEmail, - provider: providerName + actionDescribed: `an Institutional SSO account at ${providerName} is no longer linked to your account ${primaryEmail}`, + action: 'institutional SSO account no longer linked' } - EmailHandler.sendEmail( - 'emailThirdPartyIdentifierUnlinked', - emailOptions, - error => { - if (error != null) { - logger.warn(error) - } + EmailHandler.sendEmail('securityAlert', emailOptions, error => { + if (error) { + logger.warn({ err: error }) } - ) + }) } async function getUser(providerId, externalUserId) { diff --git a/services/web/app/src/Features/User/ThirdPartyIdentityManager.js b/services/web/app/src/Features/User/ThirdPartyIdentityManager.js index 8cf0c043f9..d3514597ab 100644 --- a/services/web/app/src/Features/User/ThirdPartyIdentityManager.js +++ b/services/web/app/src/Features/User/ThirdPartyIdentityManager.js @@ -9,6 +9,12 @@ const { promisifyAll } = require(`${APP_ROOT}/util/promises`) const oauthProviders = settings.oauthProviders || {} +function _getIndefiniteArticle(providerName) { + const vowels = ['a', 'e', 'i', 'o', 'u'] + if (vowels.includes(providerName.charAt(0).toLowerCase())) return 'an' + return 'a' +} + function getUser(providerId, externalUserId, callback) { if (providerId == null || externalUserId == null) { return callback(new Error('invalid arguments')) @@ -81,20 +87,21 @@ 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, - provider: oauthProviders[providerId].name + action: `${providerName} account linked`, + actionDescribed: `${indefiniteArticle} ${providerName} account was linked to your account ${ + res.email + }` } - EmailHandler.sendEmail( - 'emailThirdPartyIdentifierLinked', - emailOptions, - error => { - if (error != null) { - logger.warn(error) - } - return callback(null, res) + EmailHandler.sendEmail('securityAlert', emailOptions, error => { + if (error != null) { + logger.warn(error) } - ) + return callback(null, res) + }) } else if (retry) { // if already retried then throw error callback(new Error('update failed')) @@ -138,20 +145,21 @@ 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, - provider: oauthProviders[providerId].name + action: `${providerName} account no longer linked`, + actionDescribed: `${indefiniteArticle} ${providerName} account is no longer linked to your account ${ + res.email + }` } - EmailHandler.sendEmail( - 'emailThirdPartyIdentifierUnlinked', - emailOptions, - error => { - if (error != null) { - logger.warn(error) - } - return callback(null, res) + EmailHandler.sendEmail('securityAlert', emailOptions, error => { + if (error != null) { + logger.warn(error) } - ) + return callback(null, res) + }) } }) } diff --git a/services/web/test/unit/src/Email/EmailBuilderTests.js b/services/web/test/unit/src/Email/EmailBuilderTests.js index 0777271d1b..004472b5b8 100644 --- a/services/web/test/unit/src/Email/EmailBuilderTests.js +++ b/services/web/test/unit/src/Email/EmailBuilderTests.js @@ -13,7 +13,8 @@ describe('EmailBuilder', function() { beforeEach(function() { this.settings = { appName: 'testApp', - brandPrefix: '' + brandPrefix: '', + siteUrl: 'https://www.overleaf.com' } this.EmailBuilder = SandboxedModule.require(MODULE_PATH, { globals: { @@ -99,4 +100,25 @@ describe('EmailBuilder', function() { this.email.subject.indexOf('New Project').should.not.equal(-1) }) }) + + describe('templates', function() { + describe('securityAlert', function() { + before(function() { + this.email = 'example@overleaf.com' + 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' + } + this.email = this.EmailBuilder.buildEmail('securityAlert', this.opts) + }) + + it('should build the email', function() { + expect(this.email.html != null).to.equal(true) + expect(this.email.text != null).to.equal(true) + }) + }) + }) }) diff --git a/services/web/test/unit/src/User/SAMLIdentityManagerTests.js b/services/web/test/unit/src/User/SAMLIdentityManagerTests.js index c17df26737..9d8756e57e 100644 --- a/services/web/test/unit/src/User/SAMLIdentityManagerTests.js +++ b/services/web/test/unit/src/User/SAMLIdentityManagerTests.js @@ -63,7 +63,9 @@ describe('SAMLIdentityManager', function() { }), '../../models/User': { User: (this.User = { - findOneAndUpdate: sinon.stub(), + findOneAndUpdate: sinon.stub().returns({ + exec: sinon.stub().resolves() + }), findOne: sinon.stub().returns({ exec: sinon.stub().resolves() }), @@ -80,7 +82,12 @@ describe('SAMLIdentityManager', function() { } }), '../User/UserUpdater': (this.UserUpdater = { - addEmailAddress: sinon.stub() + addEmailAddress: sinon.stub(), + promises: { + addEmailAddress: sinon.stub().resolves(), + confirmEmail: sinon.stub().resolves(), + updateUser: sinon.stub().resolves() + } }), '../Institutions/InstitutionsAPI': this.InstitutionsAPI, 'logger-sharelatex': this.logger @@ -164,10 +171,25 @@ describe('SAMLIdentityManager', function() { } }) }) + + describe('after linking', function() { + it('should send an email notification', function() { + this.SAMLIdentityManager.linkAccounts( + this.user._id, + this.user.email, + '1', + 'Overleaf University', + () => { + expect(this.User.update).to.have.been.called + expect(this.EmailHandler.sendEmail).to.have.been.called + } + ) + }) + }) }) describe('unlinkAccounts', function() { - it('should send an email notification email', function() { + it('should send an email notification', function() { this.SAMLIdentityManager.unlinkAccounts( this.user._id, this.user.email, diff --git a/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js b/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js new file mode 100644 index 0000000000..6dfa49d56e --- /dev/null +++ b/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js @@ -0,0 +1,73 @@ +const sinon = require('sinon') +const chai = require('chai') +const { expect } = chai +const SandboxedModule = require('sandboxed-module') +const modulePath = + '../../../../app/src/Features/User/ThirdPartyIdentityManager.js' + +describe('ThirdPartyIdentityManager', function() { + beforeEach(function() { + this.userId = 'a1b2c3' + this.user = { + _id: this.userId, + email: 'example@overleaf.com' + } + this.externalUserId = 'id789' + this.externalData = {} + this.ThirdPartyIdentityManager = SandboxedModule.require(modulePath, { + globals: { + console: console + }, + requires: { + '../../../../app/src/Features/Email/EmailHandler': (this.EmailHandler = { + sendEmail: sinon.stub().yields() + }), + Errors: (this.Errors = { + ThirdPartyIdentityExistsError: sinon.stub(), + ThirdPartyUserNotFoundError: sinon.stub() + }), + '../../../../app/src/models/User': { + User: (this.User = { + findOneAndUpdate: sinon.stub().yields(undefined, this.user), + findOne: sinon.stub() + }) + }, + 'settings-sharelatex': { + oauthProviders: { + google: { + name: 'Google' + }, + orcid: { + name: 'Orcid' + } + } + } + } + }) + }) + describe('link', function() { + it('should send email alert', async function() { + await this.ThirdPartyIdentityManager.promises.link( + this.userId, + 'google', + this.externalUserId, + this.externalData + ) + const emailCall = this.EmailHandler.sendEmail.getCall(0) + expect(emailCall.args[0]).to.equal('securityAlert') + expect(emailCall.args[1].actionDescribed).to.contain( + 'a Google account was linked' + ) + }) + }) + describe('unlink', function() { + it('should send email alert', async function() { + await this.ThirdPartyIdentityManager.promises.unlink(this.userId, 'orcid') + 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' + ) + }) + }) +})