From e7ab5a187ee2997a37976a29398f96185fe96e52 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 25 Jul 2024 09:38:32 -0400 Subject: [PATCH 01/25] Implement MSC4133 to support custom profile fields. --- synapse/api/constants.py | 5 + synapse/config/experimental.py | 3 + synapse/handlers/profile.py | 121 +++++++++++++++++- synapse/handlers/sso.py | 2 +- synapse/module_api/__init__.py | 4 +- synapse/rest/client/profile.py | 106 +++++++++++++-- synapse/storage/databases/main/profile.py | 60 ++++++++- .../delta/85/07_custom_profile_fields.sql | 21 +++ tests/rest/client/test_profile.py | 83 ++++++++++++ 9 files changed, 386 insertions(+), 19 deletions(-) create mode 100644 synapse/storage/schema/main/delta/85/07_custom_profile_fields.sql diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 12d18137e07..60995835e72 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -306,3 +306,8 @@ class ApprovalNoticeMedium: class Direction(enum.Enum): BACKWARDS = "b" FORWARDS = "f" + + +class ProfileFields: + DISPLAYNAME: Final = "displayname" + AVATAR_URL: Final = "avatar_url" diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index bae9cc80476..7adf6cc6b30 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -448,3 +448,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # MSC4156: Migrate server_name to via self.msc4156_enabled: bool = experimental.get("msc4156_enabled", False) + + # MSC4133: Custom profile fields + self.msc4133_enabled: bool = experimental.get("msc4133_enabled", False) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 6663d4b271b..669802d9fec 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -22,6 +22,7 @@ import random from typing import TYPE_CHECKING, List, Optional, Union +from synapse.api.constants import ProfileFields from synapse.api.errors import ( AuthError, Codes, @@ -42,6 +43,9 @@ MAX_DISPLAYNAME_LEN = 256 MAX_AVATAR_URL_LEN = 1000 +# Field name length is specced at 255, value is server controlled. +MAX_CUSTOM_FIELD_LEN = 255 +MAX_CUSTOM_VALUE_LEN = 255 class ProfileHandler: @@ -78,13 +82,27 @@ async def get_profile(self, user_id: str, ignore_backoff: bool = True) -> JsonDi if self.hs.is_mine(target_user): profileinfo = await self.store.get_profileinfo(target_user) - if profileinfo.display_name is None and profileinfo.avatar_url is None: + extra_fields = {} + if self.hs.config.experimental.msc4133_enabled: + extra_fields = await self.store.get_profile_fields(target_user) + + if ( + profileinfo.display_name is None + and profileinfo.avatar_url is None + and not extra_fields + ): raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) - return { - "displayname": profileinfo.display_name, - "avatar_url": profileinfo.avatar_url, - } + # TODO Should this strip out empty values? + ret = {} + if profileinfo.display_name is not None: + ret[ProfileFields.DISPLAYNAME] = profileinfo.display_name + if profileinfo.avatar_url is not None: + ret[ProfileFields.AVATAR_URL] = profileinfo.avatar_url + if extra_fields: + ret.update(extra_fields) + + return ret else: try: result = await self.federation.make_query( @@ -370,6 +388,84 @@ async def check_avatar_size_and_mime_type(self, mxc: str) -> bool: return True + async def get_profile_field( + self, target_user: UserID, field_name: str + ) -> Optional[str]: + if self.hs.is_mine(target_user): + try: + field_value = await self.store.get_profile_field( + target_user, field_name + ) + except StoreError as e: + if e.code == 404: + raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) + raise + + return field_value + else: + # TODO This should be an unstable query. + try: + result = await self.federation.make_query( + destination=target_user.domain, + query_type="profile", + args={"user_id": target_user.to_string(), "field": field_name}, + ignore_backoff=True, + ) + except RequestSendFailed as e: + raise SynapseError(502, "Failed to fetch profile") from e + except HttpResponseException as e: + raise e.to_synapse_error() + + return result.get(field_name) + + async def set_profile_field( + self, + target_user: UserID, + requester: Requester, + field_name: str, + new_value: str, + by_admin: bool = False, + deactivation: bool = False, + ) -> None: + """Set a new avatar URL for a user. + + Args: + target_user: the user whose avatar URL is to be changed. + requester: The user attempting to make this change. + field_name: The name of the profile field to update. + new_value: The new field value for this user. + by_admin: Whether this change was made by an administrator. + deactivation: Whether this change was made while deactivating the user. + propagate: Whether this change also applies to the user's membership events. + """ + if not self.hs.is_mine(target_user): + raise SynapseError(400, "User is not hosted on this homeserver") + + if not by_admin and target_user != requester.user: + raise AuthError(400, "Cannot set another user's avatar_url") + + if not isinstance(new_value, str): + raise SynapseError( + 400, f"'{field_name}' must be a string", errcode=Codes.INVALID_PARAM + ) + + if not await self.check_avatar_size_and_mime_type(new_value): + raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN) + + # Same like set_displayname + if by_admin: + requester = create_requester( + target_user, authenticated_entity=requester.authenticated_entity + ) + + await self.store.set_profile_field(target_user, field_name, new_value) + + # Custom fields do not propagate into the user directory *or* rooms. + profile = await self.store.get_profileinfo(target_user) + await self._third_party_rules.on_profile_update( + target_user.to_string(), profile, by_admin, deactivation + ) + async def on_profile_query(self, args: JsonDict) -> JsonDict: """Handles federation profile query requests.""" @@ -388,11 +484,22 @@ async def on_profile_query(self, args: JsonDict) -> JsonDict: response = {} try: - if just_field is None or just_field == "displayname": + if just_field is None or just_field == ProfileFields.DISPLAYNAME: response["displayname"] = await self.store.get_profile_displayname(user) - if just_field is None or just_field == "avatar_url": + if just_field is None or just_field == ProfileFields.AVATAR_URL: response["avatar_url"] = await self.store.get_profile_avatar_url(user) + + if self.hs.config.experimental.msc4133_enabled: + if just_field is None: + response.update(await self.store.get_profile_fields(user)) + elif just_field not in ( + ProfileFields.DISPLAYNAME, + ProfileFields.AVATAR_URL, + ): + response[just_field] = await self.store.get_profile_field( + user, just_field + ) except StoreError as e: if e.code == 404: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index ee74289b6c4..d5df70af7f5 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -813,7 +813,7 @@ def is_allowed_mime_type(content_type: str) -> bool: # bail if user already has the same avatar profile = await self._profile_handler.get_profile(user_id) - if profile["avatar_url"] is not None: + if "avatar_url" in profile: server_name = profile["avatar_url"].split("/")[-2] media_id = profile["avatar_url"].split("/")[-1] if self._is_mine_server_name(server_name): diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index f6bfd93d3ce..87973a26a2b 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -1110,10 +1110,10 @@ async def update_room_membership( # Set the profile where it needs to be set. if "avatar_url" not in content: - content["avatar_url"] = profile["avatar_url"] + content["avatar_url"] = profile.get("avatar_url") if "displayname" not in content: - content["displayname"] = profile["displayname"] + content["displayname"] = profile.get("displayname") event_id, _ = await self._hs.get_room_member_handler().update_membership( requester=requester, diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index c1a80c5c3d5..6a8022c8398 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -21,10 +21,13 @@ """ This module contains REST servlets to do with profile: /profile/ """ +import re from http import HTTPStatus from typing import TYPE_CHECKING, Tuple +from synapse.api.constants import ProfileFields from synapse.api.errors import Codes, SynapseError +from synapse.handlers.profile import MAX_CUSTOM_FIELD_LEN, MAX_CUSTOM_VALUE_LEN from synapse.http.server import HttpServer from synapse.http.servlet import ( RestServlet, @@ -227,19 +230,106 @@ async def on_GET( user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) - displayname = await self.profile_handler.get_displayname(user) - avatar_url = await self.profile_handler.get_avatar_url(user) + return 200, await self.profile_handler.get_profile(user_id) - ret = {} - if displayname is not None: - ret["displayname"] = displayname - if avatar_url is not None: - ret["avatar_url"] = avatar_url - return 200, ret +class UnstableProfileRestServlet(RestServlet): + PATTERNS = [ + re.compile( + r"^/_matrix/client/unstable/uk\.tcpip\.msc4133/profile/(?P[^/]*)/(?P[^/]*)" + ) + ] + CATEGORY = "Event sending requests" + + def __init__(self, hs: "HomeServer"): + super().__init__() + self.hs = hs + self.profile_handler = hs.get_profile_handler() + self.auth = hs.get_auth() + + async def on_GET( + self, request: SynapseRequest, user_id: str, field_name: str + ) -> Tuple[int, JsonDict]: + requester_user = None + + if self.hs.config.server.require_auth_for_profile_requests: + requester = await self.auth.get_user_by_req(request) + requester_user = requester.user + + if not UserID.is_valid(user_id): + raise SynapseError( + HTTPStatus.BAD_REQUEST, "Invalid user id", Codes.INVALID_PARAM + ) + + user = UserID.from_string(user_id) + await self.profile_handler.check_profile_query_allowed(user, requester_user) + + if field_name == ProfileFields.DISPLAYNAME: + field_value = await self.profile_handler.get_displayname(user) + elif field_name == ProfileFields.AVATAR_URL: + field_value = await self.profile_handler.get_avatar_url(user) + else: + field_value = await self.profile_handler.get_profile_field(user, field_name) + + return 200, {field_name: field_value} + + async def on_PUT( + self, request: SynapseRequest, user_id: str, field_name: str + ) -> Tuple[int, JsonDict]: + requester = await self.auth.get_user_by_req(request) + user = UserID.from_string(user_id) + is_admin = await self.auth.is_server_admin(requester) + + if len(field_name) > MAX_CUSTOM_FIELD_LEN: + raise SynapseError(400, "Field name too long", errcode=Codes.INVALID_PARAM) + + content = parse_json_object_from_request(request) + try: + new_value = content[field_name] + except KeyError: + raise SynapseError( + 400, f"Missing key '{field_name}'", errcode=Codes.MISSING_PARAM + ) + + if not isinstance(new_value, str): + raise SynapseError(400, "Invalid field value", errcode=Codes.INVALID_PARAM) + if len(new_value) > MAX_CUSTOM_VALUE_LEN: + raise SynapseError(400, "Field value too long", errcode=Codes.INVALID_PARAM) + + propagate = _read_propagate(self.hs, request) + + requester_suspended = ( + await self.hs.get_datastores().main.get_user_suspended_status( + requester.user.to_string() + ) + ) + + if requester_suspended: + raise SynapseError( + 403, + "Updating avatar URL while account is suspended is not allowed.", + Codes.USER_ACCOUNT_SUSPENDED, + ) + + if field_name == ProfileFields.DISPLAYNAME: + await self.profile_handler.set_displayname( + user, requester, new_value, is_admin, propagate=propagate + ) + elif field_name == ProfileFields.AVATAR_URL: + await self.profile_handler.set_avatar_url( + user, requester, new_value, is_admin, propagate=propagate + ) + else: + await self.profile_handler.set_profile_field( + user, requester, field_name, new_value, is_admin + ) + + return 200, {} def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: ProfileDisplaynameRestServlet(hs).register(http_server) ProfileAvatarURLRestServlet(hs).register(http_server) ProfileRestServlet(hs).register(http_server) + if hs.config.experimental.msc4133_enabled: + UnstableProfileRestServlet(hs).register(http_server) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 996aea808de..98bc8ab97fc 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -18,8 +18,9 @@ # [This file includes modifications made by New Vector Limited] # # -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, Dict, List, Optional, Tuple, cast +from synapse.api.errors import StoreError from synapse.storage._base import SQLBaseStore from synapse.storage.database import ( DatabasePool, @@ -173,6 +174,45 @@ async def get_profile_avatar_url(self, user_id: UserID) -> Optional[str]: desc="get_profile_avatar_url", ) + async def get_profile_field( + self, user_id: UserID, field_name: str + ) -> Optional[str]: + """ + Get a custom profile field for a user. + + Args: + user_id: The user's ID. + field_name: The custom profile filed name. + + Returns: + The string value if the field exists, otherwise raises 404. + """ + result = await self.get_profile_fields(user_id) + if field_name not in result: + raise StoreError(404, "No row found") + return result[field_name] + + async def get_profile_fields(self, user_id: UserID) -> Dict[str, str]: + """ + Get all custom profile fields for a user. + + Args: + user_id: The user's ID. + + Returns: + A dictionary of custom profile fields. + """ + result = cast( + List[Tuple[str, str]], + await self.db_pool.simple_select_list( + table="profile_fields", + keyvalues={"user_id": user_id.to_string()}, + retcols=("name", "value"), + desc="get_profile_fields", + ), + ) + return dict(result) + async def create_profile(self, user_id: UserID) -> None: user_localpart = user_id.localpart await self.db_pool.simple_insert( @@ -222,6 +262,24 @@ async def set_profile_avatar_url( desc="set_profile_avatar_url", ) + async def set_profile_field( + self, user_id: UserID, field_name: str, new_value: str + ) -> None: + """ + Set a custom profile field for a user. + + Args: + user_id: The user's ID. + field_name: The name of the custom profile field. + new_value: The value of the custom profile field. + """ + await self.db_pool.simple_upsert( + table="profile_fields", + keyvalues={"user_id": user_id.to_string(), "name": field_name}, + values={"value": new_value}, + desc="set_profile_field", + ) + class ProfileStore(ProfileWorkerStore): pass diff --git a/synapse/storage/schema/main/delta/85/07_custom_profile_fields.sql b/synapse/storage/schema/main/delta/85/07_custom_profile_fields.sql new file mode 100644 index 00000000000..5f132f3b0a1 --- /dev/null +++ b/synapse/storage/schema/main/delta/85/07_custom_profile_fields.sql @@ -0,0 +1,21 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2024 New Vector, Ltd +-- +-- This program is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as +-- published by the Free Software Foundation, either version 3 of the +-- License, or (at your option) any later version. +-- +-- See the GNU Affero General Public License for more details: +-- . + +-- Custom profile fields. +CREATE TABLE profile_fields ( + user_id TEXT NOT NULL, + name TEXT NOT NULL, + value TEXT NOT NULL +); + +CREATE UNIQUE INDEX profile_fields_user_name ON profile_fields (user_id, name); diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index f98f3f77aae..15aedb4fb8b 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -479,6 +479,89 @@ def test_msc4069_inhibit_propagation_like_default(self) -> None: # The client requested ?propagate=true, so it should have happened. self.assertEqual(channel.json_body.get(prop), "http://my.server/pic.gif") + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_get_missing_custom_field(self) -> None: + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + ) + self.assertEqual(channel.code, HTTPStatus.NOT_FOUND, channel.result) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_get_custom_field_rejects_bad_username(self) -> None: + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{urllib.parse.quote('@alice:')}/custom_field", + ) + self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field(self) -> None: + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + content={"custom_field": "test"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual(channel.json_body, {"custom_field": "test"}) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field_noauth(self) -> None: + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + content={"custom_field": "test"}, + ) + self.assertEqual(channel.code, 401, channel.result) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field_too_long(self) -> None: + """Attempts to set a stupid custom_field should get a 400""" + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + content={"custom_field" * 100: "test"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + content={"custom_field": "test" * 100}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field_invalid(self) -> None: + """Attempts to set an invalid value for custom_field should get a 400""" + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + content={"custom_field": {"test": "bar"}}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field_other(self) -> None: + """Setting someone else's profile field should fail""" + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.other}/custom_field", + content={"custom_field": "test"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]) -> None: """Stores metadata about files in the database. From a80c0ebc1a8b6692f1a082055f165c7365e639df Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 25 Jul 2024 16:23:10 -0400 Subject: [PATCH 02/25] Newsfragment --- changelog.d/17488.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17488.feature diff --git a/changelog.d/17488.feature b/changelog.d/17488.feature new file mode 100644 index 00000000000..15cccf3ac22 --- /dev/null +++ b/changelog.d/17488.feature @@ -0,0 +1 @@ +Implement [MSC4133](https://github.com/matrix-org/matrix-spec-proposals/pull/4133) for custom profile fields. From dcacf350da5fb0c1b657f9e4acba8153af459224 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 25 Jul 2024 16:27:16 -0400 Subject: [PATCH 03/25] Move delta. --- .../01_custom_profile_fields.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename synapse/storage/schema/main/delta/{85/07_custom_profile_fields.sql => 86/01_custom_profile_fields.sql} (100%) diff --git a/synapse/storage/schema/main/delta/85/07_custom_profile_fields.sql b/synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql similarity index 100% rename from synapse/storage/schema/main/delta/85/07_custom_profile_fields.sql rename to synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql From 60b3f0060d3ca663cc9eb6516c9ff3b7954b9ebf Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 31 Jul 2024 07:25:20 -0400 Subject: [PATCH 04/25] Copy & paste error. Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- synapse/rest/client/profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 6a8022c8398..28bc2d4a683 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -307,7 +307,7 @@ async def on_PUT( if requester_suspended: raise SynapseError( 403, - "Updating avatar URL while account is suspended is not allowed.", + "Updating profile information while account is suspended is not allowed.", Codes.USER_ACCOUNT_SUSPENDED, ) From d2ab9c9cdcead5e565551c77aeab0d62dcf13ac9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 31 Jul 2024 07:25:56 -0400 Subject: [PATCH 05/25] Clarify docstring. Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- tests/rest/client/test_profile.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 15aedb4fb8b..41497bbda14 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -523,7 +523,9 @@ def test_set_custom_field_noauth(self) -> None: @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_too_long(self) -> None: - """Attempts to set a stupid custom_field should get a 400""" + """Attempts to set a custom_field name or value that is too + long should get a 400 + """ channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", From ce405ab676a0636a4cf0b9f44cfa7ec35b1a62b6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 31 Jul 2024 07:26:06 -0400 Subject: [PATCH 06/25] Correct license. --- .../storage/schema/main/delta/86/01_custom_profile_fields.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql b/synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql index 5f132f3b0a1..7cb7cf486fb 100644 --- a/synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql +++ b/synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql @@ -1,7 +1,7 @@ -- -- This file is licensed under the Affero General Public License (AGPL) version 3. -- --- Copyright (C) 2024 New Vector, Ltd +-- Copyright (C) 2024 Patrick Cloke -- -- This program is free software: you can redistribute it and/or modify -- it under the terms of the GNU Affero General Public License as From e4f9d7e7cb93b9699e8a91e81a017680c0187e53 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 31 Jul 2024 07:26:17 -0400 Subject: [PATCH 07/25] Fix typo. Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- synapse/storage/databases/main/profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 98bc8ab97fc..b5484d6a43d 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -182,7 +182,7 @@ async def get_profile_field( Args: user_id: The user's ID. - field_name: The custom profile filed name. + field_name: The custom profile field name. Returns: The string value if the field exists, otherwise raises 404. From 2c28bd5096a518c929da191e5c6af89813d5ca2e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 9 Aug 2024 16:00:40 -0400 Subject: [PATCH 08/25] Add more methods. --- synapse/handlers/profile.py | 41 ++++- synapse/rest/client/capabilities.py | 5 + synapse/rest/client/profile.py | 213 +++++++++++++++++++++- synapse/storage/databases/main/profile.py | 14 ++ tests/rest/client/test_profile.py | 98 ++++++++++ 5 files changed, 358 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 669802d9fec..d035d0acaab 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -436,29 +436,50 @@ async def set_profile_field( new_value: The new field value for this user. by_admin: Whether this change was made by an administrator. deactivation: Whether this change was made while deactivating the user. - propagate: Whether this change also applies to the user's membership events. """ if not self.hs.is_mine(target_user): raise SynapseError(400, "User is not hosted on this homeserver") if not by_admin and target_user != requester.user: - raise AuthError(400, "Cannot set another user's avatar_url") + raise AuthError(400, "Cannot set another user's profile") if not isinstance(new_value, str): raise SynapseError( 400, f"'{field_name}' must be a string", errcode=Codes.INVALID_PARAM ) - if not await self.check_avatar_size_and_mime_type(new_value): - raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN) + await self.store.set_profile_field(target_user, field_name, new_value) - # Same like set_displayname - if by_admin: - requester = create_requester( - target_user, authenticated_entity=requester.authenticated_entity - ) + # Custom fields do not propagate into the user directory *or* rooms. + profile = await self.store.get_profileinfo(target_user) + await self._third_party_rules.on_profile_update( + target_user.to_string(), profile, by_admin, deactivation + ) - await self.store.set_profile_field(target_user, field_name, new_value) + async def delete_profile_field( + self, + target_user: UserID, + requester: Requester, + field_name: str, + by_admin: bool = False, + deactivation: bool = False, + ) -> None: + """Set a new avatar URL for a user. + + Args: + target_user: the user whose avatar URL is to be changed. + requester: The user attempting to make this change. + field_name: The name of the profile field to update. + by_admin: Whether this change was made by an administrator. + deactivation: Whether this change was made while deactivating the user. + """ + if not self.hs.is_mine(target_user): + raise SynapseError(400, "User is not hosted on this homeserver") + + if not by_admin and target_user != requester.user: + raise AuthError(400, "Cannot set another user's profile") + + await self.store.delete_profile_field(target_user, field_name) # Custom fields do not propagate into the user directory *or* rooms. profile = await self.store.get_profileinfo(target_user) diff --git a/synapse/rest/client/capabilities.py b/synapse/rest/client/capabilities.py index 63b8a9364a4..096f33eac92 100644 --- a/synapse/rest/client/capabilities.py +++ b/synapse/rest/client/capabilities.py @@ -92,6 +92,11 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: "enabled": self.config.experimental.msc3664_enabled, } + if self.config.experimental.msc4133_enabled: + response["capabilities"]["k.tcpip.msc4133.profile_fields"] = { + True, + } + return HTTPStatus.OK, response diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 6a8022c8398..dc914a1cd62 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -230,10 +230,175 @@ async def on_GET( user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) + # TODO REVERT CHANGES + return 200, await self.profile_handler.get_profile(user_id) class UnstableProfileRestServlet(RestServlet): + PATTERNS = [ + re.compile( + r"^/_matrix/client/unstable/uk\.tcpip\.msc4133/profile/(?P[^/]*)" + ) + ] + CATEGORY = "Event sending requests" + + def __init__(self, hs: "HomeServer"): + super().__init__() + self.hs = hs + self.profile_handler = hs.get_profile_handler() + self.auth = hs.get_auth() + + async def on_GET( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + requester_user = None + + if self.hs.config.server.require_auth_for_profile_requests: + requester = await self.auth.get_user_by_req(request) + requester_user = requester.user + + if not UserID.is_valid(user_id): + raise SynapseError( + HTTPStatus.BAD_REQUEST, "Invalid user id", Codes.INVALID_PARAM + ) + + user = UserID.from_string(user_id) + await self.profile_handler.check_profile_query_allowed(user, requester_user) + + return 200, await self.profile_handler.get_profile(user_id) + + async def on_PATCH( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + requester = await self.auth.get_user_by_req(request) + user = UserID.from_string(user_id) + is_admin = await self.auth.is_server_admin(requester) + + content = parse_json_object_from_request(request) + + for field_name, new_value in content.items(): + if len(field_name) > MAX_CUSTOM_FIELD_LEN: + raise SynapseError( + 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE + ) + + if not isinstance(new_value, str): + raise SynapseError( + 400, + f"Invalid value for field: {field_name}", + errcode=Codes.INVALID_PARAM, + ) + # TODO REJECT IF OVERALL STORAGE IS TOO MUCH. + if len(new_value) > MAX_CUSTOM_VALUE_LEN: + raise SynapseError( + 400, "Field value too long", errcode=Codes.INVALID_PARAM + ) + + propagate = _read_propagate(self.hs, request) + + requester_suspended = ( + await self.hs.get_datastores().main.get_user_suspended_status( + requester.user.to_string() + ) + ) + + if requester_suspended: + raise SynapseError( + 403, + "Updating profile while account is suspended is not allowed.", + Codes.USER_ACCOUNT_SUSPENDED, + ) + + # TODO This isn't idempotent. + for field_name, new_value in content.items(): + if field_name == ProfileFields.DISPLAYNAME: + await self.profile_handler.set_displayname( + user, requester, new_value, is_admin, propagate=propagate + ) + elif field_name == ProfileFields.AVATAR_URL: + await self.profile_handler.set_avatar_url( + user, requester, new_value, is_admin, propagate=propagate + ) + else: + await self.profile_handler.set_profile_field( + user, requester, field_name, new_value, is_admin + ) + + return 200, {} + + async def on_POST( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + requester = await self.auth.get_user_by_req(request) + user = UserID.from_string(user_id) + is_admin = await self.auth.is_server_admin(requester) + + content = parse_json_object_from_request(request) + + for field_name, new_value in content.items(): + if len(field_name) > MAX_CUSTOM_FIELD_LEN: + raise SynapseError( + 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE + ) + + if not isinstance(new_value, str): + raise SynapseError( + 400, + f"Invalid value for field: {field_name}", + errcode=Codes.INVALID_PARAM, + ) + # TODO REJECT IF OVERALL STORAGE IS TOO MUCH. + if len(new_value) > MAX_CUSTOM_VALUE_LEN: + raise SynapseError( + 400, "Field value too long", errcode=Codes.INVALID_PARAM + ) + + propagate = _read_propagate(self.hs, request) + + requester_suspended = ( + await self.hs.get_datastores().main.get_user_suspended_status( + requester.user.to_string() + ) + ) + + if requester_suspended: + raise SynapseError( + 403, + "Updating profile while account is suspended is not allowed.", + Codes.USER_ACCOUNT_SUSPENDED, + ) + + # TODO This isn't idempotent. + + # Any fields which were not provided need to be deleted. + current_fields = await self.profile_handler.get_profile(user_id) + for field_name in (ProfileFields.DISPLAYNAME, ProfileFields.AVATAR_URL): + # If no new value is given, then set the value to empty to delete it. + if field_name not in content: + content[field_name] = "" + for field_name in current_fields.keys() - content.keys(): + await self.profile_handler.delete_profile_field(user, requester, field_name) + + # Remaining fields are created or updated. + for field_name, new_value in content.items(): + if field_name == ProfileFields.DISPLAYNAME: + await self.profile_handler.set_displayname( + user, requester, new_value, is_admin, propagate=propagate + ) + elif field_name == ProfileFields.AVATAR_URL: + await self.profile_handler.set_avatar_url( + user, requester, new_value, is_admin, propagate=propagate + ) + else: + await self.profile_handler.set_profile_field( + user, requester, field_name, new_value, is_admin + ) + + return 200, {} + + +class UnstableProfileFieldRestServlet(RestServlet): PATTERNS = [ re.compile( r"^/_matrix/client/unstable/uk\.tcpip\.msc4133/profile/(?P[^/]*)/(?P[^/]*)" @@ -281,7 +446,7 @@ async def on_PUT( is_admin = await self.auth.is_server_admin(requester) if len(field_name) > MAX_CUSTOM_FIELD_LEN: - raise SynapseError(400, "Field name too long", errcode=Codes.INVALID_PARAM) + raise SynapseError(400, "Field name too long", errcode=Codes.TOO_LARGE) content = parse_json_object_from_request(request) try: @@ -292,7 +457,8 @@ async def on_PUT( ) if not isinstance(new_value, str): - raise SynapseError(400, "Invalid field value", errcode=Codes.INVALID_PARAM) + raise SynapseError(400, "Invalid value", errcode=Codes.INVALID_PARAM) + # TODO REJECT IF OVERALL STORAGE IS TOO MUCH. if len(new_value) > MAX_CUSTOM_VALUE_LEN: raise SynapseError(400, "Field value too long", errcode=Codes.INVALID_PARAM) @@ -307,7 +473,7 @@ async def on_PUT( if requester_suspended: raise SynapseError( 403, - "Updating avatar URL while account is suspended is not allowed.", + "Updating profile while account is suspended is not allowed.", Codes.USER_ACCOUNT_SUSPENDED, ) @@ -326,10 +492,51 @@ async def on_PUT( return 200, {} + async def on_DELETE( + self, request: SynapseRequest, user_id: str, field_name: str + ) -> Tuple[int, JsonDict]: + requester = await self.auth.get_user_by_req(request) + user = UserID.from_string(user_id) + is_admin = await self.auth.is_server_admin(requester) + + if len(field_name) > MAX_CUSTOM_FIELD_LEN: + raise SynapseError(400, "Field name too long", errcode=Codes.TOO_LARGE) + + propagate = _read_propagate(self.hs, request) + + requester_suspended = ( + await self.hs.get_datastores().main.get_user_suspended_status( + requester.user.to_string() + ) + ) + + if requester_suspended: + raise SynapseError( + 403, + "Updating profile while account is suspended is not allowed.", + Codes.USER_ACCOUNT_SUSPENDED, + ) + + if field_name == ProfileFields.DISPLAYNAME: + await self.profile_handler.set_displayname( + user, requester, "", is_admin, propagate=propagate + ) + elif field_name == ProfileFields.AVATAR_URL: + await self.profile_handler.set_avatar_url( + user, requester, "", is_admin, propagate=propagate + ) + else: + await self.profile_handler.delete_profile_field( + user, requester, field_name, is_admin + ) + + return 200, {} + def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: ProfileDisplaynameRestServlet(hs).register(http_server) ProfileAvatarURLRestServlet(hs).register(http_server) ProfileRestServlet(hs).register(http_server) if hs.config.experimental.msc4133_enabled: + UnstableProfileFieldRestServlet(hs).register(http_server) UnstableProfileRestServlet(hs).register(http_server) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 98bc8ab97fc..50fc7e18481 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -280,6 +280,20 @@ async def set_profile_field( desc="set_profile_field", ) + async def delete_profile_field(self, user_id: UserID, field_name: str) -> None: + """ + Set a custom profile field for a user. + + Args: + user_id: The user's ID. + field_name: The name of the custom profile field. + """ + await self.db_pool.simple_delete( + table="profile_fields", + keyvalues={"user_id": user_id.to_string(), "name": field_name}, + desc="delete_profile_field", + ) + class ProfileStore(ProfileWorkerStore): pass diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 15aedb4fb8b..06a0edcdc87 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -512,6 +512,22 @@ def test_set_custom_field(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK, channel.result) self.assertEqual(channel.json_body, {"custom_field": "test"}) + # Overwriting the field should work. + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + content={"custom_field": "new_Value"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual(channel.json_body, {"custom_field": "new_Value"}) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_noauth(self) -> None: channel = self.make_request( @@ -551,6 +567,88 @@ def test_set_custom_field_invalid(self) -> None: ) self.assertEqual(channel.code, 400, channel.result) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field_displayname(self) -> None: + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/displayname", + content={"displayname": "test"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + displayname = self._get_displayname() + self.assertEqual(displayname, "test") + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field_avatar_url(self) -> None: + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/avatar_url", + content={"avatar_url": "mxc://test/good"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + avatar_url = self._get_avatar_url() + self.assertEqual(avatar_url, "mxc://test/good") + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_fields(self) -> None: + channel = self.make_request( + "POST", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + content={ + "avatar_url": "mxc://test/good", + "displayname": "test", + "custom": "foo", + }, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual( + channel.json_body, + { + "avatar_url": "mxc://test/good", + "displayname": "test", + "custom": "foo", + }, + ) + + # Update some fields. + channel = self.make_request( + "PATCH", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + content={ + "avatar_url": "mxc://test/second", + "displayname": "new_name", + "new_field": "test", + }, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual( + channel.json_body, + { + "avatar_url": "mxc://test/second", + "displayname": "new_name", + "new_field": "test", + "custom": "foo", + }, + ) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_other(self) -> None: """Setting someone else's profile field should fail""" From 8030ba29f4dbcb58c62b84f514cbe2612ba4df10 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 9 Aug 2024 16:02:21 -0400 Subject: [PATCH 09/25] Add comment. --- synapse/rest/client/profile.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index dc914a1cd62..d44f7a80a02 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -534,6 +534,8 @@ async def on_DELETE( def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: + # The specific displayname / avatar URL / custom field endpoints *must* appear + # before their corresponding generic profile endpoint. ProfileDisplaynameRestServlet(hs).register(http_server) ProfileAvatarURLRestServlet(hs).register(http_server) ProfileRestServlet(hs).register(http_server) From 735e8ccafb8393966f41de00e6d7198fc0b54230 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 9 Aug 2024 16:07:14 -0400 Subject: [PATCH 10/25] Get tests passing. --- synapse/http/servlet.py | 2 +- synapse/rest/client/profile.py | 13 ++++++++++--- tests/rest/client/test_profile.py | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index 08b8ff7afd4..a466553b283 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -964,7 +964,7 @@ def register(self, http_server: HttpServer) -> None: """Register this servlet with the given HTTP server.""" patterns = getattr(self, "PATTERNS", None) if patterns: - for method in ("GET", "PUT", "POST", "DELETE"): + for method in ("GET", "PUT", "POST", "DELETE", "PATCH"): if hasattr(self, "on_%s" % (method,)): servlet_classname = self.__class__.__name__ method_handler = getattr(self, "on_%s" % (method,)) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index d44f7a80a02..2aa1c75a530 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -230,9 +230,16 @@ async def on_GET( user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) - # TODO REVERT CHANGES + displayname = await self.profile_handler.get_displayname(user) + avatar_url = await self.profile_handler.get_avatar_url(user) - return 200, await self.profile_handler.get_profile(user_id) + ret = {} + if displayname is not None: + ret["displayname"] = displayname + if avatar_url is not None: + ret["avatar_url"] = avatar_url + + return 200, ret class UnstableProfileRestServlet(RestServlet): @@ -327,7 +334,7 @@ async def on_PATCH( return 200, {} - async def on_POST( + async def on_PUT( self, request: SynapseRequest, user_id: str ) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index db6ea3732fc..d5dcf260747 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -598,7 +598,7 @@ def test_set_custom_field_avatar_url(self) -> None: @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_fields(self) -> None: channel = self.make_request( - "POST", + "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", content={ "avatar_url": "mxc://test/good", From 8d9b74ea53e1367ae5f19afc8c5f5d6608ea95a3 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 12 Aug 2024 16:30:29 -0400 Subject: [PATCH 11/25] Review comments. --- synapse/handlers/profile.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index d035d0acaab..26c88f3491f 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -78,6 +78,7 @@ def __init__(self, hs: "HomeServer"): self._third_party_rules = hs.get_module_api_callbacks().third_party_event_rules async def get_profile(self, user_id: str, ignore_backoff: bool = True) -> JsonDict: + """Fetch a users full profile, including display name, avatar URL, and any custom fields.""" target_user = UserID.from_string(user_id) if self.hs.is_mine(target_user): @@ -93,7 +94,7 @@ async def get_profile(self, user_id: str, ignore_backoff: bool = True) -> JsonDi ): raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) - # TODO Should this strip out empty values? + # Do not include display name or avatar are denoted if unset. ret = {} if profileinfo.display_name is not None: ret[ProfileFields.DISPLAYNAME] = profileinfo.display_name @@ -391,6 +392,17 @@ async def check_avatar_size_and_mime_type(self, mxc: str) -> bool: async def get_profile_field( self, target_user: UserID, field_name: str ) -> Optional[str]: + """ + Fetch a user's profile from the database for local users and over federation + for remote users. + + Args: + target_user: The user ID to fetch the profile for. + field_name: The field to fetch the profile for. + + Returns: + The value for the profile field or None if the field does not exist. + """ if self.hs.is_mine(target_user): try: field_value = await self.store.get_profile_field( @@ -403,7 +415,6 @@ async def get_profile_field( return field_value else: - # TODO This should be an unstable query. try: result = await self.federation.make_query( destination=target_user.domain, @@ -427,7 +438,7 @@ async def set_profile_field( by_admin: bool = False, deactivation: bool = False, ) -> None: - """Set a new avatar URL for a user. + """Set a new profile field for a user. Args: target_user: the user whose avatar URL is to be changed. From 02b852a0f0c2ec35933b791c4e23b5a21eaeb7c1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 5 Sep 2024 13:29:37 -0400 Subject: [PATCH 12/25] Handle TODO about limiting overall size. --- synapse/rest/client/profile.py | 3 - synapse/storage/databases/main/profile.py | 128 ++++++++++++++++++---- tests/rest/client/test_profile.py | 35 +++++- 3 files changed, 142 insertions(+), 24 deletions(-) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index d378de57c85..a3f067826b1 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -296,7 +296,6 @@ async def on_PATCH( f"Invalid value for field: {field_name}", errcode=Codes.INVALID_PARAM, ) - # TODO REJECT IF OVERALL STORAGE IS TOO MUCH. if len(new_value) > MAX_CUSTOM_VALUE_LEN: raise SynapseError( 400, "Field value too long", errcode=Codes.INVALID_PARAM @@ -355,7 +354,6 @@ async def on_PUT( f"Invalid value for field: {field_name}", errcode=Codes.INVALID_PARAM, ) - # TODO REJECT IF OVERALL STORAGE IS TOO MUCH. if len(new_value) > MAX_CUSTOM_VALUE_LEN: raise SynapseError( 400, "Field value too long", errcode=Codes.INVALID_PARAM @@ -465,7 +463,6 @@ async def on_PUT( if not isinstance(new_value, str): raise SynapseError(400, "Invalid value", errcode=Codes.INVALID_PARAM) - # TODO REJECT IF OVERALL STORAGE IS TOO MUCH. if len(new_value) > MAX_CUSTOM_VALUE_LEN: raise SynapseError(400, "Field value too long", errcode=Codes.INVALID_PARAM) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index fc3fe72b175..bb3b4c71c21 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -20,6 +20,7 @@ # from typing import TYPE_CHECKING, Dict, List, Optional, Tuple, cast +from synapse.api.constants import ProfileFields from synapse.api.errors import StoreError from synapse.storage._base import SQLBaseStore from synapse.storage.database import ( @@ -35,6 +36,10 @@ from synapse.server import HomeServer +# The number of bytes that the serialized profile can have. +MAX_PROFILE_SIZE = 65536 + + class ProfileWorkerStore(SQLBaseStore): def __init__( self, @@ -255,6 +260,60 @@ async def create_profile(self, user_id: UserID) -> None: desc="create_profile", ) + def _check_profile_size( + self, txn: LoggingTransaction, user_id: str, new_field_name: str, new_value: str + ) -> None: + """ + pass + """ + # Start with 2 bytes for the braces. + total_bytes = 2 + # For each entry there are 4 quotes (2 each for key and value), 1 colon, + # and 1 comma. + PER_VALUE_EXTRA = 6 + profile_info = self.db_pool.simple_select_one_txn( + txn, + table="profiles", + keyvalues={"full_user_id": user_id}, + retcols=("displayname", "avatar_url"), + allow_none=True, + ) + if profile_info: + display_name, avatar_url = profile_info + if display_name: + total_bytes += len("displayname") + len(display_name) + PER_VALUE_EXTRA + if avatar_url: + total_bytes += len("avatar_url") + len(avatar_url) + PER_VALUE_EXTRA + + # Add the size of the current custom profile fields, ignoring the entry + # which will be overwritten. + if isinstance(txn.database_engine, PostgresEngine): + size_sql = """ + SELECT SUM(CHAR_LENGTH(name)) + SUM(CHAR_LENGTH(value)), COUNT(name) AS total_bytes + FROM profile_fields + WHERE + user_id = ? AND name != ? + """ + else: + size_sql = """ + SELECT SUM(LENGTH(name)) + SUM(LENGTH(value)), COUNT(name) AS total_bytes + FROM profile_fields + WHERE + user_id = ? AND name != ? + """ + txn.execute(size_sql, (user_id, new_field_name)) + row = cast(Tuple[Optional[int], int], txn.fetchone()) + total_bytes += (row[0] or 0) + row[1] * PER_VALUE_EXTRA + + # Add the length of the field being added. + total_bytes += len(new_field_name) + len(new_value) + PER_VALUE_EXTRA + + # There has been an over count of 1 via an additional comma. + total_bytes -= 1 + + if total_bytes > MAX_PROFILE_SIZE: + raise StoreError(400, "Profile too large") + async def set_profile_displayname( self, user_id: UserID, new_displayname: Optional[str] ) -> None: @@ -267,14 +326,25 @@ async def set_profile_displayname( name is removed. """ user_localpart = user_id.localpart - await self.db_pool.simple_upsert( - table="profiles", - keyvalues={"user_id": user_localpart}, - values={ - "displayname": new_displayname, - "full_user_id": user_id.to_string(), - }, - desc="set_profile_displayname", + + def set_profile_displayname(txn: LoggingTransaction) -> None: + if new_displayname is not None: + self._check_profile_size( + txn, user_id.to_string(), ProfileFields.DISPLAYNAME, new_displayname + ) + + self.db_pool.simple_upsert_txn( + txn, + table="profiles", + keyvalues={"user_id": user_localpart}, + values={ + "displayname": new_displayname, + "full_user_id": user_id.to_string(), + }, + ) + + await self.db_pool.runInteraction( + "set_profile_displayname", set_profile_displayname ) async def set_profile_avatar_url( @@ -289,11 +359,25 @@ async def set_profile_avatar_url( removed. """ user_localpart = user_id.localpart - await self.db_pool.simple_upsert( - table="profiles", - keyvalues={"user_id": user_localpart}, - values={"avatar_url": new_avatar_url, "full_user_id": user_id.to_string()}, - desc="set_profile_avatar_url", + + def set_profile_avatar_url(txn: LoggingTransaction) -> None: + if new_avatar_url is not None: + self._check_profile_size( + txn, user_id.to_string(), ProfileFields.AVATAR_URL, new_avatar_url + ) + + self.db_pool.simple_upsert_txn( + txn, + table="profiles", + keyvalues={"user_id": user_localpart}, + values={ + "avatar_url": new_avatar_url, + "full_user_id": user_id.to_string(), + }, + ) + + await self.db_pool.runInteraction( + "set_profile_avatar_url", set_profile_avatar_url ) async def set_profile_field( @@ -307,12 +391,18 @@ async def set_profile_field( field_name: The name of the custom profile field. new_value: The value of the custom profile field. """ - await self.db_pool.simple_upsert( - table="profile_fields", - keyvalues={"user_id": user_id.to_string(), "name": field_name}, - values={"value": new_value}, - desc="set_profile_field", - ) + + def set_profile_field(txn: LoggingTransaction) -> None: + self._check_profile_size(txn, user_id.to_string(), field_name, new_value) + + self.db_pool.simple_upsert_txn( + txn, + table="profile_fields", + keyvalues={"user_id": user_id.to_string(), "name": field_name}, + values={"value": new_value}, + ) + + await self.db_pool.runInteraction("set_profile_field", set_profile_field) async def delete_profile_field(self, user_id: UserID, field_name: str) -> None: """ diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 2ab4e674234..6ff8ccc538c 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -540,9 +540,10 @@ def test_set_custom_field_noauth(self) -> None: @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_too_long(self) -> None: - """Attempts to set a custom_field name or value that is too - long should get a 400 """ + Attempts to set a custom field name or value that is too long should get a 400 error. + """ + # Single key is too large. channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", @@ -551,6 +552,7 @@ def test_set_custom_field_too_long(self) -> None: ) self.assertEqual(channel.code, 400, channel.result) + # Single value is too large. channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", @@ -559,6 +561,35 @@ def test_set_custom_field_too_long(self) -> None: ) self.assertEqual(channel.code, 400, channel.result) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_custom_field_profile_too_long(self) -> None: + """ + Attempts to set a custom field that would push the overall profile too large. + """ + # The maximum key and values size. + key = "a" * 255 + # The first 126 work OK, len("owner") + (255 + 255 + 6) * 126 < 65536. + for i in range(126): + # Create a unique key. + key = f"{i:03}" + "a" * 252 + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + content={key: "a" * 255}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + # The next one should fail. + key = "b" * 255 + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + content={key: "a" * 255}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_invalid(self) -> None: """Attempts to set an invalid value for custom_field should get a 400""" From 00ad9113934fdcabff5ae82fc0676a9ff3331601 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 6 Sep 2024 07:45:31 -0400 Subject: [PATCH 13/25] Ensure fields exist. --- synapse/rest/client/profile.py | 3 +++ tests/rest/client/test_profile.py | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index a3f067826b1..42faaba3232 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -450,6 +450,9 @@ async def on_PUT( user = UserID.from_string(user_id) is_admin = await self.auth.is_server_admin(requester) + if not field_name: + raise SynapseError(400, "Field name too short", errcode=Codes.TOO_LARGE) + if len(field_name) > MAX_CUSTOM_FIELD_LEN: raise SynapseError(400, "Field name too long", errcode=Codes.TOO_LARGE) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 6ff8ccc538c..92d21d6790b 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -539,10 +539,19 @@ def test_set_custom_field_noauth(self) -> None: self.assertEqual(channel.code, 401, channel.result) @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) - def test_set_custom_field_too_long(self) -> None: + def test_set_custom_field_size(self) -> None: """ Attempts to set a custom field name or value that is too long should get a 400 error. """ + # Key is missing. + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/", + content={"" * 100: "test"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + # Single key is too large. channel = self.make_request( "PUT", From 87ff9f82c4ce6713406f75ec5fd161908e7c739f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 9 Sep 2024 14:21:11 -0400 Subject: [PATCH 14/25] Fix-up capabilities. --- synapse/rest/client/capabilities.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/capabilities.py b/synapse/rest/client/capabilities.py index 096f33eac92..63a83b5068b 100644 --- a/synapse/rest/client/capabilities.py +++ b/synapse/rest/client/capabilities.py @@ -93,8 +93,8 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: } if self.config.experimental.msc4133_enabled: - response["capabilities"]["k.tcpip.msc4133.profile_fields"] = { - True, + response["capabilities"]["uk.tcpip.msc4133.profile_fields"] = { + "enabled": True, } return HTTPStatus.OK, response From 4b44da31e3aaa6b8eb4351ebb3a1d33e1d09477d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 10 Sep 2024 17:04:14 -0400 Subject: [PATCH 15/25] Add to /versions. --- synapse/rest/client/versions.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/rest/client/versions.py b/synapse/rest/client/versions.py index 874869dc2d7..b3afb644143 100644 --- a/synapse/rest/client/versions.py +++ b/synapse/rest/client/versions.py @@ -175,6 +175,8 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: "org.matrix.msc4151": self.config.experimental.msc4151_enabled, # Simplified sliding sync "org.matrix.simplified_msc3575": msc3575_enabled, + # Arbitrary key-value profile fields. + "uk.tcpip.msc4133": self.config.experimental.msc4133_enabled, }, }, ) From d28bf25b07bcd5b6ef7f84d9f6f61c63300e79c1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 10 Sep 2024 17:07:58 -0400 Subject: [PATCH 16/25] Rename delta. --- .../schema/main/delta/{86 => 87}/01_custom_profile_fields.sql | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename synapse/storage/schema/main/delta/{86 => 87}/01_custom_profile_fields.sql (100%) diff --git a/synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql b/synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql similarity index 100% rename from synapse/storage/schema/main/delta/86/01_custom_profile_fields.sql rename to synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql From 52fe59f642df29eaab6700286b413f5fd63b7075 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 9 Oct 2024 14:43:22 -0400 Subject: [PATCH 17/25] Updates from MSC. --- synapse/handlers/profile.py | 10 +-- synapse/rest/client/profile.py | 29 +------ synapse/storage/databases/main/profile.py | 58 ++++++++----- .../delta/87/01_custom_profile_fields.sql | 8 +- tests/rest/client/test_profile.py | 87 +++++++++++++++++++ 5 files changed, 125 insertions(+), 67 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 637c00cc877..f7e896f2d0f 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -32,7 +32,7 @@ SynapseError, ) from synapse.storage.databases.main.media_repository import LocalMedia, RemoteMedia -from synapse.types import JsonDict, Requester, UserID, create_requester +from synapse.types import JsonDict, Requester, UserID, create_requester, JsonValue from synapse.util.caches.descriptors import cached from synapse.util.stringutils import parse_and_validate_mxc_uri @@ -45,7 +45,6 @@ MAX_AVATAR_URL_LEN = 1000 # Field name length is specced at 255, value is server controlled. MAX_CUSTOM_FIELD_LEN = 255 -MAX_CUSTOM_VALUE_LEN = 255 class ProfileHandler: @@ -462,7 +461,7 @@ async def set_profile_field( target_user: UserID, requester: Requester, field_name: str, - new_value: str, + new_value: JsonValue, by_admin: bool = False, deactivation: bool = False, ) -> None: @@ -482,11 +481,6 @@ async def set_profile_field( if not by_admin and target_user != requester.user: raise AuthError(400, "Cannot set another user's profile") - if not isinstance(new_value, str): - raise SynapseError( - 400, f"'{field_name}' must be a string", errcode=Codes.INVALID_PARAM - ) - await self.store.set_profile_field(target_user, field_name, new_value) # Custom fields do not propagate into the user directory *or* rooms. diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 42faaba3232..44b206f9ff4 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -27,7 +27,7 @@ from synapse.api.constants import ProfileFields from synapse.api.errors import Codes, SynapseError -from synapse.handlers.profile import MAX_CUSTOM_FIELD_LEN, MAX_CUSTOM_VALUE_LEN +from synapse.handlers.profile import MAX_CUSTOM_FIELD_LEN from synapse.http.server import HttpServer from synapse.http.servlet import ( RestServlet, @@ -290,17 +290,6 @@ async def on_PATCH( 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE ) - if not isinstance(new_value, str): - raise SynapseError( - 400, - f"Invalid value for field: {field_name}", - errcode=Codes.INVALID_PARAM, - ) - if len(new_value) > MAX_CUSTOM_VALUE_LEN: - raise SynapseError( - 400, "Field value too long", errcode=Codes.INVALID_PARAM - ) - propagate = _read_propagate(self.hs, request) requester_suspended = ( @@ -348,17 +337,6 @@ async def on_PUT( 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE ) - if not isinstance(new_value, str): - raise SynapseError( - 400, - f"Invalid value for field: {field_name}", - errcode=Codes.INVALID_PARAM, - ) - if len(new_value) > MAX_CUSTOM_VALUE_LEN: - raise SynapseError( - 400, "Field value too long", errcode=Codes.INVALID_PARAM - ) - propagate = _read_propagate(self.hs, request) requester_suspended = ( @@ -464,11 +442,6 @@ async def on_PUT( 400, f"Missing key '{field_name}'", errcode=Codes.MISSING_PARAM ) - if not isinstance(new_value, str): - raise SynapseError(400, "Invalid value", errcode=Codes.INVALID_PARAM) - if len(new_value) > MAX_CUSTOM_VALUE_LEN: - raise SynapseError(400, "Field value too long", errcode=Codes.INVALID_PARAM) - propagate = _read_propagate(self.hs, request) requester_suspended = ( diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index bb3b4c71c21..9932047c263 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -20,6 +20,8 @@ # from typing import TYPE_CHECKING, Dict, List, Optional, Tuple, cast +from psycopg2.extras import Json + from synapse.api.constants import ProfileFields from synapse.api.errors import StoreError from synapse.storage._base import SQLBaseStore @@ -30,7 +32,7 @@ ) from synapse.storage.databases.main.roommember import ProfileInfo from synapse.storage.engines import PostgresEngine -from synapse.types import JsonDict, UserID +from synapse.types import JsonDict, UserID, JsonValue if TYPE_CHECKING: from synapse.server import HomeServer @@ -235,16 +237,13 @@ async def get_profile_fields(self, user_id: UserID) -> Dict[str, str]: Returns: A dictionary of custom profile fields. """ - result = cast( - List[Tuple[str, str]], - await self.db_pool.simple_select_list( - table="profile_fields", - keyvalues={"user_id": user_id.to_string()}, - retcols=("name", "value"), - desc="get_profile_fields", - ), + result = await self.db_pool.simple_select_one_onecol( + table="profiles", + keyvalues={"full_user_id": user_id.to_string()}, + retcol="fields", + desc="get_profile_fields", ) - return dict(result) + return result or {} async def create_profile(self, user_id: UserID) -> None: """ @@ -263,9 +262,9 @@ async def create_profile(self, user_id: UserID) -> None: def _check_profile_size( self, txn: LoggingTransaction, user_id: str, new_field_name: str, new_value: str ) -> None: - """ - pass - """ + # XXOXXXXXXXXXXX + return + # Start with 2 bytes for the braces. total_bytes = 2 # For each entry there are 4 quotes (2 each for key and value), 1 colon, @@ -381,7 +380,7 @@ def set_profile_avatar_url(txn: LoggingTransaction) -> None: ) async def set_profile_field( - self, user_id: UserID, field_name: str, new_value: str + self, user_id: UserID, field_name: str, new_value: JsonValue ) -> None: """ Set a custom profile field for a user. @@ -395,11 +394,15 @@ async def set_profile_field( def set_profile_field(txn: LoggingTransaction) -> None: self._check_profile_size(txn, user_id.to_string(), field_name, new_value) - self.db_pool.simple_upsert_txn( - txn, - table="profile_fields", - keyvalues={"user_id": user_id.to_string(), "name": field_name}, - values={"value": new_value}, + sql = """ + INSERT INTO profiles (user_id, full_user_id, fields) VALUES (?, ?, json_build_object(?, to_jsonb(?))) + ON CONFLICT (user_id) + DO UPDATE SET full_user_id = EXCLUDED.full_user_id, fields = COALESCE(profiles.fields, '{}'::jsonb) || EXCLUDED.fields + """ + txn.execute( + sql, + # TODO Does new_value need to be canonical_json encoded? + (user_id.localpart, user_id.to_string(), field_name, Json(new_value)), ) await self.db_pool.runInteraction("set_profile_field", set_profile_field) @@ -412,11 +415,18 @@ async def delete_profile_field(self, user_id: UserID, field_name: str) -> None: user_id: The user's ID. field_name: The name of the custom profile field. """ - await self.db_pool.simple_delete( - table="profile_fields", - keyvalues={"user_id": user_id.to_string(), "name": field_name}, - desc="delete_profile_field", - ) + + def delete_profile_field(txn: LoggingTransaction) -> None: + sql = """ + UPDATE profiles SET fields = fields - ? + WHERE user_id = ? + """ + txn.execute( + sql, + (field_name, user_id.localpart), + ) + + await self.db_pool.runInteraction("delete_profile_field", delete_profile_field) class ProfileStore(ProfileWorkerStore): diff --git a/synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql b/synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql index 7cb7cf486fb..63cbd7ffa99 100644 --- a/synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql +++ b/synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql @@ -12,10 +12,4 @@ -- . -- Custom profile fields. -CREATE TABLE profile_fields ( - user_id TEXT NOT NULL, - name TEXT NOT NULL, - value TEXT NOT NULL -); - -CREATE UNIQUE INDEX profile_fields_user_name ON profile_fields (user_id, name); +ALTER TABLE profiles ADD COLUMN fields JSONB; diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 92d21d6790b..9a13ac11da9 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -529,6 +529,93 @@ def test_set_custom_field(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK, channel.result) self.assertEqual(channel.json_body, {"custom_field": "new_Value"}) + # Deleting the field should work. + channel = self.make_request( + "DELETE", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + content={}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", + ) + self.assertEqual(channel.code, HTTPStatus.NOT_FOUND, channel.result) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_set_full_profile(self) -> None: + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + content={"custom_field": "test", "displayname": "blah"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual( + channel.json_body, {"custom_field": "test", "displayname": "blah"} + ) + + # Updating with PATCH should work. + channel = self.make_request( + "PATCH", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + content={"custom_field": "new_Value", "extra_field": "value"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual( + channel.json_body, + { + "custom_field": "new_Value", + "displayname": "blah", + "extra_field": "value", + }, + ) + + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_non_string(self) -> None: + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + content={ + "bool_field": True, + "array_field": ["test"], + "object_field": {"test": "test"}, + "numeric_field": 1, + }, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual( + channel.json_body, + { + "bool_field": True, + "array_field": ["test"], + "object_field": {"test": "test"}, + "numeric_field": 1, + }, + ) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_noauth(self) -> None: channel = self.make_request( From 1c98d966ad0d70dddf8264dd7d80443508e0c70f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 31 Oct 2024 15:15:14 -0400 Subject: [PATCH 18/25] Get postgres working again. --- synapse/handlers/profile.py | 6 +- synapse/rest/client/profile.py | 4 +- synapse/storage/databases/main/profile.py | 127 +++++++++++++--------- tests/rest/client/test_profile.py | 98 +++++++++++------ 4 files changed, 145 insertions(+), 90 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index f7e896f2d0f..210a4e0a874 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -32,7 +32,7 @@ SynapseError, ) from synapse.storage.databases.main.media_repository import LocalMedia, RemoteMedia -from synapse.types import JsonDict, Requester, UserID, create_requester, JsonValue +from synapse.types import JsonDict, JsonValue, Requester, UserID, create_requester from synapse.util.caches.descriptors import cached from synapse.util.stringutils import parse_and_validate_mxc_uri @@ -43,8 +43,8 @@ MAX_DISPLAYNAME_LEN = 256 MAX_AVATAR_URL_LEN = 1000 -# Field name length is specced at 255, value is server controlled. -MAX_CUSTOM_FIELD_LEN = 255 +# Field name length is specced at 128. +MAX_CUSTOM_FIELD_LEN = 128 class ProfileHandler: diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 44b206f9ff4..adb27dcf096 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -284,7 +284,7 @@ async def on_PATCH( content = parse_json_object_from_request(request) - for field_name, new_value in content.items(): + for field_name in content.keys(): if len(field_name) > MAX_CUSTOM_FIELD_LEN: raise SynapseError( 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE @@ -331,7 +331,7 @@ async def on_PUT( content = parse_json_object_from_request(request) - for field_name, new_value in content.items(): + for field_name in content.keys(): if len(field_name) > MAX_CUSTOM_FIELD_LEN: raise SynapseError( 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 9932047c263..866332a5047 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -18,9 +18,10 @@ # [This file includes modifications made by New Vector Limited] # # -from typing import TYPE_CHECKING, Dict, List, Optional, Tuple, cast +import json +from typing import TYPE_CHECKING, Dict, Optional, Tuple, cast -from psycopg2.extras import Json +from canonicaljson import encode_canonical_json from synapse.api.constants import ProfileFields from synapse.api.errors import StoreError @@ -32,7 +33,7 @@ ) from synapse.storage.databases.main.roommember import ProfileInfo from synapse.storage.engines import PostgresEngine -from synapse.types import JsonDict, UserID, JsonValue +from synapse.types import JsonDict, JsonValue, UserID if TYPE_CHECKING: from synapse.server import HomeServer @@ -260,55 +261,61 @@ async def create_profile(self, user_id: UserID) -> None: ) def _check_profile_size( - self, txn: LoggingTransaction, user_id: str, new_field_name: str, new_value: str + self, + txn: LoggingTransaction, + user_id: UserID, + new_field_name: str, + new_value: JsonValue, ) -> None: - # XXOXXXXXXXXXXX - return - - # Start with 2 bytes for the braces. - total_bytes = 2 # For each entry there are 4 quotes (2 each for key and value), 1 colon, # and 1 comma. PER_VALUE_EXTRA = 6 - profile_info = self.db_pool.simple_select_one_txn( - txn, - table="profiles", - keyvalues={"full_user_id": user_id}, - retcols=("displayname", "avatar_url"), - allow_none=True, - ) - if profile_info: - display_name, avatar_url = profile_info - if display_name: - total_bytes += len("displayname") + len(display_name) + PER_VALUE_EXTRA - if avatar_url: - total_bytes += len("avatar_url") + len(avatar_url) + PER_VALUE_EXTRA # Add the size of the current custom profile fields, ignoring the entry # which will be overwritten. if isinstance(txn.database_engine, PostgresEngine): size_sql = """ - SELECT SUM(CHAR_LENGTH(name)) + SUM(CHAR_LENGTH(value)), COUNT(name) AS total_bytes - FROM profile_fields + SELECT + OCTET_LENGTH((fields - ?)::text), OCTET_LENGTH(displayname), OCTET_LENGTH(avatar_url) + FROM profiles WHERE - user_id = ? AND name != ? + user_id = ? """ else: size_sql = """ SELECT SUM(LENGTH(name)) + SUM(LENGTH(value)), COUNT(name) AS total_bytes - FROM profile_fields + FROM profiles WHERE - user_id = ? AND name != ? + user_id = ? """ - txn.execute(size_sql, (user_id, new_field_name)) - row = cast(Tuple[Optional[int], int], txn.fetchone()) - total_bytes += (row[0] or 0) + row[1] * PER_VALUE_EXTRA - - # Add the length of the field being added. - total_bytes += len(new_field_name) + len(new_value) + PER_VALUE_EXTRA + txn.execute( + size_sql, + ( + new_field_name, + user_id.localpart, + ), + ) + row = cast(Tuple[Optional[int], Optional[int], Optional[int]], txn.fetchone()) + + # The values return null if the column is null. + total_bytes = ( + # Discount the opening and closing braces to avoid double counting, + # but add one for a comma. + (row[0] - 1 if row[0] else 0) + + ( + row[1] + len("displayname") + PER_VALUE_EXTRA + if new_field_name != ProfileFields.DISPLAYNAME and row[1] + else 0 + ) + + ( + row[2] + len("avatar_url") + PER_VALUE_EXTRA + if new_field_name != ProfileFields.AVATAR_URL and row[2] + else 0 + ) + ) - # There has been an over count of 1 via an additional comma. - total_bytes -= 1 + # Add the length of the field being added + the braces. + total_bytes += len(encode_canonical_json({new_field_name: new_value})) if total_bytes > MAX_PROFILE_SIZE: raise StoreError(400, "Profile too large") @@ -329,7 +336,7 @@ async def set_profile_displayname( def set_profile_displayname(txn: LoggingTransaction) -> None: if new_displayname is not None: self._check_profile_size( - txn, user_id.to_string(), ProfileFields.DISPLAYNAME, new_displayname + txn, user_id, ProfileFields.DISPLAYNAME, new_displayname ) self.db_pool.simple_upsert_txn( @@ -362,7 +369,7 @@ async def set_profile_avatar_url( def set_profile_avatar_url(txn: LoggingTransaction) -> None: if new_avatar_url is not None: self._check_profile_size( - txn, user_id.to_string(), ProfileFields.AVATAR_URL, new_avatar_url + txn, user_id, ProfileFields.AVATAR_URL, new_avatar_url ) self.db_pool.simple_upsert_txn( @@ -392,18 +399,31 @@ async def set_profile_field( """ def set_profile_field(txn: LoggingTransaction) -> None: - self._check_profile_size(txn, user_id.to_string(), field_name, new_value) + self._check_profile_size(txn, user_id, field_name, new_value) - sql = """ - INSERT INTO profiles (user_id, full_user_id, fields) VALUES (?, ?, json_build_object(?, to_jsonb(?))) - ON CONFLICT (user_id) - DO UPDATE SET full_user_id = EXCLUDED.full_user_id, fields = COALESCE(profiles.fields, '{}'::jsonb) || EXCLUDED.fields - """ - txn.execute( - sql, - # TODO Does new_value need to be canonical_json encoded? - (user_id.localpart, user_id.to_string(), field_name, Json(new_value)), - ) + if isinstance(self.database_engine, PostgresEngine): + from psycopg2.extras import Json + + sql = """ + INSERT INTO profiles (user_id, full_user_id, fields) VALUES (?, ?, json_build_object(?, ?::jsonb)) + ON CONFLICT (user_id) + DO UPDATE SET full_user_id = EXCLUDED.full_user_id, fields = COALESCE(profiles.fields, '{}'::jsonb) || EXCLUDED.fields + """ + + txn.execute( + sql, + ( + user_id.localpart, + user_id.to_string(), + field_name, + # encode to canonical JSON, but then pass as a JSON object + # since we have passing bytes disabled at the database driver + # level. + Json(json.loads(encode_canonical_json(new_value))), + ), + ) + else: + raise RuntimeError("Unsupported") await self.db_pool.runInteraction("set_profile_field", set_profile_field) @@ -417,10 +437,13 @@ async def delete_profile_field(self, user_id: UserID, field_name: str) -> None: """ def delete_profile_field(txn: LoggingTransaction) -> None: - sql = """ - UPDATE profiles SET fields = fields - ? - WHERE user_id = ? - """ + if isinstance(self.database_engine, PostgresEngine): + sql = """ + UPDATE profiles SET fields = fields - ? + WHERE user_id = ? + """ + else: + raise RuntimeError("Unsupported") txn.execute( sql, (field_name, user_id.localpart), diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 9a13ac11da9..3ccbf2e07cc 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -25,12 +25,15 @@ from http import HTTPStatus from typing import Any, Dict, Optional +from canonicaljson import encode_canonical_json + from twisted.test.proto_helpers import MemoryReactor from synapse.api.errors import Codes from synapse.rest import admin from synapse.rest.client import login, profile, room from synapse.server import HomeServer +from synapse.storage.databases.main.profile import MAX_PROFILE_SIZE from synapse.types import UserID from synapse.util import Clock @@ -628,13 +631,13 @@ def test_set_custom_field_noauth(self) -> None: @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_size(self) -> None: """ - Attempts to set a custom field name or value that is too long should get a 400 error. + Attempts to set a custom field name that is too long should get a 400 error. """ # Key is missing. channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/", - content={"" * 100: "test"}, + content={"": "test"}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 400, channel.result) @@ -643,16 +646,7 @@ def test_set_custom_field_size(self) -> None: channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", - content={"custom_field" * 100: "test"}, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 400, channel.result) - - # Single value is too large. - channel = self.make_request( - "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", - content={"custom_field": "test" * 100}, + content={"c" * 129: "test"}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 400, channel.result) @@ -662,41 +656,79 @@ def test_set_custom_field_profile_too_long(self) -> None: """ Attempts to set a custom field that would push the overall profile too large. """ - # The maximum key and values size. - key = "a" * 255 - # The first 126 work OK, len("owner") + (255 + 255 + 6) * 126 < 65536. - for i in range(126): - # Create a unique key. - key = f"{i:03}" + "a" * 252 - channel = self.make_request( - "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", - content={key: "a" * 255}, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) + # Get right to the boundary: + # len("displayname") + len("owner") + 5 = 21 for the displayname + # 1 + 65499 + 5 for key "a" = 65505 + # 2 braces, 1 comma + # 3 + 21 + 65505 = 65529 < 65536. + key = "a" + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + content={key: "a" * 65499}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) - # The next one should fail. - key = "b" * 255 + # Get the entire profile. + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + canonical_json = encode_canonical_json(channel.json_body) + # 6 is the minimum bytes to store a value: 2 quotes, 1 colon, 1 comma, a non-empty key. + # Be one below that so we can prove we're at the boundary. + self.assertEqual(len(canonical_json), MAX_PROFILE_SIZE - 7) + + # The next one should fail, note the value has a (JSON) length of 2. + key = "b" channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", - content={key: "a" * 255}, + content={key: 12}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 400, channel.result) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) - def test_set_custom_field_invalid(self) -> None: - """Attempts to set an invalid value for custom_field should get a 400""" + # Setting an avatar or (longer) display name should not work. channel = self.make_request( "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", - content={"custom_field": {"test": "bar"}}, + f"/profile/{self.owner}/displayname", + content={"displayname": "owner1234567"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + + channel = self.make_request( + "PUT", + f"/profile/{self.owner}/avatar_url", + content={"avatar_url": "mxc://foo/bar"}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 400, channel.result) + # Removing a single byte should work. + key = "b" + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + content={key: 1}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + # Finally, setting a field that already exists to a value that is <= in length should work. + key = "b" + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + content={key: 2}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_displayname(self) -> None: channel = self.make_request( From 749e27df5045151b091131bfb13198d0ab24b5e9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 1 Nov 2024 11:48:01 -0400 Subject: [PATCH 19/25] Working on sqlite again. --- synapse/storage/databases/main/profile.py | 78 +++++++++++++++++------ tests/rest/client/test_profile.py | 56 +++++++++++++--- 2 files changed, 104 insertions(+), 30 deletions(-) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 866332a5047..1be11c53fe1 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -32,7 +32,7 @@ LoggingTransaction, ) from synapse.storage.databases.main.roommember import ProfileInfo -from synapse.storage.engines import PostgresEngine +from synapse.storage.engines import PostgresEngine, Sqlite3Engine from synapse.types import JsonDict, JsonValue, UserID if TYPE_CHECKING: @@ -244,6 +244,9 @@ async def get_profile_fields(self, user_id: UserID) -> Dict[str, str]: retcol="fields", desc="get_profile_fields", ) + # The SQLite driver doesn't automatically convert JSON to + if isinstance(self.database_engine, Sqlite3Engine) and result: + result = json.loads(result) return result or {} async def create_profile(self, user_id: UserID) -> None: @@ -281,20 +284,23 @@ def _check_profile_size( WHERE user_id = ? """ + txn.execute( + size_sql, + (new_field_name, user_id.localpart), + ) else: size_sql = """ - SELECT SUM(LENGTH(name)) + SUM(LENGTH(value)), COUNT(name) AS total_bytes + SELECT + LENGTH(json_remove(fields, ?)), LENGTH(displayname), LENGTH(avatar_url) FROM profiles WHERE user_id = ? """ - txn.execute( - size_sql, - ( - new_field_name, - user_id.localpart, - ), - ) + txn.execute( + size_sql, + # This will error if field_name has double quotes in it. + (f'$."{new_field_name}"', user_id.localpart), + ) row = cast(Tuple[Optional[int], Optional[int], Optional[int]], txn.fetchone()) # The values return null if the column is null. @@ -398,14 +404,19 @@ async def set_profile_field( new_value: The value of the custom profile field. """ + # Encode to canonical JSON. + canonical_value = encode_canonical_json(new_value) + def set_profile_field(txn: LoggingTransaction) -> None: self._check_profile_size(txn, user_id, field_name, new_value) if isinstance(self.database_engine, PostgresEngine): from psycopg2.extras import Json + # Note that the || jsonb operator is not recursive, any duplicate + # keys will be taken from the second value. sql = """ - INSERT INTO profiles (user_id, full_user_id, fields) VALUES (?, ?, json_build_object(?, ?::jsonb)) + INSERT INTO profiles (user_id, full_user_id, fields) VALUES (?, ?, JSON_BUILD_OBJECT(?, ?::jsonb)) ON CONFLICT (user_id) DO UPDATE SET full_user_id = EXCLUDED.full_user_id, fields = COALESCE(profiles.fields, '{}'::jsonb) || EXCLUDED.fields """ @@ -416,14 +427,33 @@ def set_profile_field(txn: LoggingTransaction) -> None: user_id.localpart, user_id.to_string(), field_name, - # encode to canonical JSON, but then pass as a JSON object - # since we have passing bytes disabled at the database driver - # level. - Json(json.loads(encode_canonical_json(new_value))), + # Pass as a JSON object since we have passing bytes disabled + # at the database driver. + Json(json.loads(canonical_value)), ), ) else: - raise RuntimeError("Unsupported") + # You may be tempted to use json_patch instead of providing the parameters + # twice, but that recursively merges objects instead of replacing. + sql = """ + INSERT INTO profiles (user_id, full_user_id, fields) VALUES (?, ?, JSON_OBJECT(?, JSON(?))) + ON CONFLICT (user_id) + DO UPDATE SET full_user_id = EXCLUDED.full_user_id, fields = JSON_SET(COALESCE(profiles.fields, '{}'), ?, JSON(?)) + """ + # This will error if field_name has double quotes in it. + json_field_name = f'$."{field_name}"' + + txn.execute( + sql, + ( + user_id.localpart, + user_id.to_string(), + json_field_name, + canonical_value, + json_field_name, + canonical_value, + ), + ) await self.db_pool.runInteraction("set_profile_field", set_profile_field) @@ -442,12 +472,20 @@ def delete_profile_field(txn: LoggingTransaction) -> None: UPDATE profiles SET fields = fields - ? WHERE user_id = ? """ + txn.execute( + sql, + (field_name, user_id.localpart), + ) else: - raise RuntimeError("Unsupported") - txn.execute( - sql, - (field_name, user_id.localpart), - ) + sql = """ + UPDATE profiles SET fields = json_remove(fields, ?) + WHERE user_id = ? + """ + txn.execute( + sql, + # This will error if field_name has double quotes in it. + (f'$."{field_name}"', user_id.localpart), + ) await self.db_pool.runInteraction("delete_profile_field", delete_profile_field) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 3ccbf2e07cc..00f276afceb 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -38,6 +38,7 @@ from synapse.util import Clock from tests import unittest +from tests.utils import USE_POSTGRES_FOR_TESTS class ProfileTestCase(unittest.HomeserverTestCase): @@ -619,6 +620,35 @@ def test_non_string(self) -> None: }, ) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) + def test_full_replace(self) -> None: + """Setting a field which is an object twice should fully replace that field (not be merged).""" + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + content={"object_field": {"test": "test"}}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + content={"object_field": {"foo": "bar"}}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual( + channel.json_body, + {"object_field": {"foo": "bar"}}, + ) + @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_noauth(self) -> None: channel = self.make_request( @@ -658,14 +688,14 @@ def test_set_custom_field_profile_too_long(self) -> None: """ # Get right to the boundary: # len("displayname") + len("owner") + 5 = 21 for the displayname - # 1 + 65499 + 5 for key "a" = 65505 + # 1 + 65498 + 5 for key "a" = 65504 # 2 braces, 1 comma - # 3 + 21 + 65505 = 65529 < 65536. + # 3 + 21 + 65498 = 65522 < 65536. key = "a" channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", - content={key: "a" * 65499}, + content={key: "a" * 65498}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 200, channel.result) @@ -678,16 +708,22 @@ def test_set_custom_field_profile_too_long(self) -> None: ) self.assertEqual(channel.code, 200, channel.result) canonical_json = encode_canonical_json(channel.json_body) - # 6 is the minimum bytes to store a value: 2 quotes, 1 colon, 1 comma, a non-empty key. + # 6 is the minimum bytes to store a value: 4 quotes, 1 colon, 1 comma, an empty key. # Be one below that so we can prove we're at the boundary. - self.assertEqual(len(canonical_json), MAX_PROFILE_SIZE - 7) + self.assertEqual(len(canonical_json), MAX_PROFILE_SIZE - 8) + + # Postgres stores JSONB with whitespace, while SQLite doesn't. + if USE_POSTGRES_FOR_TESTS: + ADDITIONAL_CHARS = 0 + else: + ADDITIONAL_CHARS = 1 # The next one should fail, note the value has a (JSON) length of 2. key = "b" channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", - content={key: 12}, + content={key: "1" + "a" * ADDITIONAL_CHARS}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 400, channel.result) @@ -696,7 +732,7 @@ def test_set_custom_field_profile_too_long(self) -> None: channel = self.make_request( "PUT", f"/profile/{self.owner}/displayname", - content={"displayname": "owner1234567"}, + content={"displayname": "owner12345678" + "a" * ADDITIONAL_CHARS}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 400, channel.result) @@ -714,17 +750,17 @@ def test_set_custom_field_profile_too_long(self) -> None: channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", - content={key: 1}, + content={key: "" + "a" * ADDITIONAL_CHARS}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 200, channel.result) # Finally, setting a field that already exists to a value that is <= in length should work. - key = "b" + key = "a" channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", - content={key: 2}, + content={key: ""}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 200, channel.result) From 6936daff605446f85cbdc1485ebdca61249cfbd4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 1 Nov 2024 11:50:29 -0400 Subject: [PATCH 20/25] Move schema delta again. --- .../schema/main/delta/{87 => 88}/01_custom_profile_fields.sql | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename synapse/storage/schema/main/delta/{87 => 88}/01_custom_profile_fields.sql (100%) diff --git a/synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql b/synapse/storage/schema/main/delta/88/01_custom_profile_fields.sql similarity index 100% rename from synapse/storage/schema/main/delta/87/01_custom_profile_fields.sql rename to synapse/storage/schema/main/delta/88/01_custom_profile_fields.sql From 64fe2227e0e3d1d703bc5a579abfc26360f75dc6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 4 Nov 2024 16:22:20 -0500 Subject: [PATCH 21/25] Fix-up max length checks. --- synapse/handlers/profile.py | 4 ++-- synapse/rest/client/profile.py | 25 +++++++++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 210a4e0a874..271489f9453 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -43,8 +43,8 @@ MAX_DISPLAYNAME_LEN = 256 MAX_AVATAR_URL_LEN = 1000 -# Field name length is specced at 128. -MAX_CUSTOM_FIELD_LEN = 128 +# Field name length is specced at 255 bytes. +MAX_CUSTOM_FIELD_LEN = 255 class ProfileHandler: diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index adb27dcf096..24daef7c939 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -285,7 +285,12 @@ async def on_PATCH( content = parse_json_object_from_request(request) for field_name in content.keys(): - if len(field_name) > MAX_CUSTOM_FIELD_LEN: + if not field_name: + raise SynapseError( + 400, "Field name too short", errcode=Codes.INVALID_PARAM + ) + + if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: raise SynapseError( 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE ) @@ -332,7 +337,12 @@ async def on_PUT( content = parse_json_object_from_request(request) for field_name in content.keys(): - if len(field_name) > MAX_CUSTOM_FIELD_LEN: + if not field_name: + raise SynapseError( + 400, "Field name too short", errcode=Codes.INVALID_PARAM + ) + + if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: raise SynapseError( 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE ) @@ -429,10 +439,10 @@ async def on_PUT( is_admin = await self.auth.is_server_admin(requester) if not field_name: - raise SynapseError(400, "Field name too short", errcode=Codes.TOO_LARGE) + raise SynapseError(400, "Field name too short", errcode=Codes.INVALID_PARAM) - if len(field_name) > MAX_CUSTOM_FIELD_LEN: - raise SynapseError(400, "Field name too long", errcode=Codes.TOO_LARGE) + if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: + raise SynapseError(400, "Field name too long", errcode=Codes.INVALID_PARAM) content = parse_json_object_from_request(request) try: @@ -479,7 +489,10 @@ async def on_DELETE( user = UserID.from_string(user_id) is_admin = await self.auth.is_server_admin(requester) - if len(field_name) > MAX_CUSTOM_FIELD_LEN: + if not field_name: + raise SynapseError(400, "Field name too short", errcode=Codes.INVALID_PARAM) + + if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: raise SynapseError(400, "Field name too long", errcode=Codes.TOO_LARGE) propagate = _read_propagate(self.hs, request) From 6c777b22604fc5222e127051c65ea1a850eccc10 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 6 Nov 2024 10:47:37 -0500 Subject: [PATCH 22/25] Fix-up fetching individual fields. --- synapse/handlers/profile.py | 4 +- synapse/rest/client/profile.py | 4 +- synapse/storage/databases/main/profile.py | 56 ++++++++++++++++++++--- tests/rest/client/test_profile.py | 47 ++++++++++++------- 4 files changed, 84 insertions(+), 27 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 271489f9453..1f88145e48a 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -418,7 +418,7 @@ async def check_avatar_size_and_mime_type(self, mxc: str) -> bool: async def get_profile_field( self, target_user: UserID, field_name: str - ) -> Optional[str]: + ) -> JsonValue: """ Fetch a user's profile from the database for local users and over federation for remote users. @@ -536,7 +536,7 @@ async def on_profile_query(self, args: JsonDict) -> JsonDict: just_field = args.get("field", None) - response = {} + response: JsonDict = {} try: if just_field is None or just_field == ProfileFields.DISPLAYNAME: response["displayname"] = await self.store.get_profile_displayname(user) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 24daef7c939..cf8959cca35 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -36,7 +36,7 @@ ) from synapse.http.site import SynapseRequest from synapse.rest.client._base import client_patterns -from synapse.types import JsonDict, UserID +from synapse.types import JsonDict, JsonValue, UserID if TYPE_CHECKING: from synapse.server import HomeServer @@ -423,7 +423,7 @@ async def on_GET( await self.profile_handler.check_profile_query_allowed(user, requester_user) if field_name == ProfileFields.DISPLAYNAME: - field_value = await self.profile_handler.get_displayname(user) + field_value: JsonValue = await self.profile_handler.get_displayname(user) elif field_name == ProfileFields.AVATAR_URL: field_value = await self.profile_handler.get_avatar_url(user) else: diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 1be11c53fe1..5a753ab1aeb 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -210,9 +210,7 @@ async def get_profile_avatar_url(self, user_id: UserID) -> Optional[str]: desc="get_profile_avatar_url", ) - async def get_profile_field( - self, user_id: UserID, field_name: str - ) -> Optional[str]: + async def get_profile_field(self, user_id: UserID, field_name: str) -> JsonValue: """ Get a custom profile field for a user. @@ -223,10 +221,54 @@ async def get_profile_field( Returns: The string value if the field exists, otherwise raises 404. """ - result = await self.get_profile_fields(user_id) - if field_name not in result: - raise StoreError(404, "No row found") - return result[field_name] + + def get_profile_field(txn: LoggingTransaction) -> JsonValue: + # This will error if field_name has double quotes in it. + field_path = f'$."{field_name}"' + + if isinstance(self.database_engine, PostgresEngine): + sql = """ + SELECT JSONB_PATH_EXISTS(fields, ?), JSONB_EXTRACT_PATH(fields, ?) + FROM profiles + WHERE user_id = ? + """ + txn.execute( + sql, + (field_path, field_name, user_id.localpart), + ) + + # Test exists first since value being None is used for both + # missing and a null JSON value. + exists, value = cast(Tuple[bool, JsonValue], txn.fetchone()) + if not exists: + raise StoreError(404, "No row found") + return value + + else: + sql = """ + SELECT JSON_TYPE(fields, ?), JSON_EXTRACT(fields, ?) + FROM profiles + WHERE user_id = ? + """ + txn.execute( + sql, + (field_path, field_path, user_id.localpart), + ) + + # If value_type is None, then the value did not exist. + value_type, value = cast( + Tuple[Optional[str], JsonValue], txn.fetchone() + ) + if not value_type: + raise StoreError(404, "No row found") + # If value_type is object or array, then need to deserialize the JSON. + # Scalar values are properly returned directly. + if value_type in ("object", "array"): + assert isinstance(value, str) + return json.loads(value) + return value + + return await self.db_pool.runInteraction("get_profile_field", get_profile_field) async def get_profile_fields(self, user_id: UserID) -> Dict[str, str]: """ diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 00f276afceb..ba4f17b7f3d 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -592,15 +592,19 @@ def test_set_full_profile(self) -> None: @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_non_string(self) -> None: + """Non-string fields are supported for custom fields.""" + fields = { + "bool_field": True, + "array_field": ["test"], + "object_field": {"test": "test"}, + "numeric_field": 1, + "null_field": None, + } + channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content={ - "bool_field": True, - "array_field": ["test"], - "object_field": {"test": "test"}, - "numeric_field": 1, - }, + content=fields, access_token=self.owner_tok, ) self.assertEqual(channel.code, 200, channel.result) @@ -610,15 +614,16 @@ def test_non_string(self) -> None: f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", ) self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - self.assertEqual( - channel.json_body, - { - "bool_field": True, - "array_field": ["test"], - "object_field": {"test": "test"}, - "numeric_field": 1, - }, - ) + self.assertEqual(channel.json_body, fields) + + # Check getting individual fields works. + for key, value in fields.items(): + channel = self.make_request( + "GET", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual(channel.json_body, {key: value}) @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_full_replace(self) -> None: @@ -673,10 +678,20 @@ def test_set_custom_field_size(self) -> None: self.assertEqual(channel.code, 400, channel.result) # Single key is too large. + key = "c" * 500 + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + content={key: "test"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 400, channel.result) + + # Key doesn't match body. channel = self.make_request( "PUT", f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/custom_field", - content={"c" * 129: "test"}, + content={"diff_key": "test"}, access_token=self.owner_tok, ) self.assertEqual(channel.code, 400, channel.result) From 11cc48aab81e7ba8abd62ef4127dec17f0a034ed Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 5 Dec 2024 16:26:29 -0500 Subject: [PATCH 23/25] Remove the PATCH endpoint. --- synapse/rest/client/profile.py | 52 ------------------------------- tests/rest/client/test_profile.py | 51 ------------------------------ 2 files changed, 103 deletions(-) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index cf8959cca35..73632596535 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -275,58 +275,6 @@ async def on_GET( return 200, await self.profile_handler.get_profile(user_id) - async def on_PATCH( - self, request: SynapseRequest, user_id: str - ) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request) - user = UserID.from_string(user_id) - is_admin = await self.auth.is_server_admin(requester) - - content = parse_json_object_from_request(request) - - for field_name in content.keys(): - if not field_name: - raise SynapseError( - 400, "Field name too short", errcode=Codes.INVALID_PARAM - ) - - if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: - raise SynapseError( - 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE - ) - - propagate = _read_propagate(self.hs, request) - - requester_suspended = ( - await self.hs.get_datastores().main.get_user_suspended_status( - requester.user.to_string() - ) - ) - - if requester_suspended: - raise SynapseError( - 403, - "Updating profile while account is suspended is not allowed.", - Codes.USER_ACCOUNT_SUSPENDED, - ) - - # TODO This isn't idempotent. - for field_name, new_value in content.items(): - if field_name == ProfileFields.DISPLAYNAME: - await self.profile_handler.set_displayname( - user, requester, new_value, is_admin, propagate=propagate - ) - elif field_name == ProfileFields.AVATAR_URL: - await self.profile_handler.set_avatar_url( - user, requester, new_value, is_admin, propagate=propagate - ) - else: - await self.profile_handler.set_profile_field( - user, requester, field_name, new_value, is_admin - ) - - return 200, {} - async def on_PUT( self, request: SynapseRequest, user_id: str ) -> Tuple[int, JsonDict]: diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index ba4f17b7f3d..6f887a64117 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -567,29 +567,6 @@ def test_set_full_profile(self) -> None: channel.json_body, {"custom_field": "test", "displayname": "blah"} ) - # Updating with PATCH should work. - channel = self.make_request( - "PATCH", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content={"custom_field": "new_Value", "extra_field": "value"}, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) - - channel = self.make_request( - "GET", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - ) - self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - self.assertEqual( - channel.json_body, - { - "custom_field": "new_Value", - "displayname": "blah", - "extra_field": "value", - }, - ) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_non_string(self) -> None: """Non-string fields are supported for custom fields.""" @@ -834,34 +811,6 @@ def test_set_custom_fields(self) -> None: }, ) - # Update some fields. - channel = self.make_request( - "PATCH", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content={ - "avatar_url": "mxc://test/second", - "displayname": "new_name", - "new_field": "test", - }, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) - - channel = self.make_request( - "GET", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - ) - self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - self.assertEqual( - channel.json_body, - { - "avatar_url": "mxc://test/second", - "displayname": "new_name", - "new_field": "test", - "custom": "foo", - }, - ) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_other(self) -> None: """Setting someone else's profile field should fail""" From adb64d863b5c0d1fa85b8bbf18a9c55846b0f261 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 6 Dec 2024 14:12:36 -0500 Subject: [PATCH 24/25] Match the MSC error codes. --- synapse/api/errors.py | 4 ++++ synapse/rest/client/profile.py | 6 ++++-- synapse/storage/databases/main/profile.py | 4 ++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 21989b6e0e8..5dd6e84289a 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -132,6 +132,10 @@ class Codes(str, Enum): # connection. UNKNOWN_POS = "M_UNKNOWN_POS" + # Part of MSC4133 + PROFILE_TOO_LARGE = "M_PROFILE_TOO_LARGE" + KEY_TOO_LARGE = "M_KEY_TOO_LARGE" + class CodeMessageException(RuntimeError): """An exception with integer code, a message string attributes and optional headers. diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 73632596535..fed4e750b56 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -292,7 +292,9 @@ async def on_PUT( if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: raise SynapseError( - 400, f"Field name too long: {field_name}", errcode=Codes.TOO_LARGE + 400, + f"Field name too long: {field_name}", + errcode=Codes.KEY_TOO_LARGE, ) propagate = _read_propagate(self.hs, request) @@ -441,7 +443,7 @@ async def on_DELETE( raise SynapseError(400, "Field name too short", errcode=Codes.INVALID_PARAM) if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: - raise SynapseError(400, "Field name too long", errcode=Codes.TOO_LARGE) + raise SynapseError(400, "Field name too long", errcode=Codes.KEY_TOO_LARGE) propagate = _read_propagate(self.hs, request) diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 5a753ab1aeb..23e9baea485 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -24,7 +24,7 @@ from canonicaljson import encode_canonical_json from synapse.api.constants import ProfileFields -from synapse.api.errors import StoreError +from synapse.api.errors import Codes, StoreError from synapse.storage._base import SQLBaseStore from synapse.storage.database import ( DatabasePool, @@ -366,7 +366,7 @@ def _check_profile_size( total_bytes += len(encode_canonical_json({new_field_name: new_value})) if total_bytes > MAX_PROFILE_SIZE: - raise StoreError(400, "Profile too large") + raise StoreError(400, "Profile too large", Codes.PROFILE_TOO_LARGE) async def set_profile_displayname( self, user_id: UserID, new_displayname: Optional[str] From 2a327bfa410b3ae1dd963fe19beafd378c20c2d4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 20 Dec 2024 13:55:42 -0500 Subject: [PATCH 25/25] Remove PUT endpoint. --- synapse/rest/client/profile.py | 117 +++--------------------------- tests/rest/client/test_profile.py | 97 +++---------------------- 2 files changed, 21 insertions(+), 193 deletions(-) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index fed4e750b56..f513d6c138f 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -230,117 +230,21 @@ async def on_GET( user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) - displayname = await self.profile_handler.get_displayname(user) - avatar_url = await self.profile_handler.get_avatar_url(user) + if self.hs.config.experimental.msc4133_enabled: + ret = await self.profile_handler.get_profile(user_id) + else: + displayname = await self.profile_handler.get_displayname(user) + avatar_url = await self.profile_handler.get_avatar_url(user) - ret = {} - if displayname is not None: - ret["displayname"] = displayname - if avatar_url is not None: - ret["avatar_url"] = avatar_url + ret = {} + if displayname is not None: + ret["displayname"] = displayname + if avatar_url is not None: + ret["avatar_url"] = avatar_url return 200, ret -class UnstableProfileRestServlet(RestServlet): - PATTERNS = [ - re.compile( - r"^/_matrix/client/unstable/uk\.tcpip\.msc4133/profile/(?P[^/]*)" - ) - ] - CATEGORY = "Event sending requests" - - def __init__(self, hs: "HomeServer"): - super().__init__() - self.hs = hs - self.profile_handler = hs.get_profile_handler() - self.auth = hs.get_auth() - - async def on_GET( - self, request: SynapseRequest, user_id: str - ) -> Tuple[int, JsonDict]: - requester_user = None - - if self.hs.config.server.require_auth_for_profile_requests: - requester = await self.auth.get_user_by_req(request) - requester_user = requester.user - - if not UserID.is_valid(user_id): - raise SynapseError( - HTTPStatus.BAD_REQUEST, "Invalid user id", Codes.INVALID_PARAM - ) - - user = UserID.from_string(user_id) - await self.profile_handler.check_profile_query_allowed(user, requester_user) - - return 200, await self.profile_handler.get_profile(user_id) - - async def on_PUT( - self, request: SynapseRequest, user_id: str - ) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request) - user = UserID.from_string(user_id) - is_admin = await self.auth.is_server_admin(requester) - - content = parse_json_object_from_request(request) - - for field_name in content.keys(): - if not field_name: - raise SynapseError( - 400, "Field name too short", errcode=Codes.INVALID_PARAM - ) - - if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: - raise SynapseError( - 400, - f"Field name too long: {field_name}", - errcode=Codes.KEY_TOO_LARGE, - ) - - propagate = _read_propagate(self.hs, request) - - requester_suspended = ( - await self.hs.get_datastores().main.get_user_suspended_status( - requester.user.to_string() - ) - ) - - if requester_suspended: - raise SynapseError( - 403, - "Updating profile while account is suspended is not allowed.", - Codes.USER_ACCOUNT_SUSPENDED, - ) - - # TODO This isn't idempotent. - - # Any fields which were not provided need to be deleted. - current_fields = await self.profile_handler.get_profile(user_id) - for field_name in (ProfileFields.DISPLAYNAME, ProfileFields.AVATAR_URL): - # If no new value is given, then set the value to empty to delete it. - if field_name not in content: - content[field_name] = "" - for field_name in current_fields.keys() - content.keys(): - await self.profile_handler.delete_profile_field(user, requester, field_name) - - # Remaining fields are created or updated. - for field_name, new_value in content.items(): - if field_name == ProfileFields.DISPLAYNAME: - await self.profile_handler.set_displayname( - user, requester, new_value, is_admin, propagate=propagate - ) - elif field_name == ProfileFields.AVATAR_URL: - await self.profile_handler.set_avatar_url( - user, requester, new_value, is_admin, propagate=propagate - ) - else: - await self.profile_handler.set_profile_field( - user, requester, field_name, new_value, is_admin - ) - - return 200, {} - - class UnstableProfileFieldRestServlet(RestServlet): PATTERNS = [ re.compile( @@ -484,4 +388,3 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: ProfileRestServlet(hs).register(http_server) if hs.config.experimental.msc4133_enabled: UnstableProfileFieldRestServlet(hs).register(http_server) - UnstableProfileRestServlet(hs).register(http_server) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 6f887a64117..a9fb7ec591f 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -548,25 +548,6 @@ def test_set_custom_field(self) -> None: ) self.assertEqual(channel.code, HTTPStatus.NOT_FOUND, channel.result) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) - def test_set_full_profile(self) -> None: - channel = self.make_request( - "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content={"custom_field": "test", "displayname": "blah"}, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) - - channel = self.make_request( - "GET", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - ) - self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - self.assertEqual( - channel.json_body, {"custom_field": "test", "displayname": "blah"} - ) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_non_string(self) -> None: """Non-string fields are supported for custom fields.""" @@ -578,20 +559,21 @@ def test_non_string(self) -> None: "null_field": None, } - channel = self.make_request( - "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content=fields, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) + for key, value in fields.items(): + channel = self.make_request( + "PUT", + f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}/{key}", + content={key: value}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) channel = self.make_request( "GET", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + f"/_matrix/client/v3/profile/{self.owner}", ) self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - self.assertEqual(channel.json_body, fields) + self.assertEqual(channel.json_body, {"displayname": "owner", **fields}) # Check getting individual fields works. for key, value in fields.items(): @@ -602,35 +584,6 @@ def test_non_string(self) -> None: self.assertEqual(channel.code, HTTPStatus.OK, channel.result) self.assertEqual(channel.json_body, {key: value}) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) - def test_full_replace(self) -> None: - """Setting a field which is an object twice should fully replace that field (not be merged).""" - channel = self.make_request( - "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content={"object_field": {"test": "test"}}, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) - - channel = self.make_request( - "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content={"object_field": {"foo": "bar"}}, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) - - channel = self.make_request( - "GET", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - ) - self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - self.assertEqual( - channel.json_body, - {"object_field": {"foo": "bar"}}, - ) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_noauth(self) -> None: channel = self.make_request( @@ -695,7 +648,7 @@ def test_set_custom_field_profile_too_long(self) -> None: # Get the entire profile. channel = self.make_request( "GET", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", + f"/_matrix/client/v3/profile/{self.owner}", access_token=self.owner_tok, ) self.assertEqual(channel.code, 200, channel.result) @@ -783,34 +736,6 @@ def test_set_custom_field_avatar_url(self) -> None: avatar_url = self._get_avatar_url() self.assertEqual(avatar_url, "mxc://test/good") - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) - def test_set_custom_fields(self) -> None: - channel = self.make_request( - "PUT", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - content={ - "avatar_url": "mxc://test/good", - "displayname": "test", - "custom": "foo", - }, - access_token=self.owner_tok, - ) - self.assertEqual(channel.code, 200, channel.result) - - channel = self.make_request( - "GET", - f"/_matrix/client/unstable/uk.tcpip.msc4133/profile/{self.owner}", - ) - self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - self.assertEqual( - channel.json_body, - { - "avatar_url": "mxc://test/good", - "displayname": "test", - "custom": "foo", - }, - ) - @unittest.override_config({"experimental_features": {"msc4133_enabled": True}}) def test_set_custom_field_other(self) -> None: """Setting someone else's profile field should fail"""