Page MenuHomePhabricator

[DRAFT][native] Introduce `persistMigrationForThreadAvatarPermission` to update thread permissions
AbandonedPublic

Authored by atul on Apr 19 2023, 10:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 22, 7:10 PM
Unknown Object (File)
Mon, Apr 22, 6:39 AM
Unknown Object (File)
Mon, Apr 22, 6:39 AM
Unknown Object (File)
Mon, Apr 22, 6:34 AM
Unknown Object (File)
Mar 4 2024, 3:18 AM
Unknown Object (File)
Mar 4 2024, 3:18 AM
Unknown Object (File)
Mar 4 2024, 3:14 AM
Unknown Object (File)
Feb 26 2024, 11:10 PM
Subscribers

Details

Reviewers
ashoat
ginsu
Summary

As part of introducing thread avatars, we added edit_thread_avatar and descendant_edit_thread_avatar permissions to role-creator.js. Roles and permissions for all threads on prod keyserver were updated to include these permissions via a recent updateRolesAndPermissionsForAllThreads migration.

If during a state check a client's threadStore is "out of sync" with the keyserver, the client will need to pull down ALL threadInfos from the keyserver. This isn't the end of the world and doesn't break anything, but it can degrade the user experience and is something we want to avoid.

The first step in doing that was filtering out certain permissions on the keyserver when computing threadInfos to be compared against those on the client. To handle this we introduced filterThreadPermissions in thread-utils to filter out *edit_thread_avatar permissions.

However, now that we want to "switch on" avatars for new clients we WILL want the keyserver to be including *edit_thread_avatar permissions in the threadInfos it computes. Because the client won't have the *edit_thread_avatar permissions persisted, there would be an inconsistency the first time the client connects to keyserver after updating the app to a codeVersion that supports avatars.

To handle this initial inconsistency, we want to introduce a migration that modified the client's persisted roles/permissions in the threadStore AFTER we update the client but BEFORE we establish connection with the keyserver.

We've done this twice in the past with edit-thread-permission-migration and manage-pins-permission-migration. I used those two migrations as a guide and tried to follow them as closely as possible.

Test Plan

I tested versions of this code as I was iterating and things seemed to work with my very new dev DB that has very few threads/thread types.

However, I haven't tested the current state of the diff at all. I'm putting it up as a DRAFT to try to surface any glaring issues as soon as possible, but I need to do more careful testing before this is ready.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul requested review of this revision.Apr 19 2023, 10:39 PM
atul added inline comments.
native/redux/edit-thread-avatar-permission-migration.js
14–20

From the previous filterThreadPermissions work I knew we were excluding edit_thread_avatar from PERSONAL and PRIVATE threads. I tried to read through role-creators to try to reason about which threadTypes should have Members permission blob that include edit_thread_avatar but found it a bit confusing.

Instead, I figured the best bet was to just call the getRolePermissionBlobs(...) function in role-creator with every threadType and see which ones included edit_thread_avatar:

b034ef.png (1×2 px, 460 KB)

Why not just move getRolePermissionBlobs(...) to lib and call it directly from this migration?

I considered doing that, but getRolePermissionBlobs(...) will always have the role permissions blob for the CURRENT version of keyserver. Which means if someone has an old client (let's say hasn't been updated in a year) and only updates far into the future (let's say a year from now), getRolePermissionBlobs(...) could have changed drastically and this migration, and any others that may depend on it, could break.

This Set is a "snapshot" of getRolePermissionBlobs that's needed for the client upgrade that supports avatars and the edit_thread_avatar permission.

30–36

My understanding is that the edit_thread_avatar "user" permission entry will be included for every member with role Members whether or not the permission is granted.

My understanding is that the permission will have a value of true and source set to the threadID of the threadInfo it's within IFF the Members role for the threadType includes the edit_thread_avatar permission.

My understanding is that it's not possible for Members to inherit permissions from ancestor threads they are descendants of... so if the current threadType does not grant Members edit_thread_avatar permission, the value will be false and the source null.

37–42

My understanding is that if a thread member does not have role name Members, then they are either an Admin or parent Admin (role is null).

If they are admin of a parent thread it's possible (guaranteed?) that they implicitly have edit_thread_avatar permissions via a "higher" descendant_edit_thread_avatar permission, but it's difficult to ascertain the source threadID.

What we're taking advantage of here is that edit_thread_color and descendant_edit_thread_color are included in the Admins permission blob for the same three threadTypes as edit_thread_avatar and descendant_edit_thread_avatar. That means that permissions will "cascade" down from threads the user is admin of in the same way. That means we can use edit_thread_color as a proxy for edit_thread_avatar when determining value and source of the permission entry.

56–65

There are three threadTypes where the Admins permissions include edit_thread_avatar and descendant_edit_thread_avatar:

58a7a3.png (268×628 px, 27 KB)

Those are also the only three threadTypes that have an Admins ThreadRolePermissionsBlob at all... so we can just add edit_thread_avatar and descendant_edit_thread_avatar to every Admins permissions blob that we have persisted.

native/redux/persist.js
508–559

This migration very closely matches the threadStore-related portions of @rohan's manage-pins-permission migration (which is migration 36 right above this one).

I'm fairly confident in the correctness of this code vs. the code in edit-thread-avatar-permission-migration which I'm not so confident about.

1368f0.png (196×956 px, 40 KB)

Okay well as a starting point trying the migration with my already correct threadStore led to some inconsistencies...

I'll try to debug and get things correct ASAP tomorrow morning, but it's clear that it's def not landable as is.

While the changes in edit-thread-avatar-permission need to be revisited, the code in persist.js is solid (shoutout @rohan who wrote it).

Personally, since we know that the code in persist.js is reliable and well tested my preference would be to put up a diff that extracts the functionality out into a function similar to unshimClientDB in unship-utils.js that can be consumed by a later diff which handles recalculating permissions and roles. That way we could "lock in" the scaffolding and consume it later with the roles/permission recalculation code.

However, that function would be "dangling" (no callsites) until the role/permission recalculating code is ready. This forces us to include two complicated chunks of logic in the same diff which IMO blows up complexity for reviewer to handle in a single diff.

Not suggesting we do that, just a thought given discussion about putting up "dangling" functionality for review.

(Talked through that offline with @atul and we agreed the "dangling" stuff would be fine)

ashoat requested changes to this revision.Apr 21 2023, 6:04 AM

Requesting changes for a migration that takes no params and just recalculates all thread permissions from "first principles" based on role-creator.js that is extracted to lib

This revision now requires changes to proceed.Apr 21 2023, 6:04 AM

Don't need this anymore, handled by other diffs