* Correctly ratelimit invites when creating a room Also allow ratelimiting for more than one action at a time.tags/v1.34.0rc1
@@ -0,0 +1 @@ | |||
Fix a bug introduced in v1.27.0 preventing users and appservices exempt from ratelimiting from creating rooms with many invitees. |
@@ -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, | |||
) | |||
@@ -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) | |||
@@ -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], | |||
@@ -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) |
@@ -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. """ | |||