diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 555d359d42..a8d4ed0a66 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -234,7 +234,11 @@ const AuthenticationController = (module.exports = { }, _loginAsyncHandlers(req, user) { - UserHandler.setupLoginData(user, function() {}) + UserHandler.setupLoginData(user, err => { + if (err != null) { + logger.warn({ err }, 'error setting up login data') + } + }) LoginRateLimiter.recordSuccessfulLogin(user.email) AuthenticationController._recordSuccessfulLogin(user._id) AuthenticationController.ipMatchCheck(req, user) diff --git a/services/web/app/src/Features/Email/Bodies/SingleCTAEmailBody.js b/services/web/app/src/Features/Email/Bodies/SingleCTAEmailBody.js index 296f21ce09..677ff32347 100644 --- a/services/web/app/src/Features/Email/Bodies/SingleCTAEmailBody.js +++ b/services/web/app/src/Features/Email/Bodies/SingleCTAEmailBody.js @@ -1,11 +1,4 @@ -/* eslint-disable - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. const _ = require('underscore') -const settings = require('settings-sharelatex') module.exports = _.template(`\ diff --git a/services/web/app/src/Features/Email/Bodies/sl-SingleCTAEmailBody.js b/services/web/app/src/Features/Email/Bodies/sl-SingleCTAEmailBody.js index 2a4e2aaa1e..3bc0382d80 100644 --- a/services/web/app/src/Features/Email/Bodies/sl-SingleCTAEmailBody.js +++ b/services/web/app/src/Features/Email/Bodies/sl-SingleCTAEmailBody.js @@ -1,11 +1,4 @@ -/* eslint-disable - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. const _ = require('underscore') -const settings = require('settings-sharelatex') module.exports = _.template(`\
@@ -15,7 +8,7 @@ module.exports = _.template(`\ <%= title %> <% } %> -
 
+
 
<% if (greeting) { %>

<%= greeting %> @@ -24,7 +17,7 @@ module.exports = _.template(`\

<%= message %>

-
 
+
 
diff --git a/services/web/app/src/Features/Email/EmailBuilder.js b/services/web/app/src/Features/Email/EmailBuilder.js index d61de36377..566eb6d65d 100644 --- a/services/web/app/src/Features/Email/EmailBuilder.js +++ b/services/web/app/src/Features/Email/EmailBuilder.js @@ -1,33 +1,17 @@ -/* eslint-disable - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const _ = require('underscore') const settings = require('settings-sharelatex') const marked = require('marked') const StringHelper = require('../Helpers/StringHelper') - -const PersonalEmailLayout = require('./Layouts/PersonalEmailLayout') -const NotificationEmailLayout = require('./Layouts/NotificationEmailLayout') const BaseWithHeaderEmailLayout = require(`./Layouts/${ settings.brandPrefix }BaseWithHeaderEmailLayout`) const SpamSafe = require('./SpamSafe') - -// Single CTA Email const SingleCTAEmailBody = require(`./Bodies/${ settings.brandPrefix }SingleCTAEmailBody`) +const NoCTAEmailBody = require(`./Bodies/NoCTAEmailBody`) -const CTAEmailTemplate = function(content) { +function CTAEmailTemplate(content) { if (content.greeting == null) { content.greeting = () => 'Hi,' } @@ -74,9 +58,7 @@ The ${settings.appName} Team - ${settings.siteUrl}\ } } -// No CTA Email -const NoCTAEmailBody = require(`./Bodies/NoCTAEmailBody`) -const NoCTAEmailTemplate = function(content) { +function NoCTAEmailTemplate(content) { if (content.greeting == null) { content.greeting = () => 'Hi,' } @@ -116,6 +98,24 @@ The ${settings.appName} Team - ${settings.siteUrl}\ } } +function buildEmail(templateName, opts) { + const template = templates[templateName] + opts.siteUrl = settings.siteUrl + opts.body = template.compiledTemplate(opts) + if ( + settings.email && + settings.email.template && + settings.email.template.customFooter + ) { + opts.body += settings.email.template.customFooter + } + return { + subject: template.subject(opts), + html: template.layout(opts), + text: template.plainTextTemplate && template.plainTextTemplate(opts) + } +} + const templates = {} templates.accountMergeToOverleafAddress = CTAEmailTemplate({ @@ -508,39 +508,82 @@ templates.emailThirdPartyIdentifierUnlinked = NoCTAEmailTemplate({ } }) +templates.ownershipTransferConfirmationPreviousOwner = NoCTAEmailTemplate({ + subject(opts) { + return `Project ownership transfer - ${settings.appName}` + }, + title(opts) { + const projectName = _.escape( + SpamSafe.safeProjectName(opts.project.name, 'Your project') + ) + return `${projectName} - Owner change` + }, + message(opts) { + const nameAndEmail = _.escape( + _formatUserNameAndEmail(opts.newOwner, 'a collaborator') + ) + const projectName = _.escape( + SpamSafe.safeProjectName(opts.project.name, 'your project') + ) + return `\ +As per your request, we have made ${nameAndEmail} the owner of ${projectName}. + +If you haven't asked to change the owner of ${projectName}, please get in touch +with us via ${settings.adminEmail}. +` + } +}) + +templates.ownershipTransferConfirmationNewOwner = CTAEmailTemplate({ + subject(opts) { + return `Project ownership transfer - ${settings.appName}` + }, + title(opts) { + const projectName = _.escape( + SpamSafe.safeProjectName(opts.project.name, 'Your project') + ) + return `${projectName} - Owner change` + }, + message(opts) { + const nameAndEmail = _.escape( + _formatUserNameAndEmail(opts.previousOwner, 'A collaborator') + ) + const projectName = _.escape( + SpamSafe.safeProjectName(opts.project.name, 'a project') + ) + return `\ +${nameAndEmail} has made you the owner of ${projectName}. You can now +manage ${projectName} sharing settings. +` + }, + ctaText(opts) { + return 'View project' + }, + ctaURL(opts) { + const projectUrl = `${ + settings.siteUrl + }/project/${opts.project._id.toString()}` + return projectUrl + } +}) + +function _formatUserNameAndEmail(user, placeholder) { + if (user.first_name && user.last_name) { + const fullName = `${user.first_name} ${user.last_name}` + if (SpamSafe.isSafeUserName(fullName)) { + if (SpamSafe.isSafeEmail(user.email)) { + return `${fullName} (${user.email})` + } else { + return fullName + } + } + } + return SpamSafe.safeEmail(user.email, placeholder) +} + module.exports = { templates, CTAEmailTemplate, NoCTAEmailTemplate, - buildEmail(templateName, opts) { - const template = templates[templateName] - opts.siteUrl = settings.siteUrl - opts.body = template.compiledTemplate(opts) - if ( - settings.email && - settings.email.template && - settings.email.template.customFooter - ) { - opts.body += settings.email.template.customFooter - } - return { - subject: template.subject(opts), - html: template.layout(opts), - text: __guardMethod__(template, 'plainTextTemplate', o => - o.plainTextTemplate(opts) - ) - } - } -} - -function __guardMethod__(obj, methodName, transform) { - if ( - typeof obj !== 'undefined' && - obj !== null && - typeof obj[methodName] === 'function' - ) { - return transform(obj, methodName) - } else { - return undefined - } + buildEmail } diff --git a/services/web/app/src/Features/Email/EmailHandler.js b/services/web/app/src/Features/Email/EmailHandler.js index 938cd62a96..c261976f8e 100644 --- a/services/web/app/src/Features/Email/EmailHandler.js +++ b/services/web/app/src/Features/Email/EmailHandler.js @@ -1,34 +1,24 @@ -/* eslint-disable - handle-callback-err, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const settings = require('settings-sharelatex') +const { callbackify } = require('util') +const Settings = require('settings-sharelatex') const EmailBuilder = require('./EmailBuilder') const EmailSender = require('./EmailSender') -if (settings.email == null) { - settings.email = { lifecycleEnabled: false } -} +const EMAIL_SETTINGS = Settings.email || {} module.exports = { - sendEmail(emailType, opts, callback) { - if (callback == null) { - callback = function(err) {} - } - const email = EmailBuilder.buildEmail(emailType, opts) - if (email.type === 'lifecycle' && !settings.email.lifecycle) { - return callback() - } - opts.html = email.html - opts.text = email.text - opts.subject = email.subject - return EmailSender.sendEmail(opts, err => callback(err)) + sendEmail: callbackify(sendEmail), + promises: { + sendEmail } } + +async function sendEmail(emailType, opts) { + const email = EmailBuilder.buildEmail(emailType, opts) + if (email.type === 'lifecycle' && !EMAIL_SETTINGS.lifecycle) { + return + } + opts.html = email.html + opts.text = email.text + opts.subject = email.subject + await EmailSender.promises.sendEmail(opts) +} diff --git a/services/web/app/src/Features/Email/EmailSender.js b/services/web/app/src/Features/Email/EmailSender.js index 0cb8699fc0..210b433781 100644 --- a/services/web/app/src/Features/Email/EmailSender.js +++ b/services/web/app/src/Features/Email/EmailSender.js @@ -1,19 +1,4 @@ -/* eslint-disable - camelcase, - handle-callback-err, - max-len, - no-undef, - standard/no-callback-literal, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -let defaultFromAddress, nm_client +const { callbackify } = require('util') const logger = require('logger-sharelatex') const metrics = require('metrics-sharelatex') const Settings = require('settings-sharelatex') @@ -21,79 +6,116 @@ const nodemailer = require('nodemailer') const sesTransport = require('nodemailer-ses-transport') const sgTransport = require('nodemailer-sendgrid-transport') const mandrillTransport = require('nodemailer-mandrill-transport') -const rateLimiter = require('../../infrastructure/RateLimiter') +const OError = require('@overleaf/o-error') +const RateLimiter = require('../../infrastructure/RateLimiter') const _ = require('underscore') -if (Settings.email != null && Settings.email.fromAddress != null) { - defaultFromAddress = Settings.email.fromAddress -} else { - defaultFromAddress = '' +const EMAIL_SETTINGS = Settings.email || {} + +module.exports = { + sendEmail: callbackify(sendEmail), + promises: { + sendEmail + } } -// provide dummy mailer unless we have a better one configured. -let client = { - sendMail(options, callback) { - if (callback == null) { - callback = function(err, status) {} +const client = getClient() + +function getClient() { + let client + if (EMAIL_SETTINGS.parameters) { + const emailParameters = EMAIL_SETTINGS.parameters + if (emailParameters.AWSAccessKeyID || EMAIL_SETTINGS.driver === 'ses') { + logger.log('using aws ses for email') + client = nodemailer.createTransport(sesTransport(emailParameters)) + } else if (emailParameters.sendgridApiKey) { + logger.log('using sendgrid for email') + client = nodemailer.createTransport( + sgTransport({ + auth: { + api_key: emailParameters.sendgridApiKey + } + }) + ) + } else if (emailParameters.MandrillApiKey) { + logger.log('using mandril for email') + client = nodemailer.createTransport( + mandrillTransport({ + auth: { + apiKey: emailParameters.MandrillApiKey + } + }) + ) + } else { + logger.log('using smtp for email') + const smtp = _.pick( + emailParameters, + 'host', + 'port', + 'secure', + 'auth', + 'ignoreTLS' + ) + client = nodemailer.createTransport(smtp) } - logger.log({ options }, 'Would send email if enabled.') - return callback() - } -} -if (Settings.email && Settings.email.parameters) { - let emailParameters = Settings.email.parameters - if (emailParameters.AWSAccessKeyID || Settings.email.driver === 'ses') { - logger.log('using aws ses for email') - nm_client = nodemailer.createTransport(sesTransport(emailParameters)) - } else if (emailParameters.sendgridApiKey) { - logger.log('using sendgrid for email') - nm_client = nodemailer.createTransport( - sgTransport({ - auth: { - api_key: emailParameters.sendgridApiKey - } - }) - ) - } else if (emailParameters.MandrillApiKey) { - logger.log('using mandril for email') - nm_client = nodemailer.createTransport( - mandrillTransport({ - auth: { - apiKey: emailParameters.MandrillApiKey - } - }) - ) } else { - logger.log('using smtp for email') - const smtp = _.pick( - emailParameters, - 'host', - 'port', - 'secure', - 'auth', - 'ignoreTLS' + logger.warn( + 'Email transport and/or parameters not defined. No emails will be sent.' ) - nm_client = nodemailer.createTransport(smtp) + client = { + async sendMail(options) { + logger.log({ options }, 'Would send email if enabled.') + } + } } -} else { - logger.warn( - 'Email transport and/or parameters not defined. No emails will be sent.' - ) - nm_client = client + return client } -if (nm_client != null) { - client = nm_client -} else { - logger.warn( - 'Failed to create email transport. Please check your settings. No email will be sent.' - ) +async function sendEmail(options) { + try { + logger.log( + { receiver: options.to, subject: options.subject }, + 'sending email' + ) + const canContinue = await checkCanSendEmail(options) + if (!canContinue) { + logger.log( + { + sendingUser_id: options.sendingUser_id, + to: options.to, + subject: options.subject, + canContinue + }, + 'rate limit hit for sending email, not sending' + ) + throw new OError({ message: 'rate limit hit sending email' }) + } + metrics.inc('email') + let sendMailOptions = { + to: options.to, + from: EMAIL_SETTINGS.fromAddress || '', + subject: options.subject, + html: options.html, + text: options.text, + replyTo: options.replyTo || EMAIL_SETTINGS.replyToAddress, + socketTimeout: 30 * 1000 + } + if (EMAIL_SETTINGS.textEncoding != null) { + sendMailOptions.textEncoding = EMAIL_SETTINGS.textEncoding + } + await client.sendMail(sendMailOptions) + logger.log(`Message sent to ${options.to}`) + } catch (err) { + throw new OError({ + message: 'error sending message' + }).withCause(err) + } } -const checkCanSendEmail = function(options, callback) { +async function checkCanSendEmail(options) { if (options.sendingUser_id == null) { // email not sent from user, not rate limited - return callback(null, true) + return true } const opts = { endpointName: 'send_email', @@ -101,56 +123,6 @@ const checkCanSendEmail = function(options, callback) { subjectName: options.sendingUser_id, throttle: 100 } - return rateLimiter.addCount(opts, callback) -} - -module.exports = { - sendEmail(options, callback) { - if (callback == null) { - callback = function(error) {} - } - logger.log( - { receiver: options.to, subject: options.subject }, - 'sending email' - ) - return checkCanSendEmail(options, function(err, canContinue) { - if (err != null) { - return callback(err) - } - if (!canContinue) { - logger.log( - { - sendingUser_id: options.sendingUser_id, - to: options.to, - subject: options.subject, - canContinue - }, - 'rate limit hit for sending email, not sending' - ) - return callback(new Error('rate limit hit sending email')) - } - metrics.inc('email') - options = { - to: options.to, - from: defaultFromAddress, - subject: options.subject, - html: options.html, - text: options.text, - replyTo: options.replyTo || Settings.email.replyToAddress, - socketTimeout: 30 * 1000 - } - if (Settings.email.textEncoding != null) { - opts.textEncoding = textEncoding - } - return client.sendMail(options, function(err, res) { - if (err != null) { - logger.warn({ err }, 'error sending message') - err = new Error('Cannot send email') - } else { - logger.log(`Message sent to ${options.to}`) - } - return callback(err) - }) - }) - } + const allowed = await RateLimiter.promises.addCount(opts) + return allowed } diff --git a/services/web/app/src/Features/Email/Layouts/BaseWithHeaderEmailLayout.js b/services/web/app/src/Features/Email/Layouts/BaseWithHeaderEmailLayout.js index 615ef06893..2e8ea42dad 100644 --- a/services/web/app/src/Features/Email/Layouts/BaseWithHeaderEmailLayout.js +++ b/services/web/app/src/Features/Email/Layouts/BaseWithHeaderEmailLayout.js @@ -1,8 +1,3 @@ -/* eslint-disable - max-len, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. const _ = require('underscore') const settings = require('settings-sharelatex') diff --git a/services/web/app/src/Features/Email/Layouts/NotificationEmailLayout.js b/services/web/app/src/Features/Email/Layouts/NotificationEmailLayout.js index dd16f96fbc..f96cf614eb 100644 --- a/services/web/app/src/Features/Email/Layouts/NotificationEmailLayout.js +++ b/services/web/app/src/Features/Email/Layouts/NotificationEmailLayout.js @@ -1,9 +1,3 @@ -/* eslint-disable - max-len, - no-useless-escape, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. const _ = require('underscore') const settings = require('settings-sharelatex') @@ -15,7 +9,7 @@ module.exports = _.template(`\ - + @@ -99,7 +93,7 @@ module.exports = _.template(`\ /*@editable*/ text-align:left; } - /* /\/\/\/\/\/\/\/\/\/\ STANDARD STYLING: PREHEADER /\/\/\/\/\/\/\/\/\/\ */ + /* ////////// STANDARD STYLING: PREHEADER ////////// */ /** * @tab Header @@ -135,7 +129,7 @@ module.exports = _.template(`\ /*@editable*/ text-decoration:underline; } - /* /\/\/\/\/\/\/\/\/\/\ STANDARD STYLING: HEADER /\/\/\/\/\/\/\/\/\/\ */ + /* ////////// STANDARD STYLING: HEADER ////////// */ /** * @tab Header @@ -180,7 +174,7 @@ module.exports = _.template(`\ max-width:600px !important; } - /* /\/\/\/\/\/\/\/\/\/\ STANDARD STYLING: MAIN BODY /\/\/\/\/\/\/\/\/\/\ */ + /* ////////// STANDARD STYLING: MAIN BODY ////////// */ /** * @tab Body @@ -221,7 +215,7 @@ module.exports = _.template(`\ height:auto; } - /* /\/\/\/\/\/\/\/\/\/\ STANDARD STYLING: FOOTER /\/\/\/\/\/\/\/\/\/\ */ + /* ////////// STANDARD STYLING: FOOTER ////////// */ /** * @tab Footer @@ -307,7 +301,7 @@ module.exports = _.template(`\ max-width:190px; } - +
diff --git a/services/web/app/src/Features/Email/SpamSafe.js b/services/web/app/src/Features/Email/SpamSafe.js index aaf73f915d..a7047d3850 100644 --- a/services/web/app/src/Features/Email/SpamSafe.js +++ b/services/web/app/src/Features/Email/SpamSafe.js @@ -1,14 +1,3 @@ -/* eslint-disable - chai-friendly/no-unused-expressions, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const XRegExp = require('xregexp') // A note about SAFE_REGEX: @@ -19,17 +8,18 @@ const XRegExp = require('xregexp') // https://www.regular-expressions.info/unicode.html#prop is a good resource for // more obscure regex features. standard RegExp does not support these +const HAN_REGEX = XRegExp('\\p{Han}') const SAFE_REGEX = XRegExp("^[\\p{L}\\p{N}\\s\\-_!'&\\(\\)]+$") const EMAIL_REGEX = XRegExp('^[\\p{L}\\p{N}.+_-]+@[\\w.-]+$') -var SpamSafe = { +const SpamSafe = { isSafeUserName(name) { return SAFE_REGEX.test(name) && name.length <= 30 }, isSafeProjectName(name) { - if (XRegExp('\\p{Han}').test(name)) { - SAFE_REGEX.test(name) && name.length <= 30 + if (HAN_REGEX.test(name)) { + return SAFE_REGEX.test(name) && name.length <= 30 } return SAFE_REGEX.test(name) && name.length <= 100 }, diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index 4c87806d55..76ebcbfca2 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js @@ -32,6 +32,10 @@ module.exports = { } PasswordResetHandler.generateAndEmailResetToken(email, (err, status) => { if (err != null) { + logger.warn( + { err }, + 'failed to generate and email password reset token' + ) res.send(500, { message: err.message }) } else if (status === 'primary') { res.send(200, { diff --git a/services/web/app/src/Features/Project/ProjectDetailsHandler.js b/services/web/app/src/Features/Project/ProjectDetailsHandler.js index 141d01a56f..1c6f579f78 100644 --- a/services/web/app/src/Features/Project/ProjectDetailsHandler.js +++ b/services/web/app/src/Features/Project/ProjectDetailsHandler.js @@ -11,6 +11,7 @@ const ProjectEntityHandler = require('./ProjectEntityHandler') const ProjectHelper = require('./ProjectHelper') const CollaboratorsHandler = require('../Collaborators/CollaboratorsHandler') const PrivilegeLevels = require('../Authorization/PrivilegeLevels') +const EmailHandler = require('../Email/EmailHandler') const settings = require('settings-sharelatex') const { callbackify } = require('util') @@ -104,21 +105,29 @@ async function setProjectDescription(projectId, description) { } async function transferOwnership(projectId, toUserId, options = {}) { - const project = await ProjectGetter.promises.getProject(projectId, { - owner_ref: 1, - collaberator_refs: 1 - }) + // Fetch project and user + const [project, toUser] = await Promise.all([ + ProjectGetter.promises.getProject(projectId, { + owner_ref: 1, + collaberator_refs: 1, + name: 1 + }), + UserGetter.promises.getUser(toUserId) + ]) if (project == null) { throw new Errors.ProjectNotFoundError({ info: { projectId: projectId } }) } + if (toUser == null) { + throw new Errors.UserNotFoundError({ info: { userId: toUserId } }) + } + + // Exit early if the transferee is already the project owner const fromUserId = project.owner_ref if (fromUserId.equals(toUserId)) { return } - const toUser = await UserGetter.promises.getUser(toUserId) - if (toUser == null) { - throw new Errors.UserNotFoundError({ info: { userId: toUserId } }) - } + + // Check permissions const collaboratorIds = project.collaberator_refs || [] if ( !options.allowTransferToNonCollaborators && @@ -126,6 +135,11 @@ async function transferOwnership(projectId, toUserId, options = {}) { ) { throw new Errors.UserNotCollaboratorError({ info: { userId: toUserId } }) } + + // Fetch the current project owner + const fromUser = await UserGetter.promises.getUser(fromUserId) + + // Transfer ownership await CollaboratorsHandler.promises.removeUserFromProject(projectId, toUserId) await Project.update( { _id: projectId }, @@ -137,9 +151,37 @@ async function transferOwnership(projectId, toUserId, options = {}) { fromUserId, PrivilegeLevels.READ_AND_WRITE ) + + // Flush project to TPDS await ProjectEntityHandler.promises.flushProjectToThirdPartyDataStore( projectId ) + + if (fromUser == null) { + // The previous owner didn't exist. This is not supposed to happen, but + // since we're changing the owner anyway, we'll just warn + logger.warn( + { projectId, ownerId: fromUserId }, + 'Project owner did not exist before ownership transfer' + ) + } else { + // Send confirmation emails + await Promise.all([ + EmailHandler.promises.sendEmail( + 'ownershipTransferConfirmationPreviousOwner', + { + to: fromUser.email, + project, + newOwner: toUser + } + ), + EmailHandler.promises.sendEmail('ownershipTransferConfirmationNewOwner', { + to: toUser.email, + project, + previousOwner: fromUser + }) + ]) + } } async function renameProject(projectId, newName) { diff --git a/services/web/app/src/Features/Subscription/SubscriptionHandler.js b/services/web/app/src/Features/Subscription/SubscriptionHandler.js index 77fbd8985a..2e5f30aec4 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionHandler.js +++ b/services/web/app/src/Features/Subscription/SubscriptionHandler.js @@ -169,7 +169,19 @@ const SubscriptionHandler = { } const ONE_HOUR_IN_MS = 1000 * 60 * 60 setTimeout( - () => EmailHandler.sendEmail('canceledSubscription', emailOpts), + () => + EmailHandler.sendEmail( + 'canceledSubscription', + emailOpts, + err => { + if (err != null) { + logger.warn( + { err }, + 'failed to send confirmation email for subscription cancellation' + ) + } + } + ), ONE_HOUR_IN_MS ) Events.emit('cancelSubscription', user._id) @@ -196,9 +208,18 @@ const SubscriptionHandler = { if (error != null) { return callback(error) } - EmailHandler.sendEmail('reactivatedSubscription', { - to: user.email - }) + EmailHandler.sendEmail( + 'reactivatedSubscription', + { to: user.email }, + err => { + if (err != null) { + logger.warn( + { err }, + 'failed to send reactivation confirmation email' + ) + } + } + ) Analytics.recordEvent(user._id, 'subscription-reactivated') return callback() } diff --git a/services/web/app/src/Features/User/UserRegistrationHandler.js b/services/web/app/src/Features/User/UserRegistrationHandler.js index 0c6ce5ed80..92c4b43031 100644 --- a/services/web/app/src/Features/User/UserRegistrationHandler.js +++ b/services/web/app/src/Features/User/UserRegistrationHandler.js @@ -139,7 +139,11 @@ const UserRegistrationHandler = { to: user.email, setNewPasswordUrl }, - () => {} + err => { + if (err != null) { + logger.warn({ err }, 'failed to send activation email') + } + } ) callback(null, user, setNewPasswordUrl) diff --git a/services/web/app/src/infrastructure/RateLimiter.js b/services/web/app/src/infrastructure/RateLimiter.js index 0d93a7f791..6c615312e4 100644 --- a/services/web/app/src/infrastructure/RateLimiter.js +++ b/services/web/app/src/infrastructure/RateLimiter.js @@ -11,14 +11,14 @@ * DS207: Consider shorter variations of null checks * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -let RateLimiter const settings = require('settings-sharelatex') const Metrics = require('metrics-sharelatex') const RedisWrapper = require('./RedisWrapper') const rclient = RedisWrapper.client('ratelimiter') const RollingRateLimiter = require('rolling-rate-limiter') +const { promisifyAll } = require('../util/promises') -module.exports = RateLimiter = { +const RateLimiter = { addCount(opts, callback) { if (settings.disableRateLimits) { return callback(null, true) @@ -54,3 +54,6 @@ module.exports = RateLimiter = { rclient.del(keyName, callback) } } + +RateLimiter.promises = promisifyAll(RateLimiter) +module.exports = RateLimiter diff --git a/services/web/test/unit/src/Email/EmailBuilderTests.js b/services/web/test/unit/src/Email/EmailBuilderTests.js index 4376ae0f98..0777271d1b 100644 --- a/services/web/test/unit/src/Email/EmailBuilderTests.js +++ b/services/web/test/unit/src/Email/EmailBuilderTests.js @@ -1,36 +1,21 @@ -/* eslint-disable - max-len, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const should = require('chai').should() const SandboxedModule = require('sandboxed-module') -const assert = require('assert') const path = require('path') -const sinon = require('sinon') -const modulePath = path.join( - __dirname, - '../../../../app/src/Features/Email/EmailBuilder' -) const { expect } = require('chai') const _ = require('underscore') _.templateSettings = { interpolate: /\{\{(.+?)\}\}/g } +const MODULE_PATH = path.join( + __dirname, + '../../../../app/src/Features/Email/EmailBuilder' +) + describe('EmailBuilder', function() { beforeEach(function() { this.settings = { appName: 'testApp', brandPrefix: '' } - return (this.EmailBuilder = SandboxedModule.require(modulePath, { + this.EmailBuilder = SandboxedModule.require(MODULE_PATH, { globals: { console: console }, @@ -40,12 +25,12 @@ describe('EmailBuilder', function() { log() {} } } - })) + }) }) describe('projectInvite', function() { beforeEach(function() { - return (this.opts = { + this.opts = { to: 'bob@bob.com', first_name: 'bob', owner: { @@ -56,44 +41,38 @@ describe('EmailBuilder', function() { url: 'http://www.project.com', name: 'standard project' } - }) + } }) describe('when sending a normal email', function() { beforeEach(function() { - return (this.email = this.EmailBuilder.buildEmail( - 'projectInvite', - this.opts - )) + this.email = this.EmailBuilder.buildEmail('projectInvite', this.opts) }) it('should have html and text properties', function() { expect(this.email.html != null).to.equal(true) - return expect(this.email.text != null).to.equal(true) + expect(this.email.text != null).to.equal(true) }) it('should not have undefined in it', function() { this.email.html.indexOf('undefined').should.equal(-1) - return this.email.subject.indexOf('undefined').should.equal(-1) + this.email.subject.indexOf('undefined').should.equal(-1) }) }) describe('when someone is up to no good', function() { beforeEach(function() { this.opts.project.name = "" - return (this.email = this.EmailBuilder.buildEmail( - 'projectInvite', - this.opts - )) + this.email = this.EmailBuilder.buildEmail('projectInvite', this.opts) }) it('should not contain unescaped html in the html part', function() { - return expect(this.email.html).to.contain('New Project') + expect(this.email.html).to.contain('New Project') }) it('should not have undefined in it', function() { this.email.html.indexOf('undefined').should.equal(-1) - return this.email.subject.indexOf('undefined').should.equal(-1) + this.email.subject.indexOf('undefined').should.equal(-1) }) }) }) @@ -112,15 +91,12 @@ describe('EmailBuilder', function() { name: 'come buy my product at http://notascam.com' } } - return (this.email = this.EmailBuilder.buildEmail( - 'projectInvite', - this.opts - )) + this.email = this.EmailBuilder.buildEmail('projectInvite', this.opts) }) it('should replace spammy project name', function() { this.email.html.indexOf('a new project').should.not.equal(-1) - return this.email.subject.indexOf('New Project').should.not.equal(-1) + this.email.subject.indexOf('New Project').should.not.equal(-1) }) }) }) diff --git a/services/web/test/unit/src/Email/EmailHandlerTests.js b/services/web/test/unit/src/Email/EmailHandlerTests.js index f0ab9784e4..e814246a67 100644 --- a/services/web/test/unit/src/Email/EmailHandlerTests.js +++ b/services/web/test/unit/src/Email/EmailHandlerTests.js @@ -1,123 +1,99 @@ -/* eslint-disable - max-len, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const should = require('chai').should() const SandboxedModule = require('sandboxed-module') -const assert = require('assert') const path = require('path') const sinon = require('sinon') -const modulePath = path.join( +const { expect } = require('chai') + +const MODULE_PATH = path.join( __dirname, '../../../../app/src/Features/Email/EmailHandler' ) -const { expect } = require('chai') describe('EmailHandler', function() { beforeEach(function() { - this.settings = { email: {} } - this.EmailBuilder = { buildEmail: sinon.stub() } - this.EmailSender = { sendEmail: sinon.stub() } - this.EmailHandler = SandboxedModule.require(modulePath, { + this.html = 'hello' + this.Settings = { email: {} } + this.EmailBuilder = { + buildEmail: sinon.stub().returns({ html: this.html }) + } + this.EmailSender = { + promises: { + sendEmail: sinon.stub().resolves() + } + } + this.EmailHandler = SandboxedModule.require(MODULE_PATH, { globals: { console: console }, requires: { './EmailBuilder': this.EmailBuilder, './EmailSender': this.EmailSender, - 'settings-sharelatex': this.settings, + 'settings-sharelatex': this.Settings, 'logger-sharelatex': { log() {} } } }) - - return (this.html = 'hello') }) describe('send email', function() { - it('should use the correct options', function(done) { - this.EmailBuilder.buildEmail.returns({ html: this.html }) - this.EmailSender.sendEmail.callsArgWith(1) - + it('should use the correct options', async function() { const opts = { to: 'bob@bob.com' } - return this.EmailHandler.sendEmail('welcome', opts, () => { - const args = this.EmailSender.sendEmail.args[0][0] - args.html.should.equal(this.html) - return done() + await this.EmailHandler.promises.sendEmail('welcome', opts) + expect(this.EmailSender.promises.sendEmail).to.have.been.calledWithMatch({ + html: this.html }) }) - it('should return the erroor', function(done) { - this.EmailBuilder.buildEmail.returns({ html: this.html }) - this.EmailSender.sendEmail.callsArgWith(1, 'error') - + it('should return the error', async function() { + this.EmailSender.promises.sendEmail.rejects(new Error('boom')) const opts = { to: 'bob@bob.com', subject: 'hello bob' } - return this.EmailHandler.sendEmail('welcome', opts, err => { - err.should.equal('error') - return done() - }) + await expect(this.EmailHandler.promises.sendEmail('welcome', opts)).to.be + .rejected }) - it('should not send an email if lifecycle is not enabled', function(done) { - this.settings.email.lifecycle = false + it('should not send an email if lifecycle is not enabled', async function() { + this.Settings.email.lifecycle = false this.EmailBuilder.buildEmail.returns({ type: 'lifecycle' }) - return this.EmailHandler.sendEmail('welcome', {}, () => { - this.EmailSender.sendEmail.called.should.equal(false) - return done() - }) + await this.EmailHandler.promises.sendEmail('welcome', {}) + expect(this.EmailSender.promises.sendEmail).not.to.have.been.called }) - it('should send an email if lifecycle is not enabled but the type is notification', function(done) { - this.settings.email.lifecycle = false + it('should send an email if lifecycle is not enabled but the type is notification', async function() { + this.Settings.email.lifecycle = false this.EmailBuilder.buildEmail.returns({ type: 'notification' }) - this.EmailSender.sendEmail.callsArgWith(1) const opts = { to: 'bob@bob.com' } - return this.EmailHandler.sendEmail('welcome', opts, () => { - this.EmailSender.sendEmail.called.should.equal(true) - return done() - }) + await this.EmailHandler.promises.sendEmail('welcome', opts) + expect(this.EmailSender.promises.sendEmail).to.have.been.called }) - it('should send lifecycle email if it is enabled', function(done) { - this.settings.email.lifecycle = true + it('should send lifecycle email if it is enabled', async function() { + this.Settings.email.lifecycle = true this.EmailBuilder.buildEmail.returns({ type: 'lifecycle' }) - this.EmailSender.sendEmail.callsArgWith(1) const opts = { to: 'bob@bob.com' } - return this.EmailHandler.sendEmail('welcome', opts, () => { - this.EmailSender.sendEmail.called.should.equal(true) - return done() - }) + await this.EmailHandler.promises.sendEmail('welcome', opts) + expect(this.EmailSender.promises.sendEmail).to.have.been.called }) describe('with plain-text email content', function() { beforeEach(function() { - return (this.text = 'hello there') + this.text = 'hello there' }) - it('should pass along the text field', function(done) { + it('should pass along the text field', async function() { this.EmailBuilder.buildEmail.returns({ html: this.html, text: this.text }) - this.EmailSender.sendEmail.callsArgWith(1) const opts = { to: 'bob@bob.com' } - return this.EmailHandler.sendEmail('welcome', opts, () => { - const args = this.EmailSender.sendEmail.args[0][0] - args.html.should.equal(this.html) - args.text.should.equal(this.text) - return done() + await this.EmailHandler.promises.sendEmail('welcome', opts) + expect( + this.EmailSender.promises.sendEmail + ).to.have.been.calledWithMatch({ + html: this.html, + text: this.text }) }) }) diff --git a/services/web/test/unit/src/Email/EmailSenderTests.js b/services/web/test/unit/src/Email/EmailSenderTests.js index 290b408773..2fab6dda23 100644 --- a/services/web/test/unit/src/Email/EmailSenderTests.js +++ b/services/web/test/unit/src/Email/EmailSenderTests.js @@ -1,31 +1,22 @@ -/* eslint-disable - max-len, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const should = require('chai').should() const SandboxedModule = require('sandboxed-module') -const assert = require('assert') const path = require('path') const sinon = require('sinon') -const modulePath = path.join( +const { expect } = require('chai') + +const MODULE_PATH = path.join( __dirname, '../../../../app/src/Features/Email/EmailSender.js' ) -const { expect } = require('chai') describe('EmailSender', function() { beforeEach(function() { - this.RateLimiter = { addCount: sinon.stub() } + this.RateLimiter = { + promises: { + addCount: sinon.stub() + } + } - this.settings = { + this.Settings = { email: { transport: 'ses', parameters: { @@ -37,11 +28,11 @@ describe('EmailSender', function() { } } - this.sesClient = { sendMail: sinon.stub() } + this.sesClient = { sendMail: sinon.stub().resolves() } this.ses = { createTransport: () => this.sesClient } - this.sender = SandboxedModule.require(modulePath, { + this.EmailSender = SandboxedModule.require(MODULE_PATH, { globals: { console: console }, @@ -49,7 +40,7 @@ describe('EmailSender', function() { nodemailer: this.ses, 'nodemailer-mandrill-transport': {}, 'nodemailer-sendgrid-transport': {}, - 'settings-sharelatex': this.settings, + 'settings-sharelatex': this.Settings, '../../infrastructure/RateLimiter': this.RateLimiter, 'logger-sharelatex': { log() {}, @@ -62,110 +53,86 @@ describe('EmailSender', function() { } }) - return (this.opts = { + this.opts = { to: 'bob@bob.com', subject: 'new email', html: '' - }) + } }) describe('sendEmail', function() { - it('should set the properties on the email to send', function(done) { - this.sesClient.sendMail.callsArgWith(1) - - return this.sender.sendEmail(this.opts, err => { - expect(err).to.not.exist - const args = this.sesClient.sendMail.args[0][0] - args.html.should.equal(this.opts.html) - args.to.should.equal(this.opts.to) - args.subject.should.equal(this.opts.subject) - return done() + it('should set the properties on the email to send', async function() { + await this.EmailSender.promises.sendEmail(this.opts) + expect(this.sesClient.sendMail).to.have.been.calledWithMatch({ + html: this.opts.html, + to: this.opts.to, + subject: this.opts.subject }) }) - it('should return a non-specific error', function(done) { - this.sesClient.sendMail.callsArgWith(1, 'error') - return this.sender.sendEmail({}, err => { - err.should.exist - err.toString().should.equal('Error: Cannot send email') - return done() + it('should return a non-specific error', async function() { + this.sesClient.sendMail.rejects(new Error('boom')) + expect(this.EmailSender.promises.sendEmail({})).to.be.rejectedWith( + 'Error: Cannot send email' + ) + }) + + it('should use the from address from settings', async function() { + await this.EmailSender.promises.sendEmail(this.opts) + expect(this.sesClient.sendMail).to.have.been.calledWithMatch({ + from: this.Settings.email.fromAddress }) }) - it('should use the from address from settings', function(done) { - this.sesClient.sendMail.callsArgWith(1) - - return this.sender.sendEmail(this.opts, () => { - const args = this.sesClient.sendMail.args[0][0] - args.from.should.equal(this.settings.email.fromAddress) - return done() + it('should use the reply to address from settings', async function() { + await this.EmailSender.promises.sendEmail(this.opts) + expect(this.sesClient.sendMail).to.have.been.calledWithMatch({ + replyTo: this.Settings.email.replyToAddress }) }) - it('should use the reply to address from settings', function(done) { - this.sesClient.sendMail.callsArgWith(1) - - return this.sender.sendEmail(this.opts, () => { - const args = this.sesClient.sendMail.args[0][0] - args.replyTo.should.equal(this.settings.email.replyToAddress) - return done() - }) - }) - - it('should use the reply to address in options as an override', function(done) { - this.sesClient.sendMail.callsArgWith(1) - + it('should use the reply to address in options as an override', async function() { this.opts.replyTo = 'someone@else.com' - return this.sender.sendEmail(this.opts, () => { - const args = this.sesClient.sendMail.args[0][0] - args.replyTo.should.equal(this.opts.replyTo) - return done() + await this.EmailSender.promises.sendEmail(this.opts) + expect(this.sesClient.sendMail).to.have.been.calledWithMatch({ + replyTo: this.opts.replyTo }) }) - it('should not send an email when the rate limiter says no', function(done) { + it('should not send an email when the rate limiter says no', async function() { this.opts.sendingUser_id = '12321312321' - this.RateLimiter.addCount.callsArgWith(1, null, false) - return this.sender.sendEmail(this.opts, () => { - this.sesClient.sendMail.called.should.equal(false) - return done() - }) + this.RateLimiter.promises.addCount.resolves(false) + await expect(this.EmailSender.promises.sendEmail(this.opts)).to.be + .rejected + expect(this.sesClient.sendMail).not.to.have.been.called }) - it('should send the email when the rate limtier says continue', function(done) { - this.sesClient.sendMail.callsArgWith(1) + it('should send the email when the rate limtier says continue', async function() { this.opts.sendingUser_id = '12321312321' - this.RateLimiter.addCount.callsArgWith(1, null, true) - return this.sender.sendEmail(this.opts, () => { - this.sesClient.sendMail.called.should.equal(true) - return done() - }) + this.RateLimiter.promises.addCount.resolves(true) + await this.EmailSender.promises.sendEmail(this.opts) + expect(this.sesClient.sendMail).to.have.been.called }) - it('should not check the rate limiter when there is no sendingUser_id', function(done) { - this.sesClient.sendMail.callsArgWith(1) - return this.sender.sendEmail(this.opts, () => { - this.sesClient.sendMail.called.should.equal(true) - this.RateLimiter.addCount.called.should.equal(false) - return done() + it('should not check the rate limiter when there is no sendingUser_id', async function() { + this.EmailSender.sendEmail(this.opts, () => { + expect(this.sesClient.sendMail).to.have.been.called + expect(this.RateLimiter.addCount).not.to.have.been.called }) }) describe('with plain-text email content', function() { beforeEach(function() { - return (this.opts.text = 'hello there') + this.opts.text = 'hello there' }) - it('should set the text property on the email to send', function(done) { - this.sesClient.sendMail.callsArgWith(1) - - return this.sender.sendEmail(this.opts, () => { - const args = this.sesClient.sendMail.args[0][0] - args.html.should.equal(this.opts.html) - args.text.should.equal(this.opts.text) - args.to.should.equal(this.opts.to) - args.subject.should.equal(this.opts.subject) - return done() + it('should set the text property on the email to send', async function() { + await this.EmailSender.promises.sendEmail(this.opts) + expect(this.sesClient.sendMail).to.have.been.calledWithMatch({ + html: this.opts.html, + text: this.opts.text, + to: this.opts.to, + subject: this.opts.subject }) }) }) diff --git a/services/web/test/unit/src/Email/SpamSafeTests.js b/services/web/test/unit/src/Email/SpamSafeTests.js index bafb32c6b0..f1ddf58261 100644 --- a/services/web/test/unit/src/Email/SpamSafeTests.js +++ b/services/web/test/unit/src/Email/SpamSafeTests.js @@ -1,14 +1,3 @@ -/* eslint-disable - max-len, - no-useless-escape, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const path = require('path') const modulePath = path.join( __dirname, @@ -80,7 +69,7 @@ describe('SpamSafe', function() { expect( SpamSafe.safeEmail(`me+${'a'.repeat(40)}@googoole.con`, 'A collaborator') ).to.equal('A collaborator') - return expect( + expect( SpamSafe.safeEmail('sendME$$$@iAmAprince.com', 'A collaborator') ).to.equal('A collaborator') }) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js index b8e6d999d4..6a99d60d37 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js @@ -46,7 +46,9 @@ describe('PasswordResetController', function() { 'settings-sharelatex': this.settings, './PasswordResetHandler': this.PasswordResetHandler, 'logger-sharelatex': { - log() {} + log() {}, + warn() {}, + error() {} }, '../../infrastructure/RateLimiter': this.RateLimiter, '../Authentication/AuthenticationController': (this.AuthenticationController = {}),