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

Bugfix: Issue #181 #187

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Bugfix: Issue #181 #187

wants to merge 2 commits into from

Conversation

davidnbooth
Copy link

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 :)

Add dummy req.session.regenerate and req.session.save for passport 0.6.0
@rkusa
Copy link
Owner

rkusa commented Nov 24, 2022

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 jaredhanson/passport#904 (comment)

Only add dummy regenerate and save methods if they do not already exist
@davidnbooth
Copy link
Author

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 :)

@rkusa
Copy link
Owner

rkusa commented Dec 6, 2022

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.

@ChiranjitSaha3013115
Copy link

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.

@lehni
Copy link
Contributor

lehni commented Feb 4, 2023

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

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

Successfully merging this pull request may close these issues.

5 participants