-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Signed-off-by: David Teller <davidt@element.io>
821ffa4
to
cb37541
Compare
synapse/handlers/room_member.py
Outdated
await self._invites_per_user_limiter.ratelimit(requester, invitee_user_id) | ||
await self._invites_per_recipient_limiter.ratelimit(requester, invitee_user_id) | ||
if requester is not None: | ||
await self._invites_per_issuer_limiter.ratelimit(requester, requester.user) |
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.
await self._invites_per_issuer_limiter.ratelimit(requester, requester.user) | |
await self._invites_per_issuer_limiter.ratelimit(requester) |
No point having a key in the ratelimiter if it's always the same as the requester, I think.
synapse/config/ratelimiting.py
Outdated
self.rc_invites_per_issuer = RateLimitConfig( | ||
config.get("rc_invites", {}).get("per_issuer", {}), | ||
defaults={"per_second": 0.003, "burst_count": 5}, | ||
) |
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.
These defaults seem very harsh for someone creating a new room or upgrading a room (if that counts here?).
e.g. if I'm making a group chat with 10 friends, then the first 5 will be invited by the burst count, but the next 5 will take me 1666 seconds to invite — that's almost half an hour!!!
I don't know what's best here; maybe the burst count should be quite a bit higher (obviously this is kinder to spammers, but I think the current number definitely will obstruct normal usage of the service. I've invited more than 5 people to a new room before.)
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.
You're right. Moving to same default rate limits as rc_invites_per_room
.
@@ -136,6 +136,11 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: | |||
defaults={"per_second": 0.003, "burst_count": 5}, | |||
) | |||
|
|||
self.rc_invites_per_issuer = RateLimitConfig( |
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.
@Yoric, @reivilibre: please do remember to add new config settings to the documentation, otherwise people get angry.
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.
see #13330
We already have mechanisms for throttling (local) invites by room and by recipient but not by issuer. This PR adds it, along with a few lines of documentation.
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.(run the linters)