User authentication, management

Question

I have recently started studying JavaScript, nodeJS, and reactJS on the front-end, and I have some questions about best practices to apply in this JavaScript world. I would like to ask for opinions regarding how I could improve my code.

Starting with an example: I have 2 functions, with different ways of handling promises

Here I have a route to add a user:

 addUsers(req,res,next) {
    try {
        const {name,email,login} = req.body;
        User.existLogin(req.body.login)
        .then(async result => {
            if(!result){
                const password = await bcrypt.hashSync(req.body.password, 10);
                User.create({ 
                    name, email, login, password }).then(result => {
                    res.status(201).json({Results: result.dataValues})
                })
            }else{
                return res.status(409).json({message: 'Login already exists'}); 
            }
        })
    } catch (error) {
        res.status(500).json({error: error})
    }
}

And here I have another function to login:

async login(req,res){
    const  user  = await User.existLogin(req.body.login);
    if (!user) { return res.status(400).json({result: 'Login is wrong '});} 
    const isPassword = await User.isPassword(user.dataValues.password, req.body.password);
    if (!isPassword) { return res.status(400).json({result: 'Password is wrong '}); } 
    const jwt = auth.signjwt(auth.payload(user));
    console.log(jwt);
    res.status(200);
    res.json(jwt);
}

Looking at this the login function seems to me to be cleaner, but I have doubts if really yours I could improve in one of two (I follow different logics to do both).

I have an auth folder, where I export functions, such as my payload, my sign, my middlware to validate my jwt, but I don't know if this is a correct decision.

const jwt = require('jsonwebtoken');
const User = require('../models/User')
const config= require('../config/dbconfig');
const moment = require('moment');

module.exports = {
    signjwt (payload) {
        return jwt.sign(payload, 
            config.secretToken
                   )
    },
    payload (usuario) {
        return {
            sub: usuario.id,
            name: usuario.nome,
            email: usuario.email,
            login: usuario.username,
            admin: true,
            iat: Math.floor(moment.now()/1000), // Timestamp de hoje
            exp: moment().add(10, 'minutes').unix() // Validade de 2 dias
        }
    },
    async auth(req,res,next){
        const token = req.header('Authorization');
        console.log(token);
        if(!token) return res.status(401).json('Unauthorized');
        try{
            const decoded = jwt.verify(token,config.secretToken);
            const user = await User.findByPk(decoded.sub);
            console.log(user);
            if(!user){
                return res.status(404).json('User not Found');
            }
            res.json(user);
            next();
        }catch(error){
            console.error(error);
            res.status(400).json('Invalid Token');
        }
    }
}

Answer 1

Initial thoughts:

This code looks very ... compressed without strictly needing to. Another thing that I notice is the level of nesting that shows in here.

Generally speaking it doesn't particularly make a difference how you decide to handle Promises, in the vast majority of cases it's just a matter of personal preference.

One thing you do not need to worry about is specially handling a promise rejection, since express automatically converts thrown errors into an appropriate response. That is what login takes advantage of to look so clean.

One final initial thing I notice is the inconsistency that shows in the code.

Inconsistencies:

Compare the use of spaces in the following example:

const  user   = await User.existLogin(req.body.login);
const isPassword = await User.isPassword(user.dataValues.password, req.body.password);

Or compare the use of braces as well as whitespace for these examples:

if(!token) return res.status(401).json('Unauthorized');
if (!user) { return res.status(400).json({result: 'Login is wrong '});}
if(!user){
    return res.status(404).json('User not Found');
}

Compare also the use of newlines and indentation levels in the following examples:

User.create({
    name, email, login, password }).then(result => {
    res.status(201).json({Results: result.dataValues})
})
return jwt.sign(payload,
    config.secretToken
           )
return {
    sub: usuario.id,
    // ....
    exp: moment().add(10, 'minutes').unix()
}

All this is to say: Pick one style and stick to it. Whichever style you decide on, it makes it easier to read if the style is consistent.

Personally I prefer a style with more space than you've shown here for most cases.

Indentation:

You're using a strange mix of indentation styles and you're making lots of use of early returns, except in some small cases where I don't understand it. I personally prefer to almost always return early inside the request handler and if there's more complex logic I'd consider extracting that into a separate function. That yields code which will look a lot like this:

function addUsers(req, res) {
    const name = req.body.name;
    const email = req.body.email;
    const login = req.body.login;

    const exists = await User.existLogin(login);
    if (exists) {
        return res.status(409).json({ message: 'Login already exists' });
    }

    const password = bcrypt.hashSync(req.body.password, 10);
    const created = User.create({ name, email, login, password });
    res.status(201).json({ results: created.dataValue });
}

Note that I removed the use of await with hashSync (should be clear why). Also note that I lower-cased the member results in the returned JSON result.

Obviously this doesn't quite have the same error behaviour as the code presented in the question, but it nicely shows how to avoid deeply nesting blocks.

HTTP / web standard authorization patterns

As a minor point I'd like to add that the auth middleware shown here is not quite following the common header authorization pattern. It's missing a response header indicating the expected authorization type. Since you're working with a token here, usually the header would be 'WWW-Authenticate: "Bearer"', but that would imply the expected Authorization header should start with "Bearer", which is not the case here.

To be more ... standard-conform I'd expect a proper bearer-token Authentication scheme to specifically define the authorization type used in the scheme. An option could be to explicitly specify you expect a jwt-token as type resulting in the following headers:

'WWW-Authenticate: "jwt-token"'
---
'Authorization: "jwt-token <jwt_token_goes_here>"'

Note that this suggestion of course would require an adjustment to the way the token is obtained, adding another way the request can be malformed :)

Conclusion:

All in all this is a solid start, to get this code to the next level just requires a few tweaks, mostly around being consistent in how the code is structured in itself.

Well done :)