Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Express-session and PassportJS #18

Closed
wstrm opened this issue Oct 12, 2014 · 4 comments
Closed

Express-session and PassportJS #18

wstrm opened this issue Oct 12, 2014 · 4 comments

Comments

@wstrm
Copy link

wstrm commented Oct 12, 2014

As I'm trying to migrate my old Socket.IO based project to Socketcluster I noticed PassportJS is not saving sessions into the store.
Probably because Socketcluster interferes the current session solution from Express that I've used? Which also mean I can not use a Redis store for Express only?

Old version would give me a user id if they are authenticated:

req.session.passport = { user: 'userID' }

But now after serialization it just stays empty:

req.session.passport = {}

Although the req.session.passport object is still there.

I don't know what I should do, neither am I sure what is causing this, any ideas?

@jondubois
Copy link
Member

Yes, the problem lies with express-session middleware (not with passport.js itself).
SocketCluster sets its own req.session property so as you pointed out it's likely that it's overwriting the one provided by express' session middleware.

I'll try to come up a way to get SC's session engine to work with the one provided by express. I'll have a look later today. Feel free to tinker with SC in the meantime and see if you can find a solution or maybe even make a pull request if you feel confident enough (but that part of the code could be tricky because it affects a lot of things).

You can use Redis with SC but it has to work alongside the default nData solution (which is used internally to do all the pub/sub magic). There are plans to bring better SC-Redis integration in the future (by making a special wrapper to allow Redis to handle all of SC's internal pub/sub and session storage needs) - That way it would optionally replace the default nData solution, but for now it should be used alongside the default nData engine (not a full replacement). They should work well alongside each other. If you were to start a fresh project though, it would definitely be simpler to just use the default solution (it's just a bit easier - and scaling will be easier down the track).

@MegaGM
Copy link
Member

MegaGM commented Oct 12, 2014

@willeponken I don't use PassportJS but after 59412c0 v0.9.75 I encountered with same issue. In my case, I didn't wanted to use Socketcluster's sessions in "Express part" of my application at all. And I also use Redis for storing sessions.
So, I just added a middleware which deletes req.session right before including a session-middleware

In the worker.js

    /* ---------------------------------------------
    * That's it!
    * ---------------------------------------------*/
    app.use(function(req, res, next) {
        delete req.session;
        next();
    });

    /* ---------------------------------------------
    * Now req.session doesn't exist, usual Express session-middlewares works as expected
    * ---------------------------------------------*/
    var sessOptions = config.get('options.sess');
    sessOptions.store = new redisStore({
        host: config.get('redis.host'),
        prefix: config.get ..... etc
    });

    app.use(sessions(sessOptions));

@jondubois
Copy link
Member

@MegaGM - Thanks for sharing your workaround. For convenience, I added a new option to prevent the default session object from being added to HTTP requests so that you don't need to add the middleware explicitly.

In version 0.9.84, you can now set the addSessionToHTTPRequest option to false if you don't want to attach SC's session object to HTTP requests (this lets you use a separate session object such as the one provided by express).

server.js:

var socketCluster = new SocketCluster({
  // ...
  addSessionToHTTPRequest: false
});

Note that this property is true by default because it's nice to be able to share the same session data across two different protocols. For the sake of migrating existing apps, though, it may be easier to keep them separate.

If you're starting a project from scratch though, it's better to leave it as true and use the default SC session object (instead of using the express-session middleware) - That way the same session object will be used for HTTP and Realtime (WebSocket) protocols - This allows you to share the same authorization tokens and permissions across both protocols.

As mentioned earlier, I intend to make a Redis-backed version of this session object (instead of nData) due to popular demand. SC's architecture is already in place to support using alternate session storage engines - We just need to create the wrapper.

@wstrm
Copy link
Author

wstrm commented Oct 13, 2014

@MegaGM Your solution worked great.
Thanks @jondubois for such a fast response and fix, going with your solution of course, Socketcluster works great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants