[$500] Chat - App allows to delete other users messages in # admin of workspace

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.3.85.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
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697532701974139

Action Performed:

Precondition: To test, user will need to have 1 workspace with multiple admins in it

1. Open the app
2. Open # admin of workspace with multiple admins
3. Hover or long press on message from other user
4. Observe that delete comment option is available

Expected Result:

App should not allow user to delete other users messages

Actual Result:

App allows user to delete other users messages in # admin

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Android: Native

Android.native.admin.allows.delete.other.user.msg.mp4

Android: mWeb Chrome

Android.chrome.admin.allows.to.delete.different.user.msg.mp4

iOS: Native

ios.native.admin.allows.to.delete.other.users.mov

iOS: mWeb Safari

ios.safari.admin.allows.to.delete.other.users.mov

MacOS: Chrome / Safari

windows.chrome.admin.allows.to.delete.msg.of.other.users.mp4

mac.chrome.admin.allows.to.delete.others.message.mov

Recording.5047.mp4

MacOS: Desktop

mac.desktop.admin.allows.to.delete.others.message.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016a6d08b85f59984d
• Upwork Job ID: 1714369820496429056
• Last Price Increase: 2023-10-17

[melvin-bot]

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

[melvin-bot]

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

[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

[melvin-bot]

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

[graylewis]

This seems like a backend issue.

[graylewis]

Proposal

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

This is primarily a backend issue. If the intended behavior is for this to be forbidden, then the backend needs to not let it happen regardless of frontend actions. That being said, the frontend should also hide the delete action when it is not applicable. According to a JSDocs comment in the ReportUtils.js file, a user should be able to delete a comment if;

...the author is this user and the action is an ADDCOMMENT action or an IOU action in an unsettled report, or if the user is a policy admin

This issue implies that it also shouldn't be allowed if the message in question was sent by another admin. Assuming that is correct, the following is a simple proposal to implement that logic on the frontend.

What is the root of the issue

The issue comes down to the function canDeleteReportAction, which the contextActionMenu component relies on to determine whether the delete action should be displayed. The last three lines of the function is as follows:

    const isAdmin = policy.role === CONST.POLICY.ROLE.ADMIN && !isDM(report);

    return isActionOwner || isAdmin;

Obviously, this code results in admins of a workspace being able to delete others' messages, even if those users are admins.

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

We have to add a check to see if the user sending the message is an admin:
return isActionOwner || (isAdmin && !actionOwnerIsAdmin);

What alternative solutions did you explore? (Optional)
NA

[jliexpensify]

@Santhosh-Sellavel what do you think? Is this a back-end issue?

[jliexpensify]

issue still remains:

[melvin-bot]

Current assignee @Santhosh-Sellavel is eligible for the Internal assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to @NikkiWines (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[jliexpensify]

Hi @NikkiWines - I have a tongue twister for you: it looks like any Admin can delete another Admin's message in the #admins room where there are multiple admins.

Is this an internal issue?

[NikkiWines]

Hmm, I'd say it's a combo - we'll need to prevent that action on the backend but it's probably also wise update the UI so we don't show the option to delete another admin's message

[jliexpensify]

Thanks! Will you tackle both @NikkiWines or just the back-end part? If so, we could turn this GH into an "update the UI" one and make it External?

[NikkiWines]

I can take on the backend, let's handle the UI update separately

[jliexpensify]

Ok cool, I assume the "update the UI" part is simply removing the option (i.e. the delete button) to delete an Admin chat in the Admin room?

If so, is it best to use this GH, or should we create a new one once you're done?

[NikkiWines]

Yep, that's correct - it basically involves actually adding a check to determine if the commenter is also an admin. If so, we won't show the option to delete the comment. And I think we can use this issue for the UI update!

And unless we need another backend update to determine if the commenter is an admin, then we should be able to do the UI update and backend change concurrently

[NikkiWines]

Backend PR is in review

[melvin-bot]

@NikkiWines, @jliexpensify, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[NikkiWines]

Backend PR is live! @Santhosh-Sellavel can you review @graylewis's proposal please 🙇

[melvin-bot]

@NikkiWines @jliexpensify @Santhosh-Sellavel this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[jliexpensify]

Not overdue - bumping @Santhosh-Sellavel

[jliexpensify]

Bumped via Slack

[Santhosh-Sellavel]

Sorry for the delay. @graylewis proposal looks good.
But one query howactionOwnerIsAdmin is computed?

[graylewis]

@Santhosh-Sellavel Roughly, we can use isAdminOfFreePolicy(policies) from Policy.js and pass it reportAction.actorAccountID from canDeleteReportAction(reportAction, reportID)

[Santhosh-Sellavel]

@graylewis That's not the case, can you please check. isAdminOfFreePolicy is to decided whether logged in user is admin or not.

[graylewis]

@Santhosh-Sellavel After doing some poking around, it looks like as of now there is no simple way to check if another user is an admin of a workspace. There is no data loaded in Onyx (unless you're on the WorkspaceMembersPage, therefore triggering API.read("OpenWorkspaceMembersPage")) that lists the users and their roles for a given workspace. There are two approaches to solving this issue:

1. We leverage existing API endpoints to get a list of all workspace members, lookup the user who created the report action (message) and check if that user is an admin. This approach would entail creating a new function in PolicyUtils.js which uses the onyx collection ONYXKEYS.COLLECTION.POLICY_MEMBERS to fetch all users, and then to look up the action creators ID in that list of members.
2. We add a 'role' field to every report action which represents the role of the person who created the report action. This seems logical to include and will probably be needed at some point down the line anyway. This solution would require some work on the backend side of things.

What do you think of these solutions @Santhosh-Sellavel? It doesn't seem like there's any other place in the app where other users' roles are referenced in this way.

One other point:
I don't have access to a 'full' policy (one that's not considered a 'free' policy), so I can't test this theory, but it seems like there is an employees field on full policies which might include the information we need (assuming managers are admins and vice-versa). I'd be interested to hear if anyone else has some insight into whether this is a feasible approach.

It's also not clear to me how different free policies are from full policies. Can free policies even have multiple admins? I can't see a way to add another admin to my workspace, but if not, why even create a #admins chat room?

[melvin-bot]

@NikkiWines @jliexpensify @Santhosh-Sellavel this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[Santhosh-Sellavel]

Both sound like an over-engineered solution.

@NikkiWines What's your thought here?

Also, can you answer this?

I don't have access to a 'full' policy (one that's not considered a 'free' policy), so I can't test this theory, but it seems like there is an employees field on full policies which might include the information we need (assuming managers are admins and vice-versa). I'd be interested to hear if anyone else has some insight into whether this is a feasible approach.

It's also not clear to me how different free policies are from full policies. Can free policies even have multiple admins? I can't see a way to add another admin to my workspace, but if not, why even create a #admins chat room?

[jliexpensify]

Free plans only allow for one Admin

Can my workspace have more than one Admin?

No. Currently, the Expensify Workplace only allows for just one admin (the Workspace creator).

[NikkiWines]

Hmmm, yeah agreed that both solutions aren't ideal. Especially if we only allow free workspaces with one admin in New Expensify right now.

It's also not clear to me how different free policies are from full policies. Can free policies even have multiple admins? I can't see a way to add another admin to my workspace, but if not, why even create a #admins chat room?

Full policies can have multiple admins and there are plans to introduce that functionality into New Expensify in the future. But yes at the moment the #admins chat room is a bit lonely 😅

I think this is a bug that users in the future could encounter, but it's not a bug someone could encounter now - at least with a normal set up for New Expensify.

I suggest we close this or put it on hold for now until we introduce multiple admins for workspaces in New Expensify. Thoughts?

[jliexpensify]

until we introduce multiple admins for workspaces in New Expensify.

@NikkiWines any ideas when this might be?

Also - do we actually want to allow Admins to delete other Admin messages? I always thought we would only want users to delete their own messages.

[NikkiWines]

@NikkiWines any ideas when this might be?

Nope, not sure where that is on the roadmap.

Also - do we actually want to allow Admins to delete other Admin messages? I always thought we would only want users to delete their own messages.

We don't want admins to be able to delete other admin messages. Admins should only be allowed to delete their own messages and non-admin messages.

[jliexpensify]

Yep, that's what I thought! Forgive my ignorance here, but can we just blanket remove the delete button in all #admins rooms? Or is that something that's really complex?

[NikkiWines]

Because we don't just want to disallow admins from deleting other admin messages in just #admins, we want to disallow it across all rooms tied to a workspace.

[jliexpensify]

Ok cool, thanks Nikki! I'm happy to pay the Reporter but lets probably close this one?

[jliexpensify]

Payment Summary

• Bug Reporter: @dhanashree-sawant $50

Upworks Job

[jliexpensify]

Paid and job closed!