tileradotorg/synapse
1
0
Fork 0
forked from MirrorHub/synapse
Code Pull requests Activity

Fix overwriting profile when making room public (#11003)

Browse source 
This splits apart `handle_new_user` into a function which adds an entry to the `user_directory` and a function which updates the room sharing tables. I plan to continue doing more of this kind of refactoring to clarify the implementation.
This commit is contained in:
David Robertson 2021-10-08 12:52:48 +01:00 committed by GitHub
parent eb9ddc8c2e
commit 670a8d9a1e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 104 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
changelog.d/11003.bugfix Normal file
View file

@ -0,0 +1 @@
Fix a long-standing bug where a user's per-room nickname/avatar would overwrite their profile in the user directory when a room was made public.

63
synapse/handlers/user_directory.py
View file

@ -242,18 +242,15 @@ class UserDirectoryHandler(StateDeltasHandler):
continue
if change is MatchChange.now_true: # The user joined
event = await self.store.get_event(event_id, allow_none=True)
# It isn't expected for this event to not exist, but we
# don't want the entire background process to break.
if event is None:
continue
profile = ProfileInfo(
avatar_url=event.content.get("avatar_url"),
display_name=event.content.get("displayname"),
)
await self._handle_new_user(room_id, state_key, profile)
# This may be the first time we've seen a remote user. If
# so, ensure we have a directory entry for them. (We don't
# need to do this for local users: their directory entry
# is created at the point of registration.
if is_remote:
await self._upsert_directory_entry_for_remote_user(
state_key, event_id
)
await self._track_user_joined_room(room_id, state_key)
else: # The user left
await self._handle_remove_user(room_id, state_key)
else:
@ -303,7 +300,7 @@ class UserDirectoryHandler(StateDeltasHandler):
room_id
)
logger.debug("Change: %r, publicness: %r", publicness, is_public)
logger.debug("Publicness change: %r, is_public: %r", publicness, is_public)
if publicness is MatchChange.now_true and not is_public:
# If we became world readable but room isn't currently public then
@ -314,42 +311,50 @@ class UserDirectoryHandler(StateDeltasHandler):
# ignore the change
return
other_users_in_room_with_profiles = (
await self.store.get_users_in_room_with_profiles(room_id)
)
users_in_room = await self.store.get_users_in_room(room_id)
# Remove every user from the sharing tables for that room.
for user_id in other_users_in_room_with_profiles.keys():
for user_id in users_in_room:
await self.store.remove_user_who_share_room(user_id, room_id)
# Then, re-add them to the tables.
# NOTE: this is not the most efficient method, as handle_new_user sets
# NOTE: this is not the most efficient method, as _track_user_joined_room sets
# up local_user -> other_user and other_user_whos_local -> local_user,
# which when ran over an entire room, will result in the same values
# being added multiple times. The batching upserts shouldn't make this
# too bad, though.
for user_id, profile in other_users_in_room_with_profiles.items():
await self._handle_new_user(room_id, user_id, profile)
for user_id in users_in_room:
await self._track_user_joined_room(room_id, user_id)
async def _handle_new_user(
self, room_id: str, user_id: str, profile: ProfileInfo
async def _upsert_directory_entry_for_remote_user(
self, user_id: str, event_id: str
) -> None:
"""Called when we might need to add user to directory
Args:
room_id: The room ID that user joined or started being public
user_id
"""A remote user has just joined a room. Ensure they have an entry in
the user directory. The caller is responsible for making sure they're
remote.
"""
event = await self.store.get_event(event_id, allow_none=True)
# It isn't expected for this event to not exist, but we
# don't want the entire background process to break.
if event is None:
return
logger.debug("Adding new user to dir, %r", user_id)
await self.store.update_profile_in_user_dir(
user_id, profile.display_name, profile.avatar_url
user_id, event.content.get("displayname"), event.content.get("avatar_url")
)
async def _track_user_joined_room(self, room_id: str, user_id: str) -> None:
"""Someone's just joined a room. Update `users_in_public_rooms` or
`users_who_share_private_rooms` as appropriate.
The caller is responsible for ensuring that the given user is not excluded
from the user directory.
"""
is_public = await self.store.is_room_world_readable_or_publicly_joinable(
room_id
)
# Now we update users who share rooms with users.
other_users_in_room = await self.store.get_users_in_room(room_id)
if is_public:

71
tests/handlers/test_user_directory.py
View file

@ -372,8 +372,6 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase):
# Alice makes two rooms. Bob joins one of them.
room1 = self.helper.create_room_as(alice, tok=alice_token)
room2 = self.helper.create_room_as(alice, tok=alice_token)
print("room1=", room1)
print("room2=", room2)
self.helper.join(room1, bob, tok=bob_token)
# The user sharing tables should have been updated.
@ -436,6 +434,75 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase):
0,
)
def test_making_room_public_doesnt_alter_directory_entry(self) -> None:
"""Per-room names shouldn't go to the directory when the room becomes public.
This isn't about preventing a leak (the room is now public, so the nickname
is too). It's about preserving the invariant that we only show a user's public
profile in the user directory results.
I made this a Synapse test case rather than a Complement one because
I think this is (strictly speaking) an implementation choice. Synapse
has chosen to only ever use the public profile when responding to a user
directory search. There's no privacy leak here, because making the room
public discloses the per-room name.
The spec doesn't mandate anything about _how_ a user
should appear in a /user_directory/search result. Hypothetical example:
suppose Bob searches for Alice. When representing Alice in a search
result, it's reasonable to use any of Alice's nicknames that Bob is
aware of. Heck, maybe we even want to use lots of them in a combined
displayname like `Alice (aka "ali", "ally", "41iC3")`.
"""
# TODO the same should apply when Alice is a remote user.
alice = self.register_user("alice", "pass")
alice_token = self.login(alice, "pass")
bob = self.register_user("bob", "pass")
bob_token = self.login(bob, "pass")
# Alice and Bob are in a private room.
room = self.helper.create_room_as(alice, is_public=False, tok=alice_token)
self.helper.invite(room, src=alice, targ=bob, tok=alice_token)
self.helper.join(room, user=bob, tok=bob_token)
# Alice has a nickname unique to that room.
self.helper.send_state(
room,
"m.room.member",
{
"displayname": "Freddy Mercury",
"membership": "join",
},
alice_token,
state_key=alice,
)
# Check Alice isn't recorded as being in a public room.
public = self.get_success(self.user_dir_helper.get_users_in_public_rooms())
self.assertNotIn((alice, room), public)
# One of them makes the room public.
self.helper.send_state(
room,
"m.room.join_rules",
{"join_rule": "public"},
alice_token,
)
# Check that Alice is now recorded as being in a public room
public = self.get_success(self.user_dir_helper.get_users_in_public_rooms())
self.assertIn((alice, room), public)
# Alice's display name remains the same in the user directory.
search_result = self.get_success(self.handler.search_users(bob, alice, 10))
self.assertEqual(
search_result["results"],
[{"display_name": "alice", "avatar_url": None, "user_id": alice}],
0,
)
def test_private_room(self) -> None:
"""
A user can be searched for only by people that are either in a public