From 060d990eaac99dfaf38b5c0a6fbea05605eee7a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Mon, 17 Oct 2022 16:53:17 +0200 Subject: [PATCH 01/13] Show erasure status when listing users in the Admin API --- synapse/storage/databases/main/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index a62b4abd4e24..e6d35e0708ac 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -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 {where_clause} """ sql = "SELECT COUNT(*) as total_users " + sql_base @@ -269,7 +270,8 @@ 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 not null as erased {sql_base} ORDER BY {order_by_column} {order}, u.name ASC LIMIT ? OFFSET ? From 30bd2bf106415caadcfdbdd1b234ef2b106cc394 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 18 Oct 2022 10:39:47 +0200 Subject: [PATCH 02/13] Use USING when joining erased_users --- synapse/storage/databases/main/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index e6d35e0708ac..5b9e3eba6861 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -261,7 +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 + LEFT JOIN erased_users AS eu USING(user_id) {where_clause} """ sql = "SELECT COUNT(*) as total_users " + sql_base From 6e602fe80420c61df4207d5408ccb90cf54a135f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 18 Oct 2022 10:47:39 +0200 Subject: [PATCH 03/13] Add changelog entry --- changelog.d/14205.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14205.feature diff --git a/changelog.d/14205.feature b/changelog.d/14205.feature new file mode 100644 index 000000000000..6692063352ba --- /dev/null +++ b/changelog.d/14205.feature @@ -0,0 +1 @@ +Show erasure status when listing users in the Admin API. From 0906a171646b62cddf5ce12a82f90d5b31958283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 18 Oct 2022 10:59:07 +0200 Subject: [PATCH 04/13] Revert "Use USING when joining erased_users" This reverts commit 30bd2bf106415caadcfdbdd1b234ef2b106cc394. --- synapse/storage/databases/main/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index 5b9e3eba6861..e6d35e0708ac 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -261,7 +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 USING(user_id) + LEFT JOIN erased_users AS eu ON u.name = eu.user_id {where_clause} """ sql = "SELECT COUNT(*) as total_users " + sql_base From d5919fa7cbbd2c3a95d27c0d6c37349ac6ab15e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 18 Oct 2022 11:00:53 +0200 Subject: [PATCH 05/13] Make the erased check work on postgres --- synapse/storage/databases/main/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index e6d35e0708ac..15e6430acff8 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -271,7 +271,7 @@ 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, - eu.user_id not null as erased + eu.user_id is not null as erased {sql_base} ORDER BY {order_by_column} {order}, u.name ASC LIMIT ? OFFSET ? From fe544e7f695f1513d48d9b190857c18cb73c7d93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 18 Oct 2022 11:19:53 +0200 Subject: [PATCH 06/13] Add a testcase for showing erased user status --- tests/rest/admin/test_user.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 4c1ce33463a9..73a6e7d3bfb0 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1092,9 +1092,10 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.other_user = self.register_user("user", "pass", displayname="User1") self.other_user_token = self.login("user", "pass") - self.url_other_user = "/_synapse/admin/v2/users/%s" % urllib.parse.quote( + self.url_users = "/_synapse/admin/v2/users" + self.url_other_user = "%s/%s" % (self.url_users, urllib.parse.quote( self.other_user - ) + )) self.url = "/_synapse/admin/v1/deactivate/%s" % urllib.parse.quote( self.other_user ) @@ -1222,6 +1223,16 @@ def test_deactivate_user_erase_true(self) -> None: self._is_erased("@user:test", True) + channel = self.make_request( + "GET", + self.url_users + '?deactivated=true', + access_token=self.admin_user_tok, + ) + + self.assertEqual(1, len(list( + filter(lambda user: user["erased"], channel.json_body["users"]) + ))) + @override_config({"max_avatar_size": 1234}) def test_deactivate_user_erase_true_avatar_nonnull_but_empty(self) -> None: """Check we can erase a user whose avatar is the empty string. From 5afe37817b5f4e48df57f3eacd944d4afa39ab1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Tue, 18 Oct 2022 11:41:53 +0200 Subject: [PATCH 07/13] Appease the style linter --- tests/rest/admin/test_user.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 73a6e7d3bfb0..cf980b282965 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1093,9 +1093,10 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.other_user = self.register_user("user", "pass", displayname="User1") self.other_user_token = self.login("user", "pass") self.url_users = "/_synapse/admin/v2/users" - self.url_other_user = "%s/%s" % (self.url_users, urllib.parse.quote( - self.other_user - )) + self.url_other_user = "%s/%s" % ( + self.url_users, + urllib.parse.quote(self.other_user), + ) self.url = "/_synapse/admin/v1/deactivate/%s" % urllib.parse.quote( self.other_user ) @@ -1225,13 +1226,14 @@ def test_deactivate_user_erase_true(self) -> None: channel = self.make_request( "GET", - self.url_users + '?deactivated=true', + self.url_users + "?deactivated=true", access_token=self.admin_user_tok, ) - self.assertEqual(1, len(list( - filter(lambda user: user["erased"], channel.json_body["users"]) - ))) + self.assertEqual( + 1, + len(list(filter(lambda user: user["erased"], channel.json_body["users"]))), + ) @override_config({"max_avatar_size": 1234}) def test_deactivate_user_erase_true_avatar_nonnull_but_empty(self) -> None: From 795f779b7b3cf4ceb6099fce1d5c7b913ab2f102 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Thu, 20 Oct 2022 16:19:33 +0200 Subject: [PATCH 08/13] Explicitly convert `erased` to bool to make SQLite consistent with Postgres This also adds us an easy way in to fix the other accidentally integered columns. --- synapse/storage/databases/main/__init__.py | 7 +++++++ tests/rest/admin/test_user.py | 7 +++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index 15e6430acff8..fc7e3a1d9460 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -279,6 +279,13 @@ def get_users_paginate_txn( 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( diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index cf980b282965..038f3ad3ec32 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1230,10 +1230,9 @@ def test_deactivate_user_erase_true(self) -> None: access_token=self.admin_user_tok, ) - self.assertEqual( - 1, - len(list(filter(lambda user: user["erased"], channel.json_body["users"]))), - ) + users = {user["name"]: user for user in channel.json_body["users"]} + + self.assertIs(users["@user:test"]["erased"], True) @override_config({"max_avatar_size": 1234}) def test_deactivate_user_erase_true_avatar_nonnull_but_empty(self) -> None: From 35c2a88105770192bf0aea97df89a11764085efe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Thu, 20 Oct 2022 16:32:18 +0200 Subject: [PATCH 09/13] Move erasure status test to UsersListTestCase --- tests/rest/admin/test_user.py | 44 +++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 038f3ad3ec32..76e86a3c518e 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -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,32 @@ 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): + user_id = self.register_user("eraseme", "eraseme") + + 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) + + deactivate_account_handler = self.hs.get_deactivate_account_handler() + self.get_success( + deactivate_account_handler.deactivate_account( + user_id, True, create_requester(user_id) + ) + ) + + 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) + def _order_test( self, expected_user_list: List[str], @@ -1092,10 +1118,8 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.other_user = self.register_user("user", "pass", displayname="User1") self.other_user_token = self.login("user", "pass") - self.url_users = "/_synapse/admin/v2/users" - self.url_other_user = "%s/%s" % ( - self.url_users, - urllib.parse.quote(self.other_user), + self.url_other_user = "/_synapse/admin/v2/users/%s" % urllib.parse.quote( + self.other_user ) self.url = "/_synapse/admin/v1/deactivate/%s" % urllib.parse.quote( self.other_user @@ -1224,16 +1248,6 @@ def test_deactivate_user_erase_true(self) -> None: self._is_erased("@user:test", True) - channel = self.make_request( - "GET", - self.url_users + "?deactivated=true", - access_token=self.admin_user_tok, - ) - - users = {user["name"]: user for user in channel.json_body["users"]} - - self.assertIs(users["@user:test"]["erased"], True) - @override_config({"max_avatar_size": 1234}) def test_deactivate_user_erase_true_avatar_nonnull_but_empty(self) -> None: """Check we can erase a user whose avatar is the empty string. From 84f27cfba45ce03fd61305d1bb07878a5dade3ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Thu, 20 Oct 2022 16:55:42 +0200 Subject: [PATCH 10/13] Include user erased status when fetching user info via the admin API --- synapse/handlers/admin.py | 1 + tests/rest/admin/test_user.py | 3 +++ 2 files changed, 4 insertions(+) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index f2989cc4a214..5bf8e863875b 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -100,6 +100,7 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: user_info_dict["avatar_url"] = profile.avatar_url user_info_dict["threepids"] = threepids user_info_dict["external_ids"] = external_ids + user_info_dict["erased"] = await self.store.is_user_erased(user.to_string()) return user_info_dict diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 76e86a3c518e..7583b2e95668 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1221,6 +1221,7 @@ def test_deactivate_user_erase_true(self) -> None: self.assertEqual("foo@bar.com", 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( @@ -1245,6 +1246,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) @@ -2783,6 +2785,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) From 576bc656d214aa3628a1b57cfef3810614e509c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Thu, 20 Oct 2022 16:58:18 +0200 Subject: [PATCH 11/13] Document the erase status in user_admin_api --- docs/admin_api/user_admin_api.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 3625c7b6c5f5..c95d6c9b0536 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -37,6 +37,7 @@ It returns a JSON body like the following: "is_guest": 0, "admin": 0, "deactivated": 0, + "erased": false, "shadow_banned": 0, "creation_ts": 1560432506, "appservice_id": null, @@ -167,6 +168,7 @@ A response body like the following is returned: "admin": 0, "user_type": null, "deactivated": 0, + "erased": false, "shadow_banned": 0, "displayname": "", "avatar_url": null, @@ -177,6 +179,7 @@ A response body like the following is returned: "admin": 1, "user_type": null, "deactivated": 0, + "erased": false, "shadow_banned": 0, "displayname": "", "avatar_url": "", @@ -247,6 +250,7 @@ The following fields are returned in the JSON response body: - `user_type` - string - Type of the user. Normal users are type `None`. This allows user type specific behaviour. There are also types `support` and `bot`. - `deactivated` - bool - Status if that user has been marked as deactivated. + - `erased` - bool - Status if that user has been marked as erased. - `shadow_banned` - bool - Status if that user has been marked as shadow banned. - `displayname` - string - The user's display name if they have set one. - `avatar_url` - string - The user's avatar URL if they have set one. From 7aec8d78d36164ae26ccd9ed690ab23c469ac4db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tadeusz=20So=C5=9Bnierz?= Date: Thu, 20 Oct 2022 17:06:01 +0200 Subject: [PATCH 12/13] Appease the linter and mypy --- synapse/storage/databases/main/__init__.py | 2 +- tests/rest/admin/test_user.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index fc7e3a1d9460..cfaedf5e0ca9 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -281,7 +281,7 @@ def get_users_paginate_txn( 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'] + columns_to_boolify = ["erased"] for user in users: for column in columns_to_boolify: user[column] = bool(user[column]) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 7583b2e95668..4d8d4b9420e9 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -924,7 +924,7 @@ 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): + def test_erasure_status(self) -> None: user_id = self.register_user("eraseme", "eraseme") channel = self.make_request( From 410bae21e6cefddc82fdde22f27279d5a9bffeab Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 21 Oct 2022 13:06:45 +0100 Subject: [PATCH 13/13] Signpost comments in tests --- tests/rest/admin/test_user.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 4d8d4b9420e9..63410ffdf14b 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -925,8 +925,10 @@ def test_filter_out_approved(self) -> None: 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") + # They should appear in the list users API, marked as not erased. channel = self.make_request( "GET", self.url + "?deactivated=true", @@ -935,13 +937,15 @@ def test_erasure_status(self) -> None: users = {user["name"]: user for user in channel.json_body["users"]} self.assertIs(users[user_id]["erased"], False) + # Deactivate that user, requesting erasure. deactivate_account_handler = self.hs.get_deactivate_account_handler() self.get_success( deactivate_account_handler.deactivate_account( - user_id, True, create_requester(user_id) + 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",