From c54f5f4c00c150627a820091a864a9c2a673b5ab Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 26 Jan 2022 12:02:54 +0000 Subject: [PATCH 1/9] Add a config flag to inhibit `M_USER_IN_USE` during registration (#11743) This is mostly motivated by the tchap use case, where usernames are automatically generated from the user's email address (in a way that allows figuring out the email address from the username). Therefore, it's an issue if we respond to requests on /register and /register/available with M_USER_IN_USE, because it can potentially leak email addresses (which include the user's real name and place of work). This commit adds a flag to inhibit the M_USER_IN_USE errors that are raised both by /register/available, and when providing a username early into the registration process. This error will still be raised if the user completes the registration process but the username conflicts. This is particularly useful when using modules (https://github.com/matrix-org/synapse/pull/11790 adds a module callback to set the username of users at registration) or SSO, since they can ensure the username is unique. More context is available in the PR that introduced this behaviour to synapse-dinsic: matrix-org/synapse-dinsic#48 - as well as the issue in the matrix-dinsic repo: matrix-org/matrix-dinsic#476 --- changelog.d/11743.feature | 1 + docs/sample_config.yaml | 10 ++++++++ synapse/config/registration.py | 12 +++++++++ synapse/handlers/register.py | 28 ++++++++++---------- synapse/rest/client/register.py | 31 +++++++++++++++++++--- tests/rest/client/test_register.py | 41 ++++++++++++++++++++++++++++++ 6 files changed, 106 insertions(+), 17 deletions(-) create mode 100644 changelog.d/11743.feature diff --git a/changelog.d/11743.feature b/changelog.d/11743.feature new file mode 100644 index 000000000..9809f48b9 --- /dev/null +++ b/changelog.d/11743.feature @@ -0,0 +1 @@ +Add a config flag to inhibit M_USER_IN_USE during registration. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 3e9def56e..39e6cd525 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1564,6 +1564,16 @@ account_threepid_delegates: # #bind_new_user_emails_to_sydent: https://example.com:8091 +# Whether to inhibit errors raised when registering a new account if the user ID +# already exists. If turned on, that requests to /register/available will always +# show a user ID as available, and Synapse won't raise an error when starting +# a registration with a user ID that already exists. However, Synapse will still +# raise an error if the registration completes and the username conflicts. +# +# Defaults to false. +# +#inhibit_user_in_use_error: true + ## Metrics ### diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 20ad33987..431757c76 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -189,6 +189,8 @@ def read_config(self, config, **kwargs): self.bind_new_user_emails_to_sydent.strip("/") ) + self.inhibit_user_in_use_error = config.get("inhibit_user_in_use_error", False) + def generate_config_section(self, generate_secrets=False, **kwargs): if generate_secrets: registration_shared_secret = 'registration_shared_secret: "%s"' % ( @@ -475,6 +477,16 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # without validation. # #bind_new_user_emails_to_sydent: https://example.com:8091 + + # Whether to inhibit errors raised when registering a new account if the user ID + # already exists. If turned on, that requests to /register/available will always + # show a user ID as available, and Synapse won't raise an error when starting + # a registration with a user ID that already exists. However, Synapse will still + # raise an error if the registration completes and the username conflicts. + # + # Defaults to false. + # + #inhibit_user_in_use_error: true """ % locals() ) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index eea041050..40d90cbf6 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -132,6 +132,7 @@ async def check_username( localpart: str, guest_access_token: Optional[str] = None, assigned_user_id: Optional[str] = None, + inhibit_user_in_use_error: bool = False, ) -> None: if types.contains_invalid_mxid_characters(localpart): raise SynapseError( @@ -171,23 +172,22 @@ async def check_username( users = await self.store.get_users_by_id_case_insensitive(user_id) if users: - if not guest_access_token: + if not inhibit_user_in_use_error and not guest_access_token: raise SynapseError( 400, "User ID already taken.", errcode=Codes.USER_IN_USE ) - - # Retrieve guest user information from provided access token - user_data = await self.auth.get_user_by_access_token(guest_access_token) - if ( - not user_data.is_guest - or UserID.from_string(user_data.user_id).localpart != localpart - ): - raise AuthError( - 403, - "Cannot register taken user ID without valid guest " - "credentials for that user.", - errcode=Codes.FORBIDDEN, - ) + if guest_access_token: + user_data = await self.auth.get_user_by_access_token(guest_access_token) + if ( + not user_data.is_guest + or UserID.from_string(user_data.user_id).localpart != localpart + ): + raise AuthError( + 403, + "Cannot register taken user ID without valid guest " + "credentials for that user.", + errcode=Codes.FORBIDDEN, + ) if guest_access_token is None: try: diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index d93501fbd..06c9684a5 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -343,15 +343,28 @@ def __init__(self, hs: "HomeServer"): ), ) + self.inhibit_user_in_use_error = ( + hs.config.registration.inhibit_user_in_use_error + ) + async def on_GET(self, request: Request) -> Tuple[int, JsonDict]: if not self.hs.config.registration.enable_registration: raise SynapseError( 403, "Registration has been disabled", errcode=Codes.FORBIDDEN ) - # We are not interested in logging in via a username in this deployment. - # Simply allow anything here as it won't be used later. - return 200, {"available": True} + if self.inhibit_user_in_use_error: + return 200, {"available": True} + + ip = request.getClientIP() + with self.ratelimiter.ratelimit(ip) as wait_deferred: + await wait_deferred + + username = parse_string(request, "username", required=True) + + await self.registration_handler.check_username(username) + + return 200, {"available": True} class RegistrationTokenValidityRestServlet(RestServlet): @@ -420,6 +433,9 @@ def __init__(self, hs: "HomeServer"): self._msc2918_enabled = ( hs.config.registration.refreshable_access_token_lifetime is not None ) + self._inhibit_user_in_use_error = ( + hs.config.registration.inhibit_user_in_use_error + ) self._registration_flows = _calculate_registration_flows( hs.config, self.auth_handler @@ -553,6 +569,15 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: session_id, UIAuthSessionDataConstants.PASSWORD_HASH, None ) + # Ensure that the username is valid. + if desired_username is not None: + await self.registration_handler.check_username( + desired_username, + guest_access_token=guest_access_token, + assigned_user_id=registered_user_id, + inhibit_user_in_use_error=self._inhibit_user_in_use_error, + ) + # Check if the user-interactive authentication flows are complete, if # not this will raise a user-interactive auth error. try: diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index f1265a824..e1123f156 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -734,6 +734,47 @@ def test_reject_invalid_email(self): {"errcode": "M_UNKNOWN", "error": "Unable to parse email address"}, ) + @override_config( + { + "inhibit_user_in_use_error": True, + } + ) + def test_inhibit_user_in_use_error(self): + """Tests that the 'inhibit_user_in_use_error' configuration flag behaves + correctly. + """ + username = "arthur" + + # Manually register the user, so we know the test isn't passing because of a lack + # of clashing. + reg_handler = self.hs.get_registration_handler() + self.get_success(reg_handler.register_user(username)) + + # Check that /available correctly ignores the username provided despite the + # username being already registered. + channel = self.make_request("GET", "register/available?username=" + username) + self.assertEquals(200, channel.code, channel.result) + + # Test that when starting a UIA registration flow the request doesn't fail because + # of a conflicting username + channel = self.make_request( + "POST", + "register", + {"username": username, "type": "m.login.password", "password": "foo"}, + ) + self.assertEqual(channel.code, 401) + self.assertIn("session", channel.json_body) + + # Test that finishing the registration fails because of a conflicting username. + session = channel.json_body["session"] + channel = self.make_request( + "POST", + "register", + {"auth": {"session": session, "type": LoginType.DUMMY}}, + ) + self.assertEqual(channel.code, 400, channel.json_body) + self.assertEqual(channel.json_body["errcode"], Codes.USER_IN_USE) + class RegisterHideProfileTestCase(unittest.HomeserverTestCase): From 30a3dd3baf8d9d5cdf4b3d60d683a762344233c2 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 26 Jan 2022 14:21:13 +0000 Subject: [PATCH 2/9] Add a module callback to set username at registration (#11790) This is in the context of mainlining the Tchap fork of Synapse. Currently in Tchap usernames are derived from the user's email address (extracted from the UIA results, more specifically the m.login.email.identity step). This change also exports the check_username method from the registration handler as part of the module API, so that a module can check if the username it's trying to generate is correct and doesn't conflict with an existing one, and fallback gracefully if not. Co-authored-by: David Robertson --- changelog.d/11790.feature | 1 + .../password_auth_provider_callbacks.md | 62 ++++++++++++++ synapse/handlers/auth.py | 58 ++++++++++++++ synapse/module_api/__init__.py | 22 +++++ synapse/rest/client/register.py | 13 ++- tests/handlers/test_password_providers.py | 80 ++++++++++++++++++- 6 files changed, 230 insertions(+), 6 deletions(-) create mode 100644 changelog.d/11790.feature diff --git a/changelog.d/11790.feature b/changelog.d/11790.feature new file mode 100644 index 000000000..4a5cc8ec3 --- /dev/null +++ b/changelog.d/11790.feature @@ -0,0 +1 @@ +Add a module callback to set username at registration. diff --git a/docs/modules/password_auth_provider_callbacks.md b/docs/modules/password_auth_provider_callbacks.md index e53abf640..ec8324d29 100644 --- a/docs/modules/password_auth_provider_callbacks.md +++ b/docs/modules/password_auth_provider_callbacks.md @@ -105,6 +105,68 @@ device ID), and the (now deactivated) access token. If multiple modules implement this callback, Synapse runs them all in order. +### `get_username_for_registration` + +_First introduced in Synapse v1.52.0_ + +```python +async def get_username_for_registration( + uia_results: Dict[str, Any], + params: Dict[str, Any], +) -> Optional[str] +``` + +Called when registering a new user. The module can return a username to set for the user +being registered by returning it as a string, or `None` if it doesn't wish to force a +username for this user. If a username is returned, it will be used as the local part of a +user's full Matrix ID (e.g. it's `alice` in `@alice:example.com`). + +This callback is called once [User-Interactive Authentication](https://spec.matrix.org/latest/client-server-api/#user-interactive-authentication-api) +has been completed by the user. It is not called when registering a user via SSO. It is +passed two dictionaries, which include the information that the user has provided during +the registration process. + +The first dictionary contains the results of the [User-Interactive Authentication](https://spec.matrix.org/latest/client-server-api/#user-interactive-authentication-api) +flow followed by the user. Its keys are the identifiers of every step involved in the flow, +associated with either a boolean value indicating whether the step was correctly completed, +or additional information (e.g. email address, phone number...). A list of most existing +identifiers can be found in the [Matrix specification](https://spec.matrix.org/v1.1/client-server-api/#authentication-types). +Here's an example featuring all currently supported keys: + +```python +{ + "m.login.dummy": True, # Dummy authentication + "m.login.terms": True, # User has accepted the terms of service for the homeserver + "m.login.recaptcha": True, # User has completed the recaptcha challenge + "m.login.email.identity": { # User has provided and verified an email address + "medium": "email", + "address": "alice@example.com", + "validated_at": 1642701357084, + }, + "m.login.msisdn": { # User has provided and verified a phone number + "medium": "msisdn", + "address": "33123456789", + "validated_at": 1642701357084, + }, + "org.matrix.msc3231.login.registration_token": "sometoken", # User has registered through the flow described in MSC3231 +} +``` + +The second dictionary contains the parameters provided by the user's client in the request +to `/_matrix/client/v3/register`. See the [Matrix specification](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3register) +for a complete list of these parameters. + +If the module cannot, or does not wish to, generate a username for this user, it must +return `None`. + +If multiple modules implement this callback, they will be considered in order. If a +callback returns `None`, Synapse falls through to the next one. The value of the first +callback that does not return `None` will be used. If this happens, Synapse will not call +any of the subsequent implementations of this callback. If every callback return `None`, +the username provided by the user is used, if any (otherwise one is automatically +generated). + + ## Example The example module below implements authentication checkers for two different login types: diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 4b66a9862..9e4724f62 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1966,6 +1966,10 @@ def run(*args: Tuple, **kwargs: Dict) -> Awaitable: Optional[Tuple[str, Optional[Callable[["LoginResponse"], Awaitable[None]]]]] ], ] +GET_USERNAME_FOR_REGISTRATION_CALLBACK = Callable[ + [JsonDict, JsonDict], + Awaitable[Optional[str]], +] class PasswordAuthProvider: @@ -1978,6 +1982,9 @@ def __init__(self) -> None: # lists of callbacks self.check_3pid_auth_callbacks: List[CHECK_3PID_AUTH_CALLBACK] = [] self.on_logged_out_callbacks: List[ON_LOGGED_OUT_CALLBACK] = [] + self.get_username_for_registration_callbacks: List[ + GET_USERNAME_FOR_REGISTRATION_CALLBACK + ] = [] # Mapping from login type to login parameters self._supported_login_types: Dict[str, Iterable[str]] = {} @@ -1992,6 +1999,9 @@ def register_password_auth_provider_callbacks( auth_checkers: Optional[ Dict[Tuple[str, Tuple[str, ...]], CHECK_AUTH_CALLBACK] ] = None, + get_username_for_registration: Optional[ + GET_USERNAME_FOR_REGISTRATION_CALLBACK + ] = None, ) -> None: # Register check_3pid_auth callback if check_3pid_auth is not None: @@ -2036,6 +2046,11 @@ def register_password_auth_provider_callbacks( # Add the new method to the list of auth_checker_callbacks for this login type self.auth_checker_callbacks.setdefault(login_type, []).append(callback) + if get_username_for_registration is not None: + self.get_username_for_registration_callbacks.append( + get_username_for_registration, + ) + def get_supported_login_types(self) -> Mapping[str, Iterable[str]]: """Get the login types supported by this password provider @@ -2191,3 +2206,46 @@ async def on_logged_out( except Exception as e: logger.warning("Failed to run module API callback %s: %s", callback, e) continue + + async def get_username_for_registration( + self, + uia_results: JsonDict, + params: JsonDict, + ) -> Optional[str]: + """Defines the username to use when registering the user, using the credentials + and parameters provided during the UIA flow. + + Stops at the first callback that returns a string. + + Args: + uia_results: The credentials provided during the UIA flow. + params: The parameters provided by the registration request. + + Returns: + The localpart to use when registering this user, or None if no module + returned a localpart. + """ + for callback in self.get_username_for_registration_callbacks: + try: + res = await callback(uia_results, params) + + if isinstance(res, str): + return res + elif res is not None: + # mypy complains that this line is unreachable because it assumes the + # data returned by the module fits the expected type. We just want + # to make sure this is the case. + logger.warning( # type: ignore[unreachable] + "Ignoring non-string value returned by" + " get_username_for_registration callback %s: %s", + callback, + res, + ) + except Exception as e: + logger.error( + "Module raised an exception in get_username_for_registration: %s", + e, + ) + raise SynapseError(code=500, msg="Internal Server Error") + + return None diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 96d7a8f2a..a9e577547 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -70,6 +70,7 @@ from synapse.handlers.auth import ( CHECK_3PID_AUTH_CALLBACK, CHECK_AUTH_CALLBACK, + GET_USERNAME_FOR_REGISTRATION_CALLBACK, ON_LOGGED_OUT_CALLBACK, AuthHandler, ) @@ -163,6 +164,7 @@ def __init__(self, hs: "HomeServer", auth_handler: AuthHandler) -> None: self._presence_stream = hs.get_event_sources().sources.presence self._state = hs.get_state_handler() self._clock: Clock = hs.get_clock() + self._registration_handler = hs.get_registration_handler() self._send_email_handler = hs.get_send_email_handler() self.custom_template_dir = hs.config.server.custom_template_directory @@ -296,6 +298,9 @@ def register_password_auth_provider_callbacks( auth_checkers: Optional[ Dict[Tuple[str, Tuple[str, ...]], CHECK_AUTH_CALLBACK] ] = None, + get_username_for_registration: Optional[ + GET_USERNAME_FOR_REGISTRATION_CALLBACK + ] = None, ) -> None: """Registers callbacks for password auth provider capabilities. @@ -305,6 +310,7 @@ def register_password_auth_provider_callbacks( check_3pid_auth=check_3pid_auth, on_logged_out=on_logged_out, auth_checkers=auth_checkers, + get_username_for_registration=get_username_for_registration, ) def register_web_resource(self, path: str, resource: Resource): @@ -1124,6 +1130,22 @@ async def get_room_state( return {key: state_events[event_id] for key, event_id in state_ids.items()} + async def check_username(self, username: str) -> None: + """Checks if the provided username uses the grammar defined in the Matrix + specification, and is already being used by an existing user. + + Added in Synapse v1.52.0. + + Args: + username: The username to check. This is the local part of the user's full + Matrix user ID, i.e. it's "alice" if the full user ID is "@alice:foo.com". + + Raises: + SynapseError with the errcode "M_USER_IN_USE" if the username is already in + use. + """ + await self._registration_handler.check_username(username) + class PublicRoomListManager: """Contains methods for adding to, removing from and querying whether a room diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 06c9684a5..e99779e4f 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -429,6 +429,7 @@ def __init__(self, hs: "HomeServer"): self.ratelimiter = hs.get_registration_ratelimiter() self.password_policy_handler = hs.get_password_policy_handler() self.clock = hs.get_clock() + self.password_auth_provider = hs.get_password_auth_provider() self._registration_enabled = self.hs.config.registration.enable_registration self._msc2918_enabled = ( hs.config.registration.refreshable_access_token_lifetime is not None @@ -715,11 +716,15 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if not password_hash: raise SynapseError(400, "Missing params: password", Codes.MISSING_PARAM) - if not self.hs.config.registration.register_mxid_from_3pid: + desired_username = await ( + self.password_auth_provider.get_username_for_registration( + auth_result, + params, + ) + ) + + if desired_username is None: desired_username = params.get("username", None) - else: - # we keep the original desired_username derived from the 3pid above - pass guest_access_token = params.get("guest_access_token", None) diff --git a/tests/handlers/test_password_providers.py b/tests/handlers/test_password_providers.py index 08e9730d4..56506caae 100644 --- a/tests/handlers/test_password_providers.py +++ b/tests/handlers/test_password_providers.py @@ -20,10 +20,11 @@ from twisted.internet import defer import synapse +from synapse.api.constants import LoginType from synapse.handlers.auth import load_legacy_password_auth_providers from synapse.module_api import ModuleApi -from synapse.rest.client import devices, login -from synapse.types import JsonDict +from synapse.rest.client import devices, login, logout, register +from synapse.types import JsonDict, UserID from tests import unittest from tests.server import FakeChannel @@ -155,6 +156,8 @@ class PasswordAuthProviderTests(unittest.HomeserverTestCase): synapse.rest.admin.register_servlets, login.register_servlets, devices.register_servlets, + logout.register_servlets, + register.register_servlets, ] def setUp(self): @@ -719,6 +722,79 @@ def custom_auth_no_local_user_fallback_test_body(self): channel = self._send_password_login("localuser", "localpass") self.assertEqual(channel.code, 400, channel.result) + def test_username(self): + """Tests that the get_username_for_registration callback can define the username + of a user when registering. + """ + self._setup_get_username_for_registration() + + username = "rin" + channel = self.make_request( + "POST", + "/register", + { + "username": username, + "password": "bar", + "auth": {"type": LoginType.DUMMY}, + }, + ) + self.assertEqual(channel.code, 200) + + # Our callback takes the username and appends "-foo" to it, check that's what we + # have. + mxid = channel.json_body["user_id"] + self.assertEqual(UserID.from_string(mxid).localpart, username + "-foo") + + def test_username_uia(self): + """Tests that the get_username_for_registration callback is only called at the + end of the UIA flow. + """ + m = self._setup_get_username_for_registration() + + # Initiate the UIA flow. + username = "rin" + channel = self.make_request( + "POST", + "register", + {"username": username, "type": "m.login.password", "password": "bar"}, + ) + self.assertEqual(channel.code, 401) + self.assertIn("session", channel.json_body) + + # Check that the callback hasn't been called yet. + m.assert_not_called() + + # Finish the UIA flow. + session = channel.json_body["session"] + channel = self.make_request( + "POST", + "register", + {"auth": {"session": session, "type": LoginType.DUMMY}}, + ) + self.assertEqual(channel.code, 200, channel.json_body) + mxid = channel.json_body["user_id"] + self.assertEqual(UserID.from_string(mxid).localpart, username + "-foo") + + # Check that the callback has been called. + m.assert_called_once() + + def _setup_get_username_for_registration(self) -> Mock: + """Registers a get_username_for_registration callback that appends "-foo" to the + username the client is trying to register. + """ + + async def get_username_for_registration(uia_results, params): + self.assertIn(LoginType.DUMMY, uia_results) + username = params["username"] + return username + "-foo" + + m = Mock(side_effect=get_username_for_registration) + + password_auth_provider = self.hs.get_password_auth_provider() + password_auth_provider.get_username_for_registration_callbacks.append(m) + + return m + def _get_login_flows(self) -> JsonDict: channel = self.make_request("GET", "/_matrix/client/r0/login") self.assertEqual(channel.code, 200, channel.result) From 9ee7f057be1cc93b7d60a112bf9f9d3ee521d5d9 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 26 Jan 2022 14:33:26 +0000 Subject: [PATCH 3/9] Remove old register_mxid_from_3pid setting --- docs/sample_config.yaml | 7 ---- synapse/config/registration.py | 8 ---- synapse/rest/client/register.py | 67 --------------------------------- 3 files changed, 82 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 39e6cd525..195dafc26 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1318,13 +1318,6 @@ oembed: # #disable_msisdn_registration: true -# Derive the user's matrix ID from a type of 3PID used when registering. -# This overrides any matrix ID the user proposes when calling /register -# The 3PID type should be present in registrations_require_3pid to avoid -# users failing to register if they don't specify the right kind of 3pid. -# -#register_mxid_from_3pid: email - # Uncomment to set the display name of new users to their email address, # rather than using the default heuristic. # diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 431757c76..168a5a950 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -44,7 +44,6 @@ def read_config(self, config, **kwargs): "registration_requires_token", False ) self.registration_shared_secret = config.get("registration_shared_secret") - self.register_mxid_from_3pid = config.get("register_mxid_from_3pid") self.register_just_use_email_for_display_name = config.get( "register_just_use_email_for_display_name", False ) @@ -232,13 +231,6 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # #disable_msisdn_registration: true - # Derive the user's matrix ID from a type of 3PID used when registering. - # This overrides any matrix ID the user proposes when calling /register - # The 3PID type should be present in registrations_require_3pid to avoid - # users failing to register if they don't specify the right kind of 3pid. - # - #register_mxid_from_3pid: email - # Uncomment to set the display name of new users to their email address, # rather than using the default heuristic. # diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index e99779e4f..82f5c701e 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -635,73 +635,6 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: 400, "%s is already in use" % medium, Codes.THREEPID_IN_USE ) - if self.hs.config.registration.register_mxid_from_3pid: - # override the desired_username based on the 3PID if any. - # reset it first to avoid folks picking their own username. - desired_username = None - - # we should have an auth_result at this point if we're going to progress - # to register the user (i.e. we haven't picked up a registered_user_id - # from our session store), in which case get ready and gen the - # desired_username - if auth_result: - if ( - self.hs.config.registration.register_mxid_from_3pid == "email" - and LoginType.EMAIL_IDENTITY in auth_result - ): - address = auth_result[LoginType.EMAIL_IDENTITY]["address"] - desired_username = synapse.types.strip_invalid_mxid_characters( - address.replace("@", "-").lower() - ) - - # find a unique mxid for the account, suffixing numbers - # if needed - while True: - try: - await self.registration_handler.check_username( - desired_username, - guest_access_token=guest_access_token, - assigned_user_id=registered_user_id, - ) - # if we got this far we passed the check. - break - except SynapseError as e: - if e.errcode == Codes.USER_IN_USE: - m = re.match(r"^(.*?)(\d+)$", desired_username) - if m: - desired_username = m.group(1) + str( - int(m.group(2)) + 1 - ) - else: - desired_username += "1" - else: - # something else went wrong. - break - - if ( - self.hs.config.registration.register_just_use_email_for_display_name - ): - desired_display_name = address - else: - # Custom mapping between email address and display name - desired_display_name = _map_email_to_displayname(address) - elif ( - self.hs.config.registration.register_mxid_from_3pid == "msisdn" - and LoginType.MSISDN in auth_result - ): - desired_username = auth_result[LoginType.MSISDN]["address"] - else: - raise SynapseError( - 400, "Cannot derive mxid from 3pid; no recognised 3pid" - ) - - if desired_username is not None: - await self.registration_handler.check_username( - desired_username, - guest_access_token=guest_access_token, - assigned_user_id=registered_user_id, - ) - if registered_user_id is not None: logger.info( "Already registered user ID %r for this session", registered_user_id From aed1351e1d13e56793523828b2a57ecff3b66797 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 26 Jan 2022 14:53:04 +0000 Subject: [PATCH 4/9] Mainline some more --- synapse/logging/context.py | 1 - synapse/rest/client/register.py | 58 ++++++++++++++++++--------------- synapse/util/caches/lrucache.py | 1 - 3 files changed, 32 insertions(+), 28 deletions(-) diff --git a/synapse/logging/context.py b/synapse/logging/context.py index d8ae3188b..1cc07eca0 100644 --- a/synapse/logging/context.py +++ b/synapse/logging/context.py @@ -55,7 +55,6 @@ def get_thread_resource_usage() -> "Optional[resource.struct_rusage]": return resource.getrusage(RUSAGE_THREAD) - except Exception: # If the system doesn't support resource.getrusage(RUSAGE_THREAD) then we # won't track resource usage. diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 82f5c701e..4afe36036 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -14,7 +14,6 @@ # limitations under the License. import logging import random -import re from typing import TYPE_CHECKING, List, Optional, Tuple from twisted.web.server import Request @@ -469,22 +468,13 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: else: should_issue_refresh_token = False - # We don't care about usernames for this deployment. In fact, the act - # of checking whether they exist already can leak metadata about - # which users are already registered. - # - # Usernames are already derived via the provided email. - # So, if they're not necessary, just ignore them. - # - # (we do still allow appservices to set them below) + # Pull out the provided username and do basic sanity checks early since + # the auth layer will store these in sessions. desired_username = None - - desired_display_name = body.get("display_name") - - # We need to retrieve the password early in order to pass it to - # application service registration - # This is specific to shadow server registration of users via an AS - password = body.pop("password", None) + if "username" in body: + if not isinstance(body["username"], str) or len(body["username"]) > 512: + raise SynapseError(400, "Invalid username") + desired_username = body["username"] # fork off as soon as possible for ASes which have completely # different registration flows to normal users @@ -503,7 +493,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # Set the desired user according to the AS API (which uses the # 'user' key not 'username'). Since this is a new addition, we'll # fallback to 'username' if they gave one. - desired_username = body.get("user", body.get("username")) + desired_username = body.get("user", desired_username) # XXX we should check that desired_username is valid. Currently # we give appservices carte blanche for any insanity in mxids, @@ -533,6 +523,16 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if not self._registration_enabled: raise SynapseError(403, "Registration has been disabled", Codes.FORBIDDEN) + # For regular registration, convert the provided username to lowercase + # before attempting to register it. This should mean that people who try + # to register with upper-case in their usernames don't get a nasty surprise. + # + # Note that we treat usernames case-insensitively in login, so they are + # free to carry on imagining that their username is CrAzYh4cKeR if that + # keeps them happy. + if desired_username is not None: + desired_username = desired_username.lower() + # Check if this account is upgrading from a guest account. guest_access_token = body.get("guest_access_token", None) @@ -541,6 +541,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # Note that we remove the password from the body since the auth layer # will store the body in the session and we don't want a plaintext # password store there. + password = body.pop("password", None) if password is not None: if not isinstance(password, str) or len(password) > 512: raise SynapseError(400, "Invalid password") @@ -626,15 +627,6 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: Codes.THREEPID_DENIED, ) - existingUid = await self.store.get_user_id_by_threepid( - medium, address - ) - - if existingUid is not None: - raise SynapseError( - 400, "%s is already in use" % medium, Codes.THREEPID_IN_USE - ) - if registered_user_id is not None: logger.info( "Already registered user ID %r for this session", registered_user_id @@ -703,6 +695,20 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: session_id ) + # TODO: This won't be needed anymore once https://github.com/matrix-org/matrix-dinsic/issues/793 + # is resolved. + desired_display_name = body.get("display_name") + if auth_result: + if LoginType.EMAIL_IDENTITY in auth_result: + address = auth_result[LoginType.EMAIL_IDENTITY]["address"] + if ( + self.hs.config.registration.register_just_use_email_for_display_name + ): + desired_display_name = address + else: + # Custom mapping between email address and display name + desired_display_name = _map_email_to_displayname(address) + registered_user_id = await self.registration_handler.register_user( localpart=desired_username, password_hash=password_hash, diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py index a0a7a9de3..0a9972e39 100644 --- a/synapse/util/caches/lrucache.py +++ b/synapse/util/caches/lrucache.py @@ -68,7 +68,6 @@ def _get_size_of(val: Any, *, recurse: bool = True) -> int: sizer.exclude_refs((), None, "") return sizer.asizeof(val, limit=100 if recurse else 0) - except ImportError: def _get_size_of(val: Any, *, recurse: bool = True) -> int: From d971386b8f6631bfbd28e46ef2fb0b786dbf62fd Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 26 Jan 2022 15:21:04 +0000 Subject: [PATCH 5/9] Update black's version to match mainline --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 0ce8beb00..4f8f8e4d7 100755 --- a/setup.py +++ b/setup.py @@ -96,7 +96,7 @@ def exec_file(path_segments): # We pin black so that our tests don't start failing on new releases. CONDITIONAL_REQUIREMENTS["lint"] = [ "isort==5.7.0", - "black==21.6b0", + "black==21.12b0", "flake8-comprehensions", "flake8-bugbear==21.3.2", "flake8", From be281d310ab27904a4527a25c7244b2a3a63daad Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 10 Jan 2022 13:40:46 +0000 Subject: [PATCH 6/9] Deal with mypy errors w/ type-hinted pynacl 1.5.0 (#11714) * Deal with mypy errors w/ type-hinted pynacl 1.5.0 Fixes #11644. I really don't like that we're monkey patching pynacl SignedKey instances with alg and version objects. But I'm too scared to make the changes necessary right now. (Ideally I would replace `signedjson.types.SingingKey` with a runtime class which wraps or inherits from `nacl.signing.SigningKey`.) C.f. https://github.com/matrix-org/python-signedjson/issues/16 --- changelog.d/11714.misc | 1 + tests/crypto/test_event_signing.py | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11714.misc diff --git a/changelog.d/11714.misc b/changelog.d/11714.misc new file mode 100644 index 000000000..7f39bf0e3 --- /dev/null +++ b/changelog.d/11714.misc @@ -0,0 +1 @@ +Fix a typechecker problem related to our (ab)use of `nacl.signing.SigningKey`s. \ No newline at end of file diff --git a/tests/crypto/test_event_signing.py b/tests/crypto/test_event_signing.py index 1c920157f..a72a0103d 100644 --- a/tests/crypto/test_event_signing.py +++ b/tests/crypto/test_event_signing.py @@ -14,6 +14,7 @@ import nacl.signing +import signedjson.types from unpaddedbase64 import decode_base64 from synapse.api.room_versions import RoomVersions @@ -35,7 +36,12 @@ class EventSigningTestCase(unittest.TestCase): def setUp(self): - self.signing_key = nacl.signing.SigningKey(SIGNING_KEY_SEED) + # NB: `signedjson` expects `nacl.signing.SigningKey` instances which have been + # monkeypatched to include new `alg` and `version` attributes. This is captured + # by the `signedjson.types.SigningKey` protocol. + self.signing_key: signedjson.types.SigningKey = nacl.signing.SigningKey( + SIGNING_KEY_SEED + ) self.signing_key.alg = KEY_ALG self.signing_key.version = KEY_VER From e738672dac351b18273dea9f0b80af2db09da40e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 26 Jan 2022 12:06:56 +0000 Subject: [PATCH 7/9] Avoid type annotation problems in prom-client (#11834) --- changelog.d/11834.misc | 1 + synapse/python_dependencies.py | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11834.misc diff --git a/changelog.d/11834.misc b/changelog.d/11834.misc new file mode 100644 index 000000000..29a5635f7 --- /dev/null +++ b/changelog.d/11834.misc @@ -0,0 +1 @@ +Workaround a type annotation problem in `prometheus_client` 0.13.0. \ No newline at end of file diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index 154e5b702..94c26bd78 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -75,7 +75,8 @@ "msgpack>=0.5.2", "phonenumbers>=8.2.0", # we use GaugeHistogramMetric, which was added in prom-client 0.4.0. - "prometheus_client>=0.4.0", + # 0.13.0 has an incorrect type annotation, see #11832. + "prometheus_client>=0.4.0,<0.13.0", # we use `order`, which arrived in attrs 19.2.0. # Note: 21.1.0 broke `/sync`, see #9936 "attrs>=19.2.0,!=21.1.0", From 78842dc829008eb3c15fe363542b74988e1b7438 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 25 Jan 2022 06:07:10 -0500 Subject: [PATCH 8/9] Ignore the jsonschema type. (#11817) --- changelog.d/11817.misc | 1 + synapse/events/validator.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11817.misc diff --git a/changelog.d/11817.misc b/changelog.d/11817.misc new file mode 100644 index 000000000..bd29d8d6e --- /dev/null +++ b/changelog.d/11817.misc @@ -0,0 +1 @@ +Compatibility with updated type hints for jsonschema 4.4.0. diff --git a/synapse/events/validator.py b/synapse/events/validator.py index cf8693496..424557301 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -246,7 +246,9 @@ def _ensure_state_event(self, event: Union[EventBase, EventBuilder]) -> None: # This could return something newer than Draft 7, but that's the current "latest" # validator. -def _create_power_level_validator() -> jsonschema.Draft7Validator: +# +# See https://github.com/python/typeshed/issues/7028 for the ignored return type. +def _create_power_level_validator() -> jsonschema.Draft7Validator: # type: ignore[valid-type] validator = jsonschema.validators.validator_for(POWER_LEVELS_SCHEMA) # by default jsonschema does not consider a frozendict to be an object so From 1e2438bd6614a854a8a270657d1e724bdd5349db Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 25 Jan 2022 20:29:28 +0000 Subject: [PATCH 9/9] Fix another jsonschema typecheck error (#11830) Similar to #11817. In `_create_power_level_validator` we - retrieve `validator`. This is a class implementing the `jsonschema.protocols.Validator` interface. In other words, `validator: Type[jsonschema.protocols.Validator]`. - we then create an second validator class by modifying the original `validator`. We return that class, which is also of type `Type[jsonschema.protocols.Validator]`. So the original annotation was incorrect: it claimed we were returning an instance of jsonSchema.Draft7Validator, not the class (or a subclass) itself. (Strictly speaking this is incorrect, because `POWER_LEVELS_SCHEMA` isn't pinned to a particular version of JSON Schema. But there are other complications with the type stubs if you try to fix this; I felt like the change herein was a decent compromise that better expresses intent). (I suspect/hope the typeshed project would welcome an effort to improve the jsonschema stubs. Let's see if I get some spare time.) --- changelog.d/11817.misc | 2 +- changelog.d/11830.misc | 1 + synapse/events/validator.py | 6 ++---- 3 files changed, 4 insertions(+), 5 deletions(-) create mode 100644 changelog.d/11830.misc diff --git a/changelog.d/11817.misc b/changelog.d/11817.misc index bd29d8d6e..3d6b2ea4d 100644 --- a/changelog.d/11817.misc +++ b/changelog.d/11817.misc @@ -1 +1 @@ -Compatibility with updated type hints for jsonschema 4.4.0. +Correct a type annotation in the event validation logic. diff --git a/changelog.d/11830.misc b/changelog.d/11830.misc new file mode 100644 index 000000000..fe248d00a --- /dev/null +++ b/changelog.d/11830.misc @@ -0,0 +1 @@ +Correct a type annotation in the event validation logic. \ No newline at end of file diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 424557301..360d24274 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import collections.abc -from typing import Iterable, Union +from typing import Iterable, Type, Union import jsonschema @@ -246,9 +246,7 @@ def _ensure_state_event(self, event: Union[EventBase, EventBuilder]) -> None: # This could return something newer than Draft 7, but that's the current "latest" # validator. -# -# See https://github.com/python/typeshed/issues/7028 for the ignored return type. -def _create_power_level_validator() -> jsonschema.Draft7Validator: # type: ignore[valid-type] +def _create_power_level_validator() -> Type[jsonschema.Draft7Validator]: validator = jsonschema.validators.validator_for(POWER_LEVELS_SCHEMA) # by default jsonschema does not consider a frozendict to be an object so