Improve order of validation and ratelimiting in room creation#18723
Conversation
| # Run the spam checker after other validation | ||
| if not is_requester_admin: | ||
| spam_check = await self._spam_checker_module_callbacks.user_may_create_room( | ||
| user_id, config | ||
| ) | ||
| if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: | ||
| raise SynapseError( | ||
| 403, | ||
| "You are not permitted to create rooms", | ||
| errcode=spam_check[0], | ||
| additional_fields=spam_check[1], | ||
| ) |
There was a problem hiding this comment.
Moved the spam checker callback after we validate the room config so the spam checker module doesn't have to deal with invalid config and flawed requests.
I still think user_may_create_room is flawed as it doesn't pick up all of the validation, massaging, and defaults we just added above. I think user_may_create_room should be deprecated in favor of a new check that we call in _send_events_for_new_room(...) so that the spam checker module has access to the actual data we're going to create the room with.
(also mentioned in #18721 (comment))
…-rate-limits-in-room-creation Conflicts: synapse/handlers/room.py
| if ratelimit: | ||
| # 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) |
There was a problem hiding this comment.
Moved the rate limiting earlier so we don't waste resources processing the request if we're going to reject it anyway.
MadLittleMods
deleted the
madlittlemods/fix-order-of-validation-and-rate-limits-in-room-creation
branch
|
Thanks for the review @reivilibre 🐊 |
2 participants
Improve order of validation and ratelimiting in room creation
Spawning from looking at this stuff while reviewing #18721
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.