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

add getDomainFromRequest option, for when domain needs to be calculated #744

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulfitz
Copy link

Thanks for express-session! Here's a small addition that is perhaps a bit niche but has been useful for us.

This adds a getDomainFromRequest option which allows cookie.domain to be computed from the request rather than set as a constant.

As an example of when this is useful, imagine serving a single app from *.foo.com and *.bar.com, with the cookie domain being .foo.com for the *.foo.com domains and .bar.com for the *.bar.com domains.

This may be related to #311

This adds a `getDomainFromRequest` option which allows `cookie.domain`
to be computed from the request rather than set as a constant.
As an example of when this is useful, imagine serving a single app
from `*.foo.com` and `*.bar.com`, with domain being `.foo.com` for the
`*.foo.com` domains and `.bar.com` for the `*.bar.com` domains.

This may be related to expressjs#311
@dougwilson
Copy link
Contributor

It seems your scenario is different from that one, and seems achievable though the use of middleware, which is what makes Express so flexible. Have you considered something like

app.use(vhost('*.foo.com', session(fooConfig)))
app.use(vhost('*.bar.com', session(barConfig)))

You can pass the same store to both if you want to store sessions in the same place.

@paulfitz
Copy link
Author

Thanks for the suggestion! I did consider something similar. In my actual case, the specific domain families are not available to the code, so the middleware needed to do a bit more gymnastics. Overall it felt clumsy, and cleaner to solve in express-session, leaving me with a single session object. However, I can see this is unlikely to be a common case, so it is fine with me if you want to close this out, I won't be offended :-)

@dougwilson
Copy link
Contributor

It's no problem, and I'm not rejecting it to be clear :) I'm more trying to better understand the use-case so we can get to a situation where this is generically solvable and doesn't end up where this needs to be done for every other option, and then someone needs it to be async, etc. etc. :)

There may be a clean way, but I guess I would say it's hard to work together without understanding what you're trying to accomplish. I believed I understood from your initial issue, but it sounds now that what was written there doesn't actually represent your true goals to get from this change.

Is it possible to outline them?

@paulfitz
Copy link
Author

I'd now edit:

As an example of when this is useful, imagine serving a single app from *.foo.com and *.bar.com, with the cookie domain being .foo.com for the *.foo.com domains and .bar.com for the *.bar.com domains.

to be: imagine serving a single app from several domain families (*.foo.com, *.bar.com, etc.) where the cookie domain should omit the subdomain, and the set of domain families is determined outside of the code (e.g. in dns and load balancer configuration).

Does that make any more sense? Thanks for your patience.

@dougwilson
Copy link
Contributor

Hi @paulfitz thank you. That does make sense. Sounds like you could use vhost to accomplish this (I just wrote it up and didn't extensively test it). The nice thing is that this works with any middleware config, and not even just this module. If this is a pattern that is useful, having a module on npm to wrap around middlewares would be super useful to everyone too.

// example lib that exports a middleware to use for your use-case:
var session = require('express-session')
var vhost = require('vhost')

module.exports = function (options) {
  var cache = Object.create(null)
  return vhost(/^(?:.+\.)?([^.]+\.[^.]+)$/, function (req, res, next) {
    var domain = req.vhost[0]
    var mw = cache[domain]

    if (!mw) {
      var opts = Object.assign({}, options)
      opts.cookie = Object.assign({}, opts.cookie || {})
      opts.cookie.domain = '.' + domain
      cache[domain] = mw = session(opts)
    }

    mw(req, res, next)
  })
}

@ramiroaisen
Copy link

ramiroaisen commented Mar 20, 2021

Upvote for this I have the same trouble here.
I want to share sessions between subdomains,
it would be really nice to have a callback domain config instead of a constant
something like this

 session({
  cookie: {
    domain: (req: IncommingMessage): string | undefined => "example.com" // or whatever
   }
})

Note that this signature is fully backwards compatible as session({ cookie: { domain: "example.com" } }) still works as expected

@uriesk
Copy link

uriesk commented Sep 11, 2022

Got the same usecase now.
I do not like the vhost or custom middleware solutions because they all create a new session middleware at every single request.
I would prefer callback function that allows to change any cookie settings in an already created session middleware (maybe someone wants to dynamically adjust the expire time?).

@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 9d2e29b to 408229e Compare January 28, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants