-
Notifications
You must be signed in to change notification settings - Fork 423
Add configurable rate limiting for the creation of rooms. #18514
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
Changes from all commits
25846ff
861778d
5804372
98ed365
bcb53cb
6f214ea
3d49fd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add configurable rate limiting for the creation of rooms. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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]] = { | ||
|
|
@@ -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) | ||
reivilibre marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| await self.creation_ratelimiter.ratelimit(requester) | ||
|
|
||
| user_id = requester.user.to_string() | ||
|
|
||
|
|
@@ -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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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