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

Preparatory work to fix the user directory assuming that any remote membership state events represent a profile change. [rei:userdirpriv] #14755

Merged
merged 13 commits into from
Mar 16, 2023
Merged
Changes from 1 commit
Commits
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
39 changes: 17 additions & 22 deletions synapse/handlers/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -357,28 +357,11 @@ async def _handle_room_membership_event(
# so, ensure we have a directory entry for them. (For local users,
# the rest of the application calls `handle_local_profile_change`.)
if is_remote:
await self._upsert_directory_entry_for_remote_user(state_key, event_id)
await self._handle_possible_remote_profile_change(
state_key, room_id, None, event_id
)
await self._track_user_joined_room(room_id, state_key)

async def _upsert_directory_entry_for_remote_user(
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

logger.debug("Adding new user to dir, %r", user_id)

await self.store.update_profile_in_user_dir(
user_id, event.content.get("displayname"), event.content.get("avatar_url")
)

async def _track_user_joined_room(self, room_id: str, joining_user_id: str) -> None:
"""Someone's just joined a room. Update `users_in_public_rooms` or
`users_who_share_private_rooms` as appropriate.
Expand Down Expand Up @@ -466,9 +449,15 @@ async def _handle_possible_remote_profile_change(
database if there are. This is intended for remote users only. The caller
is responsible for checking that the given user is remote.
"""
if not prev_event_id or not event_id:

if not event_id:
return
reivilibre marked this conversation as resolved.
Show resolved Hide resolved

if not prev_event_id:
# If we don't have an older event to fall back on, just fetch the same
# event itself.
prev_event_id = event_id

prev_event = await self.store.get_event(prev_event_id, allow_none=True)
event = await self.store.get_event(event_id, allow_none=True)

Expand All @@ -490,5 +479,11 @@ async def _handle_possible_remote_profile_change(
if not isinstance(new_avatar, str):
new_avatar = prev_avatar

if prev_name != new_name or prev_avatar != new_avatar:
if (
prev_name != new_name
or prev_avatar != new_avatar
or prev_event_id == event_id
):
# Only update if something has changed, or we didn't have a previous event
# in the first place.
await self.store.update_profile_in_user_dir(user_id, new_name, new_avatar)
Comment on lines +503 to 507
Copy link
Contributor

Choose a reason for hiding this comment

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

When there is no prev event, we can now try to update the user directory with non-strings, despite the type checks above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm this was already flawed by the looks: you could just send a dodgy event and then follow it up with another dodgy event to get the prev_event accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But update_profile_in_user_dir sets it to None if you pass in a non-string, so I suggest we just replace non-strings with None anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

That approach sounds reasonable!