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 7 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 user's avatar URL when deactivate an user with Admin API.
dklimpel marked this conversation as resolved.
Show resolved Hide resolved
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 Identity Server
- Remove all 3PIDs from Homeserver
- Delete any devices and E2E-Keys
- Delete any access tokens
- Delete password hash
- Removal from all the rooms the user is a member of
- Delete from user directory
- Reject all pending invites
- Remove all information on the user from the account_validity table
dklimpel marked this conversation as resolved.
Show resolved Hide resolved

The following actions are additionally performed when deactivating an user and
and you set ``erase`` to ``true``:
dklimpel marked this conversation as resolved.
Show resolved Hide resolved

- Remove displayname
- Remove avatar URL
dklimpel marked this conversation as resolved.
Show resolved Hide resolved
- Mark the user as erased
dklimpel marked this conversation as resolved.
Show resolved Hide resolved


Reset password
==============
Expand Down
19 changes: 17 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,13 +53,18 @@ 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,
) -> 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).
Expand Down Expand Up @@ -121,6 +127,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=True
clokep marked this conversation as resolved.
Show resolved Hide resolved
)
# Remove displayname from this user
await self._profile_handler.set_displayname(
UserID.from_string(user_id), requester, "", by_admin=True
)

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
)
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
)
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