Page MenuHomePhabricator

[lib] Introduce getUniversalCommunityRootPermissionsBlob
ClosedPublic

Authored by rohan on Nov 15 2023, 11:47 AM.
Tags
None
Referenced Files
F3440774: D9897.id33281.diff
Mon, Dec 9, 11:44 PM
Unknown Object (File)
Mon, Dec 9, 9:12 AM
Unknown Object (File)
Fri, Dec 6, 3:19 AM
Unknown Object (File)
Sat, Nov 30, 8:52 AM
Unknown Object (File)
Tue, Nov 26, 5:07 AM
Unknown Object (File)
Mon, Nov 25, 6:12 AM
Unknown Object (File)
Wed, Nov 20, 11:55 PM
Unknown Object (File)
Wed, Nov 13, 5:53 PM
Subscribers

Details

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

Reuse genesis special case permissions

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

Move genesis permissions blob to getUniversalCommunityRootPermissionsBlob

rohan published this revision for review.Nov 15 2023, 12:25 PM
ashoat requested changes to this revision.Nov 15 2023, 12:41 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

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?

Make it a no-op change

Confirmed the following:

  1. Every single permission in getUniversalCommunityRootPermissionsBlob(threadTypes.COMMUNITY_ROOT) is in getRolePermissionBlobsForCommunity(threadTypes.COMMUNITY_ROOT)
  2. Every single permission in getUniversalCommunityRootPermissionsBlob(threadTypes.COMMUNITY_ANNOUNCEMENT_ROOT) is in getRolePermissionBlobsForCommunity(threadTypes.COMMUNITY_ANNOUNCEMENT_ROOT)
  3. The original universalCommunityPermissions array in thread-permissions-types is equal to the permissions included in getUniversalCommunityRootPermissionsBlob(COMMUNITY_ANNOUNCEMENT_ROOT)
  4. 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)

  1. 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.

  1. 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

ashoat requested changes to this revision.Nov 17 2023, 12:52 PM

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
  1. 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
  2. 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.
  3. 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

lib/permissions/thread-permissions.js
409 ↗(On Diff #33280)
threadType:
  | typeof threadTypes.COMMUNITY_ANNOUNCEMENT_ROOT
  | typeof ....
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:

  1. 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
  2. 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 marked 4 inline comments as done.

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

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