From f7c9743d3ab4666bd9bf9f4a015569d7eb6351bc Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 18 Aug 2022 17:02:24 +0100 Subject: [PATCH 01/20] Add experimental config options and constant for MSC3866 --- synapse/api/errors.py | 2 ++ synapse/config/experimental.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index e6dea89c6d09..a2f1d9e1c357 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -100,6 +100,8 @@ class Codes(str, Enum): UNREDACTED_CONTENT_DELETED = "FI.MAU.MSC2815_UNREDACTED_CONTENT_DELETED" + USER_AWAITING_APPROVAL = "ORG.MATRIX.MSC3866_USER_AWAITING_APPROVAL" + class CodeMessageException(RuntimeError): """An exception with integer code and message string attributes. diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index c1ff41753994..82c80c86e0e4 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -14,10 +14,25 @@ from typing import Any +import attr + from synapse.config._base import Config from synapse.types import JsonDict +@attr.s(auto_attribs=True, frozen=True, slots=True) +class MSC3866Config: + """Configuration for MSC3866 (mandating approval for new users)""" + + # Whether the base support for the approval process is enabled. This includes the + # ability for administrators to check and update the approval of users, even if no + # approval is currently required. + enabled: bool = False + # Whether to require that new users are approved by an admin before their account + # can be used. Note that this setting is ignored if 'enabled' is false. + require_approval_for_new_accounts: bool = False + + class ExperimentalConfig(Config): """Config section for enabling experimental features""" @@ -93,3 +108,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # MSC3852: Expose last seen user agent field on /_matrix/client/v3/devices. self.msc3852_enabled: bool = experimental.get("msc3852_enabled", False) + + # MSC3866: M_USER_AWAITING_APPROVAL error code + raw_msc3866_config = experimental.get("msc3866", {}) + self.msc3866 = MSC3866Config(**raw_msc3866_config) From 1eaff087af9d6c0985a44e168089cbee0651d9f7 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 18 Aug 2022 17:05:09 +0100 Subject: [PATCH 02/20] Add storage support for checking and updating a user's approval status --- .../storage/databases/main/registration.py | 129 ++++++++++++++++++ .../main/delta/72/04users_approved_column.sql | 22 +++ tests/storage/test_registration.py | 105 +++++++++++++- 3 files changed, 255 insertions(+), 1 deletion(-) create mode 100644 synapse/storage/schema/main/delta/72/04users_approved_column.sql diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index cb63cd9b7dcc..cdf8df7b43aa 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -181,6 +181,7 @@ async def get_user_by_id(self, user_id: str) -> Optional[Dict[str, Any]]: "user_type", "deactivated", "shadow_banned", + "approved", ], allow_none=True, desc="get_user_by_id", @@ -1778,6 +1779,26 @@ async def is_guest(self, user_id: str) -> bool: return res if res else False + @cached() + async def is_user_approved(self, user_id: str) -> bool: + """Checks if a user is approved and therefore can be allowed to log in. + + Args: + user_id: the user to check the approval status of. + + Returns: + A boolean that is True if the user is approved, False otherwise. + """ + ret = await self.db_pool.simple_select_one_onecol( + table="users", + keyvalues={"name": user_id}, + retcol="approved", + allow_none=True, + desc="is_user_pending_approval", + ) + + return bool(ret) + class RegistrationBackgroundUpdateStore(RegistrationWorkerStore): def __init__( @@ -1817,6 +1838,10 @@ def __init__( unique=False, ) + self.db_pool.updates.register_background_update_handler( + "users_set_approved_flag", self._background_update_set_approved_flag + ) + async def _background_update_set_deactivated_flag( self, progress: JsonDict, batch_size: int ) -> int: @@ -1915,6 +1940,75 @@ 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 _background_update_set_approved_flag( + self, progress: JsonDict, batch_size: int + ) -> int: + """Set the 'approved' flag for all already existing users. We want to set it to + true systematically because we don't want to suddenly prevent already existing + users from logging in if the option to block registration on approval is turned + on. + """ + last_user = progress.get("user_id", "") + + def _background_update_set_approved_flag_txn(txn: LoggingTransaction) -> int: + txn.execute( + """ + SELECT name + FROM users + WHERE + approved IS NULL + AND name > ? + ORDER BY name ASC + LIMIT ? + """, + (last_user, batch_size), + ) + rows = self.db_pool.cursor_to_dict(txn) + + if len(rows) == 0: + return 0 + + for user in rows: + self.update_user_approval_status_txn(txn, user["name"], True) + + self.db_pool.updates._background_update_progress_txn( + txn, "users_set_approved_flag", {"user_id": rows[-1]["name"]} + ) + + return len(rows) + + nb_processed = await self.db_pool.runInteraction( + "users_set_approved_flag", _background_update_set_approved_flag_txn + ) + + if nb_processed < batch_size: + await self.db_pool.updates._end_background_update("users_set_approved_flag") + + return nb_processed + + def update_user_approval_status_txn( + self, txn: LoggingTransaction, user_id: str, approved: bool + ) -> None: + """Set the user's 'approved' flag to the given value. + + The boolean is turned into an int because the column is a smallint. + + Args: + txn: the current database transaction. + user_id: the user to update the flag for. + approved: the value to set the flag to. + """ + self.db_pool.simple_update_one_txn( + txn=txn, + table="users", + keyvalues={"name": user_id}, + updatevalues={"approved": int(approved)}, + ) + + # Invalidate the caches of methods that read the value of the 'approved' flag. + self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,)) + self._invalidate_cache_and_stream(txn, self.is_user_approved, (user_id,)) + class RegistrationStore(StatsStore, RegistrationBackgroundUpdateStore): def __init__( @@ -1932,6 +2026,13 @@ def __init__( self._access_tokens_id_gen = IdGenerator(db_conn, "access_tokens", "id") self._refresh_tokens_id_gen = IdGenerator(db_conn, "refresh_tokens", "id") + # If support for MSC3866 is enabled and configured to require approval for new + # account, we will create new users with an 'approved' flag set to false. + self._require_approval = ( + hs.config.experimental.msc3866.enabled + and hs.config.experimental.msc3866.require_approval_for_new_accounts + ) + async def add_access_token_to_user( self, user_id: str, @@ -2064,6 +2165,7 @@ async def register_user( admin: bool = False, user_type: Optional[str] = None, shadow_banned: bool = False, + approved: bool = False, ) -> None: """Attempts to register an account. @@ -2082,6 +2184,8 @@ async def register_user( or None for a normal user. shadow_banned: Whether the user is shadow-banned, i.e. they may be told their requests succeeded but we ignore them. + approved: Whether to consider the user has already been approved by an + administrator. Raises: StoreError if the user_id could not be registered. @@ -2098,6 +2202,7 @@ async def register_user( admin, user_type, shadow_banned, + approved, ) def _register_user( @@ -2112,11 +2217,14 @@ def _register_user( admin: bool, user_type: Optional[str], shadow_banned: bool, + approved: bool, ) -> None: user_id_obj = UserID.from_string(user_id) now = int(self._clock.time()) + pending_approval = self._require_approval and not approved + try: if was_guest: # Ensure that the guest user actually exists @@ -2142,6 +2250,7 @@ def _register_user( "admin": 1 if admin else 0, "user_type": user_type, "shadow_banned": shadow_banned, + "approved": 0 if pending_approval else 1, }, ) else: @@ -2157,6 +2266,7 @@ def _register_user( "admin": 1 if admin else 0, "user_type": user_type, "shadow_banned": shadow_banned, + "approved": 0 if pending_approval else 1, }, ) @@ -2499,6 +2609,25 @@ def start_or_continue_validation_session_txn(txn: LoggingTransaction) -> None: start_or_continue_validation_session_txn, ) + async def update_user_approval_status( + self, user_id: UserID, approved: bool + ) -> None: + """Set the user's 'approved' flag to the given value. + + The boolean will be turned into an int (in update_user_approval_status_txn) + because the column is a smallint. + + Args: + user_id: the user to update the flag for. + approved: the value to set the flag to. + """ + await self.db_pool.runInteraction( + "update_user_approval_status", + self.update_user_approval_status_txn, + user_id.to_string(), + approved, + ) + def find_max_generated_user_id_localpart(cur: Cursor) -> int: """ diff --git a/synapse/storage/schema/main/delta/72/04users_approved_column.sql b/synapse/storage/schema/main/delta/72/04users_approved_column.sql new file mode 100644 index 000000000000..c0793d4d9cc7 --- /dev/null +++ b/synapse/storage/schema/main/delta/72/04users_approved_column.sql @@ -0,0 +1,22 @@ +/* Copyright 2022 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. + */ + +-- Add a column to the users table to track whether the user needs to be approved by an +-- administrator. +ALTER TABLE users ADD COLUMN approved SMALLINT; + +-- Run a background update to set the approved flag on already existing users. +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES + (7204, 'users_set_approved_flag', '{}'); diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index a49ac1525ec7..07d0ed4132b9 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -11,11 +11,15 @@ # 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. +from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import UserTypes from synapse.api.errors import ThreepidValidationError +from synapse.server import HomeServer +from synapse.types import JsonDict, UserID +from synapse.util import Clock -from tests.unittest import HomeserverTestCase +from tests.unittest import HomeserverTestCase, override_config class RegistrationStoreTestCase(HomeserverTestCase): @@ -44,6 +48,7 @@ def test_register(self): "user_type": None, "deactivated": 0, "shadow_banned": 0, + "approved": 1, }, (self.get_success(self.store.get_user_by_id(self.user_id))), ) @@ -147,3 +152,101 @@ def test_3pid_inhibit_invalid_validation_session_error(self): ThreepidValidationError, ) self.assertEqual(e.value.msg, "Validation token not found or has expired", e) + + +class ApprovalRequiredRegistrationTestCase(HomeserverTestCase): + def default_config(self) -> JsonDict: + config = super().default_config() + + # If there's already some config for this feature in the default config, it + # means we're overriding it with @override_config. In this case we don't want + # to do anything more with it. + msc3866_config = config.get("experimental_features", {}).get("msc3866") + if msc3866_config is not None: + return config + + # Require approval for all new accounts. + config["experimental_features"] = { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + return config + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = hs.get_datastores().main + self.user_id = "@my-user:test" + self.pwhash = "{xx1}123456789" + + @override_config( + { + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": False, + } + } + } + ) + def test_approval_not_required(self) -> None: + """Tests that if we don't require approval for new accounts, newly created + accounts are automatically marked as approved. + """ + self.get_success(self.store.register_user(self.user_id, self.pwhash)) + + user = self.get_success(self.store.get_user_by_id(self.user_id)) + assert user is not None + self.assertEqual(user["approved"], 1) + + approved = self.get_success(self.store.is_user_approved(self.user_id)) + self.assertTrue(approved) + + def test_approval_required(self) -> None: + """Tests that if we require approval for new accounts, newly created accounts + are not automatically marked as approved. + """ + self.get_success(self.store.register_user(self.user_id, self.pwhash)) + + user = self.get_success(self.store.get_user_by_id(self.user_id)) + assert user is not None + self.assertEqual(user["approved"], 0) + + approved = self.get_success(self.store.is_user_approved(self.user_id)) + self.assertFalse(approved) + + def test_override(self) -> None: + """Tests that if we require approval for new accounts, but we explicitly say the + new user should be considered approved, they're marked as approved. + """ + self.get_success( + self.store.register_user( + self.user_id, + self.pwhash, + approved=True, + ) + ) + + user = self.get_success(self.store.get_user_by_id(self.user_id)) + self.assertIsNotNone(user) + assert user is not None + self.assertEqual(user["approved"], 1) + + approved = self.get_success(self.store.is_user_approved(self.user_id)) + self.assertTrue(approved) + + def test_approve_user(self) -> None: + """Tests that approving the user updates their approval status.""" + self.get_success(self.store.register_user(self.user_id, self.pwhash)) + + approved = self.get_success(self.store.is_user_approved(self.user_id)) + self.assertFalse(approved) + + self.get_success( + self.store.update_user_approval_status( + UserID.from_string(self.user_id), True + ) + ) + + approved = self.get_success(self.store.is_user_approved(self.user_id)) + self.assertTrue(approved) From 685f76f9bb792511cbba89b711bf4e2f3575b3da Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 18 Aug 2022 17:06:19 +0100 Subject: [PATCH 03/20] Block new accounts after registering if configured to do so --- synapse/rest/client/register.py | 12 ++++++++++++ tests/rest/client/test_register.py | 23 +++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 956c45e60a4b..4a977c060be1 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -433,6 +433,11 @@ def __init__(self, hs: "HomeServer"): hs.config.registration.inhibit_user_in_use_error ) + self._require_approval = ( + hs.config.experimental.msc3866.enabled + and hs.config.experimental.msc3866.require_approval_for_new_accounts + ) + self._registration_flows = _calculate_registration_flows( hs.config, self.auth_handler ) @@ -756,6 +761,13 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: access_token=return_dict.get("access_token"), ) + if self._require_approval: + raise SynapseError( + code=403, + errcode=Codes.USER_AWAITING_APPROVAL, + msg="This account needs to be approved by an administrator before it can be used.", + ) + return 200, return_dict async def _do_appservice_registration( diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index ab4277dd3171..b785c2e6527e 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -765,6 +765,29 @@ def test_inhibit_user_in_use_error(self) -> None: self.assertEqual(channel.code, 400, channel.json_body) self.assertEqual(channel.json_body["errcode"], Codes.USER_IN_USE) + @override_config( + { + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + } + ) + def test_require_approval(self) -> None: + channel = self.make_request( + "POST", + "register", + { + "username": "kermit", + "password": "monkey", + "auth": {"type": LoginType.DUMMY}, + }, + ) + self.assertEqual(403, channel.code, channel.result) + self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) + class AccountValidityTestCase(unittest.HomeserverTestCase): From 5d08fe2bc8428dd02603e562351b83e430d405b0 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 18 Aug 2022 17:07:27 +0100 Subject: [PATCH 04/20] Block login if a user requires approval and the server is configured to do so --- synapse/handlers/auth.py | 11 +++++++++++ synapse/rest/client/login.py | 15 ++++++++++++++ tests/rest/client/test_login.py | 35 +++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index bfa553504442..942f221bb8bf 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1010,6 +1010,17 @@ async def check_user_exists(self, user_id: str) -> Optional[str]: return res[0] return None + async def is_user_approved(self, user_id: str) -> bool: + """Checks if a user is approved and therefore can be allowed to log in. + + Args: + user_id: the user to check the approval status of. + + Returns: + A boolean that is True if the user is approved, False otherwise. + """ + return await self.store.is_user_approved(user_id) + async def _find_user_id_and_pwd_hash( self, user_id: str ) -> Optional[Tuple[str, str]]: diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 0437c87d8d6d..b90015ecc578 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -92,6 +92,12 @@ def __init__(self, hs: "HomeServer"): hs.config.registration.refreshable_access_token_lifetime is not None ) + # Whether we need to check if the user has been approved or not. + self._require_approval = ( + hs.config.experimental.msc3866.enabled + and hs.config.experimental.msc3866.require_approval_for_new_accounts + ) + self.auth = hs.get_auth() self.clock = hs.get_clock() @@ -220,6 +226,15 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]: except KeyError: raise SynapseError(400, "Missing JSON keys.") + if self._require_approval: + approved = await self.auth_handler.is_user_approved(result["user_id"]) + if not approved: + raise SynapseError( + code=403, + errcode=Codes.USER_AWAITING_APPROVAL, + msg="This account is pending approval by a server administrator.", + ) + well_known_data = self._well_known_builder.get_well_known() if well_known_data: result["well_known"] = well_known_data diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index e2a4d982755a..8a65d6638b3f 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -23,6 +23,8 @@ from twisted.web.resource import Resource import synapse.rest.admin +from synapse.api.constants import LoginType +from synapse.api.errors import Codes from synapse.appservice import ApplicationService from synapse.rest.client import devices, login, logout, register from synapse.rest.client.account import WhoamiRestServlet @@ -94,6 +96,7 @@ class LoginRestServletTestCase(unittest.HomeserverTestCase): logout.register_servlets, devices.register_servlets, lambda hs, http_server: WhoamiRestServlet(hs).register(http_server), + register.register_servlets, ] def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: @@ -406,6 +409,38 @@ def test_login_with_overly_long_device_id_fails(self) -> None: self.assertEqual(channel.code, 400) self.assertEqual(channel.json_body["errcode"], "M_INVALID_PARAM") + @override_config( + { + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + } + ) + def test_require_approval(self) -> None: + channel = self.make_request( + "POST", + "register", + { + "username": "kermit", + "password": "monkey", + "auth": {"type": LoginType.DUMMY}, + }, + ) + self.assertEqual(403, channel.code, channel.result) + self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) + + params = { + "type": LoginType.PASSWORD, + "identifier": {"type": "m.id.user", "user": "kermit"}, + "password": "monkey", + } + channel = self.make_request("POST", LOGIN_URL, params) + self.assertEqual(403, channel.code, channel.result) + self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) + @skip_unless(has_saml2 and HAS_OIDC, "Requires SAML2 and OIDC") class MultiSSOTestCase(unittest.HomeserverTestCase): From 7b532a95dca97353a028c052c47ec11c3956aed1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 18 Aug 2022 17:11:00 +0100 Subject: [PATCH 05/20] Change admin APIs to support checking and updating the approval status of a user --- synapse/handlers/admin.py | 5 + synapse/handlers/register.py | 8 + synapse/replication/http/register.py | 5 + synapse/rest/admin/users.py | 43 ++++- synapse/storage/databases/main/__init__.py | 9 +- tests/rest/admin/test_user.py | 175 +++++++++++++++++++++ 6 files changed, 243 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index d4fe7df533a1..883b88f7c231 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -32,6 +32,7 @@ def __init__(self, hs: "HomeServer"): self.store = hs.get_datastores().main self._storage_controllers = hs.get_storage_controllers() self._state_storage_controller = self._storage_controllers.state + self._msc3866_enabled = hs.config.experimental.msc3866.enabled async def get_whois(self, user: UserID) -> JsonDict: connections = [] @@ -74,6 +75,10 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: "is_guest", } + if self._msc3866_enabled: + # Only include the approved flag if support for MSC3866 is enabled. + user_info_to_return.add("approved") + # Restrict returned keys to a known set. user_info_dict = { key: value diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index c77d18172283..31cb486290d9 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -217,6 +217,7 @@ async def register_user( by_admin: bool = False, user_agent_ips: Optional[List[Tuple[str, str]]] = None, auth_provider_id: Optional[str] = None, + approved: bool = False, ) -> str: """Registers a new client on the server. @@ -243,6 +244,8 @@ async def register_user( user_agent_ips: Tuples of user-agents and IP addresses used during the registration process. auth_provider_id: The SSO IdP the user used, if any. + approved: True if the new user should be considered already + approved by an administrator. Returns: The registered user_id. Raises: @@ -304,6 +307,7 @@ async def register_user( user_type=user_type, address=address, shadow_banned=shadow_banned, + approved=approved, ) profile = await self.store.get_profileinfo(localpart) @@ -692,6 +696,7 @@ async def register_with_store( user_type: Optional[str] = None, address: Optional[str] = None, shadow_banned: bool = False, + approved: bool = False, ) -> None: """Register user in the datastore. @@ -710,6 +715,7 @@ async def register_with_store( api.constants.UserTypes, or None for a normal user. address: the IP address used to perform the registration. shadow_banned: Whether to shadow-ban the user + approved: Whether to mark the user as approved by an administrator """ if self.hs.config.worker.worker_app: await self._register_client( @@ -723,6 +729,7 @@ async def register_with_store( user_type=user_type, address=address, shadow_banned=shadow_banned, + approved=approved, ) else: await self.store.register_user( @@ -735,6 +742,7 @@ async def register_with_store( admin=admin, user_type=user_type, shadow_banned=shadow_banned, + approved=approved, ) # Only call the account validity module(s) on the main process, to avoid diff --git a/synapse/replication/http/register.py b/synapse/replication/http/register.py index 6c8f8388fd60..61abb529c8ae 100644 --- a/synapse/replication/http/register.py +++ b/synapse/replication/http/register.py @@ -51,6 +51,7 @@ async def _serialize_payload( # type: ignore[override] user_type: Optional[str], address: Optional[str], shadow_banned: bool, + approved: bool, ) -> JsonDict: """ Args: @@ -68,6 +69,8 @@ async def _serialize_payload( # type: ignore[override] or None for a normal user. address: the IP address used to perform the regitration. shadow_banned: Whether to shadow-ban the user + approved: Whether the user should be considered already approved by an + administrator. """ return { "password_hash": password_hash, @@ -79,6 +82,7 @@ async def _serialize_payload( # type: ignore[override] "user_type": user_type, "address": address, "shadow_banned": shadow_banned, + "approved": approved, } async def _handle_request( # type: ignore[override] @@ -99,6 +103,7 @@ async def _handle_request( # type: ignore[override] user_type=content["user_type"], address=content["address"], shadow_banned=content["shadow_banned"], + approved=content["approved"], ) return 200, {} diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index ba2f7fa6d860..b2a5e5bfd96d 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -69,6 +69,7 @@ def __init__(self, hs: "HomeServer"): self.store = hs.get_datastores().main self.auth = hs.get_auth() self.admin_handler = hs.get_admin_handler() + self._msc3866_enabled = hs.config.experimental.msc3866.enabled async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await assert_requester_is_admin(self.auth, request) @@ -95,6 +96,13 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: guests = parse_boolean(request, "guests", default=True) deactivated = parse_boolean(request, "deactivated", default=False) + # If support for MSC3866 is not enabled, apply no filtering based on the + # `approved` column. + if self._msc3866_enabled: + approved = parse_boolean(request, "approved", default=True) + else: + approved = True + order_by = parse_string( request, "order_by", @@ -115,8 +123,22 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: 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, order_by, direction + start, + limit, + user_id, + name, + guests, + deactivated, + order_by, + direction, + approved, ) + + # If support for MSC3866 is not enabled, don't show the approval flag. + if not self._msc3866_enabled: + for user in users: + del user["approved"] + ret = {"users": users, "total": total} if (start + limit) < total: ret["next_token"] = str(start + len(users)) @@ -163,6 +185,7 @@ def __init__(self, hs: "HomeServer"): self.deactivate_account_handler = hs.get_deactivate_account_handler() self.registration_handler = hs.get_registration_handler() self.pusher_pool = hs.get_pusherpool() + self._msc3866_enabled = hs.config.experimental.msc3866.enabled async def on_GET( self, request: SynapseRequest, user_id: str @@ -239,6 +262,15 @@ async def on_PUT( HTTPStatus.BAD_REQUEST, "'deactivated' parameter is not of type boolean" ) + approved: Optional[bool] = None + if "approved" in body and self._msc3866_enabled: + approved = body["approved"] + if not isinstance(approved, bool): + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "'approved' parameter is not of type boolean", + ) + # convert List[Dict[str, str]] into List[Tuple[str, str]] if external_ids is not None: new_external_ids = [ @@ -343,6 +375,9 @@ async def on_PUT( if "user_type" in body: await self.store.set_user_type(target_user, user_type) + if approved is not None: + await self.store.update_user_approval_status(target_user, approved) + user = await self.admin_handler.get_user(target_user) assert user is not None @@ -355,6 +390,10 @@ async def on_PUT( if password is not None: password_hash = await self.auth_handler.hash(password) + new_user_approved = False + if self._msc3866_enabled and approved is True: + new_user_approved = True + user_id = await self.registration_handler.register_user( localpart=target_user.localpart, password_hash=password_hash, @@ -362,6 +401,7 @@ async def on_PUT( default_display_name=displayname, user_type=user_type, by_admin=True, + approved=new_user_approved, ) if threepids is not None: @@ -550,6 +590,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_type=user_type, default_display_name=displayname, by_admin=True, + approved=True, ) result = await register._create_registration_details(user_id, body) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index 4dccbb732a20..f98c9188b911 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -203,6 +203,7 @@ async def get_users_paginate( deactivated: bool = False, order_by: str = UserSortOrder.USER_ID.value, direction: str = "f", + approved: bool = True, ) -> 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 @@ -217,6 +218,7 @@ async def get_users_paginate( deactivated: whether to include deactivated users order_by: the sort order of the returned list direction: sort ascending or descending + approved: whether to include approved users Returns: A tuple of a list of mappings from user to information and a count of total users. """ @@ -249,6 +251,11 @@ def get_users_paginate_txn( if not deactivated: filters.append("deactivated = 0") + if not approved: + # We ignore NULL values for the approved flag because these should only + # be already existing users that we consider as already approved. + filters.append("approved = 0") + where_clause = "WHERE " + " AND ".join(filters) if len(filters) > 0 else "" sql_base = f""" @@ -262,7 +269,7 @@ def get_users_paginate_txn( sql = f""" SELECT name, user_type, is_guest, admin, deactivated, shadow_banned, - displayname, avatar_url, creation_ts * 1000 as creation_ts + displayname, avatar_url, creation_ts * 1000 as creation_ts, approved {sql_base} ORDER BY {order_by_column} {order}, u.name ASC LIMIT ? OFFSET ? diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 1afd082707c2..0aee06dc565f 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -578,6 +578,16 @@ def _search_test( _search_test(None, "foo", "user_id") _search_test(None, "bar", "user_id") + @override_config( + { + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + } + ) def test_invalid_parameter(self) -> None: """ If parameters are invalid, an error is returned. @@ -623,6 +633,16 @@ def test_invalid_parameter(self) -> None: self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + # invalid approved + channel = self.make_request( + "GET", + self.url + "?approved=not_bool", + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + # unkown order_by channel = self.make_request( "GET", @@ -841,6 +861,69 @@ def test_order_by(self) -> None: self._order_test([self.admin_user, user1, user2], "creation_ts", "f") self._order_test([user2, user1, self.admin_user], "creation_ts", "b") + @override_config( + { + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + } + ) + def test_filter_out_approved(self) -> None: + """Tests that the endpoint can filter out approved users.""" + # Create our users. + self._create_users(2) + + # Get the list of users. + channel = self.make_request( + "GET", + self.url, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, channel.result) + + # Exclude the admin, because we don't want to accidentally un-approve the admin. + non_admin_user_ids = [ + user["name"] + for user in channel.json_body["users"] + if user["name"] != self.admin_user + ] + + self.assertEqual(2, len(non_admin_user_ids), non_admin_user_ids) + + # Select a user and un-approve them. We do this rather than the other way around + # because, since these users are created by an admin, we consider them already + # approved. + not_approved_user = non_admin_user_ids[0] + + channel = self.make_request( + "PUT", + f"/_synapse/admin/v2/users/{not_approved_user}", + {"approved": False}, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, channel.result) + + # Now get the list of users again, this time filtering out approved users. + channel = self.make_request( + "GET", + self.url + "?approved=false", + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, channel.result) + + non_admin_user_ids = [ + user["name"] + for user in channel.json_body["users"] + if user["name"] != self.admin_user + ] + + # We should only have our unapproved user now. + self.assertEqual(1, len(non_admin_user_ids), non_admin_user_ids) + self.assertEqual(not_approved_user, non_admin_user_ids[0]) + def _order_test( self, expected_user_list: List[str], @@ -2536,6 +2619,98 @@ def test_accidental_deactivation_prevention(self) -> None: # Ensure they're still alive self.assertEqual(0, channel.json_body["deactivated"]) + @override_config( + { + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + } + ) + def test_approve_account(self) -> None: + """Tests that approving an account correctly sets the approved flag for the user.""" + url = self.url_prefix % "@bob:test" + + # Create user + channel = self.make_request( + "PUT", + url, + access_token=self.admin_user_tok, + content={"password": "abc123"}, + ) + + self.assertEqual(201, channel.code, msg=channel.json_body) + self.assertEqual(0, channel.json_body["approved"]) + + # Get user + channel = self.make_request( + "GET", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(0, channel.json_body["approved"]) + + # Approve user + channel = self.make_request( + "PUT", + url, + access_token=self.admin_user_tok, + content={"approved": True}, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(1, channel.json_body["approved"]) + + # Check that the user is now approved + channel = self.make_request( + "GET", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(1, channel.json_body["approved"]) + + @override_config( + { + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + } + ) + def test_register_approved(self) -> None: + url = self.url_prefix % "@bob:test" + + # Create user + channel = self.make_request( + "PUT", + url, + access_token=self.admin_user_tok, + content={"password": "abc123", "approved": True}, + ) + + self.assertEqual(201, channel.code, msg=channel.json_body) + self.assertEqual("@bob:test", channel.json_body["name"]) + self.assertEqual(1, channel.json_body["approved"]) + + # Get user + channel = self.make_request( + "GET", + url, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("@bob:test", channel.json_body["name"]) + self.assertEqual(1, channel.json_body["approved"]) + def _is_erased(self, user_id: str, expect: bool) -> None: """Assert that the user is erased or not""" d = self.store.is_user_erased(user_id) From eedaed134fd847a93b4d5c4791f992d9e71a0807 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 18 Aug 2022 17:18:14 +0100 Subject: [PATCH 06/20] Changelog --- changelog.d/13556.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13556.feature diff --git a/changelog.d/13556.feature b/changelog.d/13556.feature new file mode 100644 index 000000000000..f9d63db6c0b9 --- /dev/null +++ b/changelog.d/13556.feature @@ -0,0 +1 @@ +Allow server admins to require a manual approval process before new accounts can be used (using [MSC3866](https://github.com/matrix-org/matrix-spec-proposals/pull/3866)). From ffaea1e9ec2bc714cd91c9b60c9c6bd66932cb2a Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 30 Aug 2022 14:18:36 +0100 Subject: [PATCH 07/20] Use a boolean in the database schema --- synapse/storage/databases/main/registration.py | 10 +++++----- .../schema/main/delta/72/04users_approved_column.sql | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index cdf8df7b43aa..53aa7dee6068 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1797,7 +1797,7 @@ async def is_user_approved(self, user_id: str) -> bool: desc="is_user_pending_approval", ) - return bool(ret) + return ret class RegistrationBackgroundUpdateStore(RegistrationWorkerStore): @@ -2002,7 +2002,7 @@ def update_user_approval_status_txn( txn=txn, table="users", keyvalues={"name": user_id}, - updatevalues={"approved": int(approved)}, + updatevalues={"approved": approved}, ) # Invalidate the caches of methods that read the value of the 'approved' flag. @@ -2223,7 +2223,7 @@ def _register_user( now = int(self._clock.time()) - pending_approval = self._require_approval and not approved + user_approved = approved or not self._require_approval try: if was_guest: @@ -2250,7 +2250,7 @@ def _register_user( "admin": 1 if admin else 0, "user_type": user_type, "shadow_banned": shadow_banned, - "approved": 0 if pending_approval else 1, + "approved": user_approved, }, ) else: @@ -2266,7 +2266,7 @@ def _register_user( "admin": 1 if admin else 0, "user_type": user_type, "shadow_banned": shadow_banned, - "approved": 0 if pending_approval else 1, + "approved": user_approved, }, ) diff --git a/synapse/storage/schema/main/delta/72/04users_approved_column.sql b/synapse/storage/schema/main/delta/72/04users_approved_column.sql index c0793d4d9cc7..afbae90e8c18 100644 --- a/synapse/storage/schema/main/delta/72/04users_approved_column.sql +++ b/synapse/storage/schema/main/delta/72/04users_approved_column.sql @@ -15,7 +15,7 @@ -- Add a column to the users table to track whether the user needs to be approved by an -- administrator. -ALTER TABLE users ADD COLUMN approved SMALLINT; +ALTER TABLE users ADD COLUMN approved BOOLEAN; -- Run a background update to set the approved flag on already existing users. INSERT INTO background_updates (ordering, update_name, progress_json) VALUES From 0230200fe2c3a5aafecdca5ee4ed5ec69304fe16 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 31 Aug 2022 17:28:14 +0100 Subject: [PATCH 08/20] Incorporate review --- synapse/_scripts/synapse_port_db.py | 2 +- synapse/rest/client/register.py | 4 +++- synapse/storage/databases/main/registration.py | 5 ++++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/synapse/_scripts/synapse_port_db.py b/synapse/_scripts/synapse_port_db.py index 543bba27c29e..26d670d34b71 100755 --- a/synapse/_scripts/synapse_port_db.py +++ b/synapse/_scripts/synapse_port_db.py @@ -106,7 +106,7 @@ "redactions": ["have_censored"], "room_stats_state": ["is_federatable"], "local_media_repository": ["safe_from_quarantine"], - "users": ["shadow_banned"], + "users": ["shadow_banned", "approved"], "e2e_fallback_keys_json": ["used"], "access_tokens": ["used"], "device_lists_changes_in_room": ["converted_to_destinations"], diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 4a977c060be1..668387a86b57 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -812,7 +812,9 @@ async def _create_registration_details( "user_id": user_id, "home_server": self.hs.hostname, } - if not params.get("inhibit_login", False): + # We don't want to log the user in if we're going to deny them access because + # they need to be approved first. + if not params.get("inhibit_login", False) and not self._require_approval: device_id = params.get("device_id") initial_display_name = params.get("initial_device_display_name") ( diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 53aa7dee6068..58187a56f0aa 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1797,7 +1797,10 @@ async def is_user_approved(self, user_id: str) -> bool: desc="is_user_pending_approval", ) - return ret + # We need to handle the case of the column being None because even though the + # background update *should* have taken care of setting a value for all users, + # we can't be certain it's finished running. + return bool(ret) class RegistrationBackgroundUpdateStore(RegistrationWorkerStore): From 868ab645e4ff9edc3dce8ca2dd2968a8ba8c8d60 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 21 Sep 2022 15:39:56 +0100 Subject: [PATCH 09/20] Incorporate review --- .../storage/databases/main/registration.py | 81 +++++-------------- .../03users_approved_column.sql} | 4 - 2 files changed, 21 insertions(+), 64 deletions(-) rename synapse/storage/schema/main/delta/{72/04users_approved_column.sql => 73/03users_approved_column.sql} (79%) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 978ce464eef1..8282d014ee28 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1784,25 +1784,36 @@ async def is_guest(self, user_id: str) -> bool: async def is_user_approved(self, user_id: str) -> bool: """Checks if a user is approved and therefore can be allowed to log in. + If the user's 'approved' column is NULL, we consider it as true given it means + the user was registered when support for an approval flow was either disabled + or nonexistent. + Args: user_id: the user to check the approval status of. Returns: A boolean that is True if the user is approved, False otherwise. """ - ret = await self.db_pool.simple_select_one_onecol( - table="users", - keyvalues={"name": user_id}, - retcol="approved", - allow_none=True, + + def is_user_approved_txn(txn: LoggingTransaction) -> bool: + txn.execute( + """ + SELECT COALESCE(approved, TRUE) AS approved FROM users WHERE name = ? + """, + (user_id,), + ) + + rows = self.db_pool.cursor_to_dict(txn) + + # We cast to bool because the value returned by the database engine might + # be an integer if we're using SQLite. + return bool(rows[0]["approved"]) + + return await self.db_pool.runInteraction( desc="is_user_pending_approval", + func=is_user_approved_txn, ) - # We need to handle the case of the column being None because even though the - # background update *should* have taken care of setting a value for all users, - # we can't be certain it's finished running. - return bool(ret) - class RegistrationBackgroundUpdateStore(RegistrationWorkerStore): def __init__( @@ -1842,10 +1853,6 @@ def __init__( unique=False, ) - self.db_pool.updates.register_background_update_handler( - "users_set_approved_flag", self._background_update_set_approved_flag - ) - async def _background_update_set_deactivated_flag( self, progress: JsonDict, batch_size: int ) -> int: @@ -1944,52 +1951,6 @@ 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 _background_update_set_approved_flag( - self, progress: JsonDict, batch_size: int - ) -> int: - """Set the 'approved' flag for all already existing users. We want to set it to - true systematically because we don't want to suddenly prevent already existing - users from logging in if the option to block registration on approval is turned - on. - """ - last_user = progress.get("user_id", "") - - def _background_update_set_approved_flag_txn(txn: LoggingTransaction) -> int: - txn.execute( - """ - SELECT name - FROM users - WHERE - approved IS NULL - AND name > ? - ORDER BY name ASC - LIMIT ? - """, - (last_user, batch_size), - ) - rows = self.db_pool.cursor_to_dict(txn) - - if len(rows) == 0: - return 0 - - for user in rows: - self.update_user_approval_status_txn(txn, user["name"], True) - - self.db_pool.updates._background_update_progress_txn( - txn, "users_set_approved_flag", {"user_id": rows[-1]["name"]} - ) - - return len(rows) - - nb_processed = await self.db_pool.runInteraction( - "users_set_approved_flag", _background_update_set_approved_flag_txn - ) - - if nb_processed < batch_size: - await self.db_pool.updates._end_background_update("users_set_approved_flag") - - return nb_processed - def update_user_approval_status_txn( self, txn: LoggingTransaction, user_id: str, approved: bool ) -> None: diff --git a/synapse/storage/schema/main/delta/72/04users_approved_column.sql b/synapse/storage/schema/main/delta/73/03users_approved_column.sql similarity index 79% rename from synapse/storage/schema/main/delta/72/04users_approved_column.sql rename to synapse/storage/schema/main/delta/73/03users_approved_column.sql index afbae90e8c18..9f201ab62b03 100644 --- a/synapse/storage/schema/main/delta/72/04users_approved_column.sql +++ b/synapse/storage/schema/main/delta/73/03users_approved_column.sql @@ -16,7 +16,3 @@ -- Add a column to the users table to track whether the user needs to be approved by an -- administrator. ALTER TABLE users ADD COLUMN approved BOOLEAN; - --- Run a background update to set the approved flag on already existing users. -INSERT INTO background_updates (ordering, update_name, progress_json) VALUES - (7204, 'users_set_approved_flag', '{}'); From 8d091b4d2c4de3ec73212ba552b71edfcb304d72 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 22 Sep 2022 10:53:15 +0100 Subject: [PATCH 10/20] Correctly filter on bools, not ints --- synapse/storage/databases/main/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/__init__.py b/synapse/storage/databases/main/__init__.py index f98c9188b911..939dae331fd3 100644 --- a/synapse/storage/databases/main/__init__.py +++ b/synapse/storage/databases/main/__init__.py @@ -254,7 +254,7 @@ def get_users_paginate_txn( if not approved: # We ignore NULL values for the approved flag because these should only # be already existing users that we consider as already approved. - filters.append("approved = 0") + filters.append("approved IS FALSE") where_clause = "WHERE " + " AND ".join(filters) if len(filters) > 0 else "" From a87d2f71b50cf39015cbd1dd060666cf56f8c085 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 22 Sep 2022 12:27:39 +0100 Subject: [PATCH 11/20] Don't create a new device if the new user needs approval --- synapse/rest/client/login.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index b90015ecc578..ce882a4e3451 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -55,11 +55,11 @@ class LoginResponse(TypedDict, total=False): user_id: str - access_token: str + access_token: Optional[str] home_server: str expires_in_ms: Optional[int] refresh_token: Optional[str] - device_id: str + device_id: Optional[str] well_known: Optional[Dict[str, Any]] @@ -371,6 +371,16 @@ async def _complete_login( errcode=Codes.INVALID_PARAM, ) + if self._require_approval: + approved = await self.auth_handler.is_user_approved(user_id) + if not approved: + # If the user isn't approved (and needs to be) we won't allow them to + # actually log in, so we don't want to create a device/access token. + return LoginResponse( + user_id=user_id, + home_server=self.hs.hostname, + ) + initial_display_name = login_submission.get("initial_device_display_name") ( device_id, From 08d85f5d7858fb9027def1ea554e004970d523fc Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 22 Sep 2022 12:58:16 +0100 Subject: [PATCH 12/20] Test that we raise the error on SSO logins --- tests/rest/client/test_auth.py | 21 +++++++++++++++++++++ tests/rest/client/utils.py | 10 +++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 05355c7fb6d8..c7d4dbf5f0df 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -21,6 +21,7 @@ import synapse.rest.admin from synapse.api.constants import LoginType +from synapse.api.errors import Codes from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker from synapse.rest.client import account, auth, devices, login, logout, register from synapse.rest.synapse.client import build_synapse_client_resource_tree @@ -567,6 +568,26 @@ def test_ui_auth_fails_for_incorrect_sso_user(self) -> None: body={"auth": {"session": session_id}}, ) + @skip_unless(HAS_OIDC, "requires OIDC") + @override_config( + { + "oidc_config": TEST_OIDC_CONFIG, + "experimental_features": { + "msc3866": { + "enabled": True, + "require_approval_for_new_accounts": True, + } + } + } + ) + def test_sso_not_approved(self) -> None: + """Tests that if we register a user via SSO while requiring approval for new + accounts, we still raise the correct error before logging the user in. + """ + login_resp = self.helper.login_via_oidc("username", expected_status=403) + + self.assertEqual(login_resp["errcode"], Codes.USER_AWAITING_APPROVAL) + class RefreshAuthTests(unittest.HomeserverTestCase): servlets = [ diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index dd26145bf8c1..c9f174aed859 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -543,8 +543,12 @@ def upload_media( return channel.json_body - def login_via_oidc(self, remote_user_id: str) -> JsonDict: - """Log in (as a new user) via OIDC + def login_via_oidc( + self, + remote_user_id: str, + expected_status: int = 200, + ) -> JsonDict: + """Log in via OIDC Returns the result of the final token login. @@ -578,7 +582,7 @@ def login_via_oidc(self, remote_user_id: str) -> JsonDict: "/login", content={"type": "m.login.token", "token": login_token}, ) - assert channel.code == HTTPStatus.OK + assert channel.code == expected_status, f"unexpected status in response: {channel.code}" return channel.json_body def auth_via_oidc( From 7585098a5d8205da0f90f7b474f98b40beaa08b3 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 22 Sep 2022 13:00:22 +0100 Subject: [PATCH 13/20] Test that we don't register devices for users needing approval --- tests/rest/client/test_auth.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index c7d4dbf5f0df..b0e3e96cef02 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -588,6 +588,13 @@ def test_sso_not_approved(self) -> None: self.assertEqual(login_resp["errcode"], Codes.USER_AWAITING_APPROVAL) + # Check that we didn't register a device for the user during the login attempt. + devices = self.get_success( + self.hs.get_datastores().main.get_devices_by_user("@username:test") + ) + + self.assertEqual(len(devices), 0) + class RefreshAuthTests(unittest.HomeserverTestCase): servlets = [ From 75cf99963e1613def8c1954626d672878976b8bc Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 22 Sep 2022 14:10:46 +0100 Subject: [PATCH 14/20] Lint --- tests/rest/client/test_auth.py | 2 +- tests/rest/client/utils.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index b0e3e96cef02..1669728b6349 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -577,7 +577,7 @@ def test_ui_auth_fails_for_incorrect_sso_user(self) -> None: "enabled": True, "require_approval_for_new_accounts": True, } - } + }, } ) def test_sso_not_approved(self) -> None: diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index c9f174aed859..c249a42bb641 100644 --- a/tests/rest/client/utils.py +++ b/tests/rest/client/utils.py @@ -582,7 +582,9 @@ def login_via_oidc( "/login", content={"type": "m.login.token", "token": login_token}, ) - assert channel.code == expected_status, f"unexpected status in response: {channel.code}" + assert ( + channel.code == expected_status + ), f"unexpected status in response: {channel.code}" return channel.json_body def auth_via_oidc( From df0c887e6cd8fd2cfa1a9d69227bdc1525cdaf7c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 27 Sep 2022 14:10:19 +0100 Subject: [PATCH 15/20] Incorporate review --- synapse/rest/admin/users.py | 6 +-- .../storage/databases/main/registration.py | 46 +++++++++++-------- .../main/delta/73/03users_approved_column.sql | 2 + tests/storage/test_registration.py | 4 +- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 42ebf2586c15..15ac2059aabe 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -390,9 +390,9 @@ async def on_PUT( if password is not None: password_hash = await self.auth_handler.hash(password) - new_user_approved = False - if self._msc3866_enabled and approved is True: - new_user_approved = True + new_user_approved = True + if self._msc3866_enabled and approved is not None: + new_user_approved = approved user_id = await self.registration_handler.register_user( localpart=target_user.localpart, diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 8282d014ee28..ac249a2c1f78 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -166,26 +166,34 @@ def __init__( @cached() async def get_user_by_id(self, user_id: str) -> Optional[Dict[str, Any]]: """Deprecated: use get_userinfo_by_id instead""" - return await self.db_pool.simple_select_one( - table="users", - keyvalues={"name": user_id}, - retcols=[ - "name", - "password_hash", - "is_guest", - "admin", - "consent_version", - "consent_ts", - "consent_server_notice_sent", - "appservice_id", - "creation_ts", - "user_type", - "deactivated", - "shadow_banned", - "approved", - ], - allow_none=True, + + def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: + # We could technically use simple_select_one here, but it would not perform + # the COALESCEs (unless hacked into the column names), which could yield + # confusing results. + txn.execute( + """ + 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, + COALESCE(approved, TRUE) AS approved + FROM users + WHERE name = ? + """, + (user_id,), + ) + + rows = self.db_pool.cursor_to_dict(txn) + + if len(rows) == 0: + return None + + return rows[0] + + return await self.db_pool.runInteraction( desc="get_user_by_id", + func=get_user_by_id_txn, ) async def get_userinfo_by_id(self, user_id: str) -> Optional[UserInfo]: diff --git a/synapse/storage/schema/main/delta/73/03users_approved_column.sql b/synapse/storage/schema/main/delta/73/03users_approved_column.sql index 9f201ab62b03..5328d592ea00 100644 --- a/synapse/storage/schema/main/delta/73/03users_approved_column.sql +++ b/synapse/storage/schema/main/delta/73/03users_approved_column.sql @@ -15,4 +15,6 @@ -- Add a column to the users table to track whether the user needs to be approved by an -- administrator. +-- A NULL column means the user was created before this feature was supported by Synapse, +-- and should be considered as TRUE. ALTER TABLE users ADD COLUMN approved BOOLEAN; diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index 20ed513eba3c..05ea802008ad 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -213,7 +213,7 @@ def test_approval_not_required(self) -> None: user = self.get_success(self.store.get_user_by_id(self.user_id)) assert user is not None - self.assertEqual(user["approved"], 1) + self.assertTrue(user["approved"]) approved = self.get_success(self.store.is_user_approved(self.user_id)) self.assertTrue(approved) @@ -226,7 +226,7 @@ def test_approval_required(self) -> None: user = self.get_success(self.store.get_user_by_id(self.user_id)) assert user is not None - self.assertEqual(user["approved"], 0) + self.assertFalse(user["approved"]) approved = self.get_success(self.store.is_user_approved(self.user_id)) self.assertFalse(approved) From 3f93dda0fbb48f3ed90edd67ed52d98789da3565 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 29 Sep 2022 10:53:53 +0100 Subject: [PATCH 16/20] Fix test --- tests/rest/admin/test_user.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 077b16ee8988..435d6a49cd29 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -25,10 +25,10 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import UserTypes +from synapse.api.constants import UserTypes, LoginType from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError from synapse.api.room_versions import RoomVersions -from synapse.rest.client import devices, login, logout, profile, room, sync +from synapse.rest.client import devices, login, logout, profile, room, sync, register from synapse.rest.media.v1.filepath import MediaFilePaths from synapse.server import HomeServer from synapse.types import JsonDict, UserID @@ -1355,6 +1355,7 @@ class UserRestTestCase(unittest.HomeserverTestCase): synapse.rest.admin.register_servlets, login.register_servlets, sync.register_servlets, + register.register_servlets, ] def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: @@ -2633,16 +2634,19 @@ def test_approve_account(self) -> None: """Tests that approving an account correctly sets the approved flag for the user.""" url = self.url_prefix % "@bob:test" - # Create user + # Create the user using the client-server API since otherwise the user will be + # marked as approved automatically. channel = self.make_request( - "PUT", - url, - access_token=self.admin_user_tok, - content={"password": "abc123"}, + "POST", + "register", + { + "username": "bob", + "password": "test", + "auth": {"type": LoginType.DUMMY}, + }, ) - - self.assertEqual(201, channel.code, msg=channel.json_body) - self.assertEqual(0, channel.json_body["approved"]) + self.assertEqual(403, channel.code, channel.result) + self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) # Get user channel = self.make_request( @@ -2652,7 +2656,7 @@ def test_approve_account(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(0, channel.json_body["approved"]) + self.assertFalse(channel.json_body["approved"]) # Approve user channel = self.make_request( @@ -2663,7 +2667,7 @@ def test_approve_account(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(1, channel.json_body["approved"]) + self.assertTrue(channel.json_body["approved"]) # Check that the user is now approved channel = self.make_request( @@ -2673,7 +2677,7 @@ def test_approve_account(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(1, channel.json_body["approved"]) + self.assertTrue(channel.json_body["approved"]) @override_config( { From 577967cd5676613cd7fe92e661831945b4caf6bf Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 29 Sep 2022 11:55:36 +0100 Subject: [PATCH 17/20] Lint --- 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 435d6a49cd29..1f22a577974f 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -25,10 +25,10 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import UserTypes, LoginType +from synapse.api.constants import LoginType, UserTypes from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError from synapse.api.room_versions import RoomVersions -from synapse.rest.client import devices, login, logout, profile, room, sync, register +from synapse.rest.client import devices, login, logout, profile, register, room, sync from synapse.rest.media.v1.filepath import MediaFilePaths from synapse.server import HomeServer from synapse.types import JsonDict, UserID From 7a5425ae82cb7ad555c5743e99c1cc77033edcfe Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 29 Sep 2022 12:15:25 +0100 Subject: [PATCH 18/20] Incorporate review --- synapse/storage/databases/main/registration.py | 13 ++++++++++++- tests/rest/admin/test_user.py | 6 +++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index ac249a2c1f78..e6d860cf65d0 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -191,11 +191,22 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: return rows[0] - return await self.db_pool.runInteraction( + row = await self.db_pool.runInteraction( desc="get_user_by_id", func=get_user_by_id_txn, ) + if row is not None: + # If we're using SQLite our boolean values will be integers. Because we + # present some of this data as is to e.g. server admins via REST APIs, we + # want to make sure we're returning the right type of data. + boolean_columns = ["admin", "deactivated", "shadow_banned", "approved"] + for column in boolean_columns: + if not isinstance(row[column], bool): + row[column] = bool(row[column]) + + return row + async def get_userinfo_by_id(self, user_id: str) -> Optional[UserInfo]: """Get a UserInfo object for a user by user ID. diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 1f22a577974f..9ce65ae37b00 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2656,7 +2656,7 @@ def test_approve_account(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertFalse(channel.json_body["approved"]) + self.assertIs(False, channel.json_body["approved"]) # Approve user channel = self.make_request( @@ -2667,7 +2667,7 @@ def test_approve_account(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertTrue(channel.json_body["approved"]) + self.assertIs(True, channel.json_body["approved"]) # Check that the user is now approved channel = self.make_request( @@ -2677,7 +2677,7 @@ def test_approve_account(self) -> None: ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertTrue(channel.json_body["approved"]) + self.assertIs(True, channel.json_body["approved"]) @override_config( { From 560e160d11a91e26614ee34fb88eba508ffd8433 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 29 Sep 2022 12:43:18 +0100 Subject: [PATCH 19/20] Incorporate latest change in the MSC --- synapse/api/constants.py | 11 +++++++++++ synapse/api/errors.py | 14 ++++++++++++++ synapse/rest/client/login.py | 14 ++++++++++---- synapse/rest/client/register.py | 12 ++++++++---- tests/rest/admin/test_user.py | 5 ++++- tests/rest/client/test_auth.py | 5 ++++- tests/rest/client/test_login.py | 8 +++++++- tests/rest/client/test_register.py | 9 ++++++++- 8 files changed, 66 insertions(+), 12 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index c178ddf070b4..c031903b1a32 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -269,3 +269,14 @@ class PublicRoomsFilterFields: GENERIC_SEARCH_TERM: Final = "generic_search_term" ROOM_TYPES: Final = "room_types" + + +class ApprovalNoticeMedium: + """Identifier for the medium this server will use to serve notice of approval for a + specific user's registration. + + As defined in https://github.com/matrix-org/matrix-spec-proposals/blob/babolivier/m_not_approved/proposals/3866-user-not-approved-error.md + """ + + NONE = "org.matrix.msc3866.none" + EMAIL = "org.matrix.msc3866.email" diff --git a/synapse/api/errors.py b/synapse/api/errors.py index de5a5ea5da35..c6062075690e 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -568,6 +568,20 @@ def error_dict(self, config: Optional["HomeServerConfig"]) -> "JsonDict": return cs_error(self.msg, self.errcode, **extra) +class NotApprovedError(SynapseError): + def __init__( + self, + msg: str, + approval_notice_medium: str, + ): + super().__init__( + code=403, + msg=msg, + errcode=Codes.USER_AWAITING_APPROVAL, + additional_fields={"approval_notice_medium": approval_notice_medium}, + ) + + def cs_error(msg: str, code: str = Codes.UNKNOWN, **kwargs: Any) -> "JsonDict": """Utility method for constructing an error response for client-server interactions. diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index ce882a4e3451..f554586ac3c4 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -28,7 +28,14 @@ from typing_extensions import TypedDict -from synapse.api.errors import Codes, InvalidClientTokenError, LoginError, SynapseError +from synapse.api.constants import ApprovalNoticeMedium +from synapse.api.errors import ( + Codes, + InvalidClientTokenError, + LoginError, + NotApprovedError, + SynapseError, +) from synapse.api.ratelimiting import Ratelimiter from synapse.api.urls import CLIENT_API_PREFIX from synapse.appservice import ApplicationService @@ -229,10 +236,9 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]: if self._require_approval: approved = await self.auth_handler.is_user_approved(result["user_id"]) if not approved: - raise SynapseError( - code=403, - errcode=Codes.USER_AWAITING_APPROVAL, + raise NotApprovedError( msg="This account is pending approval by a server administrator.", + approval_notice_medium=ApprovalNoticeMedium.NONE, ) well_known_data = self._well_known_builder.get_well_known() diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index b1830f61263a..de810ae3ec4e 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -21,10 +21,15 @@ import synapse import synapse.api.auth import synapse.types -from synapse.api.constants import APP_SERVICE_REGISTRATION_TYPE, LoginType +from synapse.api.constants import ( + APP_SERVICE_REGISTRATION_TYPE, + ApprovalNoticeMedium, + LoginType, +) from synapse.api.errors import ( Codes, InteractiveAuthIncompleteError, + NotApprovedError, SynapseError, ThreepidValidationError, UnrecognizedRequestError, @@ -740,10 +745,9 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: ) if self._require_approval: - raise SynapseError( - code=403, - errcode=Codes.USER_AWAITING_APPROVAL, + raise NotApprovedError( msg="This account needs to be approved by an administrator before it can be used.", + approval_notice_medium=ApprovalNoticeMedium.NONE, ) return 200, return_dict diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 9ce65ae37b00..4c1ce33463a9 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -25,7 +25,7 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import LoginType, UserTypes +from synapse.api.constants import ApprovalNoticeMedium, LoginType, UserTypes from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError from synapse.api.room_versions import RoomVersions from synapse.rest.client import devices, login, logout, profile, register, room, sync @@ -2647,6 +2647,9 @@ def test_approve_account(self) -> None: ) self.assertEqual(403, channel.code, channel.result) self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) + self.assertEqual( + ApprovalNoticeMedium.NONE, channel.json_body["approval_notice_medium"] + ) # Get user channel = self.make_request( diff --git a/tests/rest/client/test_auth.py b/tests/rest/client/test_auth.py index 1669728b6349..090cef5216de 100644 --- a/tests/rest/client/test_auth.py +++ b/tests/rest/client/test_auth.py @@ -20,7 +20,7 @@ from twisted.web.resource import Resource import synapse.rest.admin -from synapse.api.constants import LoginType +from synapse.api.constants import ApprovalNoticeMedium, LoginType from synapse.api.errors import Codes from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker from synapse.rest.client import account, auth, devices, login, logout, register @@ -587,6 +587,9 @@ def test_sso_not_approved(self) -> None: login_resp = self.helper.login_via_oidc("username", expected_status=403) self.assertEqual(login_resp["errcode"], Codes.USER_AWAITING_APPROVAL) + self.assertEqual( + ApprovalNoticeMedium.NONE, login_resp["approval_notice_medium"] + ) # Check that we didn't register a device for the user during the login attempt. devices = self.get_success( diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 8a65d6638b3f..e801ba8c8b7e 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -23,7 +23,7 @@ from twisted.web.resource import Resource import synapse.rest.admin -from synapse.api.constants import LoginType +from synapse.api.constants import ApprovalNoticeMedium, LoginType from synapse.api.errors import Codes from synapse.appservice import ApplicationService from synapse.rest.client import devices, login, logout, register @@ -431,6 +431,9 @@ def test_require_approval(self) -> None: ) self.assertEqual(403, channel.code, channel.result) self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) + self.assertEqual( + ApprovalNoticeMedium.NONE, channel.json_body["approval_notice_medium"] + ) params = { "type": LoginType.PASSWORD, @@ -440,6 +443,9 @@ def test_require_approval(self) -> None: channel = self.make_request("POST", LOGIN_URL, params) self.assertEqual(403, channel.code, channel.result) self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) + self.assertEqual( + ApprovalNoticeMedium.NONE, channel.json_body["approval_notice_medium"] + ) @skip_unless(has_saml2 and HAS_OIDC, "Requires SAML2 and OIDC") diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index 51fe820dbd39..11cf3939d84b 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -22,7 +22,11 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import APP_SERVICE_REGISTRATION_TYPE, LoginType +from synapse.api.constants import ( + APP_SERVICE_REGISTRATION_TYPE, + ApprovalNoticeMedium, + LoginType, +) from synapse.api.errors import Codes from synapse.appservice import ApplicationService from synapse.rest.client import account, account_validity, login, logout, register, sync @@ -787,6 +791,9 @@ def test_require_approval(self) -> None: ) self.assertEqual(403, channel.code, channel.result) self.assertEqual(Codes.USER_AWAITING_APPROVAL, channel.json_body["errcode"]) + self.assertEqual( + ApprovalNoticeMedium.NONE, channel.json_body["approval_notice_medium"] + ) class AccountValidityTestCase(unittest.HomeserverTestCase): From 7d717123d492e5322dc37120bd9ab3cc66a878ec Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 29 Sep 2022 13:40:36 +0100 Subject: [PATCH 20/20] Add comment to try to catch bool()ing NULLs in the future --- synapse/storage/databases/main/registration.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index e6d860cf65d0..2996d6bb4d66 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -200,6 +200,8 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]: # If we're using SQLite our boolean values will be integers. Because we # present some of this data as is to e.g. server admins via REST APIs, we # 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"] for column in boolean_columns: if not isinstance(row[column], bool):