Add ability to verify login with token code by vbudhram · Pull Request #2218 · mozilla/fxa-auth-server

vbudhram · 2017-11-12T18:29:11Z

This exposes the ability for a client to specify how it wants to verify the login.

Add tests
Cleanup code and add docs
Update to handle A/B tests

…dikoff

…dikoff,@rfk This PR exposes new APIs to create and verify sessionTokens using a short code. Couple notes - Each code has an expiration date - Codes are stored as hashes in database - Support for resending (aka regenerating) a code to come in later PR. - New endpoint `verifyTokensShortCode` used to explicitly verify shortCodes. Connects with mozilla/fxa-auth-server#2218 Feature doc: https://docs.google.com/document/d/1xQtCGBJ9XZuF1q9faZV3YIiOnxomQMHKiTP62YSf3v8/edit

vbudhram · 2017-12-06T18:26:01Z

@mozilla/fxa-devs I think this is ready, r?

rfk

This is good stuff @vbudhram! I left a bunch of smaller comments, but I also have two larger ones.

First, can you please generate a screenshot of two of the resulting emails for @ryanfeeley's explicit review?

Second, I don't see anything in here about re-sending the email-2fa email, but the design for the front-end in [1] does include a "resend" link. Do we need a new endpoint to trigger that resend? (And some additional security checks to ensure an attacker doesn't try to use it for old sessions that don't have a short-code attached?).

[1] https://docs.google.com/document/d/1xQtCGBJ9XZuF1q9faZV3YIiOnxomQMHKiTP62YSf3v8/edit?ts=5a0f23d8#heading=h.n7eo6t754fco

rfk · 2017-12-06T19:40:35Z

config/dev.json

+  "signinConfirmation": {
+    "skipForNewAccounts": {
+      "enabled": false
+    }


Is this necessary to e.g. get the tests to pass properly, or was it for local dev?

Just for local dev, but I don't think it is needed.

rfk · 2017-12-06T19:42:03Z

config/index.js

+        format: 'duration',
+        default: '1 hour',
+        env: 'SIGNIN_TOKEN_CODE_LIFETIME'
+      },


For my own sense of thoroughness, noting that these are the same parameters used by the similarly-powerful "unblock codes" feature 👍

rfk · 2017-12-06T19:43:40Z

lib/db.js

+      '/tokens/' + code + '/verifyCode',
+      { uid: accountData.uid }
+    )
+      .then(


nit: the indentation of .then( here is mis-aligned with the paren on the line above, which doesn't match the style of other code in this file.

rfk · 2017-12-06T19:46:13Z

lib/error.js

+    },
+    details
+  )
+}


Just flagging that we'll need to add support in fxa-content-server for handling these new codes.

rfk · 2017-12-06T19:50:29Z

lib/routes/account.js


+  const tokenCodeConfig = config.signinConfirmation.tokenVerificationCode
+  const tokenCodeLifetime = tokenCodeConfig && tokenCodeConfig.codeLifetime || 0
+  const tokenCodeLength = tokenCodeConfig && tokenCodeConfig.codeLength || 0


Zero doesn't seem like a safe fallback value for the code length here. A missing length should either disable the feature entirely, or throw as a hard config error of some sort.

Updated to use default value of 1 hour.

rfk · 2017-12-06T20:17:45Z

lib/routes/token-codes.js

+      path: '/tokenCodes/verify',
+      config: {
+        validate: {
+          payload: {


The tests suggest this should take a metricsContext payload param as well, but it doesn't appear to.

I removed the metricsContext requirement and emit metrics using request.emit

rfk · 2017-12-06T20:21:53Z

test/mail_helper.js

              console.log('\x1B[32m', link, '\x1B[39m')
            }
+            else if (sc) {
+              console.log('\x1B[32mSign-in code: ', sc, '\x1B[39m')


As above, this may be a little confusing because "signin code" is already the name of something else. Could we call this e.g. "verification code" or similar?

rfk · 2017-12-06T20:24:04Z

test/remote/account_login_tests.js

+          assert.equal(status.emailVerified, true, 'email is verified')
+          assert.equal(status.sessionVerified, false, 'session is not verified')
+        })
+    })


Should this also assert that the expected email was sent, like the previous test does?

rfk · 2017-12-06T20:25:53Z

test/remote/token_code_tests.js

+  const metricsContext = {
+    flowId: '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef',
+    flowBeginTime: Date.now()
+  }


ISTM the tests should assert something about this metricsContext, e.g. that the correct flow-id header was present in the outgoing email.

rfk · 2017-12-06T20:26:35Z

test/remote/token_code_tests.js

+      })
+  })
+
+  it('should consume valid code', () => {


For completeness, can you please add a remote test to assert that invalid token codes are rejected, and leave the session unverified? It'll be good to double-check that end-to-end.

vbudhram · 2017-12-19T18:33:20Z

@rfk Updated! Opted to use /session/verify/token route since I think it allows us to extend to other methods a little more clearly. r?

rfk

r+ with nits, thanks @vbudhram

I left a couple of small nits, but also realized one broader concern w.r.t metrics. IIUC users who end up in the treatment group for this experiment would no longer emit an account.confirmed metrics event. I suspect that they should, and that we should just add one in the /session/verify/token endpoint. But it's possible I don't understand all the nuance involved. Thoughts?

rfk · 2017-12-20T01:50:30Z

lib/routes/account.js


+  const tokenCodeConfig = config.signinConfirmation.tokenVerificationCode
+  const tokenCodeLifetime = tokenCodeConfig && tokenCodeConfig.codeLifetime || MS_ONE_HOUR
+  const tokenCodeLength = tokenCodeConfig && tokenCodeConfig.codeLength || 0


IIUC, you changed this to make the default for tokenCodeLifetime be 1 hour, but by concern was about the default for tokenCodeLength - we don't want a configuration error to accidentally make "" a valid verification code for all sessions. It may also be worth changing the defaults for unblockCode* above to match whatever you choose here, for consistency.

rfk · 2017-12-20T02:01:42Z

lib/routes/token-codes.js

+        const uid = request.payload.uid
+        const code = request.payload.code.toUpperCase()
+
+        customs.checkIpOnly(request, 'verifyTokenCode')


Since we've authenticated with the sessionToken here, we have the user's email address available in request.auth.credentials.email, meaning it could use customs.check here rather than customs.checkIpOnly. It's probably worth doing so, to avoid incorrectly rate-limited people who happen to be behind a shared IP.

rfk · 2017-12-20T02:05:34Z

lib/routes/token-codes.js

+
+          return request.emitMetricsEvent('tokenCodes.verified', {uid: uid})
+            .then(() => ({}))
+        }


I suspect we will need to emit an account.confirmed metrics event here, for at least two reasons:

This event is used by amplitude to generate fxa_login - email_confirmed events, so users in this experiment might otherwise disappear from our login funnels in amplitude

This is used as the flow-complete signal for non-sync logins, so users in this experiment might incorrectly appear to have failed to complete their login flow.

rfk · 2017-12-20T02:09:28Z

test/remote/token_code_tests.js

+    return Client.login(config.publicUrl, email, password, {
+      verificationMethod: 'email-2fa'
+    })
+      .then((res) => {


nit: weird indentation here?

rfk · 2017-12-20T02:09:43Z

test/remote/token_code_tests.js

+    return Client.login(config.publicUrl, email, password, {
+      verificationMethod: 'email-2fa'
+    })
+      .then((res) => {


nit: weird indentation here?

vbudhram · 2017-12-20T17:03:58Z

@rfk Thank you! Added updated metrics

vbudhram added the WIP label Nov 12, 2017

vbudhram self-assigned this Nov 12, 2017

ghost added the waffle:active label Nov 12, 2017

vbudhram added the train-101 label Nov 13, 2017

vbudhram force-pushed the feature.signincode branch from c228a7a to 47a1fe9 Compare November 15, 2017 18:03

vbudhram mentioned this pull request Nov 20, 2017

feat(codes): add support for verifying token short code mozilla/fxa-auth-db-mysql#287

Merged

vladikoff added train-102 and removed train-101 labels Nov 27, 2017

vbudhram force-pushed the feature.signincode branch 2 times, most recently from dc51a86 to cb68dcf Compare November 30, 2017 22:03

vbudhram changed the title ~~Add ability to verify login with sign-in code~~ Add ability to verify login with token code Nov 30, 2017

vbudhram force-pushed the feature.signincode branch 2 times, most recently from 73323b6 to da3798d Compare December 6, 2017 18:07

vbudhram added waffle:review and removed waffle:active WIP labels Dec 6, 2017

rfk suggested changes Dec 6, 2017

View reviewed changes

vbudhram added train-103 and removed train-102 labels Dec 14, 2017

feat(signin): add initial implementation for sign-in code

16ab206

vbudhram force-pushed the feature.signincode branch from 224d14d to 16ab206 Compare December 19, 2017 14:49

feat(emails): update docs

0de2ca9

rfk approved these changes Dec 20, 2017

View reviewed changes

feat(emails): pr fixes

b728254

vbudhram merged commit 677bdbb into master Dec 20, 2017

vbudhram deleted the feature.signincode branch February 22, 2018 16:32

Conversation

vbudhram commented Nov 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbudhram commented Dec 6, 2017

Uh oh!

rfk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbudhram commented Dec 19, 2017

Uh oh!

rfk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbudhram commented Dec 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vbudhram commented Nov 12, 2017 •

edited

Loading