App: [$500] HIGH: Review who can remove whom from groups, workspaces, and workspace objects
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:
-
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)
-
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) -
Verify that each of the above rules is being correctly enforced by:
- 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)
- 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.
- 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)
- Add a comment to this GH capturing evidence that you did this test (ie, a screenshot or console logs)
- Check off the test in the OP and go to the next
-
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 https://github.com/Expensify/App/issues/33575#issuecomment-1872623804
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
About this issue
- Original URL
- State: closed
- Created 6 months ago
- Comments: 64 (37 by maintainers)
@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!
Hm… I think it’s fine to remove a room creator (and it’s fine for them to leave).
That statement just means:
Alice's ApplesAlice's Apples / Bob“workspace chat”Great! Can you create a new issue with detailed instructions on what to do, and then link the bullet in the original post to the new issue, so we can keep track of what needs to be done?
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.
@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 😄
New issues created from this ticket:
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.
This issue is great. Leaving from the workspace will automatically make you leave from the workspace chat.
That sounds correct to me 👍 We would need to add that feature
Yes
Hey @MonilBhavsar In PR I already added Security section and links, I think for this PR thats will be it. To be clear: “3. Verify that each of the above rules is being correctly enforced by:” - for this part I need to write test steps for all the rules and test it, also test backend API calls, right?
If yes, the only thing left is to test everything fully.
"Anybody else can be freely added to a workspace chat via invite or mention, meaning they can also be freely removed either by themselves, or by admins
From the https://github.com/Expensify/App/issues/33575#issuecomment-1902784809 the only 3d point is
“meaning they can also be freely removed either by themselves, or by admins” This part here looks like is not implemented right now. Because its not possible to leave from the workspace.
Yes, I think for this we need to create a new issue for leaving/removing members from workspace chats. Remove already exists, admins can freely remove users, but its not possible to leave. We need to create a ticket “Add possibility to leave from workspace”
Applying the above rules, I think this means:
Does this sound right?
No satisfactory proposals yet. @MonilBhavsar can we float this around expert contributor group?