From df78b3ca17ceec7ee88118586d66fe516755b1b0 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 19:10:09 +0100 Subject: [PATCH 01/10] add can_requester_do_action --- synapse/api/ratelimiting.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py index ec6b3a69a2af..cfc4c7f2c052 100644 --- a/synapse/api/ratelimiting.py +++ b/synapse/api/ratelimiting.py @@ -17,6 +17,7 @@ from typing import Any, Optional, Tuple from synapse.api.errors import LimitExceededError +from synapse.types import Requester from synapse.util import Clock @@ -43,6 +44,27 @@ def __init__(self, clock: Clock, rate_hz: float, burst_count: int): # * The rate_hz of this particular entry. This can vary per request self.actions = OrderedDict() # type: OrderedDict[Any, Tuple[float, int, float]] + def can_requester_do_action( + self, + requester: Requester, + rate_hz: Optional[float] = None, + burst_count: Optional[int] = None, + update: bool = True, + _time_now_s: Optional[int] = None, + ) -> Tuple[bool, float]: + # Disable rate limiting of users belonging to any AS that is configured + # not to be rate limited in its registration file (rate_limited: true|false). + if requester.app_service and not requester.app_service.is_rate_limited(): + return [True, -1] + + return self.can_do_action( + requester.user.to_string(), + rate_hz, + burst_count, + update, + _time_now_s + ) + def can_do_action( self, key: Any, From 1876057ffdac0cb3fdf42efb9af2415ada513f55 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 19:10:34 +0100 Subject: [PATCH 02/10] use can_requester_do_action --- synapse/handlers/room_member.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 31705cdbdb7d..8cbe1b5379c3 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -459,8 +459,8 @@ async def _update_membership( if is_host_in_room: time_now_s = self.clock.time() - allowed, time_allowed = self._join_rate_limiter_local.can_do_action( - requester.user.to_string(), + allowed, time_allowed = self._join_rate_limiter_local.can_requester_do_action( + requester, ) if not allowed: @@ -470,8 +470,8 @@ async def _update_membership( else: time_now_s = self.clock.time() - allowed, time_allowed = self._join_rate_limiter_remote.can_do_action( - requester.user.to_string(), + allowed, time_allowed = self._join_rate_limiter_remote.can_requester_do_action( + requester, ) if not allowed: From a588d2d2794524d72cb255d39d76ba3fe4a7246b Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 19:10:45 +0100 Subject: [PATCH 03/10] Add tests for appservice --- tests/api/test_ratelimiting.py | 61 ++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py index d580e729c5eb..33b65c24613b 100644 --- a/tests/api/test_ratelimiting.py +++ b/tests/api/test_ratelimiting.py @@ -1,4 +1,6 @@ from synapse.api.ratelimiting import LimitExceededError, Ratelimiter +from synapse.appservice import ApplicationService +from synapse.types import create_requester from tests import unittest @@ -18,6 +20,65 @@ def test_allowed_via_can_do_action(self): self.assertTrue(allowed) self.assertEquals(20.0, time_allowed) + def test_allowed_user_via_can_requester_do_action(self): + user_requester = create_requester("@user:example.com") + limiter = Ratelimiter(clock=None, rate_hz=0.1, burst_count=1) + allowed, time_allowed = limiter.can_requester_do_action(user_requester, _time_now_s=0) + self.assertTrue(allowed) + self.assertEquals(10.0, time_allowed) + + allowed, time_allowed = limiter.can_requester_do_action(user_requester, _time_now_s=5) + self.assertFalse(allowed) + self.assertEquals(10.0, time_allowed) + + allowed, time_allowed = limiter.can_requester_do_action(user_requester, _time_now_s=10) + self.assertTrue(allowed) + self.assertEquals(20.0, time_allowed) + + def test_allowed_appservice_ratelimited_via_can_requester_do_action(self): + appservice = ApplicationService( + None, + "example.com", + id="foo", + rate_limited=True, + ) + as_requester = create_requester("@user:example.com", app_service=appservice) + + limiter = Ratelimiter(clock=None, rate_hz=0.1, burst_count=1) + allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=0) + self.assertTrue(allowed) + self.assertEquals(10.0, time_allowed) + + allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=5) + self.assertFalse(allowed) + self.assertEquals(10.0, time_allowed) + + allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=10) + self.assertTrue(allowed) + self.assertEquals(20.0, time_allowed) + + def test_allowed_appservice_via_can_requester_do_action(self): + appservice = ApplicationService( + None, + "example.com", + id="foo", + rate_limited=False, + ) + as_requester = create_requester("@user:example.com", app_service=appservice) + + limiter = Ratelimiter(clock=None, rate_hz=0.1, burst_count=1) + allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=0) + self.assertTrue(allowed) + self.assertEquals(-1, time_allowed) + + allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=5) + self.assertTrue(allowed) + self.assertEquals(-1, time_allowed) + + allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=10) + self.assertTrue(allowed) + self.assertEquals(-1, time_allowed) + def test_allowed_via_ratelimit(self): limiter = Ratelimiter(clock=None, rate_hz=0.1, burst_count=1) From 1360623ae9879b444319f53cd4936c62afa99ae4 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 19:16:11 +0100 Subject: [PATCH 04/10] changelog --- .gitignore | 44 -------------------------------------------- 1 file changed, 44 deletions(-) diff --git a/.gitignore b/.gitignore index af36c00cfaa6..e69de29bb2d1 100644 --- a/.gitignore +++ b/.gitignore @@ -1,44 +0,0 @@ -# filename patterns -*~ -.*.swp -.#* -*.deb -*.egg -*.egg-info -*.lock -*.pyc -*.snap -*.tac -_trial_temp/ -_trial_temp*/ -/out - -# stuff that is likely to exist when you run a server locally -/*.db -/*.log -/*.log.config -/*.pid -/.python-version -/*.signing.key -/env/ -/homeserver*.yaml -/logs -/media_store/ -/uploads - -# IDEs -/.idea/ -/.ropeproject/ -/.vscode/ - -# build products -!/.coveragerc -/.coverage* -/.mypy_cache/ -/.tox -/build/ -/coverage.* -/dist/ -/docs/build/ -/htmlcov -/pip-wheel-metadata/ From 9ceae6a7cbe98f8d8532517d4b55db756c03788a Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 19:16:17 +0100 Subject: [PATCH 05/10] fix types --- .gitignore | 44 +++++++++++++++++++++++++++++++++++++ synapse/api/ratelimiting.py | 2 +- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index e69de29bb2d1..af36c00cfaa6 100644 --- a/.gitignore +++ b/.gitignore @@ -0,0 +1,44 @@ +# filename patterns +*~ +.*.swp +.#* +*.deb +*.egg +*.egg-info +*.lock +*.pyc +*.snap +*.tac +_trial_temp/ +_trial_temp*/ +/out + +# stuff that is likely to exist when you run a server locally +/*.db +/*.log +/*.log.config +/*.pid +/.python-version +/*.signing.key +/env/ +/homeserver*.yaml +/logs +/media_store/ +/uploads + +# IDEs +/.idea/ +/.ropeproject/ +/.vscode/ + +# build products +!/.coveragerc +/.coverage* +/.mypy_cache/ +/.tox +/build/ +/coverage.* +/dist/ +/docs/build/ +/htmlcov +/pip-wheel-metadata/ diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py index cfc4c7f2c052..ce0c50ad16df 100644 --- a/synapse/api/ratelimiting.py +++ b/synapse/api/ratelimiting.py @@ -55,7 +55,7 @@ def can_requester_do_action( # Disable rate limiting of users belonging to any AS that is configured # not to be rate limited in its registration file (rate_limited: true|false). if requester.app_service and not requester.app_service.is_rate_limited(): - return [True, -1] + return True, -1.0 return self.can_do_action( requester.user.to_string(), From a7b60728658eb69cadde0d28ede4c159959fba97 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 19:19:45 +0100 Subject: [PATCH 06/10] changelog --- changelog.d/8139.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8139.bugfix diff --git a/changelog.d/8139.bugfix b/changelog.d/8139.bugfix new file mode 100644 index 000000000000..d3c6a42bd739 --- /dev/null +++ b/changelog.d/8139.bugfix @@ -0,0 +1 @@ +Appservices which are exempt from ratelimiting are no longer ratelimited when joining rooms. \ No newline at end of file From ef95f6790459343ec1afcf3a541c96623db70d7d Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 19:21:55 +0100 Subject: [PATCH 07/10] reformatted --- synapse/api/ratelimiting.py | 6 +---- synapse/handlers/room_member.py | 14 +++++----- tests/api/test_ratelimiting.py | 46 +++++++++++++++++++++------------ 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py index ce0c50ad16df..df1e358b59bd 100644 --- a/synapse/api/ratelimiting.py +++ b/synapse/api/ratelimiting.py @@ -58,11 +58,7 @@ def can_requester_do_action( return True, -1.0 return self.can_do_action( - requester.user.to_string(), - rate_hz, - burst_count, - update, - _time_now_s + requester.user.to_string(), rate_hz, burst_count, update, _time_now_s ) def can_do_action( diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 8cbe1b5379c3..0cd59bce3b28 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -459,9 +459,10 @@ async def _update_membership( if is_host_in_room: time_now_s = self.clock.time() - allowed, time_allowed = self._join_rate_limiter_local.can_requester_do_action( - requester, - ) + ( + allowed, + time_allowed, + ) = self._join_rate_limiter_local.can_requester_do_action(requester,) if not allowed: raise LimitExceededError( @@ -470,9 +471,10 @@ async def _update_membership( else: time_now_s = self.clock.time() - allowed, time_allowed = self._join_rate_limiter_remote.can_requester_do_action( - requester, - ) + ( + allowed, + time_allowed, + ) = self._join_rate_limiter_remote.can_requester_do_action(requester,) if not allowed: raise LimitExceededError( diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py index 33b65c24613b..1e1f30d790e0 100644 --- a/tests/api/test_ratelimiting.py +++ b/tests/api/test_ratelimiting.py @@ -23,59 +23,71 @@ def test_allowed_via_can_do_action(self): def test_allowed_user_via_can_requester_do_action(self): user_requester = create_requester("@user:example.com") limiter = Ratelimiter(clock=None, rate_hz=0.1, burst_count=1) - allowed, time_allowed = limiter.can_requester_do_action(user_requester, _time_now_s=0) + allowed, time_allowed = limiter.can_requester_do_action( + user_requester, _time_now_s=0 + ) self.assertTrue(allowed) self.assertEquals(10.0, time_allowed) - allowed, time_allowed = limiter.can_requester_do_action(user_requester, _time_now_s=5) + allowed, time_allowed = limiter.can_requester_do_action( + user_requester, _time_now_s=5 + ) self.assertFalse(allowed) self.assertEquals(10.0, time_allowed) - allowed, time_allowed = limiter.can_requester_do_action(user_requester, _time_now_s=10) + allowed, time_allowed = limiter.can_requester_do_action( + user_requester, _time_now_s=10 + ) self.assertTrue(allowed) self.assertEquals(20.0, time_allowed) def test_allowed_appservice_ratelimited_via_can_requester_do_action(self): appservice = ApplicationService( - None, - "example.com", - id="foo", - rate_limited=True, + None, "example.com", id="foo", rate_limited=True, ) as_requester = create_requester("@user:example.com", app_service=appservice) limiter = Ratelimiter(clock=None, rate_hz=0.1, burst_count=1) - allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=0) + allowed, time_allowed = limiter.can_requester_do_action( + as_requester, _time_now_s=0 + ) self.assertTrue(allowed) self.assertEquals(10.0, time_allowed) - allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=5) + allowed, time_allowed = limiter.can_requester_do_action( + as_requester, _time_now_s=5 + ) self.assertFalse(allowed) self.assertEquals(10.0, time_allowed) - allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=10) + allowed, time_allowed = limiter.can_requester_do_action( + as_requester, _time_now_s=10 + ) self.assertTrue(allowed) self.assertEquals(20.0, time_allowed) def test_allowed_appservice_via_can_requester_do_action(self): appservice = ApplicationService( - None, - "example.com", - id="foo", - rate_limited=False, + None, "example.com", id="foo", rate_limited=False, ) as_requester = create_requester("@user:example.com", app_service=appservice) limiter = Ratelimiter(clock=None, rate_hz=0.1, burst_count=1) - allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=0) + allowed, time_allowed = limiter.can_requester_do_action( + as_requester, _time_now_s=0 + ) self.assertTrue(allowed) self.assertEquals(-1, time_allowed) - allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=5) + allowed, time_allowed = limiter.can_requester_do_action( + as_requester, _time_now_s=5 + ) self.assertTrue(allowed) self.assertEquals(-1, time_allowed) - allowed, time_allowed = limiter.can_requester_do_action(as_requester, _time_now_s=10) + allowed, time_allowed = limiter.can_requester_do_action( + as_requester, _time_now_s=10 + ) self.assertTrue(allowed) self.assertEquals(-1, time_allowed) From 99e9c9ab57e82e99efe65c450446a07c1538dc18 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 20 Aug 2020 20:37:06 +0100 Subject: [PATCH 08/10] Update changelog.d/8139.bugfix Co-authored-by: Patrick Cloke --- changelog.d/8139.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8139.bugfix b/changelog.d/8139.bugfix index d3c6a42bd739..cfd23daaff5c 100644 --- a/changelog.d/8139.bugfix +++ b/changelog.d/8139.bugfix @@ -1 +1 @@ -Appservices which are exempt from ratelimiting are no longer ratelimited when joining rooms. \ No newline at end of file +Appservices which are exempt from ratelimiting are no longer ratelimited when joining rooms. This fixes a bug which was introduced in v1.19.0. From 9660ae5df78a9f0e7874ac38b1949bd3aea9461c Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 21 Aug 2020 11:52:01 +0100 Subject: [PATCH 09/10] Update changelog.d/8139.bugfix Co-authored-by: Erik Johnston --- changelog.d/8139.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8139.bugfix b/changelog.d/8139.bugfix index cfd23daaff5c..21f65d87b7d5 100644 --- a/changelog.d/8139.bugfix +++ b/changelog.d/8139.bugfix @@ -1 +1 @@ -Appservices which are exempt from ratelimiting are no longer ratelimited when joining rooms. This fixes a bug which was introduced in v1.19.0. +Fixes a bug where appservices with ratelimiting disabled would still be ratelimited when joining rooms. This bug was introduced in v1.19.0. From e46fe0810be1205d5c41caf12f4b9ccfdd93abbe Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 21 Aug 2020 11:55:00 +0100 Subject: [PATCH 10/10] Add docstring --- synapse/api/ratelimiting.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py index df1e358b59bd..e62ae50ac29d 100644 --- a/synapse/api/ratelimiting.py +++ b/synapse/api/ratelimiting.py @@ -52,6 +52,25 @@ def can_requester_do_action( update: bool = True, _time_now_s: Optional[int] = None, ) -> Tuple[bool, float]: + """Can the requester perform the action? + + Args: + requester: The requester to key off when rate limiting. The user property + will be used. + rate_hz: The long term number of actions that can be performed in a second. + Overrides the value set during instantiation if set. + burst_count: How many actions that can be performed before being limited. + Overrides the value set during instantiation if set. + update: Whether to count this check as performing the action + _time_now_s: The current time. Optional, defaults to the current time according + to self.clock. Only used by tests. + + Returns: + A tuple containing: + * A bool indicating if they can perform the action now + * The reactor timestamp for when the action can be performed next. + -1 if rate_hz is less than or equal to zero + """ # Disable rate limiting of users belonging to any AS that is configured # not to be rate limited in its registration file (rate_limited: true|false). if requester.app_service and not requester.app_service.is_rate_limited():