From 06a43f3268431197516afa0f4c07316d255ccc6b Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 14:38:08 +0100 Subject: [PATCH 01/13] Test that directory entries aren't altered by making a room public --- tests/handlers/test_user_directory.py | 69 +++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 47217f054202..9f95758d55b0 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -436,6 +436,75 @@ def test_per_room_profile_doesnt_alter_directory_entry(self) -> None: 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 From 0da3f86e75cc32700d89bc5767a2772acc5f11e5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 14:52:30 +0100 Subject: [PATCH 02/13] Split apart `handle_new_user` --- synapse/handlers/user_directory.py | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index b7b19733461e..4ab3a52dc6f3 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -253,7 +253,10 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: display_name=event.content.get("displayname"), ) - await self._handle_new_user(room_id, state_key, profile) + await self._upsert_directory_entry_for_user( + room_id, state_key, profile + ) + await self._track_user_joined_room(room_id, state_key, profile) else: # The user left await self._handle_remove_user(room_id, state_key) else: @@ -329,16 +332,16 @@ async def _handle_room_publicity_change( # 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) + await self._upsert_directory_entry_for_user(room_id, user_id, profile) + await self._track_user_joined_room(room_id, user_id, profile) - async def _handle_new_user( + async def _upsert_directory_entry_for_user( self, room_id: str, user_id: str, profile: ProfileInfo ) -> None: - """Called when we might need to add user to directory + """Someone's just joined a room. Ensure they have an entry in the user directory. - Args: - room_id: The room ID that user joined or started being public - user_id + The caller is responsible for ensuring that the given user is not excluded + from the user directory. """ logger.debug("Adding new user to dir, %r", user_id) @@ -346,10 +349,18 @@ async def _handle_new_user( user_id, profile.display_name, profile.avatar_url ) + async def _track_user_joined_room( + self, room_id: str, user_id: str, profile: ProfileInfo + ) -> 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: From d9820435088fc08ccc21d9217a6abbbe661aac8f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 14:54:25 +0100 Subject: [PATCH 03/13] Changing publicness does not affect profiles --- synapse/handlers/user_directory.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 4ab3a52dc6f3..cd896b66ff59 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -332,7 +332,6 @@ async def _handle_room_publicity_change( # 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._upsert_directory_entry_for_user(room_id, user_id, profile) await self._track_user_joined_room(room_id, user_id, profile) async def _upsert_directory_entry_for_user( From b0bbfd052c2b941cd1e14344f3ed63298fac1c4d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 14:56:20 +0100 Subject: [PATCH 04/13] _track_user_joined_room does not need a profile --- synapse/handlers/user_directory.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index cd896b66ff59..1a8200249c9b 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -256,7 +256,7 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: await self._upsert_directory_entry_for_user( room_id, state_key, profile ) - await self._track_user_joined_room(room_id, state_key, profile) + await self._track_user_joined_room(room_id, state_key) else: # The user left await self._handle_remove_user(room_id, state_key) else: @@ -317,22 +317,20 @@ async def _handle_room_publicity_change( # 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._track_user_joined_room(room_id, user_id, profile) + for user_id in users_in_room: + await self._track_user_joined_room(room_id, user_id) async def _upsert_directory_entry_for_user( self, room_id: str, user_id: str, profile: ProfileInfo @@ -349,7 +347,7 @@ async def _upsert_directory_entry_for_user( ) async def _track_user_joined_room( - self, room_id: str, user_id: str, profile: ProfileInfo + 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. From 066bd115ea4777eb76cf8d27128fa67f033684ed Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 16:50:51 +0100 Subject: [PATCH 05/13] _upsert_directory_entry_for_user doesn't need a room_id --- synapse/handlers/user_directory.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 1a8200249c9b..1313c6e2f9c3 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -253,9 +253,7 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: display_name=event.content.get("displayname"), ) - await self._upsert_directory_entry_for_user( - room_id, state_key, profile - ) + await self._upsert_directory_entry_for_user(state_key, profile) await self._track_user_joined_room(room_id, state_key) else: # The user left await self._handle_remove_user(room_id, state_key) @@ -333,7 +331,7 @@ async def _handle_room_publicity_change( await self._track_user_joined_room(room_id, user_id) async def _upsert_directory_entry_for_user( - self, room_id: str, user_id: str, profile: ProfileInfo + self, user_id: str, profile: ProfileInfo ) -> None: """Someone's just joined a room. Ensure they have an entry in the user directory. @@ -347,7 +345,9 @@ async def _upsert_directory_entry_for_user( ) async def _track_user_joined_room( - self, room_id: str, user_id: str, + 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. From deda1bbf936469186b10b57fc4bbdc8cf80917a4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 16:52:50 +0100 Subject: [PATCH 06/13] We only need to upsert user dir entries for remote users --- synapse/handlers/user_directory.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 1313c6e2f9c3..f53d59223330 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -252,8 +252,10 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: avatar_url=event.content.get("avatar_url"), display_name=event.content.get("displayname"), ) - - await self._upsert_directory_entry_for_user(state_key, profile) + if is_remote: + await self._upsert_directory_entry_for_remote_user( + state_key, profile + ) await self._track_user_joined_room(room_id, state_key) else: # The user left await self._handle_remove_user(room_id, state_key) @@ -330,13 +332,12 @@ async def _handle_room_publicity_change( for user_id in users_in_room: await self._track_user_joined_room(room_id, user_id) - async def _upsert_directory_entry_for_user( + async def _upsert_directory_entry_for_remote_user( self, user_id: str, profile: ProfileInfo ) -> None: - """Someone's just joined a room. Ensure they have an entry in the user directory. - - The caller is responsible for ensuring that the given user is not excluded - from the user directory. + """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. """ logger.debug("Adding new user to dir, %r", user_id) From 68d941a0dc1c58a86d3e45118cd75a7936c4e3c7 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 16:56:42 +0100 Subject: [PATCH 07/13] We only need to fetch a profile from an event in the remote case --- synapse/handlers/user_directory.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index f53d59223330..351707246cea 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -242,19 +242,9 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: 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"), - ) if is_remote: await self._upsert_directory_entry_for_remote_user( - state_key, profile + state_key, event_id ) await self._track_user_joined_room(room_id, state_key) else: # The user left @@ -333,12 +323,23 @@ async def _handle_room_publicity_change( await self._track_user_joined_room(room_id, user_id) async def _upsert_directory_entry_for_remote_user( - self, user_id: str, profile: ProfileInfo + self, user_id: str, event_id: str ) -> None: """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 + + profile = ProfileInfo( + avatar_url=event.content.get("avatar_url"), + display_name=event.content.get("displayname"), + ) + logger.debug("Adding new user to dir, %r", user_id) await self.store.update_profile_in_user_dir( From b8688b029a12bbc50b5f53ac5197f01e9445e47e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 16:57:25 +0100 Subject: [PATCH 08/13] Oops. Remove debug print call. --- tests/handlers/test_user_directory.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 9f95758d55b0..db65253773c6 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -372,8 +372,6 @@ def test_process_join_after_server_leaves_room(self) -> None: # 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. From 3b99ab3b8003d2660652f40ba9f70679598475c9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 17:01:41 +0100 Subject: [PATCH 09/13] Fix debug logging string --- synapse/handlers/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 351707246cea..0c91a89cd264 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -296,7 +296,7 @@ async def _handle_room_publicity_change( 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 From f96feb3e38235f472a60f8c7ce31c20d7cebcacd Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 6 Oct 2021 17:15:01 +0100 Subject: [PATCH 10/13] Changelog --- changelog.d/11003.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11003.bugfix diff --git a/changelog.d/11003.bugfix b/changelog.d/11003.bugfix new file mode 100644 index 000000000000..0786f1b886ac --- /dev/null +++ b/changelog.d/11003.bugfix @@ -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. \ No newline at end of file From 79363a9cd05d921e0e46fdb94956ea1d68d612fa Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 8 Oct 2021 12:21:46 +0100 Subject: [PATCH 11/13] Explanatory comment --- synapse/handlers/user_directory.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 0c91a89cd264..246b1db1c7d0 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -242,6 +242,10 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: continue if change is MatchChange.now_true: # The user joined + # 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 From b527e650e9e9d947163646a3848f6dcecbb4c18c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 8 Oct 2021 12:21:53 +0100 Subject: [PATCH 12/13] Fit def onto one line --- synapse/handlers/user_directory.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 246b1db1c7d0..8136b680b9fa 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -350,11 +350,7 @@ async def _upsert_directory_entry_for_remote_user( user_id, profile.display_name, profile.avatar_url ) - async def _track_user_joined_room( - self, - room_id: str, - user_id: str, - ) -> None: + 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. From 2f01e5495edb0d4d3a9a29d40eff27f5f2840c66 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 8 Oct 2021 12:24:04 +0100 Subject: [PATCH 13/13] Don't make a ProfileInfo just to rip it apart --- synapse/handlers/user_directory.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 8136b680b9fa..8810f048ba4a 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -339,15 +339,10 @@ async def _upsert_directory_entry_for_remote_user( if event is None: return - profile = ProfileInfo( - avatar_url=event.content.get("avatar_url"), - display_name=event.content.get("displayname"), - ) - 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: