[lib][keyserver] Remove ADD_MEMBERS permission from community roots
ClosedPublic
Actions

Authored by inka on May 29 2024, 2:01 AM.

Details

Reviewers

ashoat
tomek
kamil

Commits

rCOMMb65f4f3942e1: [lib][keyserver] Remove ADD_MEMBERS permission from community roots

Summary

issue: ENG-7792
baseMemberUserSurfacedPermissions is used only in getRolePermissionBlobsForCommunityRoot. userSurfacedPermissions.ADD_MEMBERS permission was parsed by getThreadPermissionBlobFromUserSurfacedPermissions and resulted in two permissions being added to the resultant memberPermissions: add_members and child_open_add_members. We want the permissions for children to stay the same, so we make sure only add_members permission is removed.

But we have to make sure that old clients still get the ADD_MEMBERS permission. This is because otherwise, state check would have found many, many inconsistencies, likely resulting in socket crash loop on prod. So the client needs to get this permission in thread info, but the request to add members should fail[[https://linear.app/comm/issue/ENG-7792/disallow-adding-users-to-community-roots-on-the-keyserver#comment-3c202516 | More datails in discussion]]

Lastly, we should migrate old thread infos on the keyservers and clients. But this is very costly, so it will be done with D12062

Test Plan

Tested new clietns (setting min code version to current code version):
Created a community. Tested that the admin and member cannot add users. Created a subchannel. Checked that both member and admin have the Add members button and checked from the member that it works.

Checked old clients by changing min code version to 999:
Created a new community → it shouldn’t allow adding new users to its root. Checked that the client thinks they can add users (button is visible). Checked that trying to actually add a user results in a server error (invalid_credentials)

Diff Detail

Repository

rCOMM Comm

Branch

inka/userStore

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.May 29 2024, 2:01 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMay 29 2024, 2:01 AM

inka added a parent revision: D12220: [lib] Render UserInfosHandler.May 29 2024, 2:02 AM

inka edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B29276: Diff 40737.May 29 2024, 2:18 AM

inka requested review of this revision.May 29 2024, 2:18 AM

inka added inline comments.May 29 2024, 2:21 AM

keyserver/src/fetchers/thread-fetchers.js
295–296 ↗	(On Diff #40737)	These will have to be updated to the proper number when we release a new app version. Not yet sure how to enforce that
lib/permissions/thread-permissions.js
232 ↗	(On Diff #40737)	This function calculated permissions for members and admins of community roots. There are three types of community roots: `COMMUNITY_ROOT`, `COMMUNITY_ANNOUNCEMENT_ROOT` and `GENESIS` member permissions are calculated by `getThreadPermissionBlobFromUserSurfacedPermissions` based on `memberUserSurfacedPermissions`. So member permissions are changed by updating `addMembersPermissions` in `thread-permission-types.js`, which are used by `getThreadPermissionBlobFromUserSurfacedPermissions`. admin permissions are specified in this function in `baseAdminPermissions`. So they are changed directly in this function
322–326 ↗	(On Diff #40737)	Because we have to add the `ADD_MEMBERS` permission back for old clients, but for GENESIS only admin members have it, I opted to keep it in GENESIS at all times. This greatly simplifies the code in `rawThreadInfoFromServerThreadInfo` Before multikeyservers are launched ENG-7134 will be implemented, so secondary keyservers will not have GENESIS. So this is only for the auth keyserver, on which the only admin is Ashoat. All users are members of the auth keyserver, so the problem we are trying to solve by disallowing adding users to communities doesn't apply to GENESIS
lib/shared/thread-utils.js
600 ↗	(On Diff #40737)	Please see this discussion for an explanation why this `source` is used

inka edited the summary of this revision. (Show Details)May 29 2024, 2:25 AM

inka added reviewers: ashoat, tomek, kamil.

Use FUTURE_CODE_VERSION

Harbormaster completed remote builds in B29277: Diff 40738.May 29 2024, 2:48 AM

I think this is causing me to be unable to send friend requests...
EDIT: its also happening on master, for two users who din't have a undirected relationship. I will investigate some more
EDIT2: it looks like one of my users had some broken state. this is not happening in general

inka added a child revision: D12062: [keyserver/lib] introduce MANAGE_FARCASTER_CHANNEL_TAGS permission.May 29 2024, 6:51 AM

inka mentioned this in D12062: [keyserver/lib] introduce MANAGE_FARCASTER_CHANNEL_TAGS permission.

inka added a child revision: D12236: [web] Stop navigating to Add Members modal from community creation.May 29 2024, 7:32 AM

This looks good, just one question inline and a request for the Test Plan: can you try to test the state check mechanism?

In particular, here's what I'm thinking:

Create a test user that is a member of some community (not just GENESIS). Make sure that the community has some test chats in it (ideally one private, one public, maybe some sidebars inside)
Compile a release build on master and deploy it to eg. a simulator. Log in as the test user
Update your local keyserver to run on this diff
Confirm that the state check mechanism does not detect any issues

keyserver/src/fetchers/thread-fetchers.js
298–299	As part of the release process, I check all `NEXT_CODE_VERSION` and update it to the release I'm about to make `FUTURE_CODE_VERSION` isn't checked, and is meant as a stand-in for when somebody wants to land something, but isn't ready for it to be released Once you're ready for this to be released, you should update this to `NEXT_CODE_VERSION`
lib/permissions/thread-permissions.js
325	You justified this change earlier by mentioning "This greatly simplifies the code in `rawThreadInfoFromServerThreadInfo`". Can you explain this in more detail? I'm wondering what exactly is special about the `GENESIS` case here

This revision now requires changes to proceed.May 29 2024, 10:21 AM

inka edited the summary of this revision. (Show Details)Jun 3 2024, 1:52 AM

inka added inline comments.Jun 3 2024, 2:02 AM

lib/permissions/thread-permissions.js
325	Non-admin members don't have a `ADD_MEMBERS` permission in GENESIS - only the admin. Both non-admin and admin members have this permission in other community roots. So this allows me to not have to check in `filterThreadPermissions` in `rawThreadInfoFromServerThreadInfo` whether the permissions I am editing are supposed to be used for admins, or other members.

I had trouble getting a release build to connect to my local keyserver, so instead I built a dev app and disconnected from metro.
What I did:

Built the app and run the ks on master
Logged onto a user who is only in GENESIS, create a new community, create secret and open chats, and some threads in this community
Killed metro
Checked out my branch
Logged stateCheckStatus in handleResponsesClientSocketMessage on the keyserver (metro is down so I cannot see client logs)

the value was always { status: 'state_validated' }

ashoat added inline comments.Jun 3 2024, 12:04 PM

keyserver/src/fetchers/thread-fetchers.js
14	Don't forget to change this when you want this to ship. If you land this with `FUTURE_CODE_VERSION` and forget to update to `NEXT_CODE_VERSION`, this work will never ship
lib/permissions/thread-permissions.js
325	I spent over 30 minutes trying to understand this. I really feel like you could have spent like 5-10min improving your explanation here, and it would have saved me a lot of time. Concretely: there's no explanation here for why you're leaving admin permissions for `GENESIS` unchanged. Is it because something breaks if you remove the `ADD_MEMBERS` permission for `GENESIS`? Or is it because it only affects my UX (not user-facing) and you wanted to avoid the extra work that would be necessary? I'm going to assume that it's the latter and accept this, but please think carefully about this feedback. In the future, I'd appreciate it if you could spend more time on your wording and explanations in diff comments. Think about what context I might be missing, and try rewording your comment multiple times.

ashoat accepted this revision.Jun 3 2024, 12:04 PM

This revision is now accepted and ready to land.Jun 3 2024, 12:04 PM

inka added inline comments.Jun 4 2024, 4:46 AM

keyserver/src/fetchers/thread-fetchers.js
14	This should be changed when we run migrations in D12062. I think this code can be landed before we change this to `NEXT_CODE_VERSION` . It works all right without changing this - clients will get `ADD_MEMBERS` permission for both old community roots that actually have it, and for new community roots that won't have it. But the UI (changes from next diffs in this stack) will not allow adding the users anyway, so this is not a problem. Once D12062 is landed, clients and keyserver will migrate the data, and then the keyserver should stop sending to the updated clients the `ADD_MEMBERS` permission. I commented on D12062 to make sure we change this flag in the diff that adds those migrations.
lib/permissions/thread-permissions.js
325	From your question You justified this change earlier by mentioning "This greatly simplifies the code in rawThreadInfoFromServerThreadInfo". Can you explain this in more detail? I'm wondering what exactly is special about the GENESIS case here I thought that This greatly simplifies the code in rawThreadInfoFromServerThreadInfo is the only part of my explanation which you didn't understand. So I explained how is the implementation simplified by this.
325	Or is it because it only affects my UX (not user-facing) and you wanted to avoid the extra work that would be necessary? Yes, the reason is that this only affects your account in production, while not being less correct than before. We are disallowing adding users to community roots, because the keyservers don't know user relationships, and this knowledge is needed to know who can add whom to chats. But the auth keyserver knows user relationships. So this issue does not apply to GENESIS. So for GENESIS it is still equally correct as it was before.