From 43ef4e51b527551ab176f5fef5a9caa71d42da43 Mon Sep 17 00:00:00 2001 From: Blade Barringer Date: Sat, 21 Nov 2015 14:56:20 -0600 Subject: [PATCH 1/5] fix(model): Remove pre validation from user model --- website/src/models/user.js | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/website/src/models/user.js b/website/src/models/user.js index 2d001a3ca6..0deca34bac 100644 --- a/website/src/models/user.js +++ b/website/src/models/user.js @@ -609,37 +609,6 @@ function _setProfileName (user) { return localUsername || facebookUsername || anonymous; } -schema.pre('validate', function beforeValidateUser (next) { - if (!this.auth.facebook.id || this.auth.local.email || this.auth.local.username) { - if (!this.auth.local.email) { - this.invalidate('auth.local.email', shared.i18n.t('missingEmail')); - return next(); - } - - if (!this.auth.local.username) { - this.invalidate('auth.local.username', shared.i18n.t('missingUsername')); - return next(); - } - } - - // Validate password and password confirmation and create hashed version - if (this.isModified('auth.local.password') || this.isNew() && !this.auth.facebook.id) { // TODO this does not catch when you already have social auth and password isn't passedß - if (!this.auth.local.password) { - this.invalidate('auth.local.password', shared.i18n.t('missingPassword')); - return next(); - } - - if (this.auth.local.password !== this.auth.local.passwordConfirmation) { - this.invalidate('auth.local.passwordConfirmation', shared.i18n.t('passwordConfirmationMatch')); - return next(); - } - - this.hashed_password = passwordUtils.encrypt(this.auth.local.password, this.auth.local.salt); // eslint-disable-line camelcase - } - - next(); -}); - schema.pre('save', function postSaveUser (next) { // Do not store password and passwordConfirmation this.auth.local.password = this.local.auth.passwordConfirmation = undefined; From 75cea0c810e3b26918c7e32d00c12712b75a9839 Mon Sep 17 00:00:00 2001 From: Blade Barringer Date: Sat, 21 Nov 2015 14:56:44 -0600 Subject: [PATCH 2/5] refactor(model): Remove password and passwordConfirmation from user model --- website/src/models/user.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/website/src/models/user.js b/website/src/models/user.js index 0deca34bac..a70d76e7d0 100644 --- a/website/src/models/user.js +++ b/website/src/models/user.js @@ -42,15 +42,6 @@ export let schema = new Schema({ lowerCaseUsername: String, hashed_password: String, // eslint-disable-line camelcase salt: String, - // password and passwordConfirmation are not stored in the database, used only for validation - password: { - type: String, - trim: true, - }, - passwordConfirmation: { - type: String, - trim: true, - }, }, timestamps: { created: {type: Date, default: Date.now}, @@ -610,9 +601,6 @@ function _setProfileName (user) { } schema.pre('save', function postSaveUser (next) { - // Do not store password and passwordConfirmation - this.auth.local.password = this.local.auth.passwordConfirmation = undefined; - // Populate new users with default content if (this.isNew) { _populateDefaultsForNewUser(this); From 074b3f5079be55102933dfb86f0f1fea28eeb846 Mon Sep 17 00:00:00 2001 From: Blade Barringer Date: Sat, 21 Nov 2015 14:57:59 -0600 Subject: [PATCH 3/5] fix(controller): Validate params for register route in user controller --- website/src/controllers/api-v3/user.js | 84 ++++++++++++++------------ 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/website/src/controllers/api-v3/user.js b/website/src/controllers/api-v3/user.js index 6354f0d142..acff305ae9 100644 --- a/website/src/controllers/api-v3/user.js +++ b/website/src/controllers/api-v3/user.js @@ -20,7 +20,7 @@ let api = {}; * @apiParam {String} username Username of the new user * @apiParam {String} email Email address of the new user * @apiParam {String} password Password for the new user account - * @apiParam {String} passwordConfirmation Password confirmation + * @apiParam {String} confirmPassword Password confirmation * * @apiSuccess {Object} user The user object */ @@ -28,32 +28,19 @@ api.registerLocal = { method: 'POST', url: '/user/auth/local/register', handler (req, res, next) { - let email = req.body.email && req.body.email.toLowerCase(); - let username = req.body.username; + let { email, username, password, confirmPassword } = req.body; + + // Validate required params + if (!username) return next(new NotAuthorized(res.t('missingUsername'))); + if (!email) return next(new NotAuthorized(res.t('missingEmail'))); + if (!validator.isEmail(email)) return next(new NotAuthorized(res.t('invalidEmail'))); + if (!password) return next(new NotAuthorized(res.t('missingPassword'))); + if (password !== confirmPassword) return next(new NotAuthorized(res.t('passwordConfirmationMatch'))); + // Get the lowercase version of username to check that we do not have duplicates // So we can search for it in the database and then reject the choosen username if 1 or more results are found - let lowerCaseUsername = username && username.toLowerCase(); - - let newUser = new User({ - auth: { - local: { - username, - lowerCaseUsername, // Store the lowercase version of the username - email, // Store email as lowercase - salt: passwordUtils.makeSalt(), - password: req.body.password, - passwordConfirmation: req.body.passwordConfirmation, - }, - }, - preferences: { - language: req.language, - }, - }); - - newUser.registeredThrough = req.headers['x-client']; // TODO is this saved somewhere? - let validationErrors = newUser.validateSync(); // Validate synchronously for speed, remove if we add any async validator - - if (validationErrors) return next(validationErrors); + let lowerCaseUsername = username.toLowerCase(); + email = email.toLowerCase(); // Search for duplicates using lowercase version of username User.findOne({$or: [ @@ -65,25 +52,48 @@ api.registerLocal = { if (user) { if (email === user.auth.local.email) return next(new NotAuthorized(res.t('emailTaken'))); // Check that the lowercase username isn't already used - if (lowerCaseUsername === user.auth.local.lowerCaseUsername) return next(new NotAuthorized(res.t('usernameTaken'))); + if (lowerCaseUsername === user.auth.local.lowerCaseUsername) { + return next(new NotAuthorized(res.t('usernameTaken'))); + } } + let salt = passwordUtils.makeSalt(); + let hashed_password = passwordUtils.encrypt(password, salt); // eslint-disable-line camelcase + let newUser = new User({ + auth: { + local: { + username, + lowerCaseUsername, + email, + salt, + hashed_password, // eslint-disable-line camelcase + }, + }, + preferences: { + language: req.language, + }, + }); + + newUser.registeredThrough = req.headers['x-client']; // TODO is this saved somewhere? + return newUser.save(); }) .then((savedUser) => { - res.status(201).json(savedUser); + if (savedUser) { + res.status(201).json(savedUser); - // Clean previous email preferences - EmailUnsubscription - .remove({email: savedUser.auth.local.email}) - .then(() => sendTxnEmail(savedUser, 'welcome')); + // Clean previous email preferences + EmailUnsubscription + .remove({email: savedUser.auth.local.email}) + .then(() => sendTxnEmail(savedUser, 'welcome')); - res.analytics.track('register', { - category: 'acquisition', - type: 'local', - gaLabel: 'local', - uuid: savedUser._id, - }); + res.analytics.track('register', { + category: 'acquisition', + type: 'local', + gaLabel: 'local', + uuid: savedUser._id, + }); + } }) .catch(next); }, From d1839b816e2db2c39b66c23202728c3c5117a2cf Mon Sep 17 00:00:00 2001 From: Blade Barringer Date: Sat, 21 Nov 2015 16:17:42 -0600 Subject: [PATCH 4/5] chore(lint): Clean up user model and controller --- website/src/controllers/api-v3/user.js | 2 +- website/src/models/user.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/website/src/controllers/api-v3/user.js b/website/src/controllers/api-v3/user.js index acff305ae9..3394e8b15a 100644 --- a/website/src/controllers/api-v3/user.js +++ b/website/src/controllers/api-v3/user.js @@ -39,8 +39,8 @@ api.registerLocal = { // Get the lowercase version of username to check that we do not have duplicates // So we can search for it in the database and then reject the choosen username if 1 or more results are found - let lowerCaseUsername = username.toLowerCase(); email = email.toLowerCase(); + let lowerCaseUsername = username.toLowerCase(); // Search for duplicates using lowercase version of username User.findOne({$or: [ diff --git a/website/src/models/user.js b/website/src/models/user.js index a70d76e7d0..0462dd5ab1 100644 --- a/website/src/models/user.js +++ b/website/src/models/user.js @@ -1,7 +1,6 @@ // User schema and model import mongoose from 'mongoose'; import shared from '../../../common'; -import passwordUtils from '../libs/api-v3/password'; import _ from 'lodash'; import validator from 'validator'; import moment from 'moment'; From 867efd707812806f90bc4c578655d55e7a835b45 Mon Sep 17 00:00:00 2001 From: Blade Barringer Date: Sat, 21 Nov 2015 17:01:56 -0600 Subject: [PATCH 5/5] fix(controller): Adjust next calls to throw errors instead inside promise --- website/src/controllers/api-v3/user.js | 30 +++++++++++--------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/website/src/controllers/api-v3/user.js b/website/src/controllers/api-v3/user.js index 3394e8b15a..b3414ca921 100644 --- a/website/src/controllers/api-v3/user.js +++ b/website/src/controllers/api-v3/user.js @@ -50,11 +50,9 @@ api.registerLocal = { .exec() .then((user) => { if (user) { - if (email === user.auth.local.email) return next(new NotAuthorized(res.t('emailTaken'))); + if (email === user.auth.local.email) throw new NotAuthorized(res.t('emailTaken')); // Check that the lowercase username isn't already used - if (lowerCaseUsername === user.auth.local.lowerCaseUsername) { - return next(new NotAuthorized(res.t('usernameTaken'))); - } + if (lowerCaseUsername === user.auth.local.lowerCaseUsername) throw new NotAuthorized(res.t('usernameTaken')); } let salt = passwordUtils.makeSalt(); @@ -79,21 +77,19 @@ api.registerLocal = { return newUser.save(); }) .then((savedUser) => { - if (savedUser) { - res.status(201).json(savedUser); + res.status(201).json(savedUser); - // Clean previous email preferences - EmailUnsubscription - .remove({email: savedUser.auth.local.email}) - .then(() => sendTxnEmail(savedUser, 'welcome')); + // Clean previous email preferences + EmailUnsubscription + .remove({email: savedUser.auth.local.email}) + .then(() => sendTxnEmail(savedUser, 'welcome')); - res.analytics.track('register', { - category: 'acquisition', - type: 'local', - gaLabel: 'local', - uuid: savedUser._id, - }); - } + res.analytics.track('register', { + category: 'acquisition', + type: 'local', + gaLabel: 'local', + uuid: savedUser._id, + }); }) .catch(next); },