Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Rate limiting invites per issuer #13125

Merged
merged 4 commits into from
Jun 30, 2022
Merged

Rate limiting invites per issuer #13125

merged 4 commits into from
Jun 30, 2022

Conversation

Yoric
Copy link
Contributor

@Yoric Yoric commented Jun 28, 2022

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

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@Yoric Yoric requested a review from a team as a code owner June 28, 2022 12:03
Signed-off-by: David Teller <davidt@element.io>
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Comment on lines 139 to 142
self.rc_invites_per_issuer = RateLimitConfig(
config.get("rc_invites", {}).get("per_issuer", {}),
defaults={"per_second": 0.003, "burst_count": 5},
)
Copy link
Contributor

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

Copy link
Contributor Author

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.

@Yoric Yoric requested a review from reivilibre June 30, 2022 05:46
@reivilibre reivilibre enabled auto-merge (squash) June 30, 2022 09:21
@reivilibre reivilibre merged commit 80c7a06 into develop Jun 30, 2022
@reivilibre reivilibre deleted the yoric/throttling-invites branch June 30, 2022 09:44
@@ -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(
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

see #13330

DMRobertson pushed a commit that referenced this pull request Jul 21, 2022
Resolves #13330.
Missed in #13125.

Signed-off-by: David Teller <davidt@element.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants