From 6a83eb44236c789320eb91e03973396d0fb696c4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 26 Oct 2023 16:43:02 +0100 Subject: [PATCH 1/8] Fix cross-worker ratelimiting c.f. #16481 --- synapse/handlers/message.py | 57 ++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 41a35ce510ff..8baa69202337 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -999,7 +999,14 @@ async def create_and_send_nonmember_event( raise ShadowBanError() if ratelimit: - await self.request_ratelimiter.ratelimit(requester, update=False) + is_admin_redaction = await self.is_admin_redaction( + event_type=event_dict["type"], + sender=event_dict["sender"], + redacts=event_dict.get("redacts"), + ) + await self.request_ratelimiter.ratelimit( + requester, is_admin_redaction=is_admin_redaction, update=False + ) # We limit the number of concurrent event sends in a room so that we # don't fork the DAG too much. If we don't limit then we can end up in @@ -1538,6 +1545,15 @@ async def _persist_events( # stream_ordering entry manually (as it was persisted on # another worker). event.internal_metadata.stream_ordering = stream_id + + if ratelimit: + is_admin_redaction = await self.is_admin_redaction( + event.type, event.sender, event.redacts + ) + await self.request_ratelimiter.ratelimit( + requester, is_admin_redaction=is_admin_redaction + ) + return event event = await self.persist_and_notify_client_events( @@ -1696,21 +1712,9 @@ async def persist_and_notify_client_events( # can apply different ratelimiting. We do this by simply checking # it's not a self-redaction (to avoid having to look up whether the # user is actually admin or not). - is_admin_redaction = False - if event.type == EventTypes.Redaction: - assert event.redacts is not None - - original_event = await self.store.get_event( - event.redacts, - redact_behaviour=EventRedactBehaviour.as_is, - get_prev_content=False, - allow_rejected=False, - allow_none=True, - ) - - is_admin_redaction = bool( - original_event and event.sender != original_event.sender - ) + is_admin_redaction = await self.is_admin_redaction( + event.type, event.sender, event.redacts + ) await self.request_ratelimiter.ratelimit( requester, is_admin_redaction=is_admin_redaction @@ -1930,6 +1934,27 @@ async def _notify() -> None: return persisted_events[-1] + async def is_admin_redaction( + self, event_type: str, sender: str, redacts: Optional[str] + ) -> bool: + is_admin_redaction = False + if event_type == EventTypes.Redaction: + assert redacts is not None + + original_event = await self.store.get_event( + redacts, + redact_behaviour=EventRedactBehaviour.as_is, + get_prev_content=False, + allow_rejected=False, + allow_none=True, + ) + + is_admin_redaction = bool( + original_event and sender != original_event.sender + ) + + return is_admin_redaction + async def _maybe_kick_guest_users( self, event: EventBase, context: EventContext ) -> None: From 36e76d425dd1cec338bb4e8510bc4fae27acd624 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 26 Oct 2023 16:45:09 +0100 Subject: [PATCH 2/8] Newsfile --- changelog.d/16558.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16558.bugfix diff --git a/changelog.d/16558.bugfix b/changelog.d/16558.bugfix new file mode 100644 index 000000000000..64f419fd8263 --- /dev/null +++ b/changelog.d/16558.bugfix @@ -0,0 +1 @@ +Fix ratelimiting of message sending when using workers, where the ratelimit would only be applied after most of the work has been done. From aa2a5b6bde366d64c7add26d3ef8b1bb48bf5798 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 26 Oct 2023 16:59:19 +0100 Subject: [PATCH 3/8] Update synapse/handlers/message.py Co-authored-by: Patrick Cloke --- synapse/handlers/message.py | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 8baa69202337..6d0a28bfe981 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1937,23 +1937,22 @@ async def _notify() -> None: async def is_admin_redaction( self, event_type: str, sender: str, redacts: Optional[str] ) -> bool: - is_admin_redaction = False - if event_type == EventTypes.Redaction: - assert redacts is not None - - original_event = await self.store.get_event( - redacts, - redact_behaviour=EventRedactBehaviour.as_is, - get_prev_content=False, - allow_rejected=False, - allow_none=True, - ) + if event_type != EventTypes.Redaction: + return False - is_admin_redaction = bool( - original_event and sender != original_event.sender - ) + assert redacts is not None - return is_admin_redaction + original_event = await self.store.get_event( + redacts, + redact_behaviour=EventRedactBehaviour.as_is, + get_prev_content=False, + allow_rejected=False, + allow_none=True, + ) + + return bool( + original_event and sender != original_event.sender + ) async def _maybe_kick_guest_users( self, event: EventBase, context: EventContext From e2a24dacaba3e3ee1c2a386a32cecb70e89e7dfc Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 26 Oct 2023 17:00:21 +0100 Subject: [PATCH 4/8] Docstring --- synapse/handlers/message.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 6d0a28bfe981..ddca5ad2f135 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1937,6 +1937,9 @@ async def _notify() -> None: async def is_admin_redaction( self, event_type: str, sender: str, redacts: Optional[str] ) -> bool: + """Return whether the event is a redaction made by an admin, and thus + should use a different ratelimiter. + """ if event_type != EventTypes.Redaction: return False @@ -1950,9 +1953,7 @@ async def is_admin_redaction( allow_none=True, ) - return bool( - original_event and sender != original_event.sender - ) + return bool(original_event and sender != original_event.sender) async def _maybe_kick_guest_users( self, event: EventBase, context: EventContext From af1c8da327fc97f67929bd1b668f2655bcbfcf6a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 26 Oct 2023 17:01:59 +0100 Subject: [PATCH 5/8] Move ratelimiting --- synapse/handlers/message.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index ddca5ad2f135..cdefcb568a42 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1515,6 +1515,18 @@ async def _persist_events( first_event.room_id ) if writer_instance != self._instance_name: + # Ratelimit before sending to the other event persister, to + # ensure that we correctly have ratelimits on both the event + # creators and event persiters. + if ratelimit: + for event, _ in events_and_context: + is_admin_redaction = await self.is_admin_redaction( + event.type, event.sender, event.redacts + ) + await self.request_ratelimiter.ratelimit( + requester, is_admin_redaction=is_admin_redaction + ) + try: result = await self.send_events( instance_name=writer_instance, @@ -1546,14 +1558,6 @@ async def _persist_events( # another worker). event.internal_metadata.stream_ordering = stream_id - if ratelimit: - is_admin_redaction = await self.is_admin_redaction( - event.type, event.sender, event.redacts - ) - await self.request_ratelimiter.ratelimit( - requester, is_admin_redaction=is_admin_redaction - ) - return event event = await self.persist_and_notify_client_events( From c801428e09acd2f6c29b84f4f5cfa9d397655d2e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 26 Oct 2023 17:24:48 +0100 Subject: [PATCH 6/8] Fix up --- synapse/handlers/message.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index cdefcb568a42..8c5e49918f96 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -999,10 +999,16 @@ async def create_and_send_nonmember_event( raise ShadowBanError() if ratelimit: + room_version = await self.store.get_room_version(event_dict["room_id"]) + if room_version.updated_redaction_rules: + redacts = event_dict["content"].get("redacts") + else: + redacts = event_dict.get("redacts") + is_admin_redaction = await self.is_admin_redaction( event_type=event_dict["type"], sender=event_dict["sender"], - redacts=event_dict.get("redacts"), + redacts=redacts, ) await self.request_ratelimiter.ratelimit( requester, is_admin_redaction=is_admin_redaction, update=False From eb5a0f6ca600ecf83f139e562ed5b8760524be83 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 26 Oct 2023 17:43:18 +0100 Subject: [PATCH 7/8] Handle non-existent room --- synapse/handlers/message.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 8c5e49918f96..b0d51a001409 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -999,7 +999,13 @@ async def create_and_send_nonmember_event( raise ShadowBanError() if ratelimit: - room_version = await self.store.get_room_version(event_dict["room_id"]) + room_id = event_dict["room_id"] + try: + room_version = await self.store.get_room_version(room_id) + except NotFoundError: + # If the room doesnt' exist. + raise AuthError(403, f"User {requester.user} not in room {room_id}") + if room_version.updated_redaction_rules: redacts = event_dict["content"].get("redacts") else: From a2981a0bdc2ac35a991c64f189041c878576cf34 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 27 Oct 2023 09:31:36 +0100 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: Patrick Cloke --- synapse/handlers/message.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index b0d51a001409..4d649b667a82 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1003,7 +1003,7 @@ async def create_and_send_nonmember_event( try: room_version = await self.store.get_room_version(room_id) except NotFoundError: - # If the room doesnt' exist. + # The room doesn't exist. raise AuthError(403, f"User {requester.user} not in room {room_id}") if room_version.updated_redaction_rules: @@ -1529,7 +1529,7 @@ async def _persist_events( if writer_instance != self._instance_name: # Ratelimit before sending to the other event persister, to # ensure that we correctly have ratelimits on both the event - # creators and event persiters. + # creators and event persisters. if ratelimit: for event, _ in events_and_context: is_admin_redaction = await self.is_admin_redaction(