From 9cf4576acdff3f6cabf712c7d9f2082e7d23b07e Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 21 Jul 2021 20:40:07 +0200 Subject: [PATCH 1/6] Add `creation_ts` to list users admin API --- docs/admin_api/user_admin_api.md | 10 +++-- synapse/rest/admin/users.py | 2 + synapse/storage/databases/main/__init__.py | 19 ++++----- synapse/storage/databases/main/stats.py | 2 + tests/rest/admin/test_user.py | 45 +++++++++++++--------- 5 files changed, 45 insertions(+), 33 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 4a65d0c3bc96..6551b567868d 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -144,7 +144,8 @@ A response body like the following is returned: "deactivated": 0, "shadow_banned": 0, "displayname": "", - "avatar_url": null + "avatar_url": null, + "creation_ts": 1560432668 }, { "name": "", "is_guest": 0, @@ -153,7 +154,8 @@ A response body like the following is returned: "deactivated": 0, "shadow_banned": 0, "displayname": "", - "avatar_url": "" + "avatar_url": "", + "creation_ts": 1561550621 } ], "next_token": "100", @@ -197,11 +199,12 @@ The following parameters should be set in the URL: - `shadow_banned` - Users are ordered by `shadow_banned` status. - `displayname` - Users are ordered alphabetically by `displayname`. - `avatar_url` - Users are ordered alphabetically by avatar URL. + - `creation_ts` - Users are ordered by when the users was created in ms. - `dir` - Direction of media order. Either `f` for forwards or `b` for backwards. Setting this value to `b` will reverse the above sort order. Defaults to `f`. -Caution. The database only has indexes on the columns `name` and `created_ts`. +Caution. The database only has indexes on the columns `name` and `creation_ts`. This means that if a different sort order is used (`is_guest`, `admin`, `user_type`, `deactivated`, `shadow_banned`, `avatar_url` or `displayname`), this can cause a large load on the database, especially for large environments. @@ -222,6 +225,7 @@ The following fields are returned in the JSON response body: - `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. + - `creation_ts` - integer - The user's creation timestamp in ms. - `next_token`: string representing a positive integer - Indication for pagination. See above. - `total` - integer - Total number of media. diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 589e47fa473c..dc8a8d6faf84 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -62,6 +62,7 @@ class UsersRestServletV2(RestServlet): The parameter `name` can be used to filter by user id or display name. The parameter `guests` can be used to exclude guest users. The parameter `deactivated` can be used to include deactivated users. + The parameter `order_by` can be used to order the result. """ def __init__(self, hs: "HomeServer"): @@ -108,6 +109,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: UserSortOrder.USER_TYPE.value, UserSortOrder.AVATAR_URL.value, UserSortOrder.SHADOW_BANNED.value, + UserSortOrder.CREATION_TS.value, ), ) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index a3fddea042af..c56c8e4402c4 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -297,27 +297,22 @@ def get_users_paginate_txn(txn): where_clause = "WHERE " + " AND ".join(filters) if len(filters) > 0 else "" - sql_base = """ + sql_base = f""" FROM users as u LEFT JOIN profiles AS p ON u.name = '@' || p.user_id || ':' || ? - {} - """.format( - where_clause - ) + {where_clause} + """ sql = "SELECT COUNT(*) as total_users " + sql_base txn.execute(sql, args) count = txn.fetchone()[0] - sql = """ - SELECT name, user_type, is_guest, admin, deactivated, shadow_banned, displayname, avatar_url + sql = f""" + SELECT name, user_type, is_guest, admin, deactivated, + shadow_banned, displayname, avatar_url, creation_ts {sql_base} ORDER BY {order_by_column} {order}, u.name ASC LIMIT ? OFFSET ? - """.format( - sql_base=sql_base, - order_by_column=order_by_column, - order=order, - ) + """ args += [limit, start] txn.execute(sql, args) users = self.db_pool.cursor_to_dict(txn) diff --git a/synapse/storage/databases/main/stats.py b/synapse/storage/databases/main/stats.py index 59d67c255b6d..d87d3b8e4316 100644 --- a/synapse/storage/databases/main/stats.py +++ b/synapse/storage/databases/main/stats.py @@ -75,6 +75,7 @@ class UserSortOrder(Enum): USER_TYPE = ordered alphabetically by `user_type` AVATAR_URL = ordered alphabetically by `avatar_url` SHADOW_BANNED = ordered by `shadow_banned` + CREATED_TS = ordered by `creation_ts` """ MEDIA_LENGTH = "media_length" @@ -88,6 +89,7 @@ class UserSortOrder(Enum): USER_TYPE = "user_type" AVATAR_URL = "avatar_url" SHADOW_BANNED = "shadow_banned" + CREATION_TS = "creation_ts" class StatsStore(StateDeltasStore): diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 4fccce34fdd5..2ea3c6208e15 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -473,7 +473,7 @@ def test_no_auth(self): """ channel = self.make_request("GET", self.url, b"{}") - self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(401, channel.code, msg=channel.json_body) self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) def test_requester_is_no_admin(self): @@ -485,7 +485,7 @@ def test_requester_is_no_admin(self): channel = self.make_request("GET", self.url, access_token=other_user_token) - self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(403, channel.code, msg=channel.json_body) self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) def test_all_users(self): @@ -497,11 +497,11 @@ def test_all_users(self): channel = self.make_request( "GET", self.url + "?deactivated=true", - b"{}", + {}, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(3, len(channel.json_body["users"])) self.assertEqual(3, channel.json_body["total"]) @@ -532,7 +532,7 @@ def _search_test( ) channel = self.make_request( "GET", - url.encode("ascii"), + url, access_token=self.admin_user_tok, ) self.assertEqual(expected_http_code, channel.code, msg=channel.json_body) @@ -598,7 +598,7 @@ def test_invalid_parameter(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) # negative from @@ -608,7 +608,7 @@ def test_invalid_parameter(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) # invalid guests @@ -618,7 +618,7 @@ def test_invalid_parameter(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"]) # invalid deactivated @@ -628,7 +628,7 @@ def test_invalid_parameter(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"]) # unkown order_by @@ -648,7 +648,7 @@ def test_invalid_parameter(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"]) def test_limit(self): @@ -666,7 +666,7 @@ def test_limit(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(channel.json_body["total"], number_users) self.assertEqual(len(channel.json_body["users"]), 5) self.assertEqual(channel.json_body["next_token"], "5") @@ -687,7 +687,7 @@ def test_from(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(channel.json_body["total"], number_users) self.assertEqual(len(channel.json_body["users"]), 15) self.assertNotIn("next_token", channel.json_body) @@ -708,7 +708,7 @@ def test_limit_and_from(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(channel.json_body["total"], number_users) self.assertEqual(channel.json_body["next_token"], "15") self.assertEqual(len(channel.json_body["users"]), 10) @@ -731,7 +731,7 @@ def test_next_token(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(channel.json_body["total"], number_users) self.assertEqual(len(channel.json_body["users"]), number_users) self.assertNotIn("next_token", channel.json_body) @@ -744,7 +744,7 @@ def test_next_token(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(channel.json_body["total"], number_users) self.assertEqual(len(channel.json_body["users"]), number_users) self.assertNotIn("next_token", channel.json_body) @@ -757,7 +757,7 @@ def test_next_token(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(channel.json_body["total"], number_users) self.assertEqual(len(channel.json_body["users"]), 19) self.assertEqual(channel.json_body["next_token"], "19") @@ -771,7 +771,7 @@ def test_next_token(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(channel.json_body["total"], number_users) self.assertEqual(len(channel.json_body["users"]), 1) self.assertNotIn("next_token", channel.json_body) @@ -781,7 +781,10 @@ def test_order_by(self): Testing order list with parameter `order_by` """ + # make sure that the users do not have the same timestamps + self.pump(1.0) user1 = self.register_user("user1", "pass1", admin=False, displayname="Name Z") + self.pump(1.0) user2 = self.register_user("user2", "pass2", admin=False, displayname="Name Y") # Modify user @@ -841,6 +844,11 @@ def test_order_by(self): self._order_test([self.admin_user, user2, user1], "avatar_url", "f") self._order_test([user1, user2, self.admin_user], "avatar_url", "b") + # order by creation_ts + self._order_test([self.admin_user, user1, user2], "creation_ts") + self._order_test([self.admin_user, user1, user2], "creation_ts", "f") + self._order_test([user2, user1, self.admin_user], "creation_ts", "b") + def _order_test( self, expected_user_list: List[str], @@ -863,7 +871,7 @@ def _order_test( url += "dir=%s" % (dir,) channel = self.make_request( "GET", - url.encode("ascii"), + url, access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) @@ -887,6 +895,7 @@ def _check_fields(self, content: JsonDict): self.assertIn("shadow_banned", u) self.assertIn("displayname", u) self.assertIn("avatar_url", u) + self.assertIn("creation_ts", u) def _create_users(self, number_users: int): """ From a93f681b775efaa4a3d8edcfbb8c188248afd41f Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 21 Jul 2021 20:49:38 +0200 Subject: [PATCH 2/6] newsfile and typo --- changelog.d/10448.feature | 1 + synapse/storage/databases/main/stats.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/10448.feature diff --git a/changelog.d/10448.feature b/changelog.d/10448.feature new file mode 100644 index 000000000000..f6579e0ca85b --- /dev/null +++ b/changelog.d/10448.feature @@ -0,0 +1 @@ +Add `creation_ts` to list users admin API. \ No newline at end of file diff --git a/synapse/storage/databases/main/stats.py b/synapse/storage/databases/main/stats.py index d87d3b8e4316..f42718cd46c1 100644 --- a/synapse/storage/databases/main/stats.py +++ b/synapse/storage/databases/main/stats.py @@ -75,7 +75,7 @@ class UserSortOrder(Enum): USER_TYPE = ordered alphabetically by `user_type` AVATAR_URL = ordered alphabetically by `avatar_url` SHADOW_BANNED = ordered by `shadow_banned` - CREATED_TS = ordered by `creation_ts` + CREATION_TS = ordered by `creation_ts` """ MEDIA_LENGTH = "media_length" From 0c084838b22de7a9d674286d5ac5fdb57490aa90 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 22 Jul 2021 09:28:19 +0200 Subject: [PATCH 3/6] correct timestamp to s --- docs/admin_api/user_admin_api.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 6551b567868d..28cab8ebf3d7 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -199,7 +199,7 @@ The following parameters should be set in the URL: - `shadow_banned` - Users are ordered by `shadow_banned` status. - `displayname` - Users are ordered alphabetically by `displayname`. - `avatar_url` - Users are ordered alphabetically by avatar URL. - - `creation_ts` - Users are ordered by when the users was created in ms. + - `creation_ts` - Users are ordered by when the users was created in s (Unix timestamp). - `dir` - Direction of media order. Either `f` for forwards or `b` for backwards. Setting this value to `b` will reverse the above sort order. Defaults to `f`. @@ -225,7 +225,7 @@ The following fields are returned in the JSON response body: - `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. - - `creation_ts` - integer - The user's creation timestamp in ms. + - `creation_ts` - integer - The user's creation timestamp in s (Unix timestamp). - `next_token`: string representing a positive integer - Indication for pagination. See above. - `total` - integer - Total number of media. From 5cbac4201d9512d6e5ef738e9f564803ce5eccc9 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 22 Jul 2021 13:13:37 +0200 Subject: [PATCH 4/6] update `self.pump(1.0)` to `self.reactor.advance` --- tests/rest/admin/test_user.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 2ea3c6208e15..37446fe4834f 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -782,9 +782,9 @@ def test_order_by(self): """ # make sure that the users do not have the same timestamps - self.pump(1.0) + self.reactor.advance user1 = self.register_user("user1", "pass1", admin=False, displayname="Name Z") - self.pump(1.0) + self.reactor.advance user2 = self.register_user("user2", "pass2", admin=False, displayname="Name Y") # Modify user @@ -2545,11 +2545,11 @@ def test_order_by(self): # create media and make sure they do not have the same timestamp media1 = self._create_media_and_access(other_user_tok, image_data1, "image.png") - self.pump(1.0) + self.reactor.advance media2 = self._create_media_and_access(other_user_tok, image_data2, "image.gif") - self.pump(1.0) + self.reactor.advance media3 = self._create_media_and_access(other_user_tok, image_data3, "image.bmp") - self.pump(1.0) + self.reactor.advance # Mark one media as safe from quarantine. self.get_success(self.store.mark_local_media_as_safe(media2)) From 773fec61075aca7f0e549a0b0d96cf39dd24289c Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 22 Jul 2021 13:38:47 +0200 Subject: [PATCH 5/6] change to time in ms --- docs/admin_api/user_admin_api.md | 8 ++++---- synapse/storage/databases/main/__init__.py | 4 ++-- tests/rest/admin/test_user.py | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 28cab8ebf3d7..160899754ede 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -145,7 +145,7 @@ A response body like the following is returned: "shadow_banned": 0, "displayname": "", "avatar_url": null, - "creation_ts": 1560432668 + "creation_ts": 1560432668000 }, { "name": "", "is_guest": 0, @@ -155,7 +155,7 @@ A response body like the following is returned: "shadow_banned": 0, "displayname": "", "avatar_url": "", - "creation_ts": 1561550621 + "creation_ts": 1561550621000 } ], "next_token": "100", @@ -199,7 +199,7 @@ The following parameters should be set in the URL: - `shadow_banned` - Users are ordered by `shadow_banned` status. - `displayname` - Users are ordered alphabetically by `displayname`. - `avatar_url` - Users are ordered alphabetically by avatar URL. - - `creation_ts` - Users are ordered by when the users was created in s (Unix timestamp). + - `creation_ts` - Users are ordered by when the users was created in ms. - `dir` - Direction of media order. Either `f` for forwards or `b` for backwards. Setting this value to `b` will reverse the above sort order. Defaults to `f`. @@ -225,7 +225,7 @@ The following fields are returned in the JSON response body: - `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. - - `creation_ts` - integer - The user's creation timestamp in s (Unix timestamp). + - `creation_ts` - integer - The user's creation timestamp in ms. - `next_token`: string representing a positive integer - Indication for pagination. See above. - `total` - integer - Total number of media. diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index c56c8e4402c4..d172b18402f8 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -307,8 +307,8 @@ def get_users_paginate_txn(txn): count = txn.fetchone()[0] sql = f""" - SELECT name, user_type, is_guest, admin, deactivated, - shadow_banned, displayname, avatar_url, creation_ts + SELECT name, user_type, is_guest, admin, deactivated, shadow_banned, + displayname, avatar_url, creation_ts * 1000 as creation_ts {sql_base} ORDER BY {order_by_column} {order}, u.name ASC LIMIT ? OFFSET ? diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 37446fe4834f..9c0055f62a25 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -782,9 +782,9 @@ def test_order_by(self): """ # make sure that the users do not have the same timestamps - self.reactor.advance + self.reactor.advance(10) user1 = self.register_user("user1", "pass1", admin=False, displayname="Name Z") - self.reactor.advance + self.reactor.advance(10) user2 = self.register_user("user2", "pass2", admin=False, displayname="Name Y") # Modify user @@ -2545,11 +2545,11 @@ def test_order_by(self): # create media and make sure they do not have the same timestamp media1 = self._create_media_and_access(other_user_tok, image_data1, "image.png") - self.reactor.advance + self.reactor.advance(10) media2 = self._create_media_and_access(other_user_tok, image_data2, "image.gif") - self.reactor.advance + self.reactor.advance(10) media3 = self._create_media_and_access(other_user_tok, image_data3, "image.bmp") - self.reactor.advance + self.reactor.advance(10) # Mark one media as safe from quarantine. self.get_success(self.store.mark_local_media_as_safe(media2)) From b13174d79c527bf67a410b3e47f00b3f8bb9fc45 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 22 Jul 2021 14:08:51 +0200 Subject: [PATCH 6/6] revert small changes --- tests/rest/admin/test_user.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 9c0055f62a25..42f50c092101 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2545,11 +2545,11 @@ def test_order_by(self): # create media and make sure they do not have the same timestamp media1 = self._create_media_and_access(other_user_tok, image_data1, "image.png") - self.reactor.advance(10) + self.pump(1.0) media2 = self._create_media_and_access(other_user_tok, image_data2, "image.gif") - self.reactor.advance(10) + self.pump(1.0) media3 = self._create_media_and_access(other_user_tok, image_data3, "image.bmp") - self.reactor.advance(10) + self.pump(1.0) # Mark one media as safe from quarantine. self.get_success(self.store.mark_local_media_as_safe(media2))