Add `order_by` to list user admin API (#9691)

6 files changed, 248 insertions, 31 deletions

diff --git a/changelog.d/9691.feature b/changelog.d/9691.feature
new file mode 100644
index 0000000000..3c711db4f5
--- /dev/null
+++ b/changelog.d/9691.feature
@@ -0,0 +1 @@
+Add `order_by` to the admin API `GET /_synapse/admin/v2/users`. Contributed by @dklimpel. 
\ No newline at end of file
diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst
index 8d4ec5a6f9..a8a5a2628c 100644
--- a/docs/admin_api/user_admin_api.rst
+++ b/docs/admin_api/user_admin_api.rst
@@ -111,35 +111,16 @@ List Accounts
 =============
 
 This API returns all local user accounts.
+By default, the response is ordered by ascending user ID.
 
-The api is::
+The API is::
 
     GET /_synapse/admin/v2/users?from=0&limit=10&guests=false
 
 To use it, you will need to authenticate by providing an ``access_token`` for a
 server admin: see `README.rst <README.rst>`_.
 
-The parameter ``from`` is optional but used for pagination, denoting the
-offset in the returned results. This should be treated as an opaque value and
-not explicitly set to anything other than the return value of ``next_token``
-from a previous call.
-
-The parameter ``limit`` is optional but is used for pagination, denoting the
-maximum number of items to return in this call. Defaults to ``100``.
-
-The parameter ``user_id`` is optional and filters to only return users with user IDs
-that contain this value. This parameter is ignored when using the ``name`` parameter.
-
-The parameter ``name`` is optional and filters to only return users with user ID localparts
-**or** displaynames that contain this value.
-
-The parameter ``guests`` is optional and if ``false`` will **exclude** guest users.
-Defaults to ``true`` to include guest users.
-
-The parameter ``deactivated`` is optional and if ``true`` will **include** deactivated users.
-Defaults to ``false`` to exclude deactivated users.
-
-A JSON body is returned with the following shape:
+A response body like the following is returned:
 
 .. code:: json
 
@@ -175,6 +156,66 @@ with ``from`` set to the value of ``next_token``. This will return a new page.
 If the endpoint does not return a ``next_token`` then there are no more users
 to paginate through.
 
+**Parameters**
+
+The following parameters should be set in the URL:
+
+- ``user_id`` - Is optional and filters to only return users with user IDs
+  that contain this value. This parameter is ignored when using the ``name`` parameter.
+- ``name`` - Is optional and filters to only return users with user ID localparts
+  **or** displaynames that contain this value.
+- ``guests`` - string representing a bool - Is optional and if ``false`` will **exclude** guest users.
+  Defaults to ``true`` to include guest users.
+- ``deactivated`` - string representing a bool - Is optional and if ``true`` will **include** deactivated users.
+  Defaults to ``false`` to exclude deactivated users.
+- ``limit`` - string representing a positive integer - Is optional but is used for pagination,
+  denoting the maximum number of items to return in this call. Defaults to ``100``.
+- ``from`` - string representing a positive integer - Is optional but used for pagination,
+  denoting the offset in the returned results. This should be treated as an opaque value and
+  not explicitly set to anything other than the return value of ``next_token`` from a previous call.
+  Defaults to ``0``.
+- ``order_by`` - The method by which to sort the returned list of users.
+  If the ordered field has duplicates, the second order is always by ascending ``name``,
+  which guarantees a stable ordering. Valid values are:
+
+  - ``name`` - Users are ordered alphabetically by ``name``. This is the default.
+  - ``is_guest`` - Users are ordered by ``is_guest`` status.
+  - ``admin`` - Users are ordered by ``admin`` status.
+  - ``user_type`` - Users are ordered alphabetically by ``user_type``.
+  - ``deactivated`` - Users are ordered by ``deactivated`` status.
+  - ``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.
+
+- ``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``.
+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.
+
+**Response**
+
+The following fields are returned in the JSON response body:
+
+- ``users`` - An array of objects, each containing information about an user.
+  User objects contain the following fields:
+
+  - ``name`` - string - Fully-qualified user ID (ex. `@user:server.com`).
+  - ``is_guest`` - bool - Status if that user is a guest account.
+  - ``admin`` - bool - Status if that user is a server administrator.
+  - ``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.
+  - ``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.
+
+- ``next_token``: string representing a positive integer - Indication for pagination. See above.
+- ``total`` - integer - Total number of media.
+
+
 Query current sessions for a user
 =================================
 
diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 309bd2771b..fa7804583a 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -36,6 +36,7 @@ from synapse.rest.admin._base import (
 )
 from synapse.rest.client.v2_alpha._base import client_patterns
 from synapse.storage.databases.main.media_repository import MediaSortOrder
+from synapse.storage.databases.main.stats import UserSortOrder
 from synapse.types import JsonDict, UserID
 
 if TYPE_CHECKING:
@@ -117,8 +118,26 @@ class UsersRestServletV2(RestServlet):
         guests = parse_boolean(request, "guests", default=True)
         deactivated = parse_boolean(request, "deactivated", default=False)
 
+        order_by = parse_string(
+            request,
+            "order_by",
+            default=UserSortOrder.NAME.value,
+            allowed_values=(
+                UserSortOrder.NAME.value,
+                UserSortOrder.DISPLAYNAME.value,
+                UserSortOrder.GUEST.value,
+                UserSortOrder.ADMIN.value,
+                UserSortOrder.DEACTIVATED.value,
+                UserSortOrder.USER_TYPE.value,
+                UserSortOrder.AVATAR_URL.value,
+                UserSortOrder.SHADOW_BANNED.value,
+            ),
+        )
+
+        direction = parse_string(request, "dir", default="f", allowed_values=("f", "b"))
+
         users, total = await self.store.get_users_paginate(
-            start, limit, user_id, name, guests, deactivated
+            start, limit, user_id, name, guests, deactivated, order_by, direction
         )
         ret = {"users": users, "total": total}
         if (start + limit) < total:
diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py
index 1d44c3aa2c..b3d16ca7ac 100644
--- a/synapse/storage/databases/main/__init__.py
+++ b/synapse/storage/databases/main/__init__.py
@@ -21,6 +21,7 @@ from typing import List, Optional, Tuple
 from synapse.api.constants import PresenceState
 from synapse.config.homeserver import HomeServerConfig
 from synapse.storage.database import DatabasePool
+from synapse.storage.databases.main.stats import UserSortOrder
 from synapse.storage.engines import PostgresEngine
 from synapse.storage.util.id_generators import (
     IdGenerator,
@@ -292,6 +293,8 @@ class DataStore(
         name: Optional[str] = None,
         guests: bool = True,
         deactivated: bool = False,
+        order_by: UserSortOrder = UserSortOrder.USER_ID.value,
+        direction: str = "f",
     ) -> Tuple[List[JsonDict], int]:
         """Function to retrieve a paginated list of users from
         users list. This will return a json list of users and the
@@ -304,6 +307,8 @@ class DataStore(
             name: search for local part of user_id or display name
             guests: whether to in include guest users
             deactivated: whether to include deactivated users
+            order_by: the sort order of the returned list
+            direction: sort ascending or descending
         Returns:
             A tuple of a list of mappings from user to information and a count of total users.
         """
@@ -312,6 +317,14 @@ class DataStore(
             filters = []
             args = [self.hs.config.server_name]
 
+            # Set ordering
+            order_by_column = UserSortOrder(order_by).value
+
+            if direction == "b":
+                order = "DESC"
+            else:
+                order = "ASC"
+
             # `name` is in database already in lower case
             if name:
                 filters.append("(name LIKE ? OR LOWER(displayname) LIKE ?)")
@@ -339,10 +352,15 @@ class DataStore(
             txn.execute(sql, args)
             count = txn.fetchone()[0]
 
-            sql = (
-                "SELECT name, user_type, is_guest, admin, deactivated, shadow_banned, displayname, avatar_url "
-                + sql_base
-                + " ORDER BY u.name LIMIT ? OFFSET ?"
+            sql = """
+                SELECT name, user_type, is_guest, admin, deactivated, shadow_banned, displayname, avatar_url
+                {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)
diff --git a/synapse/storage/databases/main/stats.py b/synapse/storage/databases/main/stats.py
index 1c99393c65..bce8946c21 100644
--- a/synapse/storage/databases/main/stats.py
+++ b/synapse/storage/databases/main/stats.py
@@ -66,18 +66,37 @@ TYPE_TO_ORIGIN_TABLE = {"room": ("rooms", "room_id"), "user": ("users", "name")}
 class UserSortOrder(Enum):
     """
     Enum to define the sorting method used when returning users
-    with get_users_media_usage_paginate
+    with get_users_paginate in __init__.py
+    and get_users_media_usage_paginate in stats.py
 
-    MEDIA_LENGTH = ordered by size of uploaded media. Smallest to largest.
-    MEDIA_COUNT = ordered by number of uploaded media. Smallest to largest.
+    When moves this to __init__.py gets `builtins.ImportError` with
+    `most likely due to a circular import`
+
+    MEDIA_LENGTH = ordered by size of uploaded media.
+    MEDIA_COUNT = ordered by number of uploaded media.
     USER_ID = ordered alphabetically by `user_id`.
+    NAME = ordered alphabetically by `user_id`. This is for compatibility reasons,
+    as the user_id is returned in the name field in the response in list users admin API.
     DISPLAYNAME = ordered alphabetically by `displayname`
+    GUEST = ordered by `is_guest`
+    ADMIN = ordered by `admin`
+    DEACTIVATED = ordered by `deactivated`
+    USER_TYPE = ordered alphabetically by `user_type`
+    AVATAR_URL = ordered alphabetically by `avatar_url`
+    SHADOW_BANNED = ordered by `shadow_banned`
     """
 
     MEDIA_LENGTH = "media_length"
     MEDIA_COUNT = "media_count"
     USER_ID = "user_id"
+    NAME = "name"
     DISPLAYNAME = "displayname"
+    GUEST = "is_guest"
+    ADMIN = "admin"
+    DEACTIVATED = "deactivated"
+    USER_TYPE = "user_type"
+    AVATAR_URL = "avatar_url"
+    SHADOW_BANNED = "shadow_banned"
 
 
 class StatsStore(StateDeltasStore):
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index cf61f284cb..0c9ec133c2 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -28,7 +28,7 @@ from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError
 from synapse.api.room_versions import RoomVersions
 from synapse.rest.client.v1 import login, logout, profile, room
 from synapse.rest.client.v2_alpha import devices, sync
-from synapse.types import JsonDict
+from synapse.types import JsonDict, UserID
 
 from tests import unittest
 from tests.server import FakeSite, make_request
@@ -467,6 +467,8 @@ class UsersListTestCase(unittest.HomeserverTestCase):
     url = "/_synapse/admin/v2/users"
 
     def prepare(self, reactor, clock, hs):
+        self.store = hs.get_datastore()
+
         self.admin_user = self.register_user("admin", "pass", admin=True)
         self.admin_user_tok = self.login("admin", "pass")
 
@@ -634,6 +636,26 @@ class UsersListTestCase(unittest.HomeserverTestCase):
         self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
         self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"])
 
+        # unkown order_by
+        channel = self.make_request(
+            "GET",
+            self.url + "?order_by=bar",
+            access_token=self.admin_user_tok,
+        )
+
+        self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"])
+
+        # invalid search order
+        channel = self.make_request(
+            "GET",
+            self.url + "?dir=bar",
+            access_token=self.admin_user_tok,
+        )
+
+        self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"])
+
     def test_limit(self):
         """
         Testing list of users with limit
@@ -759,6 +781,103 @@ class UsersListTestCase(unittest.HomeserverTestCase):
         self.assertEqual(len(channel.json_body["users"]), 1)
         self.assertNotIn("next_token", channel.json_body)
 
+    def test_order_by(self):
+        """
+        Testing order list with parameter `order_by`
+        """
+
+        user1 = self.register_user("user1", "pass1", admin=False, displayname="Name Z")
+        user2 = self.register_user("user2", "pass2", admin=False, displayname="Name Y")
+
+        # Modify user
+        self.get_success(self.store.set_user_deactivated_status(user1, True))
+        self.get_success(self.store.set_shadow_banned(UserID.from_string(user1), True))
+
+        # Set avatar URL to all users, that no user has a NULL value to avoid
+        # different sort order between SQlite and PostreSQL
+        self.get_success(self.store.set_profile_avatar_url("user1", "mxc://url3"))
+        self.get_success(self.store.set_profile_avatar_url("user2", "mxc://url2"))
+        self.get_success(self.store.set_profile_avatar_url("admin", "mxc://url1"))
+
+        # order by default (name)
+        self._order_test([self.admin_user, user1, user2], None)
+        self._order_test([self.admin_user, user1, user2], None, "f")
+        self._order_test([user2, user1, self.admin_user], None, "b")
+
+        # order by name
+        self._order_test([self.admin_user, user1, user2], "name")
+        self._order_test([self.admin_user, user1, user2], "name", "f")
+        self._order_test([user2, user1, self.admin_user], "name", "b")
+
+        # order by displayname
+        self._order_test([user2, user1, self.admin_user], "displayname")
+        self._order_test([user2, user1, self.admin_user], "displayname", "f")
+        self._order_test([self.admin_user, user1, user2], "displayname", "b")
+
+        # order by is_guest
+        # like sort by ascending name, as no guest user here
+        self._order_test([self.admin_user, user1, user2], "is_guest")
+        self._order_test([self.admin_user, user1, user2], "is_guest", "f")
+        self._order_test([self.admin_user, user1, user2], "is_guest", "b")
+
+        # order by admin
+        self._order_test([user1, user2, self.admin_user], "admin")
+        self._order_test([user1, user2, self.admin_user], "admin", "f")
+        self._order_test([self.admin_user, user1, user2], "admin", "b")
+
+        # order by deactivated
+        self._order_test([self.admin_user, user2, user1], "deactivated")
+        self._order_test([self.admin_user, user2, user1], "deactivated", "f")
+        self._order_test([user1, self.admin_user, user2], "deactivated", "b")
+
+        # order by user_type
+        # like sort by ascending name, as no special user type here
+        self._order_test([self.admin_user, user1, user2], "user_type")
+        self._order_test([self.admin_user, user1, user2], "user_type", "f")
+        self._order_test([self.admin_user, user1, user2], "is_guest", "b")
+
+        # order by shadow_banned
+        self._order_test([self.admin_user, user2, user1], "shadow_banned")
+        self._order_test([self.admin_user, user2, user1], "shadow_banned", "f")
+        self._order_test([user1, self.admin_user, user2], "shadow_banned", "b")
+
+        # order by avatar_url
+        self._order_test([self.admin_user, user2, user1], "avatar_url")
+        self._order_test([self.admin_user, user2, user1], "avatar_url", "f")
+        self._order_test([user1, user2, self.admin_user], "avatar_url", "b")
+
+    def _order_test(
+        self,
+        expected_user_list: List[str],
+        order_by: Optional[str],
+        dir: Optional[str] = None,
+    ):
+        """Request the list of users in a certain order. Assert that order is what
+        we expect
+        Args:
+            expected_user_list: The list of user_id in the order we expect to get
+                back from the server
+            order_by: The type of ordering to give the server
+            dir: The direction of ordering to give the server
+        """
+
+        url = self.url + "?deactivated=true&"
+        if order_by is not None:
+            url += "order_by=%s&" % (order_by,)
+        if dir is not None and dir in ("b", "f"):
+            url += "dir=%s" % (dir,)
+        channel = self.make_request(
+            "GET",
+            url.encode("ascii"),
+            access_token=self.admin_user_tok,
+        )
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual(channel.json_body["total"], len(expected_user_list))
+
+        returned_order = [row["name"] for row in channel.json_body["users"]]
+        self.assertEqual(expected_user_list, returned_order)
+        self._check_fields(channel.json_body["users"])
+
     def _check_fields(self, content: JsonDict):
         """Checks that the expected user attributes are present in content
         Args: