From aed1b1e05ac3dc097753de796133b46983485f9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Wed, 4 Dec 2024 20:18:53 +0100 Subject: [PATCH 01/15] Ratelimit set_presence updates --- changelog.d/18000.bugfix | 1 + .../configuration/config_documentation.md | 16 ++++++++++++++++ synapse/config/ratelimiting.py | 6 ++++++ synapse/rest/client/sync.py | 19 +++++++++++++++++-- 4 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 changelog.d/18000.bugfix diff --git a/changelog.d/18000.bugfix b/changelog.d/18000.bugfix new file mode 100644 index 00000000000..74e70455c42 --- /dev/null +++ b/changelog.d/18000.bugfix @@ -0,0 +1 @@ +Add ratelimit `rc_set_presence.per_user` to prevent load from excessive presence updates sent by clients. Contributed by @rda0. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 7a48d76bbb1..d0e5a22d9e5 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1866,6 +1866,22 @@ rc_federation: concurrent: 5 ``` --- +### `rc_set_presence` + +This option sets ratelimiting for presence. + +The `rc_set_presence.per_user` sets ratelimiting how often a specific users' presence +updates are evaluated. Ratelimited presence updates are ignored. +`per_user` defaults to `per_second: 0.1`, `burst_count: 1`. + +Example configuration: +```yaml +rc_set_presence: + per_user: + per_second: 0.1 + burst_count: 1 +``` +--- ### `federation_rr_transactions_per_room_per_second` Sets outgoing federation transaction frequency for sending read-receipts, diff --git a/synapse/config/ratelimiting.py b/synapse/config/ratelimiting.py index 3fa33f5373f..790ec619aae 100644 --- a/synapse/config/ratelimiting.py +++ b/synapse/config/ratelimiting.py @@ -228,3 +228,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: config.get("remote_media_download_burst_count", "500M") ), ) + + self.rc_set_presence_per_user = RatelimitSettings.parse( + config, + "rc_set_presence.per_user", + defaults={"per_second": 0.1, "burst_count": 1}, + ) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index f4ef84a038d..a1dac431c26 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -24,9 +24,10 @@ from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional, Tuple, Union from synapse.api.constants import AccountDataTypes, EduTypes, Membership, PresenceState -from synapse.api.errors import Codes, StoreError, SynapseError +from synapse.api.errors import Codes, LimitExceededError, StoreError, SynapseError from synapse.api.filtering import FilterCollection from synapse.api.presence import UserPresenceState +from synapse.api.ratelimiting import Ratelimiter from synapse.events.utils import ( SerializeEventConfig, format_event_for_client_v2_without_room_id, @@ -126,6 +127,13 @@ def __init__(self, hs: "HomeServer"): cache_name="sync_valid_filter", ) + # Ratelimiter for set_presence updates, keyed by requester. + self._set_presence_per_user_limiter = Ratelimiter( + store=self.store, + clock=self.clock, + cfg=hs.config.ratelimiting.rc_set_presence_per_user, + ) + async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # This will always be set by the time Twisted calls us. assert request.args is not None @@ -239,7 +247,14 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # send any outstanding server notices to the user. await self._server_notices_sender.on_user_syncing(user.to_string()) - affect_presence = set_presence != PresenceState.OFFLINE + # ignore the presence update if the ratelimit is exceeded + try: + await self._set_presence_per_user_limiter.ratelimit(requester) + except LimitExceededError: + affect_presence = False + logger.debug("User set_presence ratelimit exceeded; ignoring it.") + else: + affect_presence = set_presence != PresenceState.OFFLINE context = await self.presence_handler.user_syncing( user.to_string(), From f346e9abf27293300d74025d31c4053db77777c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Tue, 14 Jan 2025 14:59:54 +0100 Subject: [PATCH 02/15] Clarify documentation --- docs/usage/configuration/config_documentation.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index d0e5a22d9e5..593a1c7d47d 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1870,16 +1870,18 @@ rc_federation: This option sets ratelimiting for presence. -The `rc_set_presence.per_user` sets ratelimiting how often a specific users' presence -updates are evaluated. Ratelimited presence updates are ignored. +The `rc_set_presence.per_user` option sets rate limits on how often a specific +users' presence updates are evaluated. Ratelimited presence updates are +ignored, and no error is returned to the client. + `per_user` defaults to `per_second: 0.1`, `burst_count: 1`. Example configuration: ```yaml rc_set_presence: per_user: - per_second: 0.1 - burst_count: 1 + per_second: 0.05 + burst_count: 0.5 ``` --- ### `federation_rr_transactions_per_room_per_second` From 836aea04c9636f99cf22acffc45eb759350d69e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Tue, 14 Jan 2025 15:05:34 +0100 Subject: [PATCH 03/15] Add 2 unit tests 1. Send a presence update, check that it went through, immediately send another one and check that it was ignored. 2. Send a presence update, check that it went through, advancing time a sufficient amount, send another presence update and check that it also worked. --- tests/handlers/test_presence.py | 120 +++++++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 598d6c13cdb..84a8096c4c3 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -45,7 +45,7 @@ handle_update, ) from synapse.rest import admin -from synapse.rest.client import room +from synapse.rest.client import room, login, sync from synapse.server import HomeServer from synapse.storage.database import LoggingDatabaseConnection from synapse.types import JsonDict, UserID, get_domain_from_id @@ -56,7 +56,11 @@ class PresenceUpdateTestCase(unittest.HomeserverTestCase): - servlets = [admin.register_servlets] + servlets = [ + admin.register_servlets, + login.register_servlets, + sync.register_servlets, + ] def prepare( self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer @@ -425,6 +429,118 @@ def test_override(self, initial_state: str, final_state: str) -> None: wheel_timer.insert.assert_not_called() + def test_over_ratelimit_offline_to_online_to_unavailable(self) -> None: + wheel_timer = Mock() + user_id = "@user:pass" + now = 5000000 + sync_url = "/sync?access_token=%s&set_presence=%s" + + # Register the user who syncs presence + user_id = self.register_user("user", "pass") + access_token = self.login("user", "pass") + + # Get the handler (which kicks off a bunch of timers). + presence_handler = self.hs.get_presence_handler() + + # Ensure the user is initially offline. + prev_state = UserPresenceState.default(user_id) + new_state = prev_state.copy_and_replace( + state=PresenceState.OFFLINE, last_active_ts=now + ) + + state, persist_and_notify, federation_ping = handle_update( + prev_state, + new_state, + is_mine=True, + wheel_timer=wheel_timer, + now=now, + persist=False, + ) + + # Check that the user is offline. + state = self.get_success( + presence_handler.get_state(UserID.from_string(user_id)) + ) + self.assertEqual(state.state, PresenceState.OFFLINE) + + self.reactor.advance(10) + # Send sync request with set_presence=online. + channel = self.make_request("GET", sync_url % (access_token, "online")) + self.assertEqual(200, channel.code) + + # Assert the user is now online. + state = self.get_success( + presence_handler.get_state(UserID.from_string(user_id)) + ) + self.assertEqual(state.state, PresenceState.ONLINE) + + # Immediately send another sync request with set_presence=unavailable. + channel = self.make_request("GET", sync_url % (access_token, "unavailable")) + self.assertEqual(200, channel.code) + + # Assert the user is still online and presence update was ignored. + state = self.get_success( + presence_handler.get_state(UserID.from_string(user_id)) + ) + self.assertEqual(state.state, PresenceState.ONLINE) + + def test_within_ratelimit_offline_to_online_to_unavailable(self) -> None: + wheel_timer = Mock() + user_id = "@user:pass" + now = 5000000 + sync_url = "/sync?access_token=%s&set_presence=%s" + + # Register the user who syncs presence + user_id = self.register_user("user", "pass") + access_token = self.login("user", "pass") + + # Get the handler (which kicks off a bunch of timers). + presence_handler = self.hs.get_presence_handler() + + # Ensure the user is initially offline. + prev_state = UserPresenceState.default(user_id) + new_state = prev_state.copy_and_replace( + state=PresenceState.OFFLINE, last_active_ts=now + ) + + state, persist_and_notify, federation_ping = handle_update( + prev_state, + new_state, + is_mine=True, + wheel_timer=wheel_timer, + now=now, + persist=False, + ) + + # Check that the user is offline. + state = self.get_success( + presence_handler.get_state(UserID.from_string(user_id)) + ) + self.assertEqual(state.state, PresenceState.OFFLINE) + + self.reactor.advance(10) + # Send sync request with set_presence=online. + channel = self.make_request("GET", sync_url % (access_token, "online")) + self.assertEqual(200, channel.code) + + # Assert the user is now online. + state = self.get_success( + presence_handler.get_state(UserID.from_string(user_id)) + ) + self.assertEqual(state.state, PresenceState.ONLINE) + + # Advance time a sufficient amount to avoid rate limiting. + self.reactor.advance(30) + # Send another sync request with set_presence=unavailable. + channel = self.make_request("GET", sync_url % (access_token, "unavailable")) + self.assertEqual(200, channel.code) + + # Assert the user is now unavailable. + state = self.get_success( + presence_handler.get_state(UserID.from_string(user_id)) + ) + self.assertEqual(state.state, PresenceState.UNAVAILABLE) + class PresenceTimeoutTestCase(unittest.TestCase): """Tests different timers and that the timer does not change `status_msg` of user.""" From 019c15806c0f103f4dad2386f615e56575b27194 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Wed, 15 Jan 2025 12:30:01 +0100 Subject: [PATCH 04/15] Refactor rate limit unit tests to use private function --- tests/handlers/test_presence.py | 88 +++++++++++---------------------- 1 file changed, 30 insertions(+), 58 deletions(-) diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 84a8096c4c3..81352f1665c 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -45,7 +45,7 @@ handle_update, ) from synapse.rest import admin -from synapse.rest.client import room, login, sync +from synapse.rest.client import login, room, sync from synapse.server import HomeServer from synapse.storage.database import LoggingDatabaseConnection from synapse.types import JsonDict, UserID, get_domain_from_id @@ -430,61 +430,27 @@ def test_override(self, initial_state: str, final_state: str) -> None: wheel_timer.insert.assert_not_called() def test_over_ratelimit_offline_to_online_to_unavailable(self) -> None: - wheel_timer = Mock() - user_id = "@user:pass" - now = 5000000 - sync_url = "/sync?access_token=%s&set_presence=%s" - - # Register the user who syncs presence - user_id = self.register_user("user", "pass") - access_token = self.login("user", "pass") - - # Get the handler (which kicks off a bunch of timers). - presence_handler = self.hs.get_presence_handler() - - # Ensure the user is initially offline. - prev_state = UserPresenceState.default(user_id) - new_state = prev_state.copy_and_replace( - state=PresenceState.OFFLINE, last_active_ts=now - ) - - state, persist_and_notify, federation_ping = handle_update( - prev_state, - new_state, - is_mine=True, - wheel_timer=wheel_timer, - now=now, - persist=False, - ) - - # Check that the user is offline. - state = self.get_success( - presence_handler.get_state(UserID.from_string(user_id)) - ) - self.assertEqual(state.state, PresenceState.OFFLINE) - - self.reactor.advance(10) - # Send sync request with set_presence=online. - channel = self.make_request("GET", sync_url % (access_token, "online")) - self.assertEqual(200, channel.code) - - # Assert the user is now online. - state = self.get_success( - presence_handler.get_state(UserID.from_string(user_id)) - ) - self.assertEqual(state.state, PresenceState.ONLINE) + """ + Send a presence update, check that it went through, immediately send another one and + check that it was ignored. + """ + self._test_ratelimit_offline_to_online_to_unavailable(ratelimited=True) - # Immediately send another sync request with set_presence=unavailable. - channel = self.make_request("GET", sync_url % (access_token, "unavailable")) - self.assertEqual(200, channel.code) + def test_within_ratelimit_offline_to_online_to_unavailable(self) -> None: + """ + Send a presence update, check that it went through, advancing time a sufficient amount, + send another presence update and check that it also worked. + """ + self._test_ratelimit_offline_to_online_to_unavailable(ratelimited=False) - # Assert the user is still online and presence update was ignored. - state = self.get_success( - presence_handler.get_state(UserID.from_string(user_id)) - ) - self.assertEqual(state.state, PresenceState.ONLINE) + def _test_ratelimit_offline_to_online_to_unavailable( + self, ratelimited: bool + ) -> None: + """Test rate limit for presence updates sent with sync requests. - def test_within_ratelimit_offline_to_online_to_unavailable(self) -> None: + Args: + ratelimited: Test rate limited case. + """ wheel_timer = Mock() user_id = "@user:pass" now = 5000000 @@ -518,7 +484,6 @@ def test_within_ratelimit_offline_to_online_to_unavailable(self) -> None: ) self.assertEqual(state.state, PresenceState.OFFLINE) - self.reactor.advance(10) # Send sync request with set_presence=online. channel = self.make_request("GET", sync_url % (access_token, "online")) self.assertEqual(200, channel.code) @@ -529,17 +494,24 @@ def test_within_ratelimit_offline_to_online_to_unavailable(self) -> None: ) self.assertEqual(state.state, PresenceState.ONLINE) - # Advance time a sufficient amount to avoid rate limiting. - self.reactor.advance(30) + if not ratelimited: + # Advance time a sufficient amount to avoid rate limiting. + self.reactor.advance(30) + # Send another sync request with set_presence=unavailable. channel = self.make_request("GET", sync_url % (access_token, "unavailable")) self.assertEqual(200, channel.code) - # Assert the user is now unavailable. state = self.get_success( presence_handler.get_state(UserID.from_string(user_id)) ) - self.assertEqual(state.state, PresenceState.UNAVAILABLE) + + if ratelimited: + # Assert the user is still online and presence update was ignored. + self.assertEqual(state.state, PresenceState.ONLINE) + else: + # Assert the user is now unavailable. + self.assertEqual(state.state, PresenceState.UNAVAILABLE) class PresenceTimeoutTestCase(unittest.TestCase): From 5fbd3a9a8c50ae7eacb65a756162ade9f620cb84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Wed, 15 Jan 2025 15:33:34 +0100 Subject: [PATCH 05/15] Rate limit /_matrix/client/v3/presence endpoint --- changelog.d/18000.bugfix | 2 +- .../configuration/config_documentation.md | 9 ++-- synapse/config/ratelimiting.py | 4 +- synapse/rest/client/presence.py | 22 ++++++++- synapse/rest/client/sync.py | 8 ++-- tests/rest/client/test_presence.py | 45 +++++++++++++++++++ 6 files changed, 78 insertions(+), 12 deletions(-) diff --git a/changelog.d/18000.bugfix b/changelog.d/18000.bugfix index 74e70455c42..a8f1545bf5b 100644 --- a/changelog.d/18000.bugfix +++ b/changelog.d/18000.bugfix @@ -1 +1 @@ -Add ratelimit `rc_set_presence.per_user` to prevent load from excessive presence updates sent by clients. Contributed by @rda0. +Add rate limit `rc_presence.per_user`. This prevents load from excessive presence updates sent by clients via sync api. Also rate limit `/_matrix/client/v3/presence` as per the spec. Contributed by @rda0. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 593a1c7d47d..43cab172714 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1866,19 +1866,20 @@ rc_federation: concurrent: 5 ``` --- -### `rc_set_presence` +### `rc_presence` This option sets ratelimiting for presence. -The `rc_set_presence.per_user` option sets rate limits on how often a specific -users' presence updates are evaluated. Ratelimited presence updates are +The `rc_presence.per_user` option sets rate limits on how often a specific +users' presence updates are evaluated. Ratelimited presence updates sent via sync are ignored, and no error is returned to the client. +This option also sets the rate limits used in the `/_matrix/client/v3/presence` endpoint. `per_user` defaults to `per_second: 0.1`, `burst_count: 1`. Example configuration: ```yaml -rc_set_presence: +rc_presence: per_user: per_second: 0.05 burst_count: 0.5 diff --git a/synapse/config/ratelimiting.py b/synapse/config/ratelimiting.py index 790ec619aae..06af4da3c55 100644 --- a/synapse/config/ratelimiting.py +++ b/synapse/config/ratelimiting.py @@ -229,8 +229,8 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: ), ) - self.rc_set_presence_per_user = RatelimitSettings.parse( + self.rc_presence_per_user = RatelimitSettings.parse( config, - "rc_set_presence.per_user", + "rc_presence.per_user", defaults={"per_second": 0.1, "burst_count": 1}, ) diff --git a/synapse/rest/client/presence.py b/synapse/rest/client/presence.py index ecc52956e48..104d54cd890 100644 --- a/synapse/rest/client/presence.py +++ b/synapse/rest/client/presence.py @@ -24,7 +24,8 @@ import logging from typing import TYPE_CHECKING, Tuple -from synapse.api.errors import AuthError, SynapseError +from synapse.api.errors import AuthError, Codes, LimitExceededError, SynapseError +from synapse.api.ratelimiting import Ratelimiter from synapse.handlers.presence import format_user_presence_state from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request @@ -48,6 +49,14 @@ def __init__(self, hs: "HomeServer"): self.presence_handler = hs.get_presence_handler() self.clock = hs.get_clock() self.auth = hs.get_auth() + self.store = hs.get_datastores().main + + # Ratelimiter for presence updates, keyed by requester. + self._presence_per_user_limiter = Ratelimiter( + store=self.store, + clock=self.clock, + cfg=hs.config.ratelimiting.rc_presence_per_user, + ) async def on_GET( self, request: SynapseRequest, user_id: str @@ -82,6 +91,17 @@ async def on_PUT( if requester.user != user: raise AuthError(403, "Can only set your own presence state") + # ignore the presence update if the ratelimit is exceeded + try: + await self._presence_per_user_limiter.ratelimit(requester) + except LimitExceededError as e: + logger.debug("User presence ratelimit exceeded; ignoring it.") + return 429, { + "errcode": Codes.LIMIT_EXCEEDED, + "error": "Too many requests", + "retry_after_ms": e.retry_after_ms, + } + state = {} content = parse_json_object_from_request(request) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index a1dac431c26..b935830bae6 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -127,11 +127,11 @@ def __init__(self, hs: "HomeServer"): cache_name="sync_valid_filter", ) - # Ratelimiter for set_presence updates, keyed by requester. - self._set_presence_per_user_limiter = Ratelimiter( + # Ratelimiter for presence updates, keyed by requester. + self._presence_per_user_limiter = Ratelimiter( store=self.store, clock=self.clock, - cfg=hs.config.ratelimiting.rc_set_presence_per_user, + cfg=hs.config.ratelimiting.rc_presence_per_user, ) async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: @@ -249,7 +249,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # ignore the presence update if the ratelimit is exceeded try: - await self._set_presence_per_user_limiter.ratelimit(requester) + await self._presence_per_user_limiter.ratelimit(requester) except LimitExceededError: affect_presence = False logger.debug("User set_presence ratelimit exceeded; ignoring it.") diff --git a/tests/rest/client/test_presence.py b/tests/rest/client/test_presence.py index 5ced8319e19..1f2513705c2 100644 --- a/tests/rest/client/test_presence.py +++ b/tests/rest/client/test_presence.py @@ -95,3 +95,48 @@ def test_put_presence_untracked(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK) self.assertEqual(self.presence_handler.set_state.call_count, 0) + + def test_put_presence_over_ratelimit(self) -> None: + """ + Multiple PUTs to the status endpoint without sufficient delay will be rate limited. + """ + self.hs.config.server.presence_enabled = True + + body = {"presence": "here", "status_msg": "beep boop"} + channel = self.make_request( + "PUT", "/presence/%s/status" % (self.user_id,), body + ) + + self.assertEqual(channel.code, HTTPStatus.OK) + + body = {"presence": "here", "status_msg": "beep boop"} + channel = self.make_request( + "PUT", "/presence/%s/status" % (self.user_id,), body + ) + + self.assertEqual(channel.code, HTTPStatus.TOO_MANY_REQUESTS) + self.assertEqual(self.presence_handler.set_state.call_count, 1) + + def test_put_presence_within_ratelimit(self) -> None: + """ + Multiple PUTs to the status endpoint with sufficient delay should all call set_state. + """ + self.hs.config.server.presence_enabled = True + + body = {"presence": "here", "status_msg": "beep boop"} + channel = self.make_request( + "PUT", "/presence/%s/status" % (self.user_id,), body + ) + + self.assertEqual(channel.code, HTTPStatus.OK) + + # Advance time a sufficient amount to avoid rate limiting. + self.reactor.advance(30) + + body = {"presence": "here", "status_msg": "beep boop"} + channel = self.make_request( + "PUT", "/presence/%s/status" % (self.user_id,), body + ) + + self.assertEqual(channel.code, HTTPStatus.OK) + self.assertEqual(self.presence_handler.set_state.call_count, 2) From 7423299a7c49485f20a87b9f13f830ce978a4f1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Wed, 15 Jan 2025 22:22:56 +0100 Subject: [PATCH 06/15] Avoid rate limit in presence endpoint --- tests/events/test_presence_router.py | 3 +++ tests/module_api/test_api.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/tests/events/test_presence_router.py b/tests/events/test_presence_router.py index e48983ddfe7..f834fbe5c3c 100644 --- a/tests/events/test_presence_router.py +++ b/tests/events/test_presence_router.py @@ -266,6 +266,9 @@ def receiving_all_presence_test_body(self) -> None: self.assertEqual(presence_update.state, "online") self.assertEqual(presence_update.status_msg, "boop") + # Advance time a sufficient amount to avoid rate limiting. + self.reactor.advance(30) + # Have all three users send presence send_presence_update( self, diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index b6ba472d7d4..21a348b602f 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -995,6 +995,9 @@ def _test_sending_local_online_presence_to_local_user( # Now we check that we don't receive *offline* updates using ModuleApi.send_local_online_presence_to. + # Advance time a sufficient amount to avoid rate limiting. + test_case.reactor.advance(30) + # Presence sender goes offline send_presence_update( test_case, From 756443817cbc184bfabf7fec9947d17c597f4deb Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2025 18:02:54 +0000 Subject: [PATCH 07/15] Add lax rc_presence config to Complement tests So that we don't get ratelimits while testing the logic of presence in our Complement test suite. --- docker/complement/conf/workers-shared-extra.yaml.j2 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docker/complement/conf/workers-shared-extra.yaml.j2 b/docker/complement/conf/workers-shared-extra.yaml.j2 index c5228af72d0..1bc5c1bc671 100644 --- a/docker/complement/conf/workers-shared-extra.yaml.j2 +++ b/docker/complement/conf/workers-shared-extra.yaml.j2 @@ -86,6 +86,10 @@ rc_invites: per_second: 1000 burst_count: 1000 +rc_presence: + per_second: 9999 + burst_count: 9999 + federation_rr_transactions_per_room_per_second: 9999 allow_device_name_lookup_over_federation: true From 001038392dd1215285f26b6c77574c2b5a48496b Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Mon, 20 Jan 2025 18:05:24 +0000 Subject: [PATCH 08/15] minor wording change to documentation --- docs/usage/configuration/config_documentation.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 43cab172714..9d1319a232e 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1873,7 +1873,9 @@ This option sets ratelimiting for presence. The `rc_presence.per_user` option sets rate limits on how often a specific users' presence updates are evaluated. Ratelimited presence updates sent via sync are ignored, and no error is returned to the client. -This option also sets the rate limits used in the `/_matrix/client/v3/presence` endpoint. +This option also sets the rate limit for the +[`PUT /_matrix/client/v3/presence/{userId}/status`](https://spec.matrix.org/latest/client-server-api/#put_matrixclientv3presenceuseridstatus) +endpoint. `per_user` defaults to `per_second: 0.1`, `burst_count: 1`. From 62b5e967b8b7104338a829fdb7227c6355a3cdaf Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 23 Jan 2025 17:29:41 +0000 Subject: [PATCH 09/15] Correct Complement presence rate-limiting config I forgot a field! --- docker/complement/conf/workers-shared-extra.yaml.j2 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docker/complement/conf/workers-shared-extra.yaml.j2 b/docker/complement/conf/workers-shared-extra.yaml.j2 index 1bc5c1bc671..a3c4a1e8350 100644 --- a/docker/complement/conf/workers-shared-extra.yaml.j2 +++ b/docker/complement/conf/workers-shared-extra.yaml.j2 @@ -87,8 +87,9 @@ rc_invites: burst_count: 1000 rc_presence: - per_second: 9999 - burst_count: 9999 + per_user: + per_second: 9999 + burst_count: 9999 federation_rr_transactions_per_room_per_second: 9999 From 8746108de5481c4ca5163fed8aa51d1b9c88fded Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 23 Jan 2025 17:30:10 +0000 Subject: [PATCH 10/15] Set a high presence rate-limit across all unit tests --- tests/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/utils.py b/tests/utils.py index 9fd26ef348e..3f4a7bb5600 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -200,6 +200,7 @@ def default_config( "per_user": {"per_second": 10000, "burst_count": 10000}, }, "rc_3pid_validation": {"per_second": 10000, "burst_count": 10000}, + "rc_presence": {"per_user": {"per_second": 10000, "burst_count": 10000}}, "saml2_enabled": False, "public_baseurl": None, "default_identity_server": None, From ab10c2c0b140ad30d7edde652fcdc9da2154f417 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 23 Jan 2025 17:34:36 +0000 Subject: [PATCH 11/15] Set `rc_presence` to a reasonable value for presence ratelimiting unit tests --- tests/handlers/test_presence.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 81352f1665c..4cf048f0dfb 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -53,6 +53,7 @@ from tests import unittest from tests.replication._base import BaseMultiWorkerStreamTestCase +from tests.unittest import override_config class PresenceUpdateTestCase(unittest.HomeserverTestCase): @@ -429,6 +430,12 @@ def test_override(self, initial_state: str, final_state: str) -> None: wheel_timer.insert.assert_not_called() + # `rc_presence` is set very high during unit tests to avoid ratelimiting + # subtly impacting unrelated tests. We set the ratelimiting back to a + # reasonable value for the tests specific to presence ratelimiting. + @override_config( + {"rc_presence": {"per_user": {"per_second": 0.1, "burst_count": 1}}} + ) def test_over_ratelimit_offline_to_online_to_unavailable(self) -> None: """ Send a presence update, check that it went through, immediately send another one and @@ -436,6 +443,9 @@ def test_over_ratelimit_offline_to_online_to_unavailable(self) -> None: """ self._test_ratelimit_offline_to_online_to_unavailable(ratelimited=True) + @override_config( + {"rc_presence": {"per_user": {"per_second": 0.1, "burst_count": 1}}} + ) def test_within_ratelimit_offline_to_online_to_unavailable(self) -> None: """ Send a presence update, check that it went through, advancing time a sufficient amount, @@ -443,6 +453,9 @@ def test_within_ratelimit_offline_to_online_to_unavailable(self) -> None: """ self._test_ratelimit_offline_to_online_to_unavailable(ratelimited=False) + @override_config( + {"rc_presence": {"per_user": {"per_second": 0.1, "burst_count": 1}}} + ) def _test_ratelimit_offline_to_online_to_unavailable( self, ratelimited: bool ) -> None: From 521b5536475f0ee1d3f540cd013167951aac1dbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Fri, 24 Jan 2025 14:32:12 +0100 Subject: [PATCH 12/15] Do not pause sync requests when rate limiting set_presence --- synapse/api/ratelimiting.py | 8 +++++--- synapse/rest/client/sync.py | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py index b80630c5d35..229329a5aef 100644 --- a/synapse/api/ratelimiting.py +++ b/synapse/api/ratelimiting.py @@ -275,6 +275,7 @@ async def ratelimit( update: bool = True, n_actions: int = 1, _time_now_s: Optional[float] = None, + pause: Optional[float] = 0.5, ) -> None: """Checks if an action can be performed. If not, raises a LimitExceededError @@ -298,6 +299,8 @@ async def ratelimit( at all. _time_now_s: The current time. Optional, defaults to the current time according to self.clock. Only used by tests. + pause: Time in seconds to pause when an action is being limited. Defaults to 0.5 + to stop clients from "tight-looping" on retrying their request. Raises: LimitExceededError: If an action could not be performed, along with the time in @@ -316,9 +319,8 @@ async def ratelimit( ) if not allowed: - # We pause for a bit here to stop clients from "tight-looping" on - # retrying their request. - await self.clock.sleep(0.5) + if pause: + await self.clock.sleep(pause) raise LimitExceededError( limiter_name=self._limiter_name, diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index b935830bae6..e6742e89091 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -247,9 +247,9 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # send any outstanding server notices to the user. await self._server_notices_sender.on_user_syncing(user.to_string()) - # ignore the presence update if the ratelimit is exceeded + # ignore the presence update if the ratelimit is exceeded but do not pause the request try: - await self._presence_per_user_limiter.ratelimit(requester) + await self._presence_per_user_limiter.ratelimit(requester, pause=0) except LimitExceededError: affect_presence = False logger.debug("User set_presence ratelimit exceeded; ignoring it.") From f10eea8bb6c0f5576a2ea9f43070dac5db9142dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20M=C3=A4der?= Date: Fri, 24 Jan 2025 14:38:29 +0100 Subject: [PATCH 13/15] Revert "Avoid rate limit in presence endpoint" This reverts commit 7423299a7c49485f20a87b9f13f830ce978a4f1a, which is no longer needed as of 8746108de5481c4ca5163fed8aa51d1b9c88fded --- tests/events/test_presence_router.py | 3 --- tests/module_api/test_api.py | 3 --- 2 files changed, 6 deletions(-) diff --git a/tests/events/test_presence_router.py b/tests/events/test_presence_router.py index f834fbe5c3c..e48983ddfe7 100644 --- a/tests/events/test_presence_router.py +++ b/tests/events/test_presence_router.py @@ -266,9 +266,6 @@ def receiving_all_presence_test_body(self) -> None: self.assertEqual(presence_update.state, "online") self.assertEqual(presence_update.status_msg, "boop") - # Advance time a sufficient amount to avoid rate limiting. - self.reactor.advance(30) - # Have all three users send presence send_presence_update( self, diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 21a348b602f..b6ba472d7d4 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -995,9 +995,6 @@ def _test_sending_local_online_presence_to_local_user( # Now we check that we don't receive *offline* updates using ModuleApi.send_local_online_presence_to. - # Advance time a sufficient amount to avoid rate limiting. - test_case.reactor.advance(30) - # Presence sender goes offline send_presence_update( test_case, From 8fe4cc9e504a70a6c17eda56fd3436b6af833ee4 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 24 Jan 2025 15:09:23 +0000 Subject: [PATCH 14/15] Add rc_presence config to other presence ratelimiting unit tests Otherwise they're using the default, very high ratelimit config for the unit tests. --- tests/rest/client/test_presence.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/rest/client/test_presence.py b/tests/rest/client/test_presence.py index 1f2513705c2..6b9c70974a7 100644 --- a/tests/rest/client/test_presence.py +++ b/tests/rest/client/test_presence.py @@ -29,6 +29,7 @@ from synapse.util import Clock from tests import unittest +from tests.unittest import override_config class PresenceTestCase(unittest.HomeserverTestCase): @@ -96,6 +97,9 @@ def test_put_presence_untracked(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK) self.assertEqual(self.presence_handler.set_state.call_count, 0) + @override_config( + {"rc_presence": {"per_user": {"per_second": 0.1, "burst_count": 1}}} + ) def test_put_presence_over_ratelimit(self) -> None: """ Multiple PUTs to the status endpoint without sufficient delay will be rate limited. @@ -117,6 +121,9 @@ def test_put_presence_over_ratelimit(self) -> None: self.assertEqual(channel.code, HTTPStatus.TOO_MANY_REQUESTS) self.assertEqual(self.presence_handler.set_state.call_count, 1) + @override_config( + {"rc_presence": {"per_user": {"per_second": 0.1, "burst_count": 1}}} + ) def test_put_presence_within_ratelimit(self) -> None: """ Multiple PUTs to the status endpoint with sufficient delay should all call set_state. From 4d01d67df5a0e937aa7b046b3887b26454e914a0 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Fri, 24 Jan 2025 15:11:25 +0000 Subject: [PATCH 15/15] int -> float --- synapse/rest/client/sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index e6742e89091..4fb9c0c8e7a 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -249,7 +249,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # ignore the presence update if the ratelimit is exceeded but do not pause the request try: - await self._presence_per_user_limiter.ratelimit(requester, pause=0) + await self._presence_per_user_limiter.ratelimit(requester, pause=0.0) except LimitExceededError: affect_presence = False logger.debug("User set_presence ratelimit exceeded; ignoring it.")