[C+ Checklist needs completion] [$1000] Web - App allows user to open request money link of other user and displays amount page twice

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open the app and login with user A
2. Open user C report, click on plus, click on request money, enter any amount, click next and copy the URL
3. Send the URL to user B
4. Open the app in any other eligible device and login with user B
5. Open user A report and open link sent by user A
6. Observe that app allows to open link of request money between 2 different users, enter amount and click next
7. Observe that it again displays amount page of request money

Expected Result:

App should not allow you to open request money confirmation page for other users

Actual Result:

App allows you to open request money confirmation page for other users

Workaround:

Unknown

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.46-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/bb70f199-4d8e-48d4-b1ff-407beb1ca778

https://github.com/Expensify/App/assets/93399543/5a5ebc7b-3dc4-4a0e-be13-1aa9fd5a2e22

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690396372826599

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017939880358ebbb25
• Upwork Job ID: 1684841055911837696
• Last Price Increase: 2023-08-18

Issue Owner

Current Issue Owner: @greg-schroeder

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[neonbhai]

Proposal

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

App allows user to open request money link of other user and displays amount page twice

What is the root cause of that problem?

This happens because the props.iou.id is not equal to moneyRequestID (constructed from) reportID of the component, which comes from the url, since the url that initiated the component was from a different account, component resets while editing by checking the value in the variable shouldReset here: https://github.com/Expensify/App/blob/975146fa30bc65a19df8e0cca525af32ce2ceda5/src/pages/iou/steps/MoneyRequestAmountPage.js#L263

Having it reset while editing dirupts the UX of the app. A refresh should ideally not be triggered while editing

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

We should show FullPageNotFoundView when accessing a url meant for a different account.

Edit: Since the pages went through a refactor

• For Request Money/Split Bill Flow:

  • To do this we need to add a check similar to shouldReset logic here in the shouldShow prop of the component: https://github.com/Expensify/App/blob/5510a952eec41e19910a0536b4e2391a0a4aac3e/src/pages/iou/steps/NewRequestAmountPage.js#L185
  • And a BlockingView on the content sent from here
  • Note: Since development for the scan_receipt feature is going on, content has been moved from the main page so we can no longer check for invalid iou configuration on the main page (MoneyRequestSelector.js)

• For Confirm Pages:

  • Change the shouldReset logic in MoneyRequestConfirmPage to show a notFound Page when the link is invalid: https://github.com/Expensify/App/blob/ab99bf4748d03e309257e24a8828b48a118ba5b9/src/pages/iou/steps/MoneyRequestConfirmPage.js#L94-L95

• For Participants Page:

  • Instead of going back here, we would render a FullPageNotFoundView on the condition of shouldReset: props.iou.id !== moneyRequestId;: https://github.com/Expensify/App/blob/ab99bf4748d03e309257e24a8828b48a118ba5b9/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L78

• For Currency Selection Page:

  • Currently on Confirming currency, The page is dismissed if the Nav stack has <= 1 page, which means the user somehow landed on the page directly (via refresh/direct-link). To show page not found: We will wrap this component in FullPageNotFoundView and use this check to trigger it: https://github.com/Expensify/App/blob/ab99bf4748d03e309257e24a8828b48a118ba5b9/src/pages/iou/IOUCurrencySelection.js#L89

• For Money Request Description Page:

  • Add a blocking view over the content instead of triggering a reset, using the shouldReset check on the page: https://github.com/Expensify/App/blob/ab99bf4748d03e309257e24a8828b48a118ba5b9/src/pages/iou/MoneyRequestDescriptionPage.js#L55

What alternative solutions did you explore? (Optional)

We can check if the reportID in the url is one that the user has access to, by using Reports.openReport(route.params.reportID). if this returns null, we would cleanly trigger the FullPageNotFoundView on the pages in the flow.

[greg-schroeder]

app should switch URL back to request money to ensure that amount page is not repeated

I definitely don't think this makes sense. I'm pretty sure this link just shouldn't be accessible, right?

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~017939880358ebbb25

[melvin-bot[bot]]

Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[dukenv0307]

Proposal

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

Web - App allows user to open request money link of other user and displays amount page twice

What is the root cause of that problem?

We don't have a check to display not found page when we access to all request money page in a report.

When we access to request money amount page with invalid ReportID, page will goBack to amount page because the amount is empty

https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequestConfirmPage.js#L80

In amount page because report doesn't found, we go to participant page without reportID after clicking the next button.

https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequestAmountPage.js#L418

And then in participant page, props.iou.id and moneyRequestId is different because the route is missing reportID now. That makes the page go back to amount again https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L72

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

All pages in the step of requesting money should display not found page if the report with the reportID param doesn't exit in Onyx.

We can duplicate all pages in the step of requesting money. One page for the route that doesn't contain reportID, one page for the route that contains reportID. With the page is used for the route that contains reportID, we can use HOC withReportOrNotFound to display not found page easily.

We used this way to display share code page for the current user and reports.

It's a bit complicated so I put it on a branch to keep an eye on if needed https://github.com/dukenv0307/App/tree/fix/23755

What alternative solutions did you explore? (Optional)

We can refactor withReportOrNotFound HOC to help us pass the param to HOC like isRequireReportId. If isRequireReportId is false, we will accept the WrappedComponent for both cases no reportID in param and a valid reportID in the param. If it's true, we will only accept the WrappedComponent for a valid reportID.

export default function (isRequireReportId = true){

   return (WrappedComponent) => {
      .....
   }
}

As @bernhardoj mentioned, we only need to add this HOC to amount page and in this page we only need wrap not found page if the report cannot create request money.

Actually, task flow is the same with money request flow so we can also use this HOC for task flow. And in each task page we only need to display not found page if the report cannot create task.

[bernhardoj]

Proposal

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

The issue here is not that we can open other user's money request URL. Everyone can access /request/new/{reportID}. The only difference is, does the user has access to the reportID? When the user does not have access to it, pressing Next will bring the user back to the amount page again.

What is the root cause of that problem?

When the user press Next, it should be navigated to the participant page. But on the participant page, we will navigate the user back if the amount is empty or the iou ID is not the same as the current ID. The iou ID is constructed from the iou type + report id, for example, request8595261590323281. This ID tells us which request is currently active. With the example before, we can tell that the user is currently doing a money request on the reportID of 8595261590323281. https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js#L72-L79

If the current ID (that is taken from the route params) is different from what we have in Onyx, we want the user to start over the money request flow. This is to prevent the user doing a request with invalid data, for example doing a request money with split bill data (specifically the participants).

In our case, the current money request id is different from what we have in Onyx. When we open the money request with an invalid report id and press Next, it will

1. Set the money request id to request{invalidReportId} https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequestAmountPage.js#L397

2. Navigate to the participants page without a report id. https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequestAmountPage.js#L403-L419

When we arrive on the participants page, because the report id is now missing from the params, the current money request id (request) is different from what we have in Onyx (request{invalidReportId}), which "kicks" us back to the amount page.

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

We can allow the user to keep continue the money request/split bill with an invalid report id. If the report id is invalid, we will just treat it as a new money request that is initiated by the FAB. Here is what we need to do:

1. Always pass the report id here https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequestAmountPage.js#L418
2. On the money request confirm page, we need to do 2 changes: a. Modify participant should only be enabled if the report id is valid https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequestConfirmPage.js#L211-L216 canModifyParticipants={!_.isEmpty(props.report.reportID)} b. Currently, we assume if reportID exist in the param, we are initiating it from a report, but that's not the case if we directly access it from the URL. So, we should do the same as above, that is replace the reportID from param with props.report.reportID https://github.com/Expensify/App/blob/b2381f6c60acd4beca34b83c65a898eefb0594d6/src/pages/iou/steps/MoneyRequestConfirmPage.js#L115-L128

What alternative solutions did you explore? (Optional)

If we don't want to allow the user to start a request with an invalid report ID, then we can show the not found page if the reportID in the param exists but from props.report does not exist. (we will also need some conditions that are used in withReportOrNotFound)

const shouldShowNotFound = !IOUUtils.isValidMoneyRequestType(iouType) || (reportID && (!lodashGet(props.report, 'reportID') || !ReportUtils.canAccessReport(props.report, props.policies, props.betas)));
<FullPageNotFoundView shouldShow={shouldShowNotFound}>

Notice the above condition, we want to check this condition

(!lodashGet(props.report, 'reportID') || !ReportUtils.canAccessReport(props.report, props.policies, props.betas))

only if we have reportID in the params.

In case the report collection is not ready yet because of the app loading, we should show a loading indicator.

const shouldShowLoadingIndicator = reportID && props.isLoadingReportData && !lodashGet(props.report, 'reportID');
shouldShowLoadingIndicator && <FullScreenLoadingIndicator /> 

Same as above, we want to check props.isLoadingReportData && !lodashGet(props.report, 'reportID') only if we have reportID in the params.

(add the above codes in MoneyRequestSelectorPage)

We only need to do this on the amount page because there is no way the user can go further steps without going through the amount page.

(don't forget to set default props of isLoadingReportData to true)

[greg-schroeder]

Awaiting proposal review from @sobitneupane

[greg-schroeder]

Same as above

[sobitneupane]

The issue is quite complicated. Going to review it shortly.

[sobitneupane]

Thanks for the proposal @neonbhai.

Can you please extend your proposal to include other pages in request money and split bill flow.

[sobitneupane]

Thanks for the proposal @dukenv0307.

I think there are easier ways to deal with the issue. Can't we go with some simpler solution like the one proposed by @neonbhai here?

[sobitneupane]

Thanks for the proposal @bernhardoj.

But I think expected behavior is to not allow user to access such reports as suggested here.

[neonbhai]

@sobitneupane okay! will extend proposal to other pages shortly

[dukenv0307]

@sobitneupane When we open a deeplink of confirm page with an exist reportID we should reset it to the amount page to enter an amount before going to confirm page.

[bernhardoj]

But I think expected behavior is to not allow user to access such reports

This expected behavior is included in my alternative solution

[melvin-bot[bot]]

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

[sobitneupane]

Any update on this @neonbhai?

[greg-schroeder]

bump @neonbhai were you able to update your proposal?

[sobitneupane]

Have not received any update from @neonbhai. Going to review other proposals.

[neonbhai]

@sobitneupane @greg-schroeder

I sincerely apologize for being offline. I lost my laptop while travelling. Reached home today morning. I understand that time is valuable here, and I apologise for any inconvenience my delay may have caused.

I have updated the proposal, but looks like the money request flows went through two refactors this week, so some of the code has moved and the file names changed, hence updated the logic accordingly

Once again, I hope you can forgive me for not being able to update proposal.

[neonbhai]

Proposal

Updated

[melvin-bot[bot]]

@sobitneupane @greg-schroeder this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

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

[greg-schroeder]

Can you check updated proposal here @sobitneupane?

[sobitneupane]

When we open a deeplink of confirm page with an exist reportID we should reset it to the amount page to enter an amount before going to confirm page.

@dukenv0307 I don't think that is the case now. We can directly request money from confirm page.

[dukenv0307]

@sobitneupane If we display not found page using shouldReset condition in confirm page, the page also display not found page with an exist reportID when we go to this page by deeplink.

[sobitneupane]

If we display not found page using shouldReset condition in confirm page, the page also display not found page with an exist reportID when we go to this page by deeplink.

I got your point. It is definitely not expected scenario.

@neonbhai Your thoughts?

[bernhardoj]

I agree that we can't use shouldReset as the condition to show the not found page. shouldReset is not available on the initial amount page. If shouldReset is true, the user will be navigated to the amount page, so it's impossible for the user to see the not found page on further pages, for example, the confirmation page.

shouldReset is a condition to tell the app to reset the money request info in Onyx because we are trying to initiate a different type of request from what we have in Onyx, not to tell the app that the report does not exist.

@sobitneupane Btw, any thoughts on my alternative solution?

[sobitneupane]

@bernhardoj Will the alternative solution solution work for other pages like confirmation page?

[bernhardoj]

If shouldReset is true, the user will be navigated to the amount page, so it's impossible for the user to see the not found page on further pages, for example, the confirmation page.

If you mean whether we will see the not found page on the confirmation page or not, no, because of the above reason. We will see the not found page on the amount page instead.

[dukenv0307]

shouldReset is a condition to tell the app to reset the money request info in Onyx because we are trying to initiate a different type of request from what we have in Onyx, not to tell the app that the report does not exist.

We can use HOC as I mentioned.

[sobitneupane]

Looks like we don't have any simple way of solving the issue. Still, I am rooting for some simple solution. @dukenv0307 Is there any way to simplify your solution? I am not very much sold by the idea of duplicating the pages.

[bernhardoj]

@sobitneupane is there any problem with my alternative solution? Is your concern about the auto navigate to the amount page?

[sobitneupane]

Yup @bernhardoj. I think we should simply show not found page if the user don't have access to the report as suggested here.

[dukenv0307]

@sobitneupane I think we can only duplicate the amount page. Other page will reset to this page to display.

[bernhardoj]

My solution did show the not found page but on the amount page. Regarding the auto-navigate behavior, that is the expected behavior. So, currently, there will be 2 cases where the auto navigate (to amount page) will run.

1. When a user is trying to access further steps without completing the required previous step, for example: A user tries to directly open /request/new/confirmation without entering any amount and selecting a participant.

2. The money request ID in Onyx is different from the current ID from route params. This happens when we first initiate a request money flow (ID is request), then we try to directly access split bill flow (ID is split) /split/new/confirmation.

We disallow those 2 cases to happen, so we redirect the user back to the amount page.

So, with my alternate solution, when a user is accessing /request/new/confirmation/{unknownReportID} (and either case above happens), they will be navigated to /request/new/{unknownReportID} and a not found page will be shown.

We shouldn't change the redirect behavior.

[dukenv0307]

Or we can create a new HOC for request money page

[dukenv0307]

@sobitneupane We can create a new HOC here to also control the loading when we go the page by deeplink before login. In the HOC we will do nothing if the route doesn't contain reportID param. If not we will display loading page if the data is loading and then display not found page if the report cannot access or not found. We also can use the HOC for task flow because these also use for two different URL. This HOC will almost similar to withReportOrNotFound, only it will do nothing without the reportID param.

[sobitneupane]

@bernhardoj I don't think I get you completely. So, If user access confirmation page of an inaccessible reportID, user will be redirected to amount page with not found message. Is that the case? I am not sure how it will look like.

[bernhardoj]

Yes. Here is the video of that:

https://github.com/Expensify/App/assets/50919443/66f1082c-8d09-450c-8f24-fd69b404fbfa

If you want to try it, add the code to MoneyRequestSelectorPage. I updated my proposal to mention this too.

[melvin-bot[bot]]

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

[greg-schroeder]

Proposal review underway

[sobitneupane]

@bernhardoj Thanks for the update.

I think it's fine to redirect to money request page before showing Not Found Page. If it works for split bill flow from FAB, request money flow from FAB, request money flow from report Action, split bill flow from FAB, Money Selector Page for user having (not having) access to the report, Confirmation Page, Description Page, etc, we can go with your proposal.

@bernhardoj Can you please confirm it works in all above cases? We need to include all above instances in test as well..

[dukenv0307]

@sobitneupane Do you think about my comment here https://github.com/Expensify/App/issues/23755#issuecomment-1682158877?

[sobitneupane]

@dukenv0307 If the case (also control the loading when we go the page by deeplink before login) can be handled with minimum change, let's not go for HOC if it doesn't have many use cases.

[bernhardoj]

Because we only change the amount page and related to the report id, I think the minimum test needed is:

1. Open either request money/split bill from FAB and verify the page shows normally (pass)
2. Open either request money/split bill with invalid report id (either from chat or deep link) and verify the page shows not found (pass)
3. Open either participant/confirmation/description/edit with an invalid report id (either from chat or deep link) and verify the page shows not found (pass)

[dukenv0307]

@sobitneupane For the case reportID that exists, we also should check whether the report can create a request money or can create a split bill or not.