🐛 Fixed concurrent renew of access tokens

no issue

- it can happen that concurrent requests try to renew access tokens with the same refresh token
- in this case it could happen that you received a token deletion error
- add propert locking
- ensure we don't run into deadlocks
- manual testing with async.times for parallel requests (was able to reproduce the error)

core/server/auth/oauth.js

@@ -1,40 +1,66 @@
 var oauth2orize = require('oauth2orize'),
+    _ = require('lodash'),
     passport = require('passport'),
     models = require('../models'),
     errors = require('../errors'),
     authUtils = require('./utils'),
     spamPrevention = require('../middleware/api/spam-prevention'),
     i18n = require('../i18n'),
+    knex = require('../data/db').knex,
     oauthServer,
     oauth;
 
 function exchangeRefreshToken(client, refreshToken, scope, body, authInfo, done) {
-    models.Refreshtoken.findOne({token: refreshToken})
-        .then(function then(model) {
-            if (!model) {
-                return done(new errors.NoPermissionError({message: i18n.t('errors.middleware.oauth.invalidRefreshToken')}), false);
-            } else {
+    knex.transaction(function (transacting) {
+        var options = {
+            transacting: transacting
+        };
+
+        return models.Refreshtoken.findOne({token: refreshToken}, _.merge({forUpdate: true}, options))
+            .then(function then(model) {
+                if (!model) {
+                    throw new errors.NoPermissionError({
+                        message: i18n.t('errors.middleware.oauth.invalidRefreshToken')
+                    });
+                }
+
                 var token = model.toJSON();
 
-                if (token.expires > Date.now()) {
-                    spamPrevention.userLogin().reset(authInfo.ip, body.refresh_token + 'login');
-
-                    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);
+                if (token.expires <= Date.now()) {
+                    throw new errors.UnauthorizedError({
+                        message: i18n.t('errors.middleware.oauth.refreshTokenExpired')
                     });
-                } else {
-                    done(new errors.UnauthorizedError({message: i18n.t('errors.middleware.oauth.refreshTokenExpired')}), false);
                 }
-            }
-        });
+
+                // @TODO: this runs outside of the transaction
+                spamPrevention.userLogin().reset(authInfo.ip, body.refresh_token + 'login');
+
+                return authUtils.createTokens({
+                    clientId: token.client_id,
+                    userId: token.user_id,
+                    oldAccessToken: authInfo.accessToken,
+                    oldRefreshToken: refreshToken,
+                    oldRefreshId: token.id
+                }, options).then(function (response) {
+                    return {
+                        access_token: response.access_token,
+                        expires_in: response.expires_in
+                    };
+                });
+            });
+    }).then(function (response) {
+        done(null, response.access_token, {expires_in: response.expires_in});
+    }).catch(function (err) {
+        if (errors.utils.isIgnitionError(err)) {
+            return done(err, false);
+        }
+
+        done(new errors.InternalServerError({
+            err: err
+        }), false);
+    });
 }
+
 // We are required to pass in authInfo in order to reset spam counter for user login
 function exchangePassword(client, username, password, scope, body, authInfo, done) {
     if (!client || !client.id) {

core/server/auth/utils.js

@@ -31,14 +31,62 @@ _private.decreaseOldAccessTokenExpiry = function decreaseOldAccessTokenExpiry(da
         });
 };
 
-_private.destroyOldRefreshToken = function destroyOldRefreshToken(options) {
-    debug('destroyOldRefreshToken', options.token);
+_private.handleOldRefreshToken = function handleOldRefreshToken(data, options) {
+    debug('handleOldRefreshToken', data.oldRefreshToken);
 
-    if (!options.token) {
-        return Promise.resolve();
+    if (!data.oldRefreshToken) {
+        return models.Refreshtoken.add({
+            token: data.newRefreshToken,
+            user_id: data.userId,
+            client_id: data.clientId,
+            expires: data.refreshExpires
+        }, options);
     }
 
-    return models.Refreshtoken.destroyByToken(options);
+    // extend refresh token expiry
+    return models.Refreshtoken.edit({
+        expires: data.refreshExpires
+    }, _.merge({id: data.oldRefreshId}, options));
+};
+
+_private.handleTokenCreation = function handleTokenCreation(data, options) {
+    var oldAccessToken = data.oldAccessToken,
+        oldRefreshToken = data.oldRefreshToken,
+        oldRefreshId = data.oldRefreshId,
+        newAccessToken = globalUtils.uid(191),
+        newRefreshToken = globalUtils.uid(191),
+        accessExpires = Date.now() + globalUtils.ONE_MONTH_MS,
+        refreshExpires = Date.now() + globalUtils.SIX_MONTH_MS,
+        clientId = data.clientId,
+        userId = data.userId;
+
+    return _private.decreaseOldAccessTokenExpiry({token: oldAccessToken}, options)
+        .then(function () {
+            return _private.handleOldRefreshToken({
+                userId: userId,
+                clientId: clientId,
+                oldRefreshToken: oldRefreshToken,
+                oldRefreshId: oldRefreshId,
+                newRefreshToken: newRefreshToken,
+                refreshExpires: refreshExpires
+            }, options);
+        })
+        .then(function (refreshToken) {
+            return models.Accesstoken.add({
+                token: newAccessToken,
+                user_id: userId,
+                client_id: clientId,
+                issued_by: refreshToken.id,
+                expires: accessExpires
+            }, options);
+        })
+        .then(function () {
+            return {
+                access_token: newAccessToken,
+                refresh_token: newRefreshToken,
+                expires_in: globalUtils.ONE_MONTH_S
+            };
+        });
 };
 
 /**
@@ -47,53 +95,20 @@ _private.destroyOldRefreshToken = function destroyOldRefreshToken(options) {
  * 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 || {};
+module.exports.createTokens = function createTokens(data, modelOptions) {
+    data = data || {};
+    modelOptions = modelOptions || {};
+
     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;
+    if (modelOptions.transacting) {
+        return _private.handleTokenCreation(data, modelOptions);
+    }
 
     return knex.transaction(function (transaction) {
-        modelOptions = {transacting: transaction};
+        modelOptions.transacting = transaction;
 
-        return _private.decreaseOldAccessTokenExpiry({token: oldAccessToken}, modelOptions)
-            .then(function () {
-                return _private.destroyOldRefreshToken(_.merge({
-                    token: oldRefreshToken
-                }, modelOptions));
-            })
-            .then(function () {
-                return models.Refreshtoken.add({
-                    token: newRefreshToken,
-                    user_id: userId,
-                    client_id: clientId,
-                    expires: refreshExpires
-                }, modelOptions);
-            })
-            .then(function (refreshToken) {
-                return models.Accesstoken.add({
-                    token: newAccessToken,
-                    user_id: userId,
-                    client_id: clientId,
-                    issued_by: refreshToken.id,
-                    expires: accessExpires
-                }, modelOptions);
-            })
-            .then(function () {
-                return {
-                    access_token: newAccessToken,
-                    refresh_token: newRefreshToken,
-                    expires_in: globalUtils.ONE_MONTH_S
-                };
-            });
+        return _private.handleTokenCreation(data, modelOptions);
     });
 };
 

core/test/functional/routes/api/authentication_spec.js

@@ -184,6 +184,11 @@ describe('Authentication API', function () {
                                 token: accesstoken
                             }).then(function (oldAccessToken) {
                                 moment(oldAccessToken.get('expires')).diff(moment(), 'minutes').should.be.below(6);
+                                return models.Refreshtoken.findOne({
+                                    token: refreshToken
+                                });
+                            }).then(function (refreshTokenModel) {
+                                moment(refreshTokenModel.get('expires')).diff(moment(), 'month').should.be.above(5);
                                 done();
                             });
                         });

core/test/unit/auth/oauth_spec.js

@@ -269,13 +269,12 @@ describe('OAuth', function () {
                 }
             }));
 
-            sandbox.stub(authUtils, 'createTokens')
-                .returns(new Promise.reject({
-                    message: 'DB error'
-                }));
+            sandbox.stub(authUtils, 'createTokens', function () {
+                return Promise.reject(new Error('DB error'));
+            });
 
             oAuth.generateAccessToken(req, res, function (err) {
-                err.message.should.eql('DB error');
+                err.stack.should.containEql('DB error');
                 done();
             });
         });