[$500] HIGH: Review who can remove whom from groups, workspaces, and workspace objects

[quinthar]

Problem:
There are complex rules for which room members can see other room members -- rules that were recently updated because there wasn't widespread agreement on how it should work, and how it did work wasn't really to anyone's liking. There's a risk we're doing the same with who can remove people from the room.

Solution:
As discussed in Slack, these are what the rules should be, as documented in this SO:

• Nobody can leave or be removed from something they were automatically added to. For example:

  • DM members can't leave or be removed from their DMs
  • Members can't leave or be removed from their own workspace chats
  • Admins can't leave or be removed from workspace chats
  • Members can't leave or be removed from the #announce room
  • Admins can't leave or be removed from #admins
  • Domain members can't leave or be removed from their domain chat
  • Report submitters can't leave or be removed from their reports (eg, if they are the report.accountID)
  • Report managers can't leave or be removed from their reports (eg, if they are the report.managerID)
  • Group owners cannot be removed from their groups -- they need to transfer ownership first
    • Note: Groups don't exist yet, so you can skip this

• Excepting the above, admins can remove anyone. For example:

  • Group admins can remove other group admins, as well as group members
  • Workspace admins can remove other workspace admins, as well as workspace members, and invited guests

• Excepting the above, members can remove guests. For example:

  • Workspace members can remove non-workspace guests.

• Excepting the above, anybody can remove themselves from any object

  • Confirmed

As part of this GH please do the following:

1. Document the above rules in https://github.com/Expensify/App/blob/main/README.md in a new section called Security, put between Philosophy and Internationalization (add to the ToC and copy the header styles of the surrounding sections)

2. Find every place in the app that involves adding/removing people to an object's membership, and add a link to that new security section in a comment (eg, // Please see https://github.com/Expensify/App/blob/main/README.md#Security for more details)

3. Verify that each of the above rules is being correctly enforced by:

   1. Create the conditions to test the rule (eg, for "DM members can't leave or be removed from their DMs", open a test DM to another user)
   2. Manually trigger an API command that attempts to do the wrong thing (eg, attempt to remove either of the DM participants) -- ideally the server will correctly reject it, but if not, record it in this issue so we can fix the server.
   3. Verify that there is no method in the client-side app to attempt the forbidden action (eg, in the case of the DM there is no UI to show the member list, so therefore the user can't attempt it)
   4. Add a comment to this GH capturing evidence that you did this test (ie, a screenshot or console logs)
   5. Check off the test in the OP and go to the next

4. When all the boxes are checked, submit the PR or the new Security section and all the corresponding comment links.

---

Proposals work differently on this issue

• For all the checkboxes above, please identify what features are present in the App and can be worked right now. For features missing in the App like "Leaving workspace chat or #admins room or #announce room". Please flag them so we can discuss and create an issue for them
• For each checkbox, please briefly explain where and why you want to make the change to fix the access control
• Addition of jest tests is a plus!

As mentioned by David here #33575 (comment)

Great question, this is a kind of different issue than normally. So yes, we need some way to choose who to do this, and "first come first serve" doesn't seem very discerning. Can you make a proposal that explains why we should pick you over someone else? I'm not sure what that would entail -- maybe cite past experience, or show that you've already worked in this area of the code, or even pick one of the more complex ones to talk about how you would go about testing it? Something to stand out.

We'll choose the proposal that nearly fixes this issue and help us to find and figure out missing pieces

Thanks!

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fb7403bad51be652
• Upwork Job ID: 1741227152068182016
• Last Price Increase: 2024-01-13
• Automatic offers:
• aimane-chnaif | Reviewer | 28105369

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[rayane-djouah]

I would like to work on this issue, is a proposal required?

[quinthar]

Great question, this is a kind of different issue than normally. So yes, we need some way to choose who to do this, and "first come first serve" doesn't seem very discerning. Can you make a proposal that explains why we should pick you over someone else? I'm not sure what that would entail -- maybe cite past experience, or show that you've already worked in this area of the code, or even pick one of the more complex ones to talk about how you would go about testing it? Something to stand out.

[rayane-djouah]

Proposal

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

The issue involves recently updated rules about room membership removal in our application, requiring reviewing and clear documentation to ensure consistency.

What is the root cause of that problem?

N/A

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

Currently, the app does not provide features that attempt the forbidden actions. we should normalize this so developers would not attempt to include this features in the future. and we should ensure that the server reject these actions if someone attempts to do it by an API request.

There is two API endpoints for membership removals: "RemoveFromRoom" and "DeleteMembersFromWorkspace"

Here is the function that calls the RemoveFromRoom API for the room membership removal:

App/src/libs/actions/Report.ts

Lines 2189 to 2190 in 8cb48e4

	/** Removes people from a room */
	function removeFromRoom(reportID: string, targetAccountIDs: number[]) {

and here is the function that calls DeleteMembersFromWorkspace API from the workspace membership removal:

App/src/libs/actions/Policy.js

Lines 323 to 329 in 8cb48e4

	/**
	* Remove the passed members from the policy employeeList
	*
	* @param {Array} accountIDs
	* @param {String} policyID
	*/
	function removeMembers(accountIDs, policyID) {

Summary of what should I do in this issue:

1. Document the updated rules in a new "Security" section in the README.md of the Expensify/App repository.
2. Insert comments linking to this section in the app code wherever membership modification occurs:
   here, here, here, and here.
3. Conduct tests to ensure these rules are correctly enforced and documented, including evidence in the GitHub issue.
   For example let's take this rule:

DM members can't be removed from their DMs.

• Conditions to test the rule:

  1. open a test DM to another user

• Testing that there is no UI that allow the forbidden action:

Screenshot

There is no option that allow the user to remove the other user from the DM.

• Testing that the server will correctly reject the forbidden action:

1. Open the Network tab under the developers tools.
2. Create a new room
3. Invite the user to the room
4. Remove the user from the room
5. Right click the 'RemoveFromRoom' api call from the requests
6. Click on "Edit and Resend" option.
7. In the request Body, change the reportID to the user DM reportID
8. Click send
9. Observe the server response
10. Verify that the request get rejected by the server.
11. Refresh the app
12. Verify that the DM still exist and the user still have access to it.

Screen Recording

Recording.mp4

The server correctly reject the request.

4. Finalize the process with a pull request for the documentation updates and comment links.

Qualifications

• Codebase Familiarity: In-depth understanding of the Expensify/App codebase from prior work, enabling efficient implementation.
• Approach to Testing: I would create a test environment replicating the scenario. I would then attempt using possible scenarios to execute the API command, expecting the server to reject it. This test would be documented with detailed logs or screenshots.

Reply to questions

For all the checkboxes above, please identify what features are present in the App and can be worked right now. For features missing in the App like "Leaving workspace chat or #admins room or #announce room". Please flag them so we can discuss and create an issue for them

Currently, the app does not provide features that attempt the forbidden actions. we should normalize this so developers would not attempt to include this features in the future. and we should ensure that the server reject these actions if someone attempts to do it by an API request.

For each checkbox, please briefly explain where and why you want to make the change to fix the access control

If bugs are discovered during testing, I will report them so the internal team can fix it from backend.

Addition of jest tests is a plus!

I think that we should only do manual testing in this issue. Internal team will need to add automated tests for this rules to the backend code in the future.

What alternative solutions did you explore? (Optional)

[quinthar]

@MonilBhavsar what are the next steps? Do you select a proposal? Or should this have been assigned a C+?

[MonilBhavsar]

As per the process, I would like @aimane-chnaif to select a proposal and then I can finally assign them to the issue, if that sounds good

[MonilBhavsar]

I have updated the github issue description on posting proposals for this issue. cc @quinthar @aimane-chnaif
@rayane-djouah you might want to update the proposal based on that update

[melvin-bot]

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

[waterim]

Also tested backend for those rules, no forbidden actions!
Updated with screenshots with responses here

[quinthar]

Also I think there should be a separate ticket for "Leave from the workspace", or we can combine it together like: "Add possibility to leave from the workspace and workspace chats"

This issue is great. Leaving from the workspace will automatically make you leave from the workspace chat.

[quinthar]

Workspace chat can have only user and all admins, but Its not possible to add any other members/guests to those chats. Am I right? Because I tested this statement and it just doesnt work.

This is not correct. Anybody can be invited to any chat, the sole exception being DM chats, which only ever have 2 participants. But literally anybody can be added to literally any other chat. So I should be able to mention you into my workspace chat, and it should work fine.

[quinthar]

Just to clarify here - I think we need atleast one room admin in the room to manage the room. If the room creator is the only admin and they leave the room, then there would be no admin to manage the room

There's no such thing as a "room admin". I think it's fine to remove the creator, as the creator gets no special treatment -- nothing breaks if they are removed, so far as I know.

[quinthar]

Just by mention we are adding members and then this member should be able to leave or be removed from this chat, right?

I don't quite understand this question. Can you elaborate on it?

[waterim]

This is not correct. Anybody can be invited to any chat, the sole exception being DM chats, which only ever have 2 participants. But literally anybody can be added to literally any other chat. So I should be able to mention you into my workspace chat, and it should work fine.

Okay, then I will create a new ticket for that. Because it doesn’t work now, I tried several times in workspace chat to mention different users(workspace members and not) and its not inviting this user

[waterim]

Just by mention we are adding members and then this member should be able to leave or be removed from this chat, right?

I don't quite understand this question. Can you elaborate on it?

As you said we should be able to invite new members to the workspace chats by „mentioning them”.
Admin and default member of the workspace chat are not able to leave or be removed from this workspace chat, but in the same time this „invited” user should be able to leave and admin/default member should be able to remove him, right?

[quinthar]

Okay, then I will create a new ticket for [mentioning users to add them to workspace chats]. Because it doesn’t work now, I tried several times in workspace chat to mention different users(workspace members and not) and its not inviting this user

Yes please do, thanks!

[quinthar]

As you said we should be able to invite new members to the workspace chats by „mentioning them”.
Admin and default member of the workspace chat are not able to leave or be removed from this workspace chat, but in the same time this „invited” user should be able to leave and admin/default member should be able to remove him, right?

Correct.

[quinthar]

Still on track for tomorrow or Wed?

[waterim]

@quinthar if everything is fine with PR - yes, we can finish tomorrow.
I opened a PR day before yesterday, but no reviews for now! Also yesterday updated a security section and tested manually backend.

Will create an issue now and waiting for the reviews.

Also I will be OOO tomorrow and on Thursday(2 days) because of the sick leave and if something urgent will appear @rezkiy37 can help with finishing it before I come back!

[waterim]

New issues created from this ticket:

1. Add possibility to leave from workspace
2. Add users to workspace chat by mentioning them.
3. Add possibility to remove invited user from workspace chat and add possibility for invited user to leave from the workspace chat

[MonilBhavsar]

Sounds good, thanks!

[quinthar]

What remains to finish this? I saw "I opened a PR day before yesterday, but no reviews for now" who is this waiting on? What do they need to do, and when will it be done?

[aimane-chnaif]

Ah #34987 is waiting for my review. I thought we're still in discussion as Reviewing label was removed

[MonilBhavsar]

@aimane-chnaif yes PR is ready for review!
For testing can we please confirm that the rules we added in README.md are working as expected in the App. I'll update Tests section accordingly.

Also, Reviewing label was removed because that is not the only PR required for this issue or that PR does not completely fixes/closes this issue 😄

[MonilBhavsar]

PR to update Security settings in README is merged

[MonilBhavsar]

Going to close this issue now given we have two children issues #35391 and #35286 in vip-vsb and wave-8 respectively

[aimane-chnaif]

Going to close this issue now given we have two children issues #35391 and #35286 in vip-vsb and wave-8 respectively

can I be assigned those since I have context?