diff --git a/changelog.d/19830.feature b/changelog.d/19830.feature new file mode 100644 index 00000000000..3fb7545beda --- /dev/null +++ b/changelog.d/19830.feature @@ -0,0 +1 @@ +[MSC4140: Cancellable delayed events](https://github.com/matrix-org/matrix-spec-proposals/pull/4140): Rate limit the delayed event management endpoints based on the `delay_id`. diff --git a/synapse/handlers/delayed_events.py b/synapse/handlers/delayed_events.py index 6afb7c0f3c4..dcaa6c40392 100644 --- a/synapse/handlers/delayed_events.py +++ b/synapse/handlers/delayed_events.py @@ -413,7 +413,7 @@ async def cancel(self, request: SynapseRequest, delay_id: str) -> None: NotFoundError: if no matching delayed event could be found. """ assert self._is_master - await self._mgmt_ratelimit(request) + await self._mgmt_ratelimit(request, delay_id) await make_deferred_yieldable(self._initialized_from_db) next_send_ts = await self._store.cancel_delayed_event(delay_id) @@ -428,7 +428,7 @@ async def restart(self, request: SynapseRequest, delay_id: str) -> None: Raises: NotFoundError: if no matching delayed event could be found. """ - await self._mgmt_ratelimit(request) + await self._mgmt_ratelimit(request, delay_id) # Note: We don't need to wait on `self._initialized_from_db` here as the # events that deals with are already marked as processed. @@ -452,7 +452,7 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None: NotFoundError: if no matching delayed event could be found. """ assert self._is_master - await self._mgmt_ratelimit(request) + await self._mgmt_ratelimit(request, delay_id) await make_deferred_yieldable(self._initialized_from_db) event, next_send_ts = await self._store.process_target_delayed_event(delay_id) @@ -462,18 +462,27 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None: await self._send_event(event) - async def _mgmt_ratelimit(self, request: SynapseRequest) -> None: + async def _mgmt_ratelimit(self, request: SynapseRequest, delay_id: str) -> None: """ Ratelimit requests with the `_delayed_event_mgmt_ratelimiter` keyed on the + ID of the delayed event to be managed, and either the user making the request, or the request's IP address if unauthed. """ + # NOTE: it would be more accurate to run this in a transaction that also contains + # the action to be done with the delay_id, but the worst that can happen without it + # is the possibility of the delay_id disappearing after having checked for it here, + # which isn't terrible since it's used here only for applying a ratelimit. + await self._store.assert_delayed_event(delay_id) + if self._auth.has_access_token(request): requester = await self._auth.get_user_by_req(request) - key = None + requester_key = requester.user.to_string() else: requester = None - key = request.getClientAddress().host - await self._delayed_event_mgmt_ratelimiter.ratelimit(requester, key) + requester_key = request.getClientAddress().host + await self._delayed_event_mgmt_ratelimiter.ratelimit( + requester, (requester_key, delay_id) + ) async def _send_on_timeout(self) -> None: self._next_delayed_event_call = None diff --git a/synapse/storage/databases/main/delayed_events.py b/synapse/storage/databases/main/delayed_events.py index 1727f589e2a..aac4ac15722 100644 --- a/synapse/storage/databases/main/delayed_events.py +++ b/synapse/storage/databases/main/delayed_events.py @@ -210,6 +210,23 @@ def restart_delayed_event_txn( "restart_delayed_event", restart_delayed_event_txn ) + async def assert_delayed_event(self, delay_id: str) -> None: + """Checks whether there is a delayed event in the DB with the given delay_id. + + Raises: + NotFoundError: if there is no matching delayed event. + """ + + res = await self.db_pool.simple_select_one_onecol( + table="delayed_events", + keyvalues={"delay_id": delay_id}, + retcol="1", + allow_none=True, + desc="has_delayed_event", + ) + if not res: + raise NotFoundError("Delayed event not found") + async def get_count_of_delayed_events(self) -> int: """Returns the number of pending delayed events in the DB.""" diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index c3bfdf7c8d4..7fda657291e 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/tests/rest/client/test_delayed_events.py @@ -317,7 +317,7 @@ def test_cancel_delayed_state_event(self, action_in_path: bool) -> None: ) def test_cancel_delayed_event_ratelimit(self, action_in_path: bool) -> None: delay_ids = [] - for _ in range(3): + for _ in range(5): channel = self.make_request( "POST", _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 100000), @@ -329,11 +329,14 @@ def test_cancel_delayed_event_ratelimit(self, action_in_path: bool) -> None: assert delay_id is not None delay_ids.append(delay_id) - delay_id = delay_ids.pop(0) - channel = self._update_delayed_event(delay_id, "cancel", action_in_path) - self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + for _ in range(2): + delay_id = delay_ids.pop(0) + channel = self._update_delayed_event(delay_id, "cancel", action_in_path) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) delay_id = delay_ids.pop(0) + channel = self._update_delayed_event(delay_id, "restart", action_in_path) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) channel = self._update_delayed_event(delay_id, "cancel", action_in_path) self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result) @@ -347,6 +350,16 @@ def test_cancel_delayed_event_ratelimit(self, action_in_path: bool) -> None: channel = self._update_delayed_event( delay_id, "cancel", action_in_path, self.user1_access_token ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + + delay_id = delay_ids.pop(0) + channel = self._update_delayed_event( + delay_id, "restart", action_in_path, self.user1_access_token + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + channel = self._update_delayed_event( + delay_id, "cancel", action_in_path, self.user1_access_token + ) self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result) # Add the current user to the ratelimit overrides, allowing them no ratelimiting. @@ -502,7 +515,7 @@ def test_restart_delayed_state_event(self, action_in_path: bool) -> None: ) def test_restart_delayed_event_ratelimit(self, action_in_path: bool) -> None: delay_ids = [] - for _ in range(3): + for _ in range(2): channel = self.make_request( "POST", _get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 100000), @@ -510,15 +523,15 @@ def test_restart_delayed_event_ratelimit(self, action_in_path: bool) -> None: self.user1_access_token, ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - delay_id = channel.json_body.get("delay_id") - assert delay_id is not None + assert "delay_id" in channel.json_body + delay_id = channel.json_body["delay_id"] delay_ids.append(delay_id) - delay_id = delay_ids.pop(0) - channel = self._update_delayed_event(delay_id, "restart", action_in_path) - self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + for _ in range(2): + delay_id = delay_ids.pop(0) + channel = self._update_delayed_event(delay_id, "restart", action_in_path) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - delay_id = delay_ids.pop(0) channel = self._update_delayed_event(delay_id, "restart", action_in_path) self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result) @@ -528,7 +541,6 @@ def test_restart_delayed_event_ratelimit(self, action_in_path: bool) -> None: ) self.assertEqual(HTTPStatus.OK, channel.code, channel.result) - delay_id = delay_ids.pop(0) channel = self._update_delayed_event( delay_id, "restart", action_in_path, self.user1_access_token )