-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: support for logging out of all sessions #70
Conversation
Adds a function - destroyUsersSessions() - to enable deleting all sessions for a given user, given the session id for one of the user's sessions.
Some simplification in prisma-session-store.ts
@wSedlacek, if you feel at all like giving this PR a review, I'd love to get your feedback. |
Attempt to address error TS2339 in prisma-session-store.ts: Property 'uid' does not exist on type 'PartialObjectDeep<SessionData>'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been quite busy at work so I haven't had a chance to give this my full attention.
However this seems fine on the surface.
My initial thought was if this feature would be within scope of this project, perhaps this is something that express-session
needs to offer and then we could simply provide the proper interface for that feature. However I haven't had enough time to research that thought. So I will defer to @kleydon as to if this is the right place for this feature.
I left a few comments to improve code quality, but there is nothing blocking, and honestly just more preferences than anything else.
I notice there was quite a bit of work to get CI/CD passing.
At some point we should switch over to eslint, and update our tooling.
I have been working on a similar high quality eslint config, but haven't had the time to get it published in any major way.
@wSedlacek - Appreciate you having having a quick look.
I'd been back-and-forth on this as well; going to mull it over, and see if a request for this functionality has come up in the express-session project before.
Good point! Well, something for the future... |
Waiting for the merge! this would be very useful |
Hi @zaniluca, thanks for the feedback. Temporarily holding off, to verify that the express session team doesn't have an equivalent of this feature on their roadmap (which might result in future backward compatibility issues). Hoping to hear back from them this week. |
src/lib/prisma-session-store.ts
Outdated
throw Error(errMsg); | ||
} | ||
const uid = s.uid; | ||
if (typeof uid !== 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for an empty string here also? Since uid isn't required/unique, could people set it to @default('')
, which would then result in this clearing the sessions for all users who don't have the uid explicitly set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
This looks great so far, other than a couple of things I pointed out. Removing Tomorrow I should have time to pull up the changes and give it a try in our project |
@HartS - Just merged the refinement you suggested.
Great - thanks! Do post if this PR causes any trouble, or doesn't behave as you'd expect. |
Hi, so you're not going to merge this pr? Some major conflicts from the express-session team? |
Hi @zaniluca - sorry I neglected to include the rationale in the comments for this PR. Here's the issue; (quoting @ultimate-tester from a related discussion in the express-session repo):
>Therefore, my suggestion is to leave the requested functionality out of session management but persist this information in the database. Upon login, the user session ID should be stored into the database together with all the metadata you want. You might think "But then you are storing data redundantly as my session ID is already in the database", but I disagree with that. You are merely storing a reference just as you would with other relations between tables. |
Adds a function,
destroyUsersSessions()
, to enable deleting all sessions for a given user, given the session id for one of the given user's sessions.Note: Use of this functionality requires that all sessions have been added to the store using the store's
set()
function with the set() function'ssession
argument containing a (new)uid
(user id / user name) property.Question: This feature necessitate addition of a new
uid
(user id / name) field toschema.prisma
. Could this create any backward-compatability issues? Can/should they be mitigated by this library?