Make app mutation optional

[sripberger]

As discussed in #59, a reference to the Koa app is currently accepted so the middleware initializer can modify it for performance reasons. This makes sense, but it sort of couples the middleware to the app, making it tricky to use with other middleware consumers such as koa-router.

For example, if I want to use koa-router to define different behavior for different url paths-- some paths using sessions, others not. Even if I only attach the middleware for some paths, the ctx.session and ctx.sessionOpts getters will still exist on all context objects all paths, even though they shouldn't.

In fact, omitting the middleware doesn't even seem to remove much of the functionality at all. Without it, I could still get and commit session data from cookies as normal. Looking at the implementation, it seems like the middleware itself only really handles external storage initialization and the autoCommit option:

session/index.js

Lines 37 to 50 in 10bb122

	return async function session(ctx, next) {
	const sess = ctx[CONTEXT_SESSION];
	if (sess.store) await sess.initFromExternal();
	try {
	await next();
	} catch (err) {
	throw err;
	} finally {
	if (opts.autoCommit) {
	await sess.commit();
	}
	}
	};
	};

I suppose the answer here is to just not access those properties where they're not supposed to exist, but it feels messy and likely to confuse other developers who might work on my code who aren't familiar with the internals of koa-session.

I'd like to use koa-session since it is clearly the most popular and well-maintained module in this space, but at the moment this is a huge blocker for me. I'd be willing to take the performance hit in order to avoid these problems, though I'm aware not everyone feels that way.

Would you guys be open to the possibility of making the app argument optional in the initializer, falling back to the slower way of doing things if it is not provided? I imagine it will be a decent amount of work to make this change, but I'm happy to do it if you'd be receptive to it.

If not I'll just save my time and use koa-session2. :\

[jmitchell38488]

The app reference is required, for use in context.js::emit()

The emit class method is called from valid(), which validates a session as expired or valid.

session/lib/context.js

Lines 184 to 194 in c23bab4

	/**
	* @param {String} event event name
	* @param {Object} data event data
	* @api private
	*/
	emit(event, data) {
	setImmediate(() => {
	this.app.emit(`session:${event}`, data);
	});
	}

As for the storage handling, the storage is managed by the session object, primarily the Context. While storing session data in a cookie is optional (for so many reasons very bad), the preferred choice would be to store it in some sort of persistent storage, such as *SQL, Redis, NoSQL, etc. You set this by passing through a valid store object when you instantiate the session middleware.

Example

let session = new session({
    store: () => {
        _store = [];
        function get(key) {
            return _store.find(e => e.key === key);
        }
        ...
        return {get: get, set: set, destroy: destroy};
    },
    ...
});

I do agree that the app object shouldn't be deep referenced in the way that it is, just for using the emit function. The issue reporter is looking for functionality where the session isn't implicitly bound to the request, so that not every request has a session, or is a session, therefore setting unnecessary cookies.

[sripberger]

I appreciate the detailed response, of course, though I'd point out that the change which added the app reference to the middleware creation signature predates those emit calls by two years. As stated in the referenced PR #59, original reason was a performance optimization. My guess is that the events were emitted through that reference later as a sort of "might as well use this" kind of thing.

In Koa, though, the ctx object itself contains a reference to the app which could easily have been used for this purpose:

emit(event, data) { 
     setImmediate(() => { 
       this.ctx.app.emit(`session:${event}`, data); 
     }); 
   }

The problem with the performance optimization in #32 is that it makes a bunch of changes to the app object immediately upon the creation of the middleware, before any ctx objects exist, so it has to reference the app separately from those. Either that, or resort to some trickery that makes said changes lazily when the first request is received after startup.

This latter approach would not deal with the problem I am having, though, which is that I simply don't want the session getter to exist in the app.context object, which acts as the prototype for all ctx objects in the entire app. Not all parts of my app will be designed to use this cookie-based session data, and I want ctx.context to return undefined in those parts where it should not be used.

As for the storage thing, I agree that storing session data in cookies is generally bad, but for some context-- What I'm doing is re-writing some legacy REST endpoints in Node. The goal is to reproduce their functionality exactly -- so that we don't break our front ends which are unfortunately coupled to them rather extensively-- then add more a sensible graphql endpoint at a different path so we can re-write our front ends around that at a later date.

The legacy endpoints use an encrypted cookie to store a CSRF token in the user's browser. The browser never reads them-- only the sever does, so I suppose I could store them on the back end, but the front end assumes that they will never expire until the user closes their browser-- an event which the server can have no knowledge of whatsoever.

The reason it can't expire earlier is because the front end is a single-page app that gets the CSRF token from a global variable which is injected into a script tag when serving the page. The app itself never at any point requests new tokens. So if they have an expiration time, the app will break until the user refreshes the page when that expiration time passes.

So unfortunately it seems like we have to keep using the cookie for now.. So I'm aiming to reproduce it (but again, only for the legacy endpoints) with a signed cookie handled by this middleware (or another similar one).

Ideally, because this functionality is legacy and kind of gross to me, it will not leak into other parts of the app in any way, so that new front ends won't be sending and receiving this (for their purposes) completely unused cookie data, and so that middlewares and handlers in any new endpoints do not mistakenly read or write from ctx.session, which is not supposed to be a thing for said endpoints.

I hope that makes sense?

[sripberger]

Given this further discussion of the issue, I think it makes sense to rename this to make it a bit more clear.

[jmitchell38488]

It's also something that I've noticed, that there's no way to 'bind' the middleware to the app contextually, it's enforced. I also get your use case, and it makes sense, because I've used those cases before.

A few other libraries have their own prescribed idea of how to manage sessions, be it entirely at the whim of the developer when and where, or wrap the app like this one.

Good point on the pre-ctx object. I'll need to dig deeper into the koajs code to understand that a bit better.

[zavr-1]

@sripberger is 100% right the middleware should not be coupled to the app, it's really easy to change the emit logic to use ctx.app. Otherwise, it's confusing, how the session can be accessed via ctx.session on routes where session was not even installed. what are performance benefits anyway, the current logic defines session symbol on the context constructor but how much gain is there from that
it would be a breaking change though

session/test/cookie.test.js

Lines 668 to 695 in f81d713

	describe('when get session before enter session middleware', () => {
	it('should work', done => {
	const app = new Koa();
	app.keys = [ 'a', 'b' ];
	app.use(async function(ctx, next) {
	ctx.session.foo = 'hi';
	await next();
	});
	app.use(session({}, app));
	app.use(async function(ctx) {
	ctx.body = ctx.session;
	});
	request(app.callback())
	.get('/')
	.expect(200, (err, res) => {
	should.not.exist(err);
	const cookies = res.headers['set-cookie'].join(';');
	cookies.should.containEql('koa:sess=');
	request(app.callback())
	.get('/')
	.set('Cookie', cookies)
	.expect(200, done);
	});
	});
	});

[zavr-1]

@sripberger you can use this forke https://github.com/idiocc/session you're welcome

[sripberger]

@sripberger you can use this forke https://github.com/idiocc/session you're welcome

Thanks!

It turns out I'm no longer using this package at all, since I was able to convince my company to start with a brand new product-- complete with brand new clients-- rather than doing this lengthy and error-prone transition to keep support for the legacy clients. So there's no longer any need for session data cookies that are used in one part of the app, but not in another.

I appreciate that someone shares my thoughts about this situation, though. 👍