Beau Barth - Week 5 Homework - Authenticate/Authorize Exercise#22
Beau Barth - Week 5 Homework - Authenticate/Authorize Exercise#22bbarth86 wants to merge 5 commits intopce-uw-jscript400:masterfrom
Conversation
bwreid
left a comment
bwreid
left a comment
There was a problem hiding this comment.
Overall, this looks great! Just a couple JS/syntax issue got you caught up.
| username: String, | ||
| password: String, | ||
| admin: { | ||
| type: String, |
There was a problem hiding this comment.
Why a String as opposed to a Boolean?
| const payload = {username: newUser.username, password: newUser.password }; | ||
| const options = { expiresIn: '1 day' }; | ||
| const token = jsonwebtoken.sign(payload, SECRET_KEY, options); | ||
| res.status(201).json({ status, token}); |
There was a problem hiding this comment.
This is really great. It's easy to read and contains easily identifiable patterns.
| } | ||
| const isValid = await bcrypt.compare(password, user.password); | ||
| if (!isValid) { | ||
| const error = new Error(`Password is invalid.`); |
There was a problem hiding this comment.
You'll want to give errors that are vaguer in the event of logging in. Otherwise, it's easier for someone to "guess" their way into someone else's account.
| } | ||
| const { admin } = req.body; | ||
| if (admin == null) { | ||
| throw new Error(`Unable to authenticate as Admin. Please check your request and try again. `); |
There was a problem hiding this comment.
I think this error should be more about having the data be malformed than anything. By sending admin in the request body, we're just trying to set the admin permissions, not send whether or not we're an admin.
| //return next(error); | ||
| } | ||
| const isAdmin = await User.findOne({_id: isValid.id}); | ||
| if (isAdmin.admin == false) { |
There was a problem hiding this comment.
Because you're storing admin as a string, I imagine this is the reason for the issue.
'false' == false // returns falseI know. JavaScript.
| //return next(error); | ||
| } | ||
| const isAdmin = await User.findOne({_id: isValid.id}); | ||
| if (isAdmin.admin = false) { |
There was a problem hiding this comment.
This one is actually a different issue. Here, you're only using one = which means you're assigning the value of false to isAdmin.admin.
2 participants
Still have some errors i couldn't resolve. In particular, two errors were most frustrating:
Books created when user was not admin - this is noted in my books.js file, even though the user was identified as having admin: false, creation of the book still allowed. Could not figure out why this was happening.
Book reserved by user could not be returned by same user, received "reserved by another user" error. I submitted both reserve and return requests using the same token but could not get the function to work properly.
Feel like i'm starting to finally get the hang of the various model.methods, but continue to struggle with throwing errors properly as well.