JKHeadley / appy-backend

A user system to bootstrap your app.
https://appyapp.io
MIT License
108 stars 30 forks source link

support multiple sessions #12

Closed JKHeadley closed 6 years ago

JKHeadley commented 7 years ago

Currently a user can only log in to one device at a time.

henriquesa commented 7 years ago

Do you want help on this?

JKHeadley commented 7 years ago

@henriquesa Absolutely! Any help/suggestions are always welcome.

JKHeadley commented 7 years ago

@henriquesa If you would like to help, please reference #11 for some things to keep in mind.

henriquesa commented 7 years ago

Awesome! I read up on that issue and the linked SO discussion, and I think we can brainstorm a good solution.

The SO discussion seems pointed towards how to invalidate the JWT token, while avoiding a DB hit. The original poster gave up on this original idea and decided to always query the user. The second answer you pointed out has an interesting idea of using JWTs with a short lifespan. I don't think this is a good approach because I believe the user must have a way to immediately revoke access to their account of other active sessions. Appy doesn't currently use JWTs to avoid DB hits either, so there's an added implementation complexity. This isn't a huge deal though, I do think we should pick the better option, but I don't think that's it.

1) Do you agree we should ignore the extra DB hit for immediate revoking and not consider the temporary JWT approach?

then they could easily invalidate all their current tokens by changing their password.

I agree a password hash is a very easy way to invalidate these tokens when users change passwords. I'd suggest on top of this an extra endpoint where a logged in user could invalidate all their sessions for this specific use-case, a "log out of all other devices" action.

2) Assuming appy stops destroying other sessions on new logins, what do you think about this extra endpoint?

3) While I don't mind the password hash approach, do you think it's also worth it destroying sessions when the password changes?

4) Do you see any issues having the logout endpoint destroying that specific session and keeping the others active?

henriquesa commented 7 years ago

@JKHeadley on number 1, "Do you agree we should ignore" may be better read as "Do you agree we should keep ignoring", because there's already a hit on both Session and User for every request.

JKHeadley commented 7 years ago

Hi @henriquesa, thanks for the brainstorming thoughts! Here are my thoughts on your points.

  1. This depends on the type of authentication strategy being used. Please see the wiki for details on the different strategies. The default strategy for appy uses both an access token and a refresh token. The idea is that the access token has a short lifespan, but does not require a db hit, while the refresh token has a long lifespan, but requires a db hit, and therefore can be invalidated. This reduces the number of db hits while still providing a level of control over token security.
    On the other hand the session strategy does hit the db on every request, and therefore can revoke all token access immediately. In either case, allowing multiple user sessions, and how sessions are revoked, should be independent of which authentication strategy is used (unless of course the strategy does not use sessions).

  2. I agree that having an endpoint to log out of all sessions would be useful.

  3. I do feel that invalidating all tokens (not necessarily destroying all sessions) is good on a password change, simply because the nature of a password change is usually an action to gain control/increase security, and invalidating tokens falls in line with that theme.

  4. I think the logout endpoint should always only destroy the one session linked to the current token. I do see an issue with "orphan" sessions cluttering the db if multiple sessions are allowed (when a user logs in but never logs out), but I believe there are ways to account for this. The best option I see is to swap the "createdAt" session property (seen here) with an "updatedAt" property that includes an expiration period equal to the refresh token expiration period. Whenever the refresh token is updated, the "updatedAt" property should be updated to the current time as well. This guarantees that when the refresh token expires, mongodb will automatically delete the session as well. You can read more about expiring data through mongodb here.

henriquesa commented 7 years ago

Thanks for the quick reply, @JKHeadley! I did forget there are multiple strategies when thinking about this, I only took into consideration the session strategy. Agreed revoking should be independent of the choice wherever possible.

With this new info, I guess the session implementation is very straightforward. We could

  1. on login, stop invalidating other sessions
  2. on logout, only invalidate the current session
  3. create an endpoint where all sessions are invalidated
  4. implementing the updatedAt property for mongodb purging (as well as suggesting implementing a shorter expiration period to further help avoiding the mentioned orphan sessions)

Unless I'm missing something, the refresh token strat wouldn't really differ, because refresh is very similar to session. Do you think the standard token should change in any way, both in the standard token strategy and its behavior in the refresh token strategy? And do you agree with the steps above?

JKHeadley commented 7 years ago

@henriquesa That sounds good!

I do believe the refresh and session strategies use the same internal logic for handling sessions, so it should work the same for both. I don't think the standard/access token should change in any way.

I see your point with implementing shorter expiration periods. I think in the end it is up to the developer using appy how long each expiration period should be. Either way I think it makes sense for the session expiration to match the refresh/session token expiration, since one expiring invalidates the other.

JKHeadley commented 7 years ago

@henriquesa Just checking in, how's the progress with this?

labsvisual commented 6 years ago

@JKHeadley, can I join?

JKHeadley commented 6 years ago

@labsvisual I actually have an implementation in place for this feature now, but you are definitely welcome to work on other features!