From 9a695b086e2b4437d192def245801f3e173b1541 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 3 Jul 2023 17:38:01 +0200 Subject: [PATCH 01/18] Implements admin API to lock an user --- changelog.d/15870.feature | 1 + synapse/api/auth/internal.py | 7 +++ synapse/api/errors.py | 2 + synapse/handlers/admin.py | 1 + synapse/rest/admin/users.py | 17 +++++++ .../storage/databases/main/registration.py | 50 +++++++++++++++++++ .../main/delta/78/05_users_alter_locked.sql | 16 ++++++ 7 files changed, 94 insertions(+) create mode 100644 changelog.d/15870.feature create mode 100644 synapse/storage/schema/main/delta/78/05_users_alter_locked.sql diff --git a/changelog.d/15870.feature b/changelog.d/15870.feature new file mode 100644 index 000000000000..c2601ef4867a --- /dev/null +++ b/changelog.d/15870.feature @@ -0,0 +1 @@ +Implements admin API to lock an user. diff --git a/synapse/api/auth/internal.py b/synapse/api/auth/internal.py index e2ae198b196e..c4ee72c0b6f8 100644 --- a/synapse/api/auth/internal.py +++ b/synapse/api/auth/internal.py @@ -126,6 +126,13 @@ async def _wrapped_get_user_by_req( access_token, allow_expired=allow_expired ) + if await self.store.get_user_locked_status(requester.user.to_string()): + raise AuthError( + 403, + "User account has been locked", + errcode=Codes.USER_LOCKED, + ) + # Deny the request if the user account has expired. # This check is only done for regular users, not appservice ones. if not allow_expired: diff --git a/synapse/api/errors.py b/synapse/api/errors.py index af894243f8d3..59510b6e9fe6 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -80,6 +80,8 @@ class Codes(str, Enum): WEAK_PASSWORD = "M_WEAK_PASSWORD" INVALID_SIGNATURE = "M_INVALID_SIGNATURE" USER_DEACTIVATED = "M_USER_DEACTIVATED" + # USER_LOCKED = "M_USER_LOCKED" + USER_LOCKED = "ORG_MATRIX_MSC3939_USER_LOCKED" # Part of MSC3848 # https://github.com/matrix-org/matrix-spec-proposals/pull/3848 diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 119c7f838481..0e812a6d8b51 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -67,6 +67,7 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: "name", "admin", "deactivated", + "locked", "shadow_banned", "creation_ts", "appservice_id", diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 407fe9c8043a..3913b401ee54 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -271,6 +271,17 @@ async def on_PUT( HTTPStatus.BAD_REQUEST, "'deactivated' parameter is not of type boolean" ) + lock = body.get("locked", False) + if not isinstance(lock, bool): + raise SynapseError( + HTTPStatus.BAD_REQUEST, "'locked' parameter is not of type boolean" + ) + + if deactivate and lock: + raise SynapseError( + HTTPStatus.BAD_REQUEST, "An user can't be deactivated and locked" + ) + approved: Optional[bool] = None if "approved" in body and self._msc3866_enabled: approved = body["approved"] @@ -388,6 +399,12 @@ async def on_PUT( target_user.to_string() ) + if "locked" in body: + if lock and not user["locked"]: + await self.store.set_user_locked_status(user_id, True) + elif not lock and user["lock"]: + await self.store.set_user_locked_status(user_id, False) + if "user_type" in body: await self.store.set_user_type(target_user, user_type) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 676d03bb7e14..97ee9a97b940 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1116,6 +1116,27 @@ async def get_user_deactivated_status(self, user_id: str) -> bool: # Convert the integer into a boolean. return res == 1 + @cached() + async def get_user_locked_status(self, user_id: str) -> bool: + """Retrieve the value for the `locked` property for the provided user. + + Args: + user_id: The ID of the user to retrieve the status for. + + Returns: + True if the user was locked, false if the user is still active. + """ + + res = await self.db_pool.simple_select_one_onecol( + table="users", + keyvalues={"name": user_id}, + retcol="locked", + desc="get_user_locked_status", + ) + + # Convert the integer into a boolean. + return res == 1 + async def get_threepid_validation_session( self, medium: Optional[str], @@ -2111,6 +2132,35 @@ def set_user_deactivated_status_txn( self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,)) txn.call_after(self.is_guest.invalidate, (user_id,)) + async def set_user_locked_status(self, user_id: str, locked: bool) -> None: + """Set the `locked` property for the provided user to the provided value. + + Args: + user_id: The ID of the user to set the status for. + locked: The value to set for `locked`. + """ + + await self.db_pool.runInteraction( + "set_user_locked_status", + self.set_user_locked_status_txn, + user_id, + locked, + ) + + def set_user_locked_status_txn( + self, txn: LoggingTransaction, user_id: str, locked: bool + ) -> None: + self.db_pool.simple_update_one_txn( + txn=txn, + table="users", + keyvalues={"name": user_id}, + updatevalues={"locked": 1 if locked else 0}, + ) + self._invalidate_cache_and_stream(txn, self.get_user_locked_status, (user_id,)) + self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,)) + # TODO is it useful ? + txn.call_after(self.is_guest.invalidate, (user_id,)) + def update_user_approval_status_txn( self, txn: LoggingTransaction, user_id: str, approved: bool ) -> None: diff --git a/synapse/storage/schema/main/delta/78/05_users_alter_locked.sql b/synapse/storage/schema/main/delta/78/05_users_alter_locked.sql new file mode 100644 index 000000000000..b4506e53d9a2 --- /dev/null +++ b/synapse/storage/schema/main/delta/78/05_users_alter_locked.sql @@ -0,0 +1,16 @@ +/* Copyright 2023 The Matrix.org Foundation C.I.C. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +ALTER TABLE users ADD locked SMALLINT DEFAULT 0 NOT NULL; From 26058fe4f0e8731dbdde9611277b8a818dd6195f Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 4 Jul 2023 09:30:31 +0200 Subject: [PATCH 02/18] Hide locked users from user dir --- synapse/config/user_directory.py | 1 + synapse/handlers/user_directory.py | 5 ++++- synapse/storage/databases/main/user_directory.py | 11 ++++++++++- tests/api/test_auth.py | 3 +++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/synapse/config/user_directory.py b/synapse/config/user_directory.py index c9e18b91e9d2..f60ec2ea66b7 100644 --- a/synapse/config/user_directory.py +++ b/synapse/config/user_directory.py @@ -35,3 +35,4 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.user_directory_search_prefer_local_users = user_directory_config.get( "prefer_local_users", False ) + self.show_locked_users = user_directory_config.get("show_locked_users", False) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 05197edc9546..a0f5568000f0 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -94,6 +94,7 @@ def __init__(self, hs: "HomeServer"): self.is_mine_id = hs.is_mine_id self.update_user_directory = hs.config.worker.should_update_user_directory self.search_all_users = hs.config.userdirectory.user_directory_search_all_users + self.show_locked_users = hs.config.userdirectory.show_locked_users self._spam_checker_module_callbacks = hs.get_module_api_callbacks().spam_checker self._hs = hs @@ -144,7 +145,9 @@ async def search_users( ] } """ - results = await self.store.search_user_dir(user_id, search_term, limit) + results = await self.store.search_user_dir( + user_id, search_term, limit, self.show_locked_users + ) # Remove any spammy users from the results. non_spammy_users = [] diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index b0a06baf4f0c..8c61f7cb5dce 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -1003,7 +1003,11 @@ async def get_user_directory_stream_pos(self) -> Optional[int]: ) async def search_user_dir( - self, user_id: str, search_term: str, limit: int + self, + user_id: str, + search_term: str, + limit: int, + show_locked_users: bool = False, ) -> SearchResult: """Searches for users in directory @@ -1037,6 +1041,9 @@ async def search_user_dir( ) """ + if not show_locked_users: + where_clause += " AND locked != 1" + # We allow manipulating the ranking algorithm by injecting statements # based on config options. additional_ordering_statements = [] @@ -1068,6 +1075,7 @@ async def search_user_dir( SELECT d.user_id AS user_id, display_name, avatar_url FROM matching_users as t INNER JOIN user_directory AS d USING (user_id) + INNER JOIN users AS u ON t.user_id = u.name WHERE %(where_clause)s ORDER BY @@ -1123,6 +1131,7 @@ async def search_user_dir( SELECT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search as t INNER JOIN user_directory AS d USING (user_id) + INNER JOIN users AS u ON t.user_id = u.name WHERE %(where_clause)s AND value MATCH ? diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index cdb0048122a1..ce96574915fd 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -69,6 +69,7 @@ def test_get_user_by_req_user_valid_token(self) -> None: ) self.store.get_user_by_access_token = simple_async_mock(user_info) self.store.mark_access_token_as_used = simple_async_mock(None) + self.store.get_user_locked_status = simple_async_mock(False) request = Mock(args={}) request.args[b"access_token"] = [self.test_token] @@ -293,6 +294,7 @@ def test_get_user_by_req__puppeted_token__not_tracking_puppeted_mau(self) -> Non ) self.store.insert_client_ip = simple_async_mock(None) self.store.mark_access_token_as_used = simple_async_mock(None) + self.store.get_user_locked_status = simple_async_mock(False) request = Mock(args={}) request.getClientAddress.return_value.host = "127.0.0.1" request.args[b"access_token"] = [self.test_token] @@ -311,6 +313,7 @@ def test_get_user_by_req__puppeted_token__tracking_puppeted_mau(self) -> None: token_used=True, ) ) + self.store.get_user_locked_status = simple_async_mock(False) self.store.insert_client_ip = simple_async_mock(None) self.store.mark_access_token_as_used = simple_async_mock(None) request = Mock(args={}) From 7a92a3d750226acd3893d4d831e103237c18579f Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 4 Jul 2023 09:46:47 +0200 Subject: [PATCH 03/18] Prettier --- synapse/storage/databases/main/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 8c61f7cb5dce..35b1eccefb86 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -1042,7 +1042,7 @@ async def search_user_dir( """ if not show_locked_users: - where_clause += " AND locked != 1" + where_clause += " AND u.locked != 1" # We allow manipulating the ranking algorithm by injecting statements # based on config options. From d7c2a78c00541555bfd38ffc491944206b1c4af7 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 4 Jul 2023 12:30:29 +0200 Subject: [PATCH 04/18] Fix join --- synapse/storage/databases/main/user_directory.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 35b1eccefb86..e890490b9221 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -1042,7 +1042,7 @@ async def search_user_dir( """ if not show_locked_users: - where_clause += " AND u.locked != 1" + where_clause += " AND (u.locked IS NULL OR u.locked = 0)" # We allow manipulating the ranking algorithm by injecting statements # based on config options. @@ -1075,7 +1075,7 @@ async def search_user_dir( SELECT d.user_id AS user_id, display_name, avatar_url FROM matching_users as t INNER JOIN user_directory AS d USING (user_id) - INNER JOIN users AS u ON t.user_id = u.name + LEFT JOIN users AS u ON t.user_id = u.name WHERE %(where_clause)s ORDER BY @@ -1131,7 +1131,7 @@ async def search_user_dir( SELECT d.user_id AS user_id, display_name, avatar_url FROM user_directory_search as t INNER JOIN user_directory AS d USING (user_id) - INNER JOIN users AS u ON t.user_id = u.name + LEFT JOIN users AS u ON t.user_id = u.name WHERE %(where_clause)s AND value MATCH ? From 0199a9266c838cc15ac8f1ce413fe11d65a0f0cd Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 4 Jul 2023 16:45:05 +0200 Subject: [PATCH 05/18] Add test and fix code --- docs/admin_api/user_admin_api.md | 1 + synapse/rest/admin/users.py | 2 +- .../storage/databases/main/registration.py | 2 +- tests/rest/admin/test_user.py | 26 +++++++++++++++++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 229942b3112f..0b3b7bb7f895 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -146,6 +146,7 @@ Body parameters: - `admin` - **bool**, optional, defaults to `false`. Whether the user is a homeserver administrator, granting them access to the Admin API, among other things. - `deactivated` - **bool**, optional. If unspecified, deactivation state will be left unchanged. +- `locked` - **bool**, optional. If unspecified, locked state will be left unchanged. Note: the `password` field must also be set if both of the following are true: - `deactivated` is set to `false` and the user was previously deactivated (you are reactivating this user) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 3913b401ee54..c546c9aa9130 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -402,7 +402,7 @@ async def on_PUT( if "locked" in body: if lock and not user["locked"]: await self.store.set_user_locked_status(user_id, True) - elif not lock and user["lock"]: + elif not lock and user["locked"]: await self.store.set_user_locked_status(user_id, False) if "user_type" in body: diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 97ee9a97b940..76a48acce730 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -204,7 +204,7 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: SELECT name, password_hash, is_guest, admin, consent_version, consent_ts, consent_server_notice_sent, appservice_id, creation_ts, user_type, - deactivated, COALESCE(shadow_banned, FALSE) AS shadow_banned, + deactivated, locked, COALESCE(shadow_banned, FALSE) AS shadow_banned, COALESCE(approved, TRUE) AS approved FROM users WHERE name = ? diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 434bb56d4451..1dcd5f202d14 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2386,6 +2386,32 @@ def test_deactivate_user(self) -> None: # This key was removed intentionally. Ensure it is not accidentally re-included. self.assertNotIn("password_hash", channel.json_body) + def test_locked_user(self) -> None: + # User can sync + channel = self.make_request( + "GET", + "/_matrix/client/v3/sync", + access_token=self.other_user_token, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + + # Lock user + channel = self.make_request( + "PUT", + self.url_other_user, + access_token=self.admin_user_tok, + content={"locked": True}, + ) + + # User is not authorized to sync anymore + channel = self.make_request( + "GET", + "/_matrix/client/v3/sync", + access_token=self.other_user_token, + ) + self.assertEqual(403, channel.code, msg=channel.json_body) + self.assertEqual(Codes.USER_LOCKED, channel.json_body["errcode"]) + @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) def test_change_name_deactivate_user_user_directory(self) -> None: """ From d9bbcfad22cfdacbeaea6daeb961b094f994d29c Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 4 Jul 2023 17:20:52 +0200 Subject: [PATCH 06/18] Fix test --- tests/storage/test_registration.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index 05ea802008ad..ba41459d083d 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -48,6 +48,7 @@ def test_register(self) -> None: "creation_ts": 0, "user_type": None, "deactivated": 0, + "locked": 0, "shadow_banned": 0, "approved": 1, }, From 7b4e494f2abd2d1ae09da616e34881f3c678df00 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 5 Jul 2023 12:21:05 +0200 Subject: [PATCH 07/18] Add tests for user dir presence --- tests/rest/admin/test_user.py | 84 ++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 1dcd5f202d14..6ea1b8e70f12 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -29,7 +29,16 @@ from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError from synapse.api.room_versions import RoomVersions from synapse.media.filepath import MediaFilePaths -from synapse.rest.client import devices, login, logout, profile, register, room, sync +from synapse.rest.client import ( + devices, + login, + logout, + profile, + register, + room, + sync, + user_directory, +) from synapse.server import HomeServer from synapse.types import JsonDict, UserID, create_requester from synapse.util import Clock @@ -1399,6 +1408,7 @@ class UserRestTestCase(unittest.HomeserverTestCase): login.register_servlets, sync.register_servlets, register.register_servlets, + user_directory.register_servlets, ] def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: @@ -2412,6 +2422,78 @@ def test_locked_user(self) -> None: self.assertEqual(403, channel.code, msg=channel.json_body) self.assertEqual(Codes.USER_LOCKED, channel.json_body["errcode"]) + @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) + def test_locked_user_not_in_user_dir(self) -> None: + # User is available in the user dir + channel = self.make_request( + "POST", + "/_matrix/client/v3/user_directory/search", + {"search_term": self.other_user}, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("results", channel.json_body) + self.assertEqual(1, len(channel.json_body["results"])) + + # Lock user + channel = self.make_request( + "PUT", + self.url_other_user, + access_token=self.admin_user_tok, + content={"locked": True}, + ) + + # User is not available anymore in the user dir + channel = self.make_request( + "POST", + "/_matrix/client/v3/user_directory/search", + {"search_term": self.other_user}, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("results", channel.json_body) + self.assertEqual(0, len(channel.json_body["results"])) + + @override_config( + { + "user_directory": { + "enabled": True, + "search_all_users": True, + "show_locked_users": True, + } + } + ) + def test_locked_user_in_user_dir_with_show_locked_users_option(self) -> None: + # User is available in the user dir + channel = self.make_request( + "POST", + "/_matrix/client/v3/user_directory/search", + {"search_term": self.other_user}, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("results", channel.json_body) + self.assertEqual(1, len(channel.json_body["results"])) + + # Lock user + channel = self.make_request( + "PUT", + self.url_other_user, + access_token=self.admin_user_tok, + content={"locked": True}, + ) + + # User is still available in the user dir + channel = self.make_request( + "POST", + "/_matrix/client/v3/user_directory/search", + {"search_term": self.other_user}, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("results", channel.json_body) + self.assertEqual(1, len(channel.json_body["results"])) + @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) def test_change_name_deactivate_user_user_directory(self) -> None: """ From 7d8f9f58d40ddac2ad940c0a4a70cf72bda6114d Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 5 Jul 2023 14:24:50 +0200 Subject: [PATCH 08/18] Allow logout for locked users + more docs --- changelog.d/15870.feature | 2 +- docs/usage/configuration/config_documentation.md | 2 ++ synapse/api/auth/__init__.py | 1 + synapse/api/auth/internal.py | 9 +++++++-- synapse/rest/client/logout.py | 8 ++++++-- 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/changelog.d/15870.feature b/changelog.d/15870.feature index c2601ef4867a..527220d637d8 100644 --- a/changelog.d/15870.feature +++ b/changelog.d/15870.feature @@ -1 +1 @@ -Implements admin API to lock an user. +Implements an admin API to lock an user without deactivating them. Based on [MSC3939](https://github.com/matrix-org/matrix-spec-proposals/pull/3939). diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 26d7c7900cbe..297824a9e996 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3576,6 +3576,7 @@ This option has the following sub-options: * `prefer_local_users`: Defines whether to prefer local users in search query results. If set to true, local users are more likely to appear above remote users when searching the user directory. Defaults to false. +* `show_locked_users`: Defines whether to show locked users in search query results. Defaults to false. Example configuration: ```yaml @@ -3583,6 +3584,7 @@ user_directory: enabled: false search_all_users: true prefer_local_users: true + show_locked_users: true ``` --- ### `user_consent` diff --git a/synapse/api/auth/__init__.py b/synapse/api/auth/__init__.py index 90cfe39d7623..bb3f50f2ddbe 100644 --- a/synapse/api/auth/__init__.py +++ b/synapse/api/auth/__init__.py @@ -60,6 +60,7 @@ async def get_user_by_req( request: SynapseRequest, allow_guest: bool = False, allow_expired: bool = False, + allow_locked: bool = False, ) -> Requester: """Get a registered user's ID. diff --git a/synapse/api/auth/internal.py b/synapse/api/auth/internal.py index c4ee72c0b6f8..2291db4d2d1e 100644 --- a/synapse/api/auth/internal.py +++ b/synapse/api/auth/internal.py @@ -58,6 +58,7 @@ async def get_user_by_req( request: SynapseRequest, allow_guest: bool = False, allow_expired: bool = False, + allow_locked: bool = False, ) -> Requester: """Get a registered user's ID. @@ -79,7 +80,7 @@ async def get_user_by_req( parent_span = active_span() with start_active_span("get_user_by_req"): requester = await self._wrapped_get_user_by_req( - request, allow_guest, allow_expired + request, allow_guest, allow_expired, allow_locked ) if parent_span: @@ -107,6 +108,7 @@ async def _wrapped_get_user_by_req( request: SynapseRequest, allow_guest: bool, allow_expired: bool, + allow_locked: bool, ) -> Requester: """Helper for get_user_by_req @@ -126,7 +128,10 @@ async def _wrapped_get_user_by_req( access_token, allow_expired=allow_expired ) - if await self.store.get_user_locked_status(requester.user.to_string()): + # Deny the request if the user account is locked. + if not allow_locked and await self.store.get_user_locked_status( + requester.user.to_string() + ): raise AuthError( 403, "User account has been locked", diff --git a/synapse/rest/client/logout.py b/synapse/rest/client/logout.py index 94ad90942f39..2e104d488889 100644 --- a/synapse/rest/client/logout.py +++ b/synapse/rest/client/logout.py @@ -40,7 +40,9 @@ def __init__(self, hs: "HomeServer"): self._device_handler = handler async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request, allow_expired=True) + requester = await self.auth.get_user_by_req( + request, allow_expired=True, allow_locked=True + ) if requester.device_id is None: # The access token wasn't associated with a device. @@ -67,7 +69,9 @@ def __init__(self, hs: "HomeServer"): self._device_handler = handler async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request, allow_expired=True) + requester = await self.auth.get_user_by_req( + request, allow_expired=True, allow_locked=True + ) user_id = requester.user.to_string() # first delete all of the user's devices From e0a1a80b7ea331ebdd239a06fe1ffcf1add818e8 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 5 Jul 2023 14:29:45 +0200 Subject: [PATCH 09/18] Update MSC3861DelegatedAuth --- synapse/api/auth/msc3861_delegated.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index bd4fc9c0ee3d..e1ffd7d394ab 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -196,6 +196,7 @@ async def get_user_by_req( request: SynapseRequest, allow_guest: bool = False, allow_expired: bool = False, + allow_locked: bool = False, ) -> Requester: access_token = self.get_access_token_from_request(request) @@ -205,6 +206,16 @@ async def get_user_by_req( # so that we don't provision the user if they don't have enough permission: requester = await self.get_user_by_access_token(access_token, allow_expired) + # Deny the request if the user account is locked. + if not allow_locked and await self.store.get_user_locked_status( + requester.user.to_string() + ): + raise AuthError( + 403, + "User account has been locked", + errcode=Codes.USER_LOCKED, + ) + if not allow_guest and requester.is_guest: raise OAuthInsufficientScopeError([SCOPE_MATRIX_API]) From 65f7bd5d75da54f887b83ceecd59a7b5ad10fc63 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 5 Jul 2023 14:39:48 +0200 Subject: [PATCH 10/18] Missing import --- synapse/api/auth/msc3861_delegated.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index e1ffd7d394ab..6ada581d7fe8 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -27,6 +27,7 @@ from synapse.api.auth.base import BaseAuth from synapse.api.errors import ( AuthError, + Codes, HttpResponseException, InvalidClientTokenError, OAuthInsufficientScopeError, From c8e2c63d8a00bf4abbfff47a652cd0dd52c8ee65 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 7 Aug 2023 15:38:46 +0200 Subject: [PATCH 11/18] Address comments --- synapse/storage/databases/main/registration.py | 2 -- synapse/storage/schema/main/delta/80/01_users_alter_locked.sql | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 6df9bd9feeba..87266adc4e42 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -2158,8 +2158,6 @@ def set_user_locked_status_txn( ) self._invalidate_cache_and_stream(txn, self.get_user_locked_status, (user_id,)) self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,)) - # TODO is it useful ? - txn.call_after(self.is_guest.invalidate, (user_id,)) def update_user_approval_status_txn( self, txn: LoggingTransaction, user_id: str, approved: bool diff --git a/synapse/storage/schema/main/delta/80/01_users_alter_locked.sql b/synapse/storage/schema/main/delta/80/01_users_alter_locked.sql index b4506e53d9a2..21c79714412c 100644 --- a/synapse/storage/schema/main/delta/80/01_users_alter_locked.sql +++ b/synapse/storage/schema/main/delta/80/01_users_alter_locked.sql @@ -13,4 +13,4 @@ * limitations under the License. */ -ALTER TABLE users ADD locked SMALLINT DEFAULT 0 NOT NULL; +ALTER TABLE users ADD locked BOOLEAN DEFAULT FALSE NOT NULL; From bd39b24cad517f1b6740712bef117c5570c5ec39 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 7 Aug 2023 15:56:38 +0200 Subject: [PATCH 12/18] USe boolean sql column --- synapse/storage/databases/main/registration.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 87266adc4e42..bf76840ed54c 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -230,7 +230,7 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: # want to make sure we're returning the right type of data. # Note: when adding a column name to this list, be wary of NULLable columns, # since NULL values will be turned into False. - boolean_columns = ["admin", "deactivated", "shadow_banned", "approved"] + boolean_columns = ["admin", "deactivated", "shadow_banned", "approved", "locked"] for column in boolean_columns: if not isinstance(row[column], bool): row[column] = bool(row[column]) @@ -1135,7 +1135,9 @@ async def get_user_locked_status(self, user_id: str) -> bool: ) # Convert the integer into a boolean. - return res == 1 + if not isinstance(res, bool): + res = bool(res) + return res async def get_threepid_validation_session( self, @@ -2154,7 +2156,7 @@ def set_user_locked_status_txn( txn=txn, table="users", keyvalues={"name": user_id}, - updatevalues={"locked": 1 if locked else 0}, + updatevalues={"locked": locked}, ) self._invalidate_cache_and_stream(txn, self.get_user_locked_status, (user_id,)) self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,)) From b19f2c137679e32f1df54f8accbb758db24ede5f Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 7 Aug 2023 16:02:56 +0200 Subject: [PATCH 13/18] lint --- synapse/storage/databases/main/registration.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index bf76840ed54c..d92da5f30061 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -230,7 +230,13 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: # want to make sure we're returning the right type of data. # Note: when adding a column name to this list, be wary of NULLable columns, # since NULL values will be turned into False. - boolean_columns = ["admin", "deactivated", "shadow_banned", "approved", "locked"] + boolean_columns = [ + "admin", + "deactivated", + "shadow_banned", + "approved", + "locked", + ] for column in boolean_columns: if not isinstance(row[column], bool): row[column] = bool(row[column]) From 329254a56d3cead7bb11d86a68ac7c20c342af6a Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 8 Aug 2023 00:11:06 +0200 Subject: [PATCH 14/18] Fix code --- synapse/storage/databases/main/registration.py | 5 +++-- synapse/storage/databases/main/user_directory.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index d92da5f30061..63e4ee05f027 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -204,8 +204,9 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: SELECT name, password_hash, is_guest, admin, consent_version, consent_ts, consent_server_notice_sent, appservice_id, creation_ts, user_type, - deactivated, locked, COALESCE(shadow_banned, FALSE) AS shadow_banned, - COALESCE(approved, TRUE) AS approved + deactivated, COALESCE(shadow_banned, FALSE) AS shadow_banned, + COALESCE(approved, TRUE) AS approved, + COALESCE(locked, FALSE) AS locked FROM users WHERE name = ? """, diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index a12cb2a813ec..f0dc31fee649 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -1034,7 +1034,7 @@ async def search_user_dir( """ if not show_locked_users: - where_clause += " AND (u.locked IS NULL OR u.locked = 0)" + where_clause += " AND (u.locked IS NULL OR u.locked = FALSE)" # We allow manipulating the ranking algorithm by injecting statements # based on config options. From 307b6df820001fc310ca8053962144f45ab46c57 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 8 Aug 2023 01:03:13 +0200 Subject: [PATCH 15/18] fix port db --- synapse/_scripts/synapse_port_db.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/_scripts/synapse_port_db.py b/synapse/_scripts/synapse_port_db.py index 22c84fbd5b3f..1300aaf63c92 100755 --- a/synapse/_scripts/synapse_port_db.py +++ b/synapse/_scripts/synapse_port_db.py @@ -123,7 +123,7 @@ "redactions": ["have_censored"], "room_stats_state": ["is_federatable"], "rooms": ["is_public", "has_auth_chain_index"], - "users": ["shadow_banned", "approved"], + "users": ["shadow_banned", "approved", "locked"], "un_partial_stated_event_stream": ["rejection_status_changed"], "users_who_share_rooms": ["share_private"], "per_user_experimental_features": ["enabled"], From 71c5f09afee6f313a7dedfa0caad9ecdb1b7c5d9 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 8 Aug 2023 18:11:35 +0200 Subject: [PATCH 16/18] Switch to 401 soft logout --- synapse/api/auth/internal.py | 3 ++- synapse/api/auth/msc3861_delegated.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/api/auth/internal.py b/synapse/api/auth/internal.py index 2291db4d2d1e..6a5fd44ec01c 100644 --- a/synapse/api/auth/internal.py +++ b/synapse/api/auth/internal.py @@ -133,9 +133,10 @@ async def _wrapped_get_user_by_req( requester.user.to_string() ): raise AuthError( - 403, + 401, "User account has been locked", errcode=Codes.USER_LOCKED, + additional_fields={"soft_logout": True}, ) # Deny the request if the user account has expired. diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py index 6ada581d7fe8..9524102a3037 100644 --- a/synapse/api/auth/msc3861_delegated.py +++ b/synapse/api/auth/msc3861_delegated.py @@ -212,9 +212,10 @@ async def get_user_by_req( requester.user.to_string() ): raise AuthError( - 403, + 401, "User account has been locked", errcode=Codes.USER_LOCKED, + additional_fields={"soft_logout": True}, ) if not allow_guest and requester.is_guest: From 02464830135e2bb5ac9db09f260b91ea4fa8d203 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 8 Aug 2023 18:34:49 +0200 Subject: [PATCH 17/18] Fix test --- tests/rest/admin/test_user.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 5cbcc214c5f9..41a959b4d6c7 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2497,8 +2497,9 @@ def test_locked_user(self) -> None: "/_matrix/client/v3/sync", access_token=self.other_user_token, ) - self.assertEqual(403, channel.code, msg=channel.json_body) + self.assertEqual(401, channel.code, msg=channel.json_body) self.assertEqual(Codes.USER_LOCKED, channel.json_body["errcode"]) + self.assertTrue(channel.json_body["soft_logout"]) @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) def test_locked_user_not_in_user_dir(self) -> None: From ddc91e8624005067b08544e039fafa1e61201da1 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 9 Aug 2023 16:13:37 +0200 Subject: [PATCH 18/18] Remove isinstance test when converting to boolean --- synapse/storage/databases/main/registration.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 63e4ee05f027..d3a01d526fb8 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -239,8 +239,7 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: "locked", ] for column in boolean_columns: - if not isinstance(row[column], bool): - row[column] = bool(row[column]) + row[column] = bool(row[column]) return row @@ -1141,10 +1140,8 @@ async def get_user_locked_status(self, user_id: str) -> bool: desc="get_user_locked_status", ) - # Convert the integer into a boolean. - if not isinstance(res, bool): - res = bool(res) - return res + # Convert the potential integer into a boolean. + return bool(res) async def get_threepid_validation_session( self,