-
Notifications
You must be signed in to change notification settings - Fork 429
Add rate limit conf to user directory endpoint #19291
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
base: develop
Are you sure you want to change the base?
Conversation
67eac2f to
151b288
Compare
MadLittleMods
left 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.
Seems reasonable
| @@ -0,0 +1 @@ | |||
| Add a config to be able to rate limit search in the user directory. | |||
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.
What's spawning this change?
| self.rc_user_directory = RatelimitSettings.parse( | ||
| config, | ||
| "rc_user_directory", | ||
| defaults={"per_second": 0.016, "burst_count": 50}, |
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.
Any reasoning behind these values?
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.
The per_second value should be low enough so that an user can't scrape the user directory too quickly, and the burst is big-ish because you may need to get the next page of results several times, and you may make spelling mistakes to correct.
We may want to put the burst even higher actually, since invites are often issued by batches so an user may want to search for several users in a row.
I could bump it to 200 ?
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.
We're going from no rate limit to rate limit so any value is fine to start with (bump it to 200). These are the kind of thoughts I am expecting though 👍
| if not self.hs.config.userdirectory.user_directory_search_enabled: | ||
| return 200, {"limited": False, "results": []} | ||
|
|
||
| await self._per_user_limiter.ratelimit(requester) |
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.
Do we need to wrap this to return nice errors?
I see we do it here for example:
synapse/synapse/rest/client/presence.py
Lines 95 to 103 in f4320b5
| try: | |
| await self._presence_per_user_limiter.ratelimit(requester) | |
| except LimitExceededError as e: | |
| logger.debug("User presence ratelimit exceeded; ignoring it.") | |
| return 429, { | |
| "errcode": Codes.LIMIT_EXCEEDED, | |
| "error": "Too many requests", | |
| "retry_after_ms": e.retry_after_ms, | |
| } |
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.
LimitExceededError is already a SynapseError with 429 so it's probably fine without it. Just calling this out to make sure we've tried this.
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.
So we would gain mainly the log line. I don't think it's that useful since we can also see the 429 in the request log line, and 429 is pretty well defined.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.