Skip to content

Commit 4506c2b

Browse files
catarakcatarak
authored
Merge pull request #1317 from shakti97/login-signup-issue
Fix username/email case issue in login/signup
2 parents b83824d + 2b3be3a commit 4506c2b

File tree

15 files changed

+559
-91
lines changed

15 files changed

+559
-91
lines changed

‎.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,4 @@ localhost.key
1919
privkey.pem
2020

2121
storybook-static
22+
duplicates.json

‎server/config/passport.js

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ passport.deserializeUser((id, done) => {
2424
* Sign in using Email/Username and Password.
2525
*/
2626
passport.use(new LocalStrategy({ usernameField: 'email' }, (email, password, done) => {
27-
User.findByMailOrName(email.toLowerCase())
27+
User.findByEmailOrUsername(email)
2828
.then((user) => { // eslint-disable-line consistent-return
2929
if (!user) {
3030
return done(null, false, { msg: `Email ${email} not found.` });
@@ -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.findByUsername(userid, (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) => {
@@ -98,9 +98,7 @@ passport.use(new GitHubStrategy({
9898
const emails = getVerifiedEmails(profile.emails);
9999
const primaryEmail = getPrimaryEmail(profile.emails);
100100

101-
User.findOne({
102-
email: { $in: emails },
103-
}, (findByEmailErr, existingEmailUser) => {
101+
User.findByEmail(emails, (findByEmailErr, existingEmailUser) => {
104102
if (existingEmailUser) {
105103
existingEmailUser.email = existingEmailUser.email || primaryEmail;
106104
existingEmailUser.github = profile.id;
@@ -141,11 +139,9 @@ passport.use(new GoogleStrategy({
141139

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

144-
User.findOne({
145-
email: primaryEmail,
146-
}, (findByEmailErr, existingEmailUser) => {
142+
User.findByEmail(primaryEmail, (findByEmailErr, existingEmailUser) => {
147143
let username = profile._json.emails[0].value.split('@')[0];
148-
User.findOne({ username }, (findByUsernameErr, existingUsernameUser) => {
144+
User.findByUsername(username, (findByUsernameErr, existingUsernameUser) => {
149145
if (existingUsernameUser) {
150146
const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)];
151147
username = slugify(`${username} ${adj}`);

‎server/controllers/__mocks__/aws.controller.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
export const getObjectKey = jest.mock();
22
export const deleteObjectsFromS3 = jest.fn();
33
export const signS3 = jest.fn();
4-
export const copyObjectInS3 = jest.fn();
4+
export const copyObjectInS3RequestHandler = jest.fn();
55
export const listObjectsInS3ForUser = jest.fn();

‎server/controllers/aws.controller.js

Lines changed: 64 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -98,24 +98,72 @@ export function signS3(req, res) {
9898
res.json(result);
9999
}
100100

101-
export function copyObjectInS3(req, res) {
101+
export function copyObjectInS3(url, userId) {
102+
return new Promise((resolve, reject) => {
103+
const objectKey = getObjectKey(url);
104+
const fileExtension = getExtension(objectKey);
105+
const newFilename = uuid.v4() + fileExtension;
106+
const headParams = {
107+
Bucket: `${process.env.S3_BUCKET}`,
108+
Key: `${objectKey}`
109+
};
110+
client.s3.headObject(headParams, (headErr) => {
111+
if (headErr) {
112+
reject(new Error(`Object with key ${process.env.S3_BUCKET}/${objectKey} does not exist.`));
113+
return;
114+
}
115+
const params = {
116+
Bucket: `${process.env.S3_BUCKET}`,
117+
CopySource: `${process.env.S3_BUCKET}/${objectKey}`,
118+
Key: `${userId}/${newFilename}`,
119+
ACL: 'public-read'
120+
};
121+
const copy = client.copyObject(params);
122+
copy.on('err', (err) => {
123+
reject(err);
124+
});
125+
copy.on('end', (data) => {
126+
resolve(`${s3Bucket}${userId}/${newFilename}`);
127+
});
128+
});
129+
});
130+
}
131+
132+
export function copyObjectInS3RequestHandler(req, res) {
102133
const { url } = req.body;
103-
const objectKey = getObjectKey(url);
104-
const fileExtension = getExtension(objectKey);
105-
const newFilename = uuid.v4() + fileExtension;
106-
const userId = req.user.id;
107-
const params = {
108-
Bucket: `${process.env.S3_BUCKET}`,
109-
CopySource: `${process.env.S3_BUCKET}/${objectKey}`,
110-
Key: `${userId}/${newFilename}`,
111-
ACL: 'public-read'
112-
};
113-
const copy = client.copyObject(params);
114-
copy.on('err', (err) => {
115-
console.log(err);
134+
copyObjectInS3(url, req.user.id).then((newUrl) => {
135+
res.json({ url: newUrl });
116136
});
117-
copy.on('end', (data) => {
118-
res.json({ url: `${s3Bucket}${userId}/${newFilename}` });
137+
}
138+
139+
export function moveObjectToUserInS3(url, userId) {
140+
return new Promise((resolve, reject) => {
141+
const objectKey = getObjectKey(url);
142+
const fileExtension = getExtension(objectKey);
143+
const newFilename = uuid.v4() + fileExtension;
144+
const headParams = {
145+
Bucket: `${process.env.S3_BUCKET}`,
146+
Key: `${objectKey}`
147+
};
148+
client.s3.headObject(headParams, (headErr) => {
149+
if (headErr) {
150+
reject(new Error(`Object with key ${process.env.S3_BUCKET}/${objectKey} does not exist.`));
151+
return;
152+
}
153+
const params = {
154+
Bucket: `${process.env.S3_BUCKET}`,
155+
CopySource: `${process.env.S3_BUCKET}/${objectKey}`,
156+
Key: `${userId}/${newFilename}`,
157+
ACL: 'public-read'
158+
};
159+
const move = client.moveObject(params);
160+
move.on('err', (err) => {
161+
reject(err);
162+
});
163+
move.on('end', (data) => {
164+
resolve(`${s3Bucket}${userId}/${newFilename}`);
165+
});
166+
});
119167
});
120168
}
121169

‎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.findByUsername(username);
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.findByUsername(req.params.username);
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.findByUsername(username, (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.findByUsername(username, (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.findByUsername(username, (err, user) => {
1111
if (err) {
1212
reject(err);
1313
return;

‎server/controllers/user.controller.js

Lines changed: 45 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -30,71 +30,63 @@ const random = (done) => {
3030
};
3131

3232
export function findUserByUsername(username, cb) {
33-
User.findOne(
34-
{ username },
35-
(err, user) => {
36-
cb(user);
37-
}
38-
);
33+
User.findByUsername(username, (err, user) => {
34+
cb(user);
35+
});
3936
}
4037

4138
export function createUser(req, res, next) {
39+
const { username, email } = req.body;
40+
const { password } = req.body;
41+
const emailLowerCase = email.toLowerCase();
4242
const EMAIL_VERIFY_TOKEN_EXPIRY_TIME = Date.now() + (3600000 * 24); // 24 hours
4343
random((tokenError, token) => {
4444
const user = new User({
45-
username: req.body.username,
46-
email: req.body.email,
47-
password: req.body.password,
45+
username,
46+
email: emailLowerCase,
47+
password,
4848
verified: User.EmailConfirmation.Sent,
4949
verifiedToken: token,
5050
verifiedTokenExpires: EMAIL_VERIFY_TOKEN_EXPIRY_TIME,
5151
});
5252

53-
User.findOne(
54-
{
55-
$or: [
56-
{ email: req.body.email },
57-
{ username: req.body.username }
58-
]
59-
},
60-
(err, existingUser) => {
61-
if (err) {
62-
res.status(404).send({ error: err });
63-
return;
64-
}
53+
User.findByEmailAndUsername(email, username, (err, existingUser) => {
54+
if (err) {
55+
res.status(404).send({ error: err });
56+
return;
57+
}
6558

66-
if (existingUser) {
67-
const fieldInUse = existingUser.email === req.body.email ? 'Email' : 'Username';
68-
res.status(422).send({ error: `${fieldInUse} is in use` });
59+
if (existingUser) {
60+
const fieldInUse = existingUser.email.toLowerCase() === emailLowerCase ? 'Email' : 'Username';
61+
res.status(422).send({ error: `${fieldInUse} is in use` });
62+
return;
63+
}
64+
user.save((saveErr) => {
65+
if (saveErr) {
66+
next(saveErr);
6967
return;
7068
}
71-
user.save((saveErr) => {
72-
if (saveErr) {
73-
next(saveErr);
69+
req.logIn(user, (loginErr) => {
70+
if (loginErr) {
71+
next(loginErr);
7472
return;
7573
}
76-
req.logIn(user, (loginErr) => {
77-
if (loginErr) {
78-
next(loginErr);
79-
return;
80-
}
81-
82-
const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http';
83-
const mailOptions = renderEmailConfirmation({
84-
body: {
85-
domain: `${protocol}://${req.headers.host}`,
86-
link: `${protocol}://${req.headers.host}/verify?t=${token}`
87-
},
88-
to: req.user.email,
89-
});
90-
91-
mail.send(mailOptions, (mailErr, result) => { // eslint-disable-line no-unused-vars
92-
res.json(userResponse(req.user));
93-
});
74+
75+
const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http';
76+
const mailOptions = renderEmailConfirmation({
77+
body: {
78+
domain: `${protocol}://${req.headers.host}`,
79+
link: `${protocol}://${req.headers.host}/verify?t=${token}`
80+
},
81+
to: req.user.email,
82+
});
83+
84+
mail.send(mailOptions, (mailErr, result) => { // eslint-disable-line no-unused-vars
85+
res.json(userResponse(req.user));
9486
});
9587
});
96-
}
97-
);
88+
});
89+
});
9890
});
9991
}
10092

@@ -103,7 +95,10 @@ export function duplicateUserCheck(req, res) {
10395
const value = req.query[checkType];
10496
const query = {};
10597
query[checkType] = value;
106-
User.findOne(query, (err, user) => {
98+
// Don't want to use findByEmailOrUsername here, because in this case we do
99+
// want to use case-insensitive search for usernames to prevent username
100+
// duplicates, which overrides the default behavior.
101+
User.findOne(query).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
107102
if (user) {
108103
return res.json({
109104
exists: true,
@@ -147,7 +142,7 @@ export function resetPasswordInitiate(req, res) {
147142
async.waterfall([
148143
random,
149144
(token, done) => {
150-
User.findOne({ email: req.body.email }, (err, user) => {
145+
User.findByEmail(req.body.email, (err, user) => {
151146
if (!user) {
152147
res.json({ success: true, message: 'If the email is registered with the editor, an email has been sent.' });
153148
return;
@@ -277,7 +272,7 @@ export function updatePassword(req, res) {
277272
}
278273

279274
export function userExists(username, callback) {
280-
User.findOne({ username }, (err, user) => (
275+
User.findByUsername(username, (err, user) => (
281276
user ? callback(true) : callback(false)
282277
));
283278
}

0 commit comments

Comments
 (0)