[lib] Update `threadMemberHasPermission` to support `MinimallyEncodedRawThreadInfo`
ClosedPublic
Actions

Authored by atul on Nov 13 2023, 11:19 AM.

Details

Reviewers

ashoat
ginsu
tomek
rohan

Commits

rCOMMf6cf1bfc29fd: [lib] Update `threadMemberHasPermission` to support…

Summary

Check minimally encoded permissions for permission via hasPermission in threadMemberHasPermission based on minimallyEncoded flag.

NOTE: One thing I noticed when I was making this change was that permissionLookup always checks for the threadPermissions.KNOW_OF permission during permissions checks and fails all checks where KNOW_OF isn't present. Looks like this logic was added by @ashoat in https://phab.comm.dev/D1213. If this is still relevant, should I update hasPermission to always ensure that KNOW_OF bit is set?

Depends on D9840

Test Plan

Will add unit tests in future, for now counting on Flow.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Nov 13 2023, 11:19 AM

Herald added a subscriber: will. · View Herald TranscriptNov 13 2023, 11:19 AM

atul published this revision for review.Nov 13 2023, 11:28 AM

atul added a child revision: D9842: [keyserver] Remove `test-threadstore-reductions` script.Nov 13 2023, 11:49 AM

Harbormaster completed remote builds in B24047: Diff 33134.Nov 13 2023, 11:52 AM

If this is still relevant, should I update hasPermission to always ensure that KNOW_OF bit is set?

Yes, absolutely. The default should definitely be "match existing code" – if you ever have a question like this in the future for this project, please default to matching existing code, instead of doing things differently and hoping your reviewer will notice a question in the diff summary.

This revision is now accepted and ready to land.Nov 13 2023, 12:14 PM

In D9841#287271, @ashoat wrote:

If this is still relevant, should I update hasPermission to always ensure that KNOW_OF bit is set?

Yes, absolutely. The default should definitely be "match existing code" – if you ever have a question like this in the future for this project, please default to matching existing code, instead of doing things differently and hoping your reviewer will notice a question in the diff summary.

Makes sense, will update hasPermission. Had previously looked at:

when implementing hasPermission so didn't come across the KNOW_OF requirement. I'm guessing that threadHasPermission should also be updated to "go through" permissionLookup?

atul mentioned this in D9844: [lib] Update `hasPermission` to check `KNOW_OF`.Nov 13 2023, 12:44 PM

Updated hasPermission here: https://phab.comm.dev/D9844

I'm guessing that threadHasPermission should also be updated to "go through" permissionLookup?

Was initially guessing that's not necessary since I figured permissionLookup gets called for each individual permission on the keyserver before it's delivered to the client. So if a blob is missing KNOW_OF, on the client it should appear completely empty (no permissions).

That said, did a git grep and saw a threadHasPermission was recently introduced on keyserver. It's kind of risky to leave threadHasPermission in this weird state where it should only be used in certain places – probably best to update it as you suggest.

Before landing, can you create a task to track fixing up threadHasPermission?