diff --git a/changelog.d/19542.bugfix b/changelog.d/19542.bugfix new file mode 100644 index 0000000000..ab72504335 --- /dev/null +++ b/changelog.d/19542.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.26.0 that caused deactivated, erased users to not be removed from the user directory. \ No newline at end of file diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 9e0a1cb70c..72e0e8d91a 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -403,6 +403,7 @@ is set to `true`: - Remove the user's display name - Remove the user's avatar URL +- Remove the user's custom profile fields - Mark the user as erased The following actions are **NOT** performed. The list may be incomplete. diff --git a/docs/upgrade.md b/docs/upgrade.md index 0e2005f282..777e57c492 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -133,6 +133,19 @@ pip install systemd-python No action is needed if you do not use journal logging, or if you installed Synapse from the Debian packages (which handle this automatically). +## Module API: Deprecation of the `deactivation` parameter in the `set_displayname` method + +If you have Synapse modules installed that use the `set_displayname` method to change +the display name of your users, please ensure that it doesn't pass the optional +`deactivation` parameter. + +This parameter is now deprecated and it is intended to be removed in 2027. +No immediate change is necessary, however once the parameter is removed, modules passing it will produce errors. +[Issue #19546](https://github.com/element-hq/synapse/issues/19546) tracks this removal. + +From this version, when the parameter is passed, an error such as +``Deprecated `deactivation` parameter passed to `set_displayname` Module API (value: False). This will break in 2027.`` will be logged. The method will otherwise continue to work. + # Upgrading to v1.146.0 ## Drop support for Ubuntu 25.04 Plucky Puffin, and add support for 25.10 Questing Quokka diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index e4c646ce87..538bdaaaf8 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -167,13 +167,13 @@ class DeactivateAccountHandler: # Mark the user as erased, if they asked for that if erase_data: user = UserID.from_string(user_id) - # Remove avatar URL from this user - await self._profile_handler.set_avatar_url( - user, requester, "", by_admin, deactivation=True - ) - # Remove displayname from this user - await self._profile_handler.set_displayname( - user, requester, "", by_admin, deactivation=True + # Remove displayname, avatar URL and custom profile fields from this user + # + # Note that displayname and avatar URL may persist as historical state events + # in rooms, but these cases behave like message history, following + # https://spec.matrix.org/v1.17/client-server-api/#post_matrixclientv3accountdeactivate + await self._profile_handler.delete_profile_upon_deactivation( + user, requester, by_admin ) logger.info("Marking %s as erased", user_id) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 6bec901a79..d123bcdd36 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -35,6 +35,7 @@ from synapse.api.errors import ( SynapseError, ) from synapse.storage.databases.main.media_repository import LocalMedia, RemoteMedia +from synapse.storage.roommember import ProfileInfo from synapse.types import ( JsonDict, JsonMapping, @@ -193,18 +194,24 @@ class ProfileHandler: target_user: UserID, requester: Requester, new_displayname: str, + *, by_admin: bool = False, - deactivation: bool = False, propagate: bool = True, ) -> None: """Set the displayname of a user + Preconditions: + - This must NOT be called as part of deactivating the user, because we will + notify modules about the change whilst claiming it is not related + to user deactivation and we will also (if `propagate=True`) send + updates into rooms, which could cause rooms to be accidentally joined + after the deactivated user has left them. + Args: target_user: the user whose displayname is to be changed. requester: The user attempting to make this change. new_displayname: The displayname to give 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): @@ -248,12 +255,13 @@ class ProfileHandler: await self.store.set_profile_displayname(target_user, displayname_to_set) profile = await self.store.get_profileinfo(target_user) + await self.user_directory_handler.handle_local_profile_change( target_user.to_string(), profile ) await self._third_party_rules.on_profile_update( - target_user.to_string(), profile, by_admin, deactivation + target_user.to_string(), profile, by_admin, deactivation=False ) if propagate: @@ -297,18 +305,24 @@ class ProfileHandler: target_user: UserID, requester: Requester, new_avatar_url: str, + *, by_admin: bool = False, - deactivation: bool = False, propagate: bool = True, ) -> None: """Set a new avatar URL for a user. + Preconditions: + - This must NOT be called as part of deactivating the user, because we will + notify modules about the change whilst claiming it is not related + to user deactivation and we will also (if `propagate=True`) send + updates into rooms, which could cause rooms to be accidentally joined + after the deactivated user has left them. + Args: target_user: the user whose avatar URL is to be changed. requester: The user attempting to make this change. new_avatar_url: The avatar URL to give 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): @@ -350,17 +364,79 @@ class ProfileHandler: await self.store.set_profile_avatar_url(target_user, avatar_url_to_set) profile = await self.store.get_profileinfo(target_user) + await self.user_directory_handler.handle_local_profile_change( target_user.to_string(), profile ) await self._third_party_rules.on_profile_update( - target_user.to_string(), profile, by_admin, deactivation + target_user.to_string(), profile, by_admin, deactivation=False ) if propagate: await self._update_join_states(requester, target_user) + async def delete_profile_upon_deactivation( + self, + target_user: UserID, + requester: Requester, + by_admin: bool = False, + ) -> None: + """ + Clear the user's profile upon user deactivation (specifically, when user erasure is needed). + + This includes the displayname, avatar_url, all custom profile fields. + + The user directory is NOT updated in any way; it is the caller's responsibility to remove + the user from the user directory. + + Rooms' join states are NOT updated in any way; it is the caller's responsibility to soon + **leave** the room on the user's behalf, so there's no point sending new join events into + rooms to propagate the profile deletion. + See the `users_pending_deactivation` table and the associated user parter loop. + """ + if not self.hs.is_mine(target_user): + raise SynapseError(400, "User is not hosted on this homeserver") + + # Prevent users from deactivating anyone but themselves, + # except for admins who can deactivate anyone. + if not by_admin and target_user != requester.user: + # It's a little strange to have this check here, but given all the sibling + # methods have these checks, it'd be even stranger to be inconsistent and not + # have it. + raise AuthError(400, "Cannot remove another user's profile") + + if not by_admin: + current_profile = await self.store.get_profileinfo(target_user) + if not self.hs.config.registration.enable_set_displayname: + if current_profile.display_name: + # SUSPICIOUS: It seems strange to block deactivation on this, + # though this is preserving previous behaviour. + raise SynapseError( + 400, + "Changing display name is disabled on this server", + Codes.FORBIDDEN, + ) + + if not self.hs.config.registration.enable_set_avatar_url: + if current_profile.avatar_url: + # SUSPICIOUS: It seems strange to block deactivation on this, + # though this is preserving previous behaviour. + raise SynapseError( + 400, + "Changing avatar is disabled on this server", + Codes.FORBIDDEN, + ) + + await self.store.delete_profile(target_user) + + await self._third_party_rules.on_profile_update( + target_user.to_string(), + ProfileInfo(None, None), + by_admin, + deactivation=True, + ) + @cached() async def check_avatar_size_and_mime_type(self, mxc: str) -> bool: """Check that the size and content type of the avatar at the given MXC URI are @@ -482,18 +558,22 @@ class ProfileHandler: requester: Requester, field_name: str, new_value: JsonValue, + *, by_admin: bool = False, - deactivation: bool = False, ) -> None: """Set a new profile field for a user. + Preconditions: + - This must NOT be called as part of deactivating the user, because we will + notify modules about the change whilst claiming it is not related + to user deactivation. + Args: target_user: the user whose profile 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. """ if not self.hs.is_mine(target_user): raise SynapseError(400, "User is not hosted on this homeserver") @@ -506,7 +586,7 @@ class ProfileHandler: # 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 + target_user.to_string(), profile, by_admin, deactivation=False ) async def delete_profile_field( @@ -514,17 +594,21 @@ class ProfileHandler: target_user: UserID, requester: Requester, field_name: str, + *, by_admin: bool = False, - deactivation: bool = False, ) -> None: """Delete a field from a user's profile. + Preconditions: + - This must NOT be called as part of deactivating the user, because we will + notify modules about the change whilst claiming it is not related + to user deactivation. + Args: target_user: the user whose profile is to be changed. requester: The user attempting to make this change. field_name: The name of the profile field to remove. 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") @@ -537,7 +621,7 @@ class ProfileHandler: # 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 + target_user.to_string(), profile, by_admin, deactivation=False ) async def on_profile_query(self, args: JsonDict) -> JsonDict: diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index ebbe7afa84..0b2587f94f 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -522,7 +522,10 @@ class SsoHandler: authenticated_entity=user_id, ) await self._profile_handler.set_displayname( - user_id_obj, requester, attributes.display_name, True + user_id_obj, + requester, + attributes.display_name, + by_admin=True, ) if attributes.picture: await self.set_avatar(user_id, attributes.picture) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 0580f3665c..947be24d3e 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -160,6 +160,7 @@ from synapse.util.caches.descriptors import CachedFunction, cached as _cached from synapse.util.clock import Clock from synapse.util.duration import Duration from synapse.util.frozenutils import freeze +from synapse.util.sentinel import Sentinel if TYPE_CHECKING: # Old versions don't have `LiteralString` @@ -1989,27 +1990,47 @@ class ModuleApi: self, user_id: UserID, new_displayname: str, - deactivation: bool = False, + deactivation: bool | Sentinel = Sentinel.UNSET_SENTINEL, ) -> None: """Sets a user's display name. Added in Synapse v1.76.0. + (Synapse Developer note: All future arguments should be kwargs-only + due to https://github.com/element-hq/synapse/issues/19546) + Args: user_id: The user whose display name is to be changed. new_displayname: The new display name to give the user. deactivation: + **deprecated since v1.150.0** + Callers should NOT pass this argument. Instead, omit it and leave it to the default. + Will log an error if it is passed. + Remove after 2027-01-01 + Tracked by https://github.com/element-hq/synapse/issues/19546 + Whether this change was made while deactivating the user. + + Should be omitted, will produce a logged error if set to True. + It's likely that this flag should have stayed internal-only and + was accidentally exposed to the Module API. + It no longer has any function. """ requester = create_requester(user_id) + + if deactivation is not Sentinel.UNSET_SENTINEL: + logger.error( + "Deprecated `deactivation` parameter passed to `set_displayname` Module API (value: %r). This will break in 2027.", + deactivation, + ) + await self._hs.get_profile_handler().set_displayname( target_user=user_id, requester=requester, new_displayname=new_displayname, by_admin=True, - deactivation=deactivation, ) def get_current_time_msec(self) -> int: diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 807a9cad5b..8265c2d789 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -369,7 +369,7 @@ class UserRestServletV2(UserRestServletV2Get): if user: # modify user if "displayname" in body: await self.profile_handler.set_displayname( - target_user, requester, body["displayname"], True + target_user, requester, body["displayname"], by_admin=True ) if threepids is not None: @@ -418,7 +418,7 @@ class UserRestServletV2(UserRestServletV2Get): if "avatar_url" in body: await self.profile_handler.set_avatar_url( - target_user, requester, body["avatar_url"], True + target_user, requester, body["avatar_url"], by_admin=True ) if "admin" in body: @@ -526,7 +526,7 @@ class UserRestServletV2(UserRestServletV2Get): if "avatar_url" in body and isinstance(body["avatar_url"], str): await self.profile_handler.set_avatar_url( - target_user, requester, body["avatar_url"], True + target_user, requester, body["avatar_url"], by_admin=True ) user_info_dict = await self.admin_handler.get_user(target_user) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 7f3128cb61..c2ec5b3611 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -206,15 +206,15 @@ class ProfileFieldRestServlet(RestServlet): if field_name == ProfileFields.DISPLAYNAME: await self.profile_handler.set_displayname( - user, requester, new_value, is_admin, propagate=propagate + user, requester, new_value, by_admin=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 + user, requester, new_value, by_admin=is_admin, propagate=propagate ) else: await self.profile_handler.set_profile_field( - user, requester, field_name, new_value, is_admin + user, requester, field_name, new_value, by_admin=is_admin ) return 200, {} @@ -263,15 +263,15 @@ class ProfileFieldRestServlet(RestServlet): if field_name == ProfileFields.DISPLAYNAME: await self.profile_handler.set_displayname( - user, requester, "", is_admin, propagate=propagate + user, requester, "", by_admin=is_admin, propagate=propagate ) elif field_name == ProfileFields.AVATAR_URL: await self.profile_handler.set_avatar_url( - user, requester, "", is_admin, propagate=propagate + user, requester, "", by_admin=is_admin, propagate=propagate ) else: await self.profile_handler.delete_profile_field( - user, requester, field_name, is_admin + user, requester, field_name, by_admin=is_admin ) return 200, {} diff --git a/synapse/storage/databases/main/profile.py b/synapse/storage/databases/main/profile.py index 11ad516eb3..9b787e19a3 100644 --- a/synapse/storage/databases/main/profile.py +++ b/synapse/storage/databases/main/profile.py @@ -534,6 +534,18 @@ class ProfileWorkerStore(SQLBaseStore): await self.db_pool.runInteraction("delete_profile_field", delete_profile_field) + async def delete_profile(self, user_id: UserID) -> None: + """ + Deletes an entire user profile, including displayname, avatar_url and all custom fields. + Used at user deactivation when erasure is requested. + """ + + await self.db_pool.simple_delete( + desc="delete_profile", + table="profiles", + keyvalues={"full_user_id": user_id.to_string()}, + ) + class ProfileStore(ProfileWorkerStore): pass diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 0d319dff7e..1709b27f67 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -734,9 +734,9 @@ class ThirdPartyRulesTestCase(unittest.FederatingHomeserverTestCase): self.assertTrue(args[1]) self.assertFalse(args[2]) - # Check that the profile update callback was called twice (once for the display - # name and once for the avatar URL), and that the "deactivation" boolean is true. - self.assertEqual(profile_mock.call_count, 2) + # Check that the profile update callback was called once + # and that the "deactivation" boolean is true. + self.assertEqual(profile_mock.call_count, 1) args = profile_mock.call_args[0] self.assertTrue(args[3]) diff --git a/tests/rest/synapse/mas/test_users.py b/tests/rest/synapse/mas/test_users.py index 4e8cf90700..f0f26a939c 100644 --- a/tests/rest/synapse/mas/test_users.py +++ b/tests/rest/synapse/mas/test_users.py @@ -10,11 +10,14 @@ # # See the GNU Affero General Public License for more details: # . - +from http import HTTPStatus from urllib.parse import urlencode +from parameterized import parameterized + from twisted.internet.testing import MemoryReactor +from synapse.api.errors import StoreError from synapse.appservice import ApplicationService from synapse.server import HomeServer from synapse.types import JsonDict, UserID, create_requester @@ -621,6 +624,78 @@ class MasDeleteUserResource(BaseTestCase): # And erased self.assertTrue(self.get_success(store.is_user_erased(user_id=str(alice)))) + @parameterized.expand([(False,), (True,)]) + def test_user_deactivation_removes_user_from_user_directory( + self, erase: bool + ) -> None: + """ + Test that when `/delete_user` deactivates a user, they get removed from the user directory. + + This is applicable regardless of whether the `erase` flag is set. + + Regression test for: + 'Users are not removed from user directory upon deactivation if erase flag is true' + https://github.com/element-hq/synapse/issues/19540 + """ + alice = UserID("alice", "test") + store = self.hs.get_datastores().main + + # Ensure we're testing what we think we are: + # check the user is IN the directory at the start of the test + self.assertEqual( + self.get_success(store._get_user_in_directory(alice.to_string())), + ("Alice", None), + ) + + channel = self.make_request( + "POST", + "/_synapse/mas/delete_user", + shorthand=False, + access_token=self.SHARED_SECRET, + content={"localpart": "alice", "erase": erase}, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self.assertEqual(channel.json_body, {}) + + self.assertIsNone( + self.get_success(store._get_user_in_directory(alice.to_string())), + "User still present in user directory after deactivation!", + ) + + def test_user_deactivation_removes_custom_profile_fields(self) -> None: + """ + Test that when `/delete_user` deactivates a user with the `erase` flag, + their custom profile fields get removed. + """ + alice = UserID("alice", "test") + store = self.hs.get_datastores().main + + # Add custom profile field + self.get_success(store.set_profile_field(alice, "io.element.example", "hello")) + + # Ensure we're testing what we think we are: + # check the user has profile data at the start of the test + self.assertEqual( + self.get_success(store.get_profile_fields(alice)), + {"io.element.example": "hello"}, + ) + + channel = self.make_request( + "POST", + "/_synapse/mas/delete_user", + shorthand=False, + access_token=self.SHARED_SECRET, + content={"localpart": "alice", "erase": True}, + ) + self.assertEqual(channel.code, 200, channel.json_body) + self.assertEqual(channel.json_body, {}) + + # With no profile, we expect a 404 (Not Found) StoreError + with self.assertRaises(StoreError) as raised: + self.get_success_or_raise(store.get_profile_fields(alice)) + + self.assertEqual(raised.exception.code, HTTPStatus.NOT_FOUND) + def test_delete_user_missing_localpart(self) -> None: channel = self.make_request( "POST",