Fix mobile SAML/Okta double login on session resume#94082
Draft
MelvinBot wants to merge 1 commit into
Draft
Conversation
…owser Co-authored-by: Rory Abraham <roryabraham@users.noreply.github.com>
6 tasks
Contributor
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Contributor
|
@johncschuster do you want to have your customer retest the saml bug on this AdHoc build? The only thing is they won't be able to install the AdHoc build on iOS, so if that's the only device they have they won't be able to test. |
Contributor
|
Only asking because you said "They would gladly QA this if we need someone to ensure it's working" |
This file contains hidden or 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
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.
Explanation of Change
On mobile, when a SAML/Okta SSO user returns to the app after their IdP session has gone idle and completes re-authentication + MFA in the in-app browser, the app briefly freezes, logs them out, and forces a second login. The second login always succeeds.
Root cause — a race on app resume. Opening the in-app browser (
openAuthSessionAsync) backgrounds the app. When the browser closes and the app resumes, two flows fire concurrently:reconnectApp()runs with the now-expiredauthToken, gets a407 NOT_AUTHENTICATED, and triggersreauthenticate(). For SAML users it takes theaccount.isSAMLRequiredbranch and callsredirectToSignIn(undefined, true)→Onyx.clear(), wiping the session and cancelling in-flight requests.shortLivedAuthTokenand callssignInWithShortLivedAuthToken().reauthenticate()already has a guard that aborts whenisAuthenticatingWithShortLivedTokenis set — but that flag was only set optimistically insidesignInWithShortLivedAuthToken(), which runs after the browser returns. So on resume the guard is stillfalse, reauth wins the race, and the session is cleared before the SAML callback can land the user in. The second login works because the app is then in a clean state with no competing reconnection flow.Fix. Set the
isAuthenticatingWithShortLivedTokenguard (the RAM-only keyRAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN) totruebefore opening the in-app browser, so the guard is active for the entire SAML browser session andreauthenticate()aborts instead of redirecting to sign-in. The guard is reset tofalseon the cancel, error, and callback-failure paths; on success,signInWithShortLivedAuthToken()'s optimistic/finally data takes over flag management as before.The guard reliably aborts during the SAML flow because the page is only reached while
account.isLoadingistrue(SignInPageonly navigates to the SAML page whenshouldInitiateSAMLLogin, which requires!!account.isLoading), andreconnectApp()/openApp()operate on the separateIS_LOADING_APPkey, so resume does not clearaccount.isLoading. This satisfies the guard'saccount?.isLoading !== falsecondition inreauthenticate(). The new flag is a separate key fromaccount.isLoading, so the SAML success check inSAMLSignInPageis unaffected.Changes are native-only (
SAMLSignInPage/index.native.tsx), matching where the bug occurs; desktop uses a different SAML implementation.AI tests run locally by MelvinBot (informational — does not replace human Tests/QA)
npx prettieron changed files — pass (unchanged)npm run lint-changed— passnpm run typecheck(tsc) — passnpm test -- tests/actions/SessionTest.ts— pass (32 tests, incl. new test assertingsetIsAuthenticatingWithShortLivedToken(true)makesreauthenticate()abort)npm test -- tests/ui/SessionTest.tsx— pass (2 tests)npm run react-compiler-compliance-check check src/pages/signin/SAMLSignInPage/index.native.tsx— COMPILEDThis change can only be fully validated on a device with a real Okta SAML session, which MelvinBot cannot operate.
Fixed Issues
$ #86705
PROPOSAL: #86705 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
// TODO: The human co-author must fill out the offline tests.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari