Update and stabilize mutual rooms support (MSC2666) (#19511)

Updates the error codes to match MSC2666 changes (user ID query param
validation + proper errcode for requesting rooms with self), added the
new `count` field, and stabilized the endpoint.

Author: tulir

changelog.d/19511.feature

@@ -0,0 +1 @@
+Update and stabilize support for [MSC2666](https://github.com/matrix-org/matrix-spec-proposals/pull/2666): Get rooms in common with another user. Contributed by @tulir @ Beeper.

synapse/config/experimental.py

@@ -422,9 +422,6 @@ def read_config(
         # previously calculated push actions.
         self.msc2654_enabled: bool = experimental.get("msc2654_enabled", False)
 
-        # MSC2666: Query mutual rooms between two users.
-        self.msc2666_enabled: bool = experimental.get("msc2666_enabled", False)
-
         # MSC2815 (allow room moderators to view redacted event content)
         self.msc2815_enabled: bool = experimental.get("msc2815_enabled", False)
 

synapse/rest/client/mutual_rooms.py

@@ -29,7 +29,7 @@
 from synapse.http.server import HttpServer
 from synapse.http.servlet import RestServlet, parse_strings_from_args
 from synapse.http.site import SynapseRequest
-from synapse.types import JsonDict
+from synapse.types import JsonDict, UserID
 
 from ._base import client_patterns
 
@@ -65,13 +65,10 @@ def _parse_mutual_rooms_batch_token_args(args: dict[bytes, list[bytes]]) -> str
 
 class UserMutualRoomsServlet(RestServlet):
     """
-    GET /uk.half-shot.msc2666/user/mutual_rooms?user_id={user_id}&from={token} HTTP/1.1
+    GET /mutual_rooms?user_id={user_id}&from={token} HTTP/1.1
     """
 
-    PATTERNS = client_patterns(
-        "/uk.half-shot.msc2666/user/mutual_rooms$",
-        releases=(),  # This is an unstable feature
-    )
+    PATTERNS = [*client_patterns("/mutual_rooms$", releases=("v1",))]
 
     def __init__(self, hs: "HomeServer"):
         super().__init__()
@@ -82,7 +79,9 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]:
         # twisted.web.server.Request.args is incorrectly defined as Any | None
         args: dict[bytes, list[bytes]] = request.args  # type: ignore
 
-        user_ids = parse_strings_from_args(args, "user_id", required=True)
+        user_ids = parse_strings_from_args(
+            args, "user_id", required=True, encoding="utf-8"
+        )
         from_batch = _parse_mutual_rooms_batch_token_args(args)
 
         if len(user_ids) > 1:
@@ -93,13 +92,19 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]:
             )
 
         user_id = user_ids[0]
+        if not UserID.is_valid_strict(user_id):
+            raise SynapseError(
+                HTTPStatus.BAD_REQUEST,
+                "Invalid user_id query parameter",
+                errcode=Codes.INVALID_PARAM,
+            )
 
         requester = await self.auth.get_user_by_req(request)
         if user_id == requester.user.to_string():
             raise SynapseError(
                 HTTPStatus.BAD_REQUEST,
                 "You cannot request a list of shared rooms with yourself",
-                errcode=Codes.UNKNOWN,
+                errcode=Codes.INVALID_PARAM,
             )
 
         # Sort here instead of the database function, so that we don't expose
@@ -109,6 +114,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]:
                 frozenset((requester.user.to_string(), user_id))
             )
         )
+        total_count = len(rooms)
 
         if from_batch:
             # A from_batch token was provided, so cut off any rooms where the ID is
@@ -123,7 +129,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]:
 
         if len(rooms) <= MUTUAL_ROOMS_BATCH_LIMIT:
             # We've reached the end of the list, don't return a batch token
-            return 200, {"joined": rooms}
+            return 200, {"joined": rooms, "count": total_count}
 
         rooms = rooms[:MUTUAL_ROOMS_BATCH_LIMIT]
         # We use urlsafe unpadded base64 encoding for the batch token in order to
@@ -135,11 +141,14 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]:
         # in the room ID. In the event that some silly user does that, don't let
         # them paginate further.
         if next_batch == from_batch:
-            return 200, {"joined": rooms}
+            return 200, {"joined": rooms, "count": total_count}
 
-        return 200, {"joined": list(rooms), "next_batch": next_batch}
+        return 200, {
+            "joined": rooms,
+            "next_batch": next_batch,
+            "count": total_count,
+        }
 
 
 def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None:
-    if hs.config.experimental.msc2666_enabled:
-        UserMutualRoomsServlet(hs).register(http_server)
+    UserMutualRoomsServlet(hs).register(http_server)

synapse/rest/client/versions.py

@@ -144,7 +144,7 @@ async def on_GET(self, request: SynapseRequest) -> tuple[int, JsonDict]:
                     # Implements additional endpoints as described in MSC2432
                     "org.matrix.msc2432": True,
                     # Implements additional endpoints as described in MSC2666
-                    "uk.half-shot.msc2666.query_mutual_rooms": self.config.experimental.msc2666_enabled,
+                    "uk.half-shot.msc2666.query_mutual_rooms.stable": True,
                     # Whether new rooms will be set to encrypted or not (based on presets).
                     "io.element.e2ee_forced.public": self.e2ee_forced_public,
                     "io.element.e2ee_forced.private": self.e2ee_forced_private,

synapse/types/__init__.py

@@ -343,7 +343,7 @@ def is_valid(cls: type[DS], s: str) -> bool:
             # possible for invalid data to exist in room-state, etc.
             parse_and_validate_server_name(obj.domain)
             return True
-        except Exception:
+        except (SynapseError, ValueError):
             return False
 
     __repr__ = to_string
@@ -355,6 +355,29 @@ class UserID(DomainSpecificString):
 
     SIGIL = "@"
 
+    @classmethod
+    def is_valid_strict(cls, s: str) -> bool:
+        """
+        Parses the input string and attempts to ensure it is a valid and compliant user
+        ID according to https://spec.matrix.org/v1.17/appendices/#historical-user-ids.
+
+        This should be used with care: there are existing non-compliant user IDs in the
+        wild with empty or non-ASCII localparts, which will be rejected by this method.
+        """
+        if len(s.encode("utf-8")) > 255:
+            return False
+        try:
+            obj = cls.from_string(s)
+            if not is_compliant_user_id_localpart(obj.localpart):
+                return False
+            # Apply additional validation to the domain. This is only done
+            # during  is_valid (and not part of from_string) since it is
+            # possible for invalid data to exist in room-state, etc.
+            parse_and_validate_server_name(obj.domain)
+            return True
+        except (SynapseError, ValueError):
+            return False
+
 
 @attr.s(slots=True, frozen=True, repr=False)
 class RoomAlias(DomainSpecificString):
@@ -453,19 +476,46 @@ class EventID(DomainSpecificString):
 
 
 def contains_invalid_mxid_characters(localpart: str) -> bool:
-    """Check for characters not allowed in an mxid or groupid localpart
+    """
+    Check for characters not allowed in a modern user ID localpart.
+
+    This is primarily used for new registrations and MUST NOT be used to validate
+    existing user IDs, as there are real users whose user IDs don't follow this
+    character set.
+
+    See https://spec.matrix.org/v1.17/appendices/#user-identifiers
 
     Args:
         localpart: the localpart to be checked
-        use_extended_character_set: True to use the extended allowed characters
-            from MSC4009.
 
     Returns:
         True if there are any naughty characters
     """
     return any(c not in MXID_LOCALPART_ALLOWED_CHARACTERS for c in localpart)
 
 
+def is_compliant_user_id_localpart(localpart: str) -> bool:
+    """
+    Validates that the given user ID localpart is within the "compliant" range,
+    i.e. not empty and all characters are between U+0021 and U+007E inclusive.
+    See https://spec.matrix.org/v1.17/appendices/#historical-user-ids
+
+    To check if a localpart is non-historical, use contains_invalid_mxid_characters instead.
+
+    This should be used with care: there are existing non-compliant user IDs in the
+    wild with empty or non-ASCII localparts, which will be rejected by this method.
+
+    Args:
+        localpart: the localpart to be checked
+
+    Returns:
+        True if the localpart is compliant, False otherwise
+    """
+    if not localpart:
+        return False
+    return all(0x21 <= ord(c) <= 0x7E for c in localpart)
+
+
 UPPER_CASE_PATTERN = re.compile(b"[A-Z_]")
 
 # the following is a pattern which matches '=', and bytes which are not allowed in a mxid

tests/rest/client/test_mutual_rooms.py

@@ -43,12 +43,6 @@ class UserMutualRoomsTest(unittest.HomeserverTestCase):
         mutual_rooms.register_servlets,
     ]
 
-    def default_config(self) -> dict:
-        config = super().default_config()
-        experimental = config.setdefault("experimental_features", {})
-        experimental.setdefault("msc2666_enabled", True)
-        return config
-
     def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
         config = self.default_config()
         return self.setup_test_homeserver(config=config)
@@ -62,27 +56,12 @@ def _get_mutual_rooms(
     ) -> FakeChannel:
         return self.make_request(
             "GET",
-            "/_matrix/client/unstable/uk.half-shot.msc2666/user/mutual_rooms"
+            "/_matrix/client/v1/mutual_rooms"
             f"?user_id={quote(other_user)}"
             + (f"&from={quote(since_token)}" if since_token else ""),
             access_token=token,
         )
 
-    @unittest.override_config({"experimental_features": {"msc2666_enabled": False}})
-    def test_mutual_rooms_no_experimental_flag(self) -> None:
-        """
-        The endpoint should 404 if the experimental flag is not enabled.
-        """
-        # Register a user.
-        u1 = self.register_user("user1", "pass")
-        u1_token = self.login(u1, "pass")
-
-        # Check that we're unable to query the endpoint due to the endpoint
-        # being unrecognised.
-        channel = self._get_mutual_rooms(u1_token, "@not-used:test")
-        self.assertEqual(404, channel.code, channel.result)
-        self.assertEqual("M_UNRECOGNIZED", channel.json_body["errcode"], channel.result)
-
     def test_shared_room_list_public(self) -> None:
         """
         A room should show up in the shared list of rooms between two users
@@ -129,6 +108,7 @@ def _check_mutual_rooms_with(
         channel = self._get_mutual_rooms(u1_token, u2)
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(len(channel.json_body["joined"]), 1)
+        self.assertEqual(channel.json_body["count"], 1)
         self.assertEqual(channel.json_body["joined"][0], room_id_one)
 
         # Create another room and invite user2 to it
@@ -142,6 +122,7 @@ def _check_mutual_rooms_with(
         channel = self._get_mutual_rooms(u1_token, u2)
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(len(channel.json_body["joined"]), 2)
+        self.assertEqual(channel.json_body["count"], 2)
         for room_id_id in channel.json_body["joined"]:
             self.assertIn(room_id_id, [room_id_one, room_id_two])
 
@@ -167,11 +148,13 @@ def test_shared_room_list_pagination_two_pages(self) -> None:
         channel = self._get_mutual_rooms(u1_token, u2)
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(channel.json_body["joined"], room_ids[0:10])
+        self.assertEqual(channel.json_body["count"], 15)
         self.assertIn("next_batch", channel.json_body)
 
         channel = self._get_mutual_rooms(u1_token, u2, channel.json_body["next_batch"])
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(channel.json_body["joined"], room_ids[10:20])
+        self.assertEqual(channel.json_body["count"], 15)
         self.assertNotIn("next_batch", channel.json_body)
 
     def test_shared_room_list_pagination_one_page(self) -> None:
@@ -180,6 +163,7 @@ def test_shared_room_list_pagination_one_page(self) -> None:
         channel = self._get_mutual_rooms(u1_token, u2)
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(channel.json_body["joined"], room_ids)
+        self.assertEqual(channel.json_body["count"], 10)
         self.assertNotIn("next_batch", channel.json_body)
 
     def test_shared_room_list_pagination_invalid_token(self) -> None:
@@ -209,6 +193,7 @@ def test_shared_room_list_after_leave(self) -> None:
         channel = self._get_mutual_rooms(u1_token, u2)
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(len(channel.json_body["joined"]), 1)
+        self.assertEqual(channel.json_body["count"], 1)
         self.assertEqual(channel.json_body["joined"][0], room)
 
         self.helper.leave(room, user=u1, tok=u1_token)
@@ -217,11 +202,13 @@ def test_shared_room_list_after_leave(self) -> None:
         channel = self._get_mutual_rooms(u1_token, u2)
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(len(channel.json_body["joined"]), 0)
+        self.assertEqual(channel.json_body["count"], 0)
 
         # Check user2's view of shared rooms with user1
         channel = self._get_mutual_rooms(u2_token, u1)
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(len(channel.json_body["joined"]), 0)
+        self.assertEqual(channel.json_body["count"], 0)
 
     def test_shared_room_list_nonexistent_user(self) -> None:
         u1 = self.register_user("user1", "pass")
@@ -232,4 +219,27 @@ def test_shared_room_list_nonexistent_user(self) -> None:
         channel = self._get_mutual_rooms(u1_token, "@meow:example.com")
         self.assertEqual(200, channel.code, channel.result)
         self.assertEqual(len(channel.json_body["joined"]), 0)
+        self.assertEqual(channel.json_body["count"], 0)
         self.assertNotIn("next_batch", channel.json_body)
+
+    def test_shared_room_list_invalid_user(self) -> None:
+        u1 = self.register_user("user1", "pass")
+        u1_token = self.login(u1, "pass")
+
+        channel = self._get_mutual_rooms(u1_token, "@:example.com")
+        self.assertEqual(400, channel.code, channel.result)
+        self.assertEqual(
+            "M_INVALID_PARAM", channel.json_body["errcode"], channel.result
+        )
+
+        channel = self._get_mutual_rooms(u1_token, "@" + "a" * 255 + ":example.com")
+        self.assertEqual(400, channel.code, channel.result)
+        self.assertEqual(
+            "M_INVALID_PARAM", channel.json_body["errcode"], channel.result
+        )
+
+        channel = self._get_mutual_rooms(u1_token, "@🐈️:example.com")
+        self.assertEqual(400, channel.code, channel.result)
+        self.assertEqual(
+            "M_INVALID_PARAM", channel.json_body["errcode"], channel.result
+        )