From a12b2deeeb6c698404e28c75cecc8b1ece44e6e9 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 11 May 2021 14:52:43 +0100
Subject: [PATCH 01/10] Correctly ratelimit invites when creating a room

---
 synapse/api/ratelimiting.py        | 16 ++++++----
 synapse/handlers/room.py           | 27 ++++++++++++-----
 synapse/handlers/room_member.py    | 25 ++++++++++++++++
 tests/rest/client/v1/test_rooms.py | 47 ++++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 13 deletions(-)

diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index 2244b8a34062..3671c6e44a1c 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -57,6 +57,7 @@ async def can_do_action(
         rate_hz: Optional[float] = None,
         burst_count: Optional[int] = None,
         update: bool = True,
+        nb_actions: int = 1,
         _time_now_s: Optional[int] = None,
     ) -> Tuple[bool, float]:
         """Can the entity (e.g. user or IP address) perform the action?
@@ -76,6 +77,7 @@ async def can_do_action(
             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
+            nb_actions: The number of actions the user wants to do.
             _time_now_s: The current time. Optional, defaults to the current time according
                 to self.clock. Only used by tests.
 
@@ -124,17 +126,18 @@ async def can_do_action(
         time_delta = time_now_s - time_start
         performed_count = action_count - time_delta * rate_hz
         if performed_count < 0:
-            # Allow, reset back to count 1
-            allowed = True
+            # Whether the actions are allowed depends if performing them exceeds the
+            # burst count.
+            allowed = nb_actions <= burst_count
             time_start = time_now_s
-            action_count = 1.0
-        elif performed_count > burst_count - 1.0:
+            action_count = nb_actions
+        elif performed_count + nb_actions > burst_count:
             # Deny, we have exceeded our burst count
             allowed = False
         else:
             # We haven't reached our limit yet
             allowed = True
-            action_count += 1.0
+            action_count += nb_actions
 
         if update:
             self.actions[key] = (action_count, time_start, rate_hz)
@@ -182,6 +185,7 @@ async def ratelimit(
         rate_hz: Optional[float] = None,
         burst_count: Optional[int] = None,
         update: bool = True,
+        nb_actions: int = 1,
         _time_now_s: Optional[int] = None,
     ):
         """Checks if an action can be performed. If not, raises a LimitExceededError
@@ -201,6 +205,7 @@ async def ratelimit(
             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
+            nb_actions: The number of actions the user wants to do.
             _time_now_s: The current time. Optional, defaults to the current time according
                 to self.clock. Only used by tests.
 
@@ -216,6 +221,7 @@ async def ratelimit(
             rate_hz=rate_hz,
             burst_count=burst_count,
             update=update,
+            nb_actions=nb_actions,
             _time_now_s=time_now_s,
         )
 
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index fb4823a5cc6e..c9956b0952e7 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -32,7 +32,14 @@
     RoomCreationPreset,
     RoomEncryptionAlgorithms,
 )
-from synapse.api.errors import AuthError, Codes, NotFoundError, StoreError, SynapseError
+from synapse.api.errors import (
+    AuthError,
+    Codes,
+    LimitExceededError,
+    NotFoundError,
+    StoreError,
+    SynapseError
+)
 from synapse.api.filtering import Filter
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
 from synapse.events import EventBase
@@ -126,10 +133,6 @@ def __init__(self, hs: "HomeServer"):
 
         self.third_party_event_rules = hs.get_third_party_event_rules()
 
-        self._invite_burst_count = (
-            hs.config.ratelimiting.rc_invites_per_room.burst_count
-        )
-
     async def upgrade_room(
         self, requester: Requester, old_room_id: str, new_version: RoomVersion
     ) -> str:
@@ -676,9 +679,6 @@ async def create_room(
             invite_3pid_list = []
             invite_list = []
 
-        if len(invite_list) + len(invite_3pid_list) > self._invite_burst_count:
-            raise SynapseError(400, "Cannot invite so many users at once")
-
         await self.event_creation_handler.assert_accepted_privacy_policy(requester)
 
         power_level_content_override = config.get("power_level_content_override")
@@ -702,6 +702,17 @@ async def create_room(
             room_version=room_version,
         )
 
+        if invite_list or invite_3pid_list:
+            try:
+                await self.room_member_handler.ratelimit_multiple_invites(
+                    requester,
+                    room_id,
+                    nb_invites=len(invite_list) + len(invite_3pid_list),
+                    update=False,
+                )
+            except LimitExceededError:
+                raise SynapseError(400, "Cannot invite so many users at once")
+
         # Check whether this visibility value is blocked by a third party module
         allowed_by_third_party_rules = await (
             self.third_party_event_rules.check_visibility_can_be_modified(
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 20700fc5a8a4..f3a78ce4b02e 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -163,6 +163,31 @@ async def _user_left_room(self, target: UserID, room_id: str) -> None:
     async def forget(self, user: UserID, room_id: str) -> None:
         raise NotImplementedError()
 
+    async def ratelimit_multiple_invites(
+        self,
+        requester: Optional[Requester],
+        room_id: str,
+        nb_invites: int,
+        update: bool = True,
+    ):
+        """Ratelimit more than one invite sent by the given requester in the given room.
+
+        Args:
+            requester: The requester sending the invites.
+            room_id: The room the invites are being sent in.
+            nb_invites: The amount of invites to ratelimit for.
+            update: Whether to update the ratelimiter's cache.
+
+        Raises:
+            LimitExceededError: The requester can't send that many invites in the room.
+        """
+        await self._invites_per_room_limiter.ratelimit(
+            requester,
+            room_id,
+            update=update,
+            nb_actions=nb_invites,
+        )
+
     async def ratelimit_invite(
         self,
         requester: Optional[Requester],
diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py
index a3694f3d0200..304471f1e5f3 100644
--- a/tests/rest/client/v1/test_rooms.py
+++ b/tests/rest/client/v1/test_rooms.py
@@ -463,6 +463,53 @@ def test_post_room_invitees_invalid_mxid(self):
         )
         self.assertEquals(400, channel.code)
 
+    @unittest.override_config(
+        {
+            "rc_invites": {
+                "per_room": {
+                    "burst_count": 3
+                }
+            }
+        }
+    )
+    def test_post_room_invitees_ratelimit(self):
+        """Test that invites sent when creating a room are ratelimited by a RateLimiter,
+        which ratelimits them correctly, including by not limiting when the requester is
+        exempt from ratelimiting.
+        """
+
+        # Build the request's content. We use local MXIDs because invites over federation
+        # are more difficult to mock.
+        content = json.dumps({
+            "invite": [
+                "@alice1:red",
+                "@alice2:red",
+                "@alice3:red",
+                "@alice4:red",
+            ]
+        }).encode("utf8")
+
+        # Test that the invites are correctly ratelimited.
+        channel = self.make_request(
+            "POST", "/createRoom", content
+        )
+        self.assertEqual(400, channel.code)
+        self.assertEqual(
+            "Cannot invite so many users at once",
+            channel.json_body["error"],
+        )
+
+        # Add the current user to the ratelimit overrides, allowing them no ratelimiting.
+        self.get_success(
+            self.hs.get_datastore().set_ratelimit_for_user(self.user_id, 0, 0)
+        )
+
+        # Test that the invites aren't ratelimited anymore.
+        channel = self.make_request(
+            "POST", "/createRoom", content
+        )
+        self.assertEqual(200, channel.code)
+
 
 class RoomTopicTestCase(RoomBase):
     """ Tests /rooms/$room_id/topic REST events. """

From 159d88ac778c4250dffe248436d93861b011268d Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 11 May 2021 15:07:48 +0100
Subject: [PATCH 02/10] Changelog

---
 changelog.d/9968.bugfix | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/9968.bugfix

diff --git a/changelog.d/9968.bugfix b/changelog.d/9968.bugfix
new file mode 100644
index 000000000000..39e75f995650
--- /dev/null
+++ b/changelog.d/9968.bugfix
@@ -0,0 +1 @@
+Fix a bug introduced in v1.27.0 preventing users and appservices exempt from ratelimiting from creating rooms with many invitees.

From a146a8b742c48908b96c629f2e9daf7a5a95e731 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 11 May 2021 15:12:03 +0100
Subject: [PATCH 03/10] Lint

---
 synapse/handlers/room.py           |  2 +-
 tests/rest/client/v1/test_rooms.py | 36 +++++++++++-------------------
 2 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index c9956b0952e7..d75261afa47d 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -38,7 +38,7 @@
     LimitExceededError,
     NotFoundError,
     StoreError,
-    SynapseError
+    SynapseError,
 )
 from synapse.api.filtering import Filter
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py
index 304471f1e5f3..7c4bdcdfdd45 100644
--- a/tests/rest/client/v1/test_rooms.py
+++ b/tests/rest/client/v1/test_rooms.py
@@ -463,15 +463,7 @@ def test_post_room_invitees_invalid_mxid(self):
         )
         self.assertEquals(400, channel.code)
 
-    @unittest.override_config(
-        {
-            "rc_invites": {
-                "per_room": {
-                    "burst_count": 3
-                }
-            }
-        }
-    )
+    @unittest.override_config({"rc_invites": {"per_room": {"burst_count": 3}}})
     def test_post_room_invitees_ratelimit(self):
         """Test that invites sent when creating a room are ratelimited by a RateLimiter,
         which ratelimits them correctly, including by not limiting when the requester is
@@ -480,19 +472,19 @@ def test_post_room_invitees_ratelimit(self):
 
         # Build the request's content. We use local MXIDs because invites over federation
         # are more difficult to mock.
-        content = json.dumps({
-            "invite": [
-                "@alice1:red",
-                "@alice2:red",
-                "@alice3:red",
-                "@alice4:red",
-            ]
-        }).encode("utf8")
+        content = json.dumps(
+            {
+                "invite": [
+                    "@alice1:red",
+                    "@alice2:red",
+                    "@alice3:red",
+                    "@alice4:red",
+                ]
+            }
+        ).encode("utf8")
 
         # Test that the invites are correctly ratelimited.
-        channel = self.make_request(
-            "POST", "/createRoom", content
-        )
+        channel = self.make_request("POST", "/createRoom", content)
         self.assertEqual(400, channel.code)
         self.assertEqual(
             "Cannot invite so many users at once",
@@ -505,9 +497,7 @@ def test_post_room_invitees_ratelimit(self):
         )
 
         # Test that the invites aren't ratelimited anymore.
-        channel = self.make_request(
-            "POST", "/createRoom", content
-        )
+        channel = self.make_request("POST", "/createRoom", content)
         self.assertEqual(200, channel.code)
 
 

From 70a03942de30f83d6581957cfbc51843c956b3b7 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 11 May 2021 18:29:42 +0100
Subject: [PATCH 04/10] Incorporate review

---
 synapse/api/ratelimiting.py     | 16 +++++------
 synapse/handlers/room.py        | 24 +++++++++-------
 synapse/handlers/room_member.py |  8 +++---
 tests/api/test_ratelimiting.py  | 51 +++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 23 deletions(-)

diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index 3671c6e44a1c..29664fbda227 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -57,7 +57,7 @@ async def can_do_action(
         rate_hz: Optional[float] = None,
         burst_count: Optional[int] = None,
         update: bool = True,
-        nb_actions: int = 1,
+        n_actions: int = 1,
         _time_now_s: Optional[int] = None,
     ) -> Tuple[bool, float]:
         """Can the entity (e.g. user or IP address) perform the action?
@@ -77,7 +77,9 @@ async def can_do_action(
             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
-            nb_actions: The number of actions the user wants to do.
+            n_actions: The number of times the user wants to do this action. If the user
+                cannot do all of the actions, the user's action count is not incremented
+                at all.
             _time_now_s: The current time. Optional, defaults to the current time according
                 to self.clock. Only used by tests.
 
@@ -126,18 +128,16 @@ async def can_do_action(
         time_delta = time_now_s - time_start
         performed_count = action_count - time_delta * rate_hz
         if performed_count < 0:
-            # Whether the actions are allowed depends if performing them exceeds the
-            # burst count.
-            allowed = nb_actions <= burst_count
+            performed_count = 0
             time_start = time_now_s
-            action_count = nb_actions
-        elif performed_count + nb_actions > burst_count:
+
+        if performed_count + n_actions > burst_count:
             # Deny, we have exceeded our burst count
             allowed = False
         else:
             # We haven't reached our limit yet
             allowed = True
-            action_count += nb_actions
+            action_count += n_actions
 
         if update:
             self.actions[key] = (action_count, time_start, rate_hz)
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index d75261afa47d..835d874ceedb 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -679,6 +679,19 @@ async def create_room(
             invite_3pid_list = []
             invite_list = []
 
+        if invite_list or invite_3pid_list:
+            try:
+                # If there are invites in the request, see if the ratelimiting settings
+                # allow that number of invites to be sent from the current user.
+                await self.room_member_handler.ratelimit_multiple_invites(
+                    requester,
+                    room_id=None,
+                    n_invites=len(invite_list) + len(invite_3pid_list),
+                    update=False,
+                )
+            except LimitExceededError:
+                raise SynapseError(400, "Cannot invite so many users at once")
+
         await self.event_creation_handler.assert_accepted_privacy_policy(requester)
 
         power_level_content_override = config.get("power_level_content_override")
@@ -702,17 +715,6 @@ async def create_room(
             room_version=room_version,
         )
 
-        if invite_list or invite_3pid_list:
-            try:
-                await self.room_member_handler.ratelimit_multiple_invites(
-                    requester,
-                    room_id,
-                    nb_invites=len(invite_list) + len(invite_3pid_list),
-                    update=False,
-                )
-            except LimitExceededError:
-                raise SynapseError(400, "Cannot invite so many users at once")
-
         # Check whether this visibility value is blocked by a third party module
         allowed_by_third_party_rules = await (
             self.third_party_event_rules.check_visibility_can_be_modified(
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index f3a78ce4b02e..138ca10ced31 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -166,8 +166,8 @@ async def forget(self, user: UserID, room_id: str) -> None:
     async def ratelimit_multiple_invites(
         self,
         requester: Optional[Requester],
-        room_id: str,
-        nb_invites: int,
+        room_id: Optional[str],
+        n_invites: int,
         update: bool = True,
     ):
         """Ratelimit more than one invite sent by the given requester in the given room.
@@ -175,7 +175,7 @@ async def ratelimit_multiple_invites(
         Args:
             requester: The requester sending the invites.
             room_id: The room the invites are being sent in.
-            nb_invites: The amount of invites to ratelimit for.
+            n_invites: The amount of invites to ratelimit for.
             update: Whether to update the ratelimiter's cache.
 
         Raises:
@@ -185,7 +185,7 @@ async def ratelimit_multiple_invites(
             requester,
             room_id,
             update=update,
-            nb_actions=nb_invites,
+            nb_actions=n_invites,
         )
 
     async def ratelimit_invite(
diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py
index fa96ba07a590..86ecedd53c43 100644
--- a/tests/api/test_ratelimiting.py
+++ b/tests/api/test_ratelimiting.py
@@ -230,3 +230,54 @@ def test_db_user_override(self):
         # Shouldn't raise
         for _ in range(20):
             self.get_success_or_raise(limiter.ratelimit(requester, _time_now_s=0))
+
+    def test_multiple_actions(self):
+        limiter = Ratelimiter(
+            store=self.hs.get_datastore(), clock=None, rate_hz=0.1, burst_count=3
+        )
+        # Test that 4 actions aren't allowed with a maximum burst of 3.
+        allowed, time_allowed = self.get_success_or_raise(
+            limiter.can_do_action(None, key="test_id", n_actions=4, _time_now_s=0)
+        )
+        self.assertFalse(allowed)
+
+        # Test that 3 actions are allowed with a maximum burst of 3.
+        allowed, time_allowed = self.get_success_or_raise(
+            limiter.can_do_action(None, key="test_id", n_actions=3, _time_now_s=0)
+        )
+        self.assertTrue(allowed)
+        self.assertEquals(10.0, time_allowed)
+
+        # Test that, after doing these 3 actions, we can't do any more action without
+        # waiting.
+        allowed, time_allowed = self.get_success_or_raise(
+            limiter.can_do_action(None, key="test_id", n_actions=1, _time_now_s=0)
+        )
+        self.assertFalse(allowed)
+        self.assertEquals(10.0, time_allowed)
+
+        # Test that after waiting we can do only 1 action.
+        allowed, time_allowed = self.get_success_or_raise(
+            limiter.can_do_action(
+                None,
+                key="test_id",
+                update=False,
+                n_actions=1,
+                _time_now_s=10,
+            )
+        )
+        self.assertTrue(allowed)
+        self.assertEquals(20.0, time_allowed)
+
+        allowed, time_allowed = self.get_success_or_raise(
+            limiter.can_do_action(None, key="test_id", n_actions=2,_time_now_s=10)
+        )
+        self.assertFalse(allowed)
+        self.assertEquals(10.0, time_allowed)
+
+        # Test that after waiting a bit more we can do 2 actions.
+        allowed, time_allowed = self.get_success_or_raise(
+            limiter.can_do_action(None, key="test_id", n_actions=2, _time_now_s=20)
+        )
+        self.assertTrue(allowed)
+        self.assertEquals(30.0, time_allowed)

From 3308f662e80f1bc89d4c86a3a0548b2c082b0707 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 11 May 2021 18:34:22 +0100
Subject: [PATCH 05/10] Lint

---
 synapse/api/ratelimiting.py    | 6 +++---
 tests/api/test_ratelimiting.py | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index 29664fbda227..64f83c49bef4 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -185,7 +185,7 @@ async def ratelimit(
         rate_hz: Optional[float] = None,
         burst_count: Optional[int] = None,
         update: bool = True,
-        nb_actions: int = 1,
+        n_actions: int = 1,
         _time_now_s: Optional[int] = None,
     ):
         """Checks if an action can be performed. If not, raises a LimitExceededError
@@ -205,7 +205,7 @@ async def ratelimit(
             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
-            nb_actions: The number of actions the user wants to do.
+            n_actions: The number of actions the user wants to do.
             _time_now_s: The current time. Optional, defaults to the current time according
                 to self.clock. Only used by tests.
 
@@ -221,7 +221,7 @@ async def ratelimit(
             rate_hz=rate_hz,
             burst_count=burst_count,
             update=update,
-            nb_actions=nb_actions,
+            n_actions=n_actions,
             _time_now_s=time_now_s,
         )
 
diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py
index 86ecedd53c43..e8e58c309952 100644
--- a/tests/api/test_ratelimiting.py
+++ b/tests/api/test_ratelimiting.py
@@ -270,7 +270,7 @@ def test_multiple_actions(self):
         self.assertEquals(20.0, time_allowed)
 
         allowed, time_allowed = self.get_success_or_raise(
-            limiter.can_do_action(None, key="test_id", n_actions=2,_time_now_s=10)
+            limiter.can_do_action(None, key="test_id", n_actions=2, _time_now_s=10)
         )
         self.assertFalse(allowed)
         self.assertEquals(10.0, time_allowed)

From 3e13dbd7e8ce4234b0563887547eec622324a153 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 11 May 2021 18:35:11 +0100
Subject: [PATCH 06/10] Improve comment

---
 synapse/api/ratelimiting.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index 64f83c49bef4..c5a289371266 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -205,7 +205,9 @@ async def ratelimit(
             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
-            n_actions: The number of actions the user wants to do.
+            n_actions: The number of times the user wants to do this action. If the user
+                cannot do all of the actions, the user's action count is not incremented
+                at all.
             _time_now_s: The current time. Optional, defaults to the current time according
                 to self.clock. Only used by tests.
 

From e289030a096cb8a2f0be4e74b2203d2ab9e23aec Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 11 May 2021 18:39:12 +0100
Subject: [PATCH 07/10] Lint

---
 synapse/handlers/room_member.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 138ca10ced31..9a092da71597 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -185,7 +185,7 @@ async def ratelimit_multiple_invites(
             requester,
             room_id,
             update=update,
-            nb_actions=n_invites,
+            n_actions=n_invites,
         )
 
     async def ratelimit_invite(

From 0d53b05572457c1d29e41f4f29e63d03fd393f2a Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 12 May 2021 11:11:59 +0100
Subject: [PATCH 08/10] Fix tests

---
 synapse/api/ratelimiting.py    |  2 +-
 tests/api/test_ratelimiting.py | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index c5a289371266..425f01ecc671 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -137,7 +137,7 @@ async def can_do_action(
         else:
             # We haven't reached our limit yet
             allowed = True
-            action_count += n_actions
+            action_count = performed_count + n_actions
 
         if update:
             self.actions[key] = (action_count, time_start, rate_hz)
diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py
index e8e58c309952..dcf0110c16e0 100644
--- a/tests/api/test_ratelimiting.py
+++ b/tests/api/test_ratelimiting.py
@@ -267,12 +267,16 @@ def test_multiple_actions(self):
             )
         )
         self.assertTrue(allowed)
-        self.assertEquals(20.0, time_allowed)
+        # The time allowed is the current time because we could still repeat the action
+        # once.
+        self.assertEquals(10.0, time_allowed)
 
         allowed, time_allowed = self.get_success_or_raise(
             limiter.can_do_action(None, key="test_id", n_actions=2, _time_now_s=10)
         )
         self.assertFalse(allowed)
+        # The time allowed doesn't change despite allowed being False because, while we
+        # don't allow 2 actions, we could still do 1.
         self.assertEquals(10.0, time_allowed)
 
         # Test that after waiting a bit more we can do 2 actions.
@@ -280,4 +284,6 @@ def test_multiple_actions(self):
             limiter.can_do_action(None, key="test_id", n_actions=2, _time_now_s=20)
         )
         self.assertTrue(allowed)
-        self.assertEquals(30.0, time_allowed)
+        # The time allowed is the current time because we could still repeat the action
+        # once.
+        self.assertEquals(20.0, time_allowed)

From 9680166ee1fd648c5bbdd853a3e5a6855dc624b4 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 12 May 2021 14:18:35 +0100
Subject: [PATCH 09/10] uuugh

---
 synapse/api/ratelimiting.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index 425f01ecc671..8c9cc8dc3d50 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -131,7 +131,7 @@ async def can_do_action(
             performed_count = 0
             time_start = time_now_s
 
-        if performed_count + n_actions > burst_count:
+        if performed_count > burst_count - n_actions:
             # Deny, we have exceeded our burst count
             allowed = False
         else:

From b889bf6c4af782909e17f3baa31291eb36a414ad Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 12 May 2021 14:38:01 +0100
Subject: [PATCH 10/10] Add a comment explaining the uuuugh

---
 synapse/api/ratelimiting.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index 8c9cc8dc3d50..b9a10283f44a 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -131,6 +131,10 @@ async def can_do_action(
             performed_count = 0
             time_start = time_now_s
 
+        # This check would be easier read as performed_count + n_actions > burst_count,
+        # but performed_count might be a very precise float (with lots of numbers
+        # following the point) in which case Python might round it up when adding it to
+        # n_actions. Writing it this way ensures it doesn't happen.
         if performed_count > burst_count - n_actions:
             # Deny, we have exceeded our burst count
             allowed = False