[Payment due @huult] [Internal QA] Trigger SAML sign-in when trying to re-auth a SAML required account#87368
[Payment due @huult] [Internal QA] Trigger SAML sign-in when trying to re-auth a SAML required account#87368s77rt wants to merge 3 commits intoExpensify:mainfrom
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@ahmedGaber93 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfd4f8baf3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| keysToPreserve.push(ONYXKEYS.CREDENTIALS); | ||
| keysToPreserve.push(ONYXKEYS.ACCOUNT); | ||
| Onyx.merge(ONYXKEYS.CREDENTIALS, {login: currentSessionEmail, autoGeneratedLogin: null, autoGeneratedPassword: null}); | ||
| Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: true}); |
There was a problem hiding this comment.
Avoid leaving account loading for SAML reauth
Setting account.isLoading to true here breaks the native SAML callback path: src/pages/signin/SAMLSignInPage/index.native.tsx only completes sign-in when !account?.isLoading (line 57). During token-expiry reauthentication for SAML-required users, this flag is never reset before the callback runs, so even a valid shortLivedAuthToken takes the failure branch (clearSignInData + error), preventing successful re-login on iOS/Android.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Still working on it. I think we probably clear the isLoading somewhere but need to check to be sure
There was a problem hiding this comment.
OK, let me know when this PR is ready.
There was a problem hiding this comment.
I was not really able to reproduce. I think we should move forward and test this in staging/production. The isLoading must be true as that's required in order to redirect the user to the SAMLSignInPage in the first place. Since this is already working on native so that must be the case.
There was a problem hiding this comment.
@s77rt When do we need isLoading? SAMLSignInPage already uses SAMLLoadingIndicator to wait for the portal login redirect.
There was a problem hiding this comment.
ahh this one:
App/src/pages/signin/SignInPage.tsx
Line 103 in b538fb3
There was a problem hiding this comment.
In SignInPage, shouldInitiateSAMLLogin depends on the isLoading to be true in order to redirect to SAML
|
@ahmedGaber93 Are you able to test it? @s77rt please check out the bot comments |
|
On it... I'm working on fixing native build locally then I will test on native |
|
Hmm the SAML login appears to be broken on main? or is it just me cc @ahmedGaber93 Screen.Recording.2026-04-15.at.9.43.29.PM.mov |
|
I’m trying to use the email address mentioned in your video about production, but I’m encountering an error.: @s77rt Does this domain have SAML enabled?
|
|
@ahmedGaber93 I think you will need a personal domain |
Can you setup SAML on that and try again? From linked Slack thread Fedy used Auth0 so maybe it's worth trying
Not totally sure, I think the credentials are not an issue but |
|
Seems like that is not available to C+ @ahmedGaber93 would be great if you could try to setup a private domain with SAML to test this 🙏 Or ask if any other C+ already has that setup and could take over. Thanks |
|
@mountiny Asked on slack for taking over. CC @fedirjh @parasharrajat |
|
I reviewed this PR and tried to set up SAML using the email contact@huutech.com, but when I log in, it always redirects to production due to backend configuration. So I think we can't test this in local dev unless we hard-code it or update the backend. |
|
Ah riight I forgot about that. Can you confirm that SAML login works for you in production (in native) |
ScreenRecording_04-20-2026.07-30-08_1.movI tested this in production and it works. |
| if (account?.isSAMLRequired) { | ||
| Log.info(`[Reauthenticate] Redirecting to Sign In because SAML is required`); | ||
| setIsAuthenticating(false); | ||
| redirectToSignIn(undefined, true); |
There was a problem hiding this comment.
| redirectToSignIn(undefined, true); | |
| redirectToSAMLSignIn().then(() => false); |
I think we should introduce a dedicated function for redirecting to the SAML Sign-In page. This would prevent the extra redirect from sign-in → SAML indicator and simplify the sign-in page logic.
We likely only need to redirect to SAML Sign-In when re-auth is triggered by user action, rather than coupling it with the general sign-in flow. Example change here: link. What do you think?
There was a problem hiding this comment.
Not necessary. I think the central sign in flow makes sense as well so let's not change it unless we have to
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. |

Explanation of Change
For SAML required accounts, when a request fails due to auth token being expired, we cannot reauthenticate using the stored login (only SAML login is allowed) thus instead of doing that, we should redirect the user to their IdP. If their IdP is still active they will get redirected back to ND just fine, otherwise they have to login in IdP first.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/609978
PROPOSAL:
Tests
Screen.Recording.2026-04-08.at.1.01.50.PM.mov
Offline tests
n/a
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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