From c301510cd13371b0dee876551a0ed09bef2143ad Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Fri, 12 Feb 2016 20:04:26 +0000 Subject: [PATCH] Refactor gravatarLookup, remove request dependency no issue - request is quite a heavy dependency - we were only using request in 3 places: a test, storing contrib images in the gruntfile & the gravatar lookup - all 3 are relatively simple to do with the http/https module - refactored all 3, removed request --- Gruntfile.js | 16 ++--- core/server/models/user.js | 36 ++--------- core/server/utils/gravatar.js | 42 +++++++++++++ core/test/integration/api/api_users_spec.js | 10 +--- .../integration/model/model_users_spec.js | 41 +------------ core/test/unit/server_spec.js | 9 +-- core/test/unit/server_utils_spec.js | 59 ++++++++++++++++++- package.json | 1 - 8 files changed, 126 insertions(+), 88 deletions(-) create mode 100644 core/server/utils/gravatar.js diff --git a/Gruntfile.js b/Gruntfile.js index 5df0668336..203d6f5a9a 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -8,11 +8,11 @@ var _ = require('lodash'), chalk = require('chalk'), fs = require('fs-extra'), + https = require('https'), moment = require('moment'), getTopContribs = require('top-gh-contribs'), path = require('path'), Promise = require('bluebird'), - request = require('request'), escapeChar = process.platform.match(/^win/) ? '^' : '\\', cwd = process.cwd().replace(/( |\(|\))/g, escapeChar + '$1'), @@ -834,15 +834,17 @@ var _ = require('lodash'), ).then(function (results) { var contributors = results[1], contributorTemplate = '
\n \n' + - ' " alt="<%name%>" />\n' + - ' \n
', + ' " alt="<%name%>" />\n' + + ' \n', downloadImagePromise = function (url, name) { return new Promise(function (resolve, reject) { - request(url) - .pipe(fs.createWriteStream(imagePath + name)) - .on('close', resolve) - .on('error', reject); + https.get(url, function (res) { + fs.writeFile(imagePath + name, res, function () { + resolve(); + }); + }) + .on('error', reject); }); }; diff --git a/core/server/models/user.js b/core/server/models/user.js index 6d88fb88db..6b4c8bf6a6 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -2,13 +2,12 @@ var _ = require('lodash'), Promise = require('bluebird'), errors = require('../errors'), utils = require('../utils'), + gravatar = require('../utils/gravatar'), bcrypt = require('bcryptjs'), ghostBookshelf = require('./base'), crypto = require('crypto'), validator = require('validator'), - request = require('request'), validation = require('../data/validation'), - config = require('../config'), events = require('../events'), i18n = require('../i18n'), @@ -380,7 +379,7 @@ User = ghostBookshelf.Model.extend({ // Assign the hashed password userData.password = hash; // LookupGravatar - return self.gravatarLookup(userData); + return gravatar.lookup(userData); }).then(function then(userData) { // Save the user with the hashed password return ghostBookshelf.Model.add.call(self, userData, options); @@ -423,8 +422,10 @@ User = ghostBookshelf.Model.extend({ // Assign the hashed password userData.password = hash; - return Promise.join(self.gravatarLookup(userData), - ghostBookshelf.Model.generateSlug.call(this, User, userData.name, options)); + return Promise.join( + gravatar.lookup(userData), + ghostBookshelf.Model.generateSlug.call(this, User, userData.name, options) + ); }).then(function then(results) { userData = results[0]; userData.slug = results[1]; @@ -776,31 +777,6 @@ User = ghostBookshelf.Model.extend({ }); }, - gravatarLookup: function gravatarLookup(userData) { - var gravatarUrl = '//www.gravatar.com/avatar/' + - crypto.createHash('md5').update(userData.email.toLowerCase().trim()).digest('hex') + - '?s=250'; - - return new Promise(function gravatarRequest(resolve) { - if (config.isPrivacyDisabled('useGravatar')) { - return resolve(userData); - } - - request({url: 'http:' + gravatarUrl + '&d=404&r=x', timeout: 2000}, function handler(err, response) { - if (err) { - // just resolve with no image url - return resolve(userData); - } - - if (response.statusCode !== 404) { - gravatarUrl += '&d=mm&r=x'; - userData.image = gravatarUrl; - } - - resolve(userData); - }); - }); - }, // Get the user by email address, enforces case insensitivity rejects if the user is not found // When multi-user support is added, email addresses must be deduplicated with case insensitivity, so that // joe@bloggs.com and JOE@BLOGGS.COM cannot be created as two separate users. diff --git a/core/server/utils/gravatar.js b/core/server/utils/gravatar.js new file mode 100644 index 0000000000..9d65163306 --- /dev/null +++ b/core/server/utils/gravatar.js @@ -0,0 +1,42 @@ +var Promise = require('bluebird'), + config = require('../config'), + crypto = require('crypto'), + https = require('https'); + +module.exports.lookup = function lookup(userData, timeout) { + var gravatarUrl = '//www.gravatar.com/avatar/' + + crypto.createHash('md5').update(userData.email.toLowerCase().trim()).digest('hex') + + '?s=250'; + + return new Promise(function gravatarRequest(resolve) { + if (config.isPrivacyDisabled('useGravatar') || process.env.NODE_ENV.indexOf('testing') > -1) { + return resolve(userData); + } + + var request, timer, timerEnded = false; + + request = https.get('https:' + gravatarUrl + '&d=404&r=x', function (response) { + clearTimeout(timer); + if (response.statusCode !== 404 && !timerEnded) { + gravatarUrl += '&d=mm&r=x'; + userData.image = gravatarUrl; + } + + resolve(userData); + }); + + request.on('error', function () { + clearTimeout(timer); + // just resolve with no image url + if (!timerEnded) { + return resolve(userData); + } + }); + + timer = setTimeout(function () { + timerEnded = true; + request.abort(); + return resolve(userData); + }, timeout || 2000); + }); +}; diff --git a/core/test/integration/api/api_users_spec.js b/core/test/integration/api/api_users_spec.js index 79cae6abb0..f6e434676b 100644 --- a/core/test/integration/api/api_users_spec.js +++ b/core/test/integration/api/api_users_spec.js @@ -7,7 +7,7 @@ var testUtils = require('../../utils'), _ = require('lodash'), // Stuff we are testing - ModelUser = require('../../../server/models'), + models = require('../../../server/models'), UserAPI = require('../../../server/api/users'), mail = require('../../../server/api/mail'), @@ -39,7 +39,7 @@ describe('Users API', function () { it('dateTime fields are returned as Date objects', function (done) { var userData = testUtils.DataGenerator.forModel.users[0]; - ModelUser.User.check({email: userData.email, password: userData.password}).then(function (user) { + models.User.check({email: userData.email, password: userData.password}).then(function (user) { return UserAPI.read({id: user.id}); }).then(function (response) { response.users[0].created_at.should.be.an.instanceof(Date); @@ -482,7 +482,7 @@ describe('Users API', function () { UserAPI.edit( {users: [{name: 'newname', password: 'newpassword'}]}, _.extend({}, context.author, {id: userIdFor.author}) ).then(function () { - return ModelUser.User.findOne({id: userIdFor.author}).then(function (response) { + return models.User.findOne({id: userIdFor.author}).then(function (response) { response.get('name').should.eql('newname'); response.get('password').should.not.eql('newpassword'); done(); @@ -497,10 +497,6 @@ describe('Users API', function () { beforeEach(function () { newUser = _.clone(testUtils.DataGenerator.forKnex.createUser(testUtils.DataGenerator.Content.users[4])); - sandbox.stub(ModelUser.User, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - sandbox.stub(mail, 'send', function () { return Promise.resolve(); }); diff --git a/core/test/integration/model/model_users_spec.js b/core/test/integration/model/model_users_spec.js index ef25d75e48..e24ed5c7aa 100644 --- a/core/test/integration/model/model_users_spec.js +++ b/core/test/integration/model/model_users_spec.js @@ -9,6 +9,7 @@ var testUtils = require('../../utils'), // Stuff we are testing utils = require('../../../server/utils'), + gravatar = require('../../../server/utils/gravatar'), UserModel = require('../../../server/models/user').User, RoleModel = require('../../../server/models/role').Role, events = require('../../../server/events'), @@ -38,10 +39,6 @@ describe('User Model', function run() { it('can add first', function (done) { var userData = testUtils.DataGenerator.forModel.users[0]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(userData, context).then(function (createdUser) { should.exist(createdUser); createdUser.has('uuid').should.equal(true); @@ -55,10 +52,6 @@ describe('User Model', function run() { it('shortens slug if possible', function (done) { var userData = testUtils.DataGenerator.forModel.users[2]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(userData, context).then(function (createdUser) { should.exist(createdUser); createdUser.has('slug').should.equal(true); @@ -70,10 +63,6 @@ describe('User Model', function run() { it('does not short slug if not possible', function (done) { var userData = testUtils.DataGenerator.forModel.users[2]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(userData, context).then(function (createdUser) { should.exist(createdUser); createdUser.has('slug').should.equal(true); @@ -99,10 +88,6 @@ describe('User Model', function run() { it('does NOT lowercase email', function (done) { var userData = testUtils.DataGenerator.forModel.users[2]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(userData, context).then(function (createdUser) { should.exist(createdUser); createdUser.has('uuid').should.equal(true); @@ -114,7 +99,7 @@ describe('User Model', function run() { it('can find gravatar', function (done) { var userData = testUtils.DataGenerator.forModel.users[4]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { + sandbox.stub(gravatar, 'lookup', function (userData) { userData.image = 'http://www.gravatar.com/avatar/2fab21a4c4ed88e76add10650c73bae1?d=404'; return Promise.resolve(userData); }); @@ -132,7 +117,7 @@ describe('User Model', function run() { it('can handle no gravatar', function (done) { var userData = testUtils.DataGenerator.forModel.users[0]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { + sandbox.stub(gravatar, 'lookup', function (userData) { return Promise.resolve(userData); }); @@ -336,10 +321,6 @@ describe('User Model', function run() { it('can invite user', function (done) { var userData = testUtils.DataGenerator.forModel.users[4]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) { should.exist(createdUser); createdUser.has('uuid').should.equal(true); @@ -356,10 +337,6 @@ describe('User Model', function run() { it('can add active user', function (done) { var userData = testUtils.DataGenerator.forModel.users[4]; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - RoleModel.findOne().then(function (role) { userData.roles = [role.toJSON()]; @@ -406,10 +383,6 @@ describe('User Model', function run() { var userData = testUtils.DataGenerator.forModel.users[4], userId; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) { should.exist(createdUser); createdUser.has('uuid').should.equal(true); @@ -436,10 +409,6 @@ describe('User Model', function run() { var userData = testUtils.DataGenerator.forModel.users[4], userId; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) { should.exist(createdUser); createdUser.has('uuid').should.equal(true); @@ -495,10 +464,6 @@ describe('User Model', function run() { var userData = testUtils.DataGenerator.forModel.users[4], userId; - sandbox.stub(UserModel, 'gravatarLookup', function (userData) { - return Promise.resolve(userData); - }); - UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) { should.exist(createdUser); createdUser.has('uuid').should.equal(true); diff --git a/core/test/unit/server_spec.js b/core/test/unit/server_spec.js index 035589f4bc..bdeef92828 100644 --- a/core/test/unit/server_spec.js +++ b/core/test/unit/server_spec.js @@ -1,7 +1,7 @@ /*globals describe, it*/ /*jshint expr:true*/ var should = require('should'), - request = require('request'), + http = require('http'), config = require('../../../config'); describe('Server', function () { @@ -10,11 +10,12 @@ describe('Server', function () { url = 'http://' + host + ':' + port; it('should not start a connect server when required', function (done) { - request(url, function (error, response, body) { - should(response).equal(undefined); - should(body).equal(undefined); + http.get(url, function () { + done('This request should not have worked'); + }).on('error', function (error) { should(error).not.equal(undefined); should(error.code).equal('ECONNREFUSED'); + done(); }); }); diff --git a/core/test/unit/server_utils_spec.js b/core/test/unit/server_utils_spec.js index b592302d90..febbd52d39 100644 --- a/core/test/unit/server_utils_spec.js +++ b/core/test/unit/server_utils_spec.js @@ -1,11 +1,13 @@ -/*globals describe, it*/ +/*globals describe, it, beforeEach, afterEach*/ /*jshint expr:true*/ var should = require('should'), sinon = require('sinon'), + nock = require('nock'), parsePackageJson = require('../../server/utils/parse-package-json'), validateThemes = require('../../server/utils/validate-themes'), readDirectory = require('../../server/utils/read-directory'), readThemes = require('../../server/utils/read-themes'), + gravatar = require('../../server/utils/gravatar'), tempfile = require('../utils/tempfile'), utils = require('../../server/utils'), join = require('path').join, @@ -457,6 +459,61 @@ describe('Server Utilities', function () { }); }); + describe('gravatar-lookup', function () { + var currentEnv = process.env.NODE_ENV; + + beforeEach(function () { + // give environment a value that will call gravatar + process.env.NODE_ENV = 'production'; + }); + + afterEach(function () { + // reset the environment + process.env.NODE_ENV = currentEnv; + }); + + it('can successfully lookup a gravatar url', function (done) { + nock('https://www.gravatar.com') + .get('/avatar/ef6dcde5c99bb8f685dd451ccc3e050a?s=250&d=404&r=x') + .reply(200); + + gravatar.lookup({email: 'exists@example.com'}).then(function (result) { + should.exist(result); + should.exist(result.image); + result.image.should.eql('//www.gravatar.com/avatar/ef6dcde5c99bb8f685dd451ccc3e050a?s=250&d=mm&r=x'); + + done(); + }).catch(done); + }); + + it('can handle a non existant gravatar', function (done) { + nock('https://www.gravatar.com') + .get('/avatar/3a2963a39ebba98fb0724a1db2f13d63?s=250&d=404&r=x') + .reply(404); + + gravatar.lookup({email: 'invalid@example.com'}).then(function (result) { + should.exist(result); + should.not.exist(result.image); + + done(); + }).catch(done); + }); + + it('will timeout', function (done) { + nock('https://www.gravatar.com') + .get('/avatar/ef6dcde5c99bb8f685dd451ccc3e050a?s=250&d=404&r=x') + .delay(11) + .reply(200); + + gravatar.lookup({email: 'exists@example.com'}, 10).then(function (result) { + should.exist(result); + should.not.exist(result.image); + + done(); + }).catch(done); + }); + }); + describe('redirect301', function () { it('performs a 301 correctly', function (done) { var res = {}; diff --git a/package.json b/package.json index 8eac70fa5f..8680b6aeeb 100644 --- a/package.json +++ b/package.json @@ -59,7 +59,6 @@ "passport-http-bearer": "1.0.1", "passport-oauth2-client-password": "0.1.2", "path-match": "1.2.3", - "request": "2.69.0", "rss": "1.2.1", "semver": "5.1.0", "showdown-ghost": "0.3.6",