code review start #142
code review start #142
Conversation
| return next({ status: 401, message: 'Token required'}) | ||
| } | ||
| jwt.verify(token, JWT_SECRET, (err, decode)=>{ | ||
| if(err){ |
There was a problem hiding this comment.
doesnt match above white space
| /* | ||
| 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.
dbUsername actually is a collection == dbUsers
returns an array
| @@ -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.
take out the comments as normal --- delete it!
| @@ -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() | |||
| if( !role_name || role_name.trim() === undefined ){ | |||
There was a problem hiding this comment.
after || will never be called
| 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.
return is not necessary if you have "else/else if" right after...
change one or the other
| @@ -1,8 +1,12 @@ | |||
| const router = require("express").Router(); | |||
There was a problem hiding this comment.
be consistent with semicolons !!
| 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.
will the comment be a lifesaver here??
if yes, leave in
if not, take out
| 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.
keep lines under 100 at least ((maybe 80 if possible))
| @@ -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 | |||
| 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.
this is only finding the first in the array
| "knex": "^0.95.14", | ||
| "sqlite3": "^5.0.2" | ||
| "jsonwebtoken": "^8.5.1", | ||
| "knex": "^1.0.1" |
There was a problem hiding this comment.
adding risk by doing this -- might be increasing the possibility of errs
only if you need to use the new version
No description provided.