device api endpoints #110

dannycoates · 2015-10-05T21:55:51Z

new endpoints for the device api

(stealth edit by @rfk to link things in waffle: Fixes #104)

dannycoates · 2015-10-08T02:28:32Z

This needs some more tests but the main idea is done. I did some things in the sql that are a bit outside my comfort zone, so another look to make sure I'm not back asswords would be helpful. @rfk f?

rfk · 2015-10-08T03:52:35Z

fxa-auth-db-server/index.js

+        )
+        .done(next, next)
+    }
+  )


There's an asymmetry here, updating a device record via POST /device but deleting it via DELETE /device/:deviceId. For other datatypes we've been relying on fxa-auth-server to generate the ids and pass them through to us in the URL, should we do the same with devices for consistency?

So having read further down, I see why you want to generate the ids internally here. It still feels a bit out-of-place given the rest of the API, but perhaps it's the only reasonable way to get the combination of short-enough-to-not-be-identifiable and guaranteed-not-to-collide-per-user.

We should carefully consider what we're guarding against by making the device-ids short though. If they only ever appear on the wire alongside a long-and-uniquely-identifiable sessionToken id, it may not provide much value.

rfk · 2015-10-08T04:01:18Z

lib/db/schema/patch-018-019.sql

+  )
+  SELECT
+    inUid,
+    COALESCE(MAX(id), 0) + 1,


To clarify, the idea here is to to ensure that each user gets devices numbered 1, 2, 3 etc? I seem to recall MySQL having a feature that can do this with auto-increment columns if they're part of a compound primary key, which may be both more efficient and clearer. (That is to say, if you just declare the id column to be auto-increment, MySQL may magically do what you want).

Also, I reckon we'll get at least one security bug from someone freaking out about our use of "predictable identifiers in URLs".

MySQL may magically do what you want

It doesn't. There's all kinds of caveats, especially with innodb.

https://dev.mysql.com/doc/refman/5.7/en/example-auto-increment.html

Oh dear lord there's a whole section dedicated to “AUTO_INCREMENT Handling in InnoDB”...

rfk · 2015-10-08T04:02:06Z

lib/db/schema/patch-018-019.sql

+  type VARCHAR(16) NOT NULL,
+  createdAt BIGINT UNSIGNED NOT NULL,
+  callbackURL VARCHAR(255),
+  INDEX devices_uid (uid),


You shouldn't need this index, it's a prefix of the primary key so it's already covered by the main table index.

rfk · 2015-10-08T04:07:42Z

lib/db/mem.js

+          var device = devices[id] || {}
+          delete sessionTokens[device.sessionTokenId]
+          delete account.devices[device.id]
+          return {}


Should this also delete it from the global devices object?

Actually, it's not clear to me what that global devices object is for, since you should always have a reference to the account when dealing with a device.

Actually

Yeah, this model is out of date from a previous design

rfk · 2015-10-08T04:18:51Z

lib/db/mem.js

+        function (err) {
+          return []
+        }
+      )


Clearly this changes the semantics of accountDevices from "list all the session tokens" to "list all the devices", which makes sense.

Seeing the UA information joined on here like this, I wonder if we should just create a device record for all sessions, put this information in that record directly, and remove it from the sessionToken table. At the least, we should ensure every login with service=sync receives a device record, even if it's from a client too old to register itself explicitly. Otherwise weird stuff will happen with e.g. the journeybuilder events.

If not, I think we should keep an API for listing all the sessionTokens on the account, we're going to need to do that sooner or later regardless of whether they qualify as devices or not.

listing sessions is already another thing https://github.com/mozilla/fxa-auth-db-mysql/blob/master/lib/db/mysql.js#L317-L321

rfk · 2015-10-08T04:20:23Z

lib/db/schema/patch-018-019.sql

+
+CREATE TABLE devices (
+  uid BINARY(16) NOT NULL,
+  id INT UNSIGNED NOT NULL,


We should consider making this smaller than INT, especially since it appears in the primary key. Anyone who registers 4294967295 devices has bigger problems than running out of device id space.

I had it as SMALLINTbut changed it because we may want a device id for web sessions at some point and I'd rather never reuse an id* (attach detach attach detach) just in case. 65k is a lot but not well beyond my imagination for strange user behavior.

*Restrictions may apply. Integrity guarantees do not include data loss due to bad design, rodents, cosmic rays, power failure, earthquakes, flipped bits, flipped switches, alcohol, box jellyfish, kangaroos, spice girls, chemicals, climate change, apocalypse, or other acts of god.

If you delete the device with the current MAX(id), won't that id be used again when you attach a subsequent device?

?

fixed disclaimer

We should run that by the lawyers to see if there's any we missed...

rfk · 2015-10-08T04:21:42Z

Also /cc @philbooth for additional f?

philbooth · 2015-10-08T09:00:10Z

Reading this PR has been educational for me. I'm definitely not qualified to have an opinion on most of it but I'm going to post a couple of questions in-line, just for stuff I'm curious about.

I notice the API docs aren't updated yet but that was probably on your final to-do list anyway.

philbooth · 2015-10-08T09:02:17Z

lib/db/schema/patch-018-019.sql

+  AND
+    id = inId;
+
+  SELECT


What's the reason for returning this from the update?

(other than consistency with the create I mean. I realise they behave the same for upsert sanity, but why does the upsert return the device?)

(oh wait, stupid question alert, is it because the id is auto-generated?)

philbooth · 2015-10-08T09:16:07Z

lib/db/schema/patch-018-019.sql

+  ORDER BY
+    d.id
+  DESC
+  LIMIT 1;


I notice these two queries aren't in a transaction. Is it possible (albeit unlikely) that a concurrent call to createDevice could cause the wrong device id to be returned here? If not, why not?

dannycoates · 2015-10-08T19:49:09Z

I thought the short device id would be a good idea and I think it was useful to see how it'd work, but I bet @rfk is right that a uuid will be the better option long term. The compromises don't really outweigh the possible benefits.

rfk · 2015-10-20T10:28:55Z

lib/db/mysql.js

+        deviceInfo.name,
+        deviceInfo.type,
+        deviceInfo.createdAt,
+        deviceInfo.callbackURL


IIUC passing null or undefined in here will cause the COALESCE to leave the existing value unchanged. How would a client explicitly delete e.g. their callbackURL? Pass in the empty string?

rfk · 2015-10-20T10:31:58Z

There's a lot of deletions in the shrinkwrap change, just want to sanity-check whether that's expected or whether it e.g. accidentally drops dev dependencies

rfk · 2015-10-20T10:34:39Z

lib/db/schema/patch-018-019.sql

+    sessionTokenId = COALESCE(inSessionTokenId, sessionTokenId),
+    name = COALESCE(inName, name),
+    type = COALESCE(inType, type),
+    callbackURL = COALESCE(inCallbackURL, callbackURL);


I think it would be worth a test that the ON DUPLICATE KEY UPDATE logic is correctly triggered here, e.g. doing two calls to upsertDevice and confirming that the selected keys are properly overridden.

rfk · 2015-10-20T10:36:26Z

@dannycoates this is looking solid, I had a couple of little quibbles there but otherwise I'm r+

vladikoff · 2015-10-20T15:32:54Z

fxa-auth-db-server/index.js

+  api.put(
+    '/account/:id/device/:deviceId',
+    function (req, res, next) {
+      db.upsertDevice(req.params.uid, req.params.deviceId, req.body)


@rfk @dannycoates is there a way to integrate the Activity Events into this? Do we we have a way to tell that a new device was added and log device.added to Heka? (could be separate bug)

I guess that logic would go to the fxa-auth-server pr?

for completeness: yes we should do this, and yes it belongs in the corresponding auth-server PR; thanks @vladikoff

vladikoff · 2015-10-20T15:42:14Z

package.json

+    "mozlog": "2.0.2",
+    "mysql": "2.7.0",
+    "request": "2.65.0",
+    "restify": "4.0.3"
  },
  "devDependencies": {


npm-shrinkwrap.json 331 additions, 1,790 deletions not shown

Did you remove --dev from shrinkwrap?

yes, intentionally.

vladikoff · 2015-10-20T15:46:18Z

lib/db/schema/patch-018-019.sql

@@ -0,0 +1,131 @@
+


is this missing the lib/db/schema/patch-019-018.sql DB script?

/me would love a custom linter for these db migrations

rfk · 2015-10-20T23:46:55Z

shipit!

device api endpoints

dannycoates added WIP waffle:progress labels Oct 5, 2015

dannycoates force-pushed the dannycoates:i104 branch 13 times, most recently from dee2a17 to f32ba72 Oct 7, 2015

wip on device api

aec4066

dannycoates force-pushed the dannycoates:i104 branch from f32ba72 to aec4066 Oct 8, 2015

rfk added this to the FxA-45: device registration milestone Oct 8, 2015

rfk reviewed Oct 8, 2015
View changes

philbooth reviewed Oct 8, 2015
View changes

use unique device ids

6e50197

dannycoates force-pushed the dannycoates:i104 branch from ee19540 to 6e50197 Oct 13, 2015

updated api.md for devices

c7ce08b

dannycoates force-pushed the dannycoates:i104 branch 4 times, most recently from b15e306 to 00bb3c1 Oct 19, 2015

updated deps

fc3d701

dannycoates force-pushed the dannycoates:i104 branch from 00bb3c1 to fc3d701 Oct 19, 2015

dannycoates removed the WIP label Oct 19, 2015

rfk self-assigned this Oct 19, 2015

vladikoff added waffle:review and removed waffle:progress labels Oct 19, 2015

rfk reviewed Oct 20, 2015
View changes

vladikoff reviewed Oct 20, 2015
View changes

dannycoates added 2 commits Oct 20, 2015

added patch-019-018.sql

514789a

extended device test to include update

3e7c673

dannycoates force-pushed the dannycoates:i104 branch from acdaf71 to 4a71b34 Oct 20, 2015

non-key fields must be nullable for upsert to work

285085b

dannycoates force-pushed the dannycoates:i104 branch from 4a71b34 to 285085b Oct 20, 2015

rfk added a commit that referenced this pull request Oct 20, 2015

Merge pull request #110 from dannycoates/i104

77b764a

device api endpoints

rfk merged commit 77b764a into mozilla:master Oct 20, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rfk removed the waffle:review label Oct 20, 2015

rfk mentioned this pull request Oct 29, 2015

feat(docs): first draft requirements for the device registration API mozilla/fxa#69

Merged

mozilla / fxa-auth-db-mysql Archived

device api endpoints #110

device api endpoints #110

Conversation

dannycoates commented Oct 5, 2015

dannycoates commented Oct 8, 2015

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

dannycoates Oct 8, 2015 Author Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

dannycoates Oct 8, 2015 Author Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

dannycoates Oct 8, 2015 Author Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

dannycoates Oct 8, 2015 Author Member

This comment has been minimized.

rfk Oct 8, 2015 Member

This comment has been minimized.

dannycoates Oct 8, 2015 Author Member

This comment has been minimized.

rfk Oct 8, 2015 Member

rfk commented Oct 8, 2015

philbooth commented Oct 8, 2015

This comment has been minimized.

philbooth Oct 8, 2015 Contributor

This comment has been minimized.

philbooth Oct 8, 2015 Contributor

This comment has been minimized.

philbooth Oct 8, 2015 Contributor

This comment has been minimized.

philbooth Oct 8, 2015 Contributor

dannycoates commented Oct 8, 2015

This comment has been minimized.

rfk Oct 20, 2015 Member

rfk commented Oct 20, 2015

This comment has been minimized.

rfk Oct 20, 2015 Member

rfk commented Oct 20, 2015

This comment has been minimized.

vladikoff Oct 20, 2015 Contributor

This comment has been minimized.

vladikoff Oct 20, 2015 Contributor

This comment has been minimized.

rfk Oct 20, 2015 Member

This comment has been minimized.

vladikoff Oct 20, 2015 Contributor

This comment has been minimized.

dannycoates Oct 20, 2015 Author Member

This comment has been minimized.

vladikoff Oct 20, 2015 Contributor

This comment has been minimized.

dannycoates Oct 20, 2015 Author Member

This comment has been minimized.

rfk Oct 20, 2015 Member

rfk commented Oct 20, 2015

rfk Oct 8, 2015
Member

rfk Oct 8, 2015
Member

rfk Oct 8, 2015
Member

rfk Oct 8, 2015
Member

dannycoates Oct 8, 2015
Author Member

rfk Oct 8, 2015
Member

rfk Oct 8, 2015
Member

rfk Oct 8, 2015
Member

dannycoates Oct 8, 2015
Author Member

rfk Oct 8, 2015
Member

dannycoates Oct 8, 2015
Author Member

rfk Oct 8, 2015
Member

dannycoates Oct 8, 2015
Author Member

rfk Oct 8, 2015
Member

dannycoates Oct 8, 2015
Author Member

rfk Oct 8, 2015
Member

philbooth Oct 8, 2015
Contributor

philbooth Oct 8, 2015
Contributor

philbooth Oct 8, 2015
Contributor

philbooth Oct 8, 2015
Contributor

rfk Oct 20, 2015
Member

rfk Oct 20, 2015
Member

vladikoff Oct 20, 2015
Contributor

vladikoff Oct 20, 2015
Contributor

rfk Oct 20, 2015
Member

vladikoff Oct 20, 2015
Contributor

dannycoates Oct 20, 2015
Author Member

vladikoff Oct 20, 2015
Contributor

dannycoates Oct 20, 2015
Author Member

rfk Oct 20, 2015
Member