From 1dac9ada0b471bd78d59d0c357848bf936003005 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 c193ac42..be45933d 100644
--- a/common/models/user.js
+++ b/common/models/user.js
@@ -293,13 +293,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(function() { 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) {
+        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;
@@ -743,10 +753,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 ' +
             'from request headers.',
diff --git a/test/user.integration.js b/test/user.integration.js
index 45ea04b9..a27b4ed4 100644
--- a/test/user.integration.js
+++ b/test/user.integration.js
@@ -148,6 +148,17 @@ describe('users - integration', function() {
         });
       });
     });
+
+    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 7969497e..2b7dbffc 100644
--- a/test/user.test.js
+++ b/test/user.test.js
@@ -1186,6 +1186,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);