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

vbudhram · 2017-11-20T16:30:24Z

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

@mozilla/fxa-devs r?

rfk

Thanks @vbudhram. This broadly makes sense to me, although I admit I'm wary of the growing complexity around token verification in general. I left a bunch of smaller nits for your consideration.

rfk · 2017-11-22T03:12:09Z

docs/API.md


 * sessionToken : data, uid, createdAt, uaBrowser, uaBrowserVersion, uaOS, uaOSVersion, uaDeviceType,
-                 uaFormFactor, mustVerify, tokenVerificationId
+                 uaFormFactor, mustVerify, tokenVerificationId, shortCode, shortCodeExpiresAt


naming nit: I wonder if we should include the word "verification" somewhere in the name of this thing, to make it more clear what it's for.

(that may be less of a concern if it's only for internal use by this repo, and is return returned to the auth-server)

rfk · 2017-11-22T03:14:01Z

fxa-auth-db-server/index.js

  api.get('/sessionToken/:id/verified', withIdAndBody(db.sessionTokenWithVerificationStatus))
  api.get('/keyFetchToken/:id/verified', withIdAndBody(db.keyFetchTokenWithVerificationStatus))
  api.post('/tokens/:id/verify', withIdAndBody(db.verifyTokens))
+  api.post('/tokens/:shortCode/verifyShortCode', op(req => db.verifyTokensShortCode(req.params.shortCode, { uid: req.body.uid } )))


Is the withIdAndBody helper not usable here? It seems a little odd to have this route as the only one that's not using the helper.

Yea it wasn't useable. Using that would create a buffer instead of sending the string. This was based off how signinCodes do it.

rfk · 2017-11-22T03:15:09Z

fxa-auth-db-server/test/backend/db_tests.js

      })
    })

+    describe('db.verifyTokensShortCodes', () => {


nit: "ShortCodes" plural in the name here doesn't seem quite right, should it be singular?

rfk · 2017-11-22T03:19:51Z

lib/db/schema/patch-067-068.sql

+ALGORITHM = INPLACE LOCK=NONE;
+
+ALTER TABLE `unverifiedTokens`
+ADD COLUMN `shortCodeExpiresAt` BIGINT UNSIGNED DEFAULT NULL,


You should be able to add both of these columns in a single ALTER TABLE statement, which will be easier for ops to run.

rfk · 2017-11-22T03:25:02Z

lib/db/schema/patch-068-067.sql

+
+-- ALTER TABLE `unverifiedTokens`
+-- DROP COLUMN `shortCodeExpiresAt` BIGINT UNSIGNED,
+-- ALGORITHM = INPLACE, LOCK = NONE;


Should we also drop the unverifiedTokens_shortCodeHash index here, potentially before removing the columns in question?

rfk · 2017-11-22T03:42:21Z

lib/db/schema/patch-067-068.sql

+    IF @updateCount > 0 THEN
+      SIGNAL SQLSTATE '45000' SET MYSQL_ERRNO = 2101, MESSAGE_TEXT = 'Expired shortcode.';
+    END IF;
+  END IF;


If we expect the common case to be using a valid, unexpired shortCode, then we should optimize for doing a single read in that case. I think we can achieve that by having the first query include the expiry check, like:

SELECT tokenVerificationId INTO @tokenVerificationId FROM unverifiedTokens WHERE uid = uidArg AND shortCodeHash = shortCodeHashArg and shortCodeExpiresAt >= (UNIX_TIMESTAMP(NOW(3)) * 1000)

And then, if we didn't find one, we can whether it exists but is expired with a separate query like above.

rfk · 2017-11-22T03:44:09Z

docs/API.md

                 d.callbackAuthKey AS deviceCallbackAuthKey,
                 d.callbackIsExpired AS deviceCallbackIsExpired,
-                 ut.mustVerify, ut.tokenVerificationId
+                 ut.mustVerify, ut.tokenVerificationId, ut.shortCode, ut.shortCodeExpiresAt


Likewise here, this returns the shortCodeHash.

rfk · 2017-11-22T03:55:16Z

lib/db/schema/patch-067-068.sql

+  SET @tokenVerificationId = NULL;
+  SELECT tokenVerificationId INTO @tokenVerificationId FROM unverifiedTokens WHERE uid = uidArg AND shortCodeHash = shortCodeHashArg;
+
+  IF @tokenVerificationId IS NOT NULL THEN


What happens if @tokenVerificationId is NULL here? IIUC control flow will fall through and execute the UPDATE securityEvents below, which will set verified = true for all security events where tokenVerificationId is NULL. I think that's OK because such events will already have verified=true anyway, but it might be safer to bail out early if we don't find any matching tokenVerificationId.

rfk · 2017-11-22T03:55:56Z

lib/db/schema/patch-067-068.sql

+  IN `uidArg` BINARY(16)
+)
+BEGIN
+  DECLARE EXIT HANDLER FOR SQLEXCEPTION


This exit-handler logic is only necessary if you're going to use a transaction, which actually it might be worth doing here anyway because we're modifying multiple tables.

rfk · 2017-11-22T04:00:03Z

lib/db/schema/patch-067-068.sql

+
+CREATE INDEX `unverifiedTokens_shortCodeHash`
+ON `unverifiedTokens` (`shortCodeHash`)
+ALGORITHM = INPLACE LOCK=NONE;


I wonder if it's worth making this a unique index on (shortCodeHash, uid) in order to prevent the accidental creation of duplicate shortcodes for different login sessions by the same user. That should never happen in theory, but sometimes it's worth ensuring that theory and practice will match :-)

vbudhram · 2017-11-22T20:49:49Z

@rfk Thanks for review! Updated based on feedback

Opted to use tokenVerificationCode, tokenVerificationCodeHash, tokenVerificationCodeExpiresAt instead of shortCode because I think it describes it a little better (configurable code length, stored as hash)
Optimized SQL to check for valid code first

Review at your liesure.

rfk · 2017-11-23T02:05:07Z

lib/db/mem.js

        mustVerify: !! sessionToken.mustVerify,
-        uid: sessionToken.uid
+        tokenVerificationId: sessionToken.tokenVerificationId,
+        tokenVerificationCodeHash: sessionToken.tokenVerificationCode,


IIUC, this is not actually hashing the tokenVerificationCode, so the field name here is wrong. Should it hash it, for consistency with the mysql version?

rfk · 2017-11-23T02:06:01Z

lib/db/mem.js

+      }
+
+      // Is code expired?
+      if (t.tokenVerificationCodeHash.toString('hex') === tokenVerificationCode) {


Likewise here, it looks slightly off the compare a field named tokenVerificationCodeHash to a variable named tokenVerificationCode, we should either do the hashing or rename the field.

rfk

A couple of nits, but this LGTM!

rfk · 2017-11-23T02:07:59Z

lib/db/schema/patch-067-068.sql

+ADD COLUMN `tokenVerificationCodeExpiresAt` BIGINT UNSIGNED DEFAULT NULL,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+CREATE INDEX `unverifiedTokens_tokenVerificationCodeHash_uid`


Did you intend to make this a CREATE UNIQUE INDEX? If not, I don't think it needs to contain the uid column at all.

rfk · 2017-11-23T02:09:17Z

lib/db/schema/patch-067-068.sql

+    AND tokenVerificationCodeExpiresAt >= (UNIX_TIMESTAMP(NOW(3)) * 1000);
+
+  IF @tokenVerificationId IS NULL THEN
+    SET @updateCount = 0;


tiny nit: I don't really understand what the name "updateCount" is supposed to mean here; maybe "expiredCount" or something to make it clearer?

vladikoff

Some feedback

vladikoff · 2017-11-23T19:42:30Z

fxa-auth-db-server/index.js

  api.get('/sessionToken/:id/verified', withIdAndBody(db.sessionTokenWithVerificationStatus))
  api.get('/keyFetchToken/:id/verified', withIdAndBody(db.keyFetchTokenWithVerificationStatus))
  api.post('/tokens/:id/verify', withIdAndBody(db.verifyTokens))
+  api.post('/tokens/:code/verifyCode', op(req => db.verifyTokenCode(req.params.code, { uid: req.body.uid } )))


should this use something like withParams ?

vladikoff · 2017-11-23T19:50:53Z

lib/db/mem.js


+  Memory.prototype.verifyTokenCode = function (tokenVerificationCode, accountData) {
+    const uid = accountData.uid.toString('hex')
+    tokenVerificationCode = tokenVerificationCode.toString('hex')


nit: rename this to tokenVerificationCodeHex ?

vladikoff · 2017-11-23T19:54:50Z

lib/db/mysql.js

+  MySql.prototype.verifyTokenCode = function (tokenVerificationCode, accountData) {
+    return this.readFirstResult(VERIFY_TOKEN_CODE, [createHash(tokenVerificationCode), accountData.uid])
+      .then(function (result) {
+        if (result['@rowCount'] === 0) {


We don't use this @rowCount check anywhere else, is this the only way we can find out if nothing was found?

Unfortunately, this was the only way I could find that would return the row count from within a transaction. That might also explain why the affectedRows property was not returned.

vladikoff · 2017-11-23T19:55:52Z

lib/db/schema/patch-067-068.sql

@@ -0,0 +1,165 @@
+ALTER TABLE `unverifiedTokens`


is this file missing the boilerplate for SET NAMES, via https://github.com/mozilla/fxa-auth-db-mysql/blob/master/scripts/migration.sh#L35 ?

vladikoff · 2017-11-23T19:57:38Z

lib/db/schema/patch-067-068.sql

+  WHERE t.tokenId = tokenIdArg;
+END;
+
+UPDATE dbMetadata SET value = '68' WHERE name = 'schema-patch-level';


missing new line EOF

vbudhram · 2017-11-27T19:36:05Z

Made changes @vladikoff mind r?

vladikoff · 2017-11-27T21:38:48Z

lib/db/util.js


+  createHash () {
+    const hash = crypto.createHash('sha256')
+    // Use ...rest operator and forEach when we're on node 6


I thought we were on node 6 already?

@rfk confirm?

Yes, I believe we are comfortably on node 6; if this repo is still testing node 4 support, feel free to drop it

vladikoff · 2017-11-27T22:32:44Z

Thanks @vbudhram !!

vbudhram added the WIP label Nov 20, 2017

vbudhram self-assigned this Nov 20, 2017

ghost added the waffle:active label Nov 20, 2017

vbudhram force-pushed the feature.signincodes branch from 2bc32ba to bb4a7d7 Compare November 21, 2017 03:01

vbudhram changed the title ~~feat(codes): Add tokenVerification shortCode~~ feat(codes): add support for verifying token short code Nov 21, 2017

vbudhram force-pushed the feature.signincodes branch 2 times, most recently from aa15106 to c867338 Compare November 21, 2017 14:31

vbudhram added waffle:review and removed waffle:active WIP labels Nov 21, 2017

rfk suggested changes Nov 22, 2017

View reviewed changes

vbudhram force-pushed the feature.signincodes branch from 664dd10 to 6851f4e Compare November 22, 2017 20:43

rfk reviewed Nov 23, 2017

View reviewed changes

rfk approved these changes Nov 23, 2017

View reviewed changes

vladikoff reviewed Nov 23, 2017

View reviewed changes

vladikoff added waffle:active and removed waffle:review labels Nov 23, 2017

vbudhram added the shipit label Nov 27, 2017

feat(codes): add initial support for using verify token short codes

238e6e3

vbudhram force-pushed the feature.signincodes branch from d0dc563 to 238e6e3 Compare November 27, 2017 19:14

vladikoff added the train-101 label Nov 27, 2017

vladikoff self-requested a review November 27, 2017 21:25

vladikoff reviewed Nov 27, 2017

View reviewed changes

feat(tests): use spread syntax

70728a5

vladikoff merged commit ac0b814 into master Nov 27, 2017

shane-tomlinson deleted the feature.signincodes branch April 18, 2018 12:44

Conversation

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

Uh oh!

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!

vbudhram commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

vladikoff 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!

vbudhram commented Nov 27, 2017

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!

vladikoff commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vbudhram commented Nov 20, 2017 •

edited

Loading