Page MenuHomePhabricator

[keyserver/lib/native] Introduce VOICED_IN_ANNOUNCEMENT_CHANNELS thread permission

Authored by rohan on Fri, Nov 10, 11:26 AM.
Referenced Files
Unknown Object (File)
Fri, Dec 1, 9:32 PM
Unknown Object (File)
Thu, Nov 30, 11:46 AM
Unknown Object (File)
Wed, Nov 22, 7:43 AM
Unknown Object (File)
Tue, Nov 21, 11:09 AM
F887598: Screenshot 2023-11-21 at 10.16.08 AM.png
Tue, Nov 21, 7:17 AM



This diff introduces a new thread permission, VOICED_IN_ANNOUNCEMENT_CHANNELS. We also create a propgated/filtered version of this permission, `descendant_toplevel_voiced_in_announcement_channels.

The idea here is that if the user-surfaced VOICED_IN_ANNOUNCEMENT_CHANNELS permission is granted, we give the role both of these permissions. In makePermissionBlob, we can then grant voiced for an announcement thread in a really easy way.

I will be adding a updateRolesAndPermissionsForAllThreads migration once at the end of this entire stack, so that's why I didn't add it here

Addresses ENG-5785

Depends on D9901

Test Plan

I went through every step in my final testing task and was able to perform all the steps with expected results

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

rohan edited the summary of this revision. (Show Details)
rohan added inline comments.
47 ↗(On Diff #33079)

Wasn't really sure if I needed to modify this file? cc @atul

ashoat requested changes to this revision.Sat, Nov 11, 3:31 PM

This feels like a super hacky approach and I think you're applying it in the wrong part of the stack

84 ↗(On Diff #33079)

It's really confusing to read this function and not see getRolePermissionBlobs referenced anywhere.

What am I missing? If we're using getRolePermissionBlobs to create a role permissions blob, shouldn't we be using it when modifying a role permissions blob? Shouldn't the "base" person vary based on the ThreadType, rather than being universalCommunityPermissions for all ThreadTypes? (There are two different kinds of community roots that can be passed here.)

97 ↗(On Diff #33079)

Where do you confirm that the community is actually a community root? Could somebody pass in a thread that isn't a community root to this endpoint, and get some weird behavior to occur?

117–122 ↗(On Diff #33079)

I'm confused about the existing code here. I must've missed this while reviewing D8420...

It sounds like we want to avoid giving voiced to the Members (per-thread) role in any of the three "announcement" thread types? And the Voiced (per-thread) role can always have voiced.

Then in getRolePermissionBlobs, we could leave voiced for Members in COMMUNITY_ROOT. Wouldn't that remove the need for this hack?

129 ↗(On Diff #33079)

Typo ("Vocied")

133 ↗(On Diff #33079)

This is really the wrong place to handle converting VOICED_IN_ANNOUNCEMENT_CHANNELS to VOICED. It's code in keyserver, within a specific case where we're creating roles.

This conversion should be handled at a lower level. How about makePermissionsBlob? You could use parseThreadPermissionString to convert VOICED_IN_ANNOUNCEMENT_CHANNELS to VOICED.

This revision now requires changes to proceed.Sat, Nov 11, 3:31 PM
84 ↗(On Diff #33079)

getRolePermissionBlobs isn't used here since it has preset permissions for roles in different communities. With custom roles, each role starts off with none of the user-surfaced permissions (all checkboxes client-side are unticked), and the user is free to add/remove as many permissions as they'd like when editing the role as well.

If I called getRolePermissionBlobs when a user edits a role, wouldn't that just force the permissions to however the default Members blob looks like for the community type?

97 ↗(On Diff #33079)

There's not currently, but I can call threadTypeIsCommunityRoot on the thread type when it's fetched possibly

117–122 ↗(On Diff #33079)

This wasn't just for Members, it was for the creation of custom roles and editing all roles.

Members are already given voiced in getRolePermissionBlobs for COMMUNITY_ROOTs, though that means we need to handle custom roles ourselves:

  • For custom roles, how do we confer voiced to them in COMMUNITY_ROOT
  • For both members and custom roles in COMMUNITY_ANNOUNCEMENT_ROOT, if the user ticks the user-surfaced option VOICED_IN_ANNOUNCEMENT_CHANNELS client-side, then how do we give voiced to those roles

The code here was hoping to deal with these, so for all roles:

  • If the thread is a COMMUNITY_ROOT, push the voiced permission in their permissions blob (especially important for custom roles)
  • If the thread is not a COMMUNITY_ANNOUNCEMENT_ROOT, then the previous behavior was that ticking the client-side option to voice a role in an announcement channel corresponded with the voiced thread permission. This was the root cause of ENG-5183 since it didn't extend support to announcement channels
133 ↗(On Diff #33079)

makePermissionsBlob seems good, I'll work on the change!

Though I tried looking at parseThreadPermissionString, and I wonder if it's going to do the right thing here? Feel free to correct me, but it seems to parse permission strings that have propagation / filter prefixes in them (like descendant_open_voiced).

threadPermissions. VOICED_IN_ANNOUNCEMENT_CHANNELS maps to voiced_in_announcement_channels as a thread permission string, so I'm not sure if parseThreadPermissionString would help here?

84 ↗(On Diff #33079)

The general feedback is that we shouldn't have two sources of truth. You're making a fair argument against using getRolePermissionBlobs here, but it still doesn't explain why we need to use two completely separate methods for determining permissions blobs – one for role creation, and another for role modification.

Have you considered deprecating getRolePermissionBlobs in favor of programmatically computing those blobs based on a set of default userSurfacedPermissions?

97 ↗(On Diff #33079)

Yes please. This should've been done from the start...

117–122 ↗(On Diff #33079)

My intuition is that this hacky workaround should not be here. We should find a way to handle this without polluting business logic like this.

If the thread is a COMMUNITY_ROOT, push the voiced permission in their permissions blob (especially important for custom roles)

getRolePermissionBlobs has the ability to vary the set of permissions based on ThreadType. It sounds like we need the same thing for your user-surfaced permissions code. If your user-surfaced permissions code had access to the community root ThreadType, it could use a different set of universalCommunityPermissions depending on whether it is an announcement root or not.

133 ↗(On Diff #33079)

I don't feel strongly about using parseThreadPermissionString. I'm open to using any solution that goes through makePermissionsBlob.

Feel free to correct me, but it seems to parse permission strings that have propagation / filter prefixes in them

Yeah, that's what it currently does. You could of course modify it to do more, but I again I don't feel strongly.

rohan marked 5 inline comments as done.

Rebase on the stack so

  • hacky checks in role-creator are removed
  • community validation is done

Updated this diff so the voiced_in_announcement_channels thread permission is handled in makePermissionsBlob. There, if the permission exists in the blob, we add voiced to the permissions before returning it

Would be great if @atul could take a look as well. If we're not propagating VOICED_IN_ANNOUNCEMENT_CHANNELS I guess this is okay, but in that case I might need a reminder on how we're applying it in subchannels

107–117 ↗(On Diff #33393)

This seems like a really general condition to be granting VOICED... (as opposed to checking if we're in an announcement channel)

Does this have a risk of granting VOICED to non-members?

Does VOICED_IN_ANNOUNCEMENT_CHANNELS get propagated via descendent_ or child_, or does it only apply to the community root?

ashoat requested changes to this revision.Mon, Nov 20, 12:14 PM

Talked about this with @rohan and we realized there's an alternate solution here that will simplify the code we'll need to write in thread-permissions-updaters.js.

With the current approach, we only apply threadPermissions.VOICED_IN_ANNOUNCEMENT_CHANNELS to the community root. This means that in order to know whether the user has VOICED in a given channel, we'll need to query that channel's community root for permissions.

As an alternative, we could propagate threadPermissions.VOICED_IN_ANNOUNCEMENT_CHANNELS to all descendant channels using something like descendant_toplevel_voiced_in_announcement.

Then we would have all of the info we need in makePermissionsBlob. If the following three conditions are true, we would also set VOICED to true:

  2. permissions (after the KNOW_OF check) includes threadPermissions.VOICED_IN_ANNOUNCEMENT_CHANNELS
  3. The user is a member of the thread
    • We don't want to allow non-members to be voiced (unless they are admins), even if they have threadPermissions.VOICED_IN_ANNOUNCEMENT_CHANNELS
    • makePermissionsBlob doesn't appear to get an isMember parameter, but it does get rolePermissions, which presumably will be empty if (and only if) the user is not a member
    • @rohan, it would be good to confirm that assumption
109 ↗(On Diff #33393)

Let's use permissions here to mitigate the risk of accidentally granting VOICED to non-members

This revision now requires changes to proceed.Mon, Nov 20, 12:14 PM

Update approach (planning changes until I can update diff description/test plan)

rohan edited the test plan for this revision. (Show Details)

Did some investigating and it seems like we mainly have rolePermissions as null when a user is a former member and is now a ghost member of a thread (-1 role)

I checked this after reading the code comment above makePermissionsBlob.

Aside from that, I've also not been able to create a situation where (after adding members, removing them, changing roles, etc) a role that is > 0 has permissions

Screenshot 2023-11-21 at 10.16.08 AM.png (500×1 px, 108 KB)

119 ↗(On Diff #33445)

Typing this here for flow, otherwise I get the following:

Could not decide which case to select, since  case 1 [1] may work but if it doesn't  case 2 [2] looks promising too.

where case 1 and case 2 are

function permissionLookup(
  permissions: ?ThreadPermissionsBlob | ?ThreadPermissionsInfo, // here
  permission: ThreadPermission,
): boolean {
rohan requested review of this revision.Tue, Nov 21, 7:18 AM

Generally looks right, just a couple comments. Would be great if @atul could take a look as well

275 ↗(On Diff #33445)

We should use NEXT_CODE_VERSION here

47 ↗(On Diff #33445)
117 ↗(On Diff #33445)

Why do we need this check? Isn't threadTypeIsAnnouncementThread sufficient?

Address feedback (use NEXT_CODE_VERSION and use threadTypeIsAnnouncementThread)

Changes wrt minimal permissions work looks correct

This revision is now accepted and ready to land.Tue, Nov 21, 1:26 PM

Fix persist version after rebase