2
\$\begingroup\$

I have been programming a middleware for my small application that verifies if an authentication header is present and verifies the HMAC present in it. I would like to know what I could optimize in this set of code. Any tips are appreciated.

const validator = require('validator'),
      crypto = require('crypto'),
      data = require('../database.js'),
      error = require('../utils/error.js')

const auth = function() {
  return function(req, res, next) {

    if(!req.authenticationHeaderExists) {
      res.json(error.INVALID_AUTH_HEADER)
      return
    }

    const authHeader = req.get('authentication').split(':')

    const user = authHeader[0]
    const userProvidedDigest = authHeader[1]

    if(!validator.isAlphanumeric(user)) {
      res.json(error.INVALID_USER)
      return
    }

    data.findOne({ user: user }, function processResults(err, docs) {
      if(err) {
        req.log.error(err)

        res.json(error.AN_ERROR_OCCURRED)
        return
      }

      if(docs == null) {
        res.json(error.USER_NOT_FOUND)
        return
      }

      const date = new Date()
      const formattedDate = date.getUTCFullYear().toString() + date.getUTCMonth().toString() + date.getUTCDate().toString() + date.getUTCHours().toString() + date.getUTCMinutes().toString()

      const serverGeneratedDigest = crypto.createHmac('sha256', docs.secretkey).update(formattedDate + req.method + req.url).digest('hex').toString('utf8')

      if(userProvidedDigest != serverGeneratedDigest) {
        res.json(error.INVALID_DIGEST)
        return
      }

      next()
    })
  }
}

module.exports = auth
\$\endgroup\$
7
  • \$\begingroup\$ Struggling to see how you could improve on this as the code looks pretty minimal. The obvious suggestion would be to introduce a layer of caching to reduce the no. of DB trips as I presume that would most likely be the biggest bottleneck. \$\endgroup\$ Commented Dec 17, 2017 at 0:18
  • \$\begingroup\$ Alright thank you for the suggestion @James, I wont be doing too much database queries, so I don´t think caching would be necessary. Either ways, thank you for the suggestion. \$\endgroup\$ Commented Dec 17, 2017 at 12:13
  • 1
    \$\begingroup\$ doesn't this middleware run on every request? \$\endgroup\$ Commented Dec 17, 2017 at 12:27
  • \$\begingroup\$ Oh actually you're right, I'm using NeDB for the database, to store the username and secret key. What would be the best way to do the caching? \$\endgroup\$ Commented Dec 17, 2017 at 12:48
  • \$\begingroup\$ I'm not familiar with NeDB however by looking at the docs it would appear to be already suitable for fast access being an embedded / in-memory DB! Think at this point I'd be questioning are you trying to micro-optimise or do you actually have a real problem? \$\endgroup\$ Commented Dec 18, 2017 at 11:13

1 Answer 1

Reset to default
-2
\$\begingroup\$
  • You could optimize the authHeader variables to check if it exists using lexical and then do the split.
  • You need to check if the authHeader splitted length is === 2.
  • You didn't have to create a const of function returning function and then export it. Instead, you could just export the inside function as the default export.
  • Use return next(); for the last callback function or the process will still continued even if you already called next();

  const validator = require('validator'),
  crypto = require('crypto'),
  data = require('../database.js'),
  error = require('../utils/error.js')


module.exports = function(req, res, next) {
  let authHeader = req.headers.authorization && req.headers.authorization.split(':')
  if(!authHeader || authHeader.length !== 2) {
    return res.json(error.INVALID_AUTH_HEADER)
  }

  const user = authHeader[0]
  const userProvidedDigest = authHeader[1]

  if(!validator.isAlphanumeric(user)) {
    return res.json(error.INVALID_USER)
  }

  data.findOne({ user: user }, function processResults(err, docs) {
    if(err) {
      req.log.error(err)

      return res.json(error.AN_ERROR_OCCURRED)
    }

    if(docs === null) {
      return res.json(error.USER_NOT_FOUND)
    }

    const date = new Date()
    const formattedDate = date.getUTCFullYear().toString() + date.getUTCMonth().toString() + date.getUTCDate().toString() + date.getUTCHours().toString() + date.getUTCMinutes().toString()

    const serverGeneratedDigest = crypto.createHmac('sha256', docs.secretkey).update(formattedDate + req.method + req.url).digest('hex').toString('utf8')

    if(userProvidedDigest !== serverGeneratedDigest) {
      return res.json(error.INVALID_DIGEST)
    }

    return next()
  })
}
\$\endgroup\$
6
  • \$\begingroup\$ The question is how can performance be improved, not one of those suggestions address that problem. \$\endgroup\$ Commented Dec 18, 2017 at 11:09
  • \$\begingroup\$ I provide the answer how to improve the performance, what do you think this is for? \$\endgroup\$ Commented Dec 21, 2017 at 8:22
  • \$\begingroup\$ @Ihan read my previous comment - none of these suggestions will make one iota of difference to performance. \$\endgroup\$ Commented Dec 21, 2017 at 8:42
  • \$\begingroup\$ @James The question does not explicitly say "optimise performance". Also see codereview.meta.stackexchange.com/q/1884/31562 . Generally, answers are allowed to provide feedback to any aspects of the code. \$\endgroup\$ Commented Dec 21, 2017 at 11:17
  • \$\begingroup\$ @SimonForsberg based on the discussions via the comments it was my interpretation that's what the OP meant. Regardless, the improvements suggested here, IMO, won't make any sort of significant "optimisation" to the code therefore it would be making changes for the sake of it. \$\endgroup\$ Commented Dec 21, 2017 at 14:39

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.