Isolating each session per request instead of globally

[jcs224]

Right now, a single instance of the Session class is used globally per session. In my testing, this hasn't been an issue, but it should probably be addressed to prevent even the possibility of issues.

I want to avoid breaking changes to end-user code if possible. If it's not possible, we'll make it a new major version indicating breaking changes.

I have an idea of how to fix this using JavaScript Map data structure to put each session in a temporary store just for the lifecycle of the request. Then, instead of using ctx.state.session.get() to pull the request's session from the global instance, it will pull it from a Map entry. If implemented properly, it should avoid breaking user code but make the library more robust and secure.

[c0per]

Yes, I created PR #23, it basically is what you said.

One Session instance now represents only one session data for the lifecycle of one request. For every incoming request, one Session is instantiated in app.use(Session.initMiddleware()) and put to ctx.state.session. User can still get, set, flash, deleteSession on it, this part's not changed.

Another Change is the CookieStore class. It now represents a "store" to all the session data just like other stores although it doesn't store anything. It just get from ctx. It doesn't implement Store anymore as it must accept a ctx to work. I considered adding a Map to store <sessionId, ctx> pairs in CookieStore but the main problem is that session data need to be obtained in initMiddleware() before being validating or recreating. I think force CookieStore to implement Store will make the code in initMiddleware() more messy than now it is (checking if we're working with CookieStore every time needed).

[jcs224]

Cool, thanks!

I also created my own solution, in local testing. Some things similar to your PR but the approach is different. Similar to your solution, context, sessionID, and sessionCache have been removed as class properties, and I also got rid of the session cache, which was a clever trick to reduce unnecessary calls to the storage engine, but ended up making the code more complicated and likely a premature optimization anyway.

What I did instead was make ctx.state.session an object instead of a class instance that contains the methods, so it also doesn't change the end-user's way of getting/setting variables. It looks like this:

ctx.state.session = {
  async get(key : string) {
    return await self.getSessionValue(sid, key)
  },

  async set(key : string, value : string) {
    await self.setSessionValue(sid, key, value)
  },

  async flash(key : string, value : string) {
    await self.flashSessionValue(sid, key, value)
  },

  async deleteSession() {
    await self.deleteSession(sid)
    ctx.cookies.delete('session', this.cookieSetOptions)
  }
}

TL;DR, The class is still used for everything it was before, with the exception no longer storing session data in a class instance variable. Instead, session data is kept within the initMiddleware function and get, set, flash, and has are created dynamically per request. This is to prevent global conflicts while maintaining the same end-user API.

The main hesitation I have with your PR is that it is a breaking API change. For example, I like to use the created session instance across multiple files. Here is a real example of me creating a session in a file:

// session.ts
import { Session, PostgresStore } from "./deps.ts"
import 'https://deno.land/x/[email protected]/load.ts'
import sql from './db.ts'

const store = new PostgresStore(sql)
await store.initSessionsTable()

const session = new Session(store)

export default session

Then using it like so:

// routes/auth.ts
import session from '../session.ts'
import { Router } from '../deps.ts'

const authRoutes = new Router()
  .use(
    session.initMiddleware(),
  )
// ...

With your approach, I'll have to re-attach the store in each middleware file:

// store.ts
import { PostgresStore } from "./deps.ts"
import sql from './db.ts'

const store = new PostgresStore(sql)

export default store

// routes/auth.ts
import { Session, Router } from '../deps.ts'
import store from './store.ts'

Session.store = store

const authRoutes = new Router()
  .use(
    Session.initMiddleware(),
  )

It's a little less consolidated.

What I'll probably do is evaluate both branches, and report back here. I have a small app using sessions, I will probably test both approaches with it.

P.S. I like your code and the improvements to the code cleanliness itself!

[c0per]

Thank you.

Since Session.store is a static property, I don't think you need to re-attach it. They share the same static property as long as you import them from the same place I think 🤔.

[jcs224]

Okay, I'll play around with it.

I feel like we need to be kind of careful now, since there are some notable projects using this library now. Most notably, Bedrock. Hence, the hesitation to make breaking changes (even if they're better)

[c0per]

From your use case, I think of another situation where this "static property" approach don't work well, that is if someone want to use different store / cookie options for different middleware.

Maybe it's better to ditch the static properties. Pass in the store / cookie options in initMiddleware(). That way these configuration only work for that middleware, and different stores for different middleware can be achieved.

From the api change perspective, It's from

const store = new SomeStore();
const store2 = new OtherStore();

const session = new Session(store, options);
const session2 = new Session(store2, options2);

app.use(session.initMiddleware());

// some specific route:
router.use(session.initMiddleware());

to

const store = new SomeStore();
const store2 = new OtherStore();

app.use(Session.initMiddleware(store, options));

// some specific route:
router.use(Session.initMiddleware(store2, options2));

And it works with your use case too. User can create a store in store.ts and export it. And import the store and Session class and use it as .use(Session.initMiddleware(store))

[jcs224]

Ah, true. I'm not sure how common this would be, but it might be better to keep the option open.

[jcs224]

In #27, this PR builds on your changes, but the initMiddleware function now takes store and options as arguments. I removed all static properties, but the static function calls remain how they were. What were formally the static properties are passed as arguments to the various functions in the class as needed.

[c0per]

That way we can also ditch the weird (at least for me) static property stuff. All the store, session options are only used to create a middleware in initMiddleware()