Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Commit

Permalink
chore(api): remove metrics context data from deprecated endpoints
Browse files Browse the repository at this point in the history 
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 804907a 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.
  • Loading branch information
@philbooth
philbooth committed Oct 25, 2018
1 parent 326ca02 commit d884148
Show file tree
Hide file tree
Showing 11 changed files with 14 additions and 82 deletions.
2 changes: 2 additions & 0 deletions README.md
View file
Expand Up @@ -25,6 +25,8 @@ Services ecosystem.
* npm 2
* Grunt
* postfix
* memcached
* redis

## Install

Expand Down
36 changes: 0 additions & 36 deletions docs/api.md
View file
Expand Up @@ -943,12 +943,6 @@ a new `sessionToken` and `keyFetchToken`.
Indicates whether a new `sessionToken` is required, default to `false`.
<!--end-request-body-post-accountreset-sessionToken-->

* `metricsContext`: *metricsContext.schema*

<!--begin-request-body-post-accountreset-metricsContext-->

<!--end-request-body-post-accountreset-metricsContext-->

##### Error responses

Failing requests may be caused
Expand Down Expand Up @@ -1706,12 +1700,6 @@ as a query parameter.
Opaque URL-encoded string to be included in the verification link as a query parameter.
<!--end-request-body-post-recovery_emailresend_code-resume-->

* `metricsContext`: *metricsContext.schema*

<!--begin-request-body-post-recovery_emailresend_code-metricsContext-->

<!--end-request-body-post-recovery_emailresend_code-metricsContext-->

* `type`: *string, max(32), alphanum, allow(), optional*

<!--begin-request-body-post-recovery_emailresend_code-type-->
Expand Down Expand Up @@ -2214,12 +2202,6 @@ has been called.
Opaque URL-encoded string to be included in the verification link as a query parameter.
<!--end-request-body-post-passwordforgotresend_code-resume-->

* `metricsContext`: *metricsContext.schema*

<!--begin-request-body-post-passwordforgotresend_code-metricsContext-->

<!--end-request-body-post-passwordforgotresend_code-metricsContext-->

##### Response body

* `passwordForgotToken`: *string*
Expand Down Expand Up @@ -2268,12 +2250,6 @@ to reset the account password and `wrapKb`.
The code sent to the user's recovery email.
<!--end-request-body-post-passwordforgotverify_code-code-->

* `metricsContext`: *metricsContext.schema*

<!--begin-request-body-post-passwordforgotverify_code-metricsContext-->

<!--end-request-body-post-passwordforgotverify_code-metricsContext-->

* `accountResetWithRecoveryKey`: *boolean, optional*

<!--begin-request-body-post-passwordforgotverify_code-accountResetWithRecoveryKey-->
Expand Down Expand Up @@ -2359,12 +2335,6 @@ Verify a session using a recovery code.

<!--end-request-body-post-sessionverifyrecoverycode-code-->

* `metricsContext`: *metricsContext.schema*

<!--begin-request-body-post-sessionverifyrecoverycode-metricsContext-->

<!--end-request-body-post-sessionverifyrecoverycode-metricsContext-->

##### Response body

* `remaining`: *number*
Expand Down Expand Up @@ -2949,12 +2919,6 @@ Verifies the current session if the passed TOTP code is valid.
The TOTP code to check
<!--end-request-body-post-sessionverifytotp-code-->

* `metricsContext`: *metricsContext.schema*

<!--begin-request-body-post-sessionverifytotp-metricsContext-->

<!--end-request-body-post-sessionverifytotp-metricsContext-->

* `service`: *validators.service*

<!--begin-request-body-post-sessionverifytotp-service-->
Expand Down
13 changes: 1 addition & 12 deletions lib/routes/account.js
View file
Expand Up @@ -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')
}
},
Expand All @@ -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)
Expand Down
2 changes: 0 additions & 2 deletions lib/routes/emails.js
View file
Expand Up @@ -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')
Expand Down Expand Up @@ -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()
}
}
Expand Down
8 changes: 1 addition & 7 deletions lib/routes/password.js
View file
Expand Up @@ -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: {
Expand All @@ -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([
Expand Down Expand Up @@ -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()
}
},
Expand All @@ -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
Expand Down
4 changes: 1 addition & 3 deletions lib/routes/recovery-codes.js
View file
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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: {
Expand Down
1 change: 0 additions & 1 deletion lib/routes/totp.js
View file
Expand Up @@ -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
}
},
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Expand Up @@ -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",
Expand Down
5 changes: 1 addition & 4 deletions test/client/api.js
View file
Expand Up @@ -500,7 +500,6 @@ module.exports = config => {
token,
{
code: code,
metricsContext: options.metricsContext || undefined,
accountResetWithRecoveryKey: options.accountResetWithRecoveryKey || undefined
},
headers
Expand Down Expand Up @@ -829,7 +828,6 @@ module.exports = config => {
token,
{
code: code,
metricsContext: options.metricsContext,
service: options.service
}
)
Expand All @@ -855,8 +853,7 @@ module.exports = config => {
this.baseURL + '/session/verify/recoveryCode',
token,
{
code: code,
metricsContext: options.metricsContext || undefined
code: code
}
)
})
Expand Down
19 changes: 5 additions & 14 deletions test/local/routes/account.js
View file
Expand Up @@ -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',
Expand All @@ -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)
})

Expand Down Expand Up @@ -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)

Expand Down
4 changes: 2 additions & 2 deletions test/local/routes/password.js
View file
Expand Up @@ -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')
Expand Down Expand Up @@ -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')
Expand Down

0 comments on commit d884148

Please sign in to comment.