diff --git a/services/web/app/src/Features/Email/EmailBuilder.js b/services/web/app/src/Features/Email/EmailBuilder.js index 8192debdc4..afb8a9ab08 100644 --- a/services/web/app/src/Features/Email/EmailBuilder.js +++ b/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 diff --git a/services/web/app/src/Features/Email/EmailMessageHelper.js b/services/web/app/src/Features/Email/EmailMessageHelper.js index f0cccacde7..9b608c3d27 100644 --- a/services/web/app/src/Features/Email/EmailMessageHelper.js +++ b/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})` : `${text}` } module.exports = { + cleanHTML, displayLink } diff --git a/services/web/app/src/Features/User/UserEmailsConfirmationHandler.js b/services/web/app/src/Features/User/UserEmailsConfirmationHandler.js index 3f6c38dd23..e69eff4fc6 100644 --- a/services/web/app/src/Features/User/UserEmailsConfirmationHandler.js +++ b/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 diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index b278985247..ef9933f85a 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/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: [ + `Added:
${email}` + ], + 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 diff --git a/services/web/test/unit/src/Email/EmailBuilderTests.js b/services/web/test/unit/src/Email/EmailBuilderTests.js index 004472b5b8..e3c64c4d0a 100644 --- a/services/web/test/unit/src/Email/EmailBuilderTests.js +++ b/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 = `
${ + this.message + }` + this.messageNotAllowedHTML = `
${this.messageHTML}` + + this.actionDescribed = 'an action described' + this.actionDescribedHTML = `
${ + this.actionDescribed + }` + this.actionDescribedNotAllowedHTML = `
${ + 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) + }) + }) }) }) }) diff --git a/services/web/test/unit/src/Email/EmailMessageHelperTests.js b/services/web/test/unit/src/Email/EmailMessageHelperTests.js new file mode 100644 index 0000000000..1a653b68ce --- /dev/null +++ b/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 = `${this.text}` + this.fullMessage = `${this.span}
` + }) + 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) + }) + }) +}) diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index 75affe0e19..acaaae410e 100644 --- a/services/web/test/unit/src/User/UserEmailsControllerTests.js +++ b/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) }) })