kleydon / prisma-session-store

Express session store for Prisma
MIT License
116 stars 18 forks source link

How to log a user out of all sessions #69

Closed HartS closed 2 years ago

HartS commented 2 years ago

I posted https://github.com/kleydon/prisma-session-store/issues/39 a while ago (expecting the Session model to have a reference to the User model)

Recently I've been trying to figure out how to be able to destroy all sessions for a given user. As per the advice in that issue, I did put the userName data on the session, and it's stored in Prisma as a property of the JSON object in the data field.

However, this can't be queried (except perhaps when using Postgres). So if I wanted to destroy all sessions for a given user (as an admin user), I don't believe there's a way to do this without reading all session records from the database and then filtering in the application.

Is it possible to do this without reading all records with prisma-session-store?

kleydon commented 2 years ago

Hi @HartS,

Interesting; I can see how the ability to destroy all of a particular user's sessions (given the session id for one of the user's active sessions) could be useful, e.g for logging out everywhere.

Currently, the limitations are as you say; a userName can be included within a session's data field, but querying the db based on this userName may not be possible - unless the db permits queries based on properties within the data field (e.g. json object property-based querying). If the db does not permit this kind of querying, then you're right - it is currently necessary for the application to read all session records, then filter.

Currently, this session store doesn't include a notion of a userName. If a userName field were explicitly added directly to the Session model, it could enable the behavior you are after.

Some considerations I'm trying to think through:

@HartS what do you think? @wSedlacek, what's your take?

HartS commented 2 years ago

My thoughts on the considerations:

Would this change reduce the generality of the session store? Would it be odd/confusing for some session data to exist outside data (userName), while other session data exists inside data? What issues/confusion (if any) arise for existing users, if a userName field is added outside of data?

Could userName (and/or userId) be made an optional field? This would avoid any of the above 3 issues I think

Are there any security considerations, re: facilitating deletion of all sessions for a given user?

Logging out all logged in users is typically done by applications after a user password change. Some examples:

This is why changing a password often results in sessions being destroyed. I think not destroying sessions after a password change is less secure.

kleydon commented 2 years ago

@HartS - thanks for the thoughts.

Could userName (and/or userId) be made an optional field?

I think so; I'm going to spend a little time investigating today.

The next month could be a bit hectic for me, so if you don't see a PR by the end of the day, feel free to submit one if you'd like to.

kleydon commented 2 years ago

Hi @HartS,

I just put together a PR (#70) with support for destroying all sessions for a given user.

Any chance you could give this a review, and see if it works for you in practice?

One thing I'm particularly curious about is whether it introduces any unanticipated backwards compatibility issues... If you happen to notice anything - I'd love to know about it.

Cheers.

HartS commented 2 years ago

@kleydon This looks great, thank you! I already worked around it as described above (not a problem for our database since there aren't that many users/sessions), but will be happy to review this also.. might take me a week or so

kleydon commented 2 years ago

@HartS - glad to hear you aren't blocked. I'm still wrestling with some remote CI issues, but will post when these are resolved.

kleydon commented 2 years ago

@wSedlacek, @HartS: PR #70 to address issue #69 (support for logging out of ALL of a user's active sessions) now passing remote CI tests. Reviews / testing welcome!

kleydon commented 2 years ago

Currently asking the express-session folks if something along these lines might be on their roadmap: https://github.com/expressjs/session/issues/865

HartS commented 2 years ago

I should have time to take a look at this today or tomorrow. Will try replacing the current session.deleteMany workaround in our app with the destroyUserSessions and let you know how it goes.

kleydon commented 2 years ago

@revington wrote:

I implemented this [ability to log out of all of a given user's sessions] in the past by changing the genid function. The idea is to generate a random id that we can find later i.e $userid-random(). Then in your database you can query by all keys starting with user id $userid-*. The trick here is to overwrite the user session id once they log in.

@HartS - What do you think about something along the lines of this approach?

Rather than constraining / making assumptions about what goes in the data field (as PR #70 does), @revington's approach constrains the format of all sessions' record ids to something like $userid-*.

This could mostly be achieved by back-end code, by: 1) setting the dbRecordIdIsSessionId option to true, and (in back-end code) prefixing the record/session id with the user name / id once known, or (if dbRecordIdIsSessionId must be false), 2) through use of the dbRecordIdFunction option...

This approach would still require at least one change to prisma-session-store to support deleting all a user's sessions (based on record-id prefix), and it might be preferable for pre-pending username to happen automatically within this library as well; will have to think about this.

kleydon commented 2 years ago

@ultimate-tester wrote here that:

The solution provided by revington [prefixing session ids with the user's username to facilitate the capability of deleting all of a user's sessions] is definitely a good one, it will do what you expect. Though, I am not a fan of solutions that involve loops. Especially when you have a high-traffic site, you will notice problems. On top of that, having the list of sessions per user is probably not the complete functionality requested. Usually sites that offer the functionality you describe also show what kind of device the session is associated with, the login time, the IP, and other metadata.

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.

On reflection, I suspect this is the most future-proof (and computationally inexpensive) way to go.

Closing this issue (and its corresponding pr #70) for now.