Session generator/checker utility for a server

Question

I wrote a utility for generating and saving sessions in a MongoDB database as well as checking the session to make sure it's valid.

I'm a beginner when it comes to promises and I have a feeling that either I'm using them improperly or I'm not taking full advantage of them.

Here's the session utility:

// Dependencies
var crypto = require('crypto'),
    config = require('../../config'),
    Session = require('../database/schemas/session');

// Generate Function
var generate = function(length) {
  // Return a promise
  return new Promise(function(resolve, reject) {
    // Create a random value
    crypto.randomBytes(length, function(error, buffer) {
      // Determine if an error occurred
      if (error) {
        // Reject with error
        reject(error);
      } else {
        // Resolve with session ID
        resolve(buffer.toString('hex'));
      }
    });
  });
}

// Create Function
exports.create = function(account_id) {
  // Return a promise
  return new Promise(function(resolve, reject) {
    // Generate the session ID and fetch the promise
    generate(config.session.length)
      .then(function(sid) {
        // Save the session in the database and return the promise
        return(new Session({_id: sid, account_id: account_id}).save());
      })
      .then(function(session) {
        // Determine if session is not defined
        if (!session) {
          // Reject
          reject();
        } else {
          // Resolve with the session ID
          resolve(session._id);
        }
      })
      .catch(reject);
  });
}

// Check Function
exports.check = function(sid) {
  // Return a promise
  return new Promise(function(resolve, reject) {
    // Query for a session with the provided ID and fetch the promise
    Session.findOne({_id: sid}).lean().exec()
      .then(function(session) {
        // Determine if no session was found or if it expired
        if (!session) {
          // Resolve with false
          resolve(false);
        } else if (session.expires < Date.now()) {
          // Resolve with false
          resolve(false);
        } else {
          // Resolve with true by default
          resolve(true);
        }
      })
      .catch(reject);
  });
}

Here's the session schema:

// Dependencies
var mongoose = require('mongoose'),
    config = require('../../../config');

// Session Schema
var sessionSchema = mongoose.Schema({
  _id: {type: String},
  account_id: {type: mongoose.Schema.Types.ObjectId, required: true},
  expires: {type: Date, required: true, default: Date.now() + config.session.expires}
});

// Export the model of the schema
module.exports = mongoose.model('session', sessionSchema);

The looks pretty good to me. You may want to get rid of the comments (which don't add any value, and only clutter your code). Also, IMO, having distinct branches in check method is not very readable too. How about this if (!session || session.expires < Date.now()) { resolve(false); } ... ? — Igor Soloydenko
– Igor Soloydenko, Commented Oct 9, 2017 at 16:56
@IgorSoloydenko Yeah, I do need to work on my commenting skills. I agree with you on the if statements as well. Thanks for the feedback. — Acontz
– Acontz, Commented Oct 9, 2017 at 17:41

Gerrit0 · Accepted Answer · 2017-10-12 16:24:36Z

For the most part this looks pretty good, there are only a few things which I would recommend changing.

As Igor Soloydenko recommends, get rid of the comments which do nothing but restate the code. Though comments are certainly not a bad thing, they should instead be used to explain why some code is the way it is if another programmer in 6 months (you) looks at it and expects something different.
Don't wrap promises in new Promise(). The bluebird wiki has a good explanation of this. It essentially boils down to "if a function returns a promise, return that directly". Here's an example with your code. I have removed the check for a null session as save() will reject when there is an error and it is not necessary to check for it yourself.
```
exports.create = function(account_id) {
  return generate(config.session.length)
    .then(function(sid) {
      return new Session({_id: sid, account_id: account_id}).save();
    })
    // If all a method does is return, I prefer arrow functions
    .then(session => session._id);
}
```

If you are using Node v7.6.0 or newer, you can take advantage of async / await to further simplify the use of promises.

exports.create = async function(account_id) {
  let sid = await generate(config.session.length)
  let session = await new Session({_id: sid, account_id: account_id}).save();
  return session._id;
}

If you are using Node v8.0.0 or newer, you can use the built in promisify method to avoid wrapping callback based methods yourself.

const { promisify } = require('util');
var generate = function(length) {
  // If this is being called a lot, the promisify call should be saved
  return promisify(crypto.randomBytes)(length)
    .then(buffer => buffer.toString('hex'));
}

The check function can be simplified, there are several ways this could be written.

exports.check = function(sid) {
  return Session.findOne({_id: sid}).lean().exec()
    .then(function(session) {
      // Igor Soloydenko's suggestion
      if (!session || session.expires < Date.now()) {
        return false
      }
      return true;
    });
}

// This works better if there are many conditions which must be true
// Igor Soloydenko's check is probably better here.
exports.check = function(sid) {
  return Session.findOne({_id: sid}).lean().exec()
    .then(session => [
      session,
      session.expires > Date.now(),
    ].every(Boolean));
}

Stack Exchange Network

Session generator/checker utility for a server

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Session generator/checker utility for a server

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions