From 5f6828efaddcffb88a58de24adf2b3163c45b32e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 6 Jan 2022 14:41:33 +0000 Subject: [PATCH 1/6] Test case for an edgy race --- tests/handlers/test_sync.py | 140 +++++++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 638186f173f0..8c646ea72519 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -11,15 +11,14 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - from typing import Optional -from unittest.mock import Mock +from unittest.mock import MagicMock, Mock, patch from synapse.api.constants import EventTypes, JoinRules from synapse.api.errors import Codes, ResourceLimitError from synapse.api.filtering import Filtering from synapse.api.room_versions import RoomVersions -from synapse.handlers.sync import SyncConfig +from synapse.handlers.sync import SyncConfig, SyncResult from synapse.rest import admin from synapse.rest.client import knock, login, room from synapse.server import HomeServer @@ -27,6 +26,8 @@ import tests.unittest import tests.utils +from tests.test_utils import make_awaitable +from tests.test_utils.event_injection import inject_event class SyncTestCase(tests.unittest.HomeserverTestCase): @@ -186,6 +187,139 @@ def test_unknown_room_version(self): self.assertNotIn(invite_room, [r.room_id for r in result.invited]) self.assertNotIn(knock_room, [r.room_id for r in result.knocked]) + def test_ban_wins_race_with_join(self): + """Check rooms appear under the "leave" section if they lose a race to a ban. + + A complicated edge case. Best to quote RvdH from + https://github.com/matrix-org/synapse/pull/11532#discussion_r769104461: + + * you attempt to join a room + * racing with that, is a ban which comes in over federation, which ends up with + an earlier stream_ordering than the join. + * you get a sync response with a sync token which is _after_ the ban + * now your join lands; it is a valid event because its `prev_event`s predate the + ban, but will not make it into current_state_events (because bans win over + joins in state res, essentially). + * Hence, the only event in the timeline is your join ... and yet you aren't + joined. + """ + # A local user Alice creates a room. + owner = self.register_user("alice", "password") + owner_tok = self.login(owner, "password") + room_id = self.helper.create_room_as(owner, is_public=True, tok=owner_tok) + + # Do a sync as Alice to get the latest event in the room. + alice_sync_result: SyncResult = self.get_success( + self.sync_handler.wait_for_sync_for_user( + create_requester(owner), generate_sync_config(owner) + ) + ) + self.assertEqual(len(alice_sync_result.joined), 1) + self.assertEqual(alice_sync_result.joined[0].room_id, room_id) + last_room_creation_event_id = ( + alice_sync_result.joined[0].timeline.events[-1].event_id + ) + + # (Pretend that) a remote user Bob joins. + moderator = "@bob:example.com" + self.get_success( + inject_event( + self.hs, + sender=moderator, + prev_event_ids=[last_room_creation_event_id], + type=EventTypes.Member, + state_key=moderator, + room_id=room_id, + content={"membership": "join"}, + ) + ) + + # Alice makes Bob a moderator. + room_power_levels = self.helper.get_state( + room_id, + EventTypes.PowerLevels, + tok=owner_tok, + ) + room_power_levels["users"][moderator] = 50 + moderator_pl_event_id = self.helper.send_state( + room_id, + EventTypes.PowerLevels, + room_power_levels, + tok=owner_tok, + )["event_id"] + + # We now want to concoct a situation where: + # - another local user Eve attempts to join the room + # - the homeserver generates a join event with prev_events that precede the ban + # (so that it passes the "are you banned" test) + # - but the join event has a stream_ordering after that of the ban (so that it + # comes down /sync after the ban). + # + # We do this by + # 1. Ban eve from the room. + # 2. Have eve join the room. + # - mock the prev_events for the join event to precede the ban + + # First, the ban. + eve = self.register_user("eve", "password") + eve_token = self.login(eve, "password") + self.get_success( + inject_event( + self.hs, + sender=moderator, + prev_event_ids=[moderator_pl_event_id], + type=EventTypes.Member, + state_key=eve, + room_id=room_id, + content={"membership": "ban"}, + ) + ) + + # We need a sync_token for eve after the ban. + eve_requester = create_requester(eve) + eve_sync_config = generate_sync_config(eve) + eve_sync_after_ban: SyncResult = self.get_success( + self.sync_handler.wait_for_sync_for_user(eve_requester, eve_sync_config) + ) + since_token = eve_sync_after_ban.next_batch + # Sanity check this sync result. We should be banned from the room. + self.assertEqual(len(eve_sync_after_ban.joined), 0) + self.assertEqual(len(eve_sync_after_ban.archived), 1) + self.assertEqual(eve_sync_after_ban.archived[0].room_id, room_id) + + mocked_get_prev_events = patch.object( + self.hs.get_datastore(), + "get_prev_events_for_room", + new_callable=MagicMock, + return_value=make_awaitable([moderator_pl_event_id]), + ) + with mocked_get_prev_events: + self.helper.join(room_id, eve, tok=eve_token) + + # Eve makes a second, incremental sync. The join event should appear, but not + # under the "joined" section. + eve_incremental_sync_after_join: SyncResult = self.get_success( + self.sync_handler.wait_for_sync_for_user( + eve_requester, + eve_sync_config, + since_token=since_token, + ) + ) + # The incremental sync should have nothing to show. + self.assertEqual(len(eve_incremental_sync_after_join.joined), 0) + self.assertEqual(len(eve_incremental_sync_after_join.archived), 0) + + # If we did a third initial sync, we should _still_ see that eve is banned. + eve_initial_sync_after_join: SyncResult = self.get_success( + self.sync_handler.wait_for_sync_for_user( + eve_requester, + eve_sync_config, + ) + ) + self.assertEqual(len(eve_initial_sync_after_join.joined), 0) + self.assertEqual(len(eve_initial_sync_after_join.archived), 1) + self.assertEqual(eve_initial_sync_after_join.archived[0].room_id, room_id) + _request_key = 0 From b3566a7067c1e891b9e36ce16e408ca5e58c1901 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 6 Jan 2022 18:02:19 +0000 Subject: [PATCH 2/6] Changelog --- changelog.d/11701.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11701.misc diff --git a/changelog.d/11701.misc b/changelog.d/11701.misc new file mode 100644 index 000000000000..68905e041240 --- /dev/null +++ b/changelog.d/11701.misc @@ -0,0 +1 @@ +Add a test for [an edge case](https://github.com/matrix-org/synapse/pull/11532#discussion_r769104461) in the `/sync` logic. \ No newline at end of file From f6cb926c095b08337f39d94e4e1374456cd7105c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 7 Jan 2022 12:44:48 +0000 Subject: [PATCH 3/6] Update docstring --- tests/handlers/test_sync.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 8c646ea72519..740411dcb403 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -188,20 +188,28 @@ def test_unknown_room_version(self): self.assertNotIn(knock_room, [r.room_id for r in result.knocked]) def test_ban_wins_race_with_join(self): - """Check rooms appear under the "leave" section if they lose a race to a ban. + """Rooms shouldn't appear under "joined" if a join loses a race to a ban. - A complicated edge case. Best to quote RvdH from - https://github.com/matrix-org/synapse/pull/11532#discussion_r769104461: + A complicated edge case. Imagine the following scenario: * you attempt to join a room - * racing with that, is a ban which comes in over federation, which ends up with + * racing with that is a ban which comes in over federation, which ends up with an earlier stream_ordering than the join. - * you get a sync response with a sync token which is _after_ the ban + * you get a sync response with a sync token which is _after_ the ban, but before + the join * now your join lands; it is a valid event because its `prev_event`s predate the ban, but will not make it into current_state_events (because bans win over joins in state res, essentially). - * Hence, the only event in the timeline is your join ... and yet you aren't - joined. + * When we do a sync from the incremental sync, the only event in the timeline + is your join ... and yet you aren't joined. + + The ban coming in over federation isn't crucial for this behaviour; the key + requirements are: + 1. the homeserver generates a join event with prev_events that precede the ban + (so that it passes the "are you banned" test) + 2. the join event has a stream_ordering after that of the ban. + + We use monkeypatching to artificially trigger condition (1). """ # A local user Alice creates a room. owner = self.register_user("alice", "password") From 6f84217f445ccfd511cb56e6b071f462a930c401 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 7 Jan 2022 12:45:32 +0000 Subject: [PATCH 4/6] Rework test Don't use Bob any more. Don't test the contents of `archived`; just check the sync says that eve isn't joined. --- tests/handlers/test_sync.py | 88 ++++++++----------------------------- tests/rest/client/utils.py | 10 +++++ 2 files changed, 29 insertions(+), 69 deletions(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 740411dcb403..3ebed40aec6c 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -228,105 +228,55 @@ def test_ban_wins_race_with_join(self): alice_sync_result.joined[0].timeline.events[-1].event_id ) - # (Pretend that) a remote user Bob joins. - moderator = "@bob:example.com" - self.get_success( - inject_event( - self.hs, - sender=moderator, - prev_event_ids=[last_room_creation_event_id], - type=EventTypes.Member, - state_key=moderator, - room_id=room_id, - content={"membership": "join"}, - ) - ) - - # Alice makes Bob a moderator. - room_power_levels = self.helper.get_state( - room_id, - EventTypes.PowerLevels, - tok=owner_tok, - ) - room_power_levels["users"][moderator] = 50 - moderator_pl_event_id = self.helper.send_state( - room_id, - EventTypes.PowerLevels, - room_power_levels, - tok=owner_tok, - )["event_id"] - - # We now want to concoct a situation where: - # - another local user Eve attempts to join the room - # - the homeserver generates a join event with prev_events that precede the ban - # (so that it passes the "are you banned" test) - # - but the join event has a stream_ordering after that of the ban (so that it - # comes down /sync after the ban). - # - # We do this by - # 1. Ban eve from the room. - # 2. Have eve join the room. - # - mock the prev_events for the join event to precede the ban - - # First, the ban. + # Eve, a ne'er-do-well, registers. eve = self.register_user("eve", "password") eve_token = self.login(eve, "password") - self.get_success( - inject_event( - self.hs, - sender=moderator, - prev_event_ids=[moderator_pl_event_id], - type=EventTypes.Member, - state_key=eve, - room_id=room_id, - content={"membership": "ban"}, - ) - ) - # We need a sync_token for eve after the ban. + # Alice preemptively bans Eve. + self.helper.ban(room_id, owner, eve, tok=owner_tok) + + # Eve syncs. eve_requester = create_requester(eve) eve_sync_config = generate_sync_config(eve) eve_sync_after_ban: SyncResult = self.get_success( self.sync_handler.wait_for_sync_for_user(eve_requester, eve_sync_config) ) - since_token = eve_sync_after_ban.next_batch - # Sanity check this sync result. We should be banned from the room. - self.assertEqual(len(eve_sync_after_ban.joined), 0) - self.assertEqual(len(eve_sync_after_ban.archived), 1) - self.assertEqual(eve_sync_after_ban.archived[0].room_id, room_id) + # Sanity check this sync result. We shouldn't be joined to the room. + self.assertEqual(eve_sync_after_ban.joined, []) + + # Eve tries to join the room. We monkey patch the internal logic which selects + # the prev_events used when creating the join event, such that the ban does not + # precede the join. mocked_get_prev_events = patch.object( self.hs.get_datastore(), "get_prev_events_for_room", new_callable=MagicMock, - return_value=make_awaitable([moderator_pl_event_id]), + return_value=make_awaitable([last_room_creation_event_id]), ) with mocked_get_prev_events: self.helper.join(room_id, eve, tok=eve_token) - # Eve makes a second, incremental sync. The join event should appear, but not - # under the "joined" section. + # Eve makes a second, incremental sync. eve_incremental_sync_after_join: SyncResult = self.get_success( self.sync_handler.wait_for_sync_for_user( eve_requester, eve_sync_config, - since_token=since_token, + since_token=eve_sync_after_ban.next_batch, ) ) - # The incremental sync should have nothing to show. - self.assertEqual(len(eve_incremental_sync_after_join.joined), 0) - self.assertEqual(len(eve_incremental_sync_after_join.archived), 0) + # Eve should not see herself as joined to the room. + self.assertEqual(eve_incremental_sync_after_join.joined, []) - # If we did a third initial sync, we should _still_ see that eve is banned. + # If we did a third initial sync, we should _still_ see eve is not joined to the room. eve_initial_sync_after_join: SyncResult = self.get_success( self.sync_handler.wait_for_sync_for_user( eve_requester, eve_sync_config, + since_token=None, ) ) - self.assertEqual(len(eve_initial_sync_after_join.joined), 0) - self.assertEqual(len(eve_initial_sync_after_join.archived), 1) - self.assertEqual(eve_initial_sync_after_join.archived[0].room_id, room_id) + self.assertEqual(eve_initial_sync_after_join.joined, []) _request_key = 0 diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index 1af5e5cee504..f0b8d573baab 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -196,6 +196,16 @@ def leave(self, room=None, user=None, expect_code=200, tok=None): expect_code=expect_code, ) + def ban(self, room=None, src=None, targ=None, expect_code=200, tok=None): + self.change_membership( + room=room, + src=src, + targ=targ, + tok=tok, + membership=Membership.BAN, + expect_code=expect_code, + ) + def change_membership( self, room: str, From 3a812c0f7a43a4338fe5e50b8bb7dc9d0a8c254d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 7 Jan 2022 12:56:06 +0000 Subject: [PATCH 5/6] isort --- tests/handlers/test_sync.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 3ebed40aec6c..07a760e91aed 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -27,7 +27,6 @@ import tests.unittest import tests.utils from tests.test_utils import make_awaitable -from tests.test_utils.event_injection import inject_event class SyncTestCase(tests.unittest.HomeserverTestCase): From 70322ecea87af9d121b26d327bee205692de4996 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 7 Jan 2022 13:59:05 +0000 Subject: [PATCH 6/6] Tidy up ban helper - sensible signature that inherits the defaults of `change_membership` - docstring --- tests/rest/client/utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index f0b8d573baab..842438358021 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -196,14 +196,14 @@ def leave(self, room=None, user=None, expect_code=200, tok=None): expect_code=expect_code, ) - def ban(self, room=None, src=None, targ=None, expect_code=200, tok=None): + def ban(self, room: str, src: str, targ: str, **kwargs: object): + """A convenience helper: `change_membership` with `membership` preset to "ban".""" self.change_membership( room=room, src=src, targ=targ, - tok=tok, membership=Membership.BAN, - expect_code=expect_code, + **kwargs, ) def change_membership(