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

How to log a user out of all sessions #69

Closed
HartS opened this issue Dec 27, 2021 · 11 comments
Closed

How to log a user out of all sessions #69

HartS opened this issue Dec 27, 2021 · 11 comments

Comments

@HartS
Copy link

HartS commented Dec 27, 2021

I posted #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
Copy link
Owner

kleydon commented Jan 2, 2022

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:

  • 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?
  • Are there any security considerations, re: facilitating deletion of all sessions for a given user?

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

@HartS
Copy link
Author

HartS commented Jan 2, 2022

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:

  • Attempted user logins/activity gets flagged as suspicious and necessitates a password reset via reset link
  • The user suspects their password has been compromised and changes the password themselves (perhaps they've reused the password and learned that it was compromised from another service)

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
Copy link
Owner

kleydon commented Jan 3, 2022

@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
Copy link
Owner

kleydon commented Jan 4, 2022

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
Copy link
Author

HartS commented Jan 4, 2022

@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
Copy link
Owner

kleydon commented Jan 4, 2022

@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
Copy link
Owner

kleydon commented Jan 6, 2022

@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
Copy link
Owner

kleydon commented Jan 10, 2022

Currently asking the express-session folks if something along these lines might be on their roadmap:
expressjs/session#865

@HartS
Copy link
Author

HartS commented Jan 11, 2022

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
Copy link
Owner

kleydon commented Jan 17, 2022

@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
Copy link
Owner

kleydon commented Feb 15, 2022

@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.

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

No branches or pull requests

2 participants