Support MSC4446: allow moving fully read markers backwards (#19663)

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>

changelog.d/19663.feature

@@ -0,0 +1 @@
+Support [MSC4446](https://github.com/matrix-org/matrix-spec-proposals/pull/4446) for moving fully read markers backwards. Contributed by @SpiritCroc @ Beeper.

rust/src/config/mod.rs

@@ -72,4 +72,5 @@ pub struct ExperimentalConfig {
     pub msc4222_enabled: bool,
     pub msc4491_enabled: bool,
     pub msc4143_enabled: bool,
+    pub msc4446_enabled: bool,
 }

rust/src/handlers/versions.rs

@@ -266,6 +266,9 @@ pub struct UnstableFeatureMap {
     /// MSC4143: Matrix RTC transports (LiveKit backend)
     #[serde(rename = "org.matrix.msc4143")]
     msc4143_enabled: bool,
+    /// MSC4446: Allow moving the fully read marker backwards.
+    #[serde(rename = "com.beeper.msc4446")]
+    msc4446_enabled: bool,
 
     // Whether new rooms will be set to encrypted or not (based on presets).
     #[serde(rename = "io.element.e2ee_forced.public")]
@@ -316,6 +319,7 @@ pub fn synapse_config_to_global_unstable_feature_map(
         msc4445_initial_sync_timeline_topological_ordering: true,
         msc4491_enabled: config.experimental.msc4491_enabled,
         msc4143_enabled: config.experimental.msc4143_enabled,
+        msc4446_enabled: config.experimental.msc4446_enabled,
         e2ee_forced_public: config
             .room
             .encryption_enabled_by_default_for_room_presets

synapse/config/experimental.py

@@ -287,6 +287,10 @@ class ExperimentalConfig(Config):
         # (and MSC4308: Thread Subscriptions extension to Sliding Sync)
         self.msc4306_enabled: bool = experimental.get("msc4306_enabled", False)
 
+        # MSC4446: Allow moving the fully read marker backwards.
+        # Tracked in: https://github.com/element-hq/synapse/issues/19940
+        self.msc4446_enabled: bool = experimental.get("msc4446_enabled", False)
+
         # MSC4354: Sticky Events
         # Tracked in: https://github.com/element-hq/synapse/issues/19409
         # Note that sticky events persisted before this feature is enabled will not be

synapse/handlers/read_marker.py

@@ -41,7 +41,11 @@ class ReadMarkerHandler:
         )
 
     async def received_client_read_marker(
-        self, room_id: str, user_id: str, event_id: str
+        self,
+        room_id: str,
+        user_id: str,
+        event_id: str,
+        allow_backward: bool = False,
     ) -> None:
         """Updates the read marker for a given user in a given room if the event ID given
         is ahead in the stream relative to the current read marker.
@@ -59,7 +63,7 @@ class ReadMarkerHandler:
             # Get event ordering, this also ensures we know about the event
             event_ordering = await self.store.get_event_ordering(event_id, room_id)
 
-            if existing_read_marker:
+            if existing_read_marker and not allow_backward:
                 try:
                     old_event_ordering = await self.store.get_event_ordering(
                         existing_read_marker["event_id"], room_id

synapse/rest/client/read_marker.py

@@ -23,6 +23,7 @@ import logging
 from typing import TYPE_CHECKING
 
 from synapse.api.constants import ReceiptTypes
+from synapse.api.errors import Codes, SynapseError
 from synapse.http.server import HttpServer
 from synapse.http.servlet import RestServlet, parse_json_object_from_request
 from synapse.http.site import SynapseRequest
@@ -66,6 +67,21 @@ class ReadMarkerRestServlet(RestServlet):
         body = parse_json_object_from_request(request)
 
         unrecognized_types = set(body.keys()) - self._known_receipt_types
+
+        if self.config.experimental.msc4446_enabled:
+            allow_backward = body.get("com.beeper.allow_backward", False)
+            if not isinstance(allow_backward, bool):
+                raise SynapseError(
+                    400,
+                    "com.beeper.allow_backward must be a boolean.",
+                    Codes.INVALID_PARAM,
+                )
+
+            # Prevent considering the `allow_backward` field as a receipt type.
+            unrecognized_types -= {"com.beeper.allow_backward"}
+        else:
+            allow_backward = False
+
         if unrecognized_types:
             # It's fine if there are unrecognized receipt types, but let's log
             # it to help debug clients that have typoed the receipt type.
@@ -86,6 +102,7 @@ class ReadMarkerRestServlet(RestServlet):
                     room_id,
                     user_id=requester.user.to_string(),
                     event_id=event_id,
+                    allow_backward=allow_backward,
                 )
             else:
                 await self.receipts_handler.received_client_receipt(

synapse/rest/client/receipts.py

@@ -20,6 +20,7 @@
 #
 
 import logging
+from http import HTTPStatus
 from typing import TYPE_CHECKING
 
 from synapse.api.constants import MAIN_TIMELINE, ReceiptTypes
@@ -50,6 +51,7 @@ class ReceiptRestServlet(RestServlet):
         self.read_marker_handler = hs.get_read_marker_handler()
         self.presence_handler = hs.get_presence_handler()
         self._main_store = hs.get_datastores().main
+        self._msc4446_enabled = hs.config.experimental.msc4446_enabled
 
         self._known_receipt_types = {
             ReceiptTypes.READ,
@@ -73,6 +75,25 @@ class ReceiptRestServlet(RestServlet):
 
         body = parse_json_object_from_request(request)
 
+        if self._msc4446_enabled:
+            allow_backward = body.get("com.beeper.allow_backward", False)
+            if not isinstance(allow_backward, bool):
+                raise SynapseError(
+                    HTTPStatus.BAD_REQUEST,
+                    "com.beeper.allow_backward must be a boolean.",
+                    Codes.INVALID_PARAM,
+                )
+
+            if allow_backward and receipt_type != ReceiptTypes.FULLY_READ:
+                raise SynapseError(
+                    HTTPStatus.BAD_REQUEST,
+                    "com.beeper.allow_backward is only allowed to be true for "
+                    f"{ReceiptTypes.FULLY_READ}.",
+                    Codes.INVALID_PARAM,
+                )
+        else:
+            allow_backward = False
+
         # Pull the thread ID, if one exists.
         thread_id = None
         if "thread_id" in body:
@@ -108,6 +129,7 @@ class ReceiptRestServlet(RestServlet):
                 room_id,
                 user_id=requester.user.to_string(),
                 event_id=event_id,
+                allow_backward=allow_backward,
             )
         else:
             await self.receipts_handler.received_client_receipt(

tests/rest/client/test_read_marker.py

@@ -66,6 +66,19 @@ class ReadMarkerTestCase(unittest.HomeserverTestCase):
         self.store = self.hs.get_datastores().main
         self.clock = self.hs.get_clock()
 
+    def _get_fully_read_marker(self, room_id: str) -> str | None:
+        content = self.get_success(
+            self.store.get_account_data_for_room_and_type(
+                self.owner,
+                room_id,
+                "m.fully_read",
+            )
+        )
+        if content is None:
+            return None
+
+        return content.get("event_id")
+
     def test_send_read_marker(self) -> None:
         room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok)
 
@@ -98,6 +111,123 @@ class ReadMarkerTestCase(unittest.HomeserverTestCase):
         )
         self.assertEqual(channel.code, 200, channel.result)
 
+    def test_send_read_marker_does_not_move_backwards_by_default(self) -> None:
+        room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok)
+
+        older_event_id = self.helper.send(
+            room_id=room_id, body="1", tok=self.owner_tok
+        )["event_id"]
+        newer_event_id = self.helper.send(
+            room_id=room_id, body="2", tok=self.owner_tok
+        )["event_id"]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={"m.fully_read": newer_event_id},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(room_id), newer_event_id)
+
+        # Expected to be a no-op.
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={"m.fully_read": older_event_id},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(room_id), newer_event_id)
+
+    @unittest.override_config({"experimental_features": {"msc4446_enabled": True}})
+    def test_send_read_marker_can_move_backwards_with_opt_in(self) -> None:
+        room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok)
+
+        older_event_id = self.helper.send(
+            room_id=room_id, body="1", tok=self.owner_tok
+        )["event_id"]
+        newer_event_id = self.helper.send(
+            room_id=room_id, body="2", tok=self.owner_tok
+        )["event_id"]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={"m.fully_read": newer_event_id},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={"m.fully_read": older_event_id, "com.beeper.allow_backward": True},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(room_id), older_event_id)
+
+    @unittest.override_config({"experimental_features": {"msc4446_enabled": True}})
+    def test_send_read_marker_does_not_move_backwards_with_explicit_opt_out(
+        self,
+    ) -> None:
+        room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok)
+
+        older_event_id = self.helper.send(
+            room_id=room_id, body="1", tok=self.owner_tok
+        )["event_id"]
+        newer_event_id = self.helper.send(
+            room_id=room_id, body="2", tok=self.owner_tok
+        )["event_id"]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={"m.fully_read": newer_event_id},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        # Expected to be a no-op.
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={
+                "m.fully_read": older_event_id,
+                "com.beeper.allow_backward": False,
+            },
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(room_id), newer_event_id)
+
+    def test_send_read_marker_ignores_opt_in_when_feature_disabled(self) -> None:
+        room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok)
+        older_event_id = self.helper.send(
+            room_id=room_id, body="1", tok=self.owner_tok
+        )["event_id"]
+        newer_event_id = self.helper.send(
+            room_id=room_id, body="2", tok=self.owner_tok
+        )["event_id"]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={"m.fully_read": newer_event_id},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{room_id}/read_markers",
+            content={"m.fully_read": older_event_id, "com.beeper.allow_backward": True},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(room_id), newer_event_id)
+
     def test_send_read_marker_missing_previous_event(self) -> None:
         """
         Test moving a read marker from an event that previously existed but was

tests/rest/client/test_receipts.py

@@ -24,6 +24,7 @@ from twisted.internet.testing import MemoryReactor
 
 import synapse.rest.admin
 from synapse.api.constants import EduTypes, EventTypes, HistoryVisibility, ReceiptTypes
+from synapse.api.errors import Codes
 from synapse.rest.client import login, receipts, room, sync
 from synapse.server import HomeServer
 from synapse.types import JsonDict
@@ -44,6 +45,7 @@ class ReceiptsTestCase(unittest.HomeserverTestCase):
     def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
         self.url = "/sync?since=%s"
         self.next_batch = "s0"
+        self.store = hs.get_datastores().main
 
         # Register the first user
         self.user_id = self.register_user("kermit", "monkey")
@@ -59,6 +61,19 @@ class ReceiptsTestCase(unittest.HomeserverTestCase):
         # Join the second user
         self.helper.join(room=self.room_id, user=self.user2, tok=self.tok2)
 
+    def _get_fully_read_marker(self) -> str | None:
+        content = self.get_success(
+            self.store.get_account_data_for_room_and_type(
+                self.user2,
+                self.room_id,
+                ReceiptTypes.FULLY_READ,
+            )
+        )
+        if content is None:
+            return None
+
+        return content.get("event_id")
+
     def test_send_receipt(self) -> None:
         # Send a message.
         res = self.helper.send(self.room_id, body="hello", tok=self.tok)
@@ -258,6 +273,126 @@ class ReceiptsTestCase(unittest.HomeserverTestCase):
         self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST)
         self.assertEqual(channel.json_body["errcode"], "M_NOT_JSON", channel.json_body)
 
+    def test_fully_read_receipt_does_not_move_backwards_by_default(self) -> None:
+        older_event_id = self.helper.send(self.room_id, body="1", tok=self.tok)[
+            "event_id"
+        ]
+        newer_event_id = self.helper.send(self.room_id, body="2", tok=self.tok)[
+            "event_id"
+        ]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{newer_event_id}",
+            {},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(), newer_event_id)
+
+        # Expected to be a no-op.
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{older_event_id}",
+            {},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(), newer_event_id)
+
+    @unittest.override_config({"experimental_features": {"msc4446_enabled": True}})
+    def test_fully_read_receipt_can_move_backwards_with_opt_in(self) -> None:
+        older_event_id = self.helper.send(self.room_id, body="1", tok=self.tok)[
+            "event_id"
+        ]
+        newer_event_id = self.helper.send(self.room_id, body="2", tok=self.tok)[
+            "event_id"
+        ]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{newer_event_id}",
+            {},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{older_event_id}",
+            {"com.beeper.allow_backward": True},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(), older_event_id)
+
+    @unittest.override_config({"experimental_features": {"msc4446_enabled": True}})
+    def test_fully_read_receipt_does_not_move_backwards_with_explicit_opt_out(
+        self,
+    ) -> None:
+        older_event_id = self.helper.send(self.room_id, body="1", tok=self.tok)[
+            "event_id"
+        ]
+        newer_event_id = self.helper.send(self.room_id, body="2", tok=self.tok)[
+            "event_id"
+        ]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{newer_event_id}",
+            {},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        # Expected to be a no-op.
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{older_event_id}",
+            {"com.beeper.allow_backward": False},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(), newer_event_id)
+
+    @unittest.override_config({"experimental_features": {"msc4446_enabled": True}})
+    def test_allow_backward_is_rejected_for_read_receipts(self) -> None:
+        event_id = self.helper.send(self.room_id, body="1", tok=self.tok)["event_id"]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.READ}/{event_id}",
+            {"com.beeper.allow_backward": True},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+        self.assertEqual(channel.json_body["errcode"], Codes.INVALID_PARAM)
+
+    def test_allow_backward_is_ignored_when_feature_disabled(self) -> None:
+        older_event_id = self.helper.send(self.room_id, body="1", tok=self.tok)[
+            "event_id"
+        ]
+        newer_event_id = self.helper.send(self.room_id, body="2", tok=self.tok)[
+            "event_id"
+        ]
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{newer_event_id}",
+            {},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        channel = self.make_request(
+            "POST",
+            f"/rooms/{self.room_id}/receipt/{ReceiptTypes.FULLY_READ}/{older_event_id}",
+            {"com.beeper.allow_backward": True},
+            access_token=self.tok2,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertEqual(self._get_fully_read_marker(), newer_event_id)
+
     def _get_read_receipt(self) -> JsonDict | None:
         """Syncs and returns the read receipt."""
 

tests/rest/client/test_versions.py

@@ -142,6 +142,17 @@ class VersionsTestCase(unittest.HomeserverTestCase):
             channel.json_body,
         )
 
+    def test_msc4446_false_by_default(self) -> None:
+        channel = self.make_request("GET", "/_matrix/client/versions")
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertFalse(channel.json_body["unstable_features"]["com.beeper.msc4446"])
+
+    @unittest.override_config({"experimental_features": {"msc4446_enabled": True}})
+    def test_msc4446_true_if_enabled(self) -> None:
+        channel = self.make_request("GET", "/_matrix/client/versions")
+        self.assertEqual(channel.code, 200, channel.result)
+        self.assertTrue(channel.json_body["unstable_features"]["com.beeper.msc4446"])
+
     def _sanity_check_versions_response(self, versions_response: JsonDict) -> None:
         """
         Make sure this looks like a `/_matrix/client/versions` response