From 15ad07d5cebcc56c17f7d63680f5b08d656b1bbb Mon Sep 17 00:00:00 2001 From: Cassie Tarakajian Date: Tue, 14 Jul 2020 18:16:17 -0400 Subject: [PATCH] [#1314][#1489] Use collation instead of RegEx - Add case insensitive indexes for User.email and User.username - Update user queries by username or email so that they are case insensitive --- server/config/passport.js | 79 ++++++------- .../collectionForUserExists.js | 2 +- .../collection.controller/listCollections.js | 3 +- server/controllers/project.controller.js | 4 +- .../project.controller/getProjectsForUser.js | 2 +- server/controllers/user.controller.js | 106 +++++++++--------- server/models/user.js | 21 ++-- 7 files changed, 110 insertions(+), 107 deletions(-) diff --git a/server/config/passport.js b/server/config/passport.js index 3ef7a0e2..4c436160 100644 --- a/server/config/passport.js +++ b/server/config/passport.js @@ -43,7 +43,7 @@ passport.use(new LocalStrategy({ usernameField: 'email' }, (email, password, don * Authentificate using Basic Auth (Username + Api Key) */ passport.use(new BasicStrategy((userid, key, done) => { - User.findOne({ username: userid }, (err, user) => { // eslint-disable-line consistent-return + User.findOne({ username: userid }).collation({ locale: 'en', strength: 2 }).exec((err, user) => { // eslint-disable-line consistent-return if (err) { return done(err); } if (!user) { return done(null, false); } user.findMatchingKey(key, (innerErr, isMatch, keyDocument) => { @@ -100,7 +100,7 @@ passport.use(new GitHubStrategy({ User.findOne({ email: { $in: emails }, - }, (findByEmailErr, existingEmailUser) => { + }).collation({ locale: 'en', strength: 2 }).exec((findByEmailErr, existingEmailUser) => { if (existingEmailUser) { existingEmailUser.email = existingEmailUser.email || primaryEmail; existingEmailUser.github = profile.id; @@ -143,44 +143,45 @@ passport.use(new GoogleStrategy({ User.findOne({ email: primaryEmail, - }, (findByEmailErr, existingEmailUser) => { + }).collation({ locale: 'en', strength: 2 }).exec((findByEmailErr, existingEmailUser) => { let username = profile._json.emails[0].value.split('@')[0]; - User.findOne({ username }, (findByUsernameErr, existingUsernameUser) => { - if (existingUsernameUser) { - const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)]; - username = slugify(`${username} ${adj}`); - } - // what if a username is already taken from the display name too? - // then, append a random friendly word? - if (existingEmailUser) { - existingEmailUser.email = existingEmailUser.email || primaryEmail; - existingEmailUser.google = profile._json.emails[0].value; - existingEmailUser.username = existingEmailUser.username || username; - existingEmailUser.tokens.push({ kind: 'google', accessToken }); - existingEmailUser.name = existingEmailUser.name || profile._json.displayName; - existingEmailUser.verified = User.EmailConfirmation.Verified; - existingEmailUser.save((saveErr) => { - if (saveErr) { - console.log(saveErr); - } - done(null, existingEmailUser); - }); - } else { - const user = new User(); - user.email = primaryEmail; - user.google = profile._json.emails[0].value; - user.username = username; - user.tokens.push({ kind: 'google', accessToken }); - user.name = profile._json.displayName; - user.verified = User.EmailConfirmation.Verified; - user.save((saveErr) => { - if (saveErr) { - console.log(saveErr); - } - done(null, user); - }); - } - }); + User.findOne({ username }).collation({ locale: 'en', strength: 2 }) + .exec((findByUsernameErr, existingUsernameUser) => { + if (existingUsernameUser) { + const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)]; + username = slugify(`${username} ${adj}`); + } + // what if a username is already taken from the display name too? + // then, append a random friendly word? + if (existingEmailUser) { + existingEmailUser.email = existingEmailUser.email || primaryEmail; + existingEmailUser.google = profile._json.emails[0].value; + existingEmailUser.username = existingEmailUser.username || username; + existingEmailUser.tokens.push({ kind: 'google', accessToken }); + existingEmailUser.name = existingEmailUser.name || profile._json.displayName; + existingEmailUser.verified = User.EmailConfirmation.Verified; + existingEmailUser.save((saveErr) => { + if (saveErr) { + console.log(saveErr); + } + done(null, existingEmailUser); + }); + } else { + const user = new User(); + user.email = primaryEmail; + user.google = profile._json.emails[0].value; + user.username = username; + user.tokens.push({ kind: 'google', accessToken }); + user.name = profile._json.displayName; + user.verified = User.EmailConfirmation.Verified; + user.save((saveErr) => { + if (saveErr) { + console.log(saveErr); + } + done(null, user); + }); + } + }); }); }); })); diff --git a/server/controllers/collection.controller/collectionForUserExists.js b/server/controllers/collection.controller/collectionForUserExists.js index e2881fd4..8315d50c 100644 --- a/server/controllers/collection.controller/collectionForUserExists.js +++ b/server/controllers/collection.controller/collectionForUserExists.js @@ -11,7 +11,7 @@ export default function collectionForUserExists(username, collectionId, callback } function findUser() { - return User.findOne({ username }); + return User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec(); } function findCollection(owner) { diff --git a/server/controllers/collection.controller/listCollections.js b/server/controllers/collection.controller/listCollections.js index c71041b3..1def86aa 100644 --- a/server/controllers/collection.controller/listCollections.js +++ b/server/controllers/collection.controller/listCollections.js @@ -3,7 +3,8 @@ import User from '../../models/user'; async function getOwnerUserId(req) { if (req.params.username) { - const user = await User.findOne({ username: req.params.username }); + const user = + await User.findOne({ username: req.params.username }).collation({ locale: 'en', strength: 2 }).exec(); if (user && user._id) { return user._id; } diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index ab4d4a63..9cc58fd2 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -64,7 +64,7 @@ export function updateProject(req, res) { export function getProject(req, res) { const { project_id: projectId, username } = req.params; - User.findOne({ username }, (err, user) => { // eslint-disable-line + User.findOne({ username }).collation({ locale: "en", strength: 2 }).exec((err, user) => { // eslint-disable-line if (!user) { return res.status(404).send({ message: 'Project with that username does not exist' }); } @@ -141,7 +141,7 @@ export function projectExists(projectId, callback) { } export function projectForUserExists(username, projectId, callback) { - User.findOne({ username }, (err, user) => { + User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec((err, user) => { if (!user) { callback(false); return; diff --git a/server/controllers/project.controller/getProjectsForUser.js b/server/controllers/project.controller/getProjectsForUser.js index 1d9a0e34..453e2ebe 100644 --- a/server/controllers/project.controller/getProjectsForUser.js +++ b/server/controllers/project.controller/getProjectsForUser.js @@ -7,7 +7,7 @@ const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); function getProjectsForUserName(username) { return new Promise((resolve, reject) => { - User.findOne({ username }, (err, user) => { + User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec((err, user) => { if (err) { reject(err); return; diff --git a/server/controllers/user.controller.js b/server/controllers/user.controller.js index f06c3d99..56aa574e 100644 --- a/server/controllers/user.controller.js +++ b/server/controllers/user.controller.js @@ -1,6 +1,5 @@ import crypto from 'crypto'; import async from 'async'; -import escapeStringRegexp from 'escape-string-regexp'; import User from '../models/user'; import mail from '../utils/mail'; @@ -31,12 +30,9 @@ const random = (done) => { }; export function findUserByUsername(username, cb) { - User.findOne( - { username }, - (err, user) => { - cb(user); - } - ); + User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec((err, user) => { + cb(user); + }); } export function createUser(req, res, next) { @@ -54,51 +50,48 @@ export function createUser(req, res, next) { verifiedTokenExpires: EMAIL_VERIFY_TOKEN_EXPIRY_TIME, }); - User.findOne( - { - $or: [ - { email: new RegExp(`^${escapeStringRegexp(email)}$`, 'i') }, - { username: new RegExp(`^${escapeStringRegexp(username)}$`, 'i') } - ] - }, - (err, existingUser) => { - if (err) { - res.status(404).send({ error: err }); - return; - } + User.findOne({ + $or: [ + { email }, + { username } + ] + }).collation({ locale: 'en', strength: 2 }).exec((err, existingUser) => { + if (err) { + res.status(404).send({ error: err }); + return; + } - if (existingUser) { - const fieldInUse = existingUser.email.toLowerCase() === emailLowerCase ? 'Email' : 'Username'; - res.status(422).send({ error: `${fieldInUse} is in use` }); + if (existingUser) { + const fieldInUse = existingUser.email.toLowerCase() === emailLowerCase ? 'Email' : 'Username'; + res.status(422).send({ error: `${fieldInUse} is in use` }); + return; + } + user.save((saveErr) => { + if (saveErr) { + next(saveErr); return; } - user.save((saveErr) => { - if (saveErr) { - next(saveErr); + req.logIn(user, (loginErr) => { + if (loginErr) { + next(loginErr); return; } - req.logIn(user, (loginErr) => { - if (loginErr) { - next(loginErr); - return; - } - const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http'; - const mailOptions = renderEmailConfirmation({ - body: { - domain: `${protocol}://${req.headers.host}`, - link: `${protocol}://${req.headers.host}/verify?t=${token}` - }, - to: req.user.email, - }); + const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http'; + const mailOptions = renderEmailConfirmation({ + body: { + domain: `${protocol}://${req.headers.host}`, + link: `${protocol}://${req.headers.host}/verify?t=${token}` + }, + to: req.user.email, + }); - mail.send(mailOptions, (mailErr, result) => { // eslint-disable-line no-unused-vars - res.json(userResponse(req.user)); - }); + mail.send(mailOptions, (mailErr, result) => { // eslint-disable-line no-unused-vars + res.json(userResponse(req.user)); }); }); - } - ); + }); + }); }); } @@ -106,8 +99,8 @@ export function duplicateUserCheck(req, res) { const checkType = req.query.check_type; const value = req.query[checkType]; const query = {}; - query[checkType] = new RegExp(`^${escapeStringRegexp(value)}$`, 'i'); - User.findOne(query, (err, user) => { + query[checkType] = value; + User.findOne(query).collation({ locale: 'en', strength: 2 }).exec((err, user) => { if (user) { return res.json({ exists: true, @@ -151,18 +144,19 @@ export function resetPasswordInitiate(req, res) { async.waterfall([ random, (token, done) => { - User.findOne({ email: req.body.email.toLowerCase() }, (err, user) => { - if (!user) { - res.json({ success: true, message: 'If the email is registered with the editor, an email has been sent.' }); - return; - } - user.resetPasswordToken = token; - user.resetPasswordExpires = Date.now() + 3600000; // 1 hour + User.findOne({ email: req.body.email.toLowerCase() }) + .collation({ locale: 'en', strength: 2 }).exec((err, user) => { + if (!user) { + res.json({ success: true, message: 'If the email is registered with the editor, an email has been sent.' }); + return; + } + user.resetPasswordToken = token; + user.resetPasswordExpires = Date.now() + 3600000; // 1 hour - user.save((saveErr) => { - done(saveErr, token, user); + user.save((saveErr) => { + done(saveErr, token, user); + }); }); - }); }, (token, user, done) => { const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http'; @@ -281,7 +275,7 @@ export function updatePassword(req, res) { } export function userExists(username, callback) { - User.findOne({ username: new RegExp(`^${escapeStringRegexp(username)}$`, 'i') }, (err, user) => ( + User.findOne(username).collation({ locale: 'en', strength: 2 }).exec((err, user) => ( user ? callback(true) : callback(false) )); } diff --git a/server/models/user.js b/server/models/user.js index 321c2231..1f91c104 100644 --- a/server/models/user.js +++ b/server/models/user.js @@ -1,5 +1,4 @@ import mongoose from 'mongoose'; -import escapeStringRegexp from 'escape-string-regexp'; const bcrypt = require('bcrypt-nodejs'); @@ -143,16 +142,24 @@ userSchema.methods.findMatchingKey = function findMatchingKey(candidateKey, cb) }; userSchema.statics.findByMailOrName = function findByMailOrName(email) { + const isEmail = email.indexOf('@') > -1; + if (isEmail) { + const query = { + email: email.toLowerCase() + }; + // once emails are all lowercase, won't need to do collation + // but maybe it's not even necessary to make all emails lowercase?? + return this.findOne(query).collation({ locale: 'en', strength: 2 }).exec(); + } const query = { - $or: [{ - email: new RegExp(`^${escapeStringRegexp(email)}$`, 'i'), - }, { - username: new RegExp(`^${escapeStringRegexp(email)}$`, 'i'), - }], + username: email }; - return this.findOne(query).exec(); + return this.findOne(query).collation({ locale: 'en', strength: 2 }).exec(); }; userSchema.statics.EmailConfirmation = EmailConfirmationStates; +userSchema.index({ username: 1 }, { collation: { locale: 'en', strength: 2 } }); +userSchema.index({ email: 1 }, { collation: { locale: 'en', strength: 2 } }); + export default mongoose.model('User', userSchema);