Skip to content

Fix username/email case issue in login/signup #1317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jul 20, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 41 additions & 40 deletions server/config/passport.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ passport.deserializeUser((id, done) => {
* Sign in using Email/Username and Password.
*/
passport.use(new LocalStrategy({ usernameField: 'email' }, (email, password, done) => {
User.findByMailOrName(email.toLowerCase())
User.findByMailOrName(email)
.then((user) => { // eslint-disable-line consistent-return
if (!user) {
return done(null, false, { msg: `Email ${email} not found.` });
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add statics on the User model called User.findByUsername() and User.findByEmail() that will perform the User.findOne().collation(...) query.

This avoids duplication and ensures we won't forget to call it. It also means those statics can have a comment about what collation is doing as it's not very obvious.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! Good idea.

if (err) { return done(err); }
if (!user) { return done(null, false); }
user.findMatchingKey(key, (innerErr, isMatch, keyDocument) => {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
});
}
});
});
});
}));
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion server/controllers/collection.controller/listCollections.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions server/controllers/project.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
}
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
116 changes: 57 additions & 59 deletions server/controllers/user.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,71 +30,68 @@ 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) {
const { username, email } = req.body;
const { password } = req.body;
const emailLowerCase = email.toLowerCase();
const EMAIL_VERIFY_TOKEN_EXPIRY_TIME = Date.now() + (3600000 * 24); // 24 hours
random((tokenError, token) => {
const user = new User({
username: req.body.username,
email: req.body.email,
password: req.body.password,
username,
email: emailLowerCase,
password,
verified: User.EmailConfirmation.Sent,
verifiedToken: token,
verifiedTokenExpires: EMAIL_VERIFY_TOKEN_EXPIRY_TIME,
});

User.findOne(
{
$or: [
{ email: req.body.email },
{ username: req.body.username }
]
},
(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 === req.body.email ? '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,
});

mail.send(mailOptions, (mailErr, result) => { // eslint-disable-line no-unused-vars
res.json(userResponse(req.user));
});

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));
});
});
}
);
});
});
});
}

Expand All @@ -103,7 +100,7 @@ export function duplicateUserCheck(req, res) {
const value = req.query[checkType];
const query = {};
query[checkType] = value;
User.findOne(query, (err, user) => {
User.findOne(query).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
if (user) {
return res.json({
exists: true,
Expand Down Expand Up @@ -147,18 +144,19 @@ export function resetPasswordInitiate(req, res) {
async.waterfall([
random,
(token, done) => {
User.findOne({ email: req.body.email }, (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';
Expand Down Expand Up @@ -277,7 +275,7 @@ export function updatePassword(req, res) {
}

export function userExists(username, callback) {
User.findOne({ username }, (err, user) => (
User.findOne(username).collation({ locale: 'en', strength: 2 }).exec((err, user) => (
user ? callback(true) : callback(false)
));
}
Expand Down
20 changes: 14 additions & 6 deletions server/models/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,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,
}, {
username: email,
}],
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);