Page MenuHomePhabricator

[lib] Don't restrict permissions in descendant chats for announcement roots
ClosedPublic

Authored by ashoat on Aug 13 2024, 8:50 AM.
Tags
None
Referenced Files
F2841765: D13068.diff
Sun, Sep 29, 11:17 AM
Unknown Object (File)
Sat, Sep 14, 11:47 PM
Unknown Object (File)
Sat, Sep 14, 11:47 PM
Unknown Object (File)
Sat, Sep 14, 11:46 PM
Unknown Object (File)
Sat, Sep 14, 11:44 PM
Unknown Object (File)
Sun, Sep 8, 9:43 PM
Unknown Object (File)
Thu, Sep 5, 2:04 PM
Unknown Object (File)
Tue, Sep 3, 2:19 AM
Subscribers
None

Details

Summary

This addresses ENG-9030.

The memberUserSurfacedPermissions calculation currently in the codebase basically controls the default set of user-surfaced permissions granted to a member.

The problem is that right now, this default set is different depending on the community root's ThreadType. I think the intention of that logic is to restrict permissions in the community root, but user-surfaced permissions are meant to be global for the whole community, so this doesn't make sense.

This issue wasn't noticed prior to D13017 because the user-surfaced permissions were only affecting the root. Now that they're affecting the whole community, we need to revisit this logic.

This diff introduces a new approach:

  1. The code for determining memberUserSurfacedPermissions is removed, in favor of assigning the same set of default user-surfaced permissions to all members, regardless of the community root's ThreadType.
  2. We also remove a similar distinction in getRolePermissionBlobs for announcement subchannels, since they can be handled by the same mechanism that we need to replace point 1 above.
  3. Both 1 and 2 are replaced with logic in makePermissionsBlob, where we subtract a set of permissions for announcement threads where the user doesn't have VOICED_IN_ANNOUNCEMENT_CHANNELS.

Depends on D13067

Test Plan

I did the following testing in my local environment after running a migration to reset permissions:

  1. I tested an announcement community root and confirmed that:
    • Admin had permissions to send messages in the root
    • Non-admin did not have permissions to send messages in the root
    • Non-admin did have permissions to send messages in other channels
  2. I tested a non-announcement community root and confirmed that:
    • Admin had permissions to send messages in the root
    • Non-admin had permissions to send messages in the root
    • Non-admin had permissions to send messages in other channels
  3. I tested GENESIS and confirmed that:
    • Admin had permissions to send messages and react in the root
    • Non-admin did not have permissions to send messages or react in the root
    • Non-admin had permissions to send messages and react in other channels

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/types/thread-permission-types.js
243

I didn't add the MEMBER prefix in D13017 because VOICED_IN_ANNOUNCEMENT_CHANNELS wasn't previously handled in getRolePermissionBlobs. But it makes sense to have here, as most of the permissions that non-admin members get from roles ("user-surfaced permissions") only apply if the user is a member.

This change was necessary here because VOICED_IN_ANNOUNCEMENT_CHANNELS will grant the non-admin users permissions that they should only have if they are a member. We previously handled this with an isMember check in makePermissionsBlob, but that doesn't work for admins, which should have the permissions regardless of whether they are a member.

This change lets us require membership for non-admins, but not require it for admins.

lib/types/thread-types-enum.js
144

This collection is only used in threadTypeIsAnnouncementThread, and that function is only used in makePermissionsBlob.

This change makes sure that subtract the announcement set of permissions for GENESIS, just like other announcement threads. This is intentional behavior, as the set of permissions that members have in GENESIS is meant to be even more narrow than it is for other announcement threads.

This revision is now accepted and ready to land.Aug 14 2024, 7:03 AM