Merge branch 'develop' of github.com:matrix-org/synapse into erikj/debug_direct_message_checks

To aid debugging we want to sanity check the device direct messages sent
between servers to try and make sure that remote servers have the
correct device lists.

changelog.d/6678.misc

@@ -0,0 +1 @@
+Check inbound to device messages for correct devices and log any inconsistencies.

changelog.d/6751.misc

@@ -0,0 +1 @@
+Remove some unnecessary admin handler abstraction methods.

synapse/handlers/admin.py

@@ -62,68 +62,6 @@ class AdminHandler(BaseHandler):
             ret["avatar_url"] = profile.avatar_url
         return ret
 
-    async def get_users(self):
-        """Function to retrieve a list of users in users table.
-
-        Args:
-        Returns:
-            defer.Deferred: resolves to list[dict[str, Any]]
-        """
-        ret = await self.store.get_users()
-
-        return ret
-
-    async def get_users_paginate(self, start, limit, name, guests, deactivated):
-        """Function to retrieve a paginated list of users from
-        users list. This will return a json list of users.
-
-        Args:
-            start (int): start number to begin the query from
-            limit (int): number of rows to retrieve
-            name (string): filter for user names
-            guests (bool): whether to in include guest users
-            deactivated (bool): whether to include deactivated users
-        Returns:
-            defer.Deferred: resolves to json list[dict[str, Any]]
-        """
-        ret = await self.store.get_users_paginate(
-            start, limit, name, guests, deactivated
-        )
-
-        return ret
-
-    async def search_users(self, term):
-        """Function to search users list for one or more users with
-        the matched term.
-
-        Args:
-            term (str): search term
-        Returns:
-            defer.Deferred: resolves to list[dict[str, Any]]
-        """
-        ret = await self.store.search_users(term)
-
-        return ret
-
-    def get_user_server_admin(self, user):
-        """
-        Get the admin bit on a user.
-
-        Args:
-            user_id (UserID): the (necessarily local) user to manipulate
-        """
-        return self.store.is_server_admin(user)
-
-    def set_user_server_admin(self, user, admin):
-        """
-        Set the admin bit on a user.
-
-        Args:
-            user_id (UserID): the (necessarily local) user to manipulate
-            admin (bool): whether or not the user should be an admin of this server
-        """
-        return self.store.set_server_admin(user, admin)
-
     async def export_user_data(self, user_id, writer):
         """Write all data we have on the user to the given writer.
 

synapse/handlers/devicemessage.py

@@ -19,6 +19,7 @@ from canonicaljson import json
 
 from twisted.internet import defer
 
+import synapse
 from synapse.api.errors import SynapseError
 from synapse.logging.opentracing import (
     get_active_span_text_map,
@@ -31,9 +32,11 @@ from synapse.util.stringutils import random_string
 
 logger = logging.getLogger(__name__)
 
+device_list_debugging_logger = logging.getLogger("synapse.devices.DEBUG_TRACKING")
+
 
 class DeviceMessageHandler(object):
-    def __init__(self, hs):
+    def __init__(self, hs: "synapse.server.HomeServer"):
         """
         Args:
             hs (synapse.server.HomeServer): server
@@ -65,6 +68,9 @@ class DeviceMessageHandler(object):
                 logger.warning("Request for keys for non-local user %s", user_id)
                 raise SynapseError(400, "Not a user here")
 
+            if not by_device:
+                continue
+
             messages_by_device = {
                 device_id: {
                     "content": message_content,
@@ -73,8 +79,65 @@ class DeviceMessageHandler(object):
                 }
                 for device_id, message_content in by_device.items()
             }
-            if messages_by_device:
-                local_messages[user_id] = messages_by_device
+            local_messages[user_id] = messages_by_device
+
+            if (
+                device_list_debugging_logger.isEnabledFor(logging.INFO)
+                and message_type == "m.room_key_request"
+            ):
+                # If we get a request to get keys then may mean the recipient
+                # didn't know about the sender's device (or might just mean
+                # things are being a bit slow to propogate).
+                received_devices = set(by_device)
+                requesting_device_id = list(by_device.values())[0].get(
+                    "requesting_device_id", "<unknown>"
+                )
+                request_id = list(by_device.values())[0].get("request_id", "<unknown>")
+                action = list(by_device.values())[0].get("action", "<unknown>")
+                device_list_debugging_logger.info(
+                    "Received room_key %s direct message (%s, %s, %s) from %s (%s) to %s (%s).",
+                    action,
+                    message_type,
+                    message_id,
+                    request_id,
+                    sender_user_id,
+                    requesting_device_id,
+                    user_id,
+                    received_devices,
+                )
+            elif device_list_debugging_logger.isEnabledFor(logging.INFO):
+                # We expect the sending user to send the message to all the devices
+                # to the user, if they don't then that is potentially suspicious and
+                # so we log for debugging purposes.
+
+                expected_devices = yield self.store.get_devices_by_user(user_id)
+                expected_devices = set(expected_devices)
+                received_devices = set(by_device)
+                if received_devices != {"*"} and received_devices != expected_devices:
+                    # Devices that the remote didn't send to
+                    missed = expected_devices - received_devices
+
+                    # Devices the remote sent to that we don't know bout
+                    extraneous = received_devices - expected_devices
+
+                    # We try and pull out the `sender_key` from the first message,
+                    # if it has one. This just helps figure out which device the
+                    # message came from.
+                    sender_key = list(by_device.values())[0].get(
+                        "sender_key", "<unknown>"
+                    )
+
+                    device_list_debugging_logger.info(
+                        "Received direct message (%s, %s) from %s (%s) to %s with mismatched devices."
+                        " Missing: %s, extraneous: %s",
+                        message_type,
+                        message_id,
+                        sender_user_id,
+                        sender_key,
+                        user_id,
+                        missed,
+                        extraneous,
+                    )
 
         stream_id = yield self.store.add_messages_from_remote_to_device_inbox(
             origin, message_id, local_messages

synapse/rest/admin/users.py

@@ -45,6 +45,7 @@ class UsersRestServlet(RestServlet):
 
     def __init__(self, hs):
         self.hs = hs
+        self.store = hs.get_datastore()
         self.auth = hs.get_auth()
         self.admin_handler = hs.get_handlers().admin_handler
 
@@ -55,7 +56,7 @@ class UsersRestServlet(RestServlet):
         if not self.hs.is_mine(target_user):
             raise SynapseError(400, "Can only users a local user")
 
-        ret = await self.admin_handler.get_users()
+        ret = await self.store.get_users()
 
         return 200, ret
 
@@ -80,6 +81,7 @@ class UsersRestServletV2(RestServlet):
 
     def __init__(self, hs):
         self.hs = hs
+        self.store = hs.get_datastore()
         self.auth = hs.get_auth()
         self.admin_handler = hs.get_handlers().admin_handler
 
@@ -92,7 +94,7 @@ class UsersRestServletV2(RestServlet):
         guests = parse_boolean(request, "guests", default=True)
         deactivated = parse_boolean(request, "deactivated", default=False)
 
-        users = await self.admin_handler.get_users_paginate(
+        users = await self.store.get_users_paginate(
             start, limit, user_id, guests, deactivated
         )
         ret = {"users": users}
@@ -516,8 +518,8 @@ class SearchUsersRestServlet(RestServlet):
     PATTERNS = historical_admin_path_patterns("/search_users/(?P<target_user_id>[^/]*)")
 
     def __init__(self, hs):
-        self.store = hs.get_datastore()
         self.hs = hs
+        self.store = hs.get_datastore()
         self.auth = hs.get_auth()
         self.handlers = hs.get_handlers()
 
@@ -540,7 +542,7 @@ class SearchUsersRestServlet(RestServlet):
         term = parse_string(request, "term", required=True)
         logger.info("term: %s ", term)
 
-        ret = await self.handlers.admin_handler.search_users(term)
+        ret = await self.handlers.store.search_users(term)
         return 200, ret
 
 
@@ -574,8 +576,8 @@ class UserAdminServlet(RestServlet):
 
     def __init__(self, hs):
         self.hs = hs
+        self.store = hs.get_datastore()
         self.auth = hs.get_auth()
-        self.handlers = hs.get_handlers()
 
     async def on_GET(self, request, user_id):
         await assert_requester_is_admin(self.auth, request)
@@ -585,8 +587,7 @@ class UserAdminServlet(RestServlet):
         if not self.hs.is_mine(target_user):
             raise SynapseError(400, "Only local users can be admins of this homeserver")
 
-        is_admin = await self.handlers.admin_handler.get_user_server_admin(target_user)
-        is_admin = bool(is_admin)
+        is_admin = await self.store.is_server_admin(target_user)
 
         return 200, {"admin": is_admin}
 
@@ -609,8 +610,6 @@ class UserAdminServlet(RestServlet):
         if target_user == auth_user and not set_admin_to:
             raise SynapseError(400, "You may not demote yourself.")
 
-        await self.handlers.admin_handler.set_user_server_admin(
-            target_user, set_admin_to
-        )
+        await self.store.set_user_server_admin(target_user, set_admin_to)
 
         return 200, {}

synapse/storage/data_stores/main/registration.py

@@ -291,7 +291,7 @@ class RegistrationWorkerStore(SQLBaseStore):
             desc="is_server_admin",
         )
 
-        return res if res else False
+        return bool(res) if res else False
 
     def set_server_admin(self, user, admin):
         """Sets whether a user is an admin of this homeserver.