[Payment due @huult] [Internal QA] Trigger SAML sign-in when trying to re-auth a SAML required account#87368
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".
|
@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) |
| 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. |
| * @param errorMessage Error message to be displayed on the sign in page | ||
| * @param isSAMLReauthentication Whether the redirection was triggered by reauthentication for SAML required account |
There was a problem hiding this comment.
NAB: Technically they are optional
| * @param errorMessage Error message to be displayed on the sign in page | |
| * @param isSAMLReauthentication Whether the redirection was triggered by reauthentication for SAML required account | |
| * @param [errorMessage] Error message to be displayed on the sign in page | |
| * @param [isSAMLReauthentication] Whether the redirection was triggered by reauthentication for SAML required account |
|
🚧 @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. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.62-5 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. The modifications are purely internal to session re-authentication logic ( This doesn't introduce any new user-facing settings, buttons, configuration options, or workflows. The existing help site articles (Set Up SAML SSO and Troubleshoot SAML SSO Login) already document that SAML-required accounts must authenticate via their IdP, which is consistent with this behavior change. No help site changes are required. |
|
@huult Can you please test this? It will take about 2h Steps:
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.64-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.3.64-31 🚀
|
|
🤖 Payment issue created: #89320 |
|
Yes, I’ll try it and share feedback |
Screen.Recording.2026-05-01.at.20.35.53.movI’ll be back at 22:36 (my local time). |
|
@huult How did that go? Also can you please test on native too |
Screen.Recording.2026-05-02.at.21.39.54.mov@s77rt I opened Expensify using the account I logged into yesterday. I clicked around and sent some messages, and everything worked fine. Are you seeing the same, or am I missing something? |
|
Hmm I'm not sure if that tested anything. You should login and keep the tab open then after 2h or so try to navigate to any page |
|
Okay, I’ll wait for the next 2 hours and test again. |
Screen.Recording.2026-05-02.at.23.49.02.mov@s77rt I waited for 2 hours, but I didn’t see the token expire. Do I need to configure anything else? |
|
@huult Ah it looks you didn't configure SAML in ND to be required. Please update it to be required then test again |

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