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

Add ratelimiting on login #4821

Merged
merged 35 commits into from
Mar 15, 2019
Merged

Add ratelimiting on login #4821

merged 35 commits into from
Mar 15, 2019

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Mar 6, 2019

Add two ratelimiters on login (per-IP address and per-userID).
Fixes #1452

@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #4821 into develop will decrease coverage by 16.39%.
The diff coverage is 95.83%.

@@             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

@babolivier babolivier marked this pull request as ready for review March 8, 2019 17:01
@babolivier babolivier requested a review from a team March 8, 2019 17:01
Copy link
Member

@erikjohnston erikjohnston left a 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/rest/client/v1/login.py Outdated Show resolved Hide resolved
synapse/rest/client/v1/login.py Outdated Show resolved Hide resolved
synapse/config/ratelimiting.py Outdated Show resolved Hide resolved
synapse/rest/client/v1/login.py Outdated Show resolved Hide resolved
tests/rest/client/v1/test_login.py Outdated Show resolved Hide resolved
synapse/rest/client/v1/login.py Outdated Show resolved Hide resolved
synapse/rest/client/v1/login.py Outdated Show resolved Hide resolved
@babolivier babolivier requested a review from a team March 14, 2019 21:00
synapse/config/ratelimiting.py Outdated Show resolved Hide resolved
@@ -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,
Copy link
Member

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?

Copy link
Contributor Author

@babolivier babolivier Mar 15, 2019

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.

synapse/handlers/auth.py Show resolved Hide resolved
synapse/handlers/auth.py Outdated Show resolved Hide resolved
synapse/handlers/auth.py Outdated Show resolved Hide resolved
tests/rest/client/v1/test_login.py Outdated Show resolved Hide resolved
tests/rest/client/v1/test_login.py Show resolved Hide resolved
tests/rest/client/v1/test_login.py Show resolved Hide resolved
synapse/config/ratelimiting.py Outdated Show resolved Hide resolved
synapse/config/ratelimiting.py Outdated Show resolved Hide resolved
@babolivier babolivier requested a review from a team March 15, 2019 14:47
Copy link
Member

@richvdh richvdh left a 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.

@babolivier babolivier merged commit 899e523 into develop Mar 15, 2019
@erikjohnston erikjohnston deleted the babolivier/ratelimit-login branch March 16, 2019 14:24
@babolivier babolivier restored the babolivier/ratelimit-login branch November 5, 2019 11:27
@babolivier babolivier deleted the babolivier/ratelimit-login branch October 28, 2021 15:56
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