STCOR-776 optionally schedule RTR based on session data #1488
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TLDR: just like the title says, only trigger RTR on successful authentication requests. The proximate reason for this change is e2e test failures that appear to be related to RTR erroneously firing 10 seconds into a test, which is only explainable if an authn request to `/_self` succeeded. Previously, cruft in localforage certainly could lead to a call to `/_self` even when no session was active ... but it would have return 4xx with a plain text response. RTR _should_ only have kicked off if the response contained json. IOW, this change definitely patches a bug, but it doesn't reliably explain all the bad behavior seen in tests. Nonetheless, it feels like a worthwhile fix since it does fix ... something. Refs STCOR-776
Bigtest Unit Test Results193 tests ±0 188 ✅ ±0 6s ⏱️ ±0s Results for commit 13458ef. ± Comparison against base commit 83afa6b. This pull request removes 5 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Not all authentication-related responses are created equal. If the response contains AT expiration data, i.e. it is a response from `/bl-users/login-with-expiry`, use that data to configure RTR. If the response does not contain such data, i.e. it is a response from `/bl-users/_self`, pull AT expiration data from the session and use that. If we still don't have any AT expiration data, cross your fingers and try rotating in 10 seconds. This doesn't precisely handle the issue faced by e2e tests that stall when the rtr requests fires, but leveraging the session data should pushh the rtr request far enough into the future that most e2e tests will now avoid it. We should still try to understand that problem and solve it, but in the mean time this may enable us to work around it.
zburke
requested review from
JohnC-80,
ryandberger,
aidynoJ,
ostapwd and
ykumar-ebsco
zburke
changed the title
ryandberger
approved these changes
Jun 10, 2024
|
zburke
added a commit
that referenced
this pull request
Jul 25, 2024
Not all authentication-related responses are created equal. If the response contains AT expiration data, i.e. it is a response from `/bl-users/login-with-expiry`, use that data to configure RTR. If the response does not contain such data, i.e. it is a response from `/bl-users/_self`, pull AT expiration data from the session and use that. If we still don't have any AT expiration data, cross your fingers and try rotating in 10 seconds. Previously, we did not ever check the session, so RTR always fired 10 seconds into a resumed, which is exactly the situation faced in e2e tests. This doesn't precisely handle the issue faced by e2e tests that stall when the rtr requests fires, but leveraging the session data should push the rtr request far enough into the future that most e2e tests will now avoid it. We should still try to understand that problem and solve it, but in the mean time this may enable us to work around it. Refs STCOR-776 (cherry picked from commit e93a5af)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not all authentication-related responses are created equal. If the response contains AT expiration data, i.e. it is a response from
/bl-users/login-with-expiry, use that data to configure RTR. If the response does not contain such data, i.e. it is a response from/bl-users/_self, pull AT expiration data from the session and use that. If we still don't have any AT expiration data, cross your fingers and try rotating in 10 seconds.Previously, we did not ever check the session, so RTR always fired 10 seconds into a resumed session, which is exactly the situation faced in e2e tests.
This doesn't precisely handle the issue faced by e2e tests that stall when the RTR request fires, but leveraging the session data should push the RTR request far enough into the future that most e2e tests will now complete before RTR fires. We should still try to understand that problem and solve it, but in the mean time this may enable us to work around it.
Refs STCOR-776, FAT-13789