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

Fix get_users_in_room mis-use in transfer_room_state_on_room_upgrade #13960

Merged
merged 2 commits into from
Oct 1, 2022
Merged

Fix get_users_in_room mis-use in transfer_room_state_on_room_upgrade #13960

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b7f1df
Fix `get_users_in_room` mis-use in `transfer_room_state_on_room_upgrade`
MadLittleMods Sep 29, 2022
b28e165
Add changelog
MadLittleMods Sep 29, 2022
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
4 changes: 2 additions & 2 deletions synapse/handlers/room_member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,8 +1150,8 @@ async def transfer_room_state_on_room_upgrade(
logger.info("Transferring room state from %s to %s", old_room_id, room_id)

# Find all local users that were in the old room and copy over each user's state
Copy link
Contributor Author

@MadLittleMods MadLittleMods Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We now do what this comment says, but is this comment actually correct?

I don't see anything previously where we checked for is_mine_id(user_id) in the downstream functions here. It feels like we happily copied everyone over.

It makes sense to only copy over the push rules for the local users (transfer_room_state_on_room_upgrade -> copy_user_state_on_room_upgrade -> copy_push_rules_from_room_to_room_for_user)

I'm not sure about transfer_room_state_on_room_upgrade -> copy_user_state_on_room_upgrade -> copy_room_tags_and_direct_to_room though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fairly sure account data (and hence m.direct data) and room tags can only be set for local users.
ie. we used to copy over nothing for remote users, so this change looks alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woot! Good news to move this forward 🚆

ie. we used to copy over nothing for remote users, so this change looks alright.

I trust you but can you link where you see this constraint? It's not immediately obvious to me trying to follow some functions down.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no check in the copying method or its callees. It's just not possible for us to insert account data and room tags for remote users into the database.

users = await self.store.get_users_in_room(old_room_id)
await self.copy_user_state_on_room_upgrade(old_room_id, room_id, users)
local_users = await self.store.get_local_users_in_room(old_room_id)
await self.copy_user_state_on_room_upgrade(old_room_id, room_id, local_users)

# Add new room to the room directory if the old room was there
# Remove old room from the room directory
Expand Down