fix(tokens): Avoid quadratic behaviour when listing active clients. (#9…

…); r=vladikoff

docs/api.md

@@ -68,6 +68,8 @@ The currently-defined error responses are:
   - [POST /v1/developer/activate][developer-activate]
 - [POST /v1/verify][verify]
 - [POST /v1/key-data][key-data]
+- [GET /v1/client-tokens][client-tokens]
+- [DELETE /v1/client-tokens/:id][client-tokens-delete]
 
 ### GET /v1/client/:id
 
@@ -575,6 +577,78 @@ A valid response will return JSON the scoped key information for every scope tha
 }
 ```
 
+### GET /v1/client-tokens
+
+This endpoint returns a list of all clients with active OAuth tokens for the user,
+including the the scopes granted to each client
+and the last time each client was active.
+It must be authenticated with an OAuth token bearing scope "clients:write".
+
+#### Request
+
+**Example:**
+
+```sh
+curl -X GET \
+  https://oauth.accounts.firefox.com/v1/client-tokens \
+  -H 'cache-control: no-cache' \
+  -H "Authorization: Bearer 558f9980ad5a9c279beb52123653967342f702e84d3ab34c7f80427a6a37e2c0"
+```
+
+#### Response
+
+A valid 200 response will be a JSON array
+where each item as the following properties:
+
+- `id`: The hex id of the client.
+- `name`: The string name of the client.
+- `lastAccessTime`: Integer last-access time for the client.
+- `lastAccessTimeFormatted`: Localized string last-access time for the client.
+- `scope`: Sorted list of all scopes granted to the client.
+
+**Example:**
+
+```json
+[
+  {
+    "id": "5901bd09376fadaa",
+    "name": "Example",
+    "lastAccessTime": 1528334748000,
+    "lastAccessTimeFormatted": "13 days ago",
+    "scope": ["openid", "profile"]
+  },
+  {
+    "id": "23d10a14f474ca41",
+    "name": "Example Two",
+    "lastAccessTime": 1476677854037,
+    "lastAccessTimeFormatted": "2 years ago",
+    "scope": ["profile:email", "profile:uid"]
+  }
+]
+```
+
+### DELETE /v1/client-tokens/:id
+
+This endpoint deletes all tokens granted to a given client.
+It must be authenticated with an OAuth token bearing scope "clients:write".
+
+#### Request Parameters
+
+- `id`: The `client_id` of the client whose tokens should be deleted.
+
+**Example:**
+
+```sh
+curl -X DELETE
+  https://oauth.accounts.firefox.com/v1/client-tokens/5901bd09376fadaa
+  -H "Authorization: Bearer 558f9980ad5a9c279beb52123653967342f702e84d3ab34c7f80427a6a37e2c0"
+```
+
+#### Response
+
+A valid 200 response will return an empty JSON object.
+
+
 
 [client]: #get-v1clientid
 [register]: #post-v1clientregister
@@ -589,5 +663,7 @@ A valid response will return JSON the scoped key information for every scope tha
 [developer-activate]: #post-v1developeractivate
 [jwks]: #get-v1jwks
 [key-data]: #post-v1post-keydata
+[client-tokens]: #get-v1client-tokens
+[client-tokens-delete]: #delete-v1client-tokensid
 
 [Service Clients]: ./service-clients.md

lib/db/helpers.js

@@ -10,16 +10,20 @@ const unbuf = require('buf').unbuf.hex;
 
 module.exports = {
   /**
-   * This helper converts the provided 'activeClientTokens' array and
-   * groups them by the token OAuth client.
-   * In the end it creates a sorted array where all token scopes are unified together.
+   * This helper takes a list of active oauth tokens and produces an aggregate
+   * summary of the active clients, by:
+   *
+   *   * merging all scopes into a single set
+   *   * taking the max token creation time a the last access time
+   *
+   * The resulting array is in sorted order by last access time, then client name.
    *
    * @param {Array} activeClientTokens
-   * A joined array of tokens and their client names:
+   * An array of OAuth tokens, annotated with client name:
    * (OAuth client) name|(OAuth client) id|(Token) createdAt|(Token) scope
    * @returns {Array}
    */
-  getActiveClientTokens: function getActiveTokens(activeClientTokens) {
+  aggregateActiveClients: function aggregateActiveClients(activeClientTokens) {
     if (! activeClientTokens) {
       return [];
     }
@@ -32,50 +36,40 @@ module.exports = {
 
       if (! activeClients[clientIdHex]) {
         // add the OAuth client if not already in the Object
-        activeClients[clientIdHex] = clientTokenObj;
-        activeClients[clientIdHex].scope = [];
+        activeClients[clientIdHex] = {
+          id: clientTokenObj.id,
+          name: clientTokenObj.name,
+          lastAccessTime: clientTokenObj.createdAt,
+          scope: new Set()
+        };
       }
 
       scope.forEach(function (clientScope) {
         // aggregate the scopes from all available tokens
-        if (activeClients[clientIdHex].scope.indexOf(clientScope) === -1) {
-          activeClients[clientIdHex].scope.push(clientScope);
-        }
+        activeClients[clientIdHex].scope.add(clientScope);
       });
 
       var clientTokenTime = clientTokenObj.createdAt;
-      if (clientTokenTime > activeClients[clientIdHex].createdAt) {
+      if (clientTokenTime > activeClients[clientIdHex].lastAccessTime) {
         // only update the createdAt if it is newer
-        activeClients[clientIdHex].createdAt = clientTokenTime;
+        activeClients[clientIdHex].lastAccessTime = clientTokenTime;
       }
     });
 
     // Sort the scopes alphabetically, convert the Object structure to an array
     var activeClientsArray = Object.keys(activeClients).map(function (key) {
       var scopes = activeClients[key].scope;
-      scopes.sort(function(a, b) {
-        if (b < a) {
-          return 1;
-        }
-
-        if (b > a) {
-          return -1;
-        }
-
-        return 0;
-      });
-      activeClients[key].scope = scopes;
+      activeClients[key].scope = Array.from(scopes.values()).sort();
       return activeClients[key];
     });
 
-    // Sort the final Array structure first by createdAt and then name
-    var customSort = activeClientsArray.slice(0);
-    customSort.sort(function(a, b) {
-      if (b.createdAt > a.createdAt) {
+    // Sort the final Array structure first by lastAccessTime and then name
+    activeClientsArray.sort(function(a, b) {
+      if (b.lastAccessTime > a.lastAccessTime) {
         return 1;
       }
 
-      if (b.createdAt < a.createdAt) {
+      if (b.lastAccessTime < a.lastAccessTime) {
         return -1;
       }
 
@@ -90,6 +84,6 @@ module.exports = {
       return 0;
     });
 
-    return customSort;
+    return activeClientsArray;
   }
 };

lib/db/memory.js

@@ -264,7 +264,7 @@ MemoryStore.prototype = {
    * @param {String} uid User ID as hex
    * @returns {Promise}
    */
-  getActiveClientTokensByUid: function getActiveServicesByUid(uid) {
+  getActiveClientsByUid: function getActiveClientsByUid(uid) {
     var self = this;
     if (! uid) {
       return P.reject(new Error('Uid is required'));
@@ -288,7 +288,7 @@ MemoryStore.prototype = {
       }
     }
 
-    return P.resolve(helpers.getActiveClientTokens(activeClientTokens));
+    return P.resolve(helpers.aggregateActiveClients(activeClientTokens));
   },
 
   /**

lib/db/mysql/index.js

@@ -193,10 +193,9 @@ const QUERY_PURGE_EXPIRED_TOKENS = 'DELETE FROM tokens WHERE clientId NOT IN (?)
 // Returns the most recent token used with a client name and client id.
 // Does not include clients that canGrant.
 const QUERY_ACTIVE_CLIENT_TOKENS_BY_UID =
-  'SELECT DISTINCT clients.name, clients.id, tokens.createdAt as createdAt, tokens.scope as scope FROM clients, tokens ' +
-  'WHERE tokens.expiresAt > NOW() AND clients.canGrant = 0 AND clients.id = tokens.clientId AND tokens.userId=? ' +
-  'ORDER BY createdAt DESC, clients.name ' +
-  'LIMIT 10000;';
+  'SELECT tokens.clientId, tokens.createdAt, tokens.scope, clients.name ' +
+  'FROM tokens LEFT OUTER JOIN clients ON clients.id = tokens.clientId ' +
+  'WHERE tokens.userId=? AND tokens.expiresAt > NOW() AND clients.canGrant = 0;';
 const DELETE_ACTIVE_TOKENS_BY_CLIENT_AND_UID =
   'DELETE FROM tokens WHERE clientId=? AND userId=?';
 const DELETE_ACTIVE_REFRESH_TOKENS_BY_CLIENT_AND_UID =
@@ -459,11 +458,11 @@ MysqlStore.prototype = {
    * @param {String} uid User ID as hex
    * @returns {Promise}
    */
-  getActiveClientTokensByUid: function getActiveClientTokensByUid(uid) {
+  getActiveClientsByUid: function getActiveClientsByUid(uid) {
     return this._read(QUERY_ACTIVE_CLIENT_TOKENS_BY_UID, [
       buf(uid)
     ]).then(function(activeClientTokens) {
-      return helpers.getActiveClientTokens(activeClientTokens);
+      return helpers.aggregateActiveClients(activeClientTokens);
     });
   },
 

lib/routes/client-tokens/list.js

@@ -12,7 +12,7 @@ const localizeTimestamp = require('fxa-shared').l10n.localizeTimestamp({
 });
 
 function serialize(client, acceptLanguage) {
-  var lastAccessTime = client.createdAt.getTime();
+  var lastAccessTime = client.lastAccessTime.getTime();
   var lastAccessTimeFormatted = localizeTimestamp.format(lastAccessTime, acceptLanguage);
 
   return {
@@ -30,7 +30,7 @@ module.exports = {
     scope: [SCOPE_CLIENT_WRITE]
   },
   handler: function activeServices(req, reply) {
-    return db.getActiveClientTokensByUid(req.auth.credentials.user)
+    return db.getActiveClientsByUid(req.auth.credentials.user)
       .done(function(clients) {
         reply(clients.map(function(client) {
           return serialize(client, req.headers['accept-language']);

test/db/helpers.js

@@ -6,13 +6,13 @@ const assert = require('insist');
 const helpers = require('../../lib/db/helpers');
 const unique = require('../../lib/unique');
 
-describe('getActiveClientTokens', function() {
+describe('aggregateActiveClients', function() {
   var uid;
-  var activeClientIds;
+  var activeClientTokens;
   beforeEach(function() {
     uid = unique(16).toString('hex');
 
-    activeClientIds = [
+    activeClientTokens = [
       {
         id: uid,
         createdAt: '2017-01-26T14:28:16.219Z',
@@ -34,11 +34,11 @@ describe('getActiveClientTokens', function() {
     ];
   });
 
-  it('returns union of sorted scopes and latest createdAt', function() {
-    var res = helpers.getActiveClientTokens(activeClientIds);
+  it('returns union of sorted scopes, and latest createdAt as last access time', function() {
+    var res = helpers.aggregateActiveClients(activeClientTokens);
     assert.equal(res[0].id, uid);
     assert.equal(res[0].name, '123Done');
     assert.deepEqual(res[0].scope, ['clients:write', 'profile', 'profile:write']);
-    assert.equal(res[0].createdAt, '2017-01-28T14:28:16.219Z');
+    assert.equal(res[0].lastAccessTime, '2017-01-28T14:28:16.219Z');
   });
 });

test/db/index.js

@@ -589,11 +589,11 @@ describe('db', function() {
 
   describe('client-tokens', function () {
 
-    describe('getActiveClientTokensByUid', function() {
+    describe('getActiveClientsByUid', function() {
       var userId = buf(randomString(16));
 
       it('should return the active clients', function() {
-        return db.getActiveClientTokensByUid(userId)
+        return db.getActiveClientsByUid(userId)
           .then(
             function(result) {
               assert.equal(result.length, 0);