[$250] Expense - User invited to the expense report can hold the expenses

[IuliiaHerets]

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

---

Version Number: v9.0.26-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp https://expensify.testrail.io/index.php?/tests/view/4901045
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. [User A] Submit two expenses to User B
3. [User A] Click on the expense preview in the main chat to go to expense report
4. [User A] Send a user mention containing User C
5. [User A] Click Invite from the whisper message
6. [User A] Send a message in the transaction thread
7. [User C] Open the invited expense report
8. [User C] Click on any expense.
9. [User C] Click on the header > Hold.
10. [User C] Hold the expense.

Expected Result:

Invited user (User C) should not be able to hold the expenses.

Actual Result:

Invited user can hold the expenses.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6586341_1724925332136.20240829_174948.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021831812891788709823
• Upwork Job ID: 1831812891788709823
• Last Price Increase: 2024-09-19

Issue Owner

Current Issue Owner: @abdulrahuman5196

[melvin-bot]

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

@abekkala FYI 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

[Krishna2323]

Proposal


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

Expense - User invited to the expense report can hold the expenses

What is the root cause of that problem?

The canHoldUnholdReportAction function does not return the values correctly, for canHoldOrUnholdRequest constant we haven't included (isAdmin || isActionOwner || isApprover) check.

App/src/libs/ReportUtils.ts

Lines 3071 to 3085 in a37a613

	const isApprover = isMoneyRequestReport(moneyRequestReport) && moneyRequestReport?.managerID !== null && currentUserPersonalDetails?.accountID === moneyRequestReport?.managerID;
	const isAdmin = isPolicyAdmin(moneyRequestReport.policyID ?? '-1', allPolicies);
	const isOnHold = TransactionUtils.isOnHold(transaction);
	const isScanning = TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction);
	const isClosed = isClosedReport(moneyRequestReport);
	const canModifyStatus = !isTrackExpenseMoneyReport && (isAdmin || isActionOwner || isApprover);
	const canModifyUnholdStatus = !isTrackExpenseMoneyReport && (isAdmin || (isActionOwner && isHoldActionCreator) || isApprover);
	const isDeletedParentAction = isEmptyObject(parentReportAction) || ReportActionsUtils.isDeletedAction(parentReportAction);
	const canHoldOrUnholdRequest = !isRequestSettled && !isApproved && !isDeletedParentAction && !isClosed;
	const canHoldRequest = canHoldOrUnholdRequest && !isOnHold && (isRequestIOU || canModifyStatus) && !isScanning && !!transaction?.reimbursable;
	const canUnholdRequest =
	!!(canHoldOrUnholdRequest && isOnHold && !TransactionUtils.isDuplicate(transaction.transactionID, true) && (isRequestIOU ? isHoldActionCreator : canModifyUnholdStatus)) &&
	!!transaction?.reimbursable;

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


We have few options to resolve this:

1. Update the canHoldOrUnholdRequest constant to !isRequestSettled && !isApproved && !isDeletedParentAction && !isClosed && (isAdmin || isActionOwner || isApprover);
2. Update the (isRequestIOU || canModifyStatus) part in canHoldRequest to (isRequestIOU && canModifyStatus) or just remove isRequestIOU, as it seems redundant.
   

   App/src/libs/ReportUtils.ts

   Line 3082 in a37a613

   const canHoldRequest = canHoldOrUnholdRequest && !isOnHold && (isRequestIOU || canModifyStatus) && !isScanning && !!transaction?.reimbursable;

3. We can early return when !isAdmin && !isApprover && !isActionOwner is true.

    if (!isAdmin && !isApprover && !isActionOwner) {
        return {canHoldRequest: false, canUnholdRequest: false};
    }

NOTE: Minor refactor may be required if we go with any of the solution above.

What alternative solutions did you explore? (Optional)

Result

[melvin-bot]

@abekkala Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@abekkala Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

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

[melvin-bot]

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

[raza-ak]

Proposal

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

Expense - User invited to the expense report can hold the expenses

What is the root cause of that problem?

The problem arises because the system currently does not properly verify if the logged-in user is the same as the creator of the expense. This results in users who did not create the expense being able to access or hold it, violating access control rules.

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

To fix this issue, we need to update the function responsible for opening the "hold" modal. The change will ensure that access is granted only if the email of the logged-in user matches the email of the user who created the expense.The following changes in the component should resolve the issue:

Update Access Logic: Modify the function that handles opening the hold modal to verify that the logged-in user's email matches the expense creator's email (reportAction.email).

Conditional Access: Implement conditional logic to ensure that only users with matching emails are allowed to proceed.

Following changes will fix the issue:

365193203-876ce5fa-0b1b-4b19-b67a-54cfeeb9fef0.1.mp4

[melvin-bot]

@abekkala, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[abekkala]

@abdulrahuman5196 can you please review the proposal?

[melvin-bot]

@abekkala, @abdulrahuman5196 Eep! 4 days overdue now. Issues have feelings too...

[abekkala]

@abdulrahuman5196, do you have any comments on the proposal you've reviewed?

[melvin-bot]

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

[melvin-bot]

@abekkala, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

[abdulrahuman5196]

Sorry for the delay. Checking now.

[abdulrahuman5196]

@abekkala The backend API to hold/unhold is also passing. @Krishna2323 solution could fix the problem in frontend but still for backend we need internal fix. Could you loop in internal engineer for this issue?

[Krishna2323]

@abdulrahuman5196 @abekkala, can't we just add the C+ reviewed label so that the internal engineer is auto assigned?

[abekkala]

@Krishna2323 no, hot picks require an internal engineer volunteer to pick up (typically someone who would be familiar with the feature) vs. an engineer being auto assigned.

[melvin-bot]

@abekkala, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@abekkala, @abdulrahuman5196 Still overdue 6 days?! Let's take care of this!

[melvin-bot]

@abekkala, @abdulrahuman5196 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[melvin-bot]

@abekkala, @abdulrahuman5196 10 days overdue. Is anyone even seeing these? Hello?

[abekkala]

still waiting on internal pick up

[melvin-bot]

This issue has not been updated in over 14 days. @abekkala, @abdulrahuman5196 eroding to Weekly issue.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[abekkala]

I'll wait until the 3rd week of attempted retest before closing

[melvin-bot]

This issue has not been updated in over 15 days. @abekkala, @abdulrahuman5196 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mvtglobally]

Issue not reproducible during KI retests. (Third week)

[abdulrahuman5196]

If a contributor has been hired for a job and we decide to close the job before it is successfully completed
50% payment to contributor and C+ if the contributor was 🎀👀🎀 by a C+ or hired/assigned.

@abekkala I think this issue should be partially compensated since were already having an approved proposal but waiting on internal engineer assignment - #48306 (comment).

[melvin-bot]

Triggered auto assignment to @mjasikowski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[abdulrahuman5196]

Sorry @mjasikowski, I think the melvin bot wrongly assigned you since the c+ approved emoji was present

[abdulrahuman5196]

If a contributor has been hired for a job and we decide to close the job before it is successfully completed
50% payment to contributor and C+ if the contributor was by a C+ or hired/assigned.

@abekkala I think this issue should be partially compensated since were already having an approved proposal but waiting on internal engineer assignment - #48306 (comment).

@abekkala Gentle ping on this

[Krishna2323]

@abekkala, friendly bump to check this comment