[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
• 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

request.money.page.twice.mov

Recording.3944.mp4

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]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• 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
• 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.
• 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.
• 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:

App/src/pages/iou/steps/MoneyRequestAmountPage.js

Line 263 in 975146f

const shouldReset = props.iou.id !== moneyRequestID;

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:

    App/src/pages/iou/steps/NewRequestAmountPage.js

    Line 185 in 5510a95

    <FullPageNotFoundView shouldShow={!IOUUtils.isValidMoneyRequestType(iouType)}>

  • 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:

    App/src/pages/iou/steps/MoneyRequestConfirmPage.js

    Lines 94 to 95 in ab99bf4

    	if (_.isEmpty(props.iou.participants) || (props.iou.amount === 0 && !props.iou.receiptPath) || shouldReset) {
    	Navigation.goBack(ROUTES.getMoneyRequestRoute(iouType.current, reportID.current), true);

• For Participants Page:

  • Instead of going back here, we would render a FullPageNotFoundView on the condition of shouldReset: props.iou.id !== moneyRequestId;:

    App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

    Line 78 in ab99bf4

    navigateBack(true);

• 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:

    App/src/pages/iou/IOUCurrencySelection.js

    Line 89 in ab99bf4

    if (_.isEmpty(backTo) || props.navigation.getState().routes.length === 1) {

• For Money Request Description Page:

  • Add a blocking view over the content instead of triggering a reset, using the shouldReset check on the page:

    App/src/pages/iou/MoneyRequestDescriptionPage.js

    Line 55 in ab99bf4

    const shouldReset = this.props.iou.id !== moneyRequestId;

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]

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

[melvin-bot]

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

[melvin-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

App/src/pages/iou/steps/MoneyRequestConfirmPage.js

Line 80 in b2381f6

Navigation.goBack(ROUTES.getMoneyRequestRoute(iouType.current, reportID.current), true);

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

App/src/pages/iou/steps/MoneyRequestAmountPage.js

Line 418 in b2381f6

Navigation.navigate(ROUTES.getMoneyRequestParticipantsRoute(iouType.current));

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

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 72 in b2381f6

const shouldReset = props.iou.id !== moneyRequestId;

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.

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Lines 72 to 79 in b2381f6

	const shouldReset = props.iou.id !== moneyRequestId;
	if (shouldReset) {
	IOU.resetMoneyRequestInfo(moneyRequestId);
	}
	if (props.iou.amount === 0 || shouldReset) {
	navigateBack(true);
	}

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}
   

   App/src/pages/iou/steps/MoneyRequestAmountPage.js

   Line 397 in b2381f6

   IOU.setMoneyRequestId(moneyRequestID);

2. Navigate to the participants page without a report id.
   

   App/src/pages/iou/steps/MoneyRequestAmountPage.js

   Lines 403 to 419 in b2381f6

   	if (props.report.reportID) {
   	// Reinitialize the participants when the money request ID in Onyx does not match the ID from params
   	if (_.isEmpty(props.iou.participants) || shouldReset) {
   	const currentUserAccountID = props.currentUserPersonalDetails.accountID;
   	const participants = ReportUtils.isPolicyExpenseChat(props.report)
   	? [{reportID: props.report.reportID, isPolicyExpenseChat: true, selected: true}]
   	: _.chain(props.report.participantAccountIDs)
   	.filter((accountID) => currentUserAccountID !== accountID)
   	.map((accountID) => ({accountID, selected: true}))
   	.value();
   	IOU.setMoneyRequestParticipants(participants);
   	}
   	Navigation.navigate(ROUTES.getMoneyRequestConfirmationRoute(iouType.current, reportID.current));
   	return;
   	}
   	Navigation.navigate(ROUTES.getMoneyRequestParticipantsRoute(iouType.current));
   	};

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
   

   App/src/pages/iou/steps/MoneyRequestAmountPage.js

   Line 418 in b2381f6

   Navigation.navigate(ROUTES.getMoneyRequestParticipantsRoute(iouType.current));

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
   

   App/src/pages/iou/steps/MoneyRequestConfirmPage.js

   Lines 211 to 216 in b2381f6

   	// The participants can only be modified when the action is initiated from directly within a group chat and not the floating-action-button.
   	// This is because when there is a group of people, say they are on a trip, and you have some shared expenses with some of the people,
   	// but not all of them (maybe someone skipped out on dinner). Then it's nice to be able to select/deselect people from the group chat bill
   	// split rather than forcing the user to create a new group, just for that expense. The reportID is empty, when the action was initiated from
   	// the floating-action-button (since it is something that exists outside the context of a report).
   	canModifyParticipants={!_.isEmpty(reportID.current)}

   
   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
   

   App/src/pages/iou/steps/MoneyRequestConfirmPage.js

   Lines 115 to 128 in b2381f6

   	// IOUs created from a group report will have a reportID param in the route.
   	// Since the user is already viewing the report, we don't need to navigate them to the report
   	if (iouType.current === CONST.IOU.MONEY_REQUEST_TYPE.SPLIT && CONST.REGEX.NUMBER.test(reportID.current)) {
   	IOU.splitBill(
   	selectedParticipants,
   	props.currentUserPersonalDetails.login,
   	props.currentUserPersonalDetails.accountID,
   	props.iou.amount,
   	trimmedComment,
   	props.iou.currency,
   	reportID.current,
   	);
   	return;
   	}

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]

📣 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?

[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.3.88-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

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

If no regressions arise, payment will be issued on 2023-10-30. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @sobitneupane does not require payment (Eligible for Manual Requests)
• @dukenv0307 requires payment
• @dhanashree-sawant requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

• [@dangrous] The PR that introduced the bug has been identified. Link to the PR:
• [@dangrous] 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:
• [@dangrous] 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:
• [@sobitneupane / @dukenv0307] Determine if we should create a regression test for this bug.
• [@sobitneupane / @dukenv0307] 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.
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[greg-schroeder]

It seems there was a regression here: #30148

[dangrous]

Oh am I supposed to do the checklist now? I thought it was still C+s (i.e. @sobitneupane )

[dangrous]

@sobitneupane and @dukenv0307 do you think you can get that checklist done shortly? Then we can get this paid and closed out. Thanks!

[greg-schroeder]

sorry was OOO yesterday, processing today

[greg-schroeder]

Offers sent to @dhanashree-sawant and @dukenv0307

[greg-schroeder]

Just waiting on checklist

[dangrous]

Bumping on the checklist @dukenv0307 and @sobitneupane - do you think you can finish that out this week? Thanks! I can help with the first parts if needed.

[dukenv0307]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: N/A
• 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
• 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
• Determine if we should create a regression test for this bug: Yes
• 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.

[dukenv0307]

Regression Test Proposal:

• Request money with invalid reportID

1. Open a chat report
2. Click on the plus icon > Request money
3. Enter a valid amount and continue
4. Copy the URL of the confirm page
5. Paste the URL to composer, edit the reportID param to an invalid value, and send the message
6. Click on the URL that is sent and verify that the not found page appears

• The report that cannot create the money request

1. Open a chat report
2. Click on the plus icon > Request money
3. Copy the URL of the selector page
4. Paste the URL to composer, edit the reportID param to a report that cannot create the request money. i.e. the ID of the group chat or announce room, and send the message
5. Click on the URL and verify that the not found page appears

• Request money with global FAB

1. Go to FAB > Request money
2. Verify the selector page is displayed without loading
3. Open the participant page via deep link without entering an amount
4. Verify that it will reset to the selector page
5. Repeat step 3 with the confirm page, and merchant page and verify that the result is the same

• Test the HOC for some other components

1. Open the report detail page
2. Edit the URL with an invalid reportID
3. Verify that after loading, the not found page appears
4. Repeat the same step with report participant page, report setting page, report share code page
5. Verify that the result is the same

Do we agree 👍 or 👎

[melvin-bot]

@dangrous, @sobitneupane, @greg-schroeder, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[dangrous]

Tests look good to me!

[greg-schroeder]

Thanks! Filing regression test

[greg-schroeder]

paid, regression test filed

[sobitneupane]

@greg-schroeder Can we get payment summary so I can request payment.

[sobitneupane]

@greg-schroeder Can we get payment summary so I can request payment.

Bump on this @greg-schroeder

[greg-schroeder]

Oh, sorry @sobitneupane

[greg-schroeder]

Went back through the offers from a couple months ago ... $1k issue but there was a regression. Final payments were:

C: $500
C+: $500

So you can make a manual request for this one!

[sobitneupane]

Thanks @greg-schroeder

Requested payment on newDot.

[JmillsExpensify]

$500 payment approved for @sobitneupane based on comment above.