From 66e442660dac0c7a3aa4c3e408d8ba65b9af144c Mon Sep 17 00:00:00 2001 From: vladikoff Date: Tue, 22 May 2018 12:27:26 -0400 Subject: [PATCH 1/2] feat(metrics): support iframeless flow, add tests --- app/scripts/lib/metrics.js | 7 +- app/scripts/models/flow.js | 131 ++++++++++++------------ server/bin/fxa-content-server.js | 13 +++ server/config/local.json-dist | 1 + server/lib/configuration.js | 6 ++ server/lib/flow-event.js | 7 +- server/lib/routes.js | 1 + server/lib/routes/get-metrics-flow.js | 56 ++++++++++ server/lib/routes/post-metrics.js | 4 +- tests/server/flow-event.js | 2 +- tests/server/routes/get-metrics-flow.js | 112 ++++++++++++++++++++ tests/server/routes/post-metrics.js | 12 ++- tests/tests_server.js | 3 +- 13 files changed, 281 insertions(+), 74 deletions(-) create mode 100644 server/lib/routes/get-metrics-flow.js create mode 100644 tests/server/routes/get-metrics-flow.js diff --git a/app/scripts/lib/metrics.js b/app/scripts/lib/metrics.js index 2afd857cf5..5af09cb3bc 100644 --- a/app/scripts/lib/metrics.js +++ b/app/scripts/lib/metrics.js @@ -206,8 +206,9 @@ define(function (require, exports, module) { } const flowModel = new Flow({ + metrics: this, sentryMetrics: this._sentryMetrics, - window: this._window + window: this._window, }); if (flowModel.has('flowId')) { @@ -458,6 +459,10 @@ define(function (require, exports, module) { } }, + markEventLogged: function (eventName) { + this._eventMemory[eventName] = true; + }, + /** * Start a timer * diff --git a/app/scripts/models/flow.js b/app/scripts/models/flow.js index 8f1a226819..c4b3241dbe 100644 --- a/app/scripts/models/flow.js +++ b/app/scripts/models/flow.js @@ -11,79 +11,84 @@ * If unavailable there, fetch from data attributes in the DOM. */ -define(function (require, exports, module) { - 'use strict'; - const $ = require('jquery'); - const AuthErrors = require('../lib/auth-errors'); - const Backbone = require('backbone'); - const Cocktail = require('cocktail'); - const ErrorUtils = require('../lib/error-utils'); - const ResumeTokenMixin = require('./mixins/resume-token'); - const SearchParamMixin = require('./mixins/search-param'); - const vat = require('../lib/vat'); +const $ = require('jquery'); +const AuthErrors = require('../lib/auth-errors'); +const Backbone = require('backbone'); +const Cocktail = require('cocktail'); +const ErrorUtils = require('../lib/error-utils'); +const ResumeTokenMixin = require('./mixins/resume-token'); +const SearchParamMixin = require('./mixins/search-param'); +const vat = require('../lib/vat'); +const Url = require('../lib/url'); - var Model = Backbone.Model.extend({ - initialize (options) { - options = options || {}; +var Model = Backbone.Model.extend({ + initialize (options) { + options = options || {}; - this.sentryMetrics = options.sentryMetrics; - this.window = options.window || window; - - // We should either get both fields from the resume token, or neither. - // Getting one without the other is an error. - this.populateFromStringifiedResumeToken(this.getSearchParam('resume')); - if (this.has('flowId')) { - if (! this.has('flowBegin')) { - this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowBegin')); - } - } else if (this.has('flowBegin')) { - this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowId')); - } else { - this.populateFromDataAttribute('flowId'); - this.populateFromDataAttribute('flowBegin'); + this.sentryMetrics = options.sentryMetrics; + this.window = options.window || window; + this.metrics = options.metrics; + const urlParams = Url.searchParams(this.window.location.search); + // We should either get both fields from the resume token, or neither. + // Getting one without the other is an error. + this.populateFromStringifiedResumeToken(this.getSearchParam('resume')); + if (this.has('flowId')) { + if (! this.has('flowBegin')) { + this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowBegin')); } - }, + } else if (this.has('flowBegin')) { + this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowId')); + } else if (urlParams.flow_begin_time && urlParams.flow_id) { + this.set('flowId', urlParams.flow_id); + this.set('flowBegin', urlParams.flow_begin_time); + // if the urlParams are set that means flow.begin was already logged on the server-side + // therefore we mark flow.begin logged + this.metrics.markEventLogged('flow.begin'); + } else { + this.populateFromDataAttribute('flowId'); + this.populateFromDataAttribute('flowBegin'); + } + }, - defaults: { - flowBegin: null, - flowId: null - }, + defaults: { + flowBegin: null, + flowId: null + }, - populateFromDataAttribute (attribute) { - var data = $(this.window.document.body).data(attribute); - if (! data) { - this.logError(AuthErrors.toMissingDataAttributeError(attribute)); + populateFromDataAttribute (attribute) { + var data = $(this.window.document.body).data(attribute); + if (! data) { + this.logError(AuthErrors.toMissingDataAttributeError(attribute)); + } else { + const result = this.resumeTokenSchema[attribute].validate(data); + if (result.error) { + this.logError(AuthErrors.toInvalidDataAttributeError(attribute)); } else { - const result = this.resumeTokenSchema[attribute].validate(data); - if (result.error) { - this.logError(AuthErrors.toInvalidDataAttributeError(attribute)); - } else { - this.set(attribute, result.value); - } + this.set(attribute, result.value); } - }, + } + }, - logError (error) { - return ErrorUtils.captureError(error, this.sentryMetrics); - }, + logError (error) { + return ErrorUtils.captureError(error, this.sentryMetrics); + }, - resumeTokenFields: ['flowId', 'flowBegin'], + resumeTokenFields: ['flowId', 'flowBegin'], - resumeTokenSchema: { - flowBegin: vat.number().test(function (value) { - // Integers only - return value === Math.round(value); - }), - flowId: vat.hex().len(64) - } - }); + resumeTokenSchema: { + flowBegin: vat.number().test(function (value) { + // Integers only + return value === Math.round(value); + }), + flowId: vat.hex().len(64) + } +}); - Cocktail.mixin( - Model, - ResumeTokenMixin, - SearchParamMixin - ); +Cocktail.mixin( + Model, + ResumeTokenMixin, + SearchParamMixin +); - module.exports = Model; -}); +module.exports = Model; diff --git a/server/bin/fxa-content-server.js b/server/bin/fxa-content-server.js index f5be2d7722..83ace2b72e 100755 --- a/server/bin/fxa-content-server.js +++ b/server/bin/fxa-content-server.js @@ -166,6 +166,19 @@ function makeApp() { // it's a four-oh-four not found. app.use(fourOhFour); + // Handler for CORS errors + app.use((err, req, res, next) => { + if (err.message === 'CORS Error') { + res.status(401).json({ + error: 'Unauthorized', + message: 'CORS Error', + statusCode: 401 + }); + } else { + next(err); + } + }); + // The error handler must be before any other error middleware app.use(raven.ravenModule.errorHandler()); diff --git a/server/config/local.json-dist b/server/config/local.json-dist index e44c41b5ee..0d7c2804d0 100644 --- a/server/config/local.json-dist +++ b/server/config/local.json-dist @@ -21,6 +21,7 @@ "enabled": true }, "static_directory": "app", + "allowed_metrics_flow_cors_origins": ["http://127.0.0.1:8001"], "allowed_parent_origins": ["http://127.0.0.1:8080"], "csp": { "enabled": true, diff --git a/server/lib/configuration.js b/server/lib/configuration.js index 3680c85e34..a54be68183 100644 --- a/server/lib/configuration.js +++ b/server/lib/configuration.js @@ -18,6 +18,12 @@ const conf = module.exports = convict({ doc: 'context query parameters allowed to embed FxA within an IFRAME', format: Array }, + allowed_metrics_flow_cors_origins: { + default: [], + doc: 'Origins that are allowed to request the /metrics-flow endpoint', + env: 'ALLOWED_METRICS_FLOW_ORIGINS', + format: Array + }, allowed_parent_origins: { default: [], doc: 'Origins that are allowed to embed FxA within an IFRAME', diff --git a/server/lib/flow-event.js b/server/lib/flow-event.js index e2ab8d7d14..c64b586e5b 100644 --- a/server/lib/flow-event.js +++ b/server/lib/flow-event.js @@ -108,7 +108,7 @@ const PERFORMANCE_TIMINGS = [ const AUTH_VIEWS = new Set([ 'enter-email', 'force-auth', 'signin', 'signup' ]); -module.exports = (req, metrics, requestReceivedTime) => { +const metricsRequest = (req, metrics, requestReceivedTime) => { if (IS_DISABLED || ! isValidFlowData(metrics, requestReceivedTime)) { return; } @@ -292,3 +292,8 @@ function optionallySetFallbackData (eventData, key, fallback) { eventData[key] = limitLength(fallback); } } + +module.exports = { + logFlowEvent, + metricsRequest +}; diff --git a/server/lib/routes.js b/server/lib/routes.js index 2c86b96c37..33d01189ce 100644 --- a/server/lib/routes.js +++ b/server/lib/routes.js @@ -45,6 +45,7 @@ module.exports = function (config, i18n) { require('./routes/get-lbheartbeat')(), require('./routes/get-openid-configuration')(config), require('./routes/get-version.json'), + require('./routes/get-metrics-flow')(config), require('./routes/post-metrics')(), require('./routes/post-metrics-errors')(), require('./routes/redirect-complete-to-verified')(), diff --git a/server/lib/routes/get-metrics-flow.js b/server/lib/routes/get-metrics-flow.js new file mode 100644 index 0000000000..58459c7468 --- /dev/null +++ b/server/lib/routes/get-metrics-flow.js @@ -0,0 +1,56 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * 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 flowMetrics = require('../flow-metrics'); +const logFlowEvent = require('../flow-event').logFlowEvent; +const logger = require('../logging/log')('server.get-metrics-flow'); + +module.exports = function (config) { + const FLOW_ID_KEY = config.get('flow_id_key'); + const FLOW_EVENT_NAME = 'flow.begin'; + const ALLOWED_CORS_ORIGINS = config.get('allowed_metrics_flow_cors_origins'); + const CORS_OPTIONS = { + methods: 'GET', + origin: function corsOrigin(origin, callback) { + if (ALLOWED_CORS_ORIGINS.includes(origin)) { + callback(null, true); + } else { + logger.info('request.metrics-flow.bad-origin', origin); + callback(new Error('CORS Error')); + } + }, + preflightContinue: false + }; + + const route = {}; + route.method = 'get'; + route.path = '/metrics-flow'; + route.cors = CORS_OPTIONS; + + route.process = function (req, res) { + const flowEventData = flowMetrics.create(FLOW_ID_KEY, req.headers['user-agent']); + const flowBeingTime = flowEventData.flowBeginTime; + const flowId = flowEventData.flowId; + const metricsData = req.query || {}; + + metricsData.flowId = flowId; + + logFlowEvent({ + flowTime: flowBeingTime, + time: flowBeingTime, + type: FLOW_EVENT_NAME + }, metricsData, req); + + // charset must be set on json responses. + res.charset = 'utf-8'; + res.json({ + flowBeginTime: flowEventData.flowBeginTime, + flowId: flowId + }); + }; + + return route; +}; diff --git a/server/lib/routes/post-metrics.js b/server/lib/routes/post-metrics.js index f04347321c..b777f54a55 100644 --- a/server/lib/routes/post-metrics.js +++ b/server/lib/routes/post-metrics.js @@ -6,7 +6,7 @@ const _ = require('lodash'); const config = require('../configuration'); -const flowEvent = require('../flow-event'); +const flowMetricsRequest = require('../flow-event').metricsRequest; const GACollector = require('../ga-collector'); const joi = require('joi'); const logger = require('../logging/log')('server.post-metrics'); @@ -174,7 +174,7 @@ module.exports = function () { } ga.write(metrics); - flowEvent(req, metrics, requestReceivedTime); + flowMetricsRequest(req, metrics, requestReceivedTime); }); } }; diff --git a/tests/server/flow-event.js b/tests/server/flow-event.js index 2035065539..bcb6a119aa 100644 --- a/tests/server/flow-event.js +++ b/tests/server/flow-event.js @@ -44,7 +44,7 @@ registerSuite('flow-event', { './amplitude': mocks.amplitude, './configuration': mocks.config, './flow-metrics': mocks.flowMetrics - }); + }).metricsRequest; }, afterEach: function() { diff --git a/tests/server/routes/get-metrics-flow.js b/tests/server/routes/get-metrics-flow.js new file mode 100644 index 0000000000..7660590d9f --- /dev/null +++ b/tests/server/routes/get-metrics-flow.js @@ -0,0 +1,112 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * 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/. */ +const {registerSuite} = intern.getInterface('object'); +const assert = intern.getPlugin('chai').assert; +const proxyquire = require('proxyquire'); +const sinon = require('sinon'); +let instance, request, response, route, mocks, sandbox; + +registerSuite('routes/get-metrics-flow', { + before: function () { + sandbox = sinon.sandbox.create(); + mocks = { + config: { + get (key) { + switch (key) { + case 'allowed_metrics_flow_cors_origins': + return ['https://mozilla.org']; + case 'flow_id_key': + return 'foo'; + case 'flow_id_expiry': + return 7200000; + } + } + }, + flowEvent: { + logFlowEvent: sandbox.spy() + }, + }; + route = proxyquire('../../../server/lib/routes/get-metrics-flow', { + '../flow-event': mocks.flowEvent, + }); + instance = route(mocks.config); + + request = { + headers: {} + }; + response = {json: sinon.spy()}; + }, + + afterEach: function () { + sandbox.reset(); + }, + + tests: { + 'route interface is correct': function () { + assert.isFunction(route); + assert.lengthOf(route, 1); + }, + + 'instance interface is correct': function () { + assert.isObject(instance); + assert.lengthOf(Object.keys(instance), 4); + assert.equal(instance.method, 'get'); + assert.equal(instance.path, '/metrics-flow'); + assert.isObject(instance.cors); + assert.isFunction(instance.cors.origin); + assert.equal(instance.cors.methods, 'GET'); + assert.isFunction(instance.process); + assert.lengthOf(instance.process, 2); + }, + + 'response.json was called correctly': function () { + instance.process(request, response); + assert.equal(response.json.callCount, 1); + const args = response.json.args[0]; + assert.lengthOf(args, 1); + assert.ok(args[0].flowBeginTime); + assert.ok(args[0].flowId); + + assert.equal(mocks.flowEvent.logFlowEvent.callCount, 1); + const argsFlowEvent = mocks.flowEvent.logFlowEvent.args[0]; + assert.equal(argsFlowEvent.length, 3); + }, + 'supports query params': function () { + request = { + headers: {}, + query: { + entrypoint: 'zoo' + } + }; + instance.process(request, response); + + assert.equal(mocks.flowEvent.logFlowEvent.callCount, 1); + const argsFlowEvent = mocks.flowEvent.logFlowEvent.args[0]; + const eventData = argsFlowEvent[0]; + const metricsData = argsFlowEvent[1]; + assert.ok(eventData.flowTime); + assert.ok(eventData.time); + assert.equal(eventData.type, 'flow.begin'); + assert.equal(metricsData.entrypoint, 'zoo'); + assert.ok(metricsData.flowId); + }, + + 'validates CORS': function () { + const dfd = this.async(1000); + const corsFunc = instance.cors.origin; + + corsFunc('https://google.com', (err, result) => { + assert.equal(err.message, 'CORS Error'); + assert.equal(result, null); + corsFunc('https://mozilla.org', (err, result) => { + assert.equal(err, null); + assert.equal(result, true); + dfd.resolve(); + }); + }); + + return dfd; + } + } +}); diff --git a/tests/server/routes/post-metrics.js b/tests/server/routes/post-metrics.js index 028ed93b92..76de3abbe2 100644 --- a/tests/server/routes/post-metrics.js +++ b/tests/server/routes/post-metrics.js @@ -32,7 +32,9 @@ registerSuite('routes/post-metrics', { return {}; } }, - flowEvent: sandbox.spy(), + flowEvent: { + metricsRequest: sandbox.spy() + }, gaCollector: { write: sandbox.spy() }, @@ -220,9 +222,9 @@ registerSuite('routes/post-metrics', { assert.equal(args[0], mocks.request.body); }, - 'flowEvent was called correctly': function () { - assert.strictEqual(mocks.flowEvent.callCount, 1); - var args = mocks.flowEvent.args[0]; + 'flowEvent.metricsRequest was called correctly': function () { + assert.strictEqual(mocks.flowEvent.metricsRequest.callCount, 1); + var args = mocks.flowEvent.metricsRequest.args[0]; assert.lengthOf(args, 3); assert.equal(args[0], mocks.request); assert.equal(args[1], mocks.request.body); @@ -290,7 +292,7 @@ registerSuite('routes/post-metrics', { }, 'flowEvent was called': function () { - assert.strictEqual(mocks.flowEvent.callCount, 1); + assert.strictEqual(mocks.flowEvent.metricsRequest.callCount, 1); } } } diff --git a/tests/tests_server.js b/tests/tests_server.js index bb63919ae4..e4cdd803a6 100644 --- a/tests/tests_server.js +++ b/tests/tests_server.js @@ -28,10 +28,11 @@ module.exports = [ 'tests/server/remote-address.js', 'tests/server/routes/get-apple-app-site-association.js', 'tests/server/routes/get-config.js', - 'tests/server/routes/get-verify-email.js', 'tests/server/routes/get-fxa-client-configuration.js', 'tests/server/routes/get-lbheartbeat.js', + 'tests/server/routes/get-metrics-flow.js', 'tests/server/routes/get-openid-configuration.js', + 'tests/server/routes/get-verify-email.js', 'tests/server/routes/get-index.js', 'tests/server/routes/post-csp.js', 'tests/server/routes/post-metrics.js', From e1c5e1727bcab5ce2d06921fe22441c691b4f1b6 Mon Sep 17 00:00:00 2001 From: Vlad Filippov Date: Tue, 29 May 2018 11:21:26 -0400 Subject: [PATCH 2/2] feat(metrics): add tests and docs for front-end changes --- app/scripts/lib/metrics.js | 10 +++++++++- app/tests/spec/lib/metrics.js | 12 ++++++++++++ app/tests/spec/models/flow.js | 26 ++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/scripts/lib/metrics.js b/app/scripts/lib/metrics.js index 5af09cb3bc..42177cbb72 100644 --- a/app/scripts/lib/metrics.js +++ b/app/scripts/lib/metrics.js @@ -208,7 +208,7 @@ define(function (require, exports, module) { const flowModel = new Flow({ metrics: this, sentryMetrics: this._sentryMetrics, - window: this._window, + window: this._window }); if (flowModel.has('flowId')) { @@ -459,6 +459,14 @@ define(function (require, exports, module) { } }, + /** + * Marks some event already logged in metrics memory. + * + * Used in conjunction with `logEventOnce` when we know that some event was already logged elsewhere. + * Helps avoid event duplication. + * + * @param {String} eventName + */ markEventLogged: function (eventName) { this._eventMemory[eventName] = true; }, diff --git a/app/tests/spec/lib/metrics.js b/app/tests/spec/lib/metrics.js index 7664f1f21e..701c25f80f 100644 --- a/app/tests/spec/lib/metrics.js +++ b/app/tests/spec/lib/metrics.js @@ -240,6 +240,18 @@ define(function (require, exports, module) { }); }); + describe('markEventLogged', function () { + it('does not log an event if marked logged', function () { + metrics.markEventLogged('event2'); + metrics.logEventOnce('event1'); + metrics.logEventOnce('event2'); + + const filteredData = metrics.getFilteredData(); + assert.equal(filteredData.events.length, 1); + assert.equal(filteredData.events[0].type, 'event1'); + }); + }); + describe('startTimer/stopTimer', function () { it('adds a timer to output data', function () { metrics.startTimer('timer1'); diff --git a/app/tests/spec/models/flow.js b/app/tests/spec/models/flow.js index 53a02fed9c..630e38b606 100644 --- a/app/tests/spec/models/flow.js +++ b/app/tests/spec/models/flow.js @@ -21,11 +21,15 @@ define(function (require, exports, module) { var flow; var sentryMetricsMock; var windowMock; + var metricsMock; beforeEach(function () { sentryMetricsMock = { captureException: sinon.spy() }; + metricsMock = { + markEventLogged: sinon.spy() + }; windowMock = new WindowMock(); $(windowMock.document.body).removeData('flowId').removeAttr('data-flow-id'); $(windowMock.document.body).removeData('flowBegin').removeAttr('data-flow-begin'); @@ -33,6 +37,7 @@ define(function (require, exports, module) { function createFlow () { flow = new Flow({ + metrics: metricsMock, sentryMetrics: sentryMetricsMock, window: windowMock }); @@ -76,6 +81,27 @@ define(function (require, exports, module) { assert.equal(flow.get('flowBegin'), 42); }); + it('fetches from query parameters, if available', function () { + $(windowMock.document.body).attr('data-flow-id', BODY_FLOW_ID); + $(windowMock.document.body).attr('data-flow-begin', '42'); + + const QUERY_FLOW_BEGIN = '55'; + const QUERY_FLOW_ID = 'A1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103'; + + windowMock.location.search = Url.objToSearchString({ + /*eslint-disable camelcase*/ + flow_begin_time: QUERY_FLOW_BEGIN, + flow_id: QUERY_FLOW_ID + /*eslint-enable camelcase*/ + }); + + createFlow(); + + assert.equal(flow.get('flowId'), QUERY_FLOW_ID); + assert.equal(flow.get('flowBegin'), QUERY_FLOW_BEGIN); + assert.ok(metricsMock.markEventLogged.calledOnce); + }); + it('logs an error when the resume token contains `flowId` but not `flowBegin`', function () { windowMock.location.search = Url.objToSearchString({ resume: ResumeToken.stringify({ flowId: RESUME_FLOW_ID })