From 2a5d3d0b77e0e6a111781caa97173f34e08b95b7 Mon Sep 17 00:00:00 2001
From: Phil Booth <pmbooth@gmail.com>
Date: Wed, 8 Mar 2017 16:40:21 +0000
Subject: [PATCH] fix(metrics): log locale instead of accept languages on flow
 events

https://github.com/mozilla/fxa-auth-server/pull/1702

r=vbudhram,vladikoff
---
 bin/key_server.js                    | 11 ++++--
 lib/metrics/events.js                | 10 +++---
 lib/routes/account.js                |  3 +-
 lib/senders/email.js                 |  2 +-
 lib/senders/index.js                 |  8 ++---
 lib/senders/legacy_index.js          | 18 +++-------
 lib/senders/sms.js                   |  2 +-
 lib/senders/translator.js            | 15 ++++++---
 lib/server.js                        | 12 +++++--
 scripts/sms/balance.js               |  5 ++-
 scripts/sms/send.js                  |  5 ++-
 scripts/write-emails-to-disk.js      |  8 +++--
 test/local/lib/mailer_locales.js     | 11 ++++--
 test/local/lib/senders/translator.js | 50 +++++++++++++++++-----------
 test/local/lib/server.js             | 45 +++++++++++++++++++++++--
 test/local/metrics/events.js         | 13 +++++---
 test/local/routes/account.js         |  4 +--
 test/mocks.js                        |  4 ++-
 18 files changed, 152 insertions(+), 74 deletions(-)

diff --git a/bin/key_server.js b/bin/key_server.js
index e81e2e16c..2fd3bb4c3 100644
--- a/bin/key_server.js
+++ b/bin/key_server.js
@@ -2,6 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+'use strict'
+
 // Only `require()` the newrelic module if explicity enabled.
 // If required, modules will be instrumented.
 require('../lib/newrelic')()
@@ -74,7 +76,12 @@ function main() {
     log.stat(Password.stat())
   }
 
-  require('../lib/senders')(config, log)
+  let translator
+  require('../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+    .then(result => {
+      translator = result
+      return require('../lib/senders')(log, config, translator)
+    })
     .then(
       function(result) {
         senders = result
@@ -108,7 +115,7 @@ function main() {
                 config,
                 customs
               )
-              server = Server.create(log, error, config, routes, db)
+              server = Server.create(log, error, config, routes, db, translator)
 
               server.start(
                 function (err) {
diff --git a/lib/metrics/events.js b/lib/metrics/events.js
index 874c3b51f..f83da3438 100644
--- a/lib/metrics/events.js
+++ b/lib/metrics/events.js
@@ -144,7 +144,7 @@ module.exports = log => {
 
     return request.gatherMetricsContext({
       event: event,
-      locale: coalesceLocale(optionalData, request),
+      locale: marshallLocale(request),
       uid: coalesceUid(optionalData, request),
       userAgent: request.headers['user-agent']
     }).then(data => {
@@ -175,12 +175,10 @@ function optionallySetService (data, request) {
     (request.query && request.query.service)
 }
 
-function coalesceLocale (data, request) {
-  if (data && data.locale) {
-    return data.locale
+function marshallLocale (request) {
+  if (request.app && request.app.locale) {
+    return `${request.app.locale}${request.app.isLocaleAcceptable ? '' : '.default'}`
   }
-
-  return request.app && request.app.acceptLanguage
 }
 
 function coalesceUid (data, request) {
diff --git a/lib/routes/account.js b/lib/routes/account.js
index 01d18f165..09f38b31c 100644
--- a/lib/routes/account.js
+++ b/lib/routes/account.js
@@ -221,8 +221,7 @@ module.exports = function (
               account = result
 
               return request.emitMetricsEvent('account.created', {
-                uid: account.uid.toString('hex'),
-                locale: account.locale
+                uid: account.uid.toString('hex')
               })
             }
           )
diff --git a/lib/senders/email.js b/lib/senders/email.js
index 546b47f0a..c595b43b7 100644
--- a/lib/senders/email.js
+++ b/lib/senders/email.js
@@ -118,7 +118,7 @@ module.exports = function (log) {
     this.supportUrl = config.supportUrl
     this.syncUrl = config.syncUrl
     this.templates = templates
-    this.translator = translator
+    this.translator = translator.getTranslator
     this.verificationUrl = config.verificationUrl
     this.verifyLoginUrl = config.verifyLoginUrl
   }
diff --git a/lib/senders/index.js b/lib/senders/index.js
index f81964ab1..2425180d5 100644
--- a/lib/senders/index.js
+++ b/lib/senders/index.js
@@ -8,21 +8,19 @@ var P = require('../promise')
 // This indirection exists to accommodate different config properties
 // in the old auth mailer. If/when the two config files are merged and
 // there's nothing left that imports mailer/config, it is safe to merge
-// legacy_index.js and this file into one. Be careful not to mix the args
-// up when you do that, they expect config and log in a different order.
+// legacy_index.js and this file into one.
 var createSenders = require('./legacy_index')
 
-module.exports = function (config, log, sender) {
+module.exports = function (log, config, translator, sender) {
   var defaultLanguage = config.i18n.defaultLanguage
 
   return createSenders(
     log,
     {
-      locales: config.i18n.supportedLanguages,
-      defaultLanguage: defaultLanguage,
       mail: config.smtp,
       sms: config.sms
     },
+    translator,
     sender
   )
   .then(
diff --git a/lib/senders/legacy_index.js b/lib/senders/legacy_index.js
index 5ce54ce4e..2f712eea7 100644
--- a/lib/senders/legacy_index.js
+++ b/lib/senders/legacy_index.js
@@ -6,27 +6,17 @@
 // Those files should import this module rather than its sibling index.js.
 // If/when we eliminate the old mailer config and everything is importing
 // index.js, we can merge this into there and get rid of the indirection.
-// Be careful when doing that btw, they expect the log and config arguments
-// in a different order.
 
-var P = require('../promise')
 var createMailer = require('./email')
 var createSms = require('./sms')
 
-module.exports = function (log, config, sender) {
+module.exports = function (log, config, translator, sender) {
   var Mailer = createMailer(log)
-  return P.all(
-    [
-      require('./translator')(config.locales, config.defaultLanguage),
-      require('./templates')()
-    ]
-  )
-  .spread(
-    function (translator, templates) {
+  return require('./templates')()
+    .then(function (templates) {
       return {
         email: new Mailer(translator, templates, config.mail, sender),
         sms: createSms(log, translator, templates, config.sms)
       }
-    }
-  )
+    })
 }
diff --git a/lib/senders/sms.js b/lib/senders/sms.js
index 553cad63e..03aa2510e 100644
--- a/lib/senders/sms.js
+++ b/lib/senders/sms.js
@@ -97,7 +97,7 @@ module.exports = function (log, translator, templates, smsConfig) {
 
     return template({
       link: smsConfig[templateName + 'Link'],
-      translator: translator(acceptLanguage)
+      translator: translator.getTranslator(acceptLanguage)
     }).text
   }
 }
diff --git a/lib/senders/translator.js b/lib/senders/translator.js
index bf6da75d3..25be20714 100644
--- a/lib/senders/translator.js
+++ b/lib/senders/translator.js
@@ -43,11 +43,18 @@ module.exports = function (locales, defaultLanguage) {
         languageTranslations[language] = translator
       }
 
-      return function translator(acceptLanguage) {
-        var languages = i18n.parseAcceptLanguage(acceptLanguage)
-        var best = i18n.normalizeLanguage(i18n.bestLanguage(languages, supportedLanguages, defaultLanguage))
+      return {
+        getTranslator: function (acceptLanguage) {
+          return languageTranslations[getLocale(acceptLanguage)]
+        },
+
+        getLocale: getLocale
+      }
 
-        return languageTranslations[best]
+      function getLocale (acceptLanguage) {
+        var languages = i18n.parseAcceptLanguage(acceptLanguage)
+        var bestLanguage = i18n.bestLanguage(languages, supportedLanguages, defaultLanguage)
+        return i18n.normalizeLanguage(bestLanguage)
       }
     }
   )
diff --git a/lib/server.js b/lib/server.js
index e37ad58e4..140bbafda 100644
--- a/lib/server.js
+++ b/lib/server.js
@@ -43,7 +43,7 @@ function logEndpointErrors(response, log) {
   }
 }
 
-function create(log, error, config, routes, db) {
+function create(log, error, config, routes, db, translator) {
 
   // Hawk needs to calculate request signatures based on public URL,
   // not the local URL to which it is bound.
@@ -284,7 +284,11 @@ function create(log, error, config, routes, db) {
       request.app.remoteAddressChain = xff
       request.app.clientAddress = xff[clientAddressIndex]
 
-      request.app.acceptLanguage = trimLocale(request.headers['accept-language'])
+      const acceptLanguage = trimLocale(request.headers['accept-language'])
+      request.app.acceptLanguage = acceptLanguage
+      const locale = translator.getLocale(acceptLanguage)
+      request.app.locale = locale
+      request.app.isLocaleAcceptable = isLocaleAcceptable(locale, acceptLanguage)
 
       if (request.headers.authorization) {
         // Log some helpful details for debugging authentication problems.
@@ -341,6 +345,10 @@ function create(log, error, config, routes, db) {
   return server
 }
 
+function isLocaleAcceptable (locale, acceptLanguage) {
+  return RegExp(`^(?:.+, *)*${locale}(?:[,-].+)*$`, 'i').test(acceptLanguage)
+}
+
 module.exports = {
   create: create,
   // Functions below exported for testing
diff --git a/scripts/sms/balance.js b/scripts/sms/balance.js
index 15b5ebe53..64b6e30e2 100755
--- a/scripts/sms/balance.js
+++ b/scripts/sms/balance.js
@@ -15,7 +15,10 @@ if (config.sms.apiKey === NOT_SET || config.sms.apiSecret === NOT_SET) {
 
 const log = require('../../lib/log')(config.log.level, 'sms-balance')
 
-require('../../lib/senders')(config, log)
+require('../../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+  .then(translator => {
+    return require('../../lib/senders')(log, config, translator)
+  })
   .then(senders => {
     return senders.sms.balance()
   })
diff --git a/scripts/sms/send.js b/scripts/sms/send.js
index b4f2777b2..7072fee37 100755
--- a/scripts/sms/send.js
+++ b/scripts/sms/send.js
@@ -16,7 +16,10 @@ if (config.sms.apiKey === NOT_SET || config.sms.apiSecret === NOT_SET) {
 const args = parseArgs()
 const log = require('../../lib/log')(config.log.level, 'send-sms')
 
-require('../../lib/senders')(config, log)
+require('../../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+  .then(translator => {
+    return require('../../lib/senders')(log, config, translator)
+  })
   .then(senders => {
     return senders.sms.send.apply(null, args)
   })
diff --git a/scripts/write-emails-to-disk.js b/scripts/write-emails-to-disk.js
index d9196f5f6..375521d7a 100644
--- a/scripts/write-emails-to-disk.js
+++ b/scripts/write-emails-to-disk.js
@@ -27,7 +27,7 @@
  */
 
 var P = require('bluebird')
-var config = require('../config')
+const config = require('../config').getProperties()
 const createSenders = require('../lib/senders')
 var fs = require('fs')
 const log = require('../lib/senders/legacy_log')(require('../lib/senders/log')('server'))
@@ -52,8 +52,10 @@ var mailSender = {
   close: function () {}
 }
 
-
-createSenders(config.getProperties(), log, mailSender)
+require('../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+  .then(translator => {
+    return createSenders(log, config, translator, mailSender)
+  })
   .then((senders) => {
     const mailer = senders.email
     checkMessageType(mailer, messageToSend)
diff --git a/test/local/lib/mailer_locales.js b/test/local/lib/mailer_locales.js
index 00923dbe3..338512769 100644
--- a/test/local/lib/mailer_locales.js
+++ b/test/local/lib/mailer_locales.js
@@ -4,15 +4,20 @@
 
 'use strict'
 
+const ROOT_DIR = '../../..'
+
 const assert = require('insist')
-var config = require('../../../config/index').getProperties()
-var log = {}
+const config = require(`${ROOT_DIR}/config/index`).getProperties()
+const log = {}
 
 describe('mailer locales', () => {
 
   let mailer
   before(() => {
-    return require('../../../lib/senders')(config, log)
+    return require(`${ROOT_DIR}/lib/senders/translator`)(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+      .then(translator => {
+        return require(`${ROOT_DIR}/lib/senders`)(log, config, translator)
+      })
       .then(result => {
         mailer = result.email
       })
diff --git a/test/local/lib/senders/translator.js b/test/local/lib/senders/translator.js
index ac8e2c8cd..2be8da4ac 100644
--- a/test/local/lib/senders/translator.js
+++ b/test/local/lib/senders/translator.js
@@ -2,25 +2,37 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+'use strict'
+
 const assert = require('insist')
 
 require('../../../../lib/senders/translator')(['en', 'pt_br', 'DE', 'ES_AR', 'ES_cl'], 'en')
-.then(
-  function (translator) {
-    it(
-      'translator works with upper and lowercase languages',
-      function (t) {
-        var x = translator('PT-br,DE')
-        assert.equal(x.language, 'pt-BR')
-        x = translator('bu-ll,es-ar')
-        assert.equal(x.language, 'es-AR')
-        x = translator('es-CL')
-        assert.equal(x.language, 'es-CL')
-        x = translator('en-US')
-        assert.equal(x.language, 'en')
-        x = translator('db-LB') // a locale that does not exist
-        assert.equal(x.language, 'en')
-      }
-    )
-  }
-)
+.then(translator => {
+  it('returns the correct interface', () => {
+    assert.equal(typeof translator, 'object')
+    assert.equal(Object.keys(translator).length, 2)
+    assert.equal(typeof translator.getTranslator, 'function')
+    assert.equal(typeof translator.getLocale, 'function')
+  })
+
+  it('getTranslator works with upper and lowercase languages', () => {
+    let x = translator.getTranslator('PT-br,DE')
+    assert.equal(x.language, 'pt-BR')
+    x = translator.getTranslator('bu-ll,es-ar')
+    assert.equal(x.language, 'es-AR')
+    x = translator.getTranslator('es-CL')
+    assert.equal(x.language, 'es-CL')
+    x = translator.getTranslator('en-US')
+    assert.equal(x.language, 'en')
+    x = translator.getTranslator('db-LB') // a locale that does not exist
+    assert.equal(x.language, 'en')
+  })
+
+  it('getLocale works with upper and lowercase languages', () => {
+    assert.equal(translator.getLocale('PT-br,DE'), 'pt-BR')
+    assert.equal(translator.getLocale('bu-ll,es-ar'), 'es-AR')
+    assert.equal(translator.getLocale('es-CL'), 'es-CL')
+    assert.equal(translator.getLocale('en-US'), 'en')
+    assert.equal(translator.getLocale('db-LB'), 'en')
+  })
+})
diff --git a/test/local/lib/server.js b/test/local/lib/server.js
index 412779af2..31c145de2 100644
--- a/test/local/lib/server.js
+++ b/test/local/lib/server.js
@@ -10,6 +10,7 @@ const error = require('../../../lib/error')
 const hapi = require('hapi')
 const mocks = require('../../mocks')
 const server = require('../../../lib/server')
+const sinon = require('sinon')
 
 describe('lib/server', () => {
   describe('trimLocale', () => {
@@ -60,14 +61,18 @@ describe('lib/server', () => {
   })
 
   describe('create:', () => {
-    let log, config, routes, db, instance, response
+    let log, config, routes, db, instance, response, translator
 
     beforeEach(() => {
       log = mocks.spyLog()
       config = getConfig()
       routes = getRoutes()
       db = mocks.mockDB()
-      instance = server.create(log, error, config, routes, db)
+      translator = {
+        getTranslator: sinon.spy(() => ({ en: { format: () => {}, language: 'en' } })),
+        getLocale: sinon.spy(() => 'en')
+      }
+      instance = server.create(log, error, config, routes, db, translator)
     })
 
     it('returned a hapi Server instance', () => {
@@ -86,10 +91,13 @@ describe('lib/server', () => {
         assert.equal(log.summary.callCount, 0)
       })
 
-      describe('successful request:', () => {
+      describe('successful request, acceptable locale:', () => {
         beforeEach(() => {
           response = 'ok'
           return instance.inject({
+            headers: {
+              'accept-language': 'fr-CH, fr;q=0.9, en-GB, en;q=0.5'
+            },
             method: 'POST',
             url: '/account/create',
             payload: {}
@@ -103,6 +111,8 @@ describe('lib/server', () => {
           assert.equal(args[0], 'server.onRequest')
           assert.ok(args[1])
           assert.equal(args[1].path, '/account/create')
+          assert.equal(args[1].app.locale, 'en')
+          assert.equal(args[1].app.isLocaleAcceptable, true)
         })
 
         it('called log.summary correctly', () => {
@@ -123,6 +133,35 @@ describe('lib/server', () => {
         })
       })
 
+      describe('successful request, unacceptable locale:', () => {
+        beforeEach(() => {
+          response = 'ok'
+          return instance.inject({
+            headers: {
+              'accept-language': 'fr-CH, fr;q=0.9'
+            },
+            method: 'POST',
+            url: '/account/create',
+            payload: {}
+          })
+        })
+
+        it('called log.begin correctly', () => {
+          assert.equal(log.begin.callCount, 1)
+          const args = log.begin.args[0]
+          assert.equal(args[1].app.locale, 'en')
+          assert.equal(args[1].app.isLocaleAcceptable, false)
+        })
+
+        it('called log.summary once', () => {
+          assert.equal(log.summary.callCount, 1)
+        })
+
+        it('did not call log.error', () => {
+          assert.equal(log.error.callCount, 0)
+        })
+      })
+
       describe('unsuccessful request:', () => {
         beforeEach(() => {
           response = error.requestBlocked()
diff --git a/test/local/metrics/events.js b/test/local/metrics/events.js
index 1418ae19f..00764ae0b 100644
--- a/test/local/metrics/events.js
+++ b/test/local/metrics/events.js
@@ -170,7 +170,8 @@ describe('metrics/events', () => {
     const metricsContext = mocks.mockMetricsContext()
     const request = {
       app: {
-        acceptLanguage: 'en'
+        isLocaleAcceptable: false,
+        locale: 'en'
       },
       auth: {
         credentials: {
@@ -203,7 +204,7 @@ describe('metrics/events', () => {
           flow_id: 'bar',
           flow_time: 1000,
           flowCompleteSignal: 'account.signed',
-          locale: 'en',
+          locale: 'en.default',
           time,
           uid: 'deadbeef',
           userAgent: 'foo'
@@ -224,6 +225,10 @@ describe('metrics/events', () => {
     sinon.stub(Date, 'now', () => time)
     const metricsContext = mocks.mockMetricsContext()
     const request = {
+      app: {
+        isLocaleAcceptable: true,
+        locale: 'fr'
+      },
       clearMetricsContext: metricsContext.clear,
       gatherMetricsContext: metricsContext.gather,
       headers: {
@@ -247,7 +252,7 @@ describe('metrics/events', () => {
           flow_id: 'bar',
           flow_time: 2000,
           flowCompleteSignal: 'account.reminder',
-          locale: 'baz',
+          locale: 'fr',
           time,
           uid: 'qux',
           userAgent: 'foo'
@@ -257,7 +262,7 @@ describe('metrics/events', () => {
           flow_id: 'bar',
           flow_time: 2000,
           flowCompleteSignal: 'account.reminder',
-          locale: 'baz',
+          locale: 'fr',
           time,
           uid: 'qux',
           userAgent: 'foo'
diff --git a/test/local/routes/account.js b/test/local/routes/account.js
index 8fac4187f..5ac0d1d0a 100644
--- a/test/local/routes/account.js
+++ b/test/local/routes/account.js
@@ -204,6 +204,7 @@ describe('/account/create', () => {
     mockLog.error = sinon.spy()
     const mockMetricsContext = mocks.mockMetricsContext()
     const mockRequest = mocks.mockRequest({
+      locale: 'en-GB',
       log: mockLog,
       metricsContext: mockMetricsContext,
       payload: {
@@ -288,7 +289,6 @@ describe('/account/create', () => {
       assert.equal(args.length, 1, 'log.activityEvent was passed one argument')
       assert.deepEqual(args[0], {
         event: 'account.created',
-        locale: 'en',
         service: 'sync',
         userAgent: 'test user-agent',
         uid: uid.toString('hex')
@@ -302,7 +302,7 @@ describe('/account/create', () => {
         flowCompleteSignal: 'account.signed',
         flow_time: now - mockRequest.payload.metricsContext.flowBeginTime,
         flow_id: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
-        locale: 'en',
+        locale: 'en-GB',
         time: now,
         uid: uid.toString('hex'),
         userAgent: 'test user-agent'
diff --git a/test/mocks.js b/test/mocks.js
index fdbfa8042..6a5825a47 100644
--- a/test/mocks.js
+++ b/test/mocks.js
@@ -353,7 +353,9 @@ function mockRequest (data) {
   return {
     app: {
       acceptLanguage: 'en-US',
-      clientAddress: data.clientAddress || '63.245.221.32' // MTV
+      clientAddress: data.clientAddress || '63.245.221.32',
+      isLocaleAcceptable: true,
+      locale: data.locale || 'en-US'
     },
     auth: {
       credentials: data.credentials