Page MenuHomePhabricator

[lib] Add support for new member_ prefix to RawThreadInfo encoders
ClosedPublic

Authored by ashoat on Aug 7 2024, 1:23 PM.
Tags
None
Referenced Files
F2770401: D13015.diff
Thu, Sep 19, 8:37 PM
Unknown Object (File)
Sun, Sep 8, 4:55 PM
Unknown Object (File)
Sat, Sep 7, 1:47 PM
Unknown Object (File)
Sat, Sep 7, 1:47 PM
Unknown Object (File)
Sat, Sep 7, 11:37 AM
Unknown Object (File)
Sat, Sep 7, 11:36 AM
Unknown Object (File)
Fri, Sep 6, 3:16 PM
Unknown Object (File)
Wed, Sep 4, 8:08 PM
Subscribers
None

Details

Summary

I added the bits for the new prefix as high-order bits to maintain backwards compatibility.

Our new prefix only adds 1 bit to the minimally encoded role permission strings, we'll avoid triggering tHexEncodedRolePermission validation failure on existing clients.

Depends on D13014

Test Plan

The whole stack was tested as follows:

  1. Unit tests from D9686, which toggle user-surfaced permissions on and off and make sure no difference is caught. This ensures that the original issue introduced in D9686 isn't reintroduced
  2. Careful review of each descendant permission removed in D9686
  3. Create a community as userA and add userB. Grant tagging permissions to all members. Make sure userB can tag inside non-root channels
  4. Do above, then create a channel without userB, and make sure userB can't tag there either (or do anything other than view). This is the repro described here
  5. Do above, but also create a thread inside the channel (as userA) and make sure userB can't do anything inside the thread other than view, until they join the parent channel

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 7 2024, 1:47 PM
Harbormaster failed remote builds in B30987: Diff 43232!
ashoat requested review of this revision.Aug 7 2024, 1:48 PM
This revision is now accepted and ready to land.Aug 8 2024, 3:14 AM
lib/permissions/minimally-encoded-thread-permissions.js
210–213 ↗(On Diff #43232)

It might be a good idea to add a code comment explaining why the order here is different than in constructThreadPermissionString.

lib/permissions/minimally-encoded-thread-permissions.js
210–213 ↗(On Diff #43232)

This appears to be an error on my part... the returned string should have basePermissionString at the end

I can add an explanation elsewhere for why membershipPrefix is at the start if that's helpful, though

lib/permissions/minimally-encoded-thread-permissions.js
210–213 ↗(On Diff #43232)

I think the best way to avoid this error going forward is to use constructThreadPermissionString here. That function is introduced in the next diff, so instead of fixing this here I'll fix it in a follow-up diff

lib/permissions/minimally-encoded-thread-permissions.js
210–213 ↗(On Diff #43232)

For context, some history on what happened here – I initially had this right, and I think I tested it when it was correct. But then I made a mistake and accidentally lost some small amount of work, and had to reconstruct it. During reconstruction, I think I got this wrong

  1. Fix order of permission string construction. Will replace with constructThreadPermissionString in a later diff
  2. Add a comment explaining why membershipPrefix is at the front of the minimally-encoded permission string, rather than the end
lib/permissions/minimally-encoded-thread-permissions.js
211–216 ↗(On Diff #43263)

Replaced with constructThreadPermissionString in D13034