[HOLD for payment 2024-03-14] [$500] Back button in SAML page doesn't work to go back to log in page

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Found when validating PR: #35294

Version Number: 1.4.42-1
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal team
Slack conversation:

Action Performed:

1. Log in to OD with [email protected]
   account
2. Enable SAML required to sign in
3. Go to ND and enter [email protected]
   in login field
4. Press Continue
5. Confirm you're automatically redirected to your SSO providers login page (with the interstitial page being briefly shown)
6. Disable SAML toggle in OD
7. Try to go back to login page in ND

Expected Result:

Back button should work & user able to go back to login screen

Actual Result:

Back button in SAML page doesn't work to go back to log in page. Even killing the app does not work. User has to uninstall & reinstall app to get the login screen

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6381350_1708040833878.az_recorder_20240215_175134.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013f786b4a5d463af8
• Upwork Job ID: 1758563440991674368
• Last Price Increase: 2024-02-23
• Automatic offers:
• ikevin127 | Contributor | 0

[melvin-bot]

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[izarutskaya]

I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[joekaufmanexpensify]

I can reproduce this. The back button doesn't work. However, the swiping-back gesture does work for me. The back button should work if it's there though.

2024-02-16_10-12-45.mp4

[joekaufmanexpensify]

@NikkiWines I see ou led the SAML implementation. Thoughts on if this needs to be internal?

[NikkiWines]

Should be fine to be external! No back end code should need to change for this to be fixed.

[joekaufmanexpensify]

Cool cool. TY!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013f786b4a5d463af8

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

[rayane-d]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Back button in SAML page doesn't work to go back to log in page.

What is the root cause of that problem?

The cause is here:
https://github.com/Expensify/App/blame/main/src/pages/signin/SAMLSignInPage/index.native.tsx#L57-L63

What changes do you think we should make in order to solve the problem?

here:

App/src/pages/signin/SAMLSignInPage/index.native.tsx

Line 61 in b7418b9

Navigation.navigate(ROUTES.HOME);

we should go back instead of trying to navigate to the home route.

<HeaderWithBackButton
    title=""
    onBackButtonPress={() => {
        Session.clearSignInData();
        Navigation.goBack();
    }}
/>

What alternative solutions did you explore? (Optional)

N/A

[allroundexperts]

@rayane-djouah Why does navigation home does not work?

[brandonhenry]

@allroundexperts

Proposal

Please re-state the problem that we are trying to solve in this issue.

Users are unable to navigate back to the login screen from the SAML SSO page. This issue persists even after restarting the app, requiring a complete reinstallation to reset the login state.

What is the root cause of that problem?

Took a lot of setup in order to reproduce the error. Managed to do so on real iPhone XS as well.

The root issue, is caused by the fact that SAML isn't properly configured to work correctly. When a user logs in via SAML, they are taken to the SAMLSignInPage component.

This page is incorrectly receiving props which results in the webview having the incorrect url structure and broken SAML page

The reason you cannot go back to the sign in page and you are stuck even after an app reload, is because no matter what, anytime you come back to the app, it will always be set to this URL:

App/src/pages/signin/SAMLSignInPage/index.native.tsx

Line 19 in b7418b9

const samlLoginURL = `${CONFIG.EXPENSIFY.SAML_URL}?email=${credentials?.login}&referer=${CONFIG.EXPENSIFY.EXPENSIFY_CASH_REFERER}&platform=${getPlatform()}`;

Which, I am seeing turn out as https://www.expensify.com/authentication/saml/login?email=undefined&referer=ecash&platform=android

That must mean that credentials is being lost or reset at some point between clicking "Use single sign-on" and reaching the SAML page. That said, once we are on the SAML page, assuming credentials is now corrupt / not what we are expecting, we can't expect the navigate back to home button to work becuase the Session.clearSignInData is likely not clearing the right data we need to produce any re-render.

<HeaderWithBackButton
      title=""
      onBackButtonPress={() => {
          Session.clearSignInData();
          Navigation.navigate(ROUTES.HOME);
      }}
  />

What changes do you think we should make in order to solve the problem?

Potential Backend Issue
So I think this is both a frontend and backend issue. Seeing as the actual SAML integration is probably handled backend?

Main question on this is, should the backend be returning undefined for the email if it messes up? Why is it returning undefined?

To fix on the frontend:

update handleNavigationStateChange to check for email to be undefined. If so, kick us back to the login page immediately. In this function, we check for the presence of an error param in the URL, which doesn't seem to be present when this issue occurs. So, we should also check to make sure email isn't undefined. If it is always returning undefined, this is partly a backend issue.

 const handleNavigationStateChange = useCallback(
        ({url}: WebViewNativeEvent) => {
            // ... other code
            // If the login attempt is unsuccessful OR email from backend is undefined, set the error message for the account and redirect to sign in page
            if (searchParams.has('error') || searchParams.get('email') === 'undefined') {
                Session.clearSignInData();
                Session.setAccountError(searchParams.get('error') ?? 'Domain Not Setup for SAML');
                Navigation.goBack(ROUTES.HOME);
            }
        },
        [credentials?.login, shouldShowNavigation, account?.isLoading],
    );

Original:

App/src/pages/signin/SAMLSignInPage/index.native.tsx

Line 25 in b7418b9

const handleNavigationStateChange = useCallback(

[joekaufmanexpensify]

Still reviewing proposals

[allroundexperts]

That must mean that credentials is being lost or reset at some point between clicking "Use single sign-on" and reaching the SAML page

Why is it being lost?

assuming credentials is now corrupt / not what we are expecting, we can't expect the navigate back to home button to work becuase the Session.clearSignInData is likely not clearing the right data we need to produce any re-render

@brandonhenry Can you elaborate on this point? How is the corrupt session data linked with the re-render needed for showing the home page?

[joekaufmanexpensify]

Proposals still in review

[brandonhenry]

Finally got android and ios builds running locally. Digging deeper here

[brandonhenry]

So found the culprit @allroundexperts - its definitely that we need to do:

Navigation.goBack(ROUTES.SAML_SIGN_IN);

Screen.Recording.2024-02-21.at.5.56.54.PM.mov

.navigate is not working properly in this instance. I placed logs at the place of redirection and it never reaches that point. Still digging on why it's failing. Will update soon

I did notice that the SAMLSignInPage is still broken though - based on the logs in pic at bottom, here are my findings:

The log indicates that the credentials are being cleared at some point during the SAML sign-in process. Here are a few key points from the log that might help explain why the credentials and login information are being lost:

1. BeginSignIn API Call: The BeginSignIn API is called with an email, and the request is completed successfully (jsonCode: 200) meaning the initial sign-in process starts correctly.

2. Navigating to SAML Sign-in: The app navigates to the SAML sign-in route ("/sign-in-with-saml"), which indicates that the SAML sign-in process is being initiated.

3. Clearing Sign-in Data: The log shows multiple instances of "Clearing sign in data" immediately following the "Handling SAML navigation change" messages. This suggests that some part of the SAML sign-in process or navigation logic is triggering the clearing of sign-in data, including credentials.

4. Empty Credentials: After the sign-in data is cleared, the log shows "Credentials: - """, indicating that the credentials are indeed empty at this point.

[brandonhenry]

Took a fair amount of digging, but finally figured this one out. We don't need to use .goBack

@allroundexperts

Updated Proposal

Please re-state the problem that we are trying to solve in this issue.

Users are unable to navigate back to the login screen from the SAML SSO page. This issue persists even after restarting the app, requiring a complete reinstallation to reset the login state.

What is the root cause of that problem?

The issue is caused by the Navigation setup of the PublicScreens stack navigator. Specifically, this line

App/src/libs/Navigation/linkTo.ts

Line 206 in c425c34

const actionForBottomTabNavigator = getActionForBottomTabNavigator(action, rootState, policyID, shouldNavigate);

When you press the back button, actionForBottomTabNavigator is calculated and returned as an empty object. This causes the navigation to early return and never actually do anything

What changes do you think we should make in order to solve the problem?

Instead of early returning there, and breaking navigation - we should instead check if the actionForBottomTabNavigator variable is there and send the dispatch and keep running the rest of the code below

if (actionForBottomTabNavigator) root.dispatch(actionForBottomTabNavigator);

This fixes navigation. The main reason I think navigation is setup like this is because it needs to work the same for PublicScreens and AuthScreens so the navigate function works the same for both. However, I don't think we have any actionForBottomTabNavigator to calculate when not signed in (going from SAML back to the homepage). This was causing some properties to not be available in the navigation route, thus resulting in actionForBottomTabNavigator being null

Specifically bottomTabNavigatorRoute.state is undefined on this line and thats why it returns null

App/src/libs/Navigation/linkTo.ts

Line 80 in c425c34

if (!bottomTabNavigatorRoute || bottomTabNavigatorRoute.state === undefined || !action || action.type !== CONST.NAVIGATION.ACTION_TYPE.NAVIGATE) {

[joekaufmanexpensify]

Proposals still in review

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[allroundexperts]

Thanks for the updated proposal @brandonhenry. I still don't understand why the action has a payload of NAVIGATORS.BOTTOM_TAB_NAVIGATOR in the first place? Shouldn't we just have an action of type CONST.NAVIGATION.ACTION_TYPE.NAVIGATE?

[allroundexperts]

@joekaufmanexpensify I think it would be good if we can maybe raise the bounty to get more proposals.

[joekaufmanexpensify]

Not overdue

[ikevin127]

PR #37618 ready for review!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.48-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Back button in SAML page doesn't work to go back to log in page #37618

If no regressions arise, payment will be issued on 2024-03-14. 🎊

For reference, here are some details about the assignees on this issue:

• @allroundexperts requires payment through NewDot Manual Requests
• @ikevin127 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR: [Wave 8] Ideal nav  #33280
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/379028

[joekaufmanexpensify]

@allroundexperts can you complete BZ so we can prep to issue payment?

[joekaufmanexpensify]

Bumped in slack

[allroundexperts]

Reviewer checklist

1. [Wave 8] Ideal nav  #33280
2. The above PR changed the structure of navigation stacks which caused this. There isn't any single line I was able to point to.
3. N/A
4. Regression test would be useful here. The test steps given in the OP should be clear enough.

[ikevin127]

@joekaufmanexpensify 👋 Any blockers from completing the payment here ?

[joekaufmanexpensify]

Nope. I will complete it today.

[joekaufmanexpensify]

Checklist is complete. All set to issue payment. We need to pay:

• @ikevin127 $500 for PR via upwork.
• @allroundexperts $500 for C+ review via NewDot.

[joekaufmanexpensify]

@ikevin127 $500 sent and contract ended!

[joekaufmanexpensify]

@allroundexperts could you please request $500 via NewDot and confirm here once complete?

[allroundexperts]

All done. Feel free to close!

[joekaufmanexpensify]

TY!

[JmillsExpensify]

$500 approved for @allroundexperts based on summary.