From d45c1ae7bb8bce0d557e91ce06a29daad9f201cf Mon Sep 17 00:00:00 2001
From: Ritchie Martori <skawful@gmail.com>
Date: Wed, 1 Jul 2015 12:13:09 -0700
Subject: [PATCH] Fix logout to handle no or missing accessToken

Return 401 when the request does not provide any accessToken argument
or the token was not found.

Also simplify the implementation of the `logout` method to make only
a single database call (`deleteById`) instead of `findById` + `delete`.
---
 common/models/user.js    | 22 ++++++++++++++++------
 test/user.integration.js | 11 +++++++++++
 test/user.test.js        | 16 ++++++++++++++++
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/common/models/user.js b/common/models/user.js
index 324d2f93..5a246049 100644
--- a/common/models/user.js
+++ b/common/models/user.js
@@ -292,13 +292,23 @@ module.exports = function(User) {
 
   User.logout = function(tokenId, fn) {
     fn = fn || utils.createPromiseCallback();
-    this.relations.accessTokens.modelTo.findById(tokenId, function(err, accessToken) {
+
+    if (!tokenId) {
+      var err = new Error(g.f('{{accessToken}} is required to logout'));
+      err.status = 401;
+      process.nextTick(fn, err);
+      return fn.promise;
+    }
+
+    this.relations.accessTokens.modelTo.destroyById(tokenId, function(err, info) {
       if (err) {
         fn(err);
-      } else if (accessToken) {
-        accessToken.destroy(fn);
+      } else if ('count' in info && info.count === 0) {
+        var err = new Error(g.f('Could not find {{accessToken}}'));
+        err.status = 401;
+        fn(err);
       } else {
-        fn(new Error(g.f('could not find {{accessToken}}')));
+        fn();
       }
     });
     return fn.promise;
@@ -753,10 +763,10 @@ module.exports = function(User) {
       {
         description: 'Logout a user with access token.',
         accepts: [
-          {arg: 'access_token', type: 'string', required: true, http: function(ctx) {
+          {arg: 'access_token', type: 'string', http: function(ctx) {
             var req = ctx && ctx.req;
             var accessToken = req && req.accessToken;
-            var tokenID = accessToken && accessToken.id;
+            var tokenID = accessToken ? accessToken.id : undefined;
 
             return tokenID;
           }, description: 'Do not supply this argument, it is automatically extracted ' +
diff --git a/test/user.integration.js b/test/user.integration.js
index 3dfcaecc..56bd0b87 100644
--- a/test/user.integration.js
+++ b/test/user.integration.js
@@ -106,6 +106,17 @@ describe('users - integration', function() {
             .expect(200, done);
         });
     });
+
+    it('returns 401 on logout with no access token', function(done) {
+      this.post('/api/users/logout')
+        .expect(401, done);
+    });
+
+    it('returns 401 on logout with invalid access token', function(done) {
+      this.post('/api/users/logout')
+        .set('Authorization', 'unknown-token')
+        .expect(401, done);
+    });
   });
 
   describe('sub-user', function() {
diff --git a/test/user.test.js b/test/user.test.js
index 5329926d..5ba640ab 100644
--- a/test/user.test.js
+++ b/test/user.test.js
@@ -1193,6 +1193,22 @@ describe('User', function() {
       }
     });
 
+    it('fails when accessToken is not provided', function(done) {
+      User.logout(undefined, function(err) {
+        expect(err).to.have.property('message');
+        expect(err).to.have.property('status', 401);
+        done();
+      });
+    });
+
+    it('fails when accessToken is not found', function(done) {
+      User.logout('expired-access-token', function(err) {
+        expect(err).to.have.property('message');
+        expect(err).to.have.property('status', 401);
+        done();
+      });
+    });
+
     function verify(token, done) {
       assert(token);