From d8841483d7e673641153606c4ceabae97a88bdee Mon Sep 17 00:00:00 2001 From: Phil Booth Date: Tue, 23 Oct 2018 07:55:42 +0100 Subject: [PATCH] chore(api): remove metrics context data from deprecated endpoints Fixes #2496. Previously we identified some endpoints that were receiving metrics context data but would work equally well propagating stashed data instead. There's been some work to prepare those endpoints, e.g. in 804907a3 we ensured metrics context gets propagated through the account reset flow. And in the content server, as of train 123 we no longer send metrics context data to these endpoints. Ergo, it is now safe to remove it from the payload schemata. Note that this change presented us with a dilemma in the remote tests for account reset. Some of the assertions there check that metrics context data is set in the email headers correctly. That now depends on memcached in order to work, but we were disabling memcached in the test config. Rather than delete those assertions, which are valuable, I opted to enable memcached instead. So memcached is now a prerequisite for the tests to succeed in this repo. --- README.md | 2 ++ docs/api.md | 36 ----------------------------------- lib/routes/account.js | 13 +------------ lib/routes/emails.js | 2 -- lib/routes/password.js | 8 +------- lib/routes/recovery-codes.js | 4 +--- lib/routes/totp.js | 1 - package.json | 2 +- test/client/api.js | 5 +---- test/local/routes/account.js | 19 +++++------------- test/local/routes/password.js | 4 ++-- 11 files changed, 14 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index 468031a7c..eb375b998 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,8 @@ Services ecosystem. * npm 2 * Grunt * postfix +* memcached +* redis ## Install diff --git a/docs/api.md b/docs/api.md index 8d73d3e1c..ad38b3ae5 100644 --- a/docs/api.md +++ b/docs/api.md @@ -943,12 +943,6 @@ a new `sessionToken` and `keyFetchToken`. Indicates whether a new `sessionToken` is required, default to `false`. -* `metricsContext`: *metricsContext.schema* - - - - - ##### Error responses Failing requests may be caused @@ -1706,12 +1700,6 @@ as a query parameter. Opaque URL-encoded string to be included in the verification link as a query parameter. -* `metricsContext`: *metricsContext.schema* - - - - - * `type`: *string, max(32), alphanum, allow(), optional* @@ -2214,12 +2202,6 @@ has been called. Opaque URL-encoded string to be included in the verification link as a query parameter. -* `metricsContext`: *metricsContext.schema* - - - - - ##### Response body * `passwordForgotToken`: *string* @@ -2268,12 +2250,6 @@ to reset the account password and `wrapKb`. The code sent to the user's recovery email. -* `metricsContext`: *metricsContext.schema* - - - - - * `accountResetWithRecoveryKey`: *boolean, optional* @@ -2359,12 +2335,6 @@ Verify a session using a recovery code. -* `metricsContext`: *metricsContext.schema* - - - - - ##### Response body * `remaining`: *number* @@ -2949,12 +2919,6 @@ Verifies the current session if the passed TOTP code is valid. The TOTP code to check -* `metricsContext`: *metricsContext.schema* - - - - - * `service`: *validators.service* diff --git a/lib/routes/account.js b/lib/routes/account.js index 783e353a4..71268f5e8 100644 --- a/lib/routes/account.js +++ b/lib/routes/account.js @@ -942,8 +942,7 @@ module.exports = (log, db, mailer, Password, config, customs, signinUtils, push) authPW: validators.authPW, wrapKb: validators.wrapKb.optional(), recoveryKeyId: validators.recoveryKeyId.optional(), - sessionToken: isA.boolean().optional(), - metricsContext: METRICS_CONTEXT_SCHEMA + sessionToken: isA.boolean().optional() }).and('wrapKb', 'recoveryKeyId') } }, @@ -956,16 +955,6 @@ module.exports = (log, db, mailer, Password, config, customs, signinUtils, push) const recoveryKeyId = request.payload.recoveryKeyId let account, sessionToken, keyFetchToken, verifyHash, wrapWrapKb, password - request.validateMetricsContext() - - let flowCompleteSignal - if (requestHelper.wantsKeys(request)) { - flowCompleteSignal = 'account.signed' - } else { - flowCompleteSignal = 'account.reset' - } - request.setMetricsFlowCompleteSignal(flowCompleteSignal) - return checkRecoveryKey() .then(resetAccountData) .then(recoveryKeyDeleteAndEmailNotification) diff --git a/lib/routes/emails.js b/lib/routes/emails.js index 9028bc614..fe8c24a1a 100644 --- a/lib/routes/emails.js +++ b/lib/routes/emails.js @@ -7,7 +7,6 @@ const butil = require('../crypto/butil') const error = require('../error') const isA = require('joi') -const METRICS_CONTEXT_SCHEMA = require('../metrics/context').schema const P = require('../promise') const random = require('../crypto/random') const validators = require('./validators') @@ -129,7 +128,6 @@ module.exports = (log, db, mailer, config, customs, push) => { service: validators.service, redirectTo: validators.redirectTo(config.smtp.redirectDomain).optional(), resume: isA.string().max(2048).optional(), - metricsContext: METRICS_CONTEXT_SCHEMA, type: isA.string().max(32).alphanum().allow(['upgradeSession']).optional() } } diff --git a/lib/routes/password.js b/lib/routes/password.js index 8a9012481..971f80c46 100644 --- a/lib/routes/password.js +++ b/lib/routes/password.js @@ -520,8 +520,7 @@ module.exports = function ( email: validators.email().required(), service: validators.service, redirectTo: validators.redirectTo(redirectDomain).optional(), - resume: isA.string().max(2048).optional(), - metricsContext: METRICS_CONTEXT_SCHEMA + resume: isA.string().max(2048).optional() } }, response: { @@ -539,8 +538,6 @@ module.exports = function ( var service = request.payload.service || request.query.service const ip = request.app.clientAddress - request.validateMetricsContext() - const { flowId, flowBeginTime } = await request.app.metricsContext return P.all([ @@ -609,7 +606,6 @@ module.exports = function ( validate: { payload: { code: isA.string().min(32).max(32).regex(HEX_STRING).required(), - metricsContext: METRICS_CONTEXT_SCHEMA, accountResetWithRecoveryKey: isA.boolean().optional() } }, @@ -625,8 +621,6 @@ module.exports = function ( var code = request.payload.code const accountResetWithRecoveryKey = request.payload.accountResetWithRecoveryKey - request.validateMetricsContext() - const { flowId, flowBeginTime } = await request.app.metricsContext let accountResetToken diff --git a/lib/routes/recovery-codes.js b/lib/routes/recovery-codes.js index a886b97d5..798d369d9 100644 --- a/lib/routes/recovery-codes.js +++ b/lib/routes/recovery-codes.js @@ -7,7 +7,6 @@ const errors = require('../error') const isA = require('joi') const BASE_36 = require('./validators').BASE_36 -const METRICS_CONTEXT_SCHEMA = require('../metrics/context').schema const RECOVERY_CODE_SANE_MAX_LENGTH = 20 module.exports = (log, db, config, customs, mailer) => { @@ -95,8 +94,7 @@ module.exports = (log, db, config, customs, mailer) => { }, validate: { payload: { - code: isA.string().max(RECOVERY_CODE_SANE_MAX_LENGTH).regex(BASE_36).required(), - metricsContext: METRICS_CONTEXT_SCHEMA + code: isA.string().max(RECOVERY_CODE_SANE_MAX_LENGTH).regex(BASE_36).required() } }, response: { diff --git a/lib/routes/totp.js b/lib/routes/totp.js index 583a1effb..749783653 100644 --- a/lib/routes/totp.js +++ b/lib/routes/totp.js @@ -234,7 +234,6 @@ module.exports = (log, db, mailer, customs, config) => { validate: { payload: { code: isA.string().max(32).regex(validators.DIGITS).required(), - metricsContext: METRICS_CONTEXT_SCHEMA, service: validators.service } }, diff --git a/package.json b/package.json index 03e3ca0f7..2a105fe53 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "shrink": "npmshrink && npm run postinstall && scripts/tls-shrink.sh", "start": "NODE_ENV=dev scripts/start-local.sh 2>&1", "start-mysql": "NODE_ENV=dev scripts/start-local-mysql.sh 2>&1", - "test": "VERIFIER_VERSION=0 MEMCACHE_METRICS_CONTEXT_ADDRESS=none NO_COVERAGE=1 scripts/test-local.sh", + "test": "VERIFIER_VERSION=0 NO_COVERAGE=1 scripts/test-local.sh", "test-ci": "scripts/test-local.sh", "test-e2e": "NODE_ENV=dev mocha test/e2e", "test-scripts": "mocha test/scripts --exit", diff --git a/test/client/api.js b/test/client/api.js index 0bf7c84a4..8ed0531b2 100644 --- a/test/client/api.js +++ b/test/client/api.js @@ -500,7 +500,6 @@ module.exports = config => { token, { code: code, - metricsContext: options.metricsContext || undefined, accountResetWithRecoveryKey: options.accountResetWithRecoveryKey || undefined }, headers @@ -829,7 +828,6 @@ module.exports = config => { token, { code: code, - metricsContext: options.metricsContext, service: options.service } ) @@ -855,8 +853,7 @@ module.exports = config => { this.baseURL + '/session/verify/recoveryCode', token, { - code: code, - metricsContext: options.metricsContext || undefined + code: code } ) }) diff --git a/test/local/routes/account.js b/test/local/routes/account.js index 92c6a81b6..fb967a6c7 100644 --- a/test/local/routes/account.js +++ b/test/local/routes/account.js @@ -192,7 +192,7 @@ describe('/account/reset', function () { it('should have emitted metrics', () => { assert.equal(mockLog.activityEvent.callCount, 1, 'log.activityEvent was called once') - let args = mockLog.activityEvent.args[0] + const args = mockLog.activityEvent.args[0] assert.equal(args.length, 1, 'log.activityEvent was passed one argument') assert.deepEqual(args[0], { event: 'account.reset', @@ -201,12 +201,8 @@ describe('/account/reset', function () { uid: uid }, 'event data was correct') - assert.equal(mockMetricsContext.validate.callCount, 1, 'metricsContext.validate was called') - assert.equal(mockMetricsContext.validate.args[0].length, 0, 'validate was called without arguments') - assert.equal(mockMetricsContext.setFlowCompleteSignal.callCount, 1, 'metricsContext.setFlowCompleteSignal was called once') - args = mockMetricsContext.setFlowCompleteSignal.args[0] - assert.equal(args.length, 1, 'metricsContext.setFlowCompleteSignal was passed one argument') - assert.equal(args[0], 'account.signed', 'argument was event name') + assert.equal(mockMetricsContext.validate.callCount, 0) + assert.equal(mockMetricsContext.setFlowCompleteSignal.callCount, 0) assert.equal(mockMetricsContext.propagate.callCount, 2) }) @@ -250,13 +246,8 @@ describe('/account/reset', function () { assert.equal(securityEvent.ipAddr, clientAddress) assert.equal(securityEvent.name, 'account.reset') - assert.equal(mockMetricsContext.validate.callCount, 1, 'metricsContext.validate was called') - assert.equal(mockMetricsContext.validate.args[0].length, 0, 'validate was called without arguments') - - assert.equal(mockMetricsContext.setFlowCompleteSignal.callCount, 1, 'metricsContext.setFlowCompleteSignal was called once') - args = mockMetricsContext.setFlowCompleteSignal.args[0] - assert.equal(args.length, 1, 'metricsContext.setFlowCompleteSignal was passed one argument') - assert.equal(args[0], 'account.signed', 'argument was event name') + assert.equal(mockMetricsContext.validate.callCount, 0) + assert.equal(mockMetricsContext.setFlowCompleteSignal.callCount, 0) assert.equal(mockMetricsContext.propagate.callCount, 2) diff --git a/test/local/routes/password.js b/test/local/routes/password.js index 8f73ab8b0..c40b3287a 100644 --- a/test/local/routes/password.js +++ b/test/local/routes/password.js @@ -183,7 +183,7 @@ describe('/password', () => { assert.equal(mockMailer.sendRecoveryCode.callCount, 1, 'mailer.sendRecoveryCode was called once') assert.equal(mockMailer.sendRecoveryCode.args[0][2].uid, uid) - assert.equal(mockRequest.validateMetricsContext.callCount, 1, 'validateMetricsContext was called') + assert.equal(mockRequest.validateMetricsContext.callCount, 0) assert.equal(mockLog.flowEvent.callCount, 2, 'log.flowEvent was called twice') assert.equal(mockLog.flowEvent.args[0][0].event, 'password.forgot.resend_code.start', 'password.forgot.resend_code.start event was logged') assert.equal(mockLog.flowEvent.args[1][0].event, 'password.forgot.resend_code.completed', 'password.forgot.resend_code.completed event was logged') @@ -260,7 +260,7 @@ describe('/password', () => { assert.equal(args.length, 1, 'db.passwordForgotVerified was passed one argument') assert.deepEqual(args[0].uid, uid, 'db.forgotPasswordVerified was passed the correct token') - assert.equal(mockRequest.validateMetricsContext.callCount, 1, 'validateMetricsContext was called') + assert.equal(mockRequest.validateMetricsContext.callCount, 0) assert.equal(mockLog.flowEvent.callCount, 2, 'log.flowEvent was called twice') assert.equal(mockLog.flowEvent.args[0][0].event, 'password.forgot.verify_code.start', 'password.forgot.verify_code.start event was logged') assert.equal(mockLog.flowEvent.args[1][0].event, 'password.forgot.verify_code.completed', 'password.forgot.verify_code.completed event was logged')