Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Fix adding excluded users to the private room sharing tables when joining a room #11143

Merged
DMRobertson merged 9 commits into from
Oct 21, 2021
+67 −29
Merged

Fix adding excluded users to the private room sharing tables when joining a room #11143

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed6bcec
We only need to fetch users in private rooms
Oct 20, 2021
60587bd
Filter out `user_id` at the top
Oct 20, 2021
6b19eda
Discard excluded users in the top loop
Oct 20, 2021
2a3c396
The caller must check that `user_id` is included
Oct 20, 2021
6238017
Test joining a private room with an excluded user
Oct 21, 2021
00c5d9e
Tweak an existing test
Oct 21, 2021
56809e9
Changelog
Oct 21, 2021
67f6373
test docstring
Oct 21, 2021
02354e5
lint
Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11143.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug where users excluded from the directory could still be added to the `users_who_share_private_rooms` table after a regular user joins a private room.
28 changes: 13 additions & 15 deletions synapse/handlers/user_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,31 +373,29 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None:
is_public = await self.store.is_room_world_readable_or_publicly_joinable(
room_id
)
other_users_in_room = await self.store.get_users_in_room(room_id)

if is_public:
await self.store.add_users_in_public_rooms(room_id, (user_id,))
else:
users_in_room = await self.store.get_users_in_room(room_id)
other_users_in_room = [
other
for other in users_in_room
if other != user_id
and (
not self.is_mine_id(other)
or await self.store.should_include_local_user_in_dir(other)
)
]
to_insert = set()

# First, if they're our user then we need to update for every user
if self.is_mine_id(user_id):
if await self.store.should_include_local_user_in_dir(user_id):
for other_user_id in other_users_in_room:
if user_id == other_user_id:
continue

to_insert.add((user_id, other_user_id))
for other_user_id in other_users_in_room:
to_insert.add((user_id, other_user_id))

# Next we need to update for every local user in the room
for other_user_id in other_users_in_room:
if user_id == other_user_id:
continue

include_other_user = self.is_mine_id(
other_user_id
) and await self.store.should_include_local_user_in_dir(other_user_id)
if include_other_user:
if self.is_mine_id(other_user_id):
to_insert.add((other_user_id, user_id))

if to_insert:
Expand Down
59 changes: 43 additions & 16 deletions tests/handlers/test_user_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,22 +646,18 @@ def test_private_room(self) -> None:
u2_token = self.login(u2, "pass")
u3 = self.register_user("user3", "pass")

# We do not add users to the directory until they join a room.
# u1 can't see u2 until they share a private room, or u1 is in a public room.
s = self.get_success(self.handler.search_users(u1, "user2", 10))
self.assertEqual(len(s["results"]), 0)

# Get u1 and u2 into a private room.
room = self.helper.create_room_as(u1, is_public=False, tok=u1_token)
self.helper.invite(room, src=u1, targ=u2, tok=u1_token)
self.helper.join(room, user=u2, tok=u2_token)

# Check we have populated the database correctly.
shares_private = self.get_success(
self.user_dir_helper.get_users_who_share_private_rooms()
)
public_users = self.get_success(
self.user_dir_helper.get_users_in_public_rooms()
)

users, public_users, shares_private = self.get_success(self.user_dir_helper.get_tables())
self.assertEqual(users, {u1, u2, u3})
self.assertEqual(shares_private, {(u1, u2, room), (u2, u1, room)})
self.assertEqual(public_users, set())

Expand All @@ -680,14 +676,9 @@ def test_private_room(self) -> None:
# User 2 then leaves.
self.helper.leave(room, user=u2, tok=u2_token)

# Check we have removed the values.
shares_private = self.get_success(
self.user_dir_helper.get_users_who_share_private_rooms()
)
public_users = self.get_success(
self.user_dir_helper.get_users_in_public_rooms()
)

# Check this is reflected in the DB.
users, public_users, shares_private = self.get_success(self.user_dir_helper.get_tables())
self.assertEqual(users, {u1, u2, u3})
self.assertEqual(shares_private, set())
self.assertEqual(public_users, set())

Expand All @@ -698,6 +689,42 @@ def test_private_room(self) -> None:
s = self.get_success(self.handler.search_users(u1, "user3", 10))
self.assertEqual(len(s["results"]), 0)

def test_joining_private_room_with_excluded_user(self) -> None:
# Setup a support and two normal users.
alice = self.register_user("alice", "pass")
alice_token = self.login(alice, "pass")
bob = self.register_user("bob", "pass")
bob_token = self.login(bob, "pass")
support = "@support1:test"
self.get_success(
self.store.register_user(
user_id=support, password_hash=None, user_type=UserTypes.SUPPORT
Comment on lines +710 to +713
Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it valid to register using a full user ID?
I guess it is since you're doing it, but it doesn't match the style above and draws too much attention to the server ID in the user ID (this isn't a remote user, right?)

Copy link
Copy Markdown
Contributor Author

@DMRobertson DMRobertson Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cribbed from old tests, see e.g. here which I think is circa three years old.

I still don't really know how support users get created in actual usage. (If it's "make a normal user, then mark them as support in the DB", they'll get added to the directory on registration and then not removed when made support.)

Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see.

I did not notice you were using self.store.register_user here and not self.register_user.

I think (hope) that support users are born that way (e.g. using a CLI flag, or an admin GUI tool that will do shared-secret registration?):

Maybe we should one day extend self.register_user to allow passing a user type (it uses the shared-secret registration API, to my surprise — we could likely crib from the script linked above to add support under an optional kwarg).

Happy for you to leave as-is if you like though

)
)

# Alice makes a room. Inject the support user into the room.
room = self.helper.create_room_as(alice, is_public=False, tok=alice_token)
self.get_success(inject_member_event(self.hs, room, support, "join"))
Comment on lines +718 to +719
Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In keeping with remark above, I would probably prefer to do the invite & join dance rather than injecting the event (as surely the support user is a local user?). Maybe I'm being a hypocrite and we already do this nearby? :P
Though I do think I prefer to reserve injecting events for when we have to (e.g. simulating remote events or invalid events that we've already persisted for some reason)

Copy link
Copy Markdown
Contributor Author

@DMRobertson DMRobertson Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm being a hypocrite and we already do this nearby? 📦

Not a hypocrite. I do this elsewhere, but only because I don't know how to do better (as in the other comment).

I could use an appservice user instead. They're easy enough to register. (Though there is talk of not excluding AS users in the future..._

Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving it at a support user seems sensible.

You can try

self.helper.invite(room, alice, support)
self.helper.join(room, support)

(these helpers sometimes work without access tokens. I'm not sure what the conditions are for that (seems to be controlled by a flag self.hijack_auth ...), but if the above works, happy days. Otherwise, I guess we leave it as-is until we add user types support to self.register...)

Copy link
Copy Markdown
Contributor Author

@DMRobertson DMRobertson Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do this in a few places when trying to test room joins etc from support and deactivated users. But that's only because I didn't know how to do it better.

Copy link
Copy Markdown
Contributor Author

@DMRobertson DMRobertson Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try

self.helper.invite(room, alice, support)
self.helper.join(room, support)

(these helpers sometimes work without access tokens. I'm not sure what the conditions are for that (seems to be controlled by a flag self.hijack_auth ...), but if the above works, happy days. Otherwise, I guess we leave it as-is until we add user types support to self.register...)

No bueno I'm afraid:

  File "/home/dmr/workspace/synapse/tests/handlers/test_user_directory.py", line 720, in test_joining_private_room_with_excluded_user
    self.helper.join(room, support)
  File "/home/dmr/workspace/synapse/tests/rest/client/utils.py", line 114, in join
    self.change_membership(
  File "/home/dmr/workspace/synapse/tests/rest/client/utils.py", line 175, in change_membership
    assert (
AssertionError: Expected: 200, got: 401, resp: b'{"errcode":"M_MISSING_TOKEN","error":"Missing access token"}'

I think that hijack mechanism only kicks in for one nominated user on the test case class.

So I think it might be best to (regrettably) leave this as it is for now.

Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough! Thanks for entertaining it anyway :)

# Check the DB state. The support user should not be in the directory.
users, in_public, in_private = self.get_success(
self.user_dir_helper.get_tables()
)
self.assertEqual(users, {alice, bob})
self.assertEqual(in_public, set())
self.assertEqual(in_private, set())

# Then invite Bob, who accepts.
self.helper.invite(room, alice, bob, tok=alice_token)
self.helper.join(room, bob, tok=bob_token)

# Check the DB state. The support user should not be in the directory.
users, in_public, in_private = self.get_success(
self.user_dir_helper.get_tables()
)
self.assertEqual(users, {alice, bob})
self.assertEqual(in_public, set())
self.assertEqual(in_private, {(alice, bob, room), (bob, alice, room)})

def test_spam_checker(self) -> None:
"""
A user which fails the spam checks will not appear in search results.
Expand Down