From 373ae381015a28e4db513b9abd16ce8aadbef308 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 15:07:17 +0100 Subject: [PATCH 1/6] Fix join ratelimiter breaking profile updates and idempotency --- synapse/handlers/room_member.py | 46 ++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 0cd59bce3b28..9fcabb22c798 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -210,24 +210,40 @@ async def _local_membership_update( _, stream_id = await self.store.get_event_ordering(duplicate.event_id) return duplicate.event_id, stream_id - stream_id = await self.event_creation_handler.handle_new_client_event( - requester, event, context, extra_users=[target], ratelimit=ratelimit, - ) - prev_state_ids = await context.get_prev_state_ids() prev_member_event_id = prev_state_ids.get((EventTypes.Member, user_id), None) + newly_joined = False if event.membership == Membership.JOIN: - # Only fire user_joined_room if the user has actually joined the - # room. Don't bother if the user is just changing their profile - # info. newly_joined = True if prev_member_event_id: prev_member_event = await self.store.get_event(prev_member_event_id) newly_joined = prev_member_event.membership != Membership.JOIN + + # Only rate-limit if the user actually joined the room, otherwise we'll end + # up blocking profile updates. if newly_joined: - await self._user_joined_room(target, room_id) + time_now_s = self.clock.time() + ( + allowed, + time_allowed, + ) = self._join_rate_limiter_local.can_requester_do_action(requester) + + if not allowed: + raise LimitExceededError( + retry_after_ms=int(1000 * (time_allowed - time_now_s)) + ) + + stream_id = await self.event_creation_handler.handle_new_client_event( + requester, event, context, extra_users=[target], ratelimit=ratelimit, + ) + + if event.membership == Membership.JOIN and newly_joined: + # Only fire user_joined_room if the user has actually joined the + # room. Don't bother if the user is just changing their profile + # info. + await self._user_joined_room(target, room_id) elif event.membership == Membership.LEAVE: if prev_member_event_id: prev_member_event = await self.store.get_event(prev_member_event_id) @@ -457,19 +473,7 @@ async def _update_membership( # so don't really fit into the general auth process. raise AuthError(403, "Guest access not allowed") - if is_host_in_room: - time_now_s = self.clock.time() - ( - allowed, - time_allowed, - ) = self._join_rate_limiter_local.can_requester_do_action(requester,) - - if not allowed: - raise LimitExceededError( - retry_after_ms=int(1000 * (time_allowed - time_now_s)) - ) - - else: + if not is_host_in_room: time_now_s = self.clock.time() ( allowed, From 4dccad8c57479a474069a8629d16c5c691e0e145 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 15:17:58 +0100 Subject: [PATCH 2/6] Changelog --- changelog.d/8153.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8153.bugfix diff --git a/changelog.d/8153.bugfix b/changelog.d/8153.bugfix new file mode 100644 index 000000000000..2ca375f4ec9c --- /dev/null +++ b/changelog.d/8153.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.19.0 that would cause non-join operations (e.g. calling `/join` on a room the user has already joined, or sending a profile update) to be rate-limited. From c22e629a9f67a2413d87b05eca71f1701de62bd2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 16:57:38 +0100 Subject: [PATCH 3/6] Add test case --- tests/rest/client/v1/test_rooms.py | 84 +++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index ef6b775ed2b8..25d7e891ebe5 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -28,7 +28,7 @@ from synapse.handlers.pagination import PurgeStatus from synapse.rest.client.v1 import directory, login, profile, room from synapse.rest.client.v2_alpha import account -from synapse.types import JsonDict, RoomAlias +from synapse.types import JsonDict, RoomAlias, UserID from synapse.util.stringutils import random_string from tests import unittest @@ -675,6 +675,88 @@ def test_rooms_members_other_custom_keys(self): self.assertEquals(json.loads(content), channel.json_body) +class RoomMembershipUpdateTestCase(RoomBase): + user_id = "@sid1:red" + + servlets = [ + profile.register_servlets, + room.register_servlets, + ] + + @unittest.override_config( + { + "rc_joins": { + "local": {"per_second": 3, "burst_count": 3}, + } + } + ) + def test_join_local_ratelimit_profile_change(self): + """Tests that sending a profile update into all of the user's joined rooms isn't + rate-limited by the rate-limiter on joins.""" + + # Create and join more rooms than the rate-limiting config allows in a second. + room_ids = [ + self.helper.create_room_as(self.user_id), + self.helper.create_room_as(self.user_id), + self.helper.create_room_as(self.user_id), + ] + self.reactor.advance(1) + room_ids = room_ids + [ + self.helper.create_room_as(self.user_id), + self.helper.create_room_as(self.user_id), + self.helper.create_room_as(self.user_id), + ] + + # Create a profile for the user, since it hasn't been done on registration. + store = self.hs.get_datastore() + store.create_profile(UserID.from_string(self.user_id).localpart) + + # Update the display name for the user. + path = "/_matrix/client/r0/profile/%s/displayname" % self.user_id + request, channel = self.make_request("PUT", path, {"displayname": "John Doe"}) + self.render(request) + self.assertEquals(channel.code, 200, channel.json_body) + + # Check that all the rooms have been sent a profile update into. + for room_id in room_ids: + path = "/_matrix/client/r0/rooms/%s/state/m.room.member/%s" % ( + room_id, self.user_id, + ) + + request, channel = self.make_request("GET", path) + self.render(request) + self.assertEquals(channel.code, 200) + + self.assertIn("displayname", channel.json_body) + self.assertEquals(channel.json_body["displayname"], "John Doe") + + @unittest.override_config( + { + "rc_joins": { + "local": {"per_second": 3, "burst_count": 3}, + } + } + ) + def test_join_local_ratelimit_idempotent(self): + """Tests that the room join endpoints remain idempotent despite rate-limiting + on room joins.""" + room_id = self.helper.create_room_as(self.user_id) + + # Let's test both paths to be sure. + paths_to_test = [ + "/_matrix/client/r0/rooms/%s/join", + "/_matrix/client/r0/join/%s", + ] + + for path in paths_to_test: + # Make sure we send more requests than the rate-limiting config would allow + # if all of these requests ended up joining the user to a room. + for i in range(6): + request, channel = self.make_request("POST", path % room_id, {}) + self.render(request) + self.assertEquals(channel.code, 200) + + class RoomMessagesTestCase(RoomBase): """ Tests /rooms/$room_id/messages/$user_id/$msg_id REST events. """ From d96ca0574953c6d66c27ce712ff99b429ec9cd04 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 17:01:14 +0100 Subject: [PATCH 4/6] Also test ratelimiting while we're there --- tests/rest/client/v1/test_rooms.py | 16 +++++++++++++++- tests/rest/client/v1/utils.py | 14 +++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index 25d7e891ebe5..21d432660c25 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -675,7 +675,7 @@ def test_rooms_members_other_custom_keys(self): self.assertEquals(json.loads(content), channel.json_body) -class RoomMembershipUpdateTestCase(RoomBase): +class RoomJoinRatelimitTestCase(RoomBase): user_id = "@sid1:red" servlets = [ @@ -683,6 +683,20 @@ class RoomMembershipUpdateTestCase(RoomBase): room.register_servlets, ] + @unittest.override_config( + { + "rc_joins": { + "local": {"per_second": 3, "burst_count": 3}, + } + } + ) + def test_join_local_ratelimit(self): + """Tests that local joins are actually rate-limited.""" + for i in range(5): + self.helper.create_room_as(self.user_id) + + self.helper.create_room_as(self.user_id, expect_code=429) + @unittest.override_config( { "rc_joins": { diff --git a/tests/rest/client/v1/utils.py b/tests/rest/client/v1/utils.py index 8933b560d2cb..1ddb0feacdf8 100644 --- a/tests/rest/client/v1/utils.py +++ b/tests/rest/client/v1/utils.py @@ -39,7 +39,13 @@ class RestHelper(object): resource = attr.ib() auth_user_id = attr.ib() - def create_room_as(self, room_creator=None, is_public=True, tok=None): + def create_room_as( + self, + room_creator=None, + is_public=True, + tok=None, + expect_code=200, + ): temp_id = self.auth_user_id self.auth_user_id = room_creator path = "/_matrix/client/r0/createRoom" @@ -54,9 +60,11 @@ def create_room_as(self, room_creator=None, is_public=True, tok=None): ) render(request, self.resource, self.hs.get_reactor()) - assert channel.result["code"] == b"200", channel.result + assert channel.result["code"] == b"%d" % expect_code, channel.result self.auth_user_id = temp_id - return channel.json_body["room_id"] + + if expect_code == 200: + return channel.json_body["room_id"] def invite(self, room=None, src=None, targ=None, expect_code=200, tok=None): self.change_membership( From 285eb2e07ea457b1e2199e553c3cbe9e812c81ae Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 17:02:16 +0100 Subject: [PATCH 5/6] Lint --- tests/rest/client/v1/test_rooms.py | 21 +++++---------------- tests/rest/client/v1/utils.py | 6 +----- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index 21d432660c25..e74bddc1e539 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -684,11 +684,7 @@ class RoomJoinRatelimitTestCase(RoomBase): ] @unittest.override_config( - { - "rc_joins": { - "local": {"per_second": 3, "burst_count": 3}, - } - } + {"rc_joins": {"local": {"per_second": 3, "burst_count": 3}}} ) def test_join_local_ratelimit(self): """Tests that local joins are actually rate-limited.""" @@ -698,11 +694,7 @@ def test_join_local_ratelimit(self): self.helper.create_room_as(self.user_id, expect_code=429) @unittest.override_config( - { - "rc_joins": { - "local": {"per_second": 3, "burst_count": 3}, - } - } + {"rc_joins": {"local": {"per_second": 3, "burst_count": 3}}} ) def test_join_local_ratelimit_profile_change(self): """Tests that sending a profile update into all of the user's joined rooms isn't @@ -734,7 +726,8 @@ def test_join_local_ratelimit_profile_change(self): # Check that all the rooms have been sent a profile update into. for room_id in room_ids: path = "/_matrix/client/r0/rooms/%s/state/m.room.member/%s" % ( - room_id, self.user_id, + room_id, + self.user_id, ) request, channel = self.make_request("GET", path) @@ -745,11 +738,7 @@ def test_join_local_ratelimit_profile_change(self): self.assertEquals(channel.json_body["displayname"], "John Doe") @unittest.override_config( - { - "rc_joins": { - "local": {"per_second": 3, "burst_count": 3}, - } - } + {"rc_joins": {"local": {"per_second": 3, "burst_count": 3}}} ) def test_join_local_ratelimit_idempotent(self): """Tests that the room join endpoints remain idempotent despite rate-limiting diff --git a/tests/rest/client/v1/utils.py b/tests/rest/client/v1/utils.py index 1ddb0feacdf8..e66c9a4c4c6c 100644 --- a/tests/rest/client/v1/utils.py +++ b/tests/rest/client/v1/utils.py @@ -40,11 +40,7 @@ class RestHelper(object): auth_user_id = attr.ib() def create_room_as( - self, - room_creator=None, - is_public=True, - tok=None, - expect_code=200, + self, room_creator=None, is_public=True, tok=None, expect_code=200, ): temp_id = self.auth_user_id self.auth_user_id = room_creator From 11786f40a5e5276791343a08f75e9ce17806e347 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 17:42:39 +0100 Subject: [PATCH 6/6] Update changelog.d/8153.bugfix Co-authored-by: Erik Johnston --- changelog.d/8153.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8153.bugfix b/changelog.d/8153.bugfix index 2ca375f4ec9c..87a1f46ca199 100644 --- a/changelog.d/8153.bugfix +++ b/changelog.d/8153.bugfix @@ -1 +1 @@ -Fix a bug introduced in v1.19.0 that would cause non-join operations (e.g. calling `/join` on a room the user has already joined, or sending a profile update) to be rate-limited. +Fix a bug introduced in v1.19.0 that would cause e.g. profile updates to fail due to incorrect application of rate limits on join requests.