From 3d72073633b77f1814f8cb3b12aa0a8faf8247fc Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 17 Aug 2021 12:01:34 -0400 Subject: [PATCH 1/5] Do not include rooms with an unknown room version in a sync response. --- mypy.ini | 1 + synapse/handlers/sync.py | 7 +- synapse/storage/databases/main/roommember.py | 8 ++- synapse/storage/roommember.py | 1 + tests/handlers/test_sync.py | 67 +++++++++++++++++-- .../replication/slave/storage/test_events.py | 1 + 6 files changed, 76 insertions(+), 9 deletions(-) diff --git a/mypy.ini b/mypy.ini index e1b9405daa85..0e0fcbda262d 100644 --- a/mypy.ini +++ b/mypy.ini @@ -87,6 +87,7 @@ files = tests/test_utils, tests/handlers/test_password_providers.py, tests/handlers/test_room_summary.py, + tests/handlers/test_sync.py, tests/rest/client/v1/test_login.py, tests/rest/client/v2_alpha/test_auth.py, tests/util/test_itertools.py, diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index b7b299961ff3..2203c45dcc9a 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1,5 +1,4 @@ -# Copyright 2015, 2016 OpenMarket Ltd -# Copyright 2018, 2019 New Vector Ltd +# Copyright 2015-2021 The Matrix.org Foundation C.I.C. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -31,6 +30,7 @@ from synapse.api.constants import AccountDataTypes, EventTypes, Membership from synapse.api.filtering import FilterCollection +from synapse.api.room_versions import KNOWN_ROOM_VERSIONS from synapse.events import EventBase from synapse.logging.context import current_context from synapse.logging.opentracing import SynapseTags, log_kv, set_tag, start_active_span @@ -1843,6 +1843,9 @@ async def _get_all_rooms( knocked = [] for event in room_list: + if event.room_version_id not in KNOWN_ROOM_VERSIONS: + continue + if event.membership == Membership.JOIN: room_entries.append( RoomSyncResultBuilder( diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index c2f6b9d63d70..c58a4b869072 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -386,9 +386,10 @@ def _get_rooms_for_local_user_where_membership_is_txn( ) sql = """ - SELECT room_id, e.sender, c.membership, event_id, e.stream_ordering + SELECT room_id, e.sender, c.membership, event_id, e.stream_ordering, r.room_version FROM local_current_membership AS c INNER JOIN events AS e USING (room_id, event_id) + INNER JOIN rooms AS r USING (room_id) WHERE user_id = ? AND %s @@ -397,7 +398,7 @@ def _get_rooms_for_local_user_where_membership_is_txn( ) txn.execute(sql, (user_id, *args)) - results = [RoomsForUser(**r) for r in self.db_pool.cursor_to_dict(txn)] + results = [RoomsForUser(*r) for r in txn] return results @@ -447,7 +448,8 @@ async def get_rooms_for_user_with_stream_ordering( Returns: Returns the rooms the user is in currently, along with the stream - ordering of the most recent join for that user and room. + ordering of the most recent join for that user and room, along with + the room version of the room. """ return await self.db_pool.runInteraction( "get_rooms_for_user_with_stream_ordering", diff --git a/synapse/storage/roommember.py b/synapse/storage/roommember.py index 0ff66debdfcc..dd45b0238289 100644 --- a/synapse/storage/roommember.py +++ b/synapse/storage/roommember.py @@ -30,6 +30,7 @@ class RoomsForUser: membership: str event_id: str stream_ordering: int + room_version_id: str @attr.s(slots=True, frozen=True, weakref_slot=True, auto_attribs=True) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 84f05f6c584c..42ff3b2ee6ca 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -15,6 +15,9 @@ from synapse.api.errors import Codes, ResourceLimitError from synapse.api.filtering import DEFAULT_FILTER_COLLECTION from synapse.handlers.sync import SyncConfig +from synapse.rest import admin +from synapse.rest.client import login, room +from synapse.server import HomeServer from synapse.types import UserID, create_requester import tests.unittest @@ -24,8 +27,13 @@ class SyncTestCase(tests.unittest.HomeserverTestCase): """Tests Sync Handler.""" - def prepare(self, reactor, clock, hs): - self.hs = hs + servlets = [ + admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor, clock, hs: HomeServer): self.sync_handler = self.hs.get_sync_handler() self.store = self.hs.get_datastore() @@ -68,12 +76,63 @@ def test_wait_for_sync_for_user_auth_blocking(self): ) self.assertEquals(e.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + def test_unknown_room_version(self): + """ + A room with an unknown room version should not break sync (and should be excluded). + """ + user = self.register_user("user", "pass") + tok = self.login("user", "pass") + + # Create a room as the user. + joined_room = self.helper.create_room_as(user, tok=tok) + + # Both rooms should appear in the sync response. + requester = create_requester(user) + result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, sync_config=generate_sync_config(user) + ) + ) + self.assertIn(joined_room, [r.room_id for r in result.joined]) + + # Poke the database and update the room version to an unknown one. + self.get_success( + self.hs.get_datastores().main.db_pool.simple_update( + "rooms", + keyvalues={"room_id": joined_room}, + updatevalues={"room_version": "unknown-room-version"}, + desc="updated-room-version", + ) + ) + + # Blow away the cache. + self.get_success( + self.store.get_rooms_for_user_with_stream_ordering.invalidate_all() + ) + + # The room should be excluded from the sync response. + # Get a new request key. + result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, sync_config=generate_sync_config(user) + ) + ) + self.assertNotIn(joined_room, [r.room_id for r in result.joined]) + + # TODO Test a partial sync (by providing a since_token). + + +_request_key = 0 + def generate_sync_config(user_id: str) -> SyncConfig: + """Generate a sync config (with a unique request key).""" + global _request_key + _request_key += 1 return SyncConfig( - user=UserID(user_id.split(":")[0][1:], user_id.split(":")[1]), + user=UserID.from_string(user_id), filter_collection=DEFAULT_FILTER_COLLECTION, is_guest=False, - request_key="request_key", + request_key=("request_key", _request_key), device_id="device_id", ) diff --git a/tests/replication/slave/storage/test_events.py b/tests/replication/slave/storage/test_events.py index 8d1b0606c469..b25a06b4271a 100644 --- a/tests/replication/slave/storage/test_events.py +++ b/tests/replication/slave/storage/test_events.py @@ -150,6 +150,7 @@ def test_invites(self): "invite", event.event_id, event.internal_metadata.stream_ordering, + RoomVersions.V1.identifier, ) ], ) From bc67d443c2d2e0a42f8e8d978de143af1fec9a94 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 Aug 2021 08:58:28 -0400 Subject: [PATCH 2/5] Test an invite to a room of an unknown room version. --- tests/handlers/test_sync.py | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 42ff3b2ee6ca..ea8bbc60c659 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -80,13 +80,20 @@ def test_unknown_room_version(self): """ A room with an unknown room version should not break sync (and should be excluded). """ + inviter = self.register_user("creator", "pass", admin=True) + inviter_tok = self.login("@creator:test", "pass") + user = self.register_user("user", "pass") tok = self.login("user", "pass") # Create a room as the user. joined_room = self.helper.create_room_as(user, tok=tok) - # Both rooms should appear in the sync response. + # Invite the user to the room as someone else. + invite_room = self.helper.create_room_as(inviter, tok=inviter_tok) + self.helper.invite(invite_room, targ=user, tok=inviter_tok) + + # The rooms should appear in the sync response. requester = create_requester(user) result = self.get_success( self.sync_handler.wait_for_sync_for_user( @@ -94,23 +101,25 @@ def test_unknown_room_version(self): ) ) self.assertIn(joined_room, [r.room_id for r in result.joined]) + self.assertIn(invite_room, [r.room_id for r in result.invited]) # Poke the database and update the room version to an unknown one. - self.get_success( - self.hs.get_datastores().main.db_pool.simple_update( - "rooms", - keyvalues={"room_id": joined_room}, - updatevalues={"room_version": "unknown-room-version"}, - desc="updated-room-version", + for room_id in (joined_room, invite_room): + self.get_success( + self.hs.get_datastores().main.db_pool.simple_update( + "rooms", + keyvalues={"room_id": room_id}, + updatevalues={"room_version": "unknown-room-version"}, + desc="updated-room-version", + ) ) - ) # Blow away the cache. self.get_success( self.store.get_rooms_for_user_with_stream_ordering.invalidate_all() ) - # The room should be excluded from the sync response. + # The rooms should be excluded from the sync response. # Get a new request key. result = self.get_success( self.sync_handler.wait_for_sync_for_user( @@ -118,6 +127,7 @@ def test_unknown_room_version(self): ) ) self.assertNotIn(joined_room, [r.room_id for r in result.joined]) + self.assertNotIn(invite_room, [r.room_id for r in result.invited]) # TODO Test a partial sync (by providing a since_token). From 4a0ae60b974f7e26df85fac26f953d2c1eb72fe1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 Aug 2021 09:19:54 -0400 Subject: [PATCH 3/5] Test knocking against a room with an unknown room version. --- tests/handlers/test_sync.py | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index ea8bbc60c659..b17b27347d61 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -12,11 +12,13 @@ # See the License for the specific language governing permissions and # limitations under the License. +from synapse.api.constants import EventTypes, JoinRules from synapse.api.errors import Codes, ResourceLimitError from synapse.api.filtering import DEFAULT_FILTER_COLLECTION +from synapse.api.room_versions import RoomVersions from synapse.handlers.sync import SyncConfig from synapse.rest import admin -from synapse.rest.client import login, room +from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.types import UserID, create_requester @@ -29,6 +31,7 @@ class SyncTestCase(tests.unittest.HomeserverTestCase): servlets = [ admin.register_servlets, + knock.register_servlets, login.register_servlets, room.register_servlets, ] @@ -93,6 +96,23 @@ def test_unknown_room_version(self): invite_room = self.helper.create_room_as(inviter, tok=inviter_tok) self.helper.invite(invite_room, targ=user, tok=inviter_tok) + knock_room = self.helper.create_room_as( + inviter, room_version=RoomVersions.V7.identifier, tok=inviter_tok + ) + self.helper.send_state( + knock_room, + EventTypes.JoinRules, + {"join_rule": JoinRules.KNOCK}, + tok=inviter_tok, + ) + channel = self.make_request( + "POST", + "/_matrix/client/r0/knock/%s" % (knock_room,), + b"{}", + tok, + ) + self.assertEquals(200, channel.code, channel.result) + # The rooms should appear in the sync response. requester = create_requester(user) result = self.get_success( @@ -102,9 +122,10 @@ def test_unknown_room_version(self): ) self.assertIn(joined_room, [r.room_id for r in result.joined]) self.assertIn(invite_room, [r.room_id for r in result.invited]) + self.assertIn(knock_room, [r.room_id for r in result.knocked]) # Poke the database and update the room version to an unknown one. - for room_id in (joined_room, invite_room): + for room_id in (joined_room, invite_room, knock_room): self.get_success( self.hs.get_datastores().main.db_pool.simple_update( "rooms", @@ -128,6 +149,7 @@ def test_unknown_room_version(self): ) self.assertNotIn(joined_room, [r.room_id for r in result.joined]) self.assertNotIn(invite_room, [r.room_id for r in result.invited]) + self.assertNotIn(knock_room, [r.room_id for r in result.knocked]) # TODO Test a partial sync (by providing a since_token). From bf4df79141b7b2234325adb1317f662ee24ab438 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 Aug 2021 09:54:13 -0400 Subject: [PATCH 4/5] Test an incremental sync. --- tests/handlers/test_sync.py | 44 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index b17b27347d61..339c039914f1 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +from typing import Optional + from synapse.api.constants import EventTypes, JoinRules from synapse.api.errors import Codes, ResourceLimitError from synapse.api.filtering import DEFAULT_FILTER_COLLECTION @@ -89,6 +91,14 @@ def test_unknown_room_version(self): user = self.register_user("user", "pass") tok = self.login("user", "pass") + # Do an initial sync on a different device. + requester = create_requester(user) + initial_result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, sync_config=generate_sync_config(user, device_id="dev") + ) + ) + # Create a room as the user. joined_room = self.helper.create_room_as(user, tok=tok) @@ -114,7 +124,6 @@ def test_unknown_room_version(self): self.assertEquals(200, channel.code, channel.result) # The rooms should appear in the sync response. - requester = create_requester(user) result = self.get_success( self.sync_handler.wait_for_sync_for_user( requester, sync_config=generate_sync_config(user) @@ -124,6 +133,18 @@ def test_unknown_room_version(self): self.assertIn(invite_room, [r.room_id for r in result.invited]) self.assertIn(knock_room, [r.room_id for r in result.knocked]) + # Test a incremental sync (by providing a since_token). + result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + sync_config=generate_sync_config(user, device_id="dev"), + since_token=initial_result.next_batch, + ) + ) + self.assertIn(joined_room, [r.room_id for r in result.joined]) + self.assertIn(invite_room, [r.room_id for r in result.invited]) + self.assertIn(knock_room, [r.room_id for r in result.knocked]) + # Poke the database and update the room version to an unknown one. for room_id in (joined_room, invite_room, knock_room): self.get_success( @@ -135,10 +156,11 @@ def test_unknown_room_version(self): ) ) - # Blow away the cache. + # Blow away caches (supported room versions can only change due to a restart). self.get_success( self.store.get_rooms_for_user_with_stream_ordering.invalidate_all() ) + self.store._get_event_cache.clear() # The rooms should be excluded from the sync response. # Get a new request key. @@ -151,13 +173,25 @@ 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]) - # TODO Test a partial sync (by providing a since_token). + # The rooms should also not be in an incremental sync. + result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + sync_config=generate_sync_config(user, device_id="dev"), + since_token=initial_result.next_batch, + ) + ) + self.assertNotIn(joined_room, [r.room_id for r in result.joined]) + self.assertNotIn(invite_room, [r.room_id for r in result.invited]) + self.assertNotIn(knock_room, [r.room_id for r in result.knocked]) _request_key = 0 -def generate_sync_config(user_id: str) -> SyncConfig: +def generate_sync_config( + user_id: str, device_id: Optional[str] = "device_id" +) -> SyncConfig: """Generate a sync config (with a unique request key).""" global _request_key _request_key += 1 @@ -166,5 +200,5 @@ def generate_sync_config(user_id: str) -> SyncConfig: filter_collection=DEFAULT_FILTER_COLLECTION, is_guest=False, request_key=("request_key", _request_key), - device_id="device_id", + device_id=device_id, ) From ce6b9d190c1938e2fbfea8f55fa9f25375a6c4cf Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 Aug 2021 10:56:44 -0400 Subject: [PATCH 5/5] Newsfragment --- changelog.d/10644.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10644.bugfix diff --git a/changelog.d/10644.bugfix b/changelog.d/10644.bugfix new file mode 100644 index 000000000000..d88a81fd82d1 --- /dev/null +++ b/changelog.d/10644.bugfix @@ -0,0 +1 @@ +Rooms with unsupported room versions are no longer returned via `/sync`.