Skip to content

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

Closed

Description

@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

Activity

  1. converted this from a draft issue on Dec 25, 2023
  2. self-assigned this
    on Dec 25, 2023
  3. changed the title [-]CRITICAL: Review who can remove whom from workspace rooms[/-] [+]CRITICAL: Review who can remove whom from groups, workspaces, and workspace objects[/+] on Dec 29, 2023
  4. removed their assignment
    on Dec 30, 2023
  5. added
    Hot PickReady for an engineer to pick up and run with
    ExternalAdded to denote the issue can be worked on by a contributor
    and removed
    Hot PickReady for an engineer to pick up and run with
    on Dec 30, 2023
  6. melvin-bot commented on Dec 30, 2023

    @melvin-bot
  7. changed the title [-]CRITICAL: Review who can remove whom from groups, workspaces, and workspace objects[/-] [+][$500] CRITICAL: Review who can remove whom from groups, workspaces, and workspace objects[/+] on Dec 30, 2023
  8. 73 remaining items

  9. quinthar commented on Feb 2, 2024

    @quinthar
    ContributorAuthor

    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?

  10. aimane-chnaif commented on Feb 2, 2024

    @aimane-chnaif
    Contributor

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

  11. MonilBhavsar commented on Feb 2, 2024

    @MonilBhavsar
    Contributor

    @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 😄

  12. moved this from CRITICAL to HIGH in [#whatsnext] #vip-vsbon Feb 13, 2024
  13. changed the title [-][$500] CRITICAL: Review who can remove whom from groups, workspaces, and workspace objects[/-] [+][$500] HIGH: Review who can remove whom from groups, workspaces, and workspace objects[/+] on Feb 13, 2024
  14. MonilBhavsar commented on Feb 13, 2024

    @MonilBhavsar
    Contributor

    PR to update Security settings in README is merged

  15. MonilBhavsar commented on Feb 13, 2024

    @MonilBhavsar
    Contributor

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

  16. aimane-chnaif commented on Feb 13, 2024

    @aimane-chnaif
    Contributor

    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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

EngineeringExternalAdded to denote the issue can be worked on by a contributorReviewingHas a PR in reviewWeeklyKSv2

Type

No type

Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions