Skip to content

Commit 15ad07d

Browse files
catarakcatarak
committed
[#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
1 parent 16860d7 commit 15ad07d

File tree

7 files changed

+112
-109
lines changed

7 files changed

+112
-109
lines changed

‎server/config/passport.js‎

Lines changed: 40 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ passport.use(new LocalStrategy({ usernameField: 'email' }, (email, password, don
4343
* Authentificate using Basic Auth (Username + Api Key)
4444
*/
4545
passport.use(new BasicStrategy((userid, key, done) => {
46-
User.findOne({ username: userid }, (err, user) => { // eslint-disable-line consistent-return
46+
User.findOne({ username: userid }).collation({ locale: 'en', strength: 2 }).exec((err, user) => { // eslint-disable-line consistent-return
4747
if (err) { return done(err); }
4848
if (!user) { return done(null, false); }
4949
user.findMatchingKey(key, (innerErr, isMatch, keyDocument) => {
@@ -100,7 +100,7 @@ passport.use(new GitHubStrategy({
100100

101101
User.findOne({
102102
email: { $in: emails },
103-
}, (findByEmailErr, existingEmailUser) => {
103+
}).collation({ locale: 'en', strength: 2 }).exec((findByEmailErr, existingEmailUser) => {
104104
if (existingEmailUser) {
105105
existingEmailUser.email = existingEmailUser.email || primaryEmail;
106106
existingEmailUser.github = profile.id;
@@ -143,44 +143,45 @@ passport.use(new GoogleStrategy({
143143

144144
User.findOne({
145145
email: primaryEmail,
146-
}, (findByEmailErr, existingEmailUser) => {
146+
}).collation({ locale: 'en', strength: 2 }).exec((findByEmailErr, existingEmailUser) => {
147147
let username = profile._json.emails[0].value.split('@')[0];
148-
User.findOne({ username }, (findByUsernameErr, existingUsernameUser) => {
149-
if (existingUsernameUser) {
150-
const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)];
151-
username = slugify(`${username} ${adj}`);
152-
}
153-
// what if a username is already taken from the display name too?
154-
// then, append a random friendly word?
155-
if (existingEmailUser) {
156-
existingEmailUser.email = existingEmailUser.email || primaryEmail;
157-
existingEmailUser.google = profile._json.emails[0].value;
158-
existingEmailUser.username = existingEmailUser.username || username;
159-
existingEmailUser.tokens.push({ kind: 'google', accessToken });
160-
existingEmailUser.name = existingEmailUser.name || profile._json.displayName;
161-
existingEmailUser.verified = User.EmailConfirmation.Verified;
162-
existingEmailUser.save((saveErr) => {
163-
if (saveErr) {
164-
console.log(saveErr);
165-
}
166-
done(null, existingEmailUser);
167-
});
168-
} else {
169-
const user = new User();
170-
user.email = primaryEmail;
171-
user.google = profile._json.emails[0].value;
172-
user.username = username;
173-
user.tokens.push({ kind: 'google', accessToken });
174-
user.name = profile._json.displayName;
175-
user.verified = User.EmailConfirmation.Verified;
176-
user.save((saveErr) => {
177-
if (saveErr) {
178-
console.log(saveErr);
179-
}
180-
done(null, user);
181-
});
182-
}
183-
});
148+
User.findOne({ username }).collation({ locale: 'en', strength: 2 })
149+
.exec((findByUsernameErr, existingUsernameUser) => {
150+
if (existingUsernameUser) {
151+
const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)];
152+
username = slugify(`${username} ${adj}`);
153+
}
154+
// what if a username is already taken from the display name too?
155+
// then, append a random friendly word?
156+
if (existingEmailUser) {
157+
existingEmailUser.email = existingEmailUser.email || primaryEmail;
158+
existingEmailUser.google = profile._json.emails[0].value;
159+
existingEmailUser.username = existingEmailUser.username || username;
160+
existingEmailUser.tokens.push({ kind: 'google', accessToken });
161+
existingEmailUser.name = existingEmailUser.name || profile._json.displayName;
162+
existingEmailUser.verified = User.EmailConfirmation.Verified;
163+
existingEmailUser.save((saveErr) => {
164+
if (saveErr) {
165+
console.log(saveErr);
166+
}
167+
done(null, existingEmailUser);
168+
});
169+
} else {
170+
const user = new User();
171+
user.email = primaryEmail;
172+
user.google = profile._json.emails[0].value;
173+
user.username = username;
174+
user.tokens.push({ kind: 'google', accessToken });
175+
user.name = profile._json.displayName;
176+
user.verified = User.EmailConfirmation.Verified;
177+
user.save((saveErr) => {
178+
if (saveErr) {
179+
console.log(saveErr);
180+
}
181+
done(null, user);
182+
});
183+
}
184+
});
184185
});
185186
});
186187
}));

‎server/controllers/collection.controller/collectionForUserExists.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export default function collectionForUserExists(username, collectionId, callback
1111
}
1212

1313
function findUser() {
14-
return User.findOne({ username });
14+
return User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec();
1515
}
1616

1717
function findCollection(owner) {

‎server/controllers/collection.controller/listCollections.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@ import User from '../../models/user';
33

44
async function getOwnerUserId(req) {
55
if (req.params.username) {
6-
const user = await User.findOne({ username: req.params.username });
6+
const user =
7+
await User.findOne({ username: req.params.username }).collation({ locale: 'en', strength: 2 }).exec();
78
if (user && user._id) {
89
return user._id;
910
}

‎server/controllers/project.controller.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export function updateProject(req, res) {
6464

6565
export function getProject(req, res) {
6666
const { project_id: projectId, username } = req.params;
67-
User.findOne({ username }, (err, user) => { // eslint-disable-line
67+
User.findOne({ username }).collation({ locale: "en", strength: 2 }).exec((err, user) => { // eslint-disable-line
6868
if (!user) {
6969
return res.status(404).send({ message: 'Project with that username does not exist' });
7070
}
@@ -141,7 +141,7 @@ export function projectExists(projectId, callback) {
141141
}
142142

143143
export function projectForUserExists(username, projectId, callback) {
144-
User.findOne({ username }, (err, user) => {
144+
User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
145145
if (!user) {
146146
callback(false);
147147
return;

‎server/controllers/project.controller/getProjectsForUser.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ const UserNotFoundError = createApplicationErrorClass('UserNotFoundError');
77

88
function getProjectsForUserName(username) {
99
return new Promise((resolve, reject) => {
10-
User.findOne({ username }, (err, user) => {
10+
User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
1111
if (err) {
1212
reject(err);
1313
return;

‎server/controllers/user.controller.js‎

Lines changed: 52 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import crypto from 'crypto';
22
import async from 'async';
3-
import escapeStringRegexp from 'escape-string-regexp';
43

54
import User from '../models/user';
65
import mail from '../utils/mail';
@@ -31,12 +30,9 @@ const random = (done) => {
3130
};
3231

3332
export function findUserByUsername(username, cb) {
34-
User.findOne(
35-
{ username },
36-
(err, user) => {
37-
cb(user);
38-
}
39-
);
33+
User.findOne({ username }).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
34+
cb(user);
35+
});
4036
}
4137

4238
export function createUser(req, res, next) {
@@ -54,60 +50,57 @@ export function createUser(req, res, next) {
5450
verifiedTokenExpires: EMAIL_VERIFY_TOKEN_EXPIRY_TIME,
5551
});
5652

57-
User.findOne(
58-
{
59-
$or: [
60-
{ email: new RegExp(`^${escapeStringRegexp(email)}$`, 'i') },
61-
{ username: new RegExp(`^${escapeStringRegexp(username)}$`, 'i') }
62-
]
63-
},
64-
(err, existingUser) => {
65-
if (err) {
66-
res.status(404).send({ error: err });
67-
return;
68-
}
53+
User.findOne({
54+
$or: [
55+
{ email },
56+
{ username }
57+
]
58+
}).collation({ locale: 'en', strength: 2 }).exec((err, existingUser) => {
59+
if (err) {
60+
res.status(404).send({ error: err });
61+
return;
62+
}
6963

70-
if (existingUser) {
71-
const fieldInUse = existingUser.email.toLowerCase() === emailLowerCase ? 'Email' : 'Username';
72-
res.status(422).send({ error: `${fieldInUse} is in use` });
64+
if (existingUser) {
65+
const fieldInUse = existingUser.email.toLowerCase() === emailLowerCase ? 'Email' : 'Username';
66+
res.status(422).send({ error: `${fieldInUse} is in use` });
67+
return;
68+
}
69+
user.save((saveErr) => {
70+
if (saveErr) {
71+
next(saveErr);
7372
return;
7473
}
75-
user.save((saveErr) => {
76-
if (saveErr) {
77-
next(saveErr);
74+
req.logIn(user, (loginErr) => {
75+
if (loginErr) {
76+
next(loginErr);
7877
return;
7978
}
80-
req.logIn(user, (loginErr) => {
81-
if (loginErr) {
82-
next(loginErr);
83-
return;
84-
}
85-
86-
const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http';
87-
const mailOptions = renderEmailConfirmation({
88-
body: {
89-
domain: `${protocol}://${req.headers.host}`,
90-
link: `${protocol}://${req.headers.host}/verify?t=${token}`
91-
},
92-
to: req.user.email,
93-
});
94-
95-
mail.send(mailOptions, (mailErr, result) => { // eslint-disable-line no-unused-vars
96-
res.json(userResponse(req.user));
97-
});
79+
80+
const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http';
81+
const mailOptions = renderEmailConfirmation({
82+
body: {
83+
domain: `${protocol}://${req.headers.host}`,
84+
link: `${protocol}://${req.headers.host}/verify?t=${token}`
85+
},
86+
to: req.user.email,
87+
});
88+
89+
mail.send(mailOptions, (mailErr, result) => { // eslint-disable-line no-unused-vars
90+
res.json(userResponse(req.user));
9891
});
9992
});
100-
}
101-
);
93+
});
94+
});
10295
});
10396
}
10497

10598
export function duplicateUserCheck(req, res) {
10699
const checkType = req.query.check_type;
107100
const value = req.query[checkType];
108101
const query = {};
109-
query[checkType] = new RegExp(`^${escapeStringRegexp(value)}$`, 'i');
110-
User.findOne(query, (err, user) => {
102+
query[checkType] = value;
103+
User.findOne(query).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
111104
if (user) {
112105
return res.json({
113106
exists: true,
@@ -151,18 +144,19 @@ export function resetPasswordInitiate(req, res) {
151144
async.waterfall([
152145
random,
153146
(token, done) => {
154-
User.findOne({ email: req.body.email.toLowerCase() }, (err, user) => {
155-
if (!user) {
156-
res.json({ success: true, message: 'If the email is registered with the editor, an email has been sent.' });
157-
return;
158-
}
159-
user.resetPasswordToken = token;
160-
user.resetPasswordExpires = Date.now() + 3600000; // 1 hour
147+
User.findOne({ email: req.body.email.toLowerCase() })
148+
.collation({ locale: 'en', strength: 2 }).exec((err, user) => {
149+
if (!user) {
150+
res.json({ success: true, message: 'If the email is registered with the editor, an email has been sent.' });
151+
return;
152+
}
153+
user.resetPasswordToken = token;
154+
user.resetPasswordExpires = Date.now() + 3600000; // 1 hour
161155

162-
user.save((saveErr) => {
163-
done(saveErr, token, user);
156+
user.save((saveErr) => {
157+
done(saveErr, token, user);
158+
});
164159
});
165-
});
166160
},
167161
(token, user, done) => {
168162
const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http';
@@ -281,7 +275,7 @@ export function updatePassword(req, res) {
281275
}
282276

283277
export function userExists(username, callback) {
284-
User.findOne({ username: new RegExp(`^${escapeStringRegexp(username)}$`, 'i') }, (err, user) => (
278+
User.findOne(username).collation({ locale: 'en', strength: 2 }).exec((err, user) => (
285279
user ? callback(true) : callback(false)
286280
));
287281
}

‎server/models/user.js‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import mongoose from 'mongoose';
2-
import escapeStringRegexp from 'escape-string-regexp';
32

43
const bcrypt = require('bcrypt-nodejs');
54

@@ -143,16 +142,24 @@ userSchema.methods.findMatchingKey = function findMatchingKey(candidateKey, cb)
143142
};
144143

145144
userSchema.statics.findByMailOrName = function findByMailOrName(email) {
145+
const isEmail = email.indexOf('@') > -1;
146+
if (isEmail) {
147+
const query = {
148+
email: email.toLowerCase()
149+
};
150+
// once emails are all lowercase, won't need to do collation
151+
// but maybe it's not even necessary to make all emails lowercase??
152+
return this.findOne(query).collation({ locale: 'en', strength: 2 }).exec();
153+
}
146154
const query = {
147-
$or: [{
148-
email: new RegExp(`^${escapeStringRegexp(email)}$`, 'i'),
149-
}, {
150-
username: new RegExp(`^${escapeStringRegexp(email)}$`, 'i'),
151-
}],
155+
username: email
152156
};
153-
return this.findOne(query).exec();
157+
return this.findOne(query).collation({ locale: 'en', strength: 2 }).exec();
154158
};
155159

156160
userSchema.statics.EmailConfirmation = EmailConfirmationStates;
157161

162+
userSchema.index({ username: 1 }, { collation: { locale: 'en', strength: 2 } });
163+
userSchema.index({ email: 1 }, { collation: { locale: 'en', strength: 2 } });
164+
158165
export default mongoose.model('User', userSchema);

0 commit comments

Comments
 (0)