This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Ratelimit 3PID /requestToken API #9238
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
2775442
Ratelimit 3PID /requestToken API
erikjohnston 6370a3b
Tests
erikjohnston 56d5ad4
Newsfile
erikjohnston 5451935
Fix tests
erikjohnston 99aca3e
Add args doc and remove unused arg
erikjohnston ea2558e
flake8
erikjohnston File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add ratelimited to 3PID `/requestToken` API. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ | |
|
||
import synapse.rest.admin | ||
from synapse.api.constants import LoginType, Membership | ||
from synapse.api.errors import Codes | ||
from synapse.api.errors import Codes, HttpResponseException | ||
from synapse.rest.client.v1 import login, room | ||
from synapse.rest.client.v2_alpha import account, register | ||
from synapse.rest.synapse.client.password_reset import PasswordResetSubmitTokenResource | ||
|
@@ -112,6 +112,56 @@ def test_basic_password_reset(self): | |
# Assert we can't log in with the old password | ||
self.attempt_wrong_password_login("kermit", old_password) | ||
|
||
@override_config({"rc_3pid_validation": {"burst_count": 3}}) | ||
def test_ratelimit_by_email(self): | ||
"""Test that we ratelimit /requestToken for the same email. | ||
""" | ||
old_password = "monkey" | ||
new_password = "kangeroo" | ||
|
||
user_id = self.register_user("kermit", old_password) | ||
self.login("kermit", old_password) | ||
|
||
email = "[email protected]" | ||
|
||
# Add a threepid | ||
self.get_success( | ||
self.store.user_add_threepid( | ||
user_id=user_id, | ||
medium="email", | ||
address=email, | ||
validated_at=0, | ||
added_at=0, | ||
) | ||
) | ||
|
||
def reset(ip): | ||
client_secret = "foobar" | ||
session_id = self._request_token(email, client_secret, ip) | ||
|
||
self.assertEquals(len(self.email_attempts), 1) | ||
link = self._get_link_from_email() | ||
|
||
self._validate_token(link) | ||
|
||
self._reset_password(new_password, session_id, client_secret) | ||
|
||
self.email_attempts.clear() | ||
|
||
# We expect to be able to make three requests before getting rate | ||
# limited. | ||
# | ||
# We change IPs to ensure that we're not being ratelimited due to the | ||
# same IP | ||
reset("127.0.0.1") | ||
reset("127.0.0.2") | ||
reset("127.0.0.3") | ||
|
||
with self.assertRaises(HttpResponseException) as cm: | ||
reset("127.0.0.4") | ||
|
||
self.assertEqual(cm.exception.code, 429) | ||
|
||
def test_basic_password_reset_canonicalise_email(self): | ||
"""Test basic password reset flow | ||
Request password reset with different spelling | ||
|
@@ -239,13 +289,18 @@ def test_password_reset_bad_email_inhibit_error(self): | |
|
||
self.assertIsNotNone(session_id) | ||
|
||
def _request_token(self, email, client_secret): | ||
def _request_token(self, email, client_secret, ip="127.0.0.1"): | ||
channel = self.make_request( | ||
"POST", | ||
b"account/password/email/requestToken", | ||
{"client_secret": client_secret, "email": email, "send_attempt": 1}, | ||
client_ip=ip, | ||
) | ||
self.assertEquals(200, channel.code, channel.result) | ||
|
||
if channel.code != 200: | ||
raise HttpResponseException( | ||
channel.code, channel.result["reason"], channel.result["body"], | ||
) | ||
|
||
return channel.json_body["sid"] | ||
|
||
|
@@ -509,6 +564,21 @@ def test_add_email_address_casefold(self): | |
def test_address_trim(self): | ||
self.get_success(self._add_email(" [email protected] ", "[email protected]")) | ||
|
||
@override_config({"rc_3pid_validation": {"burst_count": 3}}) | ||
def test_ratelimit_by_ip(self): | ||
"""Tests that adding emails is ratelimited by IP | ||
""" | ||
|
||
# We expect to be able to set three emails before getting ratelimited. | ||
self.get_success(self._add_email("[email protected]", "[email protected]")) | ||
self.get_success(self._add_email("[email protected]", "[email protected]")) | ||
self.get_success(self._add_email("[email protected]", "[email protected]")) | ||
|
||
with self.assertRaises(HttpResponseException) as cm: | ||
self.get_success(self._add_email("[email protected]", "[email protected]")) | ||
|
||
self.assertEqual(cm.exception.code, 429) | ||
|
||
def test_add_email_if_disabled(self): | ||
"""Test adding email to profile when doing so is disallowed | ||
""" | ||
|
@@ -777,7 +847,11 @@ def _request_token( | |
body["next_link"] = next_link | ||
|
||
channel = self.make_request("POST", b"account/3pid/email/requestToken", body,) | ||
self.assertEquals(expect_code, channel.code, channel.result) | ||
|
||
if channel.code != expect_code: | ||
raise HttpResponseException( | ||
channel.code, channel.result["reason"], channel.result["body"], | ||
) | ||
|
||
return channel.json_body.get("sid") | ||
|
||
|
@@ -823,10 +897,12 @@ def _get_link_from_email(self): | |
def _add_email(self, request_email, expected_email): | ||
"""Test adding an email to profile | ||
""" | ||
previous_email_attempts = len(self.email_attempts) | ||
|
||
client_secret = "foobar" | ||
session_id = self._request_token(request_email, client_secret) | ||
|
||
self.assertEquals(len(self.email_attempts), 1) | ||
self.assertEquals(len(self.email_attempts) - previous_email_attempts, 1) | ||
link = self._get_link_from_email() | ||
|
||
self._validate_token(link) | ||
|
@@ -855,4 +931,6 @@ def _add_email(self, request_email, expected_email): | |
|
||
self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) | ||
self.assertEqual("email", channel.json_body["threepids"][0]["medium"]) | ||
self.assertEqual(expected_email, channel.json_body["threepids"][0]["address"]) | ||
|
||
threepids = {threepid["address"] for threepid in channel.json_body["threepids"]} | ||
self.assertIn(expected_email, threepids) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why this change?
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.
Because now mypy hates me. I spent a while trying to figure out how to make the type hints in
synapse/app/ratelimiting.py
accept floats or ints for burst count and I almost cried.Anyway, it doesn't really make sense for burst count to be a float so 🤷
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.
I'm surprised there isn't a
Number = Union[int, float]
🤷 We could add that tosynapse.types
if it was useful.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.
I recommend not looking at
numbers.Number