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

Add an opt in invalidate_active_sessions! method to session timeout #110

Merged
merged 5 commits into from
Jul 9, 2018

Conversation

fangbyte
Copy link
Contributor

@fangbyte fangbyte commented Feb 21, 2018

Add a configurable invalid_active_sessions! method to the session_timeouts module. Configurable, but expects an invalidate_sessions_before timestamp column on user. Does not invalidate the session if invalidate_sessions_before is not set meaning that if it is deployed it won't log out currently logged in users.

Update the gem version if necessary. Because this is opt in, it should not be a breaking change.

@arnvald

@fangbyte fangbyte changed the title Add an opt in invalidate_activate_sessions! method to session timeout Add an opt in invalidate_active_sessions! method to session timeout Feb 21, 2018
Spike on adding a configurable `invalid_active_sessions! method to the session_timeouts module. Configurable, but expects an `invalidate_sessions_before` timestamp column on user. Does not invalidate the session if `invalidate_sessions_before` is not set meaning that if it is deployed it won't log out currently logged in users.
@Ch4s3
Copy link
Contributor

Ch4s3 commented Mar 2, 2018

This looks pretty good. Can you update the readme?

@arnvald
Copy link
Contributor

arnvald commented Mar 2, 2018

@fangbyte @Ch4s3 I am not 100% sure if I'm right, but I'm afraid this concept cannot work. Let me explain it in 2 parts:

  1. Current situation - the invalidate_sessions_before is never cleared, which means that once I try to invalidate all sessions, I will never be able to log in again. Or rather, I will be able to log in just to be logged out on the next requests.

The only way to fix I can think of is to clear the invalidate_sessions_before after I log in. However that causes another big issue:

  1. With fix - since invalidate_sessions_before is cleared after log in, a following scenario can happen
  • I leave my account logged in on a public computer
  • at home, I remember that I left it there, so I click "log out all sessions", this makes the invalidate_sessions_before
  • still at home, I want to use the application again, so I log in, therefore invalidate_sessions_before value is cleared
  • on that public computer, someone opens the website. Because the invalidate_sessions_before was cleared, the session is not invalidated, so someone can use my account.

@fangbyte
Copy link
Contributor Author

fangbyte commented Mar 7, 2018

@arnvald I don't think you are logged out after the current session, since the invalidate_sessions_before timestamp is checked against the session login_time if it is present or last_action_time if login time is not present (for sessions created with the remember me token). Any sessions logged in after the invalidate_sessions_before timestamp should not be invalidated because login_time or last_action_time will be greater than the invalidate_sessions_before timestamp. I have added a test for the situation you mentioned in 1 in e9f802b

@arnvald
Copy link
Contributor

arnvald commented Mar 8, 2018

@fangbyte you're absolutely right, I misunderstood that part. The code will work as intended. Thank you for explanation!

…ns!`

There was concern that because `invalidate_sessions_before` is never cleared, there may be an issue where sessions would be cleared immediately after login. Add a spec to verify that sessions logged in after the `invalidate_sessions_before` timestamp can login and make additional requests without being cleared.
@fangbyte
Copy link
Contributor Author

fangbyte commented Mar 8, 2018

@Ch4s3 @arnvald I have updated the readme

Let me know if there's anything else I can do to move this forward.

This method never should have been private as it was meant to be part of the public API if enabled
This would throw an error if session login_time and last_action_time were not set, be more defensive and default to current time if those values aren't set.
@fangbyte
Copy link
Contributor Author

fangbyte commented Mar 9, 2018

I pushed two additional commits fixing small issues:

  • Move invalidate_active_sessions! to no longer be a protected method. It should never have been protected.
  • Added a default of Time.now.in_time_zone to the check to invalidate a session if login_time and last_action_time are not set rather than allowing it to raise a no method error on nil if those session values are not set.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Jul 9, 2018

Awesome! @fangbyte thanks so much for your patience!

@Ch4s3 Ch4s3 merged commit cc7c46f into Sorcery:master Jul 9, 2018
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.

3 participants