koa-passport icon indicating copy to clipboard operation
koa-passport copied to clipboard

Bugfix: Issue #181

Open davidnbooth opened this issue 2 years ago • 5 comments

Add dummy req.session.regenerate and req.session.save, which passport 0.6.0 expects the request object to have.

Not entirely sure if this is in line with the philosophy of this library, but it fixed the issue for me, and I tried to match the style of the existing code!

Let me know what you think :)

davidnbooth avatar Nov 23 '22 17:11 davidnbooth

Hi, thanks for the PR. I am honestly a bit torn. I'd rather see a fix in upstream passport than adding a workaround. However, I am not completely opposed to it either seeing that it might help a couple of people.

One of my main concerns is, that after adding this, a bugfix release in passport fixes it upstream in a way that is incompatible with the workaround in koa-passport breaking peoples apps. Due to that, the only change request I'd have, if we go ahead with this, is that the dummy methods are only added, if they do not already exist (assuming that some session middleware provide them).

An alternative would be to rely on a workaround outside of koa-passport, similar to

app.use(cookieSession({
    // ...
}))
// register regenerate & save after the cookieSession middleware initialization
app.use(function(request, response, next) {
    if (request.session && !request.session.regenerate) {
        request.session.regenerate = (cb) => {
            cb()
        }
    }
    if (request.session && !request.session.save) {
        request.session.save = (cb) => {
            cb()
        }
    }
    next()
})

As suggested in https://github.com/jaredhanson/passport/issues/904#issuecomment-1307558283

rkusa avatar Nov 24 '22 10:11 rkusa

Thanks for the response! I made the change you suggested, and feel a bit silly that I didn't do that to begin with 😅.

I definitely see what you mean about an upstream fix being better, and this not being a perfect solution by any means! Up to you if you want to merge this, but its here if you do :)

davidnbooth avatar Nov 24 '22 21:11 davidnbooth

Thanks for the update! I'll collect some feedback in #181 to get an idea how urgent a workaround inkoa-passport is, or whether the custom middleware (mentioned above) is good enough for most people.

rkusa avatar Dec 06 '22 13:12 rkusa

Thanks for the update! I'll collect some feedback in #181 to get an idea how urgent a workaround inkoa-passport is, or whether the custom middleware (mentioned above) is good enough for most people.

@rkusa I think it would be better if you fix this issue in Koa-passport and release it asap, we are consuming koa-passport in many of our code packages. Please let us know the timeline.

ChiranjitSaha3013115 avatar Jan 02 '23 07:01 ChiranjitSaha3013115

koa-session v6.4.0 was just released, with my PR linked above merged (https://github.com/koajs/session/pull/221). This solves the issue without any workarounds needed.

lehni avatar Feb 04 '23 15:02 lehni