From c54f5f4c00c150627a820091a864a9c2a673b5ab Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 26 Jan 2022 12:02:54 +0000 Subject: [PATCH] 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):