diff --git a/changelog.d/9968.bugfix b/changelog.d/9968.bugfix
new file mode 100644
index 0000000000..39e75f9956
--- /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.
diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index 2244b8a340..b9a10283f4 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -57,6 +57,7 @@ class Ratelimiter:
rate_hz: Optional[float] = None,
burst_count: Optional[int] = None,
update: bool = True,
+ 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?
@@ -76,6 +77,9 @@ class Ratelimiter:
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 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.
@@ -124,17 +128,20 @@ class Ratelimiter:
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
+ performed_count = 0
time_start = time_now_s
- action_count = 1.0
- elif performed_count > burst_count - 1.0:
+
+ # 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
else:
# We haven't reached our limit yet
allowed = True
- action_count += 1.0
+ action_count = performed_count + n_actions
if update:
self.actions[key] = (action_count, time_start, rate_hz)
@@ -182,6 +189,7 @@ class Ratelimiter:
rate_hz: Optional[float] = None,
burst_count: Optional[int] = None,
update: bool = True,
+ n_actions: int = 1,
_time_now_s: Optional[int] = None,
):
"""Checks if an action can be performed. If not, raises a LimitExceededError
@@ -201,6 +209,9 @@ class Ratelimiter:
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 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.
@@ -216,6 +227,7 @@ class Ratelimiter:
rate_hz=rate_hz,
burst_count=burst_count,
update=update,
+ n_actions=n_actions,
_time_now_s=time_now_s,
)
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index fb4823a5cc..835d874cee 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -32,7 +32,14 @@ from synapse.api.constants import (
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 @@ class RoomCreationHandler(BaseHandler):
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,8 +679,18 @@ class RoomCreationHandler(BaseHandler):
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")
+ 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)
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 20700fc5a8..9a092da715 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -163,6 +163,31 @@ class RoomMemberHandler(metaclass=abc.ABCMeta):
async def forget(self, user: UserID, room_id: str) -> None:
raise NotImplementedError()
+ async def ratelimit_multiple_invites(
+ self,
+ requester: Optional[Requester],
+ room_id: Optional[str],
+ n_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.
+ n_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,
+ n_actions=n_invites,
+ )
+
async def ratelimit_invite(
self,
requester: Optional[Requester],
diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py
index fa96ba07a5..dcf0110c16 100644
--- a/tests/api/test_ratelimiting.py
+++ b/tests/api/test_ratelimiting.py
@@ -230,3 +230,60 @@ class TestRatelimiter(unittest.HomeserverTestCase):
# 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)
+ # 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.
+ 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)
+ # The time allowed is the current time because we could still repeat the action
+ # once.
+ self.assertEquals(20.0, time_allowed)
diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py
index a3694f3d02..7c4bdcdfdd 100644
--- a/tests/rest/client/v1/test_rooms.py
+++ b/tests/rest/client/v1/test_rooms.py
@@ -463,6 +463,43 @@ class RoomsCreateTestCase(RoomBase):
)
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. """
|
