Skip to content
Merged
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/18514.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add configurable rate limiting for the creation of rooms.
4 changes: 4 additions & 0 deletions docker/complement/conf/workers-shared-extra.yaml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ rc_delayed_event_mgmt:
per_second: 9999
burst_count: 9999

rc_room_creation:
per_second: 9999
burst_count: 9999

federation_rr_transactions_per_room_per_second: 9999

allow_device_name_lookup_over_federation: true
Expand Down
25 changes: 25 additions & 0 deletions docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -1996,6 +1996,31 @@ rc_reports:
burst_count: 20.0
```
---
### `rc_room_creation`

*(object)* Sets rate limits for how often users are able to create rooms.

This setting has the following sub-options:

* `per_second` (number): Maximum number of requests a client can send per second.

* `burst_count` (number): Maximum number of requests a client can send before being throttled.

Default configuration:
```yaml
rc_room_creation:
per_user:
per_second: 0.016
burst_count: 10.0
```

Example configuration:
```yaml
rc_room_creation:
per_second: 1.0
burst_count: 5.0
```
---
### `federation_rr_transactions_per_room_per_second`

*(integer)* Sets outgoing federation transaction frequency for sending read-receipts, per-room.
Expand Down
11 changes: 11 additions & 0 deletions schema/synapse-config.schema.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2228,6 +2228,17 @@ properties:
examples:
- per_second: 2.0
burst_count: 20.0
rc_room_creation:
$ref: "#/$defs/rc"
description: >-
Sets rate limits for how often users are able to create rooms.
default:
per_user:
per_second: 0.016
burst_count: 10.0
examples:
- per_second: 1.0
burst_count: 5.0
federation_rr_transactions_per_room_per_second:
type: integer
description: >-
Expand Down
6 changes: 6 additions & 0 deletions synapse/config/ratelimiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,12 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
defaults={"per_second": 1, "burst_count": 5},
)

self.rc_room_creation = RatelimitSettings.parse(
config,
"rc_room_creation",
defaults={"per_second": 0.016, "burst_count": 10},
Copy link
Member

Choose a reason for hiding this comment

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

interesting per_second value... almost 1 per minute?
I don't have a problem with it, but it might warrant a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm yeah, not sure what would be a good default

)

self.rc_reports = RatelimitSettings.parse(
config,
"rc_reports",
Expand Down
36 changes: 29 additions & 7 deletions synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
SynapseError,
)
from synapse.api.filtering import Filter
from synapse.api.ratelimiting import Ratelimiter
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
from synapse.event_auth import validate_event_for_room_version
from synapse.events import EventBase
Expand Down Expand Up @@ -131,7 +132,12 @@ def __init__(self, hs: "HomeServer"):
self.room_member_handler = hs.get_room_member_handler()
self._event_auth_handler = hs.get_event_auth_handler()
self.config = hs.config
self.request_ratelimiter = hs.get_request_ratelimiter()
self.common_request_ratelimiter = hs.get_request_ratelimiter()
self.creation_ratelimiter = Ratelimiter(
store=self.store,
clock=self.clock,
cfg=self.config.ratelimiting.rc_room_creation,
)

# Room state based off defined presets
self._presets_dict: Dict[str, Dict[str, Any]] = {
Expand Down Expand Up @@ -203,7 +209,11 @@ async def upgrade_room(
Raises:
ShadowBanError if the requester is shadow-banned.
"""
await self.request_ratelimiter.ratelimit(requester)
await self.creation_ratelimiter.ratelimit(requester, update=False)

# then apply the ratelimits
await self.common_request_ratelimiter.ratelimit(requester)
await self.creation_ratelimiter.ratelimit(requester)

user_id = requester.user.to_string()

Expand Down Expand Up @@ -809,11 +819,23 @@ async def create_room(
)

if ratelimit:
# Rate limit once in advance, but don't rate limit the individual
# events in the room — room creation isn't atomic and it's very
# janky if half the events in the initial state don't make it because
# of rate limiting.
await self.request_ratelimiter.ratelimit(requester)
# Limit the rate of room creations,
# using both the limiter specific to room creations as well
# as the general request ratelimiter.
#
# Note that we don't rate limit the individual
# events in the room — room creation isn't atomic and
# historically it was very janky if half the events in the
# initial state don't make it because of rate limiting.

# First check the room creation ratelimiter without updating it
# (this is so we don't consume a token if the other ratelimiter doesn't
# allow us to proceed)
await self.creation_ratelimiter.ratelimit(requester, update=False)

# then apply the ratelimits
await self.common_request_ratelimiter.ratelimit(requester)
await self.creation_ratelimiter.ratelimit(requester)
Comment on lines +837 to +838
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's an open question if we want them both or not, but since room creation consuming tokens from the general bucket was implicitly relied upon by tests, I thought I should leave it like that for now

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - this is the bit I'm unsure of. Would need to dig into this further to see what we should be doing here, and if this is in fact the right thing to do.


room_version_id = config.get(
"room_version", self.config.server.default_room_version.identifier
Expand Down
5 changes: 5 additions & 0 deletions tests/handlers/test_room_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
from synapse.util import Clock

from tests import unittest
from tests.unittest import override_config


def _create_event(
Expand Down Expand Up @@ -245,6 +246,7 @@ def test_simple_space(self) -> None:
)
self._assert_hierarchy(result, expected)

@override_config({"rc_room_creation": {"burst_count": 1000, "per_second": 1}})
def test_large_space(self) -> None:
"""Test a space with a large number of rooms."""
rooms = [self.room]
Expand Down Expand Up @@ -527,6 +529,7 @@ def test_complex_space(self) -> None:
)
self._assert_hierarchy(result, expected)

@override_config({"rc_room_creation": {"burst_count": 1000, "per_second": 1}})
def test_pagination(self) -> None:
"""Test simple pagination works."""
room_ids = []
Expand Down Expand Up @@ -564,6 +567,7 @@ def test_pagination(self) -> None:
self._assert_hierarchy(result, expected)
self.assertNotIn("next_batch", result)

@override_config({"rc_room_creation": {"burst_count": 1000, "per_second": 1}})
def test_invalid_pagination_token(self) -> None:
"""An invalid pagination token, or changing other parameters, shoudl be rejected."""
room_ids = []
Expand Down Expand Up @@ -615,6 +619,7 @@ def test_invalid_pagination_token(self) -> None:
SynapseError,
)

@override_config({"rc_room_creation": {"burst_count": 1000, "per_second": 1}})
def test_max_depth(self) -> None:
"""Create a deep tree to test the max depth against."""
spaces = [self.space]
Expand Down