feat(totp): Add initial totp session verification logic #309

vbudhram · 2018-02-21T20:04:45Z

This PR is the next stage for TOTP tokens. It adds the ability to verify TOTP tokens and sessions.

Adds the verified, enabled property to TOTP tokens.
Adds the verificationMethod, verifiedAt property to session tokens.
Adds the mustVerify property to the session table. This property is in sync with the property on the unverified tokens table, ref: TOTP Session Verification API #301 (comment).
Can be tested against feat(totp): Add verification methods fxa-auth-server#2321

Fixes #301

vbudhram · 2018-02-26T14:38:44Z

db-server/index.js

@@ -125,6 +126,7 @@ function createServer(db) {

  api.get('/keyFetchToken/:id/verified', withIdAndBody(db.keyFetchTokenWithVerificationStatus))
  api.post('/tokens/:id/verify', withIdAndBody(db.verifyTokens))
+  api.post('/tokens/:id/verifyWith', withIdAndBody(db.verifyTokensWithMethod))


Updated this endpoint name to /tokens/:id/verifyWith and db to verifyTokensWithMethod over verifySessionWithMethod. This made sense because it verifies more than session tokens. Open to other names as well.

yea maybe verifyWith --> verifyWithMethod to make it verbose

vbudhram · 2018-02-26T14:42:38Z

db-server/test/backend/remote.js

@@ -1524,8 +1524,8 @@ module.exports = function(cfg, makeServer) {
            .spread((sessionTokenResp, keyFetchTokenResp) => {
              respOk(sessionTokenResp)
              respOk(keyFetchTokenResp)
-              const sessionToken = sessionTokenResp
-              const keyFetchToken = keyFetchTokenResp
+              const sessionToken = sessionTokenResp.obj


Unrelated, but noticed that this wasn't testing against the correct response object.

vbudhram · 2018-02-26T14:48:09Z

lib/db/mem.js

@@ -38,7 +38,7 @@ var DEVICE_FIELDS = [
  'callbackIsExpired'
 ]

-var SESSION_FIELDS = [
+const SESSION_DEVICE_FIELDS = [


This is unrelated change that confused me while working on this PR. These are session device fields that are returned when updating/retrieving devices.

vbudhram · 2018-02-26T14:54:13Z

lib/db/schema/patch-072-073.sql

+  SELECT @updateCount;
+END;
+
+CREATE PROCEDURE `sessionWithDevice_12` (


The procedures below have been updated to return the mustVerify property of session tokens from either the session table or unverified tokens table.

vbudhram · 2018-02-26T15:09:31Z

@mozilla/fxa-devs r?

philbooth · 2018-02-27T07:41:44Z

lib/db/mem.js

+        item.mustVerify = sessionTokens[id].mustVerify
+      } else {
+        item.mustVerify = unverifiedTokens[id].mustVerify
+      }


This code confused me initially. Given that item.mustVerify is already initialised to sessionTokens[id].mustVerify || null on line 546, could it be simplified to a ~~simple~~ single if (! item.mustVerify) { condition?

s/simple/single/

philbooth · 2018-02-27T07:50:20Z

db-server/test/backend/db_tests.js

@@ -1761,7 +1761,7 @@ module.exports = function (config, DB) {
          })
          .then((session) => {
            // Returns verified session
-            assert.equal(session.mustVerify, null, 'mustVerify is not set')
+            assert.equal(!! session.mustVerify, false, 'mustVerify is false')


Purely out of curiousity, do we actually need to keep using !! every time we test these properties?

assert.equal already uses == under the hood, so presumably null coerces to false anyway. And true is always true. Do any of them fail without the !!?

Unfortunately, this would break some tests because the mysql column is using TINYINT(1) instead of BOOLEAN. I don't believe there is any harm in using the boolean variable since it will return true/false instead of 0,1 which needed to be coerced. Will update!

Unfortunately, this would break some tests because the mysql column is using TINYINT(1) instead of BOOLEAN

Turns that that was not correct and tests do pass. The only other explanation I have is that it was needed when we were using TAP. In either case, they have been updated.

philbooth · 2018-02-27T08:02:55Z

lib/db/schema/patch-072-073.sql

+)
+BEGIN
+
+    UPDATE `totp` SET verified = verifiedArg, enable = enableArg WHERE uid = uidArg;


Pedantry: Looks like 2-space indentation elsewhere...

philbooth

LGTM!

philbooth · 2018-02-27T08:16:53Z

docs/DB_API.md

+    The uid of the owning account
+  * `tokenData`
+    * `verified`: Boolean whether TOTP token has been verified
+    * `enable`: Boolean whether TOTP token is enabled


This only occurred to me while reviewing the auth server PR. Should it be enabled rather than enable? That way sounds more like a boolean state and less like an instruction to me.

Yikes, looks I am not consistent with these fields/docs. I will update to use enabled.

philbooth · 2018-02-27T08:18:30Z

lib/db/mem.js

+    // Currently, users can only update the verified and enable flags.
+    // Updating shared secret and epoch will break clients.
+    totpToken.verified = token.verified
+    totpToken.enabled = token.enabled


...oh and here you are setting enabled, not enable. Is one of them wrong?

philbooth · 2018-02-27T08:20:35Z

lib/db/schema/patch-072-073.sql

+-- Add columns `verified` and `enable` to help manage TOTP token state
+ALTER TABLE `totp`
+ADD COLUMN `verified` TINYINT(1) DEFAULT FALSE,
+ADD COLUMN `enable` TINYINT(1) DEFAULT TRUE,


...yeah (he said, continuing the conversation with himself), I think enabled would be better actually, it seems more consistent with other column names.

vbudhram · 2018-02-27T21:46:16Z

@philbooth Thank you!

vladikoff · 2018-02-27T14:47:30Z

db-server/index.js

@@ -97,6 +97,7 @@ function createServer(db) {
      return db.deleteEmail(req.params.id, req.params.email)
    })
  )
+


side note in this file, above we have db.deleteEmail(req.params.id, req.params.email) but all methods below use Buffer( on the email. might be worth filing a clean up issue, I'm not sure.

Interesting, looks like this might have been missed on the original secondary email implementation. Opened issue #311.

vladikoff · 2018-02-27T14:49:55Z

docs/API.md

@@ -91,13 +91,15 @@ The following datatypes are used throughout this document:
    * forgotPasswordVerified    : `POST /passwordForgotToken/:id/verified`
 * Unverified tokens:
    * verifyTokens              : `POST /tokens/:tokenVerificationId/verify`
+    * verifyTokensWithMethod    : `POST /tokens/:tokenId/verifyWith`


this new doc uses tokenId, but above it is tokenVerificationId, both are {:id} in the route definition. Intentional ?

Yea this was intentional. The store procedure will look up the tokenVerificationId directly from the tokenId. I used id since it allowed me to reuse the helper function withIdAndBody. I don't mind updating if it makes it more clear.

vladikoff · 2018-02-27T15:10:43Z

db-server/index.js

@@ -125,6 +126,7 @@ function createServer(db) {

  api.get('/keyFetchToken/:id/verified', withIdAndBody(db.keyFetchTokenWithVerificationStatus))
  api.post('/tokens/:id/verify', withIdAndBody(db.verifyTokens))
+  api.post('/tokens/:id/verifyWith', withIdAndBody(db.verifyTokensWithMethod))


yea maybe verifyWith --> verifyWithMethod to make it verbose

vbudhram added WIP train-107 labels Feb 21, 2018

vbudhram added this to the FxA-143: MFA milestone Feb 21, 2018

vbudhram self-assigned this Feb 21, 2018

ghost added the waffle:active label Feb 21, 2018

vbudhram mentioned this pull request Feb 21, 2018

chore(tests): remove sessionToken and sessionTokenWithVerification #310

Merged

vbudhram force-pushed the feature.totp-verification branch 5 times, most recently from ebfb75f to 848838c Compare February 26, 2018 14:56

vbudhram commented Feb 26, 2018

View reviewed changes

vbudhram added waffle:review and removed WIP waffle:active labels Feb 26, 2018

vbudhram mentioned this pull request Feb 26, 2018

feat(totp): Add verification methods mozilla/fxa-auth-server#2321

Merged

1 task

philbooth reviewed Feb 27, 2018

View reviewed changes

philbooth approved these changes Feb 27, 2018

View reviewed changes

philbooth reviewed Feb 27, 2018

View reviewed changes

feat(totp): Add initial totp session verification logic

c22e19d

vbudhram force-pushed the feature.totp-verification branch from 848838c to c22e19d Compare February 27, 2018 17:44

vbudhram merged commit ee19e1b into master Feb 27, 2018

ghost removed train-107 waffle:review labels Feb 27, 2018

vladikoff reviewed Feb 27, 2018

View reviewed changes

This was referenced Feb 28, 2018

Use email buffer for DEL /email/:email route #311

Closed

feat(totp): totp updates from vlad #312

Closed

feat(totp): vlad updates for totp #313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(totp): Add initial totp session verification logic #309

feat(totp): Add initial totp session verification logic #309

vbudhram commented Feb 21, 2018 •

edited

Loading

vbudhram Feb 26, 2018

vladikoff Feb 27, 2018

vbudhram Feb 26, 2018

vbudhram Feb 26, 2018

vbudhram Feb 26, 2018

vbudhram commented Feb 26, 2018

philbooth Feb 27, 2018 •

edited

Loading

philbooth Feb 27, 2018

philbooth Feb 27, 2018

vbudhram Feb 27, 2018

vbudhram Feb 27, 2018

philbooth Feb 27, 2018

philbooth left a comment

philbooth Feb 27, 2018

vbudhram Feb 27, 2018

philbooth Feb 27, 2018

philbooth Feb 27, 2018

vbudhram commented Feb 27, 2018

vladikoff Feb 27, 2018

vbudhram Feb 28, 2018

vladikoff Feb 27, 2018

vbudhram Feb 28, 2018 •

edited

Loading

vladikoff Feb 28, 2018

vladikoff Feb 27, 2018

feat(totp): Add initial totp session verification logic #309

feat(totp): Add initial totp session verification logic #309

Conversation

vbudhram commented Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram commented Feb 26, 2018

philbooth Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram commented Feb 21, 2018 •

edited

Loading

philbooth Feb 27, 2018 •

edited

Loading

vbudhram Feb 28, 2018 •

edited

Loading