From fb6e35cedefcc263d8238a38806eb2e55445ab29 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 29 Apr 2020 15:03:39 +0100 Subject: [PATCH 1/7] Wrap 403's from /publicRooms in SynapseErrors --- synapse/federation/transport/client.py | 31 +++++++++++++++++++++----- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 383e3fdc8bef..2c8bff32a0dd 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -28,6 +28,7 @@ FEDERATION_V2_PREFIX, ) from synapse.logging.utils import log_function +from synapse.api.errors import Codes, HttpResponseException, SynapseError logger = logging.getLogger(__name__) @@ -347,9 +348,18 @@ def get_public_rooms( data["filter"] = search_filter - response = yield self.client.post_json( - destination=remote_server, path=path, data=data, ignore_backoff=True - ) + try: + response = yield self.client.post_json( + destination=remote_server, path=path, data=data, ignore_backoff=True + ) + except HttpResponseException as e: + if e.code == 403: + raise SynapseError( + 403, + "The public room list of %s is private" % (remote_server,), + errcode=Codes.FORBIDDEN, + ) + raise else: path = _create_v1_path("/publicRooms") @@ -363,9 +373,18 @@ def get_public_rooms( if since_token: args["since"] = [since_token] - response = yield self.client.get_json( - destination=remote_server, path=path, args=args, ignore_backoff=True - ) + try: + response = yield self.client.get_json( + destination=remote_server, path=path, args=args, ignore_backoff=True + ) + except HttpResponseException as e: + if e.code == 403: + raise SynapseError( + 403, + "The public room list of %s is private" % (remote_server,), + errcode=Codes.FORBIDDEN, + ) + raise return response From ae5f038f0d88d9532f4248b0bb5d5c77432775f3 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 29 Apr 2020 16:35:03 +0100 Subject: [PATCH 2/7] docstrings --- synapse/federation/federation_client.py | 40 +++++++++++++++++++------ synapse/federation/transport/client.py | 21 ++++++++----- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index a0071fec9496..c16f13ab874c 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -883,18 +883,40 @@ async def _do_send_leave(self, destination, pdu): def get_public_rooms( self, - destination, - limit=None, - since_token=None, - search_filter=None, - include_all_networks=False, - third_party_instance_id=None, - ): - if destination == self.server_name: + remote_server: str, + limit: Optional[int] = None, + since_token: Optional[str] = None, + search_filter: Optional[Dict] = None, + include_all_networks: bool = False, + third_party_instance_id: Optional[str] = None, + ) -> Optional[Dict[str, Any]]: + """Get the list of public rooms from a remote homeserver + + Args: + remote_server: The name of the remote server + limit: Maximum amount of rooms to return + since_token: Used for result pagination + search_filter: A filter dictionary to send the remote homeserver + and filter the result set + include_all_networks: Whether to include results from all third party instances + third_party_instance_id: Whether to only include results from a specific third + party instance + + Returns: + The response from the remote server, or None if `remote_server` is the same as the + local server_name + + Raises: + HttpResponseException: There was an exception returned from the remote server + SynapseException: M_FORBIDDEN when the remote server has disallowed publicRoom + requests over federation + + """ + if remote_server == self.server_name: return return self.transport_layer.get_public_rooms( - destination, + remote_server, limit, since_token, search_filter, diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 2c8bff32a0dd..7da25cdb8ff4 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -15,7 +15,7 @@ # limitations under the License. import logging -from typing import Any, Dict +from typing import Any, Dict, Optional from six.moves import urllib @@ -327,13 +327,18 @@ def send_invite_v2(self, destination, room_id, event_id, content): @log_function def get_public_rooms( self, - remote_server, - limit, - since_token, - search_filter=None, - include_all_networks=False, - third_party_instance_id=None, - ): + remote_server: str, + limit: Optional[int] = None, + since_token: Optional[str] = None, + search_filter: Optional[Dict] = None, + include_all_networks: bool = False, + third_party_instance_id: Optional[str] = None, + ) -> Dict[str, Any]: + """Get the list of public rooms from a remote homeserver + + See synapse.federation.federation_client.FederationClient.get_public_rooms for + more information. + """ if search_filter: # this uses MSC2197 (Search Filtering over Federation) path = _create_v1_path("/publicRooms") From e07979303f8fd4ff81174771e8d78a7ca3f2edcd Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 29 Apr 2020 16:38:05 +0100 Subject: [PATCH 3/7] Change error msg to be more generic --- synapse/federation/transport/client.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 7da25cdb8ff4..d5d12d6cd107 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -361,7 +361,7 @@ def get_public_rooms( if e.code == 403: raise SynapseError( 403, - "The public room list of %s is private" % (remote_server,), + "You are not allowed to view the public rooms list of %s" % (remote_server,), errcode=Codes.FORBIDDEN, ) raise @@ -386,9 +386,9 @@ def get_public_rooms( if e.code == 403: raise SynapseError( 403, - "The public room list of %s is private" % (remote_server,), + "You are not allowed to view the public rooms list of %s" % (remote_server,), errcode=Codes.FORBIDDEN, - ) + ) raise return response From a4f9634fcb4573a61f483ccab91f624dc1376d00 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 29 Apr 2020 16:50:51 +0100 Subject: [PATCH 4/7] Remove unnecessary check --- synapse/federation/federation_client.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index c16f13ab874c..e10bd8164b47 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -889,7 +889,7 @@ def get_public_rooms( search_filter: Optional[Dict] = None, include_all_networks: bool = False, third_party_instance_id: Optional[str] = None, - ) -> Optional[Dict[str, Any]]: + ) -> Dict[str, Any]: """Get the list of public rooms from a remote homeserver Args: @@ -912,9 +912,6 @@ def get_public_rooms( requests over federation """ - if remote_server == self.server_name: - return - return self.transport_layer.get_public_rooms( remote_server, limit, From 550ccc96639369d1addc72bdfd29907ed43f51f7 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 29 Apr 2020 17:17:06 +0100 Subject: [PATCH 5/7] lint --- synapse/federation/federation_client.py | 6 +++--- synapse/federation/transport/client.py | 10 ++++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index e10bd8164b47..58b13da61644 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -889,7 +889,7 @@ def get_public_rooms( search_filter: Optional[Dict] = None, include_all_networks: bool = False, third_party_instance_id: Optional[str] = None, - ) -> Dict[str, Any]: + ): """Get the list of public rooms from a remote homeserver Args: @@ -903,8 +903,8 @@ def get_public_rooms( party instance Returns: - The response from the remote server, or None if `remote_server` is the same as the - local server_name + Deferred[Dict[str, Any]]: The response from the remote server, or None if + `remote_server` is the same as the local server_name Raises: HttpResponseException: There was an exception returned from the remote server diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index d5d12d6cd107..93d40455c641 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -22,13 +22,13 @@ from twisted.internet import defer from synapse.api.constants import Membership +from synapse.api.errors import Codes, HttpResponseException, SynapseError from synapse.api.urls import ( FEDERATION_UNSTABLE_PREFIX, FEDERATION_V1_PREFIX, FEDERATION_V2_PREFIX, ) from synapse.logging.utils import log_function -from synapse.api.errors import Codes, HttpResponseException, SynapseError logger = logging.getLogger(__name__) @@ -333,7 +333,7 @@ def get_public_rooms( search_filter: Optional[Dict] = None, include_all_networks: bool = False, third_party_instance_id: Optional[str] = None, - ) -> Dict[str, Any]: + ): """Get the list of public rooms from a remote homeserver See synapse.federation.federation_client.FederationClient.get_public_rooms for @@ -361,7 +361,8 @@ def get_public_rooms( if e.code == 403: raise SynapseError( 403, - "You are not allowed to view the public rooms list of %s" % (remote_server,), + "You are not allowed to view the public rooms list of %s" + % (remote_server,), errcode=Codes.FORBIDDEN, ) raise @@ -386,7 +387,8 @@ def get_public_rooms( if e.code == 403: raise SynapseError( 403, - "You are not allowed to view the public rooms list of %s" % (remote_server,), + "You are not allowed to view the public rooms list of %s" + % (remote_server,), errcode=Codes.FORBIDDEN, ) raise From 6736979b490afa3747727a0dff1c94d4c0c6f7e9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 29 Apr 2020 17:19:57 +0100 Subject: [PATCH 6/7] Add changelog --- changelog.d/7368.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7368.bugfix diff --git a/changelog.d/7368.bugfix b/changelog.d/7368.bugfix new file mode 100644 index 000000000000..efa8a40b1f0b --- /dev/null +++ b/changelog.d/7368.bugfix @@ -0,0 +1 @@ +Improve error responses when accessing remote public room lists. \ No newline at end of file From 234d1c876a2117726260d38b184f5a3a7c06db76 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 29 Apr 2020 17:48:45 +0100 Subject: [PATCH 7/7] mypy --- synapse/federation/transport/client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 93d40455c641..060bf071975d 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -343,7 +343,9 @@ def get_public_rooms( # this uses MSC2197 (Search Filtering over Federation) path = _create_v1_path("/publicRooms") - data = {"include_all_networks": "true" if include_all_networks else "false"} + data = { + "include_all_networks": "true" if include_all_networks else "false" + } # type: Dict[str, Any] if third_party_instance_id: data["third_party_instance_id"] = third_party_instance_id if limit: