Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ratelimit presence updates #18000

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions changelog.d/18000.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
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.
19 changes: 19 additions & 0 deletions docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,25 @@ rc_federation:
concurrent: 5
```
---
### `rc_presence`

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.

`per_user` defaults to `per_second: 0.1`, `burst_count: 1`.

Example configuration:
```yaml
rc_presence:
per_user:
per_second: 0.05
burst_count: 0.5
```
---
### `federation_rr_transactions_per_room_per_second`

Sets outgoing federation transaction frequency for sending read-receipts,
Expand Down
6 changes: 6 additions & 0 deletions synapse/config/ratelimiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,3 +228,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
config.get("remote_media_download_burst_count", "500M")
),
)

self.rc_presence_per_user = RatelimitSettings.parse(
config,
"rc_presence.per_user",
defaults={"per_second": 0.1, "burst_count": 1},
)
22 changes: 21 additions & 1 deletion synapse/rest/client/presence.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
19 changes: 17 additions & 2 deletions synapse/rest/client/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -126,6 +127,13 @@ def __init__(self, hs: "HomeServer"):
cache_name="sync_valid_filter",
)

# 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) -> Tuple[int, JsonDict]:
# This will always be set by the time Twisted calls us.
assert request.args is not None
Expand Down Expand Up @@ -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._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(),
Expand Down
3 changes: 3 additions & 0 deletions tests/events/test_presence_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
92 changes: 90 additions & 2 deletions tests/handlers/test_presence.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
handle_update,
)
from synapse.rest import admin
from synapse.rest.client import room
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
Expand All @@ -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
Expand Down Expand Up @@ -425,6 +429,90 @@ 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:
Copy link
Member

Choose a reason for hiding this comment

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

As these tests are mostly the same code, could you combine the duplicate into a private function here (which takes a boolean determining whether to wait and expect the user's status to be unavailable), then call this function from each test? That would cut down on the amount of new code introduced.

"""
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)

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)

def _test_ratelimit_offline_to_online_to_unavailable(
self, ratelimited: bool
) -> None:
"""Test rate limit for presence updates sent with sync requests.

Args:
ratelimited: Test rate limited case.
"""
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)

# 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)

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)

state = self.get_success(
presence_handler.get_state(UserID.from_string(user_id))
)

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):
"""Tests different timers and that the timer does not change `status_msg` of user."""
Expand Down
3 changes: 3 additions & 0 deletions tests/module_api/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
45 changes: 45 additions & 0 deletions tests/rest/client/test_presence.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)