From 6fc0c503896c4924e0a35108cd691c24ada6deab Mon Sep 17 00:00:00 2001 From: Vijay Budhram Date: Mon, 26 Dec 2016 11:09:11 -0500 Subject: [PATCH] refactor(signin): Use new verify account sync template and add location data to others (#1600), r=@vladikoff Added new verify sync account template and text tweaks. --- lib/mailer.js | 25 +++++- lib/routes/account.js | 96 +++++++++++++---------- lib/routes/password.js | 109 ++++++++++++++++++--------- npm-shrinkwrap.json | 2 +- test/local/account_routes.js | 4 + test/local/password_routes.js | 8 ++ test/remote/account_create_tests.js | 34 +++++++-- test/remote/password_change_tests.js | 1 + test/remote/password_forgot_tests.js | 8 +- 9 files changed, 197 insertions(+), 90 deletions(-) diff --git a/lib/mailer.js b/lib/mailer.js index 6681885d..965e36ae 100644 --- a/lib/mailer.js +++ b/lib/mailer.js @@ -27,7 +27,13 @@ module.exports = function (config, log) { service: opts.service, redirectTo: opts.redirectTo, resume: opts.resume, - acceptLanguage: opts.acceptLanguage || defaultLanguage + acceptLanguage: opts.acceptLanguage || defaultLanguage, + ip: opts.ip, + location: opts.location, + uaBrowser: opts.uaBrowser, + uaBrowserVersion: opts.uaBrowserVersion, + uaOS: opts.uaOS, + uaOSVersion: opts.uaOSVersion } )) } @@ -60,7 +66,14 @@ module.exports = function (config, log) { service: opts.service, redirectTo: opts.redirectTo, resume: opts.resume, - acceptLanguage: opts.acceptLanguage || defaultLanguage + acceptLanguage: opts.acceptLanguage || defaultLanguage, + ip: opts.ip, + location: opts.location, + timeZone: opts.timeZone, + uaBrowser: opts.uaBrowser, + uaBrowserVersion: opts.uaBrowserVersion, + uaOS: opts.uaOS, + uaOSVersion: opts.uaOSVersion } )) } @@ -68,7 +81,13 @@ module.exports = function (config, log) { return P.resolve(mailer.passwordChangedEmail( { email: email, - acceptLanguage: opts.acceptLanguage || defaultLanguage + acceptLanguage: opts.acceptLanguage || defaultLanguage, + ip: opts.ip, + location: opts.location, + uaBrowser: opts.uaBrowser, + uaBrowserVersion: opts.uaBrowserVersion, + uaOS: opts.uaOS, + uaOSVersion: opts.uaOSVersion } )) } diff --git a/lib/routes/account.js b/lib/routes/account.js index 6e9382b3..b1fc2cc4 100644 --- a/lib/routes/account.js +++ b/lib/routes/account.js @@ -99,6 +99,7 @@ module.exports = function ( var locale = request.app.acceptLanguage var userAgentString = request.headers['user-agent'] var service = form.service || query.service + const ip = request.app.clientAddress var preVerified, password, verifyHash, account, sessionToken, keyFetchToken, emailCode, tokenVerificationId, authSalt request.validateMetricsContext() @@ -273,42 +274,47 @@ module.exports = function ( function sendVerifyCode () { if (! account.emailVerified) { - mailer.sendVerifyCode(account, account.emailCode, { - service: form.service || query.service, - redirectTo: form.redirectTo, - resume: form.resume, - acceptLanguage: request.app.acceptLanguage - }) - .then(function () { - // only create reminder if sendVerifyCode succeeds - verificationReminder.create({ - uid: account.uid.toString('hex') - }).catch(function (err) { - log.error({ op: 'Account.verificationReminder.create', err: err }) + return getGeoData(ip) + .then(function (geoData) { + mailer.sendVerifyCode(account, account.emailCode, userAgent.call({ + service: form.service || query.service, + redirectTo: form.redirectTo, + resume: form.resume, + acceptLanguage: request.app.acceptLanguage, + ip: ip, + location: geoData.location + }, request.headers['user-agent'], log)) + .then(function () { + // only create reminder if sendVerifyCode succeeds + verificationReminder.create({ + uid: account.uid.toString('hex') + }).catch(function (err) { + log.error({op: 'Account.verificationReminder.create', err: err}) + }) + + if (tokenVerificationId) { + // Log server-side metrics for confirming verification rates + log.info({ + op: 'account.create.confirm.start', + uid: account.uid.toString('hex'), + tokenVerificationId: tokenVerificationId + }) + } + }) + .catch(function (err) { + log.error({op: 'mailer.sendVerifyCode.1', err: err}) + + if (tokenVerificationId) { + // Log possible email bounce, used for confirming verification rates + log.error({ + op: 'account.create.confirm.error', + uid: account.uid.toString('hex'), + err: err, + tokenVerificationId: tokenVerificationId + }) + } + }) }) - - if (tokenVerificationId) { - // Log server-side metrics for confirming verification rates - log.info({ - op: 'account.create.confirm.start', - uid: account.uid.toString('hex'), - tokenVerificationId: tokenVerificationId - }) - } - }) - .catch(function (err) { - log.error({ op: 'mailer.sendVerifyCode.1', err: err }) - - if (tokenVerificationId) { - // Log possible email bounce, used for confirming verification rates - log.error({ - op: 'account.create.confirm.error', - uid: account.uid.toString('hex'), - err: err, - tokenVerificationId: tokenVerificationId - }) - } - }) } } @@ -816,12 +822,20 @@ module.exports = function ( var emailCode = tokenVerificationId ? tokenVerificationId : emailRecord.emailCode emailSent = true - return mailer.sendVerifyCode(emailRecord, emailCode, { - service: service, - redirectTo: redirectTo, - resume: resume, - acceptLanguage: request.app.acceptLanguage - }).then(() => request.emitMetricsEvent('email.verification.sent')) + return getGeoData(ip) + .then( + function (geoData) { + return mailer.sendVerifyCode(emailRecord, emailCode, userAgent.call({ + service: service, + redirectTo: redirectTo, + resume: resume, + acceptLanguage: request.app.acceptLanguage, + ip: ip, + location: geoData.location + }, request.headers['user-agent'], log)) + } + ) + .then(() => request.emitMetricsEvent('email.verification.sent')) } } diff --git a/lib/routes/password.js b/lib/routes/password.js index 19c2a57f..f5460259 100644 --- a/lib/routes/password.js +++ b/lib/routes/password.js @@ -9,6 +9,7 @@ const HEX_STRING = validators.HEX_STRING const butil = require('../crypto/butil') const P = require('../promise') +const userAgent = require('../userAgent') const random = require('../crypto/random') const requestHelper = require('../routes/utils/request_helper') @@ -28,6 +29,8 @@ module.exports = function ( push ) { + const getGeoData = require('../geodb')(log) + function failVerifyAttempt(passwordForgotToken) { return (passwordForgotToken.failAttempt()) ? db.deletePasswordForgotToken(passwordForgotToken) : @@ -146,6 +149,7 @@ module.exports = function ( var wrapKb = Buffer(request.payload.wrapKb, 'hex') var sessionTokenId = request.payload.sessionToken var wantsKeys = requestHelper.wantsKeys(request) + const ip = request.app.clientAddress var account, verifyHash, sessionToken, keyFetchToken, verifiedStatus, devicesToNotify @@ -242,12 +246,24 @@ module.exports = function ( .then( function (accountData) { account = accountData - return mailer.sendPasswordChangedNotification( - account.email, - { - acceptLanguage: request.app.acceptLanguage - } - ) + } + ) + .then( + function () { + return getGeoData(ip) + .then( + function (geoData) { + return mailer.sendPasswordChangedNotification( + account.email, + userAgent.call({ + acceptLanguage: request.app.acceptLanguage, + ip: ip, + location: geoData.location, + timeZone: geoData.timeZone + }, request.headers['user-agent'], log) + ) + } + ) } ) } @@ -346,6 +362,7 @@ module.exports = function ( log.begin('Password.forgotSend', request) var email = request.payload.email var service = request.payload.service || request.query.service + const ip = request.app.clientAddress request.validateMetricsContext() @@ -368,26 +385,34 @@ module.exports = function ( ) .then( function (passwordForgotToken) { - return mailer.sendRecoveryCode( - passwordForgotToken, - passwordForgotToken.passCode, - { - service: service, - redirectTo: request.payload.redirectTo, - resume: request.payload.resume, - acceptLanguage: request.app.acceptLanguage - } - ) - .then( - function() { - return request.emitMetricsEvent('password.forgot.send_code.completed') - } - ) - .then( - function() { - return passwordForgotToken - } - ) + return getGeoData(ip) + .then( + function (geoData) { + return mailer.sendRecoveryCode( + passwordForgotToken, + passwordForgotToken.passCode, + userAgent.call({ + service: service, + redirectTo: request.payload.redirectTo, + resume: request.payload.resume, + acceptLanguage: request.app.acceptLanguage, + ip: ip, + location: geoData.location, + timeZone: geoData.timeZone + }, request.headers['user-agent'], log) + ) + } + ) + .then( + function () { + return request.emitMetricsEvent('password.forgot.send_code.completed') + } + ) + .then( + function () { + return passwordForgotToken + } + ) } ) .done( @@ -434,6 +459,7 @@ module.exports = function ( log.begin('Password.forgotResend', request) var passwordForgotToken = request.auth.credentials var service = request.payload.service || request.query.service + const ip = request.app.clientAddress request.validateMetricsContext() @@ -446,17 +472,26 @@ module.exports = function ( 'passwordForgotResendCode') ) .then( - mailer.sendRecoveryCode.bind( - mailer, - passwordForgotToken, - passwordForgotToken.passCode, - { - service: service, - redirectTo: request.payload.redirectTo, - resume: request.payload.resume, - acceptLanguage: request.app.acceptLanguage - } - ) + function () { + return getGeoData(ip) + .then( + function (geoData) { + return mailer.sendRecoveryCode( + passwordForgotToken, + passwordForgotToken.passCode, + userAgent.call({ + service: service, + redirectTo: request.payload.redirectTo, + resume: request.payload.resume, + acceptLanguage: request.app.acceptLanguage, + ip: ip, + location: geoData.location, + timeZone: geoData.timeZone + }, request.headers['user-agent'], log) + ) + } + ) + } ) .then( function(){ diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index e48794bd..c139219b 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -895,7 +895,7 @@ "fxa-auth-mailer": { "version": "1.76.0", "from": "git+https://github.com/mozilla/fxa-auth-mailer.git#master", - "resolved": "git+https://github.com/mozilla/fxa-auth-mailer.git#ebce13a050bfa935bd9bdbbf663bbc5cbce463d2", + "resolved": "git+https://github.com/mozilla/fxa-auth-mailer.git#f430684f3f5ad50d0523d6f821384f1f9430b2b0", "dependencies": { "bluebird": { "version": "2.9.34", diff --git a/test/local/account_routes.js b/test/local/account_routes.js index fe4c66bd..ec4a39e3 100644 --- a/test/local/account_routes.js +++ b/test/local/account_routes.js @@ -825,6 +825,10 @@ describe('/account/create', function () { assert.equal(securityEvent.name, 'account.create') assert.equal(securityEvent.uid, uid) assert.equal(securityEvent.ipAddr, clientAddress) + + assert.equal(mockMailer.sendVerifyCode.callCount, 1, 'mailer.sendVerifyCode was called') + assert.equal(mockMailer.sendVerifyCode.getCall(0).args[2].location.city, 'Mountain View') + assert.equal(mockMailer.sendVerifyCode.getCall(0).args[2].location.country, 'United States') }) }) }) diff --git a/test/local/password_routes.js b/test/local/password_routes.js index 06ce11ec..130b4bfd 100644 --- a/test/local/password_routes.js +++ b/test/local/password_routes.js @@ -105,6 +105,11 @@ describe('/password', () => { assert.equal(mockLog.flowEvent.callCount, 2, 'log.flowEvent was called twice') assert.equal(mockLog.flowEvent.args[0][0], 'password.forgot.send_code.start', 'password.forgot.send_code.start event was logged') assert.equal(mockLog.flowEvent.args[1][0], 'password.forgot.send_code.completed', 'password.forgot.send_code.completed event was logged') + + assert.equal(mockMailer.sendRecoveryCode.callCount, 1, 'mailer.sendRecoveryCode was called once') + assert.equal(mockMailer.sendRecoveryCode.getCall(0).args[2].location.city, 'Mountain View') + assert.equal(mockMailer.sendRecoveryCode.getCall(0).args[2].location.country, 'United States') + assert.equal(mockMailer.sendRecoveryCode.getCall(0).args[2].timeZone, 'America/Los_Angeles') }) } ) @@ -289,6 +294,9 @@ describe('/password', () => { assert.equal(mockDB.account.callCount, 1) assert.equal(mockMailer.sendPasswordChangedNotification.callCount, 1) assert.equal(mockMailer.sendPasswordChangedNotification.firstCall.args[0], TEST_EMAIL) + assert.equal(mockMailer.sendPasswordChangedNotification.getCall(0).args[1].location.city, 'Mountain View') + assert.equal(mockMailer.sendPasswordChangedNotification.getCall(0).args[1].location.country, 'United States') + assert.equal(mockMailer.sendPasswordChangedNotification.getCall(0).args[1].timeZone, 'America/Los_Angeles') assert.equal(mockLog.activityEvent.callCount, 1, 'log.activityEvent was called once') var args = mockLog.activityEvent.args[0] diff --git a/test/remote/account_create_tests.js b/test/remote/account_create_tests.js index b9ca65e8..9d9bdbad 100644 --- a/test/remote/account_create_tests.js +++ b/test/remote/account_create_tests.js @@ -53,12 +53,12 @@ describe('remote account create', function() { ) it( - 'create and verify account', + 'create and verify sync account', () => { var email = server.uniqueEmail() var password = 'allyourbasearebelongtous' var client = null - return Client.create(config.publicUrl, email, password) + return Client.create(config.publicUrl, email, password, {service: 'sync'}) .then( function (x) { client = x @@ -77,7 +77,14 @@ describe('remote account create', function() { ) .then( function () { - return server.mailbox.waitForCode(email) + return server.mailbox.waitForEmail(email) + } + ) + .then( + function (emailData) { + assert.equal(emailData.headers['x-template-name'], 'verifySyncEmail') + assert.equal(emailData.html.indexOf('IP address') > -1, true) // Ensure some location data is present + return emailData.headers['x-verify-code'] } ) .then( @@ -532,7 +539,7 @@ describe('remote account create', function() { ) it( - 'create account for non-sync service does not get post-verify email', + 'create account for non-sync service, gets generic sign-up email and does not get post-verify email', () => { var email = server.uniqueEmail() var password = 'allyourbasearebelongtous' @@ -546,7 +553,13 @@ describe('remote account create', function() { ) .then( function () { - return server.mailbox.waitForCode(email) + return server.mailbox.waitForEmail(email) + } + ) + .then( + function (emailData) { + assert.equal(emailData.headers['x-template-name'], 'verifyEmail') + return emailData.headers['x-verify-code'] } ) .then( @@ -586,7 +599,7 @@ describe('remote account create', function() { ) it( - 'create account for unspecified service does not get post-verify email', + 'create account for unspecified service does not get create sync email and no post-verify email', () => { var email = server.uniqueEmail() var password = 'allyourbasearebelongtous' @@ -600,7 +613,14 @@ describe('remote account create', function() { ) .then( function () { - return server.mailbox.waitForCode(email) + return server.mailbox.waitForEmail(email) + } + ) + .then( + function (emailData) { + assert.equal(emailData.headers['x-template-name'], 'verifyEmail') + assert.equal(emailData.html.indexOf('IP address') === -1, true) // Does not contain location data + return emailData.headers['x-verify-code'] } ) .then( diff --git a/test/remote/password_change_tests.js b/test/remote/password_change_tests.js index c0169bd2..ec94bd01 100644 --- a/test/remote/password_change_tests.js +++ b/test/remote/password_change_tests.js @@ -222,6 +222,7 @@ describe('remote password change', function() { var link = emailData.headers['x-link'] var query = url.parse(link, true).query assert.ok(query.email, 'email is in the link') + assert.equal(emailData.html.indexOf('IP address') > -1, true, 'contains ip location data') } ) .then( diff --git a/test/remote/password_forgot_tests.js b/test/remote/password_forgot_tests.js index 6cca6838..98486f01 100644 --- a/test/remote/password_forgot_tests.js +++ b/test/remote/password_forgot_tests.js @@ -48,7 +48,13 @@ describe('remote password forgot', function() { ) .then( function () { - return server.mailbox.waitForCode(email) + return server.mailbox.waitForEmail(email) + } + ) + .then( + function (emailData) { + assert.equal(emailData.html.indexOf('IP address') > -1, true, 'contains ip location data') + return emailData.headers['x-recovery-code'] } ) .then(