Unauthorized Message Deletion: The person that's owed money can delete messages of the other iouReport participant.

[kavimuru]

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. Go to web chrome and login with User A
2. Create a group with User B and User C
3. Click on add and split money in the group
This will generate individual message to Recepients User B and C
4. Go to User B and click on the message or go to the chat and click on the split money message
5. Write some messages as User B
6. Go to User A and try to delete the messages.

TRJ Edit:

1. send a request from userA to userB, such that userA is owed money
2. As userB write a couple of messages on the iouReport directly
3. As userA hover over the message(s) and click the trashcan icon

Expected Result:

userA should NOT be able to delete UserB's messages on an iouReport

Actual Result:

UserA is able to delete UserB's messages

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.29-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

Comment here on why I think this is happening. We need to exclude iouReports from the new "admin delete" logic.

unauthorised.deletion.mp4

message.delete.iou.mp4

Expensify/Expensify Issue URL:
Issue reported by: @avi-shek-jha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686343933703009

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017c8b2f6ee327ce37
• Upwork Job ID: 1670890147893735424
• Last Price Increase: 2023-06-19

[melvin-bot]

Triggered auto assignment to @sakluger (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

[hungvu193]

This should be fixed in the backend, since the policy role of the user is "admin" which allowed user to delete comment.

[Pujan92]

Proposal

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

User A Has Ability to Remove User B's Messages in ‘Thread' of Split Money Chat

What is the root cause of that problem?

For the canDeleteReportAction we are allowing the admin role to delete the messages of other participants. As per the slack discussion only for Rooms we need this functionality.
https://github.com/Expensify/App/blob/057a18292aba57a50f791f735793f64d1aba3824/src/libs/ReportUtils.js#L261`

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

We need to add one more condition here which will also check for reportType is a policyRoom or not. If it is policyRoom then only consider the role admin condition else it will return false.

App/src/libs/ReportUtils.js

Line 261 in 057a182

return policy.role === CONST.POLICY.ROLE.ADMIN;

return isUserCreatedPolicyRoom(report) && policy.role === CONST.POLICY.ROLE.ADMIN;

[trjExpensify]

@neil-marcellini you might have some context here from this PR. Looks like something might have slipped through the net which is allowing a member with the admin role to delete other people's comments on an iouReport/in the request thread.

[neil-marcellini]

I thought that was the expected behavior. An admin of the report can delete a member's comment. We can certainly change it but we should update the backend if we do as well.

[trjExpensify]

An iouReport isn't attached to a policy, it's shared between say you and me. Why would you be able to delete my messages because you happen to be an admin on a personal policy? (if I'm reading it correctly)

[Pujan92]

My doubt is why for iouReport we get the attached policyId from the backend onyx response whereas for normal chat report it is policyID: "_FAKE_"

[trjExpensify]

I think that's to do with how we dynamically change the direction of who's currently owed and what currency the reportTotal will be based on the personal policy currency of the person that is owed.

[trjExpensify]

I'll take this from you Sasha. I've been following it up internally and looking into it. I can reproduce this, and confirmed its not the intention of the design. Basically what's happening is that:

1. when you're owed money, you technically "own" the iouReport because we use the person that's owed policyID to determine things like the report output currency
2. As such, the person who's owed is also technically the policy admin of the policy the iouReport is attached to
3. Meaning that because of this new logic to allow admins the right to delete messages, the person that's owed can delete other people's messages.

As I understand it, it'll require a backend change, so I'm going to mark this as internal to fix. I think the solution is to exclude iouReports from this logic entirely.