Skip to content

LG-11777: Handle logout request as already logged out if concurrent session exists#9842

Merged
aduth merged 6 commits intomainfrom
aduth-lg-11777-concurrent-session-logout
Jan 4, 2024
Merged

LG-11777: Handle logout request as already logged out if concurrent session exists#9842
aduth merged 6 commits intomainfrom
aduth-lg-11777-concurrent-session-logout

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jan 3, 2024

🎫 Ticket

LG-11777

Related issue: #9407

🛠 Summary of changes

Updates the behavior of the OIDC and SAML logout actions to prevent a user from being sent to the sign-in page if their session is terminated by the session_limitable logic (concurrent browser session), treating them as if they had already been logged-out, and inheriting the existing expected behavior[1][2] for redirecting a logged-out user.

📜 Testing Plan

  1. Run the OIDC or SAML sample application in a separate terminal process
  2. Go to http://localhost:9292 (OIDC) or http://localhost:4567 (SAML)
  3. Click "Sign in"
  4. Complete sign in or account creation until returned to the partner. Keep this tab open.
  5. In a separate browser or private browsing window, repeat Steps 2-4
  6. Return to your original tab from Step 4
  7. Click "Sign out"

Observe that you are redirected back to the sample application as a completed logout.

aduth added 3 commits January 3, 2024 09:03
…ession exists

changelog: Bug Fixes, Logout, Consistently handle logout request for logged out user if session terminated by sign-in with another browser
@aduth aduth requested a review from a team January 3, 2024 14:13
Comment on lines +12 to +14
def redirect_url
request.env["devise_#{warden_message}_failure_redirect_url"] || super
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way this works is we assume that a concurrent session logout throw will be handled by Devise's failure app responder. By default, this would redirect to the new_user_session_url. We're intercepting it to redirect them to the same place they originally intended, with the expectation that the user would be fully logged out at that point. It's not ideal that this incurs an additional redirect hop, but this gives the best assurance that Warden has a nil user object, since our concurrent session logout behavior only occurs after Warden has already loaded and assigned the user who's being signed out.

Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

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

Looks good and works locally for me. I appreciate the comments - they help give insight to what's going on there.

aduth and others added 2 commits January 3, 2024 15:33
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth merged commit b733821 into main Jan 4, 2024
@aduth aduth deleted the aduth-lg-11777-concurrent-session-logout branch January 4, 2024 20:17
@amirbey amirbey mentioned this pull request Jan 9, 2024
jmhooper added a commit that referenced this pull request Jan 15, 2025
Storing the locale in these places leads to a call to `current_user` which trips the session limitable raise before `devise_session_limited_failure_redirect_url` is set by these controllers.

This commit resolves this issue by not calling the before action for controller actions that set `devise_session_limited_failure_redirect_url`

See #9842
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.

5 participants