Use 'req.sessionStore' instead of directly using 'store' object.

[sumeetkakkar]

Description

Use req.sessionStore instead of directly using store object. This gives flexibility to inject additional parameters like req to store.get and store.destroy via custom traps using JavaScript Proxy without changing the interface of the Store.

Motivation and Context

We want to pass req object down to the methods of our Store implementation. store.set method can access it from session.req but it is not available in store.get and store.destroy methods.

One workaround we found is to leverage setter of req.sessionStore to create JavaScript Proxy of the store object. This proxy has traps for store.get and store.set and it adds the req object as a parameter to the function call.

To make this work we need changes in this module to use req.sessionStore.get and req.sessionStore.destroy methods so that the proxy object is used.

[knoxcard]

Hmmm, I like where you're going with this and I have a solution as well.

Memory/CPU
My concern is structure, doesn't everyone reference the same store object?
So with this pull request, we are consuming unnecessary memory/cpu resources upon every session instantiation? even thought it may be minimal.

Security?
Thought client can access req headers? When you pass req.sessionStore, it does not include data from other users, no potential for session hijacking?

[knoxcard]

First, notice how the global variable app is utilized to "carry" objects throughout the entire codebase.
These objects can now be accessed anywhere within Express, socket.io, models, controllers, requires files, etc., as long as we pass the global express variable app to them.

var express = require('express'),
  app = express()
app.set('server', require('http').createServer(app))
app.set('colors', require('chalk'))
app.set('functions', require('./lib/functions')(app))
app.set('json spaces', 4)
app.disable('x-powered-by')
app.enable('trust proxy')
app.enable('view cache')
app.engine('hbs', require('express-handlebars').create({
  helpers: require('./lib/helpers').helpers,
  extname: '.hbs',
  layoutsDir: './views/layouts'
}).engine)
app.set('view engine', 'hbs')
console.log((process.env.protocol === 'https' ? app.get('colors').bold.green.bgWhite('PRODUCTION') : app.get('colors').bold.black.bgYellow('DEVELOPMENT'))) 
app.use((req, res, next) => {
  res.locals.nonce = require('uuid/v4')
  next()
})

Next app.get('session') holds the reference to express_session. When calling express_session, simply return the parameters inside of express_session(parameters) -> session.

Now we can reference app.get('session'), for example....
app.get('session').store.destroy(session, function() { }).

var express_session = require('express-session'),
  redis_store = new (require('connect-redis')(express_session))()
var session = express_session({
  store: redis_store,
  secret: process.env.session_secret,
  name: process.env.session_name,
  rolling: true,
  saveUninitialized: true,
  unset: 'destroy',
  resave: true,
  proxy: true,
  logErrors: false,
  cookie: {
    path: '/',
    domain: '.' + process.env.app_domain,
    httpOnly: true,
    secure: process.env.protocol === 'https',
    maxAge: (60 * 60 * 1000) // 60 mins
  }
})
app.use(session)
app.set('session', session)
app.use(require('./middleware')(app))

  app.get('session').rolling // returns true
  app.get('session').resave  // returns true
  app.get('session').unset  // returns 'destroy'
  app.get('session').cookie.path // returns '/'
  app.get('session').cookie.maxAge // returns 3600000
  ...

  We can also reference the store and all of it's properties and methods
  app.get('session').store.destroy(function() {
  })

[sumeetkakkar]

Thanks for looking into this.
The idea behind this PR is to be able to trace the logging for a req. Especially to know why error happened while processing the request and one source of the error is the sessionStore. Right now we are using https://www.npmjs.com/package/continuation-local-storage and we want to move away from it.

A Store implementation which can enable us to do this is:

const Store = app.Store || app.session.Store;
class SomeStore extends Store {
  get(id, req, callback) {
    if (typeof req === 'function') {
      callback = req;
      req = void 0;
    }
    ....
    if (req) req.log(...);
    ....
  }
  set(id, session, callback) {
    ....
    if (session.req) session.req.log(...);
    ....
  }
  destroy(id, req, callback) {
    if (typeof req === 'function') {
      callback = req;
      req = void 0;
    }
    ....
    if (req) req.log(...);
    ....
  }
  ...
}

Please note that this is a modified interface and changing store.get(req.sessionID, function(err, sess){ (https://github.com/expressjs/session/blob/master/index.js#L474) will pretty much break everybody. The workaround we are exploring is to inject sessionStore getter and setter in req object and leverage req.sessionStore = store; (https://github.com/expressjs/session/blob/master/index.js#L214) in the express-session middleware function.

Object.defineProperty(req, 'sessionStore', { 
    get: function() { return this[kSessionStore]; },
    set: function(store) { 
        return this[kSessionStore]=constructJSProxy(store, this);
    } 
});

req.session.save and req.session.destroy are already using this.req.sessionStore.

The JS Proxy looks like following:

   const constructJSProxy = function constructSessionStoreProxy(store, req) {
        const handler = {
            get: function(target, prop, receiver) {
                const origMethod = target[prop];
                if (typeof origMethod !== 'function') {
                    return origMethod;
                }
                switch (origMethod) {
                    case SomeStore.prototype.get:
                    case SomeStore.prototype.destroy:
                        return function (id, callback) {
                            return origMethod.call(target, id, req, callback);
                        };
                    default:
                        return origMethod;
                }
            }
        };
        return new Proxy(store, handler);
    };

[sumeetkakkar]

@knoxcard, Please let me known if there are any specific concerns you want me to address to help merge this PR.

As I have stated, our goal is to be able to access req object in the Store's get and destroy methods. Any alternate recommendation to achieve this is also appreciated.

[knoxcard]

@sumeetkakkar - I am not a project contributor or owner to this repo. Your best bet is having @dougwilson review this.

[dougwilson]

Can you look at #712 and confirm if that will accomplish the same thing as here? It is further along and seems much more flexible, including keeping our internals internal and less at risk for user overrides causing hard to debug issues.

[sumeetkakkar]

Can you look at #712 and confirm if that will accomplish the same thing as here? It is further along and seems much more flexible, including keeping our internals internal and less at risk for user overrides causing hard to debug issues.

👍

[sumeetkakkar]

@dougwilson , Can you please share ETA for #712?

[dougwilson]

We are working though everything, just there is a large backlog. Part of resolving that is resolving the difference between these two PRs, which is why I messaged on here.

Were you able to take a look at the changes in #712 ? Do you believe they will accomplish the same thing has this pull request?

[sumeetkakkar]

Our goal is to be able access req object in store.get and store.destroy. It seems #712 will make it possible. So yes this PR can be closed in favor of #712.

Thank you!