Skip to content

Commit

Permalink
🐛 🎨 old accesstokens are not cleaned up (#8065)
Browse files Browse the repository at this point in the history
closes #8035
- create auth/utils
- use authUtils.createTokens for all cases
- decrease the expiry of the old access token before creating a new one
  • Loading branch information
kirrg001 authored and kevinansfield committed Mar 1, 2017
1 parent 23c0d69 commit fa38257
Show file tree
Hide file tree
Showing 8 changed files with 207 additions and 123 deletions.
32 changes: 0 additions & 32 deletions core/server/api/authentication.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,38 +132,6 @@ function setupTasks(setupData) {
* **See:** [API Methods](index.js.html#api%20methods)
*/
authentication = {
/**
* Generate a pair of tokens
*/
createTokens: function createTokens(data, options) {
var localAccessToken = globalUtils.uid(191),
localRefreshToken = globalUtils.uid(191),
accessExpires = Date.now() + globalUtils.ONE_MONTH_MS,
refreshExpires = Date.now() + globalUtils.SIX_MONTH_MS,
client = options.context.client_id,
user = options.context.user;

return models.Accesstoken.add({
token: localAccessToken,
user_id: user,
client_id: client,
expires: accessExpires
}).then(function () {
return models.Refreshtoken.add({
token: localRefreshToken,
user_id: user,
client_id: client,
expires: refreshExpires
});
}).then(function () {
return {
access_token: localAccessToken,
refresh_token: localRefreshToken,
expires_in: globalUtils.ONE_MONTH_S
};
});
},

/**
* @description generate a reset token for a given email address
* @param {Object} object
Expand Down
28 changes: 3 additions & 25 deletions core/server/auth/authenticate.js
Original file line number Diff line number Diff line change
@@ -1,38 +1,16 @@
var passport = require('passport'),
authUtils = require('./utils'),
errors = require('../errors'),
models = require('../models'),
events = require('../events'),
i18n = require('../i18n'),
authenticate;

function isBearerAutorizationHeader(req) {
var parts,
scheme,
credentials;

if (req.headers && req.headers.authorization) {
parts = req.headers.authorization.split(' ');
} else if (req.query && req.query.access_token) {
return true;
} else {
return false;
}

if (parts.length === 2) {
scheme = parts[0];
credentials = parts[1];
if (/^Bearer$/i.test(scheme)) {
return true;
}
}
return false;
}

authenticate = {
// ### Authenticate Client Middleware
authenticateClient: function authenticateClient(req, res, next) {
// skip client authentication if bearer token is present
if (isBearerAutorizationHeader(req)) {
if (authUtils.getBearerAutorizationToken(req)) {
return next();
}

Expand Down Expand Up @@ -92,7 +70,7 @@ authenticate = {

events.emit('user.authenticated', user);
return next(null, user, info);
} else if (isBearerAutorizationHeader(req)) {
} else if (authUtils.getBearerAutorizationToken(req)) {
return next(new errors.UnauthorizedError({
message: i18n.t('errors.middleware.auth.accessDenied')
}));
Expand Down
54 changes: 27 additions & 27 deletions core/server/auth/oauth.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
var oauth2orize = require('oauth2orize'),
passport = require('passport'),
models = require('../models'),
utils = require('../utils'),
errors = require('../errors'),
authenticationAPI = require('../api/authentication'),
authUtils = require('./utils'),
spamPrevention = require('../middleware/api/spam-prevention'),
i18n = require('../i18n'),
oauthServer,
Expand All @@ -15,23 +14,18 @@ function exchangeRefreshToken(client, refreshToken, scope, body, authInfo, done)
if (!model) {
return done(new errors.NoPermissionError({message: i18n.t('errors.middleware.oauth.invalidRefreshToken')}), false);
} else {
var token = model.toJSON(),
accessToken = utils.uid(191),
accessExpires = Date.now() + utils.ONE_MONTH_MS,
refreshExpires = Date.now() + utils.SIX_MONTH_MS;
var token = model.toJSON();

if (token.expires > Date.now()) {
spamPrevention.userLogin.reset(authInfo.ip, body.refresh_token + 'login');

models.Accesstoken.add({
token: accessToken,
user_id: token.user_id,
client_id: token.client_id,
expires: accessExpires
}).then(function then() {
return models.Refreshtoken.edit({expires: refreshExpires}, {id: token.id});
}).then(function then() {
return done(null, accessToken, {expires_in: utils.ONE_MONTH_S});
authUtils.createTokens({
clientId: token.client_id,
userId: token.user_id,
oldAccessToken: authInfo.accessToken,
oldRefreshToken: refreshToken
}).then(function (response) {
return done(null, response.access_token, {expires_in: response.expires_in});
}).catch(function handleError(error) {
return done(error, false);
});
Expand All @@ -54,7 +48,10 @@ function exchangePassword(client, username, password, scope, body, authInfo, don
// Validate the user
return models.User.check({email: username, password: password})
.then(function then(user) {
return authenticationAPI.createTokens({}, {context: {client_id: client.id, user: user.id}});
return authUtils.createTokens({
clientId: client.id,
userId: user.id
});
})
.then(function then(response) {
spamPrevention.userLogin.reset(authInfo.ip, username + 'login');
Expand All @@ -72,6 +69,7 @@ function exchangeAuthorizationCode(req, res, next) {
message: i18n.t('errors.middleware.auth.accessDenied')
}));
}

req.query.code = req.body.authorizationCode;

passport.authenticate('ghost', {session: false, failWithError: false}, function authenticate(err, user) {
Expand All @@ -89,17 +87,18 @@ function exchangeAuthorizationCode(req, res, next) {

spamPrevention.userLogin.reset(req.authInfo.ip, req.body.authorizationCode + 'login');

authenticationAPI.createTokens({}, {context: {client_id: req.client.id, user: user.id}})
.then(function then(response) {
res.json({
access_token: response.access_token,
refresh_token: response.refresh_token,
expires_in: response.expires_in
});
})
.catch(function (err) {
next(err);
authUtils.createTokens({
clientId: req.client.id,
userId: user.id
}).then(function then(response) {
res.json({
access_token: response.access_token,
refresh_token: response.refresh_token,
expires_in: response.expires_in
});
}).catch(function (err) {
next(err);
});
})(req, res, next);
}

Expand Down Expand Up @@ -156,7 +155,8 @@ oauth = {
* Important: only used for resetting the brute count (access to req.ip)
*/
req.authInfo = {
ip: req.ip
ip: req.ip,
accessToken: authUtils.getBearerAutorizationToken(req)
};

return oauthServer.token()(req, res, next);
Expand Down
116 changes: 116 additions & 0 deletions core/server/auth/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
var Promise = require('bluebird'),
_ = require('lodash'),
debug = require('ghost-ignition').debug('auth:utils'),
models = require('../models'),
globalUtils = require('../utils'),
knex = require('../data/db').knex,
_private = {};

/**
* The initial idea was to delete all old tokens connected to a user and a client.
* But if multiple browsers/apps are using the same client, we would log out them out.
* So the idea is to always decrease the expiry of the old access token if available.
* This access token auto expires and get's cleaned up on bootstrap (see oauth.js).
*/
_private.decreaseOldAccessTokenExpiry = function decreaseOldAccessTokenExpiry(data, options) {
debug('decreaseOldAccessTokenExpiry', data, options);

if (!data.token) {
return Promise.resolve();
}

return models.Accesstoken.findOne(data, options)
.then(function (oldAccessToken) {
if (!oldAccessToken) {
return Promise.resolve();
}

return models.Accesstoken.edit({
expires: Date.now() + globalUtils.FIVE_MINUTES_MS
}, _.merge({id: oldAccessToken.id}, options));
});
};

_private.destroyOldRefreshToken = function destroyOldRefreshToken(options) {
debug('destroyOldRefreshToken', options);

if (!options.token) {
return Promise.resolve();
}

return models.Refreshtoken.destroyByToken(options);
};

/**
* A user can have one token per client at a time.
* If the user requests a new pair of tokens, we decrease the expiry of the old access token
* and re-add the refresh token (this happens because this function is used for 3 different cases).
* If the operation fails in between, the user can still use e.g. the refresh token and try again.
*/
module.exports.createTokens = function createTokens(options) {
options = options || {};
debug('createTokens');

var oldAccessToken = options.oldAccessToken,
oldRefreshToken = options.oldRefreshToken,
newAccessToken = globalUtils.uid(191),
newRefreshToken = oldRefreshToken || globalUtils.uid(191),
accessExpires = Date.now() + globalUtils.ONE_MONTH_MS,
refreshExpires = Date.now() + globalUtils.SIX_MONTH_MS,
clientId = options.clientId,
userId = options.userId,
modelOptions;

return knex.transaction(function (transaction) {
modelOptions = {transacting: transaction};

return _private.decreaseOldAccessTokenExpiry({token: oldAccessToken}, modelOptions)
.then(function () {
return _private.destroyOldRefreshToken(_.merge({
token: oldRefreshToken
}, modelOptions));
})
.then(function () {
return models.Accesstoken.add({
token: newAccessToken,
user_id: userId,
client_id: clientId,
expires: accessExpires
}, modelOptions);
})
.then(function () {
return models.Refreshtoken.add({
token: newRefreshToken,
user_id: userId,
client_id: clientId,
expires: refreshExpires
}, modelOptions);
})
.then(function () {
return {
access_token: newAccessToken,
refresh_token: newRefreshToken,
expires_in: globalUtils.ONE_MONTH_S
};
});
});
};

module.exports.getBearerAutorizationToken = function (req) {
var parts,
scheme,
token;

if (req.headers && req.headers.authorization) {
parts = req.headers.authorization.split(' ');
scheme = parts[0];

if (/^Bearer$/i.test(scheme)) {
token = parts[1];
}
} else if (req.query && req.query.access_token) {
token = req.query.access_token;
}

return token;
};
1 change: 1 addition & 0 deletions core/server/models/base/token.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ Basetoken = ghostBookshelf.Model.extend({
return collection.invokeThen('destroy', options);
});
},

/**
* ### destroyByUser
* @param {[type]} options has context and id. Context is the user doing the destroy, id is the user to destroy
Expand Down
1 change: 1 addition & 0 deletions core/server/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ utils = {
ONE_MONTH_S: 2628000,
SIX_MONTH_S: 15768000,
ONE_YEAR_S: 31536000,
FIVE_MINUTES_MS: 300000,
ONE_HOUR_MS: 3600000,
ONE_DAY_MS: 86400000,
ONE_WEEK_MS: 604800000,
Expand Down
Loading

0 comments on commit fa38257

Please sign in to comment.