From 2be396ff6e750c86fe691933f2ace1c2bf9b4b82 Mon Sep 17 00:00:00 2001 From: Peter Jaszkowiak Date: Sat, 5 Dec 2020 14:25:14 -0700 Subject: [PATCH] fix: email testing and settings change from ACP - changing email SMTP settings wouldn't apply the first time - "Send Test Email" now will report emailer errors in most cases --- public/src/admin/settings/email.js | 1 + src/emailer.js | 36 +++++++----------- src/notifications.js | 6 +-- src/socket.io/admin/email.js | 59 +++++++++++++----------------- src/socket.io/admin/user.js | 11 +++++- src/socket.io/user.js | 3 +- src/user/approval.js | 3 +- src/user/bans.js | 6 +-- src/user/create.js | 4 +- src/user/digest.js | 26 ++++++------- src/user/invite.js | 1 - src/user/profile.js | 3 +- src/user/reset.js | 2 +- test/api.js | 9 +++++ test/socket.io.js | 15 ++++++++ test/user.js | 14 +++++++ 16 files changed, 114 insertions(+), 85 deletions(-) diff --git a/public/src/admin/settings/email.js b/public/src/admin/settings/email.js index 458163f8d7..8f2f7f46ed 100644 --- a/public/src/admin/settings/email.js +++ b/public/src/admin/settings/email.js @@ -22,6 +22,7 @@ define('admin/settings/email', ['ace/ace', 'admin/settings'], function (ace) { $('button[data-action="email.test"]').off('click').on('click', function () { socket.emit('admin.email.test', { template: $('#test-email').val() }, function (err) { if (err) { + console.error(err.message); return app.alertError(err.message); } app.alertSuccess('Test Email Sent'); diff --git a/src/emailer.js b/src/emailer.js index 4b9ae4a575..7a2fcdc46d 100644 --- a/src/emailer.js +++ b/src/emailer.js @@ -37,12 +37,9 @@ Emailer.listServices = () => Object.keys(wellKnownServices); Emailer._defaultPayload = {}; const smtpSettingsChanged = (config) => { - if (!prevConfig) { - prevConfig = meta.config; - } - const settings = [ 'email:smtpTransport:enabled', + 'email:smtpTransport:pool', 'email:smtpTransport:user', 'email:smtpTransport:pass', 'email:smtpTransport:service', @@ -113,8 +110,8 @@ Emailer.getTemplates = async (config) => { }; Emailer.setupFallbackTransport = (config) => { - winston.verbose('[emailer] Setting up SMTP fallback transport'); - // Enable Gmail transport if enabled in ACP + winston.verbose('[emailer] Setting up fallback transport'); + // Enable SMTP transport if enabled in ACP if (parseInt(config['email:smtpTransport:enabled'], 10) === 1) { const smtpOptions = { pool: config['email:smtpTransport:pool'], @@ -177,6 +174,11 @@ Emailer.registerApp = (expressApp) => { Emailer.setupFallbackTransport(meta.config); buildCustomTemplates(meta.config); + // need to shallow clone the config object + // otherwise prevConfig holds reference to meta.config object, + // which is updated before the pubsub handler is called + prevConfig = { ...meta.config }; + // Update default payload if new logo is uploaded pubsub.on('config:update', (config) => { if (config) { @@ -204,8 +206,7 @@ Emailer.registerApp = (expressApp) => { Emailer.send = async (template, uid, params) => { if (!app) { - winston.warn('[emailer] App not ready!'); - return; + throw Error('[emailer] App not ready!'); } const [userData, userSettings] = await Promise.all([ @@ -214,13 +215,13 @@ Emailer.send = async (template, uid, params) => { ]); if (!userData || !userData.email) { - winston.warn('uid : ' + uid + ' has no email, not sending.'); + winston.warn(`uid : ${uid} has no email, not sending "${template}" email.`); return; } const allowedTpls = ['verify_email', 'welcome', 'registration_accepted']; if (meta.config.requireEmailConfirmation && !userData['email:confirmed'] && !allowedTpls.includes(template)) { - winston.warn('uid : ' + uid + ' has not confirmed email, not sending "' + template + '" email.'); + winston.warn(`uid : ${uid} (${userData.email}) has not confirmed email, not sending "${template}" email.`); return; } @@ -229,11 +230,7 @@ Emailer.send = async (template, uid, params) => { params.uid = uid; params.username = userData.username; params.rtl = await translator.translate('[[language:dir]]', userSettings.userLang) === 'rtl'; - try { - await Emailer.sendToEmail(template, userData.email, userSettings.userLang, params); - } catch (err) { - winston.error(err.stack); - } + await Emailer.sendToEmail(template, userData.email, userSettings.userLang, params); }; Emailer.sendToEmail = async (template, email, language, params) => { @@ -313,7 +310,7 @@ Emailer.sendToEmail = async (template, email, language, params) => { } }; -Emailer.sendViaFallback = (data, callback) => { +Emailer.sendViaFallback = async (data) => { // Some minor alterations to the data to conform to nodemailer standard data.text = data.plaintext; delete data.plaintext; @@ -323,12 +320,7 @@ Emailer.sendViaFallback = (data, callback) => { delete data.from_name; winston.verbose('[emailer] Sending email to uid ' + data.uid + ' (' + data.to + ')'); - Emailer.fallbackTransport.sendMail(data, (err) => { - if (err) { - winston.error(err.stack); - } - callback(); - }); + await Emailer.fallbackTransport.sendMail(data); }; Emailer.renderAndTranslate = async (template, params, lang) => { diff --git a/src/notifications.js b/src/notifications.js index df0429c654..b98a987cb8 100644 --- a/src/notifications.js +++ b/src/notifications.js @@ -181,8 +181,8 @@ async function pushToUids(uids, notification) { } body = posts.relativeToAbsolute(body, posts.urlRegex); body = posts.relativeToAbsolute(body, posts.imgRegex); - await async.eachLimit(uids, 3, function (uid, next) { - emailer.send('notification', uid, { + await async.eachLimit(uids, 3, async (uid) => { + await emailer.send('notification', uid, { path: notification.path, notification_url: notification.path.startsWith('http') ? notification.path : nconf.get('url') + notification.path, subject: utils.stripHTMLTags(notification.subject || '[[notifications:new_notification]]'), @@ -190,7 +190,7 @@ async function pushToUids(uids, notification) { body: body, notification: notification, showUnsubscribe: true, - }, next); + }).catch(err => winston.error('[emailer.send] ' + err.stack)); }); } diff --git a/src/socket.io/admin/email.js b/src/socket.io/admin/email.js index 04874675b9..584e5785e4 100644 --- a/src/socket.io/admin/email.js +++ b/src/socket.io/admin/email.js @@ -1,25 +1,24 @@ 'use strict'; -const async = require('async'); const userDigest = require('../../user/digest'); const userEmail = require('../../user/email'); const notifications = require('../../notifications'); const emailer = require('../../emailer'); -const utils = require('../../../public/src/utils'); +const utils = require('../../utils'); const Email = module.exports; -Email.test = function (socket, data, callback) { +Email.test = async function (socket, data) { const payload = { subject: '[[email:test-email.subject]]', }; switch (data.template) { case 'digest': - userDigest.execute({ + await userDigest.execute({ interval: 'alltime', subscribers: [socket.uid], - }, callback); + }); break; case 'banned': @@ -28,42 +27,36 @@ Email.test = function (socket, data, callback) { until: utils.toISOString(Date.now()), reason: 'Test Reason', }); - emailer.send(data.template, socket.uid, payload, callback); + await emailer.send(data.template, socket.uid, payload); break; case 'welcome': - userEmail.sendValidationEmail(socket.uid, { + await userEmail.sendValidationEmail(socket.uid, { force: 1, - }, callback); + }); break; - case 'notification': - async.waterfall([ - function (next) { - notifications.create({ - type: 'test', - bodyShort: '[[email:notif.test.short]]', - bodyLong: '[[email:notif.test.long]]', - nid: 'uid:' + socket.uid + ':test', - path: '/', - from: socket.uid, - }, next); - }, - function (notifObj, next) { - emailer.send('notification', socket.uid, { - path: notifObj.path, - subject: utils.stripHTMLTags(notifObj.subject || '[[notifications:new_notification]]'), - intro: utils.stripHTMLTags(notifObj.bodyShort), - body: notifObj.bodyLong || '', - notification: notifObj, - showUnsubscribe: true, - }, next); - }, - ], callback); - break; + case 'notification': { + const notification = await notifications.create({ + type: 'test', + bodyShort: '[[email:notif.test.short]]', + bodyLong: '[[email:notif.test.long]]', + nid: 'uid:' + socket.uid + ':test', + path: '/', + from: socket.uid, + }); + await emailer.send('notification', socket.uid, { + path: notification.path, + subject: utils.stripHTMLTags(notification.subject || '[[notifications:new_notification]]'), + intro: utils.stripHTMLTags(notification.bodyShort), + body: notification.bodyLong || '', + notification, + showUnsubscribe: true, + }); + } break; default: - emailer.send(data.template, socket.uid, payload, callback); + await emailer.send(data.template, socket.uid, payload); break; } }; diff --git a/src/socket.io/admin/user.js b/src/socket.io/admin/user.js index a0070d7d37..8e1c91a2ab 100644 --- a/src/socket.io/admin/user.js +++ b/src/socket.io/admin/user.js @@ -87,9 +87,18 @@ User.sendValidationEmail = async function (socket, uids) { throw new Error('[[error:email-confirmations-are-disabled]]'); } + const failed = []; + await async.eachLimit(uids, 50, async function (uid) { - await user.email.sendValidationEmail(uid, { force: true }); + await user.email.sendValidationEmail(uid, { force: true }).catch((err) => { + winston.error('[user.create] Validation email failed to send\n[emailer.send] ' + err.stack); + failed.push(uid); + }); }); + + if (failed.length) { + throw Error(`Email sending failed for the following uids, check server logs for more info: ${failed.join(',')}`); + } }; User.sendPasswordResetEmail = async function (socket, uids) { diff --git a/src/socket.io/user.js b/src/socket.io/user.js index ed7466a377..6e7f03321f 100644 --- a/src/socket.io/user.js +++ b/src/socket.io/user.js @@ -1,6 +1,7 @@ 'use strict'; const util = require('util'); +const winston = require('winston'); const sleep = util.promisify(setTimeout); const api = require('../api'); @@ -117,7 +118,7 @@ SocketUser.reset.commit = async function (socket, data) { username: username, date: parsedDate, subject: '[[email:reset.notify.subject]]', - }); + }).catch(err => winston.error('[emailer.send] ' + err.stack)); }; SocketUser.isFollowing = async function (socket, data) { diff --git a/src/user/approval.js b/src/user/approval.js index 507aa7ef1b..94514963e4 100644 --- a/src/user/approval.js +++ b/src/user/approval.js @@ -1,6 +1,7 @@ 'use strict'; const validator = require('validator'); +const winston = require('winston'); const cronJob = require('cron').CronJob; const db = require('../database'); @@ -78,7 +79,7 @@ module.exports = function (User) { subject: '[[email:welcome-to, ' + (meta.config.title || meta.config.browserTitle || 'NodeBB') + ']]', template: 'registration_accepted', uid: uid, - }); + }).catch(err => winston.error('[emailer.send] ' + err.stack)); const total = await db.incrObjectField('registration:queue:approval:times', 'totalTime', Math.floor((Date.now() - creation_time) / 60000)); const counter = await db.incrObjectField('registration:queue:approval:times', 'counter', 1); await db.setObjectField('registration:queue:approval:times', 'average', total / counter); diff --git a/src/user/bans.js b/src/user/bans.js index d35370cb57..617ecce3bc 100644 --- a/src/user/bans.js +++ b/src/user/bans.js @@ -53,11 +53,7 @@ module.exports = function (User) { until: until ? (new Date(until)).toUTCString().replace(/,/g, '\\,') : false, reason: reason, }; - try { - await emailer.send('banned', uid, data); - } catch (err) { - winston.error('[emailer.send] ' + err.message); - } + await emailer.send('banned', uid, data).catch(err => winston.error('[emailer.send] ' + err.stack)); return banData; }; diff --git a/src/user/create.js b/src/user/create.js index b622a8b5a3..690f7b7846 100644 --- a/src/user/create.js +++ b/src/user/create.js @@ -1,6 +1,8 @@ 'use strict'; const zxcvbn = require('zxcvbn'); +const winston = require('winston'); + const db = require('../database'); const utils = require('../utils'); const slugify = require('../slugify'); @@ -116,7 +118,7 @@ module.exports = function (User) { if (userData.email && userData.uid > 1 && meta.config.requireEmailConfirmation) { User.email.sendValidationEmail(userData.uid, { email: userData.email, - }); + }).catch(err => winston.error('[user.create] Validation email failed to send\n[emailer.send] ' + err.stack)); } if (userNameChanged) { await User.notifications.sendNameChangeNotification(userData.uid, userData.username); diff --git a/src/user/digest.js b/src/user/digest.js index 689d0ff771..4ae34abd14 100644 --- a/src/user/digest.js +++ b/src/user/digest.js @@ -125,21 +125,17 @@ Digest.send = async function (data) { emailsSent += 1; const now = new Date(); - try { - await emailer.send('digest', userObj.uid, { - subject: '[[email:digest.subject, ' + (now.getFullYear() + '/' + (now.getMonth() + 1) + '/' + now.getDate()) + ']]', - username: userObj.username, - userslug: userObj.userslug, - notifications: unreadNotifs, - recent: recentTopics, - topTopics: topTopics, - popularTopics: popularTopics, - interval: data.interval, - showUnsubscribe: true, - }); - } catch (err) { - winston.error('[user/jobs] Could not send digest email\n' + err.stack); - } + await emailer.send('digest', userObj.uid, { + subject: '[[email:digest.subject, ' + (now.getFullYear() + '/' + (now.getMonth() + 1) + '/' + now.getDate()) + ']]', + username: userObj.username, + userslug: userObj.userslug, + notifications: unreadNotifs, + recent: recentTopics, + topTopics: topTopics, + popularTopics: popularTopics, + interval: data.interval, + showUnsubscribe: true, + }).catch(err => winston.error('[user/jobs] Could not send digest email\n[emailer.send] ' + err.stack)); if (data.interval !== 'alltime') { await db.sortedSetAdd('digest:delivery', now.getTime(), userObj.uid); diff --git a/src/user/invite.js b/src/user/invite.js index 4c20eab70a..8085ae4ece 100644 --- a/src/user/invite.js +++ b/src/user/invite.js @@ -49,7 +49,6 @@ module.exports = function (User) { } const data = await prepareInvitation(uid, email, groupsToJoin); - await emailer.sendToEmail('invitation', email, meta.config.defaultLang, data); }; diff --git a/src/user/profile.js b/src/user/profile.js index 2a571d4cfc..fee66f7fe2 100644 --- a/src/user/profile.js +++ b/src/user/profile.js @@ -3,6 +3,7 @@ const async = require('async'); const validator = require('validator'); +const winston = require('winston'); const utils = require('../utils'); const slugify = require('../slugify'); @@ -246,7 +247,7 @@ module.exports = function (User) { email: newEmail, subject: '[[email:email.verify-your-email.subject]]', template: 'verify_email', - }); + }).catch(err => winston.error('[user.create] Validation email failed to send\n[emailer.send] ' + err.stack)); } } diff --git a/src/user/reset.js b/src/user/reset.js index eabf826c5a..24a3869438 100644 --- a/src/user/reset.js +++ b/src/user/reset.js @@ -55,7 +55,7 @@ UserReset.send = async function (email) { subject: '[[email:password-reset-requested]]', template: 'reset', uid: uid, - }); + }).catch(err => winston.error('[emailer.send] ' + err.stack)); }; UserReset.commit = async function (code, password) { diff --git a/test/api.js b/test/api.js index d444f0337f..4d929743c3 100644 --- a/test/api.js +++ b/test/api.js @@ -68,9 +68,13 @@ describe('API', async () => { async function dummySearchHook(data) { return [1]; } + async function dummyEmailerHook(data) { + // pretend to handle sending emails + } after(async function () { plugins.unregisterHook('core', 'filter:search.query', dummySearchHook); + plugins.unregisterHook('emailer-test', 'filter:email.send'); }); async function setupData() { @@ -145,6 +149,11 @@ describe('API', async () => { hook: 'filter:search.query', method: dummySearchHook, }); + // Attach an emailer hook so related requests do not error + plugins.registerHook('emailer-test', { + hook: 'filter:email.send', + method: dummyEmailerHook, + }); jar = await helpers.loginUser('admin', '123456'); diff --git a/test/socket.io.js b/test/socket.io.js index f041037d1d..8a4e7fa432 100644 --- a/test/socket.io.js +++ b/test/socket.io.js @@ -240,6 +240,21 @@ describe('socket.io', function () { describe('validation emails', function () { var meta = require('../src/meta'); + var plugins = require('../src/plugins'); + + async function dummyEmailerHook(data) { + // pretend to handle sending emails + } + before(function () { + // Attach an emailer hook so related requests do not error + plugins.registerHook('emailer-test', { + hook: 'filter:email.send', + method: dummyEmailerHook, + }); + }); + after(function () { + plugins.unregisterHook('emailer-test', 'filter:email.send'); + }); it('should validate emails', function (done) { socketAdmin.user.validateEmail({ uid: adminUid }, [regularUid], function (err) { diff --git a/test/user.js b/test/user.js index 253657afa2..405ba3adc0 100644 --- a/test/user.js +++ b/test/user.js @@ -25,7 +25,18 @@ describe('User', function () { var testUid; var testCid; + var plugins = require('../src/plugins'); + + async function dummyEmailerHook(data) { + // pretend to handle sending emails + } before(function (done) { + // Attach an emailer hook so related requests do not error + plugins.registerHook('emailer-test', { + hook: 'filter:email.send', + method: dummyEmailerHook, + }); + Categories.create({ name: 'Test Category', description: 'A test', @@ -39,6 +50,9 @@ describe('User', function () { done(); }); }); + after(function () { + plugins.unregisterHook('emailer-test', 'filter:email.send'); + }); beforeEach(function () { userData = {