Performance and login security for NodeJS using Express with Express-sessions

Question

I am new to this part of backend development using JS and the truth being a new programming language (for me) it is worrying that it suddenly does not meet the structuring and / or security standards.

CLEAN ARCHITECTURE

API / CONTROLLERS / SERVICES / LIBS

ROUTE

//Required application
const express = require('express');
//Required services
const LoginServive = require('../../services/auth/login');

// Schemas
const {
  authenticationIdSchema,
  authenticationSchema
} = require('../../utils/schemas/auth/login');

// Middleware
const verifyToken = require('../../utils/middleware/verifyTokenHandler.js');
const validationHandler = require('../../utils/middleware/validationHandler')

function loginApi(app)
{
  const router = express.Router();
  app.use("/api/v1/login", router);

  //Class call
  const loginService = new LoginServive();

  /*
  ******************************************************************************
  ******************************************************************************
  ******************************    POST METHOD   ******************************
  ******************************************************************************
  ******************************************************************************
  */
  router.post('/',
         validationHandler(authenticationSchema),
         async function(req, res, next) {
    const { body:login } = req;

    try
    {
      const response = await loginService.login({ login });
      const data     = response[0];
      req.session.user = {
        _id:response[1].id,
        _username: response[1].username,
        _name: response[1].name,
        _role: response[1].role
      };
      console.log("Session: ", req.session.user);

      res.status(201).json(data);
    } catch (err) {
      next(err);
    }
  });

}

module.exports = loginApi;

SERVICE

const pool = require('../../lib/db');
const bcrypt = require('bcrypt');
const jwt = require('jsonwebtoken');
const { config } = require('../../config');

class LoginService
{
  async login({ login })
  {
    //Parameters
    const { username, password } = login;
    //Api Info
    const SECRET_API  = config.secretApi;

    if (username && password)
    {
      const user_query = `SELECT usr.id,
                                 usr.username,
                                 usr.email,
                                 usr.hash,
                                 CONCAT(pple.name, ' ', pple.lastname) AS full_username,
                                 usr.user_status_id,
                                 usr.user_type_id
                          FROM users usr
                          INNER JOIN people pple
                          ON pple.id = usr.people_id
                          INNER JOIN user_states usts
                          ON usts.id = usr.user_status_id
                          INNER JOIN user_types utps
                          ON utps.id = usr.user_type_id
                          WHERE usr.data_status_id = '1'
                            AND usr.email = ?`;

      const rows = await pool.query(user_query, [username]);

      if (rows.length > 0) {
        const { id, username, email, hash, full_username, user_status_id, user_type_id } = rows[0];

        // Compare hash with the plain text password
        const match = await bcrypt.compare(password, hash);

        if (match) {
          //Get the access token
          const token = jwt.sign({ _id: id }, SECRET_API, {
            expiresIn: 60 * 60 * 8
          });

          const response = [
            {
              status: 201,
              message:'You are welcome.',
              role: user_type_id,
              token:token
            },
            {
              id:id,
              username: username,
              name: full_username,
              role: user_type_id
            }
          ];

          const authenticationId = await Promise.resolve(response);
          return authenticationId || [];

        } else {
          const authenticationId = {
            status: 401,
            message:'Wrong password.'
          };

          return authenticationId || [];
        }

      } else {
        const authenticationId = {
          status: 401,
          message:'The user does not exist.'
        };

        return authenticationId || [];
      }
    }
  };
};

module.exports = LoginService;

MySQL SESSION STORE

For the session data store I am implementing express-mysql-session this as reflected in the direct documentation of express-session.

Warning The default server-side session storage, MemoryStore, is purposely not designed for a production environment. It will leak memory under most conditions, does not scale past a single process, and is meant for debugging and developing.

QUESTIONS

I am trying to implement the Clean Architecture development model and set aside the MVC model, looking for maximum performance in this development part.

1. Is my code exposed to some kind of SQL injection or other type of security flaw?
2. The user session is an important issue (due to security and performance issues). It is a good practice for what I am doing, since I expected to do it directly from the service but I had problems accessing the request req.
3. Is any part of the code improvable?
4. Should I have the session data assigned in the token { _id: id } or implement express-session?

Answer 1

Is my code exposed to some kind of SQL injection

You're using parameterized queries, as you should be, so it shouldn't be an issue. (though, I don't see where you're using the [username] in the query?)

Is any part of the code improvable?

Some of it. IMO, in Javascript, classes are generally useful when you want to bundle together instance data with operations on that data. If you don't have any instance data, using a class is potentially confusing and doesn't accomplish anything over using an ordinary function instead. Consider changing the service to:

module.exports = async function loginService({ login }) {
  //Parameters
  const { username, password } = login;
  // etc

const loginService = require('../../services/auth/login');

(make sure the spelling is correct, continuing to use LoginServive could cause bugs later)

If you do decide to keep using classes, make sure not to put semicolons at the end of methods or the class definitions - they do not belong there. Maybe consider a linter.

Does your validationHandler ensure that requests contain a username and password? If so, then in the login function, there's no need to test it like you do below:

const { username, password } = login;
if (username && password) {

If the validator doesn't ensure that requests contain those properties, then you have a bug: if a request is made without those properties, the current login function will not return anything, which means that in the route:

const response = await loginService.login({ login });
const data     = response[0];

an error will be thrown, because response is undefined. Maybe only call loginService if credentials are provided - otherwise, call next() to go to the next route.

You have a few cases when you define an object and then test its truthyness, eg:

const authenticationId = {
  status: 401,
  message:'Wrong password.'
};
return authenticationId || [];

This is superfluous, because objects are always truthy; the object you just created will always be returned. It'll never return an empty array instead.

But this points to another problem. You're sometimes returning an object, and you're sometimes returning an array. This makes the shape of the return value more difficult to parse, understand, and test. Consider consistently returning an object which is always the same shape, and if you want to examine it, check possible different properties.

The above points to yet another problem. In the route, the only thing you do with the response is try to use its 0 and 1 properties. If it returns a { status: 401, message:'Wrong password.' } object instead, your script will throw an error when trying to access response[1].id, because response[1] doesn't exist. Instead, use the consistent shape of the object to check the return value from login and figure out what to call res.status with. (See below for full code)

response is a potentially confusing variable name in a route for a variable which is not the res parameter. Maybe call it loginResult instead.

Rather than a bunch of nested conditions:

  if (...) {
    if (...) {
      if (...) {
        return authenticationId || [];
      }
    }
  }
};

The logic would probably be easier to follow if, when a condition is not met, handle the problem immediately and return. This way, readers of the code don't have to keep in their head, for every block: "This block is entered if X condition is reached, so when the block ends, need to remember to handle when X condition isn't reached."

Regarding

const response = [
  // ...
];
const authenticationId = await Promise.resolve(response);

There's no need to do await Promise.resolve on something - await will unwrap a Promise, and can be used on non-Promises. But since response is an array literal, nothing asynchronous is going on, so there's no reason to await there.

Rather than having login format the row to be returned to the route, and for the route to reformat it again to set to req.session.user, consider taking the row and formatting it as needed for the session just once, reducing superfluous object reorganization.

In full:

// route
router.post(
  '/',
  validationHandler(authenticationSchema),
  async (req, res, next) => {
    const { body } = req;
    if (!body.username && !body.password) {
      next();
      return;
    }
    try {
      const loginResult = await loginService.login(body);
      if (loginResult.errorMessage) {
        res.status(401).json({
          message: loginResult.errorMessage
        });
        return;
      }
      // Login succeeded:
      const { userSession, responseData } = loginResult;
      req.session.user = userSession;
      console.log("Session: ", req.session.user);
      res.status(201).json(responseData);
    } catch (err) {
      // Are you sure you want to go to the next route here?
      // If there's an error, maybe respond with status 500 instead
      next(err);
    }
  }
);

// service
// You can put the constants up here so they don't mix with the main login logic
const SECRET_API  = config.secretApi;
const userQuery = `SELECT usr.id,
                           usr.username,
                           usr.email,
                           usr.hash,
                           CONCAT(pple.name, ' ', pple.lastname) AS full_username,
                           usr.user_status_id,
                           usr.user_type_id
                    FROM users usr
                    INNER JOIN people pple
                    ON pple.id = usr.people_id
                    INNER JOIN user_states usts
                    ON usts.id = usr.user_status_id
                    INNER JOIN user_types utps
                    ON utps.id = usr.user_type_id
                    WHERE usr.data_status_id = '1'
                      AND usr.email = ?`;
module.exports = async ({ username, password }) => {
  const rows = await pool.query(userQuery, [username]);
  if (rows.length === 0) {
    return { errorMessage: 'The user does not exist.' };
  }
  const { id, email, hash, full_username, user_type_id } = rows[0];
  // Compare hash with the plain text password
  const match = await bcrypt.compare(password, hash);
  if (!match) {
    return { errorMessage: 'Wrong password.' };
  }
  //Get the access token
  const token = jwt.sign({ _id: id }, SECRET_API, {
    expiresIn: 60 * 60 * 8
  });
  return {
    responseData: {
      status: 201,
      message:'You are welcome.',
      role: user_type_id,
      token
    },
    userSession: {
      _id: id,
      _username: username,
      _name: full_username,
      _role: user_type_id
    }
  };
};