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

session_timeout: make sure remember_me's before_logout hook gets called #358

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jankoegel
Copy link

@jankoegel jankoegel commented Feb 14, 2024

The Problem

  • if an app uses both the session_timeout as well as the remember_me submodule the following can happen:
    • even if a user's invalidate_sessions_before timestamp is set to now, their remember_me sessions will not be invalidated
    • reason: the remember_me cookie doesn't get deleted during the session_timeout submodule's validate_session
  • debug log that shows the flow of events without this PR:
    -> VALIDATE SESSION
    expired? false
    invalidated? true
    💣 RESETTING SESSION
    reset @current_user
    🍪 login_from_cookie
    🎂 register_login_time
    😱 assigning current user again
    

In the code

@jankoegel jankoegel mentioned this pull request Feb 14, 2024
3 tasks
@PedroAugustoRamalhoDuarte

@jankoegel i am trying to get the test from my PR to yours, but the test are broken because rspec version with rails 7.1:
rspec/rspec-mocks#1530

I have upgrade the rspec, but a lot of other test become broken due the upgrade rspec-rails from version 3 to version 6. I will open a PR from my test branch to yours and after that we check with @joshbuker its OK to merge everything together

Copy link
Member

@joshbuker joshbuker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting...This looks like it should work, but appears to be failing the OAuth session timeout specs.

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