-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
code review start #142
base: main
Are you sure you want to change the base?
code review start #142
Changes from all commits
6dc2ad9
41d255b
73c8894
d242bd8
1ca587c
b75cd49
83b02da
414c721
d2e731a
e8976e6
b7eca89
976eac0
b7d1222
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
const jwt = require('jsonwebtoken') | ||
const { JWT_SECRET } = require("../secrets"); // use this secret! | ||
|
||
const User = require('../users/users-model') | ||
|
||
const restricted = (req, res, next) => { | ||
/* | ||
If the user does not provide a token in the Authorization header: | ||
|
@@ -10,12 +13,23 @@ const restricted = (req, res, next) => { | |
|
||
If the provided token does not verify: | ||
status 401 | ||
{ | ||
{ | ||
"message": "Token invalid" | ||
} | ||
|
||
Put the decoded token in the req object, to make life easier for middlewares downstream! | ||
*/ | ||
const token = req.headers.authorization | ||
if (!token){ | ||
return next({ status: 401, message: 'Token required'}) | ||
} | ||
jwt.verify(token, JWT_SECRET, (err, decode)=>{ | ||
if(err){ | ||
return next({ status: 401, message: 'Token invalid'}) | ||
} | ||
req.decodedJWT = decode | ||
next() | ||
}) | ||
} | ||
|
||
const only = role_name => (req, res, next) => { | ||
|
@@ -29,17 +43,34 @@ const only = role_name => (req, res, next) => { | |
|
||
Pull the decoded token from the req object, to avoid verifying it again! | ||
*/ | ||
if ( req.decodedJWT.role_name === role_name ){ | ||
next() | ||
} else { | ||
next({ status: 403, message: 'This is not for you'}) | ||
} | ||
} | ||
|
||
|
||
const checkUsernameExists = (req, res, next) => { | ||
const checkUsernameExists = async (req, res, next) => { | ||
/* | ||
If the username in req.body does NOT exist in the database | ||
status 401 | ||
{ | ||
"message": "Invalid credentials" | ||
} | ||
*/ | ||
const { username } = req.body | ||
try{ | ||
const dbUsername = await User.findBy({ username }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dbUsername actually is a collection == dbUsers returns an array There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. generalize |
||
if( !dbUsername ){ | ||
return next({ status: 401, message: 'Invalid credentials'}) | ||
} else{ | ||
req.user = dbUsername | ||
next() | ||
} | ||
} catch(err){ | ||
next(err) | ||
} | ||
} | ||
|
||
|
||
|
@@ -62,6 +93,21 @@ const validateRoleName = (req, res, next) => { | |
"message": "Role name can not be longer than 32 chars" | ||
} | ||
*/ | ||
let { role_name } = req.body | ||
// let role_name = role_name.trim() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. take out the comments as normal --- delete it! |
||
if( !role_name || role_name.trim() === undefined ){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. after || will never be called |
||
req.role_name = 'student' | ||
next() | ||
} else { | ||
if ( role_name.trim() === 'admin'){ | ||
return next({ status: 422, message: 'Role name can not be admin'}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return is not necessary if you have "else/else if" right after... |
||
} else if (role_name.trim().length > 32 ) { | ||
return next({ status: 422, message: 'Role name can not be longer than 32 chars' }) | ||
} else { | ||
req.role_name = role_name.trim() | ||
next() | ||
} | ||
} | ||
} | ||
|
||
module.exports = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,12 @@ | ||
const router = require("express").Router(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. be consistent with semicolons !! |
||
const bcrypt = require('bcryptjs') | ||
const tokenBuilder = require('./helpers') | ||
const { checkUsernameExists, validateRoleName } = require('./auth-middleware'); | ||
const { JWT_SECRET } = require("../secrets"); // use this secret! | ||
const { BCRYPT_ROUNDS } = require("../secrets"); // use this secret! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will the comment be a lifesaver here?? if yes, leave in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no misleading comments |
||
|
||
router.post("/register", validateRoleName, (req, res, next) => { | ||
const User = require('./../users/users-model') | ||
|
||
router.post("/register", validateRoleName, async (req, res, next) => { | ||
/** | ||
[POST] /api/auth/register { "username": "anna", "password": "1234", "role_name": "angel" } | ||
|
||
|
@@ -14,6 +18,16 @@ router.post("/register", validateRoleName, (req, res, next) => { | |
"role_name": "angel" | ||
} | ||
*/ | ||
|
||
const { username, password/* , role_name */ } = req.body | ||
const hash = bcrypt.hashSync(password, BCRYPT_ROUNDS) | ||
const role = req.role_name | ||
try { | ||
const newUser = await User.add({ username, password: hash, role_name: role }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keep lines under 100 at least ((maybe 80 if possible)) |
||
res.status(201).json(newUser) | ||
} catch (err) { | ||
next(err) | ||
} | ||
}); | ||
|
||
|
||
|
@@ -37,6 +51,20 @@ router.post("/login", checkUsernameExists, (req, res, next) => { | |
"role_name": "admin" // the role of the authenticated user | ||
} | ||
*/ | ||
|
||
try { | ||
let user = req.user | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could have destructured |
||
let { password } = req.body | ||
|
||
if (user && bcrypt.compareSync(password, user.password)) { | ||
const token = tokenBuilder(user) | ||
res.json({ message: `${user.username} is back!`, token }) | ||
} else { | ||
next({ status: 401, message: 'Invalid Credentials' }) | ||
} | ||
} catch (err) { | ||
next(err) | ||
} | ||
}); | ||
|
||
module.exports = router; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
const jwt = require('jsonwebtoken') | ||
const { JWT_SECRET } = require('../secrets') | ||
|
||
function tokenBuilder(user) { | ||
const payload = { | ||
subject: user.user_id, | ||
username: user.username, | ||
role_name: user.role_name, | ||
} | ||
const options = { | ||
expiresIn: '1d', | ||
} | ||
const token = jwt.sign(payload, JWT_SECRET, options) | ||
|
||
return token | ||
} | ||
|
||
module.exports = tokenBuilder |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,9 @@ function find() { | |
} | ||
] | ||
*/ | ||
return db('users as u') | ||
.leftJoin('roles as r', 'r.role_id', 'u.role_id') | ||
.select('u.user_id', 'u.username', 'r.role_name') | ||
} | ||
|
||
function findBy(filter) { | ||
|
@@ -34,6 +37,10 @@ function findBy(filter) { | |
} | ||
] | ||
*/ | ||
return db('users as u') | ||
.leftJoin('roles as r', 'r.role_id', 'u.role_id') | ||
.select('u.*', 'r.*') | ||
.where(filter).first() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is only finding the first in the array |
||
} | ||
|
||
function findById(user_id) { | ||
|
@@ -47,6 +54,11 @@ function findById(user_id) { | |
"role_name": "instructor" | ||
} | ||
*/ | ||
return db('users as u') | ||
.leftJoin('roles as r', 'r.role_id', 'u.role_id') | ||
.select('u.user_id', 'username', 'r.role_name') | ||
.where({ user_id }) | ||
.first() | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
require('dotenv').config() | ||
|
||
const server = require('./api/server.js'); | ||
|
||
const PORT = process.env.PORT || 9000; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt match above white space