Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove user's avatar URL and displayname when deactivate an user with Admin API #8932

Merged
merged 14 commits into from
Jan 12, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8932.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove a user's avatar URL and display name when deactivated with the Admin API.
21 changes: 21 additions & 0 deletions docs/admin_api/user_admin_api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ Body parameters:

- ``deactivated``, optional. If unspecified, deactivation state will be left
unchanged on existing accounts and set to ``false`` for new accounts.
A user cannot be erased by deactivating with this API. For details on deactivating users see
`Deactivate Account <#deactivate-account>`_.

If the user already exists then optional parameters default to the current value.

Expand Down Expand Up @@ -248,6 +250,25 @@ server admin: see `README.rst <README.rst>`_.
The erase parameter is optional and defaults to ``false``.
An empty body may be passed for backwards compatibility.

The following actions are performed when deactivating an user:

- Try to unpind 3PIDs from the identity server
- Remove all 3PIDs from the homeserver
- Delete all devices and E2EE keys
- Delete all access tokens
- Delete the password hash
- Removal from all rooms the user is a member of
- Remove the user from the user directory
- Reject all pending invites
- Remove all account validity information related to the user

The following additional actions are performed during deactivation if``erase``
is set to ``true``:

- Remove the user's display name
- Remove the user's avatar URL
- Mark the user as erased


Reset password
==============
Expand Down
21 changes: 19 additions & 2 deletions synapse/handlers/deactivate_account.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

from synapse.api.errors import SynapseError
from synapse.metrics.background_process_metrics import run_as_background_process
from synapse.types import UserID, create_requester
from synapse.types import Requester, UserID, create_requester

from ._base import BaseHandler

Expand All @@ -38,6 +38,7 @@ def __init__(self, hs: "HomeServer"):
self._device_handler = hs.get_device_handler()
self._room_member_handler = hs.get_room_member_handler()
self._identity_handler = hs.get_identity_handler()
self._profile_handler = hs.get_profile_handler()
self.user_directory_handler = hs.get_user_directory_handler()
self._server_name = hs.hostname

Expand All @@ -52,16 +53,23 @@ def __init__(self, hs: "HomeServer"):
self._account_validity_enabled = hs.config.account_validity.enabled

async def deactivate_account(
self, user_id: str, erase_data: bool, id_server: Optional[str] = None
self,
user_id: str,
erase_data: bool,
requester: Requester,
id_server: Optional[str] = None,
by_admin: bool = False,
) -> bool:
"""Deactivate a user's account

Args:
user_id: ID of user to be deactivated
erase_data: whether to GDPR-erase the user's data
requester: The user attempting to make this change.
id_server: Use the given identity server when unbinding
any threepids. If None then will attempt to unbind using the
identity server specified when binding (if known).
by_admin: Whether this change was made by an administrator.

Returns:
True if identity server supports removing threepids, otherwise False.
Expand Down Expand Up @@ -121,6 +129,15 @@ async def deactivate_account(

# Mark the user as erased, if they asked for that
if erase_data:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please only create a single UserID instance and re-use it.

# Remove avatar URL from this user
await self._profile_handler.set_avatar_url(
UserID.from_string(user_id), requester, "", by_admin
)
# Remove displayname from this user
await self._profile_handler.set_displayname(
UserID.from_string(user_id), requester, "", by_admin
)

logger.info("Marking %s as erased", user_id)
await self.store.mark_user_erased(user_id)

Expand Down
8 changes: 7 additions & 1 deletion synapse/handlers/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,13 +286,19 @@ async def set_avatar_url(
400, "Avatar URL is too long (max %i)" % (MAX_AVATAR_URL_LEN,)
)

avatar_url_to_set = new_avatar_url # type: Optional[str]
clokep marked this conversation as resolved.
Show resolved Hide resolved
if new_avatar_url == "":
avatar_url_to_set = None

# Same like set_displayname
if by_admin:
requester = create_requester(
target_user, authenticated_entity=requester.authenticated_entity
)

await self.store.set_profile_avatar_url(target_user.localpart, new_avatar_url)
await self.store.set_profile_avatar_url(
target_user.localpart, avatar_url_to_set
)

if self.hs.config.user_directory_search_all_users:
profile = await self.store.get_profileinfo(target_user.localpart)
Expand Down
20 changes: 14 additions & 6 deletions synapse/rest/admin/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ async def on_PUT(self, request, user_id):

if deactivate and not user["deactivated"]:
await self.deactivate_account_handler.deactivate_account(
target_user.to_string(), False
target_user.to_string(), False, requester, by_admin=True
)
elif not deactivate and user["deactivated"]:
if "password" not in body:
Expand Down Expand Up @@ -486,12 +486,22 @@ async def on_GET(self, request, user_id):
class DeactivateAccountRestServlet(RestServlet):
PATTERNS = admin_patterns("/deactivate/(?P<target_user_id>[^/]*)")

def __init__(self, hs):
def __init__(self, hs: "HomeServer"):
self._deactivate_account_handler = hs.get_deactivate_account_handler()
self.auth = hs.get_auth()
self.is_mine = hs.is_mine
self.store = hs.get_datastore()

async def on_POST(self, request, target_user_id):
async def on_POST(self, request: str, target_user_id: str) -> Tuple[int, JsonDict]:
await assert_requester_is_admin(self.auth, request)

if not self.is_mine(UserID.from_string(target_user_id)):
raise SynapseError(400, "Can only lookup local users")
dklimpel marked this conversation as resolved.
Show resolved Hide resolved

if not await self.store.get_user_by_id(target_user_id):
raise NotFoundError("User not found")

requester = await self.auth.get_user_by_req(request)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it would be better to move this to the top and then use assert_user_is_admin instead of pulling data from the database twice.

body = parse_json_object_from_request(request, allow_empty_body=True)
erase = body.get("erase", False)
if not isinstance(erase, bool):
Expand All @@ -501,10 +511,8 @@ async def on_POST(self, request, target_user_id):
Codes.BAD_JSON,
)

UserID.from_string(target_user_id)

result = await self._deactivate_account_handler.deactivate_account(
target_user_id, erase
target_user_id, erase, requester, by_admin=True
)
if result:
id_server_unbind_result = "success"
Expand Down
7 changes: 5 additions & 2 deletions synapse/rest/client/v2_alpha/account.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ async def on_POST(self, request):
# allow ASes to deactivate their own users
if requester.app_service:
await self._deactivate_account_handler.deactivate_account(
requester.user.to_string(), erase
requester.user.to_string(), erase, requester
)
return 200, {}

Expand All @@ -316,7 +316,10 @@ async def on_POST(self, request):
"deactivate your account",
)
result = await self._deactivate_account_handler.deactivate_account(
requester.user.to_string(), erase, id_server=body.get("id_server")
requester.user.to_string(),
erase,
requester,
id_server=body.get("id_server"),
)
if result:
id_server_unbind_result = "success"
Expand Down
2 changes: 1 addition & 1 deletion synapse/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ def get_initial_sync_handler(self) -> InitialSyncHandler:
return InitialSyncHandler(self)

@cache_in_self
def get_profile_handler(self):
def get_profile_handler(self) -> ProfileHandler:
return ProfileHandler(self)

@cache_in_self
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/databases/main/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ async def set_profile_displayname(
)

async def set_profile_avatar_url(
self, user_localpart: str, new_avatar_url: str
self, user_localpart: str, new_avatar_url: Optional[str]
) -> None:
await self.db_pool.simple_update_one(
table="profiles",
Expand Down
30 changes: 30 additions & 0 deletions tests/handlers/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,21 @@ def test_set_my_name(self):
"Frank",
)

# Set displayname to an empty string
yield defer.ensureDeferred(
self.handler.set_displayname(
self.frank, synapse.types.create_requester(self.frank), ""
)
)

self.assertIsNone(
(
yield defer.ensureDeferred(
self.store.get_profile_displayname(self.frank.localpart)
)
)
)

@defer.inlineCallbacks
def test_set_my_name_if_disabled(self):
self.hs.config.enable_set_displayname = False
Expand Down Expand Up @@ -223,6 +238,21 @@ def test_set_my_avatar(self):
"http://my.server/me.png",
)

# Set avatar to an empty string
yield defer.ensureDeferred(
self.handler.set_avatar_url(
self.frank, synapse.types.create_requester(self.frank), "",
)
)

self.assertIsNone(
(
yield defer.ensureDeferred(
self.store.get_profile_avatar_url(self.frank.localpart)
)
),
)

@defer.inlineCallbacks
def test_set_my_avatar_if_disabled(self):
self.hs.config.enable_set_avatar_url = False
Expand Down
Loading