Skip to content

W5: Auth Exercise#14

Open
charlottewest wants to merge 2 commits intopce-uw-jscript400:masterfrom
charlottewest:master
Open

W5: Auth Exercise#14
charlottewest wants to merge 2 commits intopce-uw-jscript400:masterfrom
charlottewest:master

Conversation

@charlottewest
Copy link

No description provided.

Copy link
Collaborator

@bwreid bwreid left a comment

Choose a reason for hiding this comment

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

This looks excellent, Charlotte. As mentioned in the comments, my biggest piece of advice to you would be to try and reduce the depth of your code. Let me know if you have any questions.

const e = new Error('invalid login credentials')
e.status = 400
next(e)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

With try/catch, we could do something like throw new Error('invalid login credentials'). This would move the error to your .catch() and shorten the "depth" of your code.

try {
const token = req.headers.authorization.split('Bearer ')[1]
const payload = jsonwebtoken.verify(token, SECRET_KEY)
if (payload) { //if payload (token verification) returns true, cont.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make your code a bit easier to read, you can also build "guards" into your code. This means flipping the statement so that instead of looking for the path you want to go down, you try and kick the user out for any errors first. For example:

if (!payload) throw new Error(...)

This can make your code much shorter.

changeUser.admin = true
await changeUser.save()

res.json({ status, changeUser })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember that if you use object shorthand, the key will match. So in this case, your response will have a key of "changeUser." I don't think this will make much sense to the end user as opposed to just "user."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants