From ae239280cb742968808ca1a4b7d7a84f67ca8177 Mon Sep 17 00:00:00 2001 From: Olivier 'reivilibre Date: Wed, 11 Mar 2026 15:38:45 +0000 Subject: [PATCH] Fix a bug introduced in v1.26.0 that caused deactivated, erased users to not be removed from the user directory. (#19542) Fixes: #19540 Fixes: #16290 (side effect of the proposed fix) Closes: #12804 (side effect of the proposed fix) Introduced in: https://github.com/matrix-org/synapse/pull/8932 --- This PR is a relatively simple simplification of the profile change on deactivation that appears to remove multiple bugs. This PR's **primary motivating fix** is #19540: when a user is deactivated and erased, they would be kept in the user directory. This bug appears to have been here since #8932 (previously https://github.com/matrix-org/synapse/pull/8932) (v1.26.0). The root cause of this bug is that after removing the user from the user directory, we would immediately update their displayname and avatar to empty strings (one at a time), which re-inserts the user into the user directory. With this PR, we now delete the entire `profiles` row upon user erasure, which is cleaner (from a 'your database goes back to zero after deactivating and erasing a user' point of view) and only needs one database operation (instead of doing displayname then avatar). With this PR, we also no longer send the 2 (deferred) `m.room.member` `join` events to every room to propagate the displayname and avatar_url changes. This is good for two reasons: - the user is about to get parted from those rooms anyway, so this reduces the number of state events sent per room from 3 to 1. (More efficient for us in the moment and leaves less litter in the room DAG.) - it is possible for the displayname/avatar update to be sent **after** the user parting, which seems as though it could trigger the user to be re-joined to a public room. (With that said, although this sounds vaguely familiar in my lossy memory, I can't find a ticket that actually describes this bug, so this might be fictional. Edit: #16290 seems to describe this, although the title is misleading.) Additionally, as a side effect of the proposed fix (deleting the `profiles` row), this PR also now deletes custom profile fields upon user erasure, which is a new feature/bugfix (not sure which) in its own right. I do not see a ticket that corresponds to this feature gap, possibly because custom profile fields are still a niche feature without mainstream support (to the best of my knowledge). Tests are included for the primary bugfix and for the cleanup of custom profile fields. ### `set_displayname` module API change This change includes a minor _technically_-breaking change to the module API. The change concerns `set_displayname` which is exposed to the module API with a `deactivation: bool = False` flag, matching the internal handler method it wraps. I suspect that this is a mistake caused by overly-faithfully piping through the args from the wrapped method (this Module API was introduced in https://github.com/matrix-org/synapse/pull/14629/changes#diff-0b449f6f95672437cf04f0b5512572b4a6a729d2759c438b7c206ea249619885R1592). The linked PR did the same for `by_admin` originally before it was changed. The `deactivation` flag's only purpose is to be piped through to other Module API callbacks when a module has registered to be notified about profile changes. My claim is that it makes no sense for the Module API to have this flag because it is not the one doing the deactivation, thus it should never be in a position to set this to `True`. My proposed change keeps the flag (for function signature compatibility), but turns it into a no-op (with a `ERROR` log when it's set to True by the module). The Module API callback notifying of the module-caused displayname change will therefore now always have `deactivation = False`. *Discussed in [`#synapse-dev:matrix.org`](https://matrix.to/#/!i5D5LLct_DYG-4hQprLzrxdbZ580U9UB6AEgFnk6rZQ/$1f8N6G_EJUI_I_LvplnVAF2UFZTw_FzgsPfB6pbcPKk?via=element.io&via=matrix.org&via=beeper.com)* --------- Signed-off-by: Olivier 'reivilibre --- changelog.d/19542.bugfix | 1 + docs/admin_api/user_admin_api.md | 1 + docs/upgrade.md | 13 +++ synapse/handlers/deactivate_account.py | 14 +-- synapse/handlers/profile.py | 108 +++++++++++++++++--- synapse/handlers/sso.py | 5 +- synapse/module_api/__init__.py | 25 ++++- synapse/rest/admin/users.py | 6 +- synapse/rest/client/profile.py | 12 +-- synapse/storage/databases/main/profile.py | 12 +++ tests/rest/client/test_third_party_rules.py | 6 +- tests/rest/synapse/mas/test_users.py | 77 +++++++++++++- 12 files changed, 245 insertions(+), 35 deletions(-) create mode 100644 changelog.d/19542.bugfix 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",