Page MenuHomePhabricator

[keyserver] Fix keyserver allowing adding unauthorized users to chats
ClosedPublic

Authored by inka on Jul 11 2024, 6:54 AM.
Tags
None
Referenced Files
F3193399: D12727.id42280.diff
Fri, Nov 8, 10:39 PM
Unknown Object (File)
Tue, Nov 5, 3:22 AM
Unknown Object (File)
Tue, Nov 5, 3:22 AM
Unknown Object (File)
Fri, Oct 18, 10:29 PM
Unknown Object (File)
Fri, Oct 18, 10:29 PM
Unknown Object (File)
Fri, Oct 18, 10:24 PM
Unknown Object (File)
Sep 25 2024, 8:25 PM
Unknown Object (File)
Sep 25 2024, 8:25 PM
Subscribers
None

Details

Summary

issue: ENG-8801
It should not be possible to create a subchannel in a chat under GENESIS with users the current user is not friends with, even if they are not in the parent chat
It should not be possible to add a user that is not a friend of the current user, even if they are not in the parent chat, to a subchannel of a chat under GENESIS
To see the full explanation of what behaviour we want in all cases, please see this comment

Test Plan

I tested the behaviour of keyserver by removing filtering from UI and allowing it to display all users.
Tested that it is not possible to create a subchannel in GENESIS subchannel that would contain users that are not in parent chat, unless they are our friends

Tested that it is not possible to create a chat through ChatThreadComposer with non-friend (even if UI allows it), or create direct GENESIS subchannels with non-friends (from admin)

Tested that it IS possible to create direct GENESIS subchannels with friends (from admin)

Tested that it IS possible to create subchannel in GENESIS subchannel with members of the parent even if they are not our friends.

Tested that it IS possible to create to create a subchannel in a different community with users that are in this community but we are not friends with.

Tested that it IS NOT possible to create a subchannel in a different community with users that are not in this community and we are not friends with.

Diff Detail

Repository
rCOMM Comm
Branch
inka/fix
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

For chats under GENESIS we want:

  1. for direct GENESIS children we want the containingThreadID to be null
  2. for other chats under GENESIS we want containingThreadID to be the id of parent chat
keyserver/src/fetchers/thread-permission-fetchers.js
374–379

For chats in different communities than GENESIS we require that the user we are trying to add is a member of that community
For GENESIS we require that the user is a member of parent OR current users friend
containingThreadID for chats under GENESIS is the chats parent (or null for direct GENESIS children) because of other changes in this diff

keyserver/src/fetchers/thread-permission-fetchers.js
377–380

We are undoing D6744, because it only handled GENESIS direct children. In this diff we handle all GENESIS descendants

inka requested review of this revision.Jul 11 2024, 7:11 AM

I'd like to avoid querying determineThreadAncestry twice

keyserver/src/creators/thread-creator.js
165–169

The way you've built this, we end up querying determineThreadAncestry twice. Instead, can we keep the old await determineThreadAncestryPromise call here, and pass the result to the new function?

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

Hmm... realized we didn't consider a depth 3 subchannel of GENESIS in my Linear comment here:

GENESIS -> depth 1 -> depth 2 -> depth 3

This logic will lead to the child of depth 3 have a containing thread of depth 2. But in the mental model of "we're treating subchannels of GENESIS like a community", the containing thread should arguably be depth 1.

(However, there's an exception for sidebars, which should have the containing thread set to the parent. See the logic in getContainingThreadID.)

I think to fix this we'd need to pass in the parentContainingThreadID here, which might not be worth it. I don't feel super strongly about this, and if it's too complicated to make this change then we can probably leave it as-is

This revision is now accepted and ready to land.Jul 11 2024, 8:53 PM
keyserver/src/fetchers/thread-fetchers.js
396

Initially I have spent some time trying to implement this lake that, but it seemed like a lot of extra computation. We would have to get all thread infos and traverse them upwards to get the parentContainingThreadID. So we would have to either call the db or keep this in keyservers memory.
(Then I realised your comment said "parent", and figured you said that precisely because the other option is complicated)

Don't call determineThreadAncestry twice

ashoat added inline comments.
keyserver/src/creators/thread-creator.js
166 ↗(On Diff #42253)

Can we rename to containingThreadIDForPossibleMemberResolution?

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

Thanks for trying!

keyserver/src/updaters/thread-updaters.js
590 ↗(On Diff #42253)

Can we rename to containingThreadIDForPossibleMemberResolution?

Rename to containingThreadIDForPossibleMemberResolution