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

STCOR-869 do not store /logout as a "return-to" URL #1510

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

zburke
Copy link
Member

@zburke zburke commented Jul 25, 2024

When a session ends due to timeout, the current location is stored in order to allow the subsequent session to begin where the previous one left off. If the "session timeout" event fires more than once**, however, this could lead to the /logout location being stored as the "return to" location with obvious dire consequences.

There are two changes here:

  1. Don't allow locations beginning with /logout to be stored. This fixes the symptom, not the root cause, but is still worthwhile.
  2. Store the session-timeout interval ID in redux, and manage that timer via a redux action. Even though this still shouldn't fire more than once, if it does, this allows us to cancel the previous timer before adding the next one. This is an attempt to fix the root cause.

Refs STCOR-869

When a session ends due to timeout, the current location is stored in
order to allow the subsequent session to begin where the previous one
left off. If the "session timeout" event fires more than once**,
however, this could lead to the `/logout` location being stored as
the "return to" location with obvious dire consequences.

There are two changes here:
1. Don't allow locations beginning with `/logout` to be stored. This
   fixes the symptom, not the root cause, but is still worthwhile.
2. Store the session-timeout interval ID in redux, and manage that timer
   via a redux action. Even though this _still_ shouldn't fire more than
   once, if it does, this allows us to cancel the previous timer before
   adding the next one. This is an attempt to fix the root cause.

Refs STCOR-869
@zburke zburke requested review from ryandberger and aidynoJ July 25, 2024 22:32
Copy link

github-actions bot commented Jul 25, 2024

Jest Unit Test Results

276 tests  +6   276 ✅ +6   1m 0s ⏱️ -1s
 54 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 3a7a5f1. ± Comparison against base commit 48ca517.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 25, 2024

Bigtest Unit Test Results

192 tests  ±0   187 ✅ ±0   6s ⏱️ ±0s
  1 suites ±0     5 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 3a7a5f1. ± Comparison against base commit 48ca517.

♻️ This comment has been updated with latest results.

Copy link

Copy link
Member

@ryandberger ryandberger left a comment

Choose a reason for hiding this comment

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

LGTM

@zburke zburke merged commit c25e068 into keycloak-ramsons Jul 26, 2024
26 checks passed
@zburke zburke deleted the STCOR-869-ramsons branch July 26, 2024 20:12
zburke added a commit that referenced this pull request Dec 26, 2024
`setRtrFlsWarningTimeout()` now sends `rtrFlsWarningTimeout` rather than
`rtrFlsTimeout`. This bug was introduced in PR #1511 (onto
`keycloak-quesnelia`), which looks like a bad copy-pasta from PR #1510
(onto `keycloak-ramsons`).

When RTR is correctly configured such that sessions end at a fixed
(static) datestamp, this change will have no effect: the original timer
will not be canceled when the tokens rotate, but it won't matter because
the end-point isn't moving, meaning the original timer will remain
accurate.

When RTR is incorrectly configured, however, and a session's end-point
is extended on rotation as in a sliding window, this change resolves a
bug that would cause the session-timeout banner to display a "Time
remaining: 00:00" message without actually ending the session. This
occurred because the original timer would not be canceled when the
tokens rotated even though a new, later end-point had been established.

Refs STCOR-930
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.

2 participants