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\$
  • \$\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\$ – James Dec 17 '17 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\$ – TomasLife Dec 17 '17 at 12:13
  • 1
    \$\begingroup\$ doesn't this middleware run on every request? \$\endgroup\$ – James Dec 17 '17 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\$ – TomasLife Dec 17 '17 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\$ – James Dec 18 '17 at 11:13
-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\$
  • \$\begingroup\$ The question is how can performance be improved, not one of those suggestions address that problem. \$\endgroup\$ – James Dec 18 '17 at 11:09
  • \$\begingroup\$ I provide the answer how to improve the performance, what do you think this is for? \$\endgroup\$ – Ihsan Dec 21 '17 at 8:22
  • \$\begingroup\$ @Ihan read my previous comment - none of these suggestions will make one iota of difference to performance. \$\endgroup\$ – James Dec 21 '17 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\$ – Simon Forsberg Dec 21 '17 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\$ – James Dec 21 '17 at 14:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.