From 54ce81c86d163b883df67b97540426759a9f6363 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2019 10:46:38 +0100 Subject: [PATCH 1/7] Allow use of different ratelimits for admin redactions. This is useful to allow room admins to quickly deal with a large number of abusive messages. --- synapse/config/ratelimiting.py | 13 +++++++++++ synapse/handlers/_base.py | 41 +++++++++++++++++++++++++--------- synapse/handlers/message.py | 8 ++++++- synapse/server.py | 4 ++++ 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/synapse/config/ratelimiting.py b/synapse/config/ratelimiting.py index 33f31cf21..b4df6612d 100644 --- a/synapse/config/ratelimiting.py +++ b/synapse/config/ratelimiting.py @@ -80,6 +80,12 @@ class RatelimitConfig(Config): "federation_rr_transactions_per_room_per_second", 50 ) + rc_admin_redaction = config.get("rc_admin_redaction") + if rc_admin_redaction: + self.rc_admin_redaction = RateLimitConfig(rc_admin_redaction) + else: + self.rc_admin_redaction = None + def generate_config_section(self, **kwargs): return """\ ## Ratelimiting ## @@ -102,6 +108,9 @@ class RatelimitConfig(Config): # - one for login that ratelimits login requests based on the account the # client is attempting to log into, based on the amount of failed login # attempts for this account. + # - one for ratelimiting redactions by room admins. If this is not explicitly + # set then it uses the same ratelimiting as per rc_message. This is useful + # to allow room admins to quickly deal with abuse quickly. # # The defaults are as shown below. # @@ -123,6 +132,10 @@ class RatelimitConfig(Config): # failed_attempts: # per_second: 0.17 # burst_count: 3 + # + #rc_admin_redaction: + # per_second: 1 + # burst_count: 50 # Ratelimiting settings for incoming federation diff --git a/synapse/handlers/_base.py b/synapse/handlers/_base.py index c29c78bd6..853b72d8e 100644 --- a/synapse/handlers/_base.py +++ b/synapse/handlers/_base.py @@ -45,6 +45,7 @@ class BaseHandler(object): self.state_handler = hs.get_state_handler() self.distributor = hs.get_distributor() self.ratelimiter = hs.get_ratelimiter() + self.admin_redaction_ratelimiter = hs.get_admin_redaction_ratelimiter() self.clock = hs.get_clock() self.hs = hs @@ -53,7 +54,7 @@ class BaseHandler(object): self.event_builder_factory = hs.get_event_builder_factory() @defer.inlineCallbacks - def ratelimit(self, requester, update=True): + def ratelimit(self, requester, update=True, is_admin_redaction=False): """Ratelimits requests. Args: @@ -62,6 +63,9 @@ class BaseHandler(object): Set to False when doing multiple checks for one request (e.g. to check up front if we would reject the request), and set to True for the last call for a given request. + is_admin_redaction (bool): Whether this is a room admin/moderator + redacting an event. If so then we may apply different + ratelimits depending on config. Raises: LimitExceededError if the request should be ratelimited @@ -90,16 +94,33 @@ class BaseHandler(object): messages_per_second = override.messages_per_second burst_count = override.burst_count else: - messages_per_second = self.hs.config.rc_message.per_second - burst_count = self.hs.config.rc_message.burst_count + # We default to different values if this is an admin redaction and + # the config is set + if is_admin_redaction and self.hs.config.rc_admin_redaction: + messages_per_second = self.hs.config.rc_admin_redaction.per_second + burst_count = self.hs.config.rc_admin_redaction.burst_count + else: + messages_per_second = self.hs.config.rc_message.per_second + burst_count = self.hs.config.rc_message.burst_count - allowed, time_allowed = self.ratelimiter.can_do_action( - user_id, - time_now, - rate_hz=messages_per_second, - burst_count=burst_count, - update=update, - ) + if is_admin_redaction and self.hs.config.rc_admin_redaction: + # If we have separate config for admin redactions we use a separate + # ratelimiter. + allowed, time_allowed = self.admin_redaction_ratelimiter.can_do_action( + user_id, + time_now, + rate_hz=messages_per_second, + burst_count=burst_count, + update=update, + ) + else: + allowed, time_allowed = self.ratelimiter.can_do_action( + user_id, + time_now, + rate_hz=messages_per_second, + burst_count=burst_count, + update=update, + ) if not allowed: raise LimitExceededError( retry_after_ms=int(1000 * (time_allowed - time_now)) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 111f7c7e2..184170ef8 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -729,7 +729,13 @@ class EventCreationHandler(object): assert not self.config.worker_app if ratelimit: - yield self.base_handler.ratelimit(requester) + is_admin_redaction = ( + event.type == EventTypes.Redaction + and event.sender != requester.user.to_string() + ) + yield self.base_handler.ratelimit( + requester, is_admin_redaction=is_admin_redaction + ) yield self.base_handler.maybe_kick_guest_users(event, context) diff --git a/synapse/server.py b/synapse/server.py index 9e28dba2b..1fcc7375d 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -221,6 +221,7 @@ class HomeServer(object): self.clock = Clock(reactor) self.distributor = Distributor() self.ratelimiter = Ratelimiter() + self.admin_redaction_ratelimiter = Ratelimiter() self.registration_ratelimiter = Ratelimiter() self.datastore = None @@ -279,6 +280,9 @@ class HomeServer(object): def get_registration_ratelimiter(self): return self.registration_ratelimiter + def get_admin_redaction_ratelimiter(self): + return self.admin_redaction_ratelimiter + def build_federation_client(self): return FederationClient(self) From 2434c0084b670f54eb530b3ab6fad26aea6a075d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2019 10:48:52 +0100 Subject: [PATCH 2/7] Newsfile --- changelog.d/6015.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/6015.feature diff --git a/changelog.d/6015.feature b/changelog.d/6015.feature new file mode 100644 index 000000000..42aaffced --- /dev/null +++ b/changelog.d/6015.feature @@ -0,0 +1 @@ +Add config option to increase ratelimits for room admins redacting messages. From 8df88b5ff37641a07a1e75f0a08d3744c5140452 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2019 10:58:26 +0100 Subject: [PATCH 3/7] Update sample config --- docs/sample_config.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 9b1ae58a2..ce657921b 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -518,6 +518,9 @@ log_config: "CONFDIR/SERVERNAME.log.config" # - one for login that ratelimits login requests based on the account the # client is attempting to log into, based on the amount of failed login # attempts for this account. +# - one for ratelimiting redactions by room admins. If this is not explicitly +# set then it uses the same ratelimiting as per rc_message. This is useful +# to allow room admins to quickly deal with abuse quickly. # # The defaults are as shown below. # @@ -539,6 +542,10 @@ log_config: "CONFDIR/SERVERNAME.log.config" # failed_attempts: # per_second: 0.17 # burst_count: 3 +# +#rc_admin_redaction: +# per_second: 1 +# burst_count: 50 # Ratelimiting settings for incoming federation From c64c3bb4c5b740e3f505708bc5dde0b5b29de6b8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2019 11:16:17 +0100 Subject: [PATCH 4/7] Fix how we check for self redaction --- synapse/handlers/message.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 184170ef8..f97590941 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -729,10 +729,24 @@ class EventCreationHandler(object): assert not self.config.worker_app if ratelimit: - is_admin_redaction = ( - event.type == EventTypes.Redaction - and event.sender != requester.user.to_string() - ) + # We check if this is a room admin redacting an event so that we + # can apply different ratelimiting. We do this by simply checking + # its not a self-redaction (to avoid having to look up whether the + # user is actually admin or not). + is_admin_redaction = False + if event.type == EventTypes.Redaction: + original_event = yield self.store.get_event( + event.redacts, + check_redacted=False, + get_prev_content=False, + allow_rejected=False, + allow_none=True, + ) + + is_admin_redaction = ( + original_event and event.sender != original_event.sender + ) + yield self.base_handler.ratelimit( requester, is_admin_redaction=is_admin_redaction ) From caa9d6fed719a8a80eb4a998d32f09577d04f927 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2019 11:16:23 +0100 Subject: [PATCH 5/7] Add test for admin redaction ratelimiting. --- tests/rest/client/test_redactions.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/rest/client/test_redactions.py b/tests/rest/client/test_redactions.py index fe66e397c..1b1e991c4 100644 --- a/tests/rest/client/test_redactions.py +++ b/tests/rest/client/test_redactions.py @@ -30,6 +30,14 @@ class RedactionsTestCase(HomeserverTestCase): sync.register_servlets, ] + def make_homeserver(self, reactor, clock): + config = self.default_config() + + config["rc_message"] = {"per_second": 0.2, "burst_count": 10} + config["rc_admin_redaction"] = {"per_second": 1, "burst_count": 100} + + return self.setup_test_homeserver(config=config) + def prepare(self, reactor, clock, hs): # register a couple of users self.mod_user_id = self.register_user("user1", "pass") @@ -177,3 +185,20 @@ class RedactionsTestCase(HomeserverTestCase): self._redact_event( self.other_access_token, self.room_id, create_event_id, expect_code=403 ) + + def test_redact_event_as_moderator_ratelimit(self): + """Tests that the correct ratelimiting is applied to redactions + """ + + message_ids = [] + # as a regular user, send messages to redact + for _ in range(20): + b = self.helper.send(room_id=self.room_id, tok=self.other_access_token) + message_ids.append(b["event_id"]) + self.reactor.advance(10) # To get around ratelimits + + # as the moderator, send a bunch of redactions redaction + for msg_id in message_ids: + # These should all succeed, even though this would be denied by + # standard message ratelimiter + self._redact_event(self.mod_access_token, self.room_id, msg_id) From 57dd41a45b4df5d736e2f30d40926b60f367b500 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2019 13:54:50 +0100 Subject: [PATCH 6/7] Fix comments Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- synapse/config/ratelimiting.py | 2 +- synapse/handlers/_base.py | 2 +- synapse/handlers/message.py | 2 +- tests/rest/client/test_redactions.py | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/config/ratelimiting.py b/synapse/config/ratelimiting.py index b4df6612d..587e2862b 100644 --- a/synapse/config/ratelimiting.py +++ b/synapse/config/ratelimiting.py @@ -110,7 +110,7 @@ class RatelimitConfig(Config): # attempts for this account. # - one for ratelimiting redactions by room admins. If this is not explicitly # set then it uses the same ratelimiting as per rc_message. This is useful - # to allow room admins to quickly deal with abuse quickly. + # to allow room admins to deal with abuse quickly. # # The defaults are as shown below. # diff --git a/synapse/handlers/_base.py b/synapse/handlers/_base.py index 853b72d8e..d15c6282f 100644 --- a/synapse/handlers/_base.py +++ b/synapse/handlers/_base.py @@ -105,7 +105,7 @@ class BaseHandler(object): if is_admin_redaction and self.hs.config.rc_admin_redaction: # If we have separate config for admin redactions we use a separate - # ratelimiter. + # ratelimiter allowed, time_allowed = self.admin_redaction_ratelimiter.can_do_action( user_id, time_now, diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index f97590941..1f8272784 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -731,7 +731,7 @@ class EventCreationHandler(object): if ratelimit: # We check if this is a room admin redacting an event so that we # can apply different ratelimiting. We do this by simply checking - # its not a self-redaction (to avoid having to look up whether the + # it's not a self-redaction (to avoid having to look up whether the # user is actually admin or not). is_admin_redaction = False if event.type == EventTypes.Redaction: diff --git a/tests/rest/client/test_redactions.py b/tests/rest/client/test_redactions.py index 1b1e991c4..d2bcf256f 100644 --- a/tests/rest/client/test_redactions.py +++ b/tests/rest/client/test_redactions.py @@ -197,8 +197,8 @@ class RedactionsTestCase(HomeserverTestCase): message_ids.append(b["event_id"]) self.reactor.advance(10) # To get around ratelimits - # as the moderator, send a bunch of redactions redaction + # as the moderator, send a bunch of redactions for msg_id in message_ids: # These should all succeed, even though this would be denied by - # standard message ratelimiter + # the standard message ratelimiter self._redact_event(self.mod_access_token, self.room_id, msg_id) From 66ace43546e516938fa40b38bdd0d8d3ca9f9e31 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 11 Sep 2019 14:50:40 +0100 Subject: [PATCH 7/7] Update sample config --- docs/sample_config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index ce657921b..c970a1c67 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -520,7 +520,7 @@ log_config: "CONFDIR/SERVERNAME.log.config" # attempts for this account. # - one for ratelimiting redactions by room admins. If this is not explicitly # set then it uses the same ratelimiting as per rc_message. This is useful -# to allow room admins to quickly deal with abuse quickly. +# to allow room admins to deal with abuse quickly. # # The defaults are as shown below. #