From 33b61bc2e252830bc929e011c02b358b90a50c6a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 4 Feb 2021 10:18:25 -0500 Subject: [PATCH] Handle empty rooms when generating email notifications. (#9257) Fixes some exceptions if the room state isn't quite as expected. If the expected state events aren't found, try to find them in the historical room state. If they still aren't found, fallback to a reasonable, although ugly, value. --- changelog.d/9257.bugfix | 1 + synapse/push/mailer.py | 213 ++++++++++++++++++++++++++++++++------- tests/push/test_email.py | 51 +++++++++- 3 files changed, 226 insertions(+), 39 deletions(-) create mode 100644 changelog.d/9257.bugfix diff --git a/changelog.d/9257.bugfix b/changelog.d/9257.bugfix new file mode 100644 index 000000000000..5d0bd88dce8a --- /dev/null +++ b/changelog.d/9257.bugfix @@ -0,0 +1 @@ +Fix long-standing bug where sending email push would fail for rooms that the server had since left. diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index 8a6dcff30d8a..d10201b6b383 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -34,6 +34,7 @@ descriptor_from_member_events, name_from_member_event, ) +from synapse.storage.state import StateFilter from synapse.types import StateMap, UserID from synapse.util.async_helpers import concurrently_execute from synapse.visibility import filter_events_for_client @@ -110,6 +111,7 @@ def __init__( self.sendmail = self.hs.get_sendmail() self.store = self.hs.get_datastore() + self.state_store = self.hs.get_storage().state self.macaroon_gen = self.hs.get_macaroon_generator() self.state_handler = self.hs.get_state_handler() self.storage = hs.get_storage() @@ -217,7 +219,17 @@ async def send_notification_mail( push_actions: Iterable[Dict[str, Any]], reason: Dict[str, Any], ) -> None: - """Send email regarding a user's room notifications""" + """ + Send email regarding a user's room notifications + + Params: + app_id: The application receiving the notification. + user_id: The user receiving the notification. + email_address: The email address receiving the notification. + push_actions: All outstanding notifications. + reason: The notification that was ready and is the cause of an email + being sent. + """ rooms_in_order = deduped_ordered_list([pa["room_id"] for pa in push_actions]) notif_events = await self.store.get_events( @@ -241,7 +253,7 @@ async def send_notification_mail( except StoreError: user_display_name = user_id - async def _fetch_room_state(room_id): + async def _fetch_room_state(room_id: str) -> None: room_state = await self.store.get_current_state_ids(room_id) state_by_room[room_id] = room_state @@ -255,7 +267,7 @@ async def _fetch_room_state(room_id): rooms = [] for r in rooms_in_order: - roomvars = await self.get_room_vars( + roomvars = await self._get_room_vars( r, user_id, notifs_by_room[r], notif_events, state_by_room[r] ) rooms.append(roomvars) @@ -271,7 +283,7 @@ async def _fetch_room_state(room_id): # Only one room has new stuff room_id = list(notifs_by_room.keys())[0] - summary_text = await self.make_summary_text_single_room( + summary_text = await self._make_summary_text_single_room( room_id, notifs_by_room[room_id], state_by_room[room_id], @@ -279,13 +291,13 @@ async def _fetch_room_state(room_id): user_id, ) else: - summary_text = await self.make_summary_text( + summary_text = await self._make_summary_text( notifs_by_room, state_by_room, notif_events, reason ) template_vars = { "user_display_name": user_display_name, - "unsubscribe_link": self.make_unsubscribe_link( + "unsubscribe_link": self._make_unsubscribe_link( user_id, app_id, email_address ), "summary_text": summary_text, @@ -349,7 +361,7 @@ async def send_email( ) ) - async def get_room_vars( + async def _get_room_vars( self, room_id: str, user_id: str, @@ -357,6 +369,20 @@ async def get_room_vars( notif_events: Dict[str, EventBase], room_state_ids: StateMap[str], ) -> Dict[str, Any]: + """ + Generate the variables for notifications on a per-room basis. + + Args: + room_id: The room ID + user_id: The user receiving the notification. + notifs: The outstanding push actions for this room. + notif_events: The events related to the above notifications. + room_state_ids: The event IDs of the current room state. + + Returns: + A dictionary to be added to the template context. + """ + # Check if one of the notifs is an invite event for the user. is_invite = False for n in notifs: @@ -373,12 +399,12 @@ async def get_room_vars( "hash": string_ordinal_total(room_id), # See sender avatar hash "notifs": [], "invite": is_invite, - "link": self.make_room_link(room_id), + "link": self._make_room_link(room_id), } # type: Dict[str, Any] if not is_invite: for n in notifs: - notifvars = await self.get_notif_vars( + notifvars = await self._get_notif_vars( n, user_id, notif_events[n["event_id"]], room_state_ids ) @@ -405,13 +431,26 @@ async def get_room_vars( return room_vars - async def get_notif_vars( + async def _get_notif_vars( self, notif: Dict[str, Any], user_id: str, notif_event: EventBase, room_state_ids: StateMap[str], ) -> Dict[str, Any]: + """ + Generate the variables for a single notification. + + Args: + notif: The outstanding notification for this room. + user_id: The user receiving the notification. + notif_event: The event related to the above notification. + room_state_ids: The event IDs of the current room state. + + Returns: + A dictionary to be added to the template context. + """ + results = await self.store.get_events_around( notif["room_id"], notif["event_id"], @@ -420,7 +459,7 @@ async def get_notif_vars( ) ret = { - "link": self.make_notif_link(notif), + "link": self._make_notif_link(notif), "ts": notif["received_ts"], "messages": [], } @@ -431,22 +470,51 @@ async def get_notif_vars( the_events.append(notif_event) for event in the_events: - messagevars = await self.get_message_vars(notif, event, room_state_ids) + messagevars = await self._get_message_vars(notif, event, room_state_ids) if messagevars is not None: ret["messages"].append(messagevars) return ret - async def get_message_vars( + async def _get_message_vars( self, notif: Dict[str, Any], event: EventBase, room_state_ids: StateMap[str] ) -> Optional[Dict[str, Any]]: + """ + Generate the variables for a single event, if possible. + + Args: + notif: The outstanding notification for this room. + event: The event under consideration. + room_state_ids: The event IDs of the current room state. + + Returns: + A dictionary to be added to the template context, or None if the + event cannot be processed. + """ if event.type != EventTypes.Message and event.type != EventTypes.Encrypted: return None - sender_state_event_id = room_state_ids[("m.room.member", event.sender)] - sender_state_event = await self.store.get_event(sender_state_event_id) - sender_name = name_from_member_event(sender_state_event) - sender_avatar_url = sender_state_event.content.get("avatar_url") + # Get the sender's name and avatar from the room state. + type_state_key = ("m.room.member", event.sender) + sender_state_event_id = room_state_ids.get(type_state_key) + if sender_state_event_id: + sender_state_event = await self.store.get_event( + sender_state_event_id + ) # type: Optional[EventBase] + else: + # Attempt to check the historical state for the room. + historical_state = await self.state_store.get_state_for_event( + event.event_id, StateFilter.from_types((type_state_key,)) + ) + sender_state_event = historical_state.get(type_state_key) + + if sender_state_event: + sender_name = name_from_member_event(sender_state_event) + sender_avatar_url = sender_state_event.content.get("avatar_url") + else: + # No state could be found, fallback to the MXID. + sender_name = event.sender + sender_avatar_url = None # 'hash' for deterministically picking default images: use # sender_hash % the number of default images to choose from @@ -471,18 +539,25 @@ async def get_message_vars( ret["msgtype"] = msgtype if msgtype == "m.text": - self.add_text_message_vars(ret, event) + self._add_text_message_vars(ret, event) elif msgtype == "m.image": - self.add_image_message_vars(ret, event) + self._add_image_message_vars(ret, event) if "body" in event.content: ret["body_text_plain"] = event.content["body"] return ret - def add_text_message_vars( + def _add_text_message_vars( self, messagevars: Dict[str, Any], event: EventBase ) -> None: + """ + Potentially add a sanitised message body to the message variables. + + Args: + messagevars: The template context to be modified. + event: The event under consideration. + """ msgformat = event.content.get("format") messagevars["format"] = msgformat @@ -495,16 +570,20 @@ def add_text_message_vars( elif body: messagevars["body_text_html"] = safe_text(body) - def add_image_message_vars( + def _add_image_message_vars( self, messagevars: Dict[str, Any], event: EventBase ) -> None: """ Potentially add an image URL to the message variables. + + Args: + messagevars: The template context to be modified. + event: The event under consideration. """ if "url" in event.content: messagevars["image_url"] = event.content["url"] - async def make_summary_text_single_room( + async def _make_summary_text_single_room( self, room_id: str, notifs: List[Dict[str, Any]], @@ -517,7 +596,7 @@ async def make_summary_text_single_room( Args: room_id: The ID of the room. - notifs: The notifications for this room. + notifs: The push actions for this room. room_state_ids: The state map for the room. notif_events: A map of event ID -> notification event. user_id: The user receiving the notification. @@ -600,11 +679,11 @@ async def make_summary_text_single_room( "app": self.app_name, } - return await self.make_summary_text_from_member_events( + return await self._make_summary_text_from_member_events( room_id, notifs, room_state_ids, notif_events ) - async def make_summary_text( + async def _make_summary_text( self, notifs_by_room: Dict[str, List[Dict[str, Any]]], room_state_ids: Dict[str, StateMap[str]], @@ -615,7 +694,7 @@ async def make_summary_text( Make a summary text for the email when multiple rooms have notifications. Args: - notifs_by_room: A map of room ID to the notifications for that room. + notifs_by_room: A map of room ID to the push actions for that room. room_state_ids: A map of room ID to the state map for that room. notif_events: A map of event ID -> notification event. reason: The reason this notification is being sent. @@ -632,11 +711,11 @@ async def make_summary_text( } room_id = reason["room_id"] - return await self.make_summary_text_from_member_events( + return await self._make_summary_text_from_member_events( room_id, notifs_by_room[room_id], room_state_ids[room_id], notif_events ) - async def make_summary_text_from_member_events( + async def _make_summary_text_from_member_events( self, room_id: str, notifs: List[Dict[str, Any]], @@ -648,7 +727,7 @@ async def make_summary_text_from_member_events( Args: room_id: The ID of the room. - notifs: The notifications for this room. + notifs: The push actions for this room. room_state_ids: The state map for the room. notif_events: A map of event ID -> notification event. @@ -657,14 +736,45 @@ async def make_summary_text_from_member_events( """ # If the room doesn't have a name, say who the messages # are from explicitly to avoid, "messages in the Bob room" - sender_ids = {notif_events[n["event_id"]].sender for n in notifs} - member_events = await self.store.get_events( - [room_state_ids[("m.room.member", s)] for s in sender_ids] - ) + # Find the latest event ID for each sender, note that the notifications + # are already in descending received_ts. + sender_ids = {} + for n in notifs: + sender = notif_events[n["event_id"]].sender + if sender not in sender_ids: + sender_ids[sender] = n["event_id"] + + # Get the actual member events (in order to calculate a pretty name for + # the room). + member_event_ids = [] + member_events = {} + for sender_id, event_id in sender_ids.items(): + type_state_key = ("m.room.member", sender_id) + sender_state_event_id = room_state_ids.get(type_state_key) + if sender_state_event_id: + member_event_ids.append(sender_state_event_id) + else: + # Attempt to check the historical state for the room. + historical_state = await self.state_store.get_state_for_event( + event_id, StateFilter.from_types((type_state_key,)) + ) + sender_state_event = historical_state.get(type_state_key) + if sender_state_event: + member_events[event_id] = sender_state_event + member_events.update(await self.store.get_events(member_event_ids)) + + if not member_events: + # No member events were found! Maybe the room is empty? + # Fallback to the room ID (note that if there was a room name this + # would already have been used previously). + return self.email_subjects.messages_in_room % { + "room": room_id, + "app": self.app_name, + } # There was a single sender. - if len(sender_ids) == 1: + if len(member_events) == 1: return self.email_subjects.messages_from_person % { "person": descriptor_from_member_events(member_events.values()), "app": self.app_name, @@ -676,7 +786,16 @@ async def make_summary_text_from_member_events( "app": self.app_name, } - def make_room_link(self, room_id: str) -> str: + def _make_room_link(self, room_id: str) -> str: + """ + Generate a link to open a room in the web client. + + Args: + room_id: The room ID to generate a link to. + + Returns: + A link to open a room in the web client. + """ if self.hs.config.email_riot_base_url: base_url = "%s/#/room" % (self.hs.config.email_riot_base_url) elif self.app_name == "Vector": @@ -686,7 +805,16 @@ def make_room_link(self, room_id: str) -> str: base_url = "https://matrix.to/#" return "%s/%s" % (base_url, room_id) - def make_notif_link(self, notif: Dict[str, str]) -> str: + def _make_notif_link(self, notif: Dict[str, str]) -> str: + """ + Generate a link to open an event in the web client. + + Args: + notif: The notification to generate a link for. + + Returns: + A link to open the notification in the web client. + """ if self.hs.config.email_riot_base_url: return "%s/#/room/%s/%s" % ( self.hs.config.email_riot_base_url, @@ -702,9 +830,20 @@ def make_notif_link(self, notif: Dict[str, str]) -> str: else: return "https://matrix.to/#/%s/%s" % (notif["room_id"], notif["event_id"]) - def make_unsubscribe_link( + def _make_unsubscribe_link( self, user_id: str, app_id: str, email_address: str ) -> str: + """ + Generate a link to unsubscribe from email notifications. + + Args: + user_id: The user receiving the notification. + app_id: The application receiving the notification. + email_address: The email address receiving the notification. + + Returns: + A link to unsubscribe from email notifications. + """ params = { "access_token": self.macaroon_gen.generate_delete_pusher_token(user_id), "app_id": app_id, diff --git a/tests/push/test_email.py b/tests/push/test_email.py index c4e1e7ed8575..22f452ec248d 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -124,13 +124,18 @@ def test_simple_sends_email(self): ) self.helper.join(room=room, user=self.others[0].id, tok=self.others[0].token) - # The other user sends some messages + # The other user sends a single message. self.helper.send(room, body="Hi!", tok=self.others[0].token) - self.helper.send(room, body="There!", tok=self.others[0].token) # We should get emailed about that message self._check_for_mail() + # The other user sends multiple messages. + self.helper.send(room, body="Hi!", tok=self.others[0].token) + self.helper.send(room, body="There!", tok=self.others[0].token) + + self._check_for_mail() + def test_invite_sends_email(self): # Create a room and invite the user to it room = self.helper.create_room_as(self.others[0].id, tok=self.others[0].token) @@ -217,6 +222,45 @@ def test_multiple_rooms(self): # We should get emailed about those messages self._check_for_mail() + def test_empty_room(self): + """All users leaving a room shouldn't cause the pusher to break.""" + # Create a simple room with two users + room = self.helper.create_room_as(self.user_id, tok=self.access_token) + self.helper.invite( + room=room, src=self.user_id, tok=self.access_token, targ=self.others[0].id + ) + self.helper.join(room=room, user=self.others[0].id, tok=self.others[0].token) + + # The other user sends a single message. + self.helper.send(room, body="Hi!", tok=self.others[0].token) + + # Leave the room before the message is processed. + self.helper.leave(room, self.user_id, tok=self.access_token) + self.helper.leave(room, self.others[0].id, tok=self.others[0].token) + + # We should get emailed about that message + self._check_for_mail() + + def test_empty_room_multiple_messages(self): + """All users leaving a room shouldn't cause the pusher to break.""" + # Create a simple room with two users + room = self.helper.create_room_as(self.user_id, tok=self.access_token) + self.helper.invite( + room=room, src=self.user_id, tok=self.access_token, targ=self.others[0].id + ) + self.helper.join(room=room, user=self.others[0].id, tok=self.others[0].token) + + # The other user sends a single message. + self.helper.send(room, body="Hi!", tok=self.others[0].token) + self.helper.send(room, body="There!", tok=self.others[0].token) + + # Leave the room before the message is processed. + self.helper.leave(room, self.user_id, tok=self.access_token) + self.helper.leave(room, self.others[0].id, tok=self.others[0].token) + + # We should get emailed about that message + self._check_for_mail() + def test_encrypted_message(self): room = self.helper.create_room_as(self.user_id, tok=self.access_token) self.helper.invite( @@ -269,3 +313,6 @@ def _check_for_mail(self): pushers = list(pushers) self.assertEqual(len(pushers), 1) self.assertTrue(pushers[0].last_stream_ordering > last_stream_ordering) + + # Reset the attempts. + self.email_attempts = []