[CP Staging] [Payment due @huult] Coalesce concurrent SAML reauthentication redirects to fix SSO sign-in flashing#96108
Conversation
|
@huult can you please test this thoroughly? |
|
we need to release it to production to test. Could you help me after I've reviewed the code quality? |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🎯 @huult, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
@mountiny We need to cherry-pick this to production for manual testing. Please let me know when it's ready in prod, and I will test it. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
…alesce (cherry picked from commit fa0cefa) (cherry-picked to staging by mountiny)
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.34-10 🚀
|
|
🤖 I reviewed the changes in this PR against the help site files under No help site changes are required. This is an internal bug fix — it coalesces the burst of concurrent SAML Why no docs update is needed
@allgandalf, since no help site changes are needed, no draft PR was created. If you believe a troubleshooting note should be added anyway (e.g., a line in the SAML/SSO troubleshooting article), let me know and I'll draft one. |
|
Unable to sign in when the required SAML turn on, Tested on Xiaomi Poco F5 / Android 13 in v9.4.34-10. 1000211853.mp4In Desktop continuous loading displayed Recording.3217.mp4 |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.34-14 🚀
|
|
🤖 No help site changes required. This PR is an internal bug fix in the auth/reauthentication layer — it coalesces the burst of concurrent SAML reauthentication redirects that fire when a session token expires, so an SSO user sees a single IdP prompt instead of a flashing series of redirects. It changes I reviewed the existing SAML/SSO help articles and none of them document the behavior this PR touches:
These articles cover how to configure SAML SSO and how to troubleshoot login failures (certificates, IdP metadata, lockouts). The fix simply restores the intended single-redirect behavior on token expiry — it does not add, remove, or change any documented user-facing behavior — so there is nothing to update on the help site and no draft PR is needed. @allgandalf, if you'd still like a docs PR created (e.g. you consider any of this worth documenting), let me know and I'll open one. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
|
|
🤖 Payment issue created: #96239 |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.34-14 🚀
|
|
🤖 No help site changes required. This PR is an internal bug fix in the auth/reauthentication layer — it coalesces the burst of concurrent SAML reauthentication redirects that fire when a session token expires, so an SSO user sees a single IdP prompt instead of a flashing series of redirects. It only changes I reviewed the existing SAML/SSO help articles and none of them document the behavior this PR touches:
These articles cover how to configure SAML SSO and how to troubleshoot login failures (certificates, IdP metadata, lockouts). The fix simply restores the intended single-redirect behavior on token expiry — it does not add, remove, or change any documented user-facing behavior — so there is nothing to update on the help site and no draft PR is needed. @allgandalf, if you'd still like a docs PR created (e.g. you consider any of this worth documenting), let me know and I'll open one. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.35-1 🚀
|
|
🤖 Re-checked — my conclusion is unchanged: no help site changes are required for this PR. It's an internal fix in the auth/reauthentication layer ( (This matches the earlier reviews already posted on this PR.) @allgandalf, if you'd still like a docs PR created for any part of this, let me know and I'll open one. |
|
@m-natarajan Could you try reproducing this issue on production #96108 (comment) I haven't been able to reproduce it on production so far. |
|
I asked for a retest |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.4.35-6 🚀
|
Explanation of Change
SSO users on a SAML-required domain are sent back to their identity provider when their session token expires. The bug is that the sign-in page flashes and the SSO prompt fires several times in a row instead of once.
When the token expires the app sends a burst of requests at the same time (reconnectApp, openApp, AuthenticatePusher and so on) and they all come back
407in the same tick. Inreauthenticate()a SAML-required account cannot silently reauthenticate because there is no stored password, so every one of those407s takes theaccount.isSAMLRequiredbranch and callsredirectToSignIn()on its own. Each call clears the session and re-mounts the sign-in page, and each mount re-initiates SAML, so the user sees the screen flash and the IdP prompt open several times.Production logs from an affected account (anonymized) show the pattern. On reconnect, three commands hit the expired token and
407inside the same half second:Each of those
407s then drives its ownreauthenticate()->redirectToSignIn(), firing in the same instant (this is the flash):This coalesces those redirects. The first expired request in a burst queues the redirect and the rest are skipped, until the next SAML sign-in begins. The reset is keyed on the existing
RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKENflag, which flips on when a new short-lived-token exchange starts, so a later token expiry is still allowed to redirect. The result is one clean redirect and one SSO prompt per expiry instead of a flashing burst.This does not change the redirect behaviour for non-SAML accounts, and it does not change how often a session expires.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/659015
PROPOSAL: N/A (internal fix)
Tests
407.Offline tests
SAML sign-in needs a network connection. While offline the requests never reach the server, so no
407and no reauthentication redirect is triggered. There is no offline-specific behaviour for this change.QA Steps
Same as the Tests section above. Requires a test account on a domain with SAML/SSO set to required, since the flow cannot be reproduced without one.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.