-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4821 +/- ##
===========================================
- Coverage 75.18% 58.78% -16.4%
===========================================
Files 340 324 -16
Lines 34984 33849 -1135
Branches 5739 5590 -149
===========================================
- Hits 26303 19899 -6404
- Misses 7062 12571 +5509
+ Partials 1619 1379 -240 |
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.
Looks good, apart from a few niggles. Rich is probably right that we might want to think about how to make the config a little nicer
synapse/handlers/auth.py
Outdated
@@ -569,6 +574,12 @@ def check_user_exists(self, user_id): | |||
defer.Deferred: (unicode) canonical_user_id, or None if zero or | |||
multiple matches | |||
""" | |||
self._account_ratelimiter.ratelimit( | |||
user_id, time_now_s=self._clock.time(), | |||
rate_hz=self.hs.config.rc_login.account.per_second, |
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.
can you pull out these config settings in the constructor please?
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.
Nope, because it makes the tests fail, since the test define special config parameters after initiating the handlers and servlets.
I had them in the constructor initially but couldn't find a sensible way to make the tests pass that way so I reverted that part.
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.
lgtm! Please squash and merge once the CI passes.
Add two ratelimiters on login (per-IP address and per-userID).
Fixes #1452