-
-
Notifications
You must be signed in to change notification settings - Fork 983
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
base: master
Are you sure you want to change the base?
Conversation
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
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. |
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 :-) |
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? |
I'd now edit:
to be: imagine serving a single app from several domain families ( Does that make any more sense? Thanks for your patience. |
Hi @paulfitz thank you. That does make sense. Sounds like you could use // 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)
})
} |
Upvote for this I have the same trouble here. session({
cookie: {
domain: (req: IncommingMessage): string | undefined => "example.com" // or whatever
}
}) Note that this signature is fully backwards compatible as |
Got the same usecase now. |
9d2e29b
to
408229e
Compare
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 allowscookie.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