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

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/cffc3706-c7d8-459f-a681-09cd59a9ce71

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[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. https://github.com/Expensify/App/blob/a37a613f8e4db14b087365dea4fb882262dbc385/src/libs/ReportUtils.ts#L3071-L3085

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. https://github.com/Expensify/App/blob/a37a613f8e4db14b087365dea4fb882262dbc385/src/libs/ReportUtils.ts#L3082
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[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

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

https://github.com/user-attachments/assets/b34f7b7a-c4e9-4a6d-a874-0f689d7c1c7d

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

[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? 💸

[abekkala]

Looks like we'll need an internal fix first

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@abekkala, @abdulrahuman5196 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[abekkala]

we need an internal fix first

[melvin-bot[bot]]

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

[abekkala]

waiting on internal - internal and hot pick applied

[Krishna2323]

@abdulrahuman5196 @abekkala, I think we can make the frontend changes first since they are not entirely related to the backend changes. WDYT?

[melvin-bot[bot]]

@abekkala, @abdulrahuman5196 12 days overdue now... This issue's end is nigh!

[abdulrahuman5196]

I think we can make the frontend changes first since they are not entirely related to the backend changes. WDYT?

Yes. I agree we can make both frontend and backend changes in parallel. Still we would need a engineer to approve that? That's why wanted to get eyes from internal engineer on this.

[abekkala]

@Krishna2323

I think we can make the frontend changes first since they are not entirely related to the backend changes. WDYT?

Yes. I agree we can make both frontend and backend changes in parallel. Still we would need a engineer to approve that? That's why wanted to get eyes from internal engineer on this.

yes, this is correct. We are waiting for an Internal eng. to pick this one up

[melvin-bot[bot]]

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

[abekkala]

Not sure why the hot pick label didn't put it in that status??

[melvin-bot[bot]]

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

[abekkala]

still waiting for an internal pick up on this one

[abekkala]

as I'm going ooo until Oct 20 @OfstadC will be watching over this one until I return

STATUS: this is an Internal / Hot Pick - still waiting on someone to pick up for fix. I'm not sure there will be much movement in one week.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[abdulrahuman5196]

still waiting for pick up

[abekkala]

I'm back from ooo - unassigning @OfstadC

[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[bot]]

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

[melvin-bot[bot]]

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

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

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