Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add creation_ts to list users admin API #10448

Merged
merged 6 commits into from
Jul 22, 2021
Merged

Add creation_ts to list users admin API #10448

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9cf4576
Add `creation_ts` to list users admin API
dklimpel Jul 21, 2021
a93f681
newsfile and typo
dklimpel Jul 21, 2021
0c08483
correct timestamp to s
dklimpel Jul 22, 2021
5cbac42
update `self.pump(1.0)` to `self.reactor.advance`
dklimpel Jul 22, 2021
773fec6
change to time in ms
dklimpel Jul 22, 2021
b13174d
revert small changes
dklimpel Jul 22, 2021
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/10448.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add `creation_ts` to list users admin API.
10 changes: 7 additions & 3 deletions docs/admin_api/user_admin_api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ A response body like the following is returned:
"deactivated": 0,
"shadow_banned": 0,
"displayname": "<User One>",
"avatar_url": null
"avatar_url": null,
"creation_ts": 1560432668
}, {
"name": "<user_id2>",
"is_guest": 0,
Expand All @@ -153,7 +154,8 @@ A response body like the following is returned:
"deactivated": 0,
"shadow_banned": 0,
"displayname": "<User Two>",
"avatar_url": "<avatar_url>"
"avatar_url": "<avatar_url>",
"creation_ts": 1561550621
}
],
"next_token": "100",
Expand Down Expand Up @@ -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 s (Unix timestamp).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make it return a value in milliseconds instead of seconds please? The users table is an old one and as such as a few inconsistencies with the rest of Synapse, one of them being using seconds instead of milliseconds. The rest of Synapse uses milliseconds, so I think it'd be nice if we could try to head towards consistency here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can do that, but then there is a mismatch to Query user API
What do we do with the Query user API? Also change to ms? With a version increment?

I would like to prefer that both APIs returns the same values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah somehow I've missed that the query user API also returns a value in seconds. Ideally I'd like it to change to ms too (with a v3 of the endpoint in order not to confuse users). I'd suggest doing that in a separate PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like a v3 for this small change.
But we can start here to use ms.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes definitely. And agreed that v3 is maybe a bit overkill for such a small change, but I also don't think we should change the unit in the v2 endpoint, lest we break existing scripts using it.


- `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.
Expand All @@ -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 s (Unix timestamp).

- `next_token`: string representing a positive integer - Indication for pagination. See above.
- `total` - integer - Total number of media.
Expand Down
2 changes: 2 additions & 0 deletions synapse/rest/admin/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"):
Expand Down Expand Up @@ -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,
),
)

Expand Down
19 changes: 7 additions & 12 deletions synapse/storage/databases/main/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions synapse/storage/databases/main/stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
CREATION_TS = ordered by `creation_ts`
"""

MEDIA_LENGTH = "media_length"
Expand All @@ -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):
Expand Down
45 changes: 27 additions & 18 deletions tests/rest/admin/test_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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):
Expand All @@ -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"])

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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):
Expand All @@ -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")
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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")
Expand All @@ -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)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to use self.reactor.advance here instead

user2 = self.register_user("user2", "pass2", admin=False, displayname="Name Y")

# Modify user
Expand Down Expand Up @@ -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],
Expand All @@ -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)
Expand All @@ -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):
"""
Expand Down