This repository was archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Show erasure status when listing users in the Admin API #14205
Merged
DMRobertson
merged 15 commits into
matrix-org:develop
from
tadzik:tadzik/show-erased-status-in-admin-user-listing
Oct 21, 2022
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
060d990
Show erasure status when listing users in the Admin API
tadzik 30bd2bf
Use USING when joining erased_users
tadzik 6e602fe
Add changelog entry
tadzik 0906a17
Revert "Use USING when joining erased_users"
tadzik d5919fa
Make the erased check work on postgres
tadzik 960205a
Merge branch 'develop' into tadzik/show-erased-status-in-admin-user-l…
tadzik fe544e7
Add a testcase for showing erased user status
tadzik 5afe378
Appease the style linter
tadzik 795f779
Explicitly convert `erased` to bool to make SQLite consistent with Po…
tadzik 15ac386
Merge branch 'develop' into tadzik/show-erased-status-in-admin-user-l…
tadzik 35c2a88
Move erasure status test to UsersListTestCase
tadzik 84f27cf
Include user erased status when fetching user info via the admin API
tadzik 576bc65
Document the erase status in user_admin_api
tadzik 7aec8d7
Appease the linter and mypy
tadzik 410bae2
Signpost comments in tests
DMRobertson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Show erasure status when listing users in the Admin API. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -201,7 +201,7 @@ async def get_users_paginate( | |
name: Optional[str] = None, | ||
guests: bool = True, | ||
deactivated: bool = False, | ||
order_by: str = UserSortOrder.USER_ID.value, | ||
order_by: str = UserSortOrder.NAME.value, | ||
direction: str = "f", | ||
approved: bool = True, | ||
) -> Tuple[List[JsonDict], int]: | ||
|
@@ -261,6 +261,7 @@ def get_users_paginate_txn( | |
sql_base = f""" | ||
FROM users as u | ||
LEFT JOIN profiles AS p ON u.name = '@' || p.user_id || ':' || ? | ||
LEFT JOIN erased_users AS eu ON u.name = eu.user_id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a significant impact on performance with an additional left join on a large list? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a query plan here: #14205 (comment) There is a sequential scan mentioned under the sorting section, but that mentions the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{where_clause} | ||
""" | ||
sql = "SELECT COUNT(*) as total_users " + sql_base | ||
|
@@ -269,14 +270,22 @@ def get_users_paginate_txn( | |
|
||
sql = f""" | ||
SELECT name, user_type, is_guest, admin, deactivated, shadow_banned, | ||
displayname, avatar_url, creation_ts * 1000 as creation_ts, approved | ||
displayname, avatar_url, creation_ts * 1000 as creation_ts, approved, | ||
eu.user_id is not null as erased | ||
{sql_base} | ||
ORDER BY {order_by_column} {order}, u.name ASC | ||
LIMIT ? OFFSET ? | ||
""" | ||
args += [limit, start] | ||
txn.execute(sql, args) | ||
users = self.db_pool.cursor_to_dict(txn) | ||
|
||
# some of those boolean values are returned as integers when we're on SQLite | ||
columns_to_boolify = ["erased"] | ||
for user in users: | ||
for column in columns_to_boolify: | ||
user[column] = bool(user[column]) | ||
|
||
return users, count | ||
|
||
return await self.db_pool.runInteraction( | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ | |
from synapse.rest.client import devices, login, logout, profile, register, room, sync | ||
from synapse.rest.media.v1.filepath import MediaFilePaths | ||
from synapse.server import HomeServer | ||
from synapse.types import JsonDict, UserID | ||
from synapse.types import JsonDict, UserID, create_requester | ||
from synapse.util import Clock | ||
|
||
from tests import unittest | ||
|
@@ -924,6 +924,36 @@ def test_filter_out_approved(self) -> None: | |
self.assertEqual(1, len(non_admin_user_ids), non_admin_user_ids) | ||
self.assertEqual(not_approved_user, non_admin_user_ids[0]) | ||
|
||
def test_erasure_status(self) -> None: | ||
# Create a new user. | ||
user_id = self.register_user("eraseme", "eraseme") | ||
DMRobertson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# They should appear in the list users API, marked as not erased. | ||
channel = self.make_request( | ||
"GET", | ||
self.url + "?deactivated=true", | ||
access_token=self.admin_user_tok, | ||
) | ||
users = {user["name"]: user for user in channel.json_body["users"]} | ||
self.assertIs(users[user_id]["erased"], False) | ||
DMRobertson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Deactivate that user, requesting erasure. | ||
deactivate_account_handler = self.hs.get_deactivate_account_handler() | ||
self.get_success( | ||
deactivate_account_handler.deactivate_account( | ||
user_id, erase_data=True, requester=create_requester(user_id) | ||
) | ||
) | ||
|
||
# Repeat the list users query. They should now be marked as erased. | ||
channel = self.make_request( | ||
"GET", | ||
self.url + "?deactivated=true", | ||
access_token=self.admin_user_tok, | ||
) | ||
users = {user["name"]: user for user in channel.json_body["users"]} | ||
self.assertIs(users[user_id]["erased"], True) | ||
DMRobertson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def _order_test( | ||
self, | ||
expected_user_list: List[str], | ||
|
@@ -1195,6 +1225,7 @@ def test_deactivate_user_erase_true(self) -> None: | |
self.assertEqual("[email protected]", channel.json_body["threepids"][0]["address"]) | ||
self.assertEqual("mxc://servername/mediaid", channel.json_body["avatar_url"]) | ||
self.assertEqual("User1", channel.json_body["displayname"]) | ||
self.assertFalse(channel.json_body["erased"]) | ||
|
||
# Deactivate and erase user | ||
channel = self.make_request( | ||
|
@@ -1219,6 +1250,7 @@ def test_deactivate_user_erase_true(self) -> None: | |
self.assertEqual(0, len(channel.json_body["threepids"])) | ||
self.assertIsNone(channel.json_body["avatar_url"]) | ||
self.assertIsNone(channel.json_body["displayname"]) | ||
self.assertTrue(channel.json_body["erased"]) | ||
|
||
self._is_erased("@user:test", True) | ||
|
||
|
@@ -2757,6 +2789,7 @@ def _check_fields(self, content: JsonDict) -> None: | |
self.assertIn("avatar_url", content) | ||
self.assertIn("admin", content) | ||
self.assertIn("deactivated", content) | ||
self.assertIn("erased", content) | ||
self.assertIn("shadow_banned", content) | ||
self.assertIn("creation_ts", content) | ||
self.assertIn("appservice_id", content) | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.