From a2769b0974d6e4d6832c59c1091b64d533c20a60 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Mar 2021 22:11:33 +0100 Subject: [PATCH 1/7] Add an admin API to manage ratelimit for a specific user --- docs/admin_api/user_admin_api.rst | 111 +++++++++++ synapse/rest/admin/__init__.py | 2 + synapse/rest/admin/users.py | 109 +++++++++++ synapse/storage/databases/main/room.py | 65 ++++++- tests/rest/admin/test_user.py | 258 +++++++++++++++++++++++++ 5 files changed, 540 insertions(+), 5 deletions(-) diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst index 8d4ec5a6f913..9f286a3ffda0 100644 --- a/docs/admin_api/user_admin_api.rst +++ b/docs/admin_api/user_admin_api.rst @@ -823,3 +823,114 @@ The following parameters should be set in the URL: - ``user_id`` - The fully qualified MXID: for example, ``@user:server.com``. The user must be local. + +Override ratelimiting for users +=============================== + +This API allows to override or disable rate limiting for a specific user. +There are specific APIs to set, get and delete a ratelimit. + +Get status of ratelimit +----------------------- + +The API is:: + + GET /_synapse/admin/v1/users//override_ratelimit + +To use it, you will need to authenticate by providing an ``access_token`` for a +server admin: see `README.rst `_. + +A response body like the following is returned: + +.. code:: json + + { + "messages_per_second": 0, + "burst_count": 0 + } + +**Parameters** + +The following parameters should be set in the URL: + +- ``user_id`` - The fully qualified MXID: for example, ``@user:server.com``. The user must + be local. + +**Response** + +The following fields are returned in the JSON response body: + +- ``messages_per_second`` - integer - The number of actions that can + be performed in a second. +- ``burst_count`` - integer - How many actions that can be performed + before being limited. + +If **no** custom ratelimit is set, the values are ``null``. + +Set ratelimit +------------- + +The API is:: + + POST /_synapse/admin/v1/users//override_ratelimit + +To use it, you will need to authenticate by providing an ``access_token`` for a +server admin: see `README.rst `_. + +A response body like the following is returned: + +.. code:: json + + { + "messages_per_second": 0, + "burst_count": 0 + } + +**Parameters** + +The following parameters should be set in the URL: + +- ``user_id`` - The fully qualified MXID: for example, ``@user:server.com``. The user must + be local. + +Body parameters: + +- ``messages_per_second`` - positive integer, optional. The number of actions that can + be performed in a second. Defaults to ``0``. +- ``burst_count`` - positive integer, optional. How many actions that can be performed + before being limited. Defaults to ``0``. + +To disable users' ratelimit set both values to ``0``. + +**Response** + +The following fields are returned in the JSON response body: + +- ``messages_per_second`` - integer - The number of actions that can + be performed in a second. +- ``burst_count`` - integer - How many actions that can be performed + before being limited. + +Delete ratelimit +---------------- + +The API is:: + + DELETE /_synapse/admin/v1/users//override_ratelimit + +To use it, you will need to authenticate by providing an ``access_token`` for a +server admin: see `README.rst `_. + +An empty JSON dict is returned. + +.. code:: json + + {} + +**Parameters** + +The following parameters should be set in the URL: + +- ``user_id`` - The fully qualified MXID: for example, ``@user:server.com``. The user must + be local. + diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index 8457db1e2275..fe3cd9c14748 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -54,6 +54,7 @@ AccountValidityRenewServlet, DeactivateAccountRestServlet, PushersRestServlet, + RateLimitRestServlet, ResetPasswordRestServlet, SearchUsersRestServlet, ShadowBanRestServlet, @@ -240,6 +241,7 @@ def register_servlets(hs, http_server): ShadowBanRestServlet(hs).register(http_server) ForwardExtremitiesRestServlet(hs).register(http_server) RoomEventContextServlet(hs).register(http_server) + RateLimitRestServlet(hs).register(http_server) def register_servlets_for_client_rest_resource(hs, http_server): diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index aaa56a7024e0..ca3babf587d8 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -982,3 +982,112 @@ async def on_POST( await self.store.set_shadow_banned(UserID.from_string(user_id), True) return 200, {} + + +class RateLimitRestServlet(RestServlet): + """An admin API to override ratelimiting for an user. + + Example: + POST /_synapse/admin/v1/users/@test:example.com/override_ratelimit + { + "messages_per_second": 0, + "burst_count": 0 + } + 200 OK + { + "messages_per_second": 0, + "burst_count": 0 + } + """ + + PATTERNS = admin_patterns("/users/(?P[^/]*)/override_ratelimit") + + def __init__(self, hs: "HomeServer"): + self.hs = hs + self.store = hs.get_datastore() + self.auth = hs.get_auth() + + async def on_GET( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + await assert_requester_is_admin(self.auth, request) + + if not self.hs.is_mine_id(user_id): + raise SynapseError(400, "Can only lookup local users") + + if not await self.store.get_user_by_id(user_id): + raise NotFoundError("User not found") + + ratelimit = await self.store.get_ratelimit_for_user(user_id) + + if ratelimit: + messages_per_second = ratelimit.messages_per_second + burst_count = ratelimit.burst_count + else: + messages_per_second = None + burst_count = None + + ret = { + "messages_per_second": messages_per_second, + "burst_count": burst_count, + } + + return 200, ret + + async def on_POST( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + await assert_requester_is_admin(self.auth, request) + + if not self.hs.is_mine_id(user_id): + raise SynapseError(400, "Only local users can be ratelimited") + + if not await self.store.get_user_by_id(user_id): + raise NotFoundError("User not found") + + body = parse_json_object_from_request(request, allow_empty_body=True) + + messages_per_second = body.get("messages_per_second", 0) + burst_count = body.get("burst_count", 0) + + if not isinstance(messages_per_second, int) or messages_per_second < 0: + raise SynapseError( + 400, + "%r parameter must be a positive int" % (messages_per_second,), + errcode=Codes.INVALID_PARAM, + ) + + if not isinstance(burst_count, int) or burst_count < 0: + raise SynapseError( + 400, + "%r parameter must be a positive int" % (burst_count,), + errcode=Codes.INVALID_PARAM, + ) + + await self.store.set_ratelimit_for_user( + user_id, messages_per_second, burst_count + ) + ratelimit = await self.store.get_ratelimit_for_user(user_id) + assert ratelimit is not None + + ret = { + "messages_per_second": ratelimit.messages_per_second, + "burst_count": ratelimit.burst_count, + } + + return 200, ret + + async def on_DELETE( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + await assert_requester_is_admin(self.auth, request) + + if not self.hs.is_mine_id(user_id): + raise SynapseError(400, "Only local users can be ratelimited") + + if not await self.store.get_user_by_id(user_id): + raise NotFoundError("User not found") + + await self.store.delete_ratelimit_for_user(user_id) + + return 200, {} diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 9cbcd53026d8..1be6ac4ba284 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -521,13 +521,11 @@ def _get_rooms_paginate_txn(txn): ) @cached(max_entries=10000) - async def get_ratelimit_for_user(self, user_id): - """Check if there are any overrides for ratelimiting for the given - user + async def get_ratelimit_for_user(self, user_id) -> Optional[RatelimitOverride]: + """Check if there are any overrides for ratelimiting for the given user Args: - user_id (str) - + user_id Returns: RatelimitOverride if there is an override, else None. If the contents of RatelimitOverride are None or 0 then ratelimitng has been @@ -549,6 +547,63 @@ async def get_ratelimit_for_user(self, user_id): else: return None + async def set_ratelimit_for_user( + self, user_id: str, messages_per_second: int, burst_count: int + ) -> None: + """Sets whether a user shadow-banned. + Args: + user: user ID of the user to test + messages_per_second: + burst_count: + """ + + def set_ratelimit_txn(txn): + self.db_pool.simple_upsert_txn( + txn, + table="ratelimit_override", + keyvalues={"user_id": user_id}, + values={ + "messages_per_second": messages_per_second, + "burst_count": burst_count, + }, + ) + + self._invalidate_cache_and_stream( + txn, self.get_ratelimit_for_user, (user_id,) + ) + + await self.db_pool.runInteraction("set_ratelimit", set_ratelimit_txn) + + async def delete_ratelimit_for_user(self, user_id: str) -> None: + """Sets whether a user shadow-banned. + Args: + user: user ID of the user to test + shadow_banned: true iff the user is to be shadow-banned, false otherwise. + """ + + def delete_ratelimit_txn(txn): + row = self.db_pool.simple_select_one_txn( + txn, + table="ratelimit_override", + keyvalues={"user_id": user_id}, + retcols=["user_id"], + allow_none=True, + ) + + if not row: + return + + # They are there, delete them. + self.db_pool.simple_delete_one_txn( + txn, "ratelimit_override", keyvalues={"user_id": user_id} + ) + + self._invalidate_cache_and_stream( + txn, self.get_ratelimit_for_user, (user_id,) + ) + + await self.db_pool.runInteraction("delete_ratelimit", delete_ratelimit_txn) + @cached() async def get_retention_policy_for_room(self, room_id): """Get the retention policy for a given room. diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index cf61f284cb98..a9f88caff825 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2893,3 +2893,261 @@ def test_success(self): # Ensure the user is shadow-banned (and the cache was cleared). result = self.get_success(self.store.get_user_by_access_token(other_user_token)) self.assertTrue(result.shadow_banned) + + +class RateLimitTestCase(unittest.HomeserverTestCase): + + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + ] + + 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") + + self.other_user = self.register_user("user", "pass") + self.url = ( + "/_synapse/admin/v1/users/%s/override_ratelimit" + % urllib.parse.quote(self.other_user) + ) + + def test_no_auth(self): + """ + Try to get information of an user without authentication. + """ + channel = self.make_request("GET", self.url, b"{}") + + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) + + channel = self.make_request("POST", self.url, b"{}") + + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) + + channel = self.make_request("DELETE", self.url, b"{}") + + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) + + def test_requester_is_no_admin(self): + """ + If the user is not a server admin, an error is returned. + """ + other_user_token = self.login("user", "pass") + + 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(Codes.FORBIDDEN, channel.json_body["errcode"]) + + channel = self.make_request( + "POST", + self.url, + access_token=other_user_token, + ) + + self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) + + channel = self.make_request( + "DELETE", + self.url, + access_token=other_user_token, + ) + + self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) + + def test_user_does_not_exist(self): + """ + Tests that a lookup for a user that does not exist returns a 404 + """ + url = "/_synapse/admin/v1/users/@unknown_person:test/override_ratelimit" + + channel = self.make_request( + "GET", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(404, channel.code, msg=channel.json_body) + self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) + + channel = self.make_request( + "POST", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(404, channel.code, msg=channel.json_body) + self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) + + channel = self.make_request( + "DELETE", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(404, channel.code, msg=channel.json_body) + self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) + + def test_user_is_not_local(self): + """ + Tests that a lookup for a user that is not a local returns a 400 + """ + url = ( + "/_synapse/admin/v1/users/@unknown_person:unknown_domain/override_ratelimit" + ) + + channel = self.make_request( + "GET", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual("Can only lookup local users", channel.json_body["error"]) + + channel = self.make_request( + "POST", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual( + "Only local users can be rate-limited", channel.json_body["error"] + ) + + channel = self.make_request( + "DELETE", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual( + "Only local users can be rate-limited", channel.json_body["error"] + ) + + def test_invalid_parameter(self): + """ + If parameters are invalid, an error is returned. + """ + # messages_per_second is a string + channel = self.make_request( + "POST", + self.url, + access_token=self.admin_user_tok, + content={"messages_per_second": "string"}, + ) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + + # messages_per_second is negative + channel = self.make_request( + "POST", + self.url, + access_token=self.admin_user_tok, + content={"messages_per_second": -1}, + ) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + + # burst_count is a string + channel = self.make_request( + "POST", + self.url, + access_token=self.admin_user_tok, + content={"burst_count": "string"}, + ) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + + # burst_count is negative + channel = self.make_request( + "POST", + self.url, + access_token=self.admin_user_tok, + content={"burst_count": -1}, + ) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + + def test_success(self): + """ + Rate-limiting (set/update/delete) should succeed for an admin. + """ + # request status + channel = self.make_request( + "GET", + self.url, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertIsNone(channel.json_body["messages_per_second"]) + self.assertIsNone(channel.json_body["burst_count"]) + + # set ratelimit + channel = self.make_request( + "POST", + self.url, + access_token=self.admin_user_tok, + content={"messages_per_second": 10, "burst_count": 11}, + ) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(10, channel.json_body["messages_per_second"]) + self.assertEqual(11, channel.json_body["burst_count"]) + + # update ratelimit + channel = self.make_request( + "POST", + self.url, + access_token=self.admin_user_tok, + content={"messages_per_second": 20, "burst_count": 21}, + ) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(20, channel.json_body["messages_per_second"]) + self.assertEqual(21, channel.json_body["burst_count"]) + + # request status + channel = self.make_request( + "GET", + self.url, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(20, channel.json_body["messages_per_second"]) + self.assertEqual(21, channel.json_body["burst_count"]) + + # delete ratelimit + channel = self.make_request( + "DELETE", + self.url, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertNotIn("messages_per_second", channel.json_body) + self.assertNotIn("burst_count", channel.json_body) + + # request status + channel = self.make_request( + "GET", + self.url, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertIsNone(channel.json_body["messages_per_second"]) + self.assertIsNone(channel.json_body["burst_count"]) From 9762a50c265673ee063fe3630950e47271d55384 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Mar 2021 22:43:45 +0100 Subject: [PATCH 2/7] changelog and small fix for mypy because of broken dev --- changelog.d/9648.feature | 1 + synapse/rest/client/v1/room.py | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 changelog.d/9648.feature diff --git a/changelog.d/9648.feature b/changelog.d/9648.feature new file mode 100644 index 000000000000..bc77026039cb --- /dev/null +++ b/changelog.d/9648.feature @@ -0,0 +1 @@ +Add an admin API to manage ratelimit for a specific user. \ No newline at end of file diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index b7aa82a6543a..e9e66d67b46a 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -42,6 +42,7 @@ parse_json_object_from_request, parse_string, ) +from synapse.http.site import SynapseRequest from synapse.logging.opentracing import set_tag from synapse.rest.client.transactions import HttpTransactionCache from synapse.rest.client.v2_alpha._base import client_patterns @@ -1010,7 +1011,7 @@ def __init__(self, hs: "HomeServer"): self._auth = hs.get_auth() self._space_summary_handler = hs.get_space_summary_handler() - async def on_GET(self, request: Request, room_id: str) -> Tuple[int, JsonDict]: + async def on_GET(self, request: SynapseRequest, room_id: str) -> Tuple[int, JsonDict]: requester = await self._auth.get_user_by_req(request, allow_guest=True) return 200, await self._space_summary_handler.get_space_summary( @@ -1020,7 +1021,7 @@ async def on_GET(self, request: Request, room_id: str) -> Tuple[int, JsonDict]: max_rooms_per_space=parse_integer(request, "max_rooms_per_space"), ) - async def on_POST(self, request: Request, room_id: str) -> Tuple[int, JsonDict]: + async def on_POST(self, request: SynapseRequest, room_id: str) -> Tuple[int, JsonDict]: requester = await self._auth.get_user_by_req(request, allow_guest=True) content = parse_json_object_from_request(request) From ce450760a93d050f270fcae4bff94fd32483f972 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Mar 2021 22:54:10 +0100 Subject: [PATCH 3/7] lint --- synapse/rest/client/v1/room.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index e9e66d67b46a..6c722d634ddb 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -21,8 +21,6 @@ from typing import TYPE_CHECKING, List, Optional, Tuple from urllib import parse as urlparse -from twisted.web.server import Request - from synapse.api.constants import EventTypes, Membership from synapse.api.errors import ( AuthError, @@ -1011,7 +1009,9 @@ def __init__(self, hs: "HomeServer"): self._auth = hs.get_auth() self._space_summary_handler = hs.get_space_summary_handler() - async def on_GET(self, request: SynapseRequest, room_id: str) -> Tuple[int, JsonDict]: + async def on_GET( + self, request: SynapseRequest, room_id: str + ) -> Tuple[int, JsonDict]: requester = await self._auth.get_user_by_req(request, allow_guest=True) return 200, await self._space_summary_handler.get_space_summary( @@ -1021,7 +1021,9 @@ async def on_GET(self, request: SynapseRequest, room_id: str) -> Tuple[int, Json max_rooms_per_space=parse_integer(request, "max_rooms_per_space"), ) - async def on_POST(self, request: SynapseRequest, room_id: str) -> Tuple[int, JsonDict]: + async def on_POST( + self, request: SynapseRequest, room_id: str + ) -> Tuple[int, JsonDict]: requester = await self._auth.get_user_by_req(request, allow_guest=True) content = parse_json_object_from_request(request) From 44f947071e865850c142509720445caf85611a2f Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 18 Mar 2021 23:24:39 +0100 Subject: [PATCH 4/7] Typo in tests --- tests/rest/admin/test_user.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index a9f88caff825..285c22b7f54a 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -3024,7 +3024,7 @@ def test_user_is_not_local(self): self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual( - "Only local users can be rate-limited", channel.json_body["error"] + "Only local users can be ratelimited", channel.json_body["error"] ) channel = self.make_request( @@ -3035,7 +3035,7 @@ def test_user_is_not_local(self): self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual( - "Only local users can be rate-limited", channel.json_body["error"] + "Only local users can be ratelimited", channel.json_body["error"] ) def test_invalid_parameter(self): From c933cbbfa4bd1e9833d03045141ab2cdde9274cd Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 22 Mar 2021 22:12:24 +0100 Subject: [PATCH 5/7] Change `GET` if no custom ratelimit is set to `{}` --- docs/admin_api/user_admin_api.rst | 11 ++++++++--- synapse/rest/admin/users.py | 14 +++++--------- synapse/storage/databases/main/room.py | 19 ++++++++++--------- tests/rest/admin/test_user.py | 10 +++++----- 4 files changed, 28 insertions(+), 26 deletions(-) diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst index 9f286a3ffda0..1f6e1e864888 100644 --- a/docs/admin_api/user_admin_api.rst +++ b/docs/admin_api/user_admin_api.rst @@ -827,7 +827,7 @@ The following parameters should be set in the URL: Override ratelimiting for users =============================== -This API allows to override or disable rate limiting for a specific user. +This API allows to override or disable ratelimiting for a specific user. There are specific APIs to set, get and delete a ratelimit. Get status of ratelimit @@ -861,11 +861,16 @@ The following parameters should be set in the URL: The following fields are returned in the JSON response body: - ``messages_per_second`` - integer - The number of actions that can - be performed in a second. + be performed in a second. `0` or `null` mean that ratelimiting is disabled + for this user. - ``burst_count`` - integer - How many actions that can be performed before being limited. -If **no** custom ratelimit is set, the values are ``null``. +If **no** custom ratelimit is set, an empty JSON dict is returned. + +.. code:: json + + {} Set ratelimit ------------- diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index ca3babf587d8..9326c2137772 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -1021,16 +1021,12 @@ async def on_GET( ratelimit = await self.store.get_ratelimit_for_user(user_id) if ratelimit: - messages_per_second = ratelimit.messages_per_second - burst_count = ratelimit.burst_count + ret = { + "messages_per_second": ratelimit.messages_per_second, + "burst_count": ratelimit.burst_count, + } else: - messages_per_second = None - burst_count = None - - ret = { - "messages_per_second": messages_per_second, - "burst_count": burst_count, - } + ret = {} return 200, ret diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 1be6ac4ba284..ba12797201cc 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -521,11 +521,13 @@ def _get_rooms_paginate_txn(txn): ) @cached(max_entries=10000) - async def get_ratelimit_for_user(self, user_id) -> Optional[RatelimitOverride]: + async def get_ratelimit_for_user( + self, user_id: str + ) -> Optional[RatelimitOverride]: """Check if there are any overrides for ratelimiting for the given user Args: - user_id + user_id: user ID of the user Returns: RatelimitOverride if there is an override, else None. If the contents of RatelimitOverride are None or 0 then ratelimitng has been @@ -550,11 +552,11 @@ async def get_ratelimit_for_user(self, user_id) -> Optional[RatelimitOverride]: async def set_ratelimit_for_user( self, user_id: str, messages_per_second: int, burst_count: int ) -> None: - """Sets whether a user shadow-banned. + """Sets whether a user is set an overridden ratelimit. Args: - user: user ID of the user to test - messages_per_second: - burst_count: + user_id: user ID of the user + messages_per_second: The number of actions that can be performed in a second. + burst_count: How many actions that can be performed before being limited. """ def set_ratelimit_txn(txn): @@ -575,10 +577,9 @@ def set_ratelimit_txn(txn): await self.db_pool.runInteraction("set_ratelimit", set_ratelimit_txn) async def delete_ratelimit_for_user(self, user_id: str) -> None: - """Sets whether a user shadow-banned. + """Delete an overridden ratelimit for a user. Args: - user: user ID of the user to test - shadow_banned: true iff the user is to be shadow-banned, false otherwise. + user_id: user ID of the user """ def delete_ratelimit_txn(txn): diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 285c22b7f54a..d68820582f4e 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2916,7 +2916,7 @@ def prepare(self, reactor, clock, hs): def test_no_auth(self): """ - Try to get information of an user without authentication. + Try to get information of a user without authentication. """ channel = self.make_request("GET", self.url, b"{}") @@ -3097,8 +3097,8 @@ def test_success(self): access_token=self.admin_user_tok, ) self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) - self.assertIsNone(channel.json_body["messages_per_second"]) - self.assertIsNone(channel.json_body["burst_count"]) + self.assertNotIn("messages_per_second", channel.json_body) + self.assertNotIn("burst_count", channel.json_body) # set ratelimit channel = self.make_request( @@ -3149,5 +3149,5 @@ def test_success(self): access_token=self.admin_user_tok, ) self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) - self.assertIsNone(channel.json_body["messages_per_second"]) - self.assertIsNone(channel.json_body["burst_count"]) + self.assertNotIn("messages_per_second", channel.json_body) + self.assertNotIn("burst_count", channel.json_body) From c242bf7e5b00da6a1676ce0c19a066e517b52c21 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 22 Mar 2021 22:17:09 +0100 Subject: [PATCH 6/7] lint --- synapse/storage/databases/main/room.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index ba12797201cc..47fb12f3f64f 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -521,9 +521,7 @@ def _get_rooms_paginate_txn(txn): ) @cached(max_entries=10000) - async def get_ratelimit_for_user( - self, user_id: str - ) -> Optional[RatelimitOverride]: + async def get_ratelimit_for_user(self, user_id: str) -> Optional[RatelimitOverride]: """Check if there are any overrides for ratelimiting for the given user Args: @@ -577,7 +575,7 @@ def set_ratelimit_txn(txn): await self.db_pool.runInteraction("set_ratelimit", set_ratelimit_txn) async def delete_ratelimit_for_user(self, user_id: str) -> None: - """Delete an overridden ratelimit for a user. + """Delete an overridden ratelimit for a user. Args: user_id: user ID of the user """ From a13722b9bf897bcbd42750c302f57d24784ec98a Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 10 Apr 2021 13:31:27 +0200 Subject: [PATCH 7/7] add conversion from `null` to `0` --- docs/admin_api/user_admin_api.rst | 13 ++++++------- synapse/rest/admin/users.py | 10 ++++++++-- tests/rest/admin/test_user.py | 26 ++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst index 6e71697e5145..dbce9c90b6c7 100644 --- a/docs/admin_api/user_admin_api.rst +++ b/docs/admin_api/user_admin_api.rst @@ -202,7 +202,7 @@ 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`). + - ``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``. @@ -902,10 +902,9 @@ The following parameters should be set in the URL: The following fields are returned in the JSON response body: - ``messages_per_second`` - integer - The number of actions that can - be performed in a second. `0` or `null` mean that ratelimiting is disabled - for this user. -- ``burst_count`` - integer - How many actions that can be performed - before being limited. + be performed in a second. `0` mean that ratelimiting is disabled for this user. +- ``burst_count`` - integer - How many actions that can be performed before + being limited. If **no** custom ratelimit is set, an empty JSON dict is returned. @@ -954,8 +953,8 @@ The following fields are returned in the JSON response body: - ``messages_per_second`` - integer - The number of actions that can be performed in a second. -- ``burst_count`` - integer - How many actions that can be performed - before being limited. +- ``burst_count`` - integer - How many actions that can be performed before + being limited. Delete ratelimit ---------------- diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 35278b5dd4d4..04990c71fb57 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -1020,9 +1020,15 @@ async def on_GET( ratelimit = await self.store.get_ratelimit_for_user(user_id) if ratelimit: + # convert `null` to `0` for consistency + # both values do the same in retelimit handler ret = { - "messages_per_second": ratelimit.messages_per_second, - "burst_count": ratelimit.burst_count, + "messages_per_second": 0 + if ratelimit.messages_per_second is None + else ratelimit.messages_per_second, + "burst_count": 0 + if ratelimit.burst_count is None + else ratelimit.burst_count, } else: ret = {} diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index a0b12d337252..fb488b0724d5 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -3205,6 +3205,32 @@ def test_invalid_parameter(self): self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + def test_return_zero_when_null(self): + """ + If values in database are `null` API should return an int `0` + """ + + self.get_success( + self.store.db_pool.simple_upsert( + table="ratelimit_override", + keyvalues={"user_id": self.other_user}, + values={ + "messages_per_second": None, + "burst_count": None, + }, + ) + ) + + # request status + channel = self.make_request( + "GET", + self.url, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(0, channel.json_body["messages_per_second"]) + self.assertEqual(0, channel.json_body["burst_count"]) + def test_success(self): """ Rate-limiting (set/update/delete) should succeed for an admin.