Nodejs async with express server

Question

I use the following code which works as expected. The code uses open ID connect to login user. Because I'm pretty new to node and express, it will be great if I can get some tips for the async usage- e.g. if I’m doing it and the error handling correctly.

The code is doing connect via oidc layer https://en.wikipedia.org/wiki/OpenID_Connect which does a user login according to client secret and client id, then the authorization server is calling to the redirect route of the application and if everything is OK the user logged in to the system.

This is the index.js

const express = require('express');
const logon = require('./logon');

const app = express();
const port = process.env.PORT || 4000;

logon(app)
  .then(() => {
    console.log('process started');
  });
app.use(express.json());

app.listen(port,
  () => console.log(`listening on port: ${port}`));

This is the logon.js

const { Issuer, Strategy } = require('openid-client');
const cookieParser = require('cookie-parser');
const cookieSession = require('cookie-session');
const azpi = require('./azpi');
const bodyParser = require('body-parser');
const passport = require('passport');


module.exports = async (app) => {

  let oSrv;
  const durl = `${process.env.srvurl}/.well-known/openid-configuration`;
  try {
    oSrv = await Issuer.discover(durl);
  } catch (err) {
    console.log('error occured', err);
    return;
  }

  app.get('/', prs(), passport.authenticate('oidc'));

  const oSrvCli = new oSrv.Client({
    client_id: process.env.ci,
    client_secret: process.env.cs,
    token_endpoint_auth_method: 'client_secret_basic',
  });

  passport.serializeUser((user, done) => {
    done(null, user);
  });
  passport.deserializeUser((obj, done) => {
    done(null, obj);
  });

  const cfg = {
    scope: 'openid',
    redirect_uri: process.env.ruri,
    response_type: 'code',
    response_mode: 'form_post',
  };

  const prs = () => (req, res, next) => {
    passport.use(
      'oidc',
      new Strategy({ oSrvCli , cfg }, (tokenset, done) => {
        const claims = tokenset.claims();
        const user = {
          name: claims.name,
          id: claims.sub,
          id_token: tokenset.id_token,
        };
        return done(null, user);
      }),
    );
    next();
  };
  app.use(
    bodyParser.urlencoded({
      extended: false,
    }),
  );
  app.use(cookieParser('csec'));
  app.use(
    cookieSession({
      name: 'zta-auth',
      secret: 'csect',
    }),
  );

  app.use(passport.initialize());
  app.use(passport.session());

  app.get('/redirect', async (req, res, next) => {
    passport.authenticate('oidc', async (err, user) => {
      if (err) {
        console.log(`Authentication failed: ${err}`);
        return next(err);
      }
      if (!user) {
        return res.send('no identity');
      }

      req.login(user, async (e) => {
        if (e) {
          console.log('not able to login', e);
          return next(e);
        }
        try {
          const url = await azpi.GetUsers(user.id_token);
          return res.redirect(url);
        } catch (er) {
          res.send(er.message);
        }
      });
    })(req, res, next);   
  });
};

Is my async code usage is okay?

Answer 1

Comments

While the code is mostly simple to follow, it would be good to add comments to document what decisions you made. Even if you are the only one maintaining this code your future self might not remember what considerations you had in the past. At least document the input, output and purpose of functions. Some style guides call for comments to be in certain formats - e.g. JSDoc in Google style guide.

Nesting Levels

The nesting levels gets a bit deep towards the end of logon.js- some might describe it a bit as “callback hell”. callbackhell.com has some good tips for keeping code shallow- like naming anonymous functions so they can be moved out (also may allow for use in unit and/or feature tests), modularize (which is already done somewhat with logon.js), etc.

Variable name format

It would be best to use consistent formatting of variable names. I have worked on a PHP codebase that makes quite extensive use of hungarian notation but I am not fond of it and I haven't seen it used very often with JavaScript code. I happened to recently see this Stack Overflow question about Hungarian notation and while it is marked as off-topic it does have many answers. The top voted answer contains a link to an article by Joel Spolsky, CEO and co-founder of Stack Overflow: Making Wrong Code Look Wrong. I know realize that much of the code throughout that codebase is “Systems Hungarian”.

It appears there are two variables named using Hungarian notation (i.e. oSrv and oSrvCli) while most all other variables are all lowercase or camelCase. There seems to be one other outlier: durl- which seems to be the discovery URL. One could name that as discoveryURL, or take the server URL/host name out of it and make it a real constant - e.g. const DISCOVERY_PATH = '/.well-known/openid-configuration'; , which can be appended to process.env.srvurl when used.

Async functions

As was mentioned in a comment, the async keyword has likely been applied to more functions that necessary. For example, only functions that contain the await keyword need to be declared as asynchronous- e.g. the function assigned to module.exports, the callback to req.login()).

Error handling

By default, if authentication fails, Passport will respond with a 401 Unauthorized status, and any additional route handlers will not be invoked.¹

passport.authenticate('oidc', async (err, user) => {
    if (err) {
        console.log(`Authentication failed: ${err}`);
        return next(err);
    }

This error handling seems fine. Would it be helpful to log the user object in the callback to the call to passport.authenticate()? If so, consider sanitizing any sensitive information that may be attached. You could also consider an error logging service.

It appears your code follows the Custom Callback option for passport.authenticate(). The sample under that section Custom Callback checks for err and when it is anything other than false-y it will return next(err)

If the built-in options are not sufficient for handling an authentication request, a custom callback can be provided to allow the application to handle success or failure.

app.get('/login', function(req, res, next) {
  passport.authenticate('local', function(err, user, info) {
    if (err) { return next(err); }

If authentication failed, user will be set to false. If an exception occurred, err will be set. An optional info argument will be passed, containing additional details provided by the strategy's verify callback.

It may be useful for you to read other reviews about nodeJS and passport as well as nodeJS and error-handling. The posts below might also be helpful:

• Express Passport (node.js) error handling
• How to show custom error messages using passport and express

Promisifying passport.authenticate()

This Stack overflow post might be interesting if you want to use await with the call to passport.authenticate():

I've been failing to get passport.authenticate to work at all inside of an async/await or promise pattern. Here is an example I feel should work, but it fails to execute passport.authenticate().

const passport = require("passport");
let user;
try {
   user = await __promisifiedPassportAuthentication();
   console.log("You'll never have this ", user);
} catch (err) {
   throw err;
}
function __promisifiedPassportAuthentication() {
    return new Promise((resolve, reject) => {
        console.log("I run");
        passport.authenticate('local', (err, user, info) => {
             if (err) reject(err);
             if (user) resolve(user);
         });
    });
}

The self-answer claims that the OP needed to invoke the function returned by the call to passport.authenticate() passing req and res, which likely need to get passed to __promisifiedPassportAuthentication().