dandri/synapse
1
0
Fork 0
mirror of https://github.com/element-hq/synapse.git synced 2026-08-29 09:58:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix a bug introduced in v1.26.0 that caused deactivated, erased users to not be removed from the user directory. (#19542)

Browse Source 
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 <oliverw@matrix.org>
This commit is contained in:
Olivier 'reivilibre
2026-03-11 15:38:45 +00:00
committed by GitHub
parent 59c9e92aed
commit ae239280cb
12 changed files with 245 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
changelog.d/19542.bugfix
+1
View File
@@ -0,0 +1 @@
Fix a bug introduced in v1.26.0 that caused deactivated, erased users to not be removed from the user directory.
docs/admin_api/user_admin_api.md
+1
View File
@@ -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.
docs/upgrade.md
+13
View File
@@ -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
synapse/handlers/deactivate_account.py
+7 -7
View File
@@ -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)
synapse/handlers/profile.py
+96 -12
View File
@@ -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:
synapse/handlers/sso.py
+4 -1
View File
@@ -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)
synapse/module_api/__init__.py
+23 -2
View File
@@ -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:
synapse/rest/admin/users.py
+3 -3
View File
@@ -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)
synapse/rest/client/profile.py
+6 -6
View File
@@ -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, {}
synapse/storage/databases/main/profile.py
+12
View File
@@ -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
tests/rest/client/test_third_party_rules.py
+3 -3
View File
@@ -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])
tests/rest/synapse/mas/test_users.py
+76 -1
View File
@@ -10,11 +10,14 @@
#
# See the GNU Affero General Public License for more details:
# <https://www.gnu.org/licenses/agpl-3.0.html>.
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",