feat(devices): Devices capabilities #320

eoger · 2018-03-16T04:11:12Z

Connects to mozilla/fxa-auth-server#2320 (please read for context).

When fetching a device infos, we want to query devices and devicesCapabilities at the same time.
Since MySQL doesn't have an Array type, we pull the device capabilities using a GROUP_CONCAT and convert them to an array in JS.
For creating or updating a device, we have no choice but to make multiple queries in the same transaction (see the new writeMultiple method).
We maintain a "capability name" <> "capability id" mapping. In practice this is the CAPABILITIES array in mysql.js.
If device.capabilities is supplied in updateDevice, it is implied that we are replacing the current device capabilities.

philbooth

@eoger, I think this looks good! Shout if you disagree with any of the comments, I seem to be having an opinionated day today!

philbooth · 2018-03-16T10:33:02Z

.gitignore

@@ -1,3 +1,4 @@
+.DS_Store


Not sure, but IIRC we prefer to keep platform-specific ignores out of the project .gitignore and ask people to add it to their own ~/.gitignore_global instead.

Yes, please use a global git ignore

philbooth · 2018-03-16T10:49:01Z

lib/db/mysql.js

+      const index = CAPABILITIES.indexOf(c)
+      if (index !== -1) {
+        acc.push({sql: ADD_CAPABILITY, params: [uid, deviceId, index]})
+      }


I wonder whether it might be more helpful to clients to fail the request completely if they submit an unrecognised capability, rather than dropping it silently?

Or do you envisage that validation taking place in the auth server? If that's the case it seems unfortunate that we have to define the valid capability strings in two separate repos. Instead, maybe the auth server could take responsibility for mapping between the capability string and its enumeration, leaving this repo just handling integers?

philbooth · 2018-03-16T11:08:26Z

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

+  deviceId BINARY(16) NOT NULL,
+  capability TINYINT UNSIGNED NOT NULL,
+  PRIMARY KEY (uid, deviceId, capability),
+  FOREIGN KEY (uid, deviceId) REFERENCES devices(uid, id) ON DELETE CASCADE


Sigh, I should have used ON DELETE CASCADE on the devices table itself. And unverifiedTokens. 🤦‍♂️

philbooth · 2018-03-16T11:10:57Z

lib/db/mysql.js

+        return query(connection, sql, params)
+        .then(
+          function (result) {
+            log.trace('MySql.write', { sql, result })


This trace message should be MySql.writeMultiple presumably?

philbooth · 2018-03-16T11:25:00Z

lib/db/mysql.js

+          function (result) {
+            log.trace('MySql.write', { sql, result })
+            if (resultHandler) {
+              return resultHandler(i, result)


Just thinking out loud, ignore me if it's rubbish, but...

Instead of passing in one resultHandler and then giving it an index so it knows which query it's being invoked for, would it be a little bit cleaner if resultHandler was a property of the objects inside the queries array? So each query can have a dedicated result handler that doesn't need to check the value of i?

philbooth · 2018-03-16T11:31:08Z

lib/db/mysql.js

  }

-  // Select : devices d, sessionTokens s, accounts a
+  // Since MySQL doesn't have an ARRAY type, we store arrays as comma-separated


More bike-shedding, but this comment reads a little bit inaccurately to me. Unless I misunderstand, we're not storing them as comma-separated values, they're stored as rows in devicesCapabilities. They're just returned from queries as comma-separated values, right?

philbooth · 2018-03-16T11:39:06Z

lib/db/mysql.js

  }

+  // Enum capability name -> capability id (index)
+  const CAPABILITIES = ['pushbox']


Should we also define a const CAPABILITY_IDS at this point, containing the inverse mapping from string to integer? It would save us having to rely on indexOf in the rest of the code, and could be generated with CAPABILITIES.reduce so no need to actually define anything really.

philbooth · 2018-03-16T11:49:07Z

lib/db/mysql.js

+    const capabilitiesQueries = []
+    if (deviceInfo.hasOwnProperty('capabilities')) {
+      capabilitiesQueries.push({sql: PURGE_CAPABILITIES, params: [uid, deviceId]})
+      addLegalCapabilities(uid, deviceId, deviceInfo, capabilitiesQueries)


Bike-shedding but I think, on balance, I don't like this pattern of passing in the array and modifying it (or not) inside addLegalCapabilities. It's kind of small-scale action-at-a-distance.

Would it be cleaner if addLegalCapabilities always returns a fresh array and the caller can then be explicit about joining them with capabilitiesQueries.concat if it needs to?

philbooth · 2018-03-16T11:57:26Z

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

+  IN `inCapability` TINYINT UNSIGNED
+)
+BEGIN
+  INSERT INTO devicesCapabilities(


Even more bike-shedding (sorry)... are we sure about the plural on the devices part here? deviceCapabilities sounds more natural to me.

eoger · 2018-03-16T19:10:57Z

Thank you @philbooth that was really useful feedback!
I've updated my patch with your comments and added tests.

philbooth · 2018-03-19T06:28:48Z

lib/db/util.js

  ['totp-2fa', 2]   // TOTP code
 ])

+// If you modify one of these maps, modify the other.


This is fine for now but I think, if the capabilities list gets longer in the future, we should definitely switch to generating one of these programmatically so that there's only one definition to maintain.

philbooth

Splendid! Let's also add the new error information to the docs for createDevice and updateDevice, then get this merged!

philbooth · 2018-03-19T06:33:53Z

docs/API.md

      "callbackAuthKey": "w3b14Zjc-Afj2SDOLOyong",
-      "callbackIsExpired": false
+      "callbackIsExpired": false,
+      "capabilities": ["pushbox"]


Can you also document the new error response in the section below?

philbooth · 2018-03-19T06:34:20Z

docs/API.md

      "callbackAuthKey": "w3b14Zjc-Afj2SDOLOyong",
-      "callbackIsExpired": false
+      "callbackIsExpired": false,
+      "capabilities": ["pushbox"]


Can you also document the new error response in the section below?

philbooth · 2018-03-19T06:35:07Z

docs/DB_API.md

  * `callbackAuthKey` (string):
    Auth key for push service
+  * `capabilities` (array):
+    Array of strings describing the current device capabilities


Can you also document the new error under Rejects with?

philbooth · 2018-03-19T06:35:24Z

docs/DB_API.md

  * `callbackAuthKey` (string):
    Auth key for push service
+  * `capabilities` (array):
+    Array of strings describing the current device capabilities


Can you also document the new error under Rejects with?

Connects to mozilla/fxa-auth-server#2320

eoger · 2018-03-19T14:10:38Z

All done, thank you!

ghost assigned eoger Mar 16, 2018

ghost added the waffle:active label Mar 16, 2018

eoger added waffle:review and removed waffle:active labels Mar 16, 2018

philbooth reviewed Mar 16, 2018

View reviewed changes

vbudhram mentioned this pull request Mar 16, 2018

feat(totp): initial recovery codes #319

Merged

eoger force-pushed the devices-capabilities branch from 48753a3 to 2e41a11 Compare March 16, 2018 19:08

eoger mentioned this pull request Mar 16, 2018

feat(devices): Devices capabilities mozilla/fxa-auth-server#2350

Merged

philbooth reviewed Mar 19, 2018

View reviewed changes

philbooth approved these changes Mar 19, 2018

View reviewed changes

feat(devices): Devices capabilities

9577f83

Connects to mozilla/fxa-auth-server#2320

eoger force-pushed the devices-capabilities branch from 2e41a11 to 9577f83 Compare March 19, 2018 14:10

eoger merged commit 4808a1c into master Mar 19, 2018

ghost removed the waffle:review label Mar 19, 2018

eoger deleted the devices-capabilities branch March 19, 2018 14:28

feat(devices): Devices capabilities #320

feat(devices): Devices capabilities #320

Uh oh!

Conversation

eoger commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

eoger commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

eoger commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eoger commented Mar 16, 2018 •

edited

Loading