Skip to content

Commit 1dac9ad

Browse files
ritchbajtos
authored andcommitted
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`.
1 parent 2ade55e commit 1dac9ad

File tree

3 files changed

+43
-6
lines changed

3 files changed

+43
-6
lines changed

common/models/user.js

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -293,13 +293,23 @@ module.exports = function(User) {
293293

294294
User.logout = function(tokenId, fn) {
295295
fn = fn || utils.createPromiseCallback();
296-
this.relations.accessTokens.modelTo.findById(tokenId, function(err, accessToken) {
296+
297+
if (!tokenId) {
298+
var err = new Error(g.f('{{accessToken}} is required to logout'));
299+
err.status = 401;
300+
process.nextTick(function() { fn(err); });
301+
return fn.promise;
302+
}
303+
304+
this.relations.accessTokens.modelTo.destroyById(tokenId, function(err, info) {
297305
if (err) {
298306
fn(err);
299-
} else if (accessToken) {
300-
accessToken.destroy(fn);
307+
} else if ('count' in info && info.count === 0) {
308+
err = new Error(g.f('Could not find {{accessToken}}'));
309+
err.status = 401;
310+
fn(err);
301311
} else {
302-
fn(new Error(g.f('could not find {{accessToken}}')));
312+
fn();
303313
}
304314
});
305315
return fn.promise;
@@ -743,10 +753,10 @@ module.exports = function(User) {
743753
{
744754
description: 'Logout a user with access token.',
745755
accepts: [
746-
{arg: 'access_token', type: 'string', required: true, http: function(ctx) {
756+
{arg: 'access_token', type: 'string', http: function(ctx) {
747757
var req = ctx && ctx.req;
748758
var accessToken = req && req.accessToken;
749-
var tokenID = accessToken && accessToken.id;
759+
var tokenID = accessToken ? accessToken.id : undefined;
750760
return tokenID;
751761
}, description: 'Do not supply this argument, it is automatically extracted ' +
752762
'from request headers.',

test/user.integration.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,17 @@ describe('users - integration', function() {
148148
});
149149
});
150150
});
151+
152+
it('returns 401 on logout with no access token', function(done) {
153+
this.post('/api/users/logout')
154+
.expect(401, done);
155+
});
156+
157+
it('returns 401 on logout with invalid access token', function(done) {
158+
this.post('/api/users/logout')
159+
.set('Authorization', 'unknown-token')
160+
.expect(401, done);
161+
});
151162
});
152163

153164
describe('sub-user', function() {

test/user.test.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1186,6 +1186,22 @@ describe('User', function() {
11861186
}
11871187
});
11881188

1189+
it('fails when accessToken is not provided', function(done) {
1190+
User.logout(undefined, function(err) {
1191+
expect(err).to.have.property('message');
1192+
expect(err).to.have.property('status', 401);
1193+
done();
1194+
});
1195+
});
1196+
1197+
it('fails when accessToken is not found', function(done) {
1198+
User.logout('expired-access-token', function(err) {
1199+
expect(err).to.have.property('message');
1200+
expect(err).to.have.property('status', 401);
1201+
done();
1202+
});
1203+
});
1204+
11891205
function verify(token, done) {
11901206
assert(token);
11911207

0 commit comments

Comments
 (0)