fix(db): don't return zombie devices from accountDevices

#165

r=vbudhram

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

@@ -1021,9 +1021,10 @@ module.exports = function(config, DB) {
         test(
           'db.accountDevices',
           function (t) {
-            t.plan(73)
+            t.plan(75)
             var deviceId = newUuid()
             var sessionTokenId = hex32()
+            var zombieSessionTokenId = hex32()
             var createdAt = Date.now()
             var deviceInfo = {
               name: 'test device',
@@ -1157,7 +1158,7 @@ module.exports = function(config, DB) {
                 return db.createSessionToken(newSessionTokenId, SESSION_TOKEN)
               })
               .then(function () {
-                // Update the device
+                // Update the device name and session token
                 return db.updateDevice(ACCOUNT.uid, deviceId, {
                   sessionTokenId: newSessionTokenId,
                   name: 'updated name',
@@ -1182,6 +1183,8 @@ module.exports = function(config, DB) {
                 t.equal(device.callbackURL, deviceInfo.callbackURL, 'callbackURL unchanged')
                 t.equal(device.callbackPublicKey, deviceInfo.callbackPublicKey, 'callbackPublicKey unchanged')
                 t.equal(device.callbackAuthKey, deviceInfo.callbackAuthKey, 'callbackAuthKey unchanged')
+
+                // Update the device type and callback params
                 return db.updateDevice(ACCOUNT.uid, deviceId, {
                   type: 'desktop',
                   callbackURL: '',
@@ -1208,6 +1211,36 @@ module.exports = function(config, DB) {
                 t.equal(device.callbackPublicKey, '', 'callbackPublicKey updated')
                 t.equal(device.callbackAuthKey, '', 'callbackAuthKey updated')
 
+                // Make the device a zombie, by giving it a non-existent session token
+                return db.updateDevice(ACCOUNT.uid, deviceId, {
+                  sessionTokenId: zombieSessionTokenId
+                })
+                .catch(function () {
+                  t.fail('updating an existing device should not have failed')
+                })
+              })
+              .then(function () {
+                // Fetch all of the devices for the account
+                return db.accountDevices(ACCOUNT.uid)
+              })
+              .then(function (devices) {
+                t.equal(devices.length, 0, 'devices is empty')
+
+                // Reinstate the previous session token for the device
+                return db.updateDevice(ACCOUNT.uid, deviceId, {
+                  sessionTokenId: newSessionTokenId
+                })
+                .catch(function () {
+                  t.fail('updating an existing device should not have failed')
+                })
+              })
+              .then(function () {
+                // Fetch all of the devices for the account
+                return db.accountDevices(ACCOUNT.uid)
+              })
+              .then(function (devices) {
+                t.equal(devices.length, 1, 'devices contains one item again')
+
                 // Attempt to create a second device with the same session token
                 return db.createDevice(ACCOUNT.uid, newUuid(), {
                   sessionTokenId: newSessionTokenId,

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

@@ -508,8 +508,9 @@ module.exports = function(cfg, server) {
   test(
     'device handling',
     function (t) {
-      t.plan(51)
+      t.plan(61)
       var user = fake.newUserDataHex()
+      var zombieUser = fake.newUserDataHex()
       return client.getThen('/account/' + user.accountId + '/devices')
         .then(function(r) {
           respOk(t, r)
@@ -582,6 +583,33 @@ module.exports = function(cfg, server) {
           t.equal(devices[0].uaOSVersion, user.sessionToken.uaOSVersion, 'uaOSVersion is correct')
           t.equal(devices[0].uaDeviceType, user.sessionToken.uaDeviceType, 'uaDeviceType is correct')
           t.equal(devices[0].lastAccessTime, user.sessionToken.createdAt, 'lastAccessTime is correct')
+
+          return client.postThen('/account/' + user.accountId + '/device/' + user.deviceId + '/update', {
+            sessionTokenId: zombieUser.sessionTokenId
+          })
+        })
+        .then(function(r) {
+          respOk(t, r)
+          return client.getThen('/account/' + user.accountId + '/devices')
+        })
+        .then(function(r) {
+          respOk(t, r)
+          var devices = r.obj
+          t.equal(devices.length, 0, 'devices is empty')
+
+          return client.postThen('/account/' + user.accountId + '/device/' + user.deviceId + '/update', {
+            sessionTokenId: user.sessionTokenId
+          })
+        })
+        .then(function(r) {
+          respOk(t, r)
+          return client.getThen('/account/' + user.accountId + '/devices')
+        })
+        .then(function(r) {
+          respOk(t, r)
+          var devices = r.obj
+          t.equal(devices.length, 1, 'devices contains one item again')
+
           return client.delThen('/account/' + user.accountId + '/device/' + user.deviceId)
         })
         .then(function(r) {

lib/db/mem.js

@@ -392,19 +392,25 @@ module.exports = function (log, error) {
     return getAccountByUid(uid)
       .then(
         function(account) {
-          return Object.keys(account.devices).map(
-            function (id) {
-              var device = account.devices[id]
-              var sessionKey = (device.sessionTokenId || '').toString('hex')
-              var session = sessionTokens[sessionKey]
-              if (session) {
-                SESSION_FIELDS.forEach(function (key) {
-                  device[key] = session[key]
-                })
+          return Object.keys(account.devices)
+            .map(
+              function (id) {
+                var device = account.devices[id]
+                var sessionKey = (device.sessionTokenId || '').toString('hex')
+                var session = sessionTokens[sessionKey]
+                if (session) {
+                  SESSION_FIELDS.forEach(function (key) {
+                    device[key] = session[key]
+                  })
+                  return device
+                }
               }
-              return device
-            }
-          )
+            )
+            .filter(
+              function (device) {
+                return !! device
+              }
+            )
         },
         function (err) {
           return []

lib/db/mysql.js

@@ -336,7 +336,7 @@ module.exports = function (log, error) {
   //          d.callbackPublicKey, d.callbackAuthKey, s.uaBrowser, s.uaBrowserVersion,
   //          s.uaOS, s.uaOSVersion, s.uaDeviceType, s.lastAccessTime
   // Where  : d.uid = $1
-  var ACCOUNT_DEVICES = 'CALL accountDevices_4(?)'
+  var ACCOUNT_DEVICES = 'CALL accountDevices_5(?)'
 
   MySql.prototype.accountDevices = function (uid) {
     return this.readOneFromFirstResult(ACCOUNT_DEVICES, [uid])

lib/db/patch.js

@@ -4,4 +4,4 @@
 
 // The expected patch level of the database. Update if you add a new
 // patch in the schema/ directory.
-module.exports.level = 30
+module.exports.level = 31

lib/db/schema/patch-030-031.sql

@@ -0,0 +1,28 @@
+-- Alter the accountDevices stored procedure to use INNER JOIN instead of
+-- LEFT JOIN. There are zombie device records, we don't care about them.
+CREATE PROCEDURE `accountDevices_5` (
+  IN `uidArg` BINARY(16)
+)
+BEGIN
+  SELECT
+    d.uid,
+    d.id,
+    d.sessionTokenId,
+    d.name,
+    d.type,
+    d.createdAt,
+    d.callbackURL,
+    d.callbackPublicKey,
+    d.callbackAuthKey,
+    s.uaBrowser,
+    s.uaBrowserVersion,
+    s.uaOS,
+    s.uaOSVersion,
+    s.uaDeviceType,
+    s.lastAccessTime
+  FROM devices d INNER JOIN sessionTokens s
+  ON d.sessionTokenId = s.tokenId
+  WHERE d.uid = uidArg;
+END;
+
+UPDATE dbMetadata SET value = '31' WHERE name = 'schema-patch-level';

lib/db/schema/patch-031-030.sql

@@ -0,0 +1,4 @@
+-- DROP PROCEDURE `accountDevices_5`;
+
+-- UPDATE dbMetadata SET value = '30' WHERE name = 'schema-patch-level';
+