Details

Reviewers

atul
ginsu
ashoat

Commits

rCOMM67f55a80135f: [lib] Introduce getUniversalCommunityRootPermissionsBlob

Summary

getUniversalCommunityRootPermissionsBlob is going to emulate the way we construct permissions blobs so it's more extendible towards separating permissions included for all roles in both COMMUNITY_ROOT and COMMUNITY_ANNOUNCEMENT_ROOT.

I'm not completetly deleting universalCommunityPermissions in thread-permissions-types.js yet, but I will do that once I land this entire stack + rebase the stack starting at D9493, since otherwise it'll cause a ton of merge conflicts and broken unit tests.

The rest of this stack will update usages of universalCommunityPermissions besides the unit test with this new method.

Addressess ENG-5761

Depends on D9514

Test Plan

Created a community root on both master and this diff stack. Compared the MariaDB JSON blobs for Members permissions
Created a community announcement root on both master and this diff stack. Compared the MariaDB JSON blobs for Members permissions

In both cases, the JSON was identical

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Nov 15 2023, 11:47 AM

Herald added a subscriber: tomek. · View Herald TranscriptNov 15 2023, 11:47 AM

Reuse genesis special case permissions

rohan added inline comments.Nov 15 2023, 11:54 AM

lib/permissions/thread-permissions.js
179–190 ↗	(On Diff #33281)	Defined this at the top scope so I can reuse `genesisMemberPermissions` in both `getRolePermissionBlobsForCommunity` and `getUniversalCommunityRootPermissionsBlob`. I can always revert it back to how it is and reconstruct `genesisMemberPermissions` in each function if reviewer's prefer it
321–322 ↗	(On Diff #33281)	These are now defined at the top scope
365–366 ↗	(On Diff #33281)	This are now defined at the top scope

rohan added a child revision: D9898: [keyserver] Use getUniversalCommunityRootPermissionsBlob in validate-role-permissions script.Nov 15 2023, 11:56 AM

Harbormaster completed remote builds in B24163: Diff 33280.Nov 15 2023, 12:22 PM

Move genesis permissions blob to getUniversalCommunityRootPermissionsBlob

rohan published this revision for review.Nov 15 2023, 12:25 PM

Harbormaster completed remote builds in B24164: Diff 33281.Nov 15 2023, 12:28 PM

Am I missing something? Seems like a lot of issues here. I didn't get through the whole review (it's pretty time-consuming to trace each individual permission through these function calls) but it seems pretty clear that at least some of it is wrong. Hoping you can fix it up before I do a full review so I can save some time.

lib/permissions/thread-permissions.js
195–196 ↗	(On Diff #33295)	Why do we still need these lines?
403 ↗	(On Diff #33295)	Why not use the `OPEN_DESCENDANT` variable?
404 ↗	(On Diff #33295)	What's this line doing here?

This revision now requires changes to proceed.Nov 15 2023, 12:41 PM

In D9897#288679, @ashoat wrote:

Am I missing something? Seems like a lot of issues here. I didn't get through the whole review (it's pretty time-consuming to trace each individual permission through these function calls) but it seems pretty clear that at least some of it is wrong. Hoping you can fix it up before I do a full review so I can save some time.

I probably should've specified in the diff description - I meant to and forgot when writing it out. I'm making this change based on the current state of master, not any of my permissions code that's up for review. I included things like descendant_open_voiced because that's what is on master right now and I'd like to make this stack mirror that. I'm not adding/removing any permissions in this diff

Once this is landed, I can rebase D9493 which removes descendant_open_voiced (for example).

Here, I'm just taking the existing set of universalCommunityPermissions, and essentially returning them based on a provided thread type. Like I mentioned in our 1:1, for now they'll be the same except for voiced being included in COMMUNITY_ROOT and not in COMMUNITY_ANNOUNCEMENT_ROOT.

If you're aware of that but still noticed some issues that are not answered by this, then I'll go ahead and take another pass at this before you do a full review like you asked.

lib/permissions/thread-permissions.js
195–196 ↗	(On Diff #33295)	They aren't included in `genesisMemberPermissions`, so removing them means the member blob would no longer have them. I'm not making any real changes to `getRolePermissionBlobsForCommunity` right now, I'm just introducing `getUniversalCommunityRootPermissionsBlob` in this diff
403 ↗	(On Diff #33295)	I should - will update this
404 ↗	(On Diff #33295)	I'm making changes based off of the current state of `master`, and that's included on `master`. The goal is to try to make this a 1:1 change between what's already in the codebase. Once this stack is accepted, I plan to rebase D9493's stack, which removes `descendant_open_voiced`

You say you're matching master, but it seems to me that you're changing the behavior of getRolePermissionBlobsForCommunity in this diff.

Can you update this diff so it's a "no-op"? If there's a change you want to make to the return of getRolePermissionBlobsForCommunity, I'd prefer for those changes to be in a separate diff, and very carefully justified.

I might be missing something, but I thought the whole point of all of our weeks of back-and-forth was to get your new roles code to match up with the existing stuff. It seems like this diff is not progressing us forward on that.

If you want to push back again, I'd suggest trying to schedule a meeting ASAP instead of continuing on this diff.

lib/permissions/thread-permissions.js
186 ↗	(On Diff #33295)	Why don't we pass `threadType` here?
195–196 ↗	(On Diff #33295)	Shouldn't these lines be handled by `getUniversalCommunityRootPermissionsBlob`? It makes sense that they wouldn't be present for `GENESIS`, but `getUniversalCommunityRootPermissionsBlob` is presumably supposed to be able to take any `ThreadType`
404 ↗	(On Diff #33295)	I do not see `descendant_open_voiced` anywhere in `getRolePermissionBlobs`... what am I missing?

Harbormaster completed remote builds in B24177: Diff 33295.Nov 15 2023, 2:53 PM

rohan edited the summary of this revision. (Show Details)Nov 17 2023, 7:09 AM

rohan added a parent revision: D9514: [lib/web/native] Show voiced_in_announcement_channels user-surfaced permission for all community types.

rohan added a child revision: D9804: [lib] Include Voiced permission blob for announcement channels.

rohan removed a child revision: D9804: [lib] Include Voiced permission blob for announcement channels.Nov 17 2023, 7:11 AM

Make it a no-op change

Confirmed the following:

Every single permission in getUniversalCommunityRootPermissionsBlob(threadTypes.COMMUNITY_ROOT) is in getRolePermissionBlobsForCommunity(threadTypes.COMMUNITY_ROOT)
Every single permission in getUniversalCommunityRootPermissionsBlob(threadTypes.COMMUNITY_ANNOUNCEMENT_ROOT) is in getRolePermissionBlobsForCommunity(threadTypes.COMMUNITY_ANNOUNCEMENT_ROOT)
The original universalCommunityPermissions array in thread-permissions-types is equal to the permissions included in getUniversalCommunityRootPermissionsBlob(COMMUNITY_ANNOUNCEMENT_ROOT)
The original universalCommunityPermissions array in thread-permissions-types is equal to the permissions included in getUniversalCommunityRootPermissionsBlob(COMMUNITY_ROOT) except that getUniversalCommunityRootPermissionsBlob(COMMUNITY_ROOT) includes voiced (this is essential to removing the hack in role-creator)

Also confirmed the following that will be useful for part 2 of this change (updating getRolePermissionBlobsForCommunity to use user-surfaced permission sets)

getRolePermissionBlobsForCommunity(threadTypes.COMMUNITY_ROOT) has the following additional permissions:

{
  react_to_message: true,
  edit_message: true,
  add_members: true,
  child_open_add_members: true,
  edit_entries: true,
  edit_thread: true,
  edit_thread_color: true,
  edit_thread_description: true,
  edit_thread_avatar: true,
  create_subthreads: true
}

These correspond to 5 user-surfaced permissions: REACT_TO_MESSAGES, ADD_MEMBERS, EDIT_MESSAGES, EDIT_CALENDAR and CREATE_AND_EDIT_CHANNELS. This is confirmed by creating a regular community and seeing the Members role already has these selected.

getRolePermissionBlobsForCommunity(threadTypes.COMMUNITY_ANNOUNCEMENT_ROOT) has the following additional permissions:

{
  react_to_message: true,
  edit_message: true,
  add_members: true,
  child_open_add_members: true
}

These correspond to three user-surfaced permissions: REACT_TO_MESSAGES, ADD_MEMBERS and EDIT_MESSAGES. This is confirmed by creating an announcement root community and seeing the Members role already has these three user-surfaced permissions checked.

As mentioned above, part 2 of this change will come following D9901. It will involve rewriting getRolePermissionBlobsForCommunity to use configurableCommunityPermissions where its needed to unify the permissions system

Harbormaster completed remote builds in B24242: Diff 33380.Nov 17 2023, 7:46 AM

Please do NOT update this diff again until you schedule a 1:1 call with me

lib/permissions/thread-permissions.js
408 ↗	(On Diff #33380)	I don't like that you're just introducing a dangling function here. Can you bring this diff back to actually updating `getRolePermissionBlobsForCommunity`?
435 ↗	(On Diff #33380)	`openChildAddMembers` appears to be missing
442 ↗	(On Diff #33380)	In what scenario does this happen? I think it should be replaced with an `invariant`

This revision now requires changes to proceed.Nov 17 2023, 12:52 PM

The WHOLE point of this diff is to unify the existing role logic with your new logic. You need to make getRolePermissionBlobs work based off of:
- A new, modified version of universalCommunityPermissions that differentiates based on ThreadType
- configurableCommunityPermissions
This diff needs to actually change the code in getRolePermissionBlobs. It is not enough to introduce some new utility. If you want to skip this diff into multiple, please do not request review until all of the diffs are ready.
This diff should be a no-op. There should be no changes to the result of getRolePermissionBlobs. It should simply be updated to function based on your new logic.

lib/permissions/thread-permissions.js
408 ↗	(On Diff #33380)	This function is supposed to be based on `universalCommunityPermissions` + `configurableCommunityPermissions` now, but I see zero references to those

ashoat added inline comments.Nov 17 2023, 2:34 PM

lib/permissions/thread-permissions.js
409 ↗	(On Diff #33280)	threadType: \| typeof threadTypes.COMMUNITY_ANNOUNCEMENT_ROOT \| typeof ....

rohan mentioned this in D9898: [keyserver] Use getUniversalCommunityRootPermissionsBlob in validate-role-permissions script.Nov 20 2023, 9:17 AM

rohan removed a child revision: D9898: [keyserver] Use getUniversalCommunityRootPermissionsBlob in validate-role-permissions script.

rohan mentioned this in D9899: [keyserver] Use getUniversalCommunityRootPermissionsBlob in role-creator.

rohan added a child revision: D9900: [keyserver] Validate community ID corresponds to a community root in role-creator.

Update

rohan planned changes to this revision.Nov 20 2023, 9:20 AM

Merge diffs (for now)

Harbormaster completed remote builds in B24282: Diff 33432.Nov 20 2023, 9:50 AM

Harbormaster completed remote builds in B24285: Diff 33435.Nov 20 2023, 10:03 AM

rohan planned changes to this revision.Nov 20 2023, 11:30 AM

ashoat accepted this revision.Nov 20 2023, 11:56 AM

ashoat added inline comments.

lib/permissions/thread-permissions.js
163 ↗	(On Diff #33435)	Let's export this and call it from `role-creator.js` to avoid duplicating code
164 ↗	(On Diff #33435)	Unless we plan to mutate `communityUserSurfacedPermissions`, we should always take `$ReadOnlyArray`
199–207 ↗	(On Diff #33435)	Pull this out of the function definition
309 ↗	(On Diff #33435)	No need to redeclare this on every invocation
lib/types/thread-types-enum.js
62–65 ↗	(On Diff #33435)	Looked into this and Flow is treating this as `number`, which isn't what we want... we want `8 \| 9 \| 12`. We can fix this in two ways: By extracting the definitions of `COMMUNITY_ROOT`, `COMMUNITY_ANNOUNCEMENT_ROOT`, and `GENESIS` into their own declarations, each with specific typehints of 8 / 9 / 12, OR By adding a typehint to the definition of `threadTypes` so that Flow is able to conclude that eg. `threadTypes.GENESIS` is 12, not `number`

rohan edited the test plan for this revision. (Show Details)Nov 21 2023, 8:19 AM

Address feedback.

Had some flow issues with type refinement when using CommunityRootThreadType, so instead of spending a ton of time on it I decided to use an invariant in getUniversalCommunityRootPermissionsBlob which achieves the same effect of validating the thread type parameter

This revision is now accepted and ready to land.Nov 21 2023, 8:33 AM

Harbormaster completed remote builds in B24309: Diff 33461.Nov 21 2023, 8:53 AM

Thanks for linking the try-Flow thing – makes it a lot easier for me to confirm the issue

Rebase

Harbormaster completed remote builds in B24337: Diff 33494.Nov 22 2023, 8:05 AM

Closed by commit rCOMM67f55a80135f: [lib] Introduce getUniversalCommunityRootPermissionsBlob (authored by rohan). · Explain WhyNov 22 2023, 8:48 AM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM67f55a80135f: [lib] Introduce getUniversalCommunityRootPermissionsBlob.

[lib] Introduce getUniversalCommunityRootPermissionsBlob
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 33494

keyserver/src/creators/role-creator.js

keyserver/src/scripts/validate-role-permissions.js

lib/permissions/thread-permissions.js

[lib] Introduce getUniversalCommunityRootPermissionsBlobClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 33494

keyserver/src/creators/role-creator.js

keyserver/src/scripts/validate-role-permissions.js

lib/permissions/thread-permissions.js

[lib] Introduce getUniversalCommunityRootPermissionsBlob
ClosedPublic
Actions

Revision Contents
Changeset List