Page MenuHomePhabricator
Differential D12543

[keyserver] Use `threadIsWithBlockedUserOnlyWithoutAdminRoleCheck` instead of `threadFrozenDueToBlock` in `checkThreadsFrozen`
Needs RevisionPublic
Actions

Authored by atul on Fri, Jun 21, 2:39 PM.
Tags
None
Referenced Files
F2163808: D12543.id41737.diff
Mon, Jul 1, 9:14 PM
F2163245: D12543.id41738.diff
Mon, Jul 1, 7:36 PM
F2162185: D12543.id41737.diff
Mon, Jul 1, 2:04 PM
F2160932: D12543.id41735.diff
Mon, Jul 1, 10:40 AM
F2157558: D12543.id41624.diff
Mon, Jul 1, 2:05 AM
Unknown Object (File)
Sun, Jun 30, 2:45 AM
Unknown Object (File)
Sun, Jun 30, 1:09 AM
Unknown Object (File)
Thu, Jun 27, 5:00 PM
View All 19 Files
Subscribers
None

Details

Reviewers
ashoat
Summary

Context: https://linear.app/comm/issue/ENG-7249/update-checkthreadsfrozen

We do sequential fetchThreadInfos to get communityThreadInfos so we can determine whether there is an admin without "sketchy" CHANGE_ROLE permission check.

Depends on D12502

Test Plan

flow + set some breakpoints and observed correct behavior:

43c03c.png (360×1 px, 102 KB)

Diff Detail

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

Event Timeline

atul created this revision.Fri, Jun 21, 2:39 PM
atul published this revision for review.Fri, Jun 21, 2:39 PM
Harbormaster failed remote builds in B29881: Diff 41624!Fri, Jun 21, 2:52 PM
atul added a child revision: D12554: [lib] Remove `threadFrozenDueToBlock`, `threadOrParentThreadHasAdminRole` and `memberHadAdminPowers`.Mon, Jun 24, 10:40 AM
atul mentioned this in D12556: [lib] Remove `threadOrParentThreadHasAdminRole` and `memberHadAdminPowers`.Mon, Jun 24, 10:42 AM
atul updated this revision to Diff 41735.Wed, Jun 26, 1:39 PM

rebase outside of flow stack

atul updated this revision to Diff 41737.Wed, Jun 26, 1:44 PM

fix import

atul edited the test plan for this revision. (Show Details)Wed, Jun 26, 1:47 PM
atul removed a parent revision: D12502: [lib] Resolve `flow` issues in `getRelativeMemberInfos`.
Harbormaster failed remote builds in B29941: Diff 41735!Wed, Jun 26, 1:52 PM
Harbormaster completed remote builds in B29943: Diff 41737.Wed, Jun 26, 2:00 PM
atul removed reviewers: ginsu, tomek.Thu, Jun 27, 1:45 PM
atul added a parent revision: D12502: [lib] Resolve `flow` issues in `getRelativeMemberInfos`.
ashoat requested changes to this revision.Mon, Jul 1, 12:59 PM

Concerned about community roots

keyserver/src/fetchers/thread-permission-fetchers.js
173 ↗(On Diff #41737)

Good to clarify it's an ID rather than a RawThreadInfo

173 ↗(On Diff #41737)

For a community root, threadInfo.community will be null

(In fact, that is the only case where threadInfo.community won't be set)

Two questions:

  1. How do we handle this check for community roots? It looks like it doesn't work currently
  2. Do we have any similar issues on the client side? I'm pretty sure threadInfo.community isn't set for community roots there either
200 ↗(On Diff #41737)

What happens if a thread has a community, but we fail to fetch it? It looks like we just assume that the chat has no admin?

Given that all threads on the keyserver either are community roots or have a community, I wonder if we should consider a case where we failed to fetch the community root to be a potential privacy check failure. What do you think?

This revision now requires changes to proceed.Mon, Jul 1, 12:59 PM

Revision Contents
Changeset List

PathSize
keyserver/
src/
fetchers/
55 lines
lib/
shared/
4 lines

Diff 41624

View Options

keyserver/src/fetchers/thread-permission-fetchers.js

Loading...
View Options

lib/shared/thread-utils.js

Loading...