From c5169e23c4b8e15ce13400b044e844d5f485731d Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Thu, 14 Dec 2017 13:52:20 +0100 Subject: [PATCH] Moved unique identifier generation to lib/security refs #9178 --- core/server/lib/security/identifier.js | 28 ++++++++++++++ core/server/lib/security/index.js | 4 ++ core/server/models/user.js | 6 +-- core/server/services/auth/auth-strategies.js | 3 +- core/server/services/auth/utils.js | 5 ++- core/server/services/themes/Storage.js | 3 +- core/server/utils/index.js | 38 +------------------ .../api/api_authentication_spec.js | 26 ++++++------- .../services/auth/auth-strategies_spec.js | 3 +- 9 files changed, 57 insertions(+), 59 deletions(-) create mode 100644 core/server/lib/security/identifier.js diff --git a/core/server/lib/security/identifier.js b/core/server/lib/security/identifier.js new file mode 100644 index 00000000000..921738e744e --- /dev/null +++ b/core/server/lib/security/identifier.js @@ -0,0 +1,28 @@ +'use strict'; + +let _private = {}; + +// @TODO: replace with crypto.randomBytes +_private.getRandomInt = function (min, max) { + return Math.floor(Math.random() * (max - min + 1)) + min; +}; + +/** + * Return a unique identifier with the given `len`. + * + * @param {Number} maxLength + * @return {String} + * @api private + */ +module.exports.uid = function uid(maxLength) { + var buf = [], + chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789', + charLength = chars.length, + i; + + for (i = 0; i < maxLength; i = i + 1) { + buf.push(chars[_private.getRandomInt(0, charLength - 1)]); + } + + return buf.join(''); +}; diff --git a/core/server/lib/security/index.js b/core/server/lib/security/index.js index 0e85c30a96f..25d5e9f492c 100644 --- a/core/server/lib/security/index.js +++ b/core/server/lib/security/index.js @@ -11,5 +11,9 @@ module.exports = { get string() { return require('./string'); + }, + + get identifier() { + return require('./identifier'); } }; diff --git a/core/server/models/user.js b/core/server/models/user.js index 99daff13661..8b944bd6f01 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -6,7 +6,7 @@ var _ = require('lodash'), ghostBookshelf = require('./base'), baseUtils = require('./base/utils'), common = require('../lib/common'), - utils = require('../utils'), + security = require('../lib/security'), gravatar = require('../utils/gravatar'), validation = require('../data/validation'), pipeline = require('../lib/promise/pipeline'), @@ -41,7 +41,7 @@ User = ghostBookshelf.Model.extend({ var baseDefaults = ghostBookshelf.Model.prototype.defaults.call(this); return _.merge({ - password: utils.uid(50) + password: security.identifier.uid(50) }, baseDefaults); }, @@ -157,7 +157,7 @@ User = ghostBookshelf.Model.extend({ if (options.importing) { // always set password to a random uid when importing - this.set('password', utils.uid(50)); + this.set('password', security.identifier.uid(50)); // lock users so they have to follow the password reset flow if (this.get('status') !== 'inactive') { diff --git a/core/server/services/auth/auth-strategies.js b/core/server/services/auth/auth-strategies.js index fdfd9ba1e7e..76fc0bedfb5 100644 --- a/core/server/services/auth/auth-strategies.js +++ b/core/server/services/auth/auth-strategies.js @@ -1,6 +1,5 @@ var _ = require('lodash'), models = require('../../models'), - globalUtils = require('../../utils'), common = require('../../lib/common'), security = require('../../lib/security'), strategies; @@ -116,7 +115,7 @@ strategies = { return models.User.add({ email: profile.email, name: profile.name, - password: globalUtils.uid(50), + password: security.identifier.uid(50), roles: [invite.toJSON().role_id], ghost_auth_id: profile.id, ghost_auth_access_token: ghostAuthAccessToken diff --git a/core/server/services/auth/utils.js b/core/server/services/auth/utils.js index b0cf203da6d..9c3ab0e4159 100644 --- a/core/server/services/auth/utils.js +++ b/core/server/services/auth/utils.js @@ -2,6 +2,7 @@ var Promise = require('bluebird'), _ = require('lodash'), debug = require('ghost-ignition').debug('auth:utils'), models = require('../../models'), + security = require('../../lib/security'), globalUtils = require('../../utils'), knex = require('../../data/db').knex, _private = {}; @@ -53,8 +54,8 @@ _private.handleTokenCreation = function handleTokenCreation(data, options) { var oldAccessToken = data.oldAccessToken, oldRefreshToken = data.oldRefreshToken, oldRefreshId = data.oldRefreshId, - newAccessToken = globalUtils.uid(191), - newRefreshToken = globalUtils.uid(191), + newAccessToken = security.identifier.uid(191), + newRefreshToken = security.identifier.uid(191), accessExpires = Date.now() + globalUtils.ONE_MONTH_MS, refreshExpires = Date.now() + globalUtils.SIX_MONTH_MS, clientId = data.clientId, diff --git a/core/server/services/themes/Storage.js b/core/server/services/themes/Storage.js index c143b844ccb..3a3d17c292d 100644 --- a/core/server/services/themes/Storage.js +++ b/core/server/services/themes/Storage.js @@ -5,6 +5,7 @@ var fs = require('fs-extra'), path = require('path'), Promise = require('bluebird'), config = require('../../config'), + security = require('../../lib/security'), globalUtils = require('../../utils'), LocalFileStorage = require('../../adapters/storage/LocalFileStorage'); @@ -30,7 +31,7 @@ class ThemeStorage extends LocalFileStorage { themePath = path.join(self.storagePath, themeName), zipName = themeName + '.zip', // store this in a unique temporary folder - zipBasePath = path.join(os.tmpdir(), globalUtils.uid(10)), + zipBasePath = path.join(os.tmpdir(), security.identifier.uid(10)), zipPath = path.join(zipBasePath, zipName), stream; diff --git a/core/server/utils/index.js b/core/server/utils/index.js index b8c66662349..35f1b59a68f 100644 --- a/core/server/utils/index.js +++ b/core/server/utils/index.js @@ -1,17 +1,4 @@ -var utils, - getRandomInt; - -/** - * Return a random int, used by `utils.uid()` - * - * @param {Number} min - * @param {Number} max - * @return {Number} - * @api private - */ -getRandomInt = function (min, max) { - return Math.floor(Math.random() * (max - min + 1)) + min; -}; +var utils; utils = { /** @@ -32,29 +19,6 @@ utils = { ONE_YEAR_MS: 31536000000, // eslint-enable key-spacing */ - /** - * Return a unique identifier with the given `len`. - * - * utils.uid(10); - * // => "FDaS435D2z" - * - * @param {Number} len - * @return {String} - * @api private - */ - uid: function (len) { - var buf = [], - chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789', - charlen = chars.length, - i; - - for (i = 0; i < len; i = i + 1) { - buf.push(chars[getRandomInt(0, charlen - 1)]); - } - - return buf.join(''); - }, - readCSV: require('./read-csv'), zipFolder: require('./zip-folder'), ghostVersion: require('./ghost-version') diff --git a/core/test/integration/api/api_authentication_spec.js b/core/test/integration/api/api_authentication_spec.js index 870ec67c1ac..dfadb26c9a0 100644 --- a/core/test/integration/api/api_authentication_spec.js +++ b/core/test/integration/api/api_authentication_spec.js @@ -3,14 +3,14 @@ var should = require('should'), testUtils = require('../../utils'), _ = require('lodash'), Promise = require('bluebird'), - uid = require('../../../server/utils').uid, AuthAPI = require('../../../server/api/authentication'), mail = require('../../../server/api/mail'), models = require('../../../server/models'), common = require('../../../server/lib/common'), + security = require('../../../server/lib/security'), context = testUtils.context, - Accesstoken, - Refreshtoken, + accessToken, + refreshToken, User, sandbox = sinon.sandbox.create(); @@ -203,8 +203,8 @@ describe('Authentication API', function () { describe('Completed', function () { before(function () { - Accesstoken = require('../../../server/models/accesstoken').Accesstoken; - Refreshtoken = require('../../../server/models/refreshtoken').Refreshtoken; + accessToken = require('../../../server/models/accesstoken').Accesstoken; + refreshToken = require('../../../server/models/refreshtoken').Refreshtoken; User = require('../../../server/models/user').User; }); @@ -369,9 +369,9 @@ describe('Authentication API', function () { }); it('should allow an access token to be revoked', function (done) { - var id = uid(191); + var id = security.identifier.uid(191); - Accesstoken.add({ + accessToken.add({ token: id, expires: Date.now() + 8640000, user_id: testUtils.DataGenerator.Content.users[0].id, @@ -388,7 +388,7 @@ describe('Authentication API', function () { should.exist(response); response.token.should.equal(id); - return Accesstoken.findOne({token: id}); + return accessToken.findOne({token: id}); }).then(function (token) { should.not.exist(token); @@ -436,9 +436,9 @@ describe('Authentication API', function () { }); it('should allow a refresh token to be revoked', function (done) { - var id = uid(191); + var id = security.identifier.uid(191); - Refreshtoken.add({ + refreshToken.add({ token: id, expires: Date.now() + 8640000, user_id: testUtils.DataGenerator.Content.users[0].id, @@ -455,7 +455,7 @@ describe('Authentication API', function () { should.exist(response); response.token.should.equal(id); - return Refreshtoken.findOne({token: id}); + return refreshToken.findOne({token: id}); }).then(function (token) { should.not.exist(token); @@ -464,9 +464,9 @@ describe('Authentication API', function () { }); it('should return success when attempting to revoke an invalid token', function (done) { - var id = uid(191); + var id = security.identifier.uid(191); - Accesstoken.add({ + accessToken.add({ token: id, expires: Date.now() + 8640000, user_id: testUtils.DataGenerator.Content.users[0].id, diff --git a/core/test/unit/services/auth/auth-strategies_spec.js b/core/test/unit/services/auth/auth-strategies_spec.js index 94b61b070b1..1d9e0fb8bee 100644 --- a/core/test/unit/services/auth/auth-strategies_spec.js +++ b/core/test/unit/services/auth/auth-strategies_spec.js @@ -6,6 +6,7 @@ var should = require('should'), authStrategies = require('../../../../server/services/auth/auth-strategies'), Models = require('../../../../server/models'), common = require('../../../../server/lib/common'), + security = require('../../../../server/lib/security'), urlService = require('../../../../server/services/url'), globalUtils = require('../../../../server/utils'), @@ -286,7 +287,7 @@ describe('Auth Strategies', function () { role_id: '2' }); - sandbox.stub(globalUtils, 'uid').returns('12345678'); + sandbox.stub(security.identifier, 'uid').returns('12345678'); userFindOneStub.returns(Promise.resolve(null));