Details

Reviewers

ashoat
ginsu
atul

Commits

rCOMM11d0868e0328: [lib] Prevent editing role permissions from adding unexpected permissions

Summary

Full context is in the comment in comment in ENG-5622, but basically there was not a clear 1:1 match of permissions before and after editing a role sometimes. This is because the user-surfaced permissions provided some descendant_* permissions that Members, upon creation, did not originally have.

This diff fixes that so there's a clear equality between the two when permissions are edited.

Some user-surfaced permissions like KNOW_OF_SECRET_CHANNELS should still have their descendant_* permissions since that's how we can allow non-Admins to see secret channels

Addresses ENG-5622

After this, I'll need to update the MariaDB migration in D9599 and the redux persist migration in D9608 to remove all of these permissions. That's why I've added them to an array in lib called permissionsToRemoveInMigration so I can share one common object, rather than copy and pasting the huge collection of strings in both migrations.

Adding @ashoat as a reviewer here since I think he may have the most context on my proposed solution

Depends on D9493

Test Plan

Created some communities, edited some permissions for the Members role and then removed them, and created some custom roles with some random permissions.

Ran yarn script dist/scripts/validate-role-permissions.js to verify that there are no discrepencies. Before this diff, the permissions blob would have unexpected permissions like

deepDiff(existingRolePermissions, baseExpectedPermissionBlob) = {
  "join_thread": true,
  "descendant_react_to_message": true,
  "descendant_edit_message": true,
  "descendant_add_members": true,
  "descendant_edit_entries": true,
  "descendant_edit_thread": true,
  "descendant_edit_thread_description": true,
  "descendant_edit_thread_color": true,
  "descendant_toplevel_create_subthreads": true,
  "descendant_edit_thread_avatar": true,
  "descendant_toplevel_create_sidebars": true
}

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Nov 3 2023, 10:58 AM

Herald added subscribers: will, tomek. · View Herald TranscriptNov 3 2023, 10:58 AM

rohan added a child revision: D9599: [keyserver] Write MariaDB migration to remove incorrect permission from roles.Nov 3 2023, 10:58 AM

rohan edited the summary of this revision. (Show Details)Nov 3 2023, 11:08 AM

Harbormaster completed remote builds in B23724: Diff 32683.Nov 3 2023, 11:16 AM

rohan requested review of this revision.Nov 3 2023, 11:16 AM

Resigning since I don't have a ton of context and adding @ashoat as blocking.

Feel free to re-add me if there's anything specific I can be helpful with.

rohan edited the summary of this revision. (Show Details)Nov 3 2023, 11:53 AM

I'm worried that the test plan isn't thorough enough to make sure all of the permissions match up exactly. Would it be possible to write some unit tests that test creating some test chats (covering all thread types), then adding / removing each user-surfaced permission to each community-level role and making sure no changes occur?

lib/types/thread-permission-types.js
196–199 ↗	(On Diff #32683)	Why does this one stay? Was it in the old system as well?

rohan mentioned this in D9599: [keyserver] Write MariaDB migration to remove incorrect permission from roles.Nov 6 2023, 7:18 AM

rohan added inline comments.Nov 6 2023, 10:02 AM

lib/types/thread-permission-types.js
196–199 ↗	(On Diff #32683)	Yeah it was part of the base member permissions

Makes sense. What did you think of my test plan suggestion?

lib/types/thread-permission-types.js
196–199 ↗	(On Diff #32683)	Got it. Kind of weird that we let somebody who isn't in chat X add somebody else to chat X, but don't want to add that to the scope here

In D9686#284580, @ashoat wrote:

Makes sense. What did you think of my test plan suggestion?

Yeah your suggestion definitely makes sense, and adding the unit tests instead of just relying on the script will be useful in general. Since the affected roles are at a community-level, I wrote some unit tests that do the following on COMMUNITY_ROOT and COMMUNITY_ANNOUNCEMENT_ROOT:

take the Members permission blobs
add all universalCommunityPermissions (this should have no change)
add all configurableCommunityPermissions
remove all configurableCommunityPermissions
add back the originally-included user-surfaced permissions

Please feel free to let me know if perhaps I misunderstood your request.

Add unit tests

Your tests aren't testing what I wanted to test, and I'm concered about their quality. You can keep them if you want, but you'll have to address the issues I highlighted inline. Might be easier to just get rid of them.

Regarding what I want tested:

Since the affected roles are at a community-level, I wrote some unit tests that do the following on COMMUNITY_ROOT and COMMUNITY_ANNOUNCEMENT_ROOT

This part makes sense. From there you end up straying really far from what I was asking.

Let me try to rephrase with a bit more specificity:

Do the following procedure once for COMMUNITY_ROOT and once for COMMUNITY_ANNOUNCEMENT_ROOT
Create a community with default permissions
For each configurable permission, attempt to "toggle" the permission twice
- If it's set, then unset it and then set it again. If it's not set, then set it and then unset it.
- For each configurable permission, confirm that toggling the permission twice will not change the permissions blob

Some general principles:

Please do not copy-paste any lists you find elsewhere in the codebase. Please import them instead, or find a way to share the code.
If you find yourself writing a bunch of utility functions in the test file like you did in that last revision, that probably means you're not testing the right thing. Try to find ways to directly test the code in the codebase.

lib/types/thread-permission-types.test.js
90–93 ↗	(On Diff #32829)	What's the point of checking this right after assignment?
101–103 ↗	(On Diff #32829)	You're checking that `constructedBlobWithUniversalPermissions` is a subset of `membersPermissionBlob` Don't you want to check that they're exactly equal? (Might be easier to read and make sense of things if you used more readable checks, eg. `expectSubset` or `expectDeepEqual` – not sure if they exist, but I imagine it would be easy to implement in this file if not)
202–206 ↗	(On Diff #32829)	What's the point of this step? You're adding and then immediately removing... might as well just remove. The only code you end up checking with your add step is the code in this test file
214–219 ↗	(On Diff #32829)	Presumably after stripping out all configurable permissions from the university blob, you're left with exactly the university set. Shouldn't you check set equality rather than check subset?
221 ↗	(On Diff #32829)	Don't love that you're basically copy-pasting the list of default member permissions here. Can you source this from the right place instead of reproducing a new list?

This revision now requires changes to proceed.Nov 6 2023, 12:40 PM

Harbormaster completed remote builds in B23816: Diff 32829.Nov 6 2023, 12:53 PM

Add unit tests that make sure the permissions blob before and after toggling each user-surfaced permission remains the same.

To be honest, this could just be one unit test in its own describe block since it's the same piece of code that uses a different membersPermissionBlob. I thought it made sense to have it both times in the two describe blocks that separate COMMUNITY_ANNOUNCEMENT_ROOT and COMMUNITY_ROOT, but I'm ok to also make write one unit test that does both simultaneously.

Previously, this test would have failed since, for example:

Members could react to messages
Toggling userSurfacedPermissions.REACT_TO_MESSAGES would remove this permission
Toggling it back would add react_to_messages back, but it would also add descendant_react_to_messages back to the permissions blob

rohan added inline comments.Nov 7 2023, 11:16 AM

lib/types/thread-permission-types.test.js
36–38 ↗	(On Diff #32918)	Added this tiny change here to use `_isEqual`. It makes more sense than using `deepDiff`
86–88 ↗	(On Diff #32918)	Added this tiny change here to use `_isEqual`. It makes more sense than using `deepDiff`