From f4a0ec7e1ca92068c79bd7b16239bbc324556eb8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:48:32 +0000 Subject: [PATCH 01/15] Allow make_query to use unstable prefixes --- synapse/federation/transport/client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 8782586cd6b4..7db6ddb43407 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -235,8 +235,9 @@ async def make_query( args: dict, retry_on_dns_fail: bool, ignore_backoff: bool = False, + prefix: str = FEDERATION_V1_PREFIX, ) -> JsonDict: - path = _create_v1_path("/query/%s", query_type) + path = _create_path(prefix, "/query/%s", query_type) return await self.client.get_json( destination=destination, From 3bd6d056099b543152237cddf223a677b8f63cc2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:49:14 +0000 Subject: [PATCH 02/15] Add an AccountHandler --- synapse/handlers/account.py | 144 ++++++++++++++++++++++++++++++++++++ synapse/server.py | 5 ++ 2 files changed, 149 insertions(+) create mode 100644 synapse/handlers/account.py diff --git a/synapse/handlers/account.py b/synapse/handlers/account.py new file mode 100644 index 000000000000..f3538ced8143 --- /dev/null +++ b/synapse/handlers/account.py @@ -0,0 +1,144 @@ +# Copyright 2022 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. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from typing import TYPE_CHECKING, Dict, List, Tuple + +from synapse.api.errors import Codes, SynapseError +from synapse.types import JsonDict, UserID + +if TYPE_CHECKING: + from synapse.server import HomeServer + + +class AccountHandler: + def __init__(self, hs: "HomeServer"): + self._store = hs.get_datastore() + self._is_mine = hs.is_mine + self._federation_client = hs.get_federation_client() + + async def get_account_statuses( + self, + user_ids: List[bytes], + allow_remote: bool, + ) -> Tuple[JsonDict, List[str]]: + """Get account statuses for a list of user IDs. + + If one or more account(s) belong to remote homeservers, retrieve their status(es) + over federation if allowed. + + Args: + user_ids: The list of accounts to retrieve the status of. + allow_remote: Whether to try to retrieve the status of remote accounts, if + any. + + Returns: + The account statuses as well as the list of users whose statuses could not be + retrieved. + + Raises: + SynapseError if a required parameter is missing or malformed, or if one of + the accounts isn't local to this homeserver and allow_remote is False. + """ + statuses = {} + failures = [] + remote_users: List[UserID] = [] + + for raw_user_id in user_ids: + try: + user_id = UserID.from_string(raw_user_id.decode("ascii")) + except (AttributeError, SynapseError): + raise SynapseError( + 400, + f"Not a valid Matrix user ID: {raw_user_id}", + Codes.INVALID_PARAM, + ) + + if self._is_mine(user_id): + status = await self._get_local_account_status(user_id) + statuses[user_id.to_string()] = status + else: + if not allow_remote: + raise SynapseError( + 400, + f"Not a local user: {raw_user_id}", + Codes.INVALID_PARAM, + ) + + remote_users.append(user_id) + + if allow_remote and len(remote_users) > 0: + remote_statuses, remote_failures = await self._get_remote_account_statuses( + remote_users, + ) + + statuses.update(remote_statuses) + failures += remote_failures + + return statuses, failures + + async def _get_local_account_status(self, user_id: UserID) -> JsonDict: + """Retrieve the status of a local account. + + Args: + user_id: The account to retrieve the status of. + + Returns: + The account's status. + """ + status = {"exists": False} + + userinfo = await self._store.get_userinfo_by_id(user_id.to_string()) + + if userinfo is not None: + status = { + "exists": True, + "deactivated": userinfo.is_deactivated, + } + + return status + + async def _get_remote_account_statuses( + self, remote_users: List[UserID] + ) -> Tuple[JsonDict, List[str]]: + """Send out federation requests to retrieve the statuses of remote accounts. + + Args: + remote_users: The accounts to retrieve the statuses of. + + Returns: + The statuses of the accounts, and a list of accounts for which no status + could be retrieved. + """ + # Group remote users by destination, so we only send one request per remote + # homeserver. + by_destination: Dict[str, List[str]] = {} + for user in remote_users: + if user.domain not in by_destination: + by_destination[user.domain] = [] + + by_destination[user.domain].append(user.to_string()) + + # Retrieve the statuses and failures for remote accounts. + final_statuses: JsonDict = {} + final_failures: List[str] = [] + for destination, users in by_destination.items(): + statuses, failures = await self._federation_client.get_account_status( + destination, + users, + ) + + final_statuses.update(statuses) + final_failures += failures + + return final_statuses, final_failures diff --git a/synapse/server.py b/synapse/server.py index 3032f0b738a8..603946ccb789 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -62,6 +62,7 @@ from synapse.federation.transport.client import TransportLayerClient from synapse.groups.attestations import GroupAttestationSigning, GroupAttestionRenewer from synapse.groups.groups_server import GroupsServerHandler, GroupsServerWorkerHandler +from synapse.handlers.account import AccountHandler from synapse.handlers.account_data import AccountDataHandler from synapse.handlers.account_validity import AccountValidityHandler from synapse.handlers.admin import AdminHandler @@ -807,6 +808,10 @@ def get_event_auth_handler(self) -> EventAuthHandler: def get_external_cache(self) -> ExternalCache: return ExternalCache(self) + @cache_in_self + def get_account_handler(self) -> AccountHandler: + return AccountHandler(self) + @cache_in_self def get_outbound_redis_connection(self) -> "RedisProtocol": """ From 60458bf150a13580703aafd6f49e45f467a681bd Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:49:50 +0000 Subject: [PATCH 03/15] Implement the federation endpoint from MSC3720 --- synapse/federation/federation_client.py | 51 ++++++++++++++++++- .../federation/transport/server/federation.py | 38 ++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 74f17aa4daa3..c7ec94b5ebff 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -53,10 +53,11 @@ RoomVersion, RoomVersions, ) +from synapse.api.urls import FEDERATION_UNSTABLE_PREFIX from synapse.events import EventBase, builder from synapse.federation.federation_base import FederationBase, event_from_pdu_json from synapse.federation.transport.client import SendJoinResponse -from synapse.types import JsonDict, get_domain_from_id +from synapse.types import JsonDict, UserID, get_domain_from_id from synapse.util.async_helpers import concurrently_execute from synapse.util.caches.expiringcache import ExpiringCache from synapse.util.retryutils import NotRetryingDestination @@ -1526,6 +1527,54 @@ async def timestamp_to_event( except ValueError as e: raise InvalidResponseError(str(e)) + async def get_account_status( + self, destination: str, user_ids: List[str] + ) -> Tuple[JsonDict, List[str]]: + """Retrieves account statuses for a given list of users on a given remote + homeserver. + + If the request fails for any reason, all user IDs for this destination are marked + as failed. + + Args: + destination: the destination to contact + user_ids: the user ID(s) for which to request account status(es) + + Returns: + The account statuses, as well as the list of user IDs for which it was not + possible to retrieve a status. + """ + try: + res = await self.transport_layer.make_query( + destination=destination, + query_type="account_status", + args={"user_id": user_ids}, + retry_on_dns_fail=True, + prefix=FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc3720", + ) + except Exception: + # If the query failed for any reason, mark all the users as failed. + return {}, user_ids + + statuses = res.get("account_statuses", {}) + failures = res.get("failures", []) + + if not isinstance(statuses, dict) or not isinstance(failures, list): + # Make sure we're not feeding back malformed data back to the caller. + logger.warning( + "Destination %s responded with malformed data to account_status query", + destination, + ) + return {}, user_ids + + for user_id in user_ids: + # Any account whose status is missing is a user we failed to receive the + # status of. + if user_id not in statuses: + failures.append(user_id) + + return statuses, failures + @attr.s(frozen=True, slots=True, auto_attribs=True) class TimestampToEventResponse: diff --git a/synapse/federation/transport/server/federation.py b/synapse/federation/transport/server/federation.py index d86dfede4e89..5073676df48e 100644 --- a/synapse/federation/transport/server/federation.py +++ b/synapse/federation/transport/server/federation.py @@ -744,6 +744,43 @@ async def on_GET( return 200, complexity +class AccountStatusServlet(BaseFederationServerServlet): + PATH = "/org.matrix.msc3720/query/account_status" + PREFIX = FEDERATION_UNSTABLE_PREFIX + + def __init__( + self, + hs: "HomeServer", + authenticator: Authenticator, + ratelimiter: FederationRateLimiter, + server_name: str, + ): + super().__init__(hs, authenticator, ratelimiter, server_name) + self._account_handler = hs.get_account_handler() + + async def on_GET( + self, + origin: str, + content: Literal[None], + query: Dict[bytes, List[bytes]], + ) -> Tuple[int, JsonDict]: + # Handle MSC3720 account statuses requests. + # TODO: when the MSC has released into the spec, this handler should be moved + # to a query handler + if b"user_id" not in query: + raise SynapseError( + 400, "Required parameter 'user_id' is missing", Codes.MISSING_PARAM + ) + + user_ids: List[bytes] = query[b"user_id"] + statuses, failures = await self._account_handler.get_account_statuses( + user_ids, + allow_remote=False, + ) + + return 200, {"account_statuses": statuses, "failures": failures} + + FEDERATION_SERVLET_CLASSES: Tuple[Type[BaseFederationServlet], ...] = ( FederationSendServlet, FederationEventServlet, @@ -775,4 +812,5 @@ async def on_GET( FederationRoomHierarchyUnstableServlet, FederationV1SendKnockServlet, FederationMakeKnockServlet, + AccountStatusServlet, ) From f739dfe6f7aec3599bb337b929a2602b413a6bf0 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:50:14 +0000 Subject: [PATCH 04/15] Add a configuration flag to enable the feature --- synapse/config/experimental.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index f05a803a7104..2ae42b4b4d8b 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -61,3 +61,6 @@ def read_config(self, config: JsonDict, **kwargs): self.msc2409_to_device_messages_enabled: bool = experimental.get( "msc2409_to_device_messages_enabled", False ) + + # MSC3720 (Account status endpoint) + self.msc3720_enabled: bool = experimental.get("msc3720_enabled", False) From 093067a02e81f02dcde88f540ba8f080a33bfc7f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:50:35 +0000 Subject: [PATCH 05/15] Implement the client-side endpoint and capability from MSC3720 --- synapse/rest/client/account.py | 35 ++++++++++++++++++++++++++++- synapse/rest/client/capabilities.py | 5 +++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index cfa2aee76d49..0cc4754d789a 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -16,7 +16,7 @@ import logging import random from http import HTTPStatus -from typing import TYPE_CHECKING, Optional, Tuple +from typing import TYPE_CHECKING, List, Optional, Tuple from urllib.parse import urlparse from twisted.web.server import Request @@ -894,6 +894,36 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: return 200, response +class AccountStatusRestServlet(RestServlet): + PATTERNS = client_patterns( + "/org.matrix.msc3720/account_status$", unstable=True, releases=() + ) + + def __init__(self, hs: "HomeServer"): + super().__init__() + self._auth = hs.get_auth() + self._store = hs.get_datastore() + self._is_mine = hs.is_mine + self._federation_client = hs.get_federation_client() + self._account_handler = hs.get_account_handler() + + async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + await self._auth.get_user_by_req(request) + + if b"user_id" not in request.args: + raise SynapseError( + 400, "Required parameter 'user_id' is missing", Codes.MISSING_PARAM + ) + + user_ids: List[bytes] = request.args[b"user_id"] + statuses, failures = await self._account_handler.get_account_statuses( + user_ids, + allow_remote=True, + ) + + return 200, {"account_statuses": statuses, "failures": failures} + + def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: EmailPasswordRequestTokenRestServlet(hs).register(http_server) PasswordRestServlet(hs).register(http_server) @@ -908,3 +938,6 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: ThreepidUnbindRestServlet(hs).register(http_server) ThreepidDeleteRestServlet(hs).register(http_server) WhoamiRestServlet(hs).register(http_server) + + if hs.config.experimental.msc3720_enabled: + AccountStatusRestServlet(hs).register(http_server) diff --git a/synapse/rest/client/capabilities.py b/synapse/rest/client/capabilities.py index 5c0e3a568007..a81791953982 100644 --- a/synapse/rest/client/capabilities.py +++ b/synapse/rest/client/capabilities.py @@ -76,6 +76,11 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if self.config.experimental.msc3440_enabled: response["capabilities"]["io.element.thread"] = {"enabled": True} + if self.config.experimental.msc3720_enabled: + response["capabilities"]["org.matrix.msc3720.account_status"] = { + "enabled": True, + } + return 200, response From bc73ae7d3f152dace5837a2d70f7118c6b66d0c0 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:50:50 +0000 Subject: [PATCH 06/15] Add tests --- tests/rest/client/test_account.py | 186 +++++++++++++++++++++++++++++- 1 file changed, 182 insertions(+), 4 deletions(-) diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index 89d85b0a1701..c57b06f76b51 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -1,6 +1,4 @@ -# Copyright 2015-2016 OpenMarket Ltd -# Copyright 2017-2018 New Vector Ltd -# Copyright 2019 The Matrix.org Foundation C.I.C. +# Copyright 2022 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. @@ -17,16 +15,22 @@ import os import re from email.parser import Parser -from typing import Optional +from typing import Dict, List, Optional +from unittest.mock import Mock import pkg_resources +from twisted.internet.testing import MemoryReactor + import synapse.rest.admin from synapse.api.constants import LoginType, Membership from synapse.api.errors import Codes, HttpResponseException from synapse.appservice import ApplicationService +from synapse.rest import admin from synapse.rest.client import account, login, register, room from synapse.rest.synapse.client.password_reset import PasswordResetSubmitTokenResource +from synapse.server import HomeServer +from synapse.util import Clock from tests import unittest from tests.server import FakeSite, make_request @@ -1037,3 +1041,177 @@ def _add_email(self, request_email, expected_email): threepids = {threepid["address"] for threepid in channel.json_body["threepids"]} self.assertIn(expected_email, threepids) + + +class AccountStatusTestCase(unittest.HomeserverTestCase): + servlets = [ + account.register_servlets, + admin.register_servlets, + login.register_servlets, + ] + + url = "/_matrix/client/unstable/org.matrix.msc3720/account_status" + + def make_homeserver(self, reactor, clock): + config = self.default_config() + config["experimental_features"] = {"msc3720_enabled": True} + + return self.setup_test_homeserver(config=config) + + def prepare(self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer): + self.requester = self.register_user("requester", "password") + self.requester_tok = self.login("requester", "password") + self.server_name = homeserver.config.server.server_name + + def test_missing_mxid(self): + """Tests that not providing any MXID raises an error.""" + self._test_status( + users=None, + expected_status_code=400, + expected_errcode=Codes.MISSING_PARAM, + ) + + def test_invalid_mxid(self): + """Tests that providing an invalid MXID raises an error.""" + self._test_status( + users=["bad:test"], + expected_status_code=400, + expected_errcode=Codes.INVALID_PARAM, + ) + + def test_local_user_not_exists(self): + """Tests that the account status endpoints correctly reports that a user doesn't + exist. + """ + user = "@unknown:" + self.hs.config.server.server_name + + self._test_status( + users=[user], + expected_statuses={ + user: { + "exists": False, + }, + }, + expected_failures=[], + ) + + def test_local_user_exists(self): + """Tests that the account status endpoint correctly reports that a user doesn't + exist. + """ + user = self.register_user("someuser", "password") + + self._test_status( + users=[user], + expected_statuses={ + user: { + "exists": True, + "deactivated": False, + }, + }, + expected_failures=[], + ) + + def test_local_user_deactivated(self): + """Tests that the account status endpoint correctly reports a deactivated user.""" + user = self.register_user("someuser", "password") + self.get_success( + self.hs.get_datastore().set_user_deactivated_status(user, deactivated=True) + ) + + self._test_status( + users=[user], + expected_statuses={ + user: { + "exists": True, + "deactivated": True, + }, + }, + expected_failures=[], + ) + + def test_mixed_local_and_remote_users(self): + """Tests that if some users are remote the account status endpoint correctly + merges the remote responses with the local result. + """ + # We use 3 users: one doesn't exist but belongs on the local homeserver, one is + # deactivated and belongs on one remote homeserver, and one belongs to another + # remote homeserver that didn't return any result (the federation code should + # mark that user as a failure). + users = [ + "@unknown:" + self.hs.config.server.server_name, + "@deactivated:remote", + "@failed:otherremote", + ] + + async def get_json(destination, path, args, *a, **kwa): + if destination == "remote": + return { + "account_statuses": { + users[1]: { + "exists": True, + "deactivated": True, + }, + } + } + if destination == "otherremote": + return {} + + # Register a mock that will return the expected result depending on the remote. + self.hs.get_federation_http_client().get_json = Mock(side_effect=get_json) + + # Check that we've got the correct response from the client-side endpoint. + self._test_status( + users=users, + expected_statuses={ + users[0]: { + "exists": False, + }, + users[1]: { + "exists": True, + "deactivated": True, + }, + }, + expected_failures=[users[2]], + ) + + def _test_status( + self, + users: Optional[List[str]], + expected_status_code: int = 200, + expected_statuses: Optional[Dict[str, Dict[str, bool]]] = None, + expected_failures: Optional[List[str]] = None, + expected_errcode: Optional[str] = None, + ): + """Send a request to the account status endpoint and check that the response + matches with what's expected. + + Args: + users: The account(s) to request the status of, if any. If set to None, no + `user_id` query parameter will be included in the request. + expected_status_code: The expected HTTP status code. + expected_statuses: The expected account statuses, if any. + expected_failures: The expected failures, if any. + expected_errcode: The expected Matrix error code, if any. + """ + if users is None: + url = self.url + else: + url = self.url + "?user_id=" + "&user_id=".join(users) + + channel = self.make_request( + method="GET", + path=url, + access_token=self.requester_tok, + ) + + self.assertEqual(channel.code, expected_status_code) + + if expected_statuses is not None: + self.assertEqual(channel.json_body["account_statuses"], expected_statuses) + + if expected_failures is not None: + self.assertEqual(channel.json_body["failures"], expected_failures) + + if expected_errcode is not None: + self.assertEqual(channel.json_body["errcode"], expected_errcode) From 4fa53f970bd70599aad9967b8e00a591da9443d9 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:53:56 +0000 Subject: [PATCH 07/15] Newsfile --- changelog.d/12001.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12001.feature diff --git a/changelog.d/12001.feature b/changelog.d/12001.feature new file mode 100644 index 000000000000..dc1153c49ef4 --- /dev/null +++ b/changelog.d/12001.feature @@ -0,0 +1 @@ +Implement experimental support for [MSC3720](https://github.com/matrix-org/matrix-doc/pull/3720) (account status endpoints). From 053843d0dc83338c8f9d17aa50ff2b7370537715 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 15:59:27 +0000 Subject: [PATCH 08/15] Lint --- synapse/federation/federation_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index c7ec94b5ebff..92482d7c2d10 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -57,7 +57,7 @@ from synapse.events import EventBase, builder from synapse.federation.federation_base import FederationBase, event_from_pdu_json from synapse.federation.transport.client import SendJoinResponse -from synapse.types import JsonDict, UserID, get_domain_from_id +from synapse.types import JsonDict, get_domain_from_id from synapse.util.async_helpers import concurrently_execute from synapse.util.caches.expiringcache import ExpiringCache from synapse.util.retryutils import NotRetryingDestination From d8f11bd18a35fa7fc91d1964cce62bbb0b7e0cdc Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 16:34:08 +0000 Subject: [PATCH 09/15] Appease mypy --- synapse/handlers/account.py | 7 ++++--- synapse/rest/client/account.py | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/account.py b/synapse/handlers/account.py index f3538ced8143..bbb4c4dc5f16 100644 --- a/synapse/handlers/account.py +++ b/synapse/handlers/account.py @@ -54,13 +54,14 @@ async def get_account_statuses( failures = [] remote_users: List[UserID] = [] - for raw_user_id in user_ids: + for user_id_bytes in user_ids: try: - user_id = UserID.from_string(raw_user_id.decode("ascii")) + raw_user_id = user_id_bytes.decode("ascii") + user_id = UserID.from_string(raw_user_id) except (AttributeError, SynapseError): raise SynapseError( 400, - f"Not a valid Matrix user ID: {raw_user_id}", + f"Not a valid Matrix user ID: {user_id_bytes.decode('utf8')}", Codes.INVALID_PARAM, ) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 0cc4754d789a..49c726050661 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -16,7 +16,7 @@ import logging import random from http import HTTPStatus -from typing import TYPE_CHECKING, List, Optional, Tuple +from typing import TYPE_CHECKING, List, Optional, Tuple, Dict, Any from urllib.parse import urlparse from twisted.web.server import Request @@ -910,12 +910,13 @@ def __init__(self, hs: "HomeServer"): async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await self._auth.get_user_by_req(request) - if b"user_id" not in request.args: + args: Dict[bytes, List[bytes]] = request.args # type: ignore[assignment] + if b"user_id" not in args: raise SynapseError( 400, "Required parameter 'user_id' is missing", Codes.MISSING_PARAM ) - user_ids: List[bytes] = request.args[b"user_id"] + user_ids: List[bytes] = args[b"user_id"] statuses, failures = await self._account_handler.get_account_statuses( user_ids, allow_remote=True, From d37b31a9b2929299e99acb550caa84bf7df15db2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 15 Feb 2022 16:37:01 +0000 Subject: [PATCH 10/15] Lint --- synapse/rest/client/account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 49c726050661..0470893835b1 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -16,7 +16,7 @@ import logging import random from http import HTTPStatus -from typing import TYPE_CHECKING, List, Optional, Tuple, Dict, Any +from typing import TYPE_CHECKING, Dict, List, Optional, Tuple from urllib.parse import urlparse from twisted.web.server import Request From 8d61e966864c68c83d89886e14f238987fb08d40 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 17 Feb 2022 12:51:25 +0100 Subject: [PATCH 11/15] Apply suggestions from code review Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- synapse/federation/transport/server/federation.py | 4 ++-- tests/rest/client/test_account.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/federation/transport/server/federation.py b/synapse/federation/transport/server/federation.py index 5048bced1a5d..68732f57ce97 100644 --- a/synapse/federation/transport/server/federation.py +++ b/synapse/federation/transport/server/federation.py @@ -767,8 +767,8 @@ async def on_GET( class AccountStatusServlet(BaseFederationServerServlet): - PATH = "/org.matrix.msc3720/query/account_status" - PREFIX = FEDERATION_UNSTABLE_PREFIX + PATH = "/query/account_status" + PREFIX = FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc3720" def __init__( self, diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index c57b06f76b51..03a6bc6f26f1 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -20,7 +20,7 @@ import pkg_resources -from twisted.internet.testing import MemoryReactor +from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin from synapse.api.constants import LoginType, Membership From 63d76a7334084397131c7acef72237968d27661b Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 21 Feb 2022 17:51:10 +0000 Subject: [PATCH 12/15] Incorporate review and MSC changes --- synapse/federation/federation_client.py | 27 +++++++++------ synapse/federation/transport/client.py | 18 ++++++++++ .../federation/transport/server/__init__.py | 9 ++++- .../federation/transport/server/federation.py | 21 +++++------- synapse/handlers/account.py | 9 +++-- synapse/rest/client/account.py | 13 ++++--- tests/rest/client/test_account.py | 34 ++++++++++++++----- 7 files changed, 88 insertions(+), 43 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 92482d7c2d10..181e01c268a8 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -57,7 +57,7 @@ from synapse.events import EventBase, builder from synapse.federation.federation_base import FederationBase, event_from_pdu_json from synapse.federation.transport.client import SendJoinResponse -from synapse.types import JsonDict, get_domain_from_id +from synapse.types import JsonDict, get_domain_from_id, UserID from synapse.util.async_helpers import concurrently_execute from synapse.util.caches.expiringcache import ExpiringCache from synapse.util.retryutils import NotRetryingDestination @@ -1545,13 +1545,7 @@ async def get_account_status( possible to retrieve a status. """ try: - res = await self.transport_layer.make_query( - destination=destination, - query_type="account_status", - args={"user_id": user_ids}, - retry_on_dns_fail=True, - prefix=FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc3720", - ) + res = await self.transport_layer.get_account_status(destination, user_ids) except Exception: # If the query failed for any reason, mark all the users as failed. return {}, user_ids @@ -1570,10 +1564,23 @@ async def get_account_status( for user_id in user_ids: # Any account whose status is missing is a user we failed to receive the # status of. - if user_id not in statuses: + if user_id not in statuses and user_id not in failures: failures.append(user_id) - return statuses, failures + # Filter out any user ID that doesn't belong to the remote server that sent its + # status. + def filter_status(item: Tuple[str, JsonDict]) -> bool: + # item is a (key, value) tuple. + user_id = item[0] + try: + return UserID.from_string(user_id).domain == destination + except SynapseError: + # If the user ID doesn't parse, ignore it. + return False + + filtered_statuses = dict(filter(filter_status, statuses.items())) + + return filtered_statuses, failures @attr.s(frozen=True, slots=True, auto_attribs=True) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 7db6ddb43407..175b575e6336 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -1220,6 +1220,24 @@ async def get_room_hierarchy_unstable( args={"suggested_only": "true" if suggested_only else "false"}, ) + async def get_account_status( + self, destination: str, user_ids: List[str] + ) -> JsonDict: + """ + Args: + destination: The remote server. + user_ids: The user ID(s) for which to request account status(es). + """ + path = _create_path( + FEDERATION_UNSTABLE_PREFIX, "/org.matrix.msc3720/account_status" + ) + + return await self.client.post_json( + destination=destination, + path=path, + data={"user_ids": user_ids} + ) + def _create_path(federation_prefix: str, path: str, *args: str) -> str: """ diff --git a/synapse/federation/transport/server/__init__.py b/synapse/federation/transport/server/__init__.py index db4fe2c79803..15e2e1383273 100644 --- a/synapse/federation/transport/server/__init__.py +++ b/synapse/federation/transport/server/__init__.py @@ -24,7 +24,7 @@ ) from synapse.federation.transport.server.federation import ( FEDERATION_SERVLET_CLASSES, - FederationTimestampLookupServlet, + FederationTimestampLookupServlet, FederationAccountStatusServlet, ) from synapse.federation.transport.server.groups_local import GROUP_LOCAL_SERVLET_CLASSES from synapse.federation.transport.server.groups_server import ( @@ -336,6 +336,13 @@ def register_servlets( ): continue + # Only allow the `/account_status` servlet if msc3720 is enabled + if ( + servletclass == FederationAccountStatusServlet + and not hs.config.experimental.msc3720_enabled + ): + continue + servletclass( hs=hs, authenticator=authenticator, diff --git a/synapse/federation/transport/server/federation.py b/synapse/federation/transport/server/federation.py index 68732f57ce97..4d75e58bfc81 100644 --- a/synapse/federation/transport/server/federation.py +++ b/synapse/federation/transport/server/federation.py @@ -766,7 +766,7 @@ async def on_GET( return 200, complexity -class AccountStatusServlet(BaseFederationServerServlet): +class FederationAccountStatusServlet(BaseFederationServerServlet): PATH = "/query/account_status" PREFIX = FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc3720" @@ -780,23 +780,20 @@ def __init__( super().__init__(hs, authenticator, ratelimiter, server_name) self._account_handler = hs.get_account_handler() - async def on_GET( + async def on_POST( self, origin: str, - content: Literal[None], - query: Dict[bytes, List[bytes]], + content: JsonDict, + query: Mapping[bytes, Sequence[bytes]], + room_id: str, ) -> Tuple[int, JsonDict]: - # Handle MSC3720 account statuses requests. - # TODO: when the MSC has released into the spec, this handler should be moved - # to a query handler - if b"user_id" not in query: + if "user_ids" not in content: raise SynapseError( - 400, "Required parameter 'user_id' is missing", Codes.MISSING_PARAM + 400, "Required parameter 'user_ids' is missing", Codes.MISSING_PARAM ) - user_ids: List[bytes] = query[b"user_id"] statuses, failures = await self._account_handler.get_account_statuses( - user_ids, + content["user_ids"], allow_remote=False, ) @@ -834,5 +831,5 @@ async def on_GET( FederationRoomHierarchyUnstableServlet, FederationV1SendKnockServlet, FederationMakeKnockServlet, - AccountStatusServlet, + FederationAccountStatusServlet, ) diff --git a/synapse/handlers/account.py b/synapse/handlers/account.py index bbb4c4dc5f16..f8cfe9f6dec7 100644 --- a/synapse/handlers/account.py +++ b/synapse/handlers/account.py @@ -29,7 +29,7 @@ def __init__(self, hs: "HomeServer"): async def get_account_statuses( self, - user_ids: List[bytes], + user_ids: List[str], allow_remote: bool, ) -> Tuple[JsonDict, List[str]]: """Get account statuses for a list of user IDs. @@ -54,14 +54,13 @@ async def get_account_statuses( failures = [] remote_users: List[UserID] = [] - for user_id_bytes in user_ids: + for raw_user_id in user_ids: try: - raw_user_id = user_id_bytes.decode("ascii") user_id = UserID.from_string(raw_user_id) - except (AttributeError, SynapseError): + except SynapseError: raise SynapseError( 400, - f"Not a valid Matrix user ID: {user_id_bytes.decode('utf8')}", + f"Not a valid Matrix user ID: {raw_user_id}", Codes.INVALID_PARAM, ) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 0470893835b1..548a0b2e0ba7 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -35,7 +35,7 @@ RestServlet, assert_params_in_dict, parse_json_object_from_request, - parse_string, + parse_string, parse_strings_from_args, ) from synapse.http.site import SynapseRequest from synapse.metrics import threepid_send_requests @@ -907,18 +907,17 @@ def __init__(self, hs: "HomeServer"): self._federation_client = hs.get_federation_client() self._account_handler = hs.get_account_handler() - async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await self._auth.get_user_by_req(request) - args: Dict[bytes, List[bytes]] = request.args # type: ignore[assignment] - if b"user_id" not in args: + body = parse_json_object_from_request(request) + if "user_ids" not in body: raise SynapseError( - 400, "Required parameter 'user_id' is missing", Codes.MISSING_PARAM + 400, "Required parameter 'user_ids' is missing", Codes.MISSING_PARAM ) - user_ids: List[bytes] = args[b"user_id"] statuses, failures = await self._account_handler.get_account_statuses( - user_ids, + body["user_ids"], allow_remote=True, ) diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index 03a6bc6f26f1..3fea2bc1656e 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -1142,9 +1142,10 @@ def test_mixed_local_and_remote_users(self): "@unknown:" + self.hs.config.server.server_name, "@deactivated:remote", "@failed:otherremote", + "@bad:badremote" ] - async def get_json(destination, path, args, *a, **kwa): + async def post_json(destination, path, data, *a, **kwa): if destination == "remote": return { "account_statuses": { @@ -1156,9 +1157,23 @@ async def get_json(destination, path, args, *a, **kwa): } if destination == "otherremote": return {} + if destination == "badremote": + # badremote tries to overwrite the status of a user that doesn't belong + # to it (i.e. users[1]) with false data, which Synapse is expected to + # ignore. + return { + "account_statuses": { + users[3]: { + "exists": False, + }, + users[1]: { + "exists": False, + } + } + } # Register a mock that will return the expected result depending on the remote. - self.hs.get_federation_http_client().get_json = Mock(side_effect=get_json) + self.hs.get_federation_http_client().post_json = Mock(side_effect=post_json) # Check that we've got the correct response from the client-side endpoint. self._test_status( @@ -1171,6 +1186,9 @@ async def get_json(destination, path, args, *a, **kwa): "exists": True, "deactivated": True, }, + users[3]: { + "exists": False, + }, }, expected_failures=[users[2]], ) @@ -1194,14 +1212,14 @@ def _test_status( expected_failures: The expected failures, if any. expected_errcode: The expected Matrix error code, if any. """ - if users is None: - url = self.url - else: - url = self.url + "?user_id=" + "&user_id=".join(users) + content = {} + if users is not None: + content["user_ids"] = users channel = self.make_request( - method="GET", - path=url, + method="POST", + path=self.url, + content=content, access_token=self.requester_tok, ) From 0889c896548f3685f13eedfb4d23b2d6b836cc97 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 21 Feb 2022 17:56:26 +0000 Subject: [PATCH 13/15] Lint --- synapse/federation/federation_client.py | 3 +-- synapse/federation/transport/client.py | 4 +--- synapse/federation/transport/server/__init__.py | 3 ++- synapse/rest/client/account.py | 4 ++-- tests/rest/client/test_account.py | 4 ++-- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index b10ab8646baf..16cde3d8f479 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -53,11 +53,10 @@ RoomVersion, RoomVersions, ) -from synapse.api.urls import FEDERATION_UNSTABLE_PREFIX from synapse.events import EventBase, builder from synapse.federation.federation_base import FederationBase, event_from_pdu_json from synapse.federation.transport.client import SendJoinResponse -from synapse.types import JsonDict, get_domain_from_id, UserID +from synapse.types import JsonDict, UserID, get_domain_from_id from synapse.util.async_helpers import concurrently_execute from synapse.util.caches.expiringcache import ExpiringCache from synapse.util.retryutils import NotRetryingDestination diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 97400a05144e..42e01e13729d 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -1239,9 +1239,7 @@ async def get_account_status( ) return await self.client.post_json( - destination=destination, - path=path, - data={"user_ids": user_ids} + destination=destination, path=path, data={"user_ids": user_ids} ) diff --git a/synapse/federation/transport/server/__init__.py b/synapse/federation/transport/server/__init__.py index 15e2e1383273..67a634790712 100644 --- a/synapse/federation/transport/server/__init__.py +++ b/synapse/federation/transport/server/__init__.py @@ -24,7 +24,8 @@ ) from synapse.federation.transport.server.federation import ( FEDERATION_SERVLET_CLASSES, - FederationTimestampLookupServlet, FederationAccountStatusServlet, + FederationAccountStatusServlet, + FederationTimestampLookupServlet, ) from synapse.federation.transport.server.groups_local import GROUP_LOCAL_SERVLET_CLASSES from synapse.federation.transport.server.groups_server import ( diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 3bfdc38b5ddb..5802de5b7c58 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -16,7 +16,7 @@ import logging import random from http import HTTPStatus -from typing import TYPE_CHECKING, Dict, List, Optional, Tuple +from typing import TYPE_CHECKING, Optional, Tuple from urllib.parse import urlparse from twisted.web.server import Request @@ -35,7 +35,7 @@ RestServlet, assert_params_in_dict, parse_json_object_from_request, - parse_string, parse_strings_from_args, + parse_string, ) from synapse.http.site import SynapseRequest from synapse.metrics import threepid_send_requests diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index 0c5d4df85130..afaa597f6597 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -1145,7 +1145,7 @@ def test_mixed_local_and_remote_users(self): "@unknown:" + self.hs.config.server.server_name, "@deactivated:remote", "@failed:otherremote", - "@bad:badremote" + "@bad:badremote", ] async def post_json(destination, path, data, *a, **kwa): @@ -1171,7 +1171,7 @@ async def post_json(destination, path, data, *a, **kwa): }, users[1]: { "exists": False, - } + }, } } From 38cc18ce5b3fabf6973e7800c11a5031efc5ff33 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 22 Feb 2022 14:21:16 +0000 Subject: [PATCH 14/15] Filter failures received over federation --- synapse/federation/federation_client.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 16cde3d8f479..111d53eecdff 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -1580,19 +1580,24 @@ async def get_account_status( failures.append(user_id) # Filter out any user ID that doesn't belong to the remote server that sent its - # status. - def filter_status(item: Tuple[str, JsonDict]) -> bool: - # item is a (key, value) tuple. - user_id = item[0] + # status (or failure). + def filter_user_id(user_id: str) -> bool: try: return UserID.from_string(user_id).domain == destination except SynapseError: # If the user ID doesn't parse, ignore it. return False - filtered_statuses = dict(filter(filter_status, statuses.items())) + filtered_statuses = dict( + # item is a (key, value) tuple, so item[0] is the user ID. + filter(lambda item: filter_user_id(item[0]), statuses.items()) + ) + + filtered_failures = list( + filter(filter_user_id, failures) + ) - return filtered_statuses, failures + return filtered_statuses, filtered_failures @attr.s(frozen=True, slots=True, auto_attribs=True) From fb51368609e223649c268b7852b39e104b6a02bb Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 22 Feb 2022 14:39:46 +0000 Subject: [PATCH 15/15] Lint --- synapse/federation/federation_client.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 111d53eecdff..bae0b3371a67 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -1593,9 +1593,7 @@ def filter_user_id(user_id: str) -> bool: filter(lambda item: filter_user_id(item[0]), statuses.items()) ) - filtered_failures = list( - filter(filter_user_id, failures) - ) + filtered_failures = list(filter(filter_user_id, failures)) return filtered_statuses, filtered_failures