diff options
| -rw-r--r-- | CHANGES.md | 18 | ||||
| -rw-r--r-- | debian/changelog | 6 | ||||
| -rw-r--r-- | pyproject.toml | 2 | ||||
| -rw-r--r-- | synapse/events/third_party_rules.py | 5 | ||||
| -rw-r--r-- | synapse/federation/sender/per_destination_queue.py | 11 | ||||
| -rw-r--r-- | synapse/handlers/federation.py | 29 | ||||
| -rw-r--r-- | synapse/visibility.py | 67 | ||||
| -rw-r--r-- | tests/federation/test_federation_catch_up.py | 87 | ||||
| -rw-r--r-- | tests/rest/client/test_third_party_rules.py | 19 | ||||
| -rw-r--r-- | tests/test_utils/event_injection.py | 31 | ||||
| -rw-r--r-- | tests/test_visibility.py | 40 |
11 files changed, 271 insertions, 44 deletions
diff --git a/CHANGES.md b/CHANGES.md index 212ebe2f36..43259f323c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,19 @@ +Synapse 1.79.0rc2 (2023-03-13) +============================== + +Bugfixes +-------- + +- Fix a bug introduced in Synapse 1.79.0rc1 where attempting to register a `on_remove_user_third_party_identifier` module API callback would be a no-op. ([\#15227](https://github.com/matrix-org/synapse/issues/15227)) +- Fix a rare bug introduced in Synapse 1.73 where events could remain unsent to other homeservers after a faster-join to a room. ([\#15248](https://github.com/matrix-org/synapse/issues/15248)) + + +Internal Changes +---------------- + +- Refactor `filter_events_for_server`. ([\#15240](https://github.com/matrix-org/synapse/issues/15240)) + + Synapse 1.79.0rc1 (2023-03-07) ============================== @@ -47,7 +63,7 @@ Improved Documentation Deprecations and Removals ------------------------- -- Deprecate the `on_threepid_bind` module callback, to be replaced by [`on_add_user_third_party_identifier`](https://matrix-org.github.io/synapse/v1.79/modules/third_party_rules_callbacks.html#on_add_user_third_party_identifier). See [upgrade notes](https://github.com/matrix-org/synapse/blob/release-v1.79/docs/upgrade.md#upgrading-to-v1790). ([\#15044] +- Deprecate the `on_threepid_bind` module callback, to be replaced by [`on_add_user_third_party_identifier`](https://matrix-org.github.io/synapse/v1.79/modules/third_party_rules_callbacks.html#on_add_user_third_party_identifier). See [upgrade notes](https://github.com/matrix-org/synapse/blob/release-v1.79/docs/upgrade.md#upgrading-to-v1790). ([\#15044](https://github.com/matrix-org/synapse/issues/15044)) - Remove the unspecced `room_alias` field from the [`/createRoom`](https://spec.matrix.org/v1.6/client-server-api/#post_matrixclientv3createroom) response. ([\#15093](https://github.com/matrix-org/synapse/issues/15093)) - Remove the unspecced `PUT` on the `/knock/{roomIdOrAlias}` endpoint. ([\#15189](https://github.com/matrix-org/synapse/issues/15189)) - Remove the undocumented and unspecced `type` parameter to the `/thumbnail` endpoint. ([\#15137](https://github.com/matrix-org/synapse/issues/15137)) diff --git a/debian/changelog b/debian/changelog index 871c695f07..a91521f6b2 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +matrix-synapse-py3 (1.79.0~rc2) stable; urgency=medium + + * New Synapse release 1.79.0rc2. + + -- Synapse Packaging team <packages@matrix.org> Mon, 13 Mar 2023 12:54:21 +0000 + matrix-synapse-py3 (1.79.0~rc1) stable; urgency=medium * New Synapse release 1.79.0rc1. diff --git a/pyproject.toml b/pyproject.toml index 074ac2c11e..067203ed75 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -89,7 +89,7 @@ manifest-path = "rust/Cargo.toml" [tool.poetry] name = "matrix-synapse" -version = "1.79.0rc1" +version = "1.79.0rc2" description = "Homeserver for the Matrix decentralised comms protocol" authors = ["Matrix.org Team and Contributors <packages@matrix.org>"] license = "Apache-2.0" diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 3e4d52c8d8..61d4530be7 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -247,6 +247,11 @@ class ThirdPartyEventRules: on_add_user_third_party_identifier ) + if on_remove_user_third_party_identifier is not None: + self._on_remove_user_third_party_identifier_callbacks.append( + on_remove_user_third_party_identifier + ) + async def check_event_allowed( self, event: EventBase, diff --git a/synapse/federation/sender/per_destination_queue.py b/synapse/federation/sender/per_destination_queue.py index ffc9d95ee7..31c5c2b7de 100644 --- a/synapse/federation/sender/per_destination_queue.py +++ b/synapse/federation/sender/per_destination_queue.py @@ -497,8 +497,8 @@ class PerDestinationQueue: # # Note: `catchup_pdus` will have exactly one PDU per room. for pdu in catchup_pdus: - # The PDU from the DB will be the last PDU in the room from - # *this server* that wasn't sent to the remote. However, other + # The PDU from the DB will be the newest PDU in the room from + # *this server* that we tried---but were unable---to send to the remote. # servers may have sent lots of events since then, and we want # to try and tell the remote only about the *latest* events in # the room. This is so that it doesn't get inundated by events @@ -516,6 +516,11 @@ class PerDestinationQueue: # If the event is in the extremities, then great! We can just # use that without having to do further checks. room_catchup_pdus = [pdu] + elif await self._store.is_partial_state_room(pdu.room_id): + # We can't be sure which events the destination should + # see using only partial state. Avoid doing so, and just retry + # sending our the newest PDU the remote is missing from us. + room_catchup_pdus = [pdu] else: # If not, fetch the extremities and figure out which we can # send. @@ -547,6 +552,8 @@ class PerDestinationQueue: self._server_name, new_pdus, redact=False, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) # If we've filtered out all the extremities, fall back to diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 5f2057269d..80156ef343 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -392,7 +392,7 @@ class FederationHandler: get_prev_content=False, ) - # We set `check_history_visibility_only` as we might otherwise get false + # We unset `filter_out_erased_senders` as we might otherwise get false # positives from users having been erased. filtered_extremities = await filter_events_for_server( self._storage_controllers, @@ -400,7 +400,8 @@ class FederationHandler: self.server_name, events_to_check, redact=False, - check_history_visibility_only=True, + filter_out_erased_senders=False, + filter_out_remote_partial_state_events=False, ) if filtered_extremities: extremities_to_request.append(bp.event_id) @@ -1331,7 +1332,13 @@ class FederationHandler: ) events = await filter_events_for_server( - self._storage_controllers, origin, self.server_name, events + self._storage_controllers, + origin, + self.server_name, + events, + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) return events @@ -1362,7 +1369,13 @@ class FederationHandler: await self._event_auth_handler.assert_host_in_room(event.room_id, origin) events = await filter_events_for_server( - self._storage_controllers, origin, self.server_name, [event] + self._storage_controllers, + origin, + self.server_name, + [event], + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) event = events[0] return event @@ -1390,7 +1403,13 @@ class FederationHandler: ) missing_events = await filter_events_for_server( - self._storage_controllers, origin, self.server_name, missing_events + self._storage_controllers, + origin, + self.server_name, + missing_events, + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) return missing_events diff --git a/synapse/visibility.py b/synapse/visibility.py index e442de3173..468e22f8f6 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -14,7 +14,17 @@ # limitations under the License. import logging from enum import Enum, auto -from typing import Collection, Dict, FrozenSet, List, Optional, Tuple +from typing import ( + Collection, + Dict, + FrozenSet, + List, + Mapping, + Optional, + Sequence, + Set, + Tuple, +) import attr from typing_extensions import Final @@ -565,29 +575,43 @@ async def filter_events_for_server( storage: StorageControllers, target_server_name: str, local_server_name: str, - events: List[EventBase], - redact: bool = True, - check_history_visibility_only: bool = False, + events: Sequence[EventBase], + *, + redact: bool, + filter_out_erased_senders: bool, + filter_out_remote_partial_state_events: bool, ) -> List[EventBase]: - """Filter a list of events based on whether given server is allowed to + """Filter a list of events based on whether the target server is allowed to see them. + For a fully stated room, the target server is allowed to see an event E if: + - the state at E has world readable or shared history vis, OR + - the state at E says that the target server is in the room. + + For a partially stated room, the target server is allowed to see E if: + - E was created by this homeserver, AND: + - the partial state at E has world readable or shared history vis, OR + - the partial state at E says that the target server is in the room. + + TODO: state before or state after? + Args: storage - server_name + target_server_name + local_server_name events - redact: Whether to return a redacted version of the event, or - to filter them out entirely. - check_history_visibility_only: Whether to only check the - history visibility, rather than things like if the sender has been + redact: Controls what to do with events which have been filtered out. + If True, include their redacted forms; if False, omit them entirely. + filter_out_erased_senders: If true, also filter out events whose sender has been erased. This is used e.g. during pagination to decide whether to backfill or not. - + filter_out_remote_partial_state_events: If True, also filter out events in + partial state rooms created by other homeservers. Returns The filtered events. """ - def is_sender_erased(event: EventBase, erased_senders: Dict[str, bool]) -> bool: + def is_sender_erased(event: EventBase, erased_senders: Mapping[str, bool]) -> bool: if erased_senders and erased_senders[event.sender]: logger.info("Sender of %s has been erased, redacting", event.event_id) return True @@ -616,7 +640,7 @@ async def filter_events_for_server( # server has no users in the room: redact return False - if not check_history_visibility_only: + if filter_out_erased_senders: erased_senders = await storage.main.are_users_erased(e.sender for e in events) else: # We don't want to check whether users are erased, which is equivalent @@ -631,15 +655,15 @@ async def filter_events_for_server( # otherwise a room could be fully joined after we retrieve those, which would then bypass # this check but would base the filtering on an outdated view of the membership events. - partial_state_invisible_events = set() - if not check_history_visibility_only: + partial_state_invisible_event_ids: Set[str] = set() + if filter_out_remote_partial_state_events: for e in events: sender_domain = get_domain_from_id(e.sender) if ( sender_domain != local_server_name and await storage.main.is_partial_state_room(e.room_id) ): - partial_state_invisible_events.add(e) + partial_state_invisible_event_ids.add(e.event_id) # Let's check to see if all the events have a history visibility # of "shared" or "world_readable". If that's the case then we don't @@ -658,17 +682,20 @@ async def filter_events_for_server( target_server_name, ) - to_return = [] - for e in events: + def include_event_in_output(e: EventBase) -> bool: erased = is_sender_erased(e, erased_senders) visible = check_event_is_visible( event_to_history_vis[e.event_id], event_to_memberships.get(e.event_id, {}) ) - if e in partial_state_invisible_events: + if e.event_id in partial_state_invisible_event_ids: visible = False - if visible and not erased: + return visible and not erased + + to_return = [] + for e in events: + if include_event_in_output(e): to_return.append(e) elif redact: to_return.append(prune_event(e)) diff --git a/tests/federation/test_federation_catch_up.py b/tests/federation/test_federation_catch_up.py index 6381583c24..391ae51707 100644 --- a/tests/federation/test_federation_catch_up.py +++ b/tests/federation/test_federation_catch_up.py @@ -1,4 +1,5 @@ -from typing import Callable, List, Optional, Tuple +from typing import Callable, Collection, List, Optional, Tuple +from unittest import mock from unittest.mock import Mock from twisted.test.proto_helpers import MemoryReactor @@ -500,3 +501,87 @@ class FederationCatchUpTestCases(FederatingHomeserverTestCase): self.assertEqual(len(sent_pdus), 1) self.assertEqual(sent_pdus[0].event_id, event_2.event_id) self.assertFalse(per_dest_queue._catching_up) + + def test_catch_up_is_not_blocked_by_remote_event_in_partial_state_room( + self, + ) -> None: + """Detects (part of?) https://github.com/matrix-org/synapse/issues/15220.""" + # ARRANGE: + # - a local user (u1) + # - a room which contains u1 and two remote users, @u2:host2 and @u3:other + # - events in that room such that + # - history visibility is restricted + # - u1 sent message events e1 and e2 + # - afterwards, u3 sent a remote event e3 + # - catchup to begin for host2; last successfully sent event was e1 + per_dest_queue, sent_pdus = self.make_fake_destination_queue() + + self.register_user("u1", "you the one") + u1_token = self.login("u1", "you the one") + room = self.helper.create_room_as("u1", tok=u1_token) + self.helper.send_state( + room_id=room, + event_type="m.room.history_visibility", + body={"history_visibility": "joined"}, + tok=u1_token, + ) + self.get_success( + event_injection.inject_member_event(self.hs, room, "@u2:host2", "join") + ) + self.get_success( + event_injection.inject_member_event(self.hs, room, "@u3:other", "join") + ) + + # create some events + event_id_1 = self.helper.send(room, "hello", tok=u1_token)["event_id"] + event_id_2 = self.helper.send(room, "world", tok=u1_token)["event_id"] + # pretend that u3 changes their displayname + event_id_3 = self.get_success( + event_injection.inject_member_event(self.hs, room, "@u3:other", "join") + ).event_id + + # destination_rooms should already be populated, but let us pretend that we already + # sent (successfully) up to and including event id 1 + event_1 = self.get_success(self.hs.get_datastores().main.get_event(event_id_1)) + assert event_1.internal_metadata.stream_ordering is not None + self.get_success( + self.hs.get_datastores().main.set_destination_last_successful_stream_ordering( + "host2", event_1.internal_metadata.stream_ordering + ) + ) + + # also fetch event 2 so we can compare its stream ordering to the sender's + # last_successful_stream_ordering later + event_2 = self.get_success(self.hs.get_datastores().main.get_event(event_id_2)) + + # Mock event 3 as having partial state + self.get_success( + event_injection.mark_event_as_partial_state(self.hs, event_id_3, room) + ) + + # Fail the test if we block on full state for event 3. + async def mock_await_full_state(event_ids: Collection[str]) -> None: + if event_id_3 in event_ids: + raise AssertionError("Tried to await full state for event_id_3") + + # ACT + with mock.patch.object( + self.hs.get_storage_controllers().state._partial_state_events_tracker, + "await_full_state", + mock_await_full_state, + ): + self.get_success(per_dest_queue._catch_up_transmission_loop()) + + # ASSERT + # We should have: + # - not sent event 3: it's not ours, and the room is partial stated + # - fallen back to sending event 2: it's the most recent event in the room + # we tried to send to host2 + # - completed catch-up + self.assertEqual(len(sent_pdus), 1) + self.assertEqual(sent_pdus[0].event_id, event_id_2) + self.assertFalse(per_dest_queue._catching_up) + self.assertEqual( + per_dest_queue._last_successful_stream_ordering, + event_2.internal_metadata.stream_ordering, + ) diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 3b99513707..753ecc8d16 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -941,18 +941,16 @@ class ThirdPartyRulesTestCase(unittest.FederatingHomeserverTestCase): just before associating and removing a 3PID to/from an account. """ # Pretend to be a Synapse module and register both callbacks as mocks. - third_party_rules = self.hs.get_third_party_event_rules() on_add_user_third_party_identifier_callback_mock = Mock( return_value=make_awaitable(None) ) on_remove_user_third_party_identifier_callback_mock = Mock( return_value=make_awaitable(None) ) - third_party_rules._on_threepid_bind_callbacks.append( - on_add_user_third_party_identifier_callback_mock - ) - third_party_rules._on_threepid_bind_callbacks.append( - on_remove_user_third_party_identifier_callback_mock + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules.register_third_party_rules_callbacks( + on_add_user_third_party_identifier=on_add_user_third_party_identifier_callback_mock, + on_remove_user_third_party_identifier=on_remove_user_third_party_identifier_callback_mock, ) # Register an admin user. @@ -1008,12 +1006,12 @@ class ThirdPartyRulesTestCase(unittest.FederatingHomeserverTestCase): when a user is deactivated and their third-party ID associations are deleted. """ # Pretend to be a Synapse module and register both callbacks as mocks. - third_party_rules = self.hs.get_third_party_event_rules() on_remove_user_third_party_identifier_callback_mock = Mock( return_value=make_awaitable(None) ) - third_party_rules._on_threepid_bind_callbacks.append( - on_remove_user_third_party_identifier_callback_mock + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules.register_third_party_rules_callbacks( + on_remove_user_third_party_identifier=on_remove_user_third_party_identifier_callback_mock, ) # Register an admin user. @@ -1039,6 +1037,9 @@ class ThirdPartyRulesTestCase(unittest.FederatingHomeserverTestCase): ) self.assertEqual(channel.code, 200, channel.json_body) + # Check that the mock was not called on the act of adding a third-party ID. + on_remove_user_third_party_identifier_callback_mock.assert_not_called() + # Now deactivate the user. channel = self.make_request( "PUT", diff --git a/tests/test_utils/event_injection.py b/tests/test_utils/event_injection.py index a6330ed840..9679904c33 100644 --- a/tests/test_utils/event_injection.py +++ b/tests/test_utils/event_injection.py @@ -102,3 +102,34 @@ async def create_event( context = await unpersisted_context.persist(event) return event, context + + +async def mark_event_as_partial_state( + hs: synapse.server.HomeServer, + event_id: str, + room_id: str, +) -> None: + """ + (Falsely) mark an event as having partial state. + + Naughty, but occasionally useful when checking that partial state doesn't + block something from happening. + + If the event already has partial state, this insert will fail (event_id is unique + in this table). + """ + store = hs.get_datastores().main + await store.db_pool.simple_upsert( + table="partial_state_rooms", + keyvalues={"room_id": room_id}, + values={}, + insertion_values={"room_id": room_id}, + ) + + await store.db_pool.simple_insert( + table="partial_state_events", + values={ + "room_id": room_id, + "event_id": event_id, + }, + ) diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 2801a950a8..9ed330f554 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -63,7 +63,13 @@ class FilterEventsForServerTestCase(unittest.HomeserverTestCase): filtered = self.get_success( filter_events_for_server( - self._storage_controllers, "test_server", "hs", events_to_filter + self._storage_controllers, + "test_server", + "hs", + events_to_filter, + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) ) @@ -85,7 +91,13 @@ class FilterEventsForServerTestCase(unittest.HomeserverTestCase): self.assertEqual( self.get_success( filter_events_for_server( - self._storage_controllers, "remote_hs", "hs", [outlier] + self._storage_controllers, + "remote_hs", + "hs", + [outlier], + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) ), [outlier], @@ -96,7 +108,13 @@ class FilterEventsForServerTestCase(unittest.HomeserverTestCase): filtered = self.get_success( filter_events_for_server( - self._storage_controllers, "remote_hs", "local_hs", [outlier, evt] + self._storage_controllers, + "remote_hs", + "local_hs", + [outlier, evt], + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) ) self.assertEqual(len(filtered), 2, f"expected 2 results, got: {filtered}") @@ -108,7 +126,13 @@ class FilterEventsForServerTestCase(unittest.HomeserverTestCase): # be redacted) filtered = self.get_success( filter_events_for_server( - self._storage_controllers, "other_server", "local_hs", [outlier, evt] + self._storage_controllers, + "other_server", + "local_hs", + [outlier, evt], + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) ) self.assertEqual(filtered[0], outlier) @@ -143,7 +167,13 @@ class FilterEventsForServerTestCase(unittest.HomeserverTestCase): # ... and the filtering happens. filtered = self.get_success( filter_events_for_server( - self._storage_controllers, "test_server", "local_hs", events_to_filter + self._storage_controllers, + "test_server", + "local_hs", + events_to_filter, + redact=True, + filter_out_erased_senders=True, + filter_out_remote_partial_state_events=True, ) ) |