Skip to content
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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
50 changes: 48 additions & 2 deletions api/auth/auth-middleware.js
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:
Expand All @@ -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){
Copy link
Author

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

return next({ status: 401, message: 'Token invalid'})
}
req.decodedJWT = decode
next()
})
}

const only = role_name => (req, res, next) => {
Expand All @@ -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 })
Copy link
Author

Choose a reason for hiding this comment

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

dbUsername actually is a collection == dbUsers

returns an array

Copy link
Author

Choose a reason for hiding this comment

The 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)
}
}


Expand All @@ -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()
Copy link
Author

Choose a reason for hiding this comment

The 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 ){
Copy link
Author

Choose a reason for hiding this comment

The 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'})
Copy link
Author

Choose a reason for hiding this comment

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

return is not necessary if you have "else/else if" right after...
change one or the other

} 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 = {
Expand Down
32 changes: 30 additions & 2 deletions api/auth/auth-router.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
const router = require("express").Router();
Copy link
Author

Choose a reason for hiding this comment

The 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!
Copy link
Author

Choose a reason for hiding this comment

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

will the comment be a lifesaver here??

if yes, leave in
if not, take out

Copy link
Author

Choose a reason for hiding this comment

The 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" }

Expand All @@ -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 })
Copy link
Author

Choose a reason for hiding this comment

The 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)
}
});


Expand All @@ -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
Copy link
Author

Choose a reason for hiding this comment

The 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;
18 changes: 18 additions & 0 deletions api/auth/helpers.js
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
5 changes: 4 additions & 1 deletion api/secrets/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,8 @@
developers cloning this repo won't be able to run the project as is.
*/
module.exports = {

BCRYPT_ROUNDS: process.env.BCRYPT_ROUNDS || 8,
NODE_ENV: process.env.NODE_ENV || 'development',
PORT: process.env.PORT || 9000,
JWT_SECRET: process.env.JWT_SECRET || 'keep it secret!',
}
12 changes: 12 additions & 0 deletions api/users/users-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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()
Copy link
Author

Choose a reason for hiding this comment

The 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) {
Expand All @@ -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()
}

/**
Expand Down
Binary file modified data/auth.db3
Binary file not shown.
2 changes: 2 additions & 0 deletions index.js
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;
Expand Down
Loading