Details

Reviewers

ashoat
tomek

Commits

rCOMM3cd205451a32: [keyserver] Strip `member.permissions` for `FUTURE_CODE_VERSION` clients

Summary

Add stripMemberPermissions option to rawThreadInfoFromServerThreadInfo to strip member permissions for new clients.

Depends on D12763

Test Plan

Did some basic testing where I hardcoded stripMemberPermissions being passed to rawThreadInfoFromServerThreadInfo and set breakpoints to check output.

Will do additional testing with validators + after getting basic client migration working.

With stripMemberPermissions=false:

(can observe that each of the two members have permissions field set to string)

With stripMemberPermissions=true:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Jul 16 2024, 2:45 PM

atul published this revision for review.Jul 16 2024, 2:49 PM

atul added inline comments.

lib/shared/thread-utils.js
897–906 ↗	(On Diff #42361)	Wanted to specify exact type of object before "crossing boundary" and making call to `stripMemberPermissionsFromRawThreadInfo`. The output of `rawThreadInfoFromServerThreadInfo` is "enforced" at runtime via validators which makes this a "safe" `any`-cast IMO.

Harbormaster completed remote builds in B30398: Diff 42361.Jul 16 2024, 3:18 PM

ashoat requested changes to this revision.Jul 17 2024, 12:29 PM

ashoat added inline comments.

keyserver/src/fetchers/thread-fetchers.js
311–314 ↗	(On Diff #42361)	Isn't this backwards? Don't we want to strip member permissions for newer clients, not for older clients?
lib/shared/thread-utils.js
875 ↗	(On Diff #42361)	It looks like this will be `true` for modern clients: const specialRoleFieldSupported = hasMinCodeVersion(viewer.platformDetails, { native: 336, web: 79, }); Doesn't that mean we'll never strip the member permissions?

This revision now requires changes to proceed.Jul 17 2024, 12:29 PM

atul added a child revision: D12787: [lib] Add `memberInfoSansPermissionsValidator` to `convertClientDBThreadInfoToRawThreadInfo`.Jul 17 2024, 12:36 PM

rebase after addressing feedback in previous diffs in stack, still need to fix this diff up

Lot of things that need to be fixed up here.

atul added inline comments.Jul 17 2024, 2:28 PM

keyserver/src/fetchers/thread-fetchers.js
311–314 ↗	(On Diff #42361)	Yeah, will flip.
lib/shared/thread-utils.js
875 ↗	(On Diff #42361)	Yeah this came up pretty immediately during testing, will update

fix logic, still need to do more testing

Harbormaster completed remote builds in B30439: Diff 42415.Jul 17 2024, 2:58 PM

Updated test plan after some more testing.

Harbormaster completed remote builds in B30442: Diff 42418.Jul 17 2024, 3:14 PM

atul added a child revision: D12788: [lib] Introduce `stripMemberPermissionsFromRawThreadInfos`.Jul 17 2024, 3:34 PM

ashoat accepted this revision.Jul 18 2024, 10:29 AM

ashoat added inline comments.

lib/shared/thread-utils.js
869 ↗	(On Diff #42418)	Is the "deprecated" name really appropriate here? It seems like it's deprecated on the client, but will always be necessary in `keyserver` to go from a `ServerThreadInfo` to a type that works on clients (minimally-encoded)

This revision is now accepted and ready to land.Jul 18 2024, 10:29 AM

rebase

atul added inline comments.Jul 18 2024, 12:50 PM

lib/shared/thread-utils.js
869 ↗	(On Diff #42418)	That's fair, wanted to avoid "Legacy" but something like `minimallyEncodeRawThreadInfoSansMemberPermissions` or something might work.
869 ↗	(On Diff #42418)	*`minimallyEncodeRawThreadInfoWithMemberPermissions`