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 all 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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ localhost.key
privkey.pem

storybook-static
duplicates.json
14 changes: 5 additions & 9 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.findByEmailOrUsername(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.findByUsername(userid, (err, user) => { // eslint-disable-line consistent-return
if (err) { return done(err); }
if (!user) { return done(null, false); }
user.findMatchingKey(key, (innerErr, isMatch, keyDocument) => {
Expand Down Expand Up @@ -98,9 +98,7 @@ passport.use(new GitHubStrategy({
const emails = getVerifiedEmails(profile.emails);
const primaryEmail = getPrimaryEmail(profile.emails);

User.findOne({
email: { $in: emails },
}, (findByEmailErr, existingEmailUser) => {
User.findByEmail(emails, (findByEmailErr, existingEmailUser) => {
if (existingEmailUser) {
existingEmailUser.email = existingEmailUser.email || primaryEmail;
existingEmailUser.github = profile.id;
Expand Down Expand Up @@ -141,11 +139,9 @@ passport.use(new GoogleStrategy({

const primaryEmail = profile._json.emails[0].value;

User.findOne({
email: primaryEmail,
}, (findByEmailErr, existingEmailUser) => {
User.findByEmail(primaryEmail, (findByEmailErr, existingEmailUser) => {
let username = profile._json.emails[0].value.split('@')[0];
User.findOne({ username }, (findByUsernameErr, existingUsernameUser) => {
User.findByUsername(username, (findByUsernameErr, existingUsernameUser) => {
if (existingUsernameUser) {
const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)];
username = slugify(`${username} ${adj}`);
Expand Down
2 changes: 1 addition & 1 deletion server/controllers/__mocks__/aws.controller.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
export const getObjectKey = jest.mock();
export const deleteObjectsFromS3 = jest.fn();
export const signS3 = jest.fn();
export const copyObjectInS3 = jest.fn();
export const copyObjectInS3RequestHandler = jest.fn();
export const listObjectsInS3ForUser = jest.fn();
80 changes: 64 additions & 16 deletions server/controllers/aws.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,24 +98,72 @@ export function signS3(req, res) {
res.json(result);
}

export function copyObjectInS3(req, res) {
export function copyObjectInS3(url, userId) {
return new Promise((resolve, reject) => {
const objectKey = getObjectKey(url);
const fileExtension = getExtension(objectKey);
const newFilename = uuid.v4() + fileExtension;
const headParams = {
Bucket: `${process.env.S3_BUCKET}`,
Key: `${objectKey}`
};
client.s3.headObject(headParams, (headErr) => {
if (headErr) {
reject(new Error(`Object with key ${process.env.S3_BUCKET}/${objectKey} does not exist.`));
return;
}
const params = {
Bucket: `${process.env.S3_BUCKET}`,
CopySource: `${process.env.S3_BUCKET}/${objectKey}`,
Key: `${userId}/${newFilename}`,
ACL: 'public-read'
};
const copy = client.copyObject(params);
copy.on('err', (err) => {
reject(err);
});
copy.on('end', (data) => {
resolve(`${s3Bucket}${userId}/${newFilename}`);
});
});
});
}

export function copyObjectInS3RequestHandler(req, res) {
const { url } = req.body;
const objectKey = getObjectKey(url);
const fileExtension = getExtension(objectKey);
const newFilename = uuid.v4() + fileExtension;
const userId = req.user.id;
const params = {
Bucket: `${process.env.S3_BUCKET}`,
CopySource: `${process.env.S3_BUCKET}/${objectKey}`,
Key: `${userId}/${newFilename}`,
ACL: 'public-read'
};
const copy = client.copyObject(params);
copy.on('err', (err) => {
console.log(err);
copyObjectInS3(url, req.user.id).then((newUrl) => {
res.json({ url: newUrl });
});
copy.on('end', (data) => {
res.json({ url: `${s3Bucket}${userId}/${newFilename}` });
}

export function moveObjectToUserInS3(url, userId) {
return new Promise((resolve, reject) => {
const objectKey = getObjectKey(url);
const fileExtension = getExtension(objectKey);
const newFilename = uuid.v4() + fileExtension;
const headParams = {
Bucket: `${process.env.S3_BUCKET}`,
Key: `${objectKey}`
};
client.s3.headObject(headParams, (headErr) => {
if (headErr) {
reject(new Error(`Object with key ${process.env.S3_BUCKET}/${objectKey} does not exist.`));
return;
}
const params = {
Bucket: `${process.env.S3_BUCKET}`,
CopySource: `${process.env.S3_BUCKET}/${objectKey}`,
Key: `${userId}/${newFilename}`,
ACL: 'public-read'
};
const move = client.moveObject(params);
move.on('err', (err) => {
reject(err);
});
move.on('end', (data) => {
resolve(`${s3Bucket}${userId}/${newFilename}`);
});
});
});
}

Expand Down
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.findByUsername(username);
}

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.findByUsername(req.params.username);
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.findByUsername(username, (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.findByUsername(username, (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.findByUsername(username, (err, user) => {
if (err) {
reject(err);
return;
Expand Down
95 changes: 45 additions & 50 deletions server/controllers/user.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,71 +30,63 @@ const random = (done) => {
};

export function findUserByUsername(username, cb) {
User.findOne(
{ username },
(err, user) => {
cb(user);
}
);
User.findByUsername(username, (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.findByEmailAndUsername(email, username, (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 +95,10 @@ export function duplicateUserCheck(req, res) {
const value = req.query[checkType];
const query = {};
query[checkType] = value;
User.findOne(query, (err, user) => {
// Don't want to use findByEmailOrUsername here, because in this case we do
// want to use case-insensitive search for usernames to prevent username
// duplicates, which overrides the default behavior.
User.findOne(query).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
if (user) {
return res.json({
exists: true,
Expand Down Expand Up @@ -147,7 +142,7 @@ export function resetPasswordInitiate(req, res) {
async.waterfall([
random,
(token, done) => {
User.findOne({ email: req.body.email }, (err, user) => {
User.findByEmail(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;
Expand Down Expand Up @@ -277,7 +272,7 @@ export function updatePassword(req, res) {
}

export function userExists(username, callback) {
User.findOne({ username }, (err, user) => (
User.findByUsername(username, (err, user) => (
user ? callback(true) : callback(false)
));
}
Expand Down
Loading