diff options
| -rw-r--r-- | changelog.d/13241.removal | 1 | ||||
| -rw-r--r-- | synapse/handlers/identity.py | 142 | ||||
| -rw-r--r-- | synapse/handlers/room.py | 20 | ||||
| -rw-r--r-- | synapse/handlers/room_member.py | 6 | ||||
| -rw-r--r-- | synapse/rest/client/room.py | 20 | ||||
| -rw-r--r-- | synapse/rest/media/v1/media_repository.py | 1 | ||||
| -rw-r--r-- | tests/rest/client/test_identity.py | 3 | ||||
| -rw-r--r-- | tests/rest/client/test_rooms.py | 18 | ||||
| -rw-r--r-- | tests/rest/client/test_shadow_banned.py | 7 |
9 files changed, 81 insertions, 137 deletions
diff --git a/changelog.d/13241.removal b/changelog.d/13241.removal new file mode 100644 index 0000000000..60b0e7969c --- /dev/null +++ b/changelog.d/13241.removal @@ -0,0 +1 @@ +Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token`, which was not permitted by the spec. Contributed by @Vetchu. \ No newline at end of file diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 9571d461c8..93d09e9939 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -538,11 +538,7 @@ class IdentityHandler: raise SynapseError(400, "Error contacting the identity server") async def lookup_3pid( - self, - id_server: str, - medium: str, - address: str, - id_access_token: Optional[str] = None, + self, id_server: str, medium: str, address: str, id_access_token: str ) -> Optional[str]: """Looks up a 3pid in the passed identity server. @@ -557,60 +553,15 @@ class IdentityHandler: Returns: the matrix ID of the 3pid, or None if it is not recognized. """ - if id_access_token is not None: - try: - results = await self._lookup_3pid_v2( - id_server, id_access_token, medium, address - ) - return results - - except Exception as e: - # Catch HttpResponseExcept for a non-200 response code - # Check if this identity server does not know about v2 lookups - if isinstance(e, HttpResponseException) and e.code == 404: - # This is an old identity server that does not yet support v2 lookups - logger.warning( - "Attempted v2 lookup on v1 identity server %s. Falling " - "back to v1", - id_server, - ) - else: - logger.warning("Error when looking up hashing details: %s", e) - return None - - return await self._lookup_3pid_v1(id_server, medium, address) - - async def _lookup_3pid_v1( - self, id_server: str, medium: str, address: str - ) -> Optional[str]: - """Looks up a 3pid in the passed identity server using v1 lookup. - Args: - id_server: The server name (including port, if required) - of the identity server to use. - medium: The type of the third party identifier (e.g. "email"). - address: The third party identifier (e.g. "foo@example.com"). - - Returns: - the matrix ID of the 3pid, or None if it is not recognized. - """ try: - data = await self.blacklisting_http_client.get_json( - "%s%s/_matrix/identity/api/v1/lookup" % (id_server_scheme, id_server), - {"medium": medium, "address": address}, + results = await self._lookup_3pid_v2( + id_server, id_access_token, medium, address ) - - if "mxid" in data: - # note: we used to verify the identity server's signature here, but no longer - # require or validate it. See the following for context: - # https://github.com/matrix-org/synapse/issues/5253#issuecomment-666246950 - return data["mxid"] - except RequestTimedOutError: - raise SynapseError(500, "Timed out contacting identity server") - except OSError as e: - logger.warning("Error from v1 identity server lookup: %s" % (e,)) - - return None + return results + except Exception as e: + logger.warning("Error when looking up hashing details: %s", e) + return None async def _lookup_3pid_v2( self, id_server: str, id_access_token: str, medium: str, address: str @@ -739,7 +690,7 @@ class IdentityHandler: room_type: Optional[str], inviter_display_name: str, inviter_avatar_url: str, - id_access_token: Optional[str] = None, + id_access_token: str, ) -> Tuple[str, List[Dict[str, str]], Dict[str, str], str]: """ Asks an identity server for a third party invite. @@ -760,7 +711,7 @@ class IdentityHandler: inviter_display_name: The current display name of the inviter. inviter_avatar_url: The URL of the inviter's avatar. - id_access_token (str|None): The access token to authenticate to the identity + id_access_token (str): The access token to authenticate to the identity server with Returns: @@ -792,71 +743,24 @@ class IdentityHandler: invite_config["org.matrix.web_client_location"] = self._web_client_location # Add the identity service access token to the JSON body and use the v2 - # Identity Service endpoints if id_access_token is present + # Identity Service endpoints data = None - base_url = "%s%s/_matrix/identity" % (id_server_scheme, id_server) - if id_access_token: - key_validity_url = "%s%s/_matrix/identity/v2/pubkey/isvalid" % ( - id_server_scheme, - id_server, - ) + key_validity_url = "%s%s/_matrix/identity/v2/pubkey/isvalid" % ( + id_server_scheme, + id_server, + ) - # Attempt a v2 lookup - url = base_url + "/v2/store-invite" - try: - data = await self.blacklisting_http_client.post_json_get_json( - url, - invite_config, - {"Authorization": create_id_access_token_header(id_access_token)}, - ) - except RequestTimedOutError: - raise SynapseError(500, "Timed out contacting identity server") - except HttpResponseException as e: - if e.code != 404: - logger.info("Failed to POST %s with JSON: %s", url, e) - raise e - - if data is None: - key_validity_url = "%s%s/_matrix/identity/api/v1/pubkey/isvalid" % ( - id_server_scheme, - id_server, + url = "%s%s/_matrix/identity/v2/store-invite" % (id_server_scheme, id_server) + try: + data = await self.blacklisting_http_client.post_json_get_json( + url, + invite_config, + {"Authorization": create_id_access_token_header(id_access_token)}, ) - url = base_url + "/api/v1/store-invite" - - try: - data = await self.blacklisting_http_client.post_json_get_json( - url, invite_config - ) - except RequestTimedOutError: - raise SynapseError(500, "Timed out contacting identity server") - except HttpResponseException as e: - logger.warning( - "Error trying to call /store-invite on %s%s: %s", - id_server_scheme, - id_server, - e, - ) - - if data is None: - # Some identity servers may only support application/x-www-form-urlencoded - # types. This is especially true with old instances of Sydent, see - # https://github.com/matrix-org/sydent/pull/170 - try: - data = await self.blacklisting_http_client.post_urlencoded_get_json( - url, invite_config - ) - except HttpResponseException as e: - logger.warning( - "Error calling /store-invite on %s%s with fallback " - "encoding: %s", - id_server_scheme, - id_server, - e, - ) - raise e - - # TODO: Check for success + except RequestTimedOutError: + raise SynapseError(500, "Timed out contacting identity server") + token = data["token"] public_keys = data.get("public_keys", []) if "public_key" in data: diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index f64a8690a5..33e9a87002 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -19,6 +19,7 @@ import math import random import string from collections import OrderedDict +from http import HTTPStatus from typing import ( TYPE_CHECKING, Any, @@ -704,8 +705,8 @@ class RoomCreationHandler: was, requested, `room_alias`. Secondly, the stream_id of the last persisted event. Raises: - SynapseError if the room ID couldn't be stored, or something went - horribly wrong. + SynapseError if the room ID couldn't be stored, 3pid invitation config + validation failed, or something went horribly wrong. ResourceLimitError if server is blocked to some resource being exceeded """ @@ -731,6 +732,19 @@ class RoomCreationHandler: invite_3pid_list = config.get("invite_3pid", []) invite_list = config.get("invite", []) + # validate each entry for correctness + for invite_3pid in invite_3pid_list: + if not all( + key in invite_3pid + for key in ("medium", "address", "id_server", "id_access_token") + ): + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "all of `medium`, `address`, `id_server` and `id_access_token` " + "are required when making a 3pid invite", + Codes.MISSING_PARAM, + ) + if not is_requester_admin: spam_check = await self.spam_checker.user_may_create_room(user_id) if spam_check != NOT_SPAM: @@ -978,7 +992,7 @@ class RoomCreationHandler: for invite_3pid in invite_3pid_list: id_server = invite_3pid["id_server"] - id_access_token = invite_3pid.get("id_access_token") # optional + id_access_token = invite_3pid["id_access_token"] address = invite_3pid["address"] medium = invite_3pid["medium"] # Note that do_3pid_invite can raise a ShadowBanError, but this was diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index e726997d83..5d4adf5bfd 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -1382,7 +1382,7 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): id_server: str, requester: Requester, txn_id: Optional[str], - id_access_token: Optional[str] = None, + id_access_token: str, prev_event_ids: Optional[List[str]] = None, depth: Optional[int] = None, ) -> Tuple[str, int]: @@ -1397,7 +1397,7 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): requester: The user making the request. txn_id: The transaction ID this is part of, or None if this is not part of a transaction. - id_access_token: The optional identity server access token. + id_access_token: Identity server access token. depth: Override the depth used to order the event in the DAG. prev_event_ids: The event IDs to use as the prev events Should normally be set to None, which will cause the depth to be calculated @@ -1494,7 +1494,7 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): room_id: str, user: UserID, txn_id: Optional[str], - id_access_token: Optional[str] = None, + id_access_token: str, prev_event_ids: Optional[List[str]] = None, depth: Optional[int] = None, ) -> Tuple[EventBase, int]: diff --git a/synapse/rest/client/room.py b/synapse/rest/client/room.py index 0e2834008e..0bca012535 100644 --- a/synapse/rest/client/room.py +++ b/synapse/rest/client/room.py @@ -17,6 +17,7 @@ import logging import re from enum import Enum +from http import HTTPStatus from typing import TYPE_CHECKING, Awaitable, Dict, List, Optional, Tuple from urllib import parse as urlparse @@ -947,7 +948,16 @@ class RoomMembershipRestServlet(TransactionRestServlet): # cheekily send invalid bodies. content = {} - if membership_action == "invite" and self._has_3pid_invite_keys(content): + if membership_action == "invite" and all( + key in content for key in ("medium", "address") + ): + if not all(key in content for key in ("id_server", "id_access_token")): + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "`id_server` and `id_access_token` are required when doing 3pid invite", + Codes.MISSING_PARAM, + ) + try: await self.room_member_handler.do_3pid_invite( room_id, @@ -957,7 +967,7 @@ class RoomMembershipRestServlet(TransactionRestServlet): content["id_server"], requester, txn_id, - content.get("id_access_token"), + content["id_access_token"], ) except ShadowBanError: # Pretend the request succeeded. @@ -994,12 +1004,6 @@ class RoomMembershipRestServlet(TransactionRestServlet): return 200, return_value - def _has_3pid_invite_keys(self, content: JsonDict) -> bool: - for key in {"id_server", "medium", "address"}: - if key not in content: - return False - return True - def on_PUT( self, request: SynapseRequest, room_id: str, membership_action: str, txn_id: str ) -> Awaitable[Tuple[int, JsonDict]]: diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index 7435fd9130..9dd3c8d4bb 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -64,7 +64,6 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) - # How often to run the background job to update the "recently accessed" # attribute of local and remote media. UPDATE_RECENTLY_ACCESSED_TS = 60 * 1000 # 1 minute diff --git a/tests/rest/client/test_identity.py b/tests/rest/client/test_identity.py index dc17c9d113..b0c8215744 100644 --- a/tests/rest/client/test_identity.py +++ b/tests/rest/client/test_identity.py @@ -25,7 +25,6 @@ from tests import unittest class IdentityTestCase(unittest.HomeserverTestCase): - servlets = [ synapse.rest.admin.register_servlets_for_client_rest_resource, room.register_servlets, @@ -33,7 +32,6 @@ class IdentityTestCase(unittest.HomeserverTestCase): ] def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: - config = self.default_config() config["enable_3pid_lookup"] = False self.hs = self.setup_test_homeserver(config=config) @@ -54,6 +52,7 @@ class IdentityTestCase(unittest.HomeserverTestCase): "id_server": "testis", "medium": "email", "address": "test@example.com", + "id_access_token": tok, } request_url = ("/rooms/%s/invite" % (room_id)).encode("ascii") channel = self.make_request( diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index aa2f578441..c7eb88d33f 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -3461,3 +3461,21 @@ class ThreepidInviteTestCase(unittest.HomeserverTestCase): # Also check that it stopped before calling _make_and_store_3pid_invite. make_invite_mock.assert_called_once() + + def test_400_missing_param_without_id_access_token(self) -> None: + """ + Test that a 3pid invite request returns 400 M_MISSING_PARAM + if we do not include id_access_token. + """ + channel = self.make_request( + method="POST", + path="/rooms/" + self.room_id + "/invite", + content={ + "id_server": "example.com", + "medium": "email", + "address": "teresa@example.com", + }, + access_token=self.tok, + ) + self.assertEqual(channel.code, 400) + self.assertEqual(channel.json_body["errcode"], "M_MISSING_PARAM") diff --git a/tests/rest/client/test_shadow_banned.py b/tests/rest/client/test_shadow_banned.py index c50f034b34..c807a37bc2 100644 --- a/tests/rest/client/test_shadow_banned.py +++ b/tests/rest/client/test_shadow_banned.py @@ -97,7 +97,12 @@ class RoomTestCase(_ShadowBannedBase): channel = self.make_request( "POST", "/rooms/%s/invite" % (room_id,), - {"id_server": "test", "medium": "email", "address": "test@test.test"}, + { + "id_server": "test", + "medium": "email", + "address": "test@test.test", + "id_access_token": "anytoken", + }, access_token=self.banned_access_token, ) self.assertEqual(200, channel.code, channel.result) |