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
Details
Details
flow + set some breakpoints and observed correct behavior:
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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:
|
| 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? |
| keyserver/src/fetchers/thread-permission-fetchers.js | ||
|---|---|---|
| 173 ↗ | (On Diff #41737) |
That's a good point. We're not properly handling the case where one of the entries in threadInfos is a community root. We will have fetched the community threadInfo in the initial call to fetchThreadInfos, but it won't be included in communityThreadInfos which is what communityRootMembersToRole is derived from. It doesn't make sense to fetch the same threadInfo twice, so I think the best bet is to concatenate threadInfos and communityThreadInfos like: const combinedThreadInfos = { ...threadInfos, ...communityThreadInfos, }; and compute communityRootMembersToRole from combinedThreadInfos. Think it would look something like: 
We don't have this issue on the client because there's no sequential "fetching" of community root ThreadInfos. We have all of the ThreadInfos for all the community roots in Redux and call _mapValues with a collection that includes relevant community roots. There is similar logic on the client in useCommunityRootMembersToRole which is consumed by useThreadsWithPermission and useThreadFrozenDueToViewerBlock.
So I don't think there are any problems on the client side. |
| 200 ↗ | (On Diff #41737) | Yeah, it wouldn't hurt to check that thread's community root was successfully fetched. Will update this diff. |
Comment Actions
construct communityRootMembersToRole with both threadInfos and communityThreadInfos to handle scenario described here: https://phab.comm.dev/D12543?id=41737#inline-73658 where community root is one of the threadIDs passed into checkThreadsFrozen.
Set breakpoint and observed that combinedThreadInfos and communityRootMembersToRole were constructed as expected:
Comment Actions
update communityMembersToRole within for loop to fallback to threadID if threadInfo.community is null (which means it's a community root itself).
| keyserver/src/fetchers/thread-permission-fetchers.js | ||
|---|---|---|
| 200 ↗ | (On Diff #41737) | Haven't addressed this point yet, will take closer look in the morning.
How could we handle that case? My understanding is that this scenario would imply that the data in keyserverDB is malformed? Or maybe there's some race condition where a community is eg deleted in between the calls to fetchThreadInfos (haven't thought this through)? Does that mean we should throw some sort of Error and reject promise? |
Comment Actions
Solution to community root issue looks good! Passing back to figure out what to do if communityMembersToRole isn't present. Might be worth also considering a scenario where it's present, but there is no m.id key
| keyserver/src/fetchers/thread-permission-fetchers.js | ||
|---|---|---|
| 201–202 ↗ | (On Diff #42098) | Can we add something like this comment above this line? // We fall back to threadID because the only case where threadInfo.community // is not set is for a community root, in which case the thread's community // root is the thread itself. |
| 200 ↗ | (On Diff #41737) | My initial thought was just to fail the permission check, ie. not return that threadID. I don't think we need to throw an exception I'm not 100% sure about failing the permissions check... it feels like the right thing, but I'd appreciate if you could analyze the repercussions |
| keyserver/src/fetchers/thread-permission-fetchers.js | ||
|---|---|---|
| 200 ↗ | (On Diff #41737) | Added "early continue" check that looks like: if ( threadInfo.community && !(threadInfo.community in communityRootMembersToRole) ) { continue; } in latest revision |
Comment Actions
Passing back to figure out what to do if communityMembersToRole isn't present.
Is this the same concern as https://phab.comm.dev/D12543?id=41737#inline-73702 or is this a separate scenario?
With the
if ( threadInfo.community && !(threadInfo.community in communityRootMembersToRole) ) { continue; }
check we're handling case that community root fetch fails. Only other way communityMembersToRole isn't present is if communityRootMembersToRole[threadID] isn't present which shouldn't be possible?
Might be worth also considering a scenario where it's present, but there is no m.id key
Wouldn't it be impossible for the member to be an admin if there's no m.id key? Not sure I understand the scenario in which this would occur
Please let me know if I'm missing something obvious
Comment Actions
Just jotting down my understanding
- The high level purpose of checkThreadsFrozen is to see if
- the permissions in permissionsToCheck are affected by blocks
- if the thread is "with blocked users only," in which case we add it to the threadIDsWithDisabledPermissions set.
- The memberHasAdminRole check is to potentially "override" threadIsWithBlockedUserOnly... check and exclude thread from threadIDsWithDisabledPermissions
My initial thought was just to fail the permission check, ie. not return that threadID. I don't think we need to throw an exception
Therefore if the database result set is malformed and we aren't able to properly check if there's a member that's an admin in community root, shouldn't we opt to include the threadID in threadIDsWithDisabledPermissions rather than exclude it? Think it'd make sense to err on the side of narrower vs. more expansive permissions if there's an error case?
I think I'm misunderstanding the error scenarios and the logic needed to handle them. Personal instinct would be to throw some sort of error if there's malformed data vs. having to add logic to try to account for scenarios that shouldn't(?) occur?
Definitely let me know if there's something I'm misunderstanding. Will continue reading through code to understand better since it's been a second.
EDIT: Ignore, confused myself.
Comment Actions
JK, this should be the correct logic. Think I got thrown off by
My initial thought was just to fail the permission check, ie. not return that threadID.
checkThreadsFrozen return value is threadIDs that fail permission check, so we should include those threadIDs in threadIDsWithDisabledPermissions.
The following logic
if ( threadInfo.community && !(threadInfo.community in communityRootMembersToRole) ) { threadIDsWithDisabledPermissions.add(threadID); continue; }
is basically failing permission check immediately upon malformed/missing community root before even checking threadIsWithBlockedUserOnlyWithoutAdminRoleCheck or erroneously falling back to communityRootMembersToRole[threadID] if thread is NOT community root and communityRootMembersToRole[threadInfo.community] should be present.
Comment Actions
Therefore if the database result set is malformed and we aren't able to properly check if there's a member that's an admin in community root, shouldn't we opt to include the threadID in threadIDsWithDisabledPermissions rather than exclude it? Think it'd make sense to err on the side of narrower vs. more expansive permissions if there's an error case?
I think I'm misunderstanding the error scenarios and the logic needed to handle them. Personal instinct would be to throw some sort of error if there's malformed data vs. having to add logic to try to account for scenarios that shouldn't(?) occur?
Yeah I think it would be better to assume that the permissions is missing if it can't be fetched, rather than assuming it's present.
Open to throwing an error as well, but if we do that we'll need to do an analysis of the impact... eg. will it cause some callsite to break partway through? We wouldn't want to (for instance) result in a thread being created with no messages, or an account being created with no chats.
Comment Actions
We wouldn't want to (for instance) result in a thread being created with no messages, or an account being created with no chats.
Ah yeah, there's a lot more to consider with that. Will land this approach.