From abdcd9fe02359295c6cdc789e8d17de90731dbdb Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 13 Jan 2022 16:55:53 +0000 Subject: [PATCH 1/9] Add a config flag to ignore client-supplied usernames at registration 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). If this config flag is turned on, Synapse ignores any username supplied by the client on both endpoints, and instead generates a username automatically for the user. In the future, there will be a module callback that allows modules to decide which username to give the user (by e.g. deriving it from an email address), so usernames generated with this flag turned on won't be limited to integers. --- changelog.d/11742.feature | 1 + synapse/config/registration.py | 9 +++++++ synapse/rest/client/register.py | 13 +++++++-- tests/rest/client/test_register.py | 43 ++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 changelog.d/11742.feature diff --git a/changelog.d/11742.feature b/changelog.d/11742.feature new file mode 100644 index 000000000000..4c191cb42916 --- /dev/null +++ b/changelog.d/11742.feature @@ -0,0 +1 @@ +Add a configuration flag to ignore client-supplied usernames upon registration. diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 7a059c6decc0..57024cc6e8c8 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -190,6 +190,8 @@ def read_config(self, config, **kwargs): # The success template used during fallback auth. self.fallback_success_template = self.read_template("auth_success.html") + self.ignore_client_username = config.get("ignore_client_username", False) + def generate_config_section(self, generate_secrets=False, **kwargs): if generate_secrets: registration_shared_secret = 'registration_shared_secret: "%s"' % ( @@ -446,6 +448,13 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # Defaults to true. # #auto_join_rooms_for_guests: false + + # Whether to ignore the username supplied by the client in the registration + # request and instead automatically generate one. + # + # Defaults to false. + # + #ignore_client_username: true """ % locals() ) diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 8b56c76aed66..60b3c02767bd 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -339,12 +339,21 @@ def __init__(self, hs: "HomeServer"): ), ) + self.client_username_ignored = hs.config.registration.ignore_client_username + 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 ) + # If we're ignoring client-supplied usernames when registering new users, we want + # to just tell the client any username is available, so it understands the + # registration will succeed if they provide this username. This is also in place + # to avoid leaking existing usernames on the server. + if self.client_username_ignored: + return 200, {"available": True} + ip = request.getClientIP() with self.ratelimiter.ratelimit(ip) as wait_deferred: await wait_deferred @@ -422,6 +431,7 @@ def __init__(self, hs: "HomeServer"): self._refresh_tokens_enabled = ( hs.config.registration.refreshable_access_token_lifetime is not None ) + self._ignore_client_username = hs.config.registration.ignore_client_username self._registration_flows = _calculate_registration_flows( hs.config, self.auth_handler @@ -458,7 +468,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # Pull out the provided username and do basic sanity checks early since # the auth layer will store these in sessions. desired_username = None - if "username" in body: + if not self._ignore_client_username and "username" in body: if not isinstance(body["username"], str) or len(body["username"]) > 512: raise SynapseError(400, "Invalid username") desired_username = body["username"] @@ -627,7 +637,6 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if not password_hash: raise SynapseError(400, "Missing params: password", Codes.MISSING_PARAM) - desired_username = params.get("username", None) guest_access_token = params.get("guest_access_token", None) if desired_username is not None: diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index 6e7c0f11df3e..46911a28cec1 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -25,6 +25,7 @@ from synapse.appservice import ApplicationService from synapse.rest.client import account, account_validity, login, logout, register, sync from synapse.storage._base import db_to_json +from synapse.types import UserID from tests import unittest from tests.unittest import override_config @@ -726,6 +727,48 @@ def test_reject_invalid_email(self): {"errcode": "M_UNKNOWN", "error": "Unable to parse email address"}, ) + @override_config( + { + "ignore_client_username": True, + } + ) + def test_ignore_client_username(self): + """Tests that the 'ignore_client_username' 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) + + # Try to register a user with the same username. + request_data = json.dumps( + { + "username": username, + "password": "foo", + "auth": {"type": LoginType.DUMMY}, + }, + ) + channel = self.make_request("POST", self.url, request_data) + + # Check that the registration succeeded (since we ignored the username parameter) + # and that there's a user_id parameter in the response body. + self.assertEquals(200, channel.code, channel.result) + self.assertIn("user_id", channel.json_body) + + # Check that the user_id parameter from the response body differs from the + # username provided in the request, proving that the request parameter got + # successfully ignored. + user_id = UserID.from_string(channel.json_body["user_id"]) + self.assertNotEquals(user_id.localpart, username) + class AccountValidityTestCase(unittest.HomeserverTestCase): From 52369472157d28ec21ab045d11a527a9b2809459 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 13 Jan 2022 17:02:51 +0000 Subject: [PATCH 2/9] Fix changelog number --- changelog.d/{11742.feature => 11743.feature} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{11742.feature => 11743.feature} (100%) diff --git a/changelog.d/11742.feature b/changelog.d/11743.feature similarity index 100% rename from changelog.d/11742.feature rename to changelog.d/11743.feature From d7f4db11a7fab3de9f8baf216a57672832593e11 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 14 Jan 2022 10:49:34 +0000 Subject: [PATCH 3/9] Sample config --- docs/sample_config.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 26894fae349a..8d16426d5ac2 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1436,6 +1436,13 @@ account_threepid_delegates: # #auto_join_rooms_for_guests: false +# Whether to ignore the username supplied by the client in the registration +# request and instead automatically generate one. +# +# Defaults to false. +# +#ignore_client_username: true + ## Metrics ### From 4c77107b57ef5b7863b95730b255d7b083407d3a Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 14 Jan 2022 11:42:15 +0000 Subject: [PATCH 4/9] Correctly ignore the username when pulling it from a UIA session --- synapse/rest/client/register.py | 6 ++++++ tests/rest/client/test_register.py | 24 ++++++++++++++++-------- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 60b3c02767bd..e6165f0c1d23 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -637,6 +637,12 @@ 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._ignore_client_username: + # Usernames will be stored in UIA sessions regardless of this setting, so + # if it's on we just don't try to retrieve the username from the session + # data. + desired_username = params.get("username", None) + guest_access_token = params.get("guest_access_token", None) if desired_username is not None: diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index 46911a28cec1..0215967676df 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -748,15 +748,23 @@ def test_ignore_client_username(self): channel = self.make_request("GET", "register/available?username=" + username) self.assertEquals(200, channel.code, channel.result) - # Try to register a user with the same username. - request_data = json.dumps( - { - "username": username, - "password": "foo", - "auth": {"type": LoginType.DUMMY}, - }, + # Check that the registration succeeded (since we ignored the username parameter) + # and that there's a user_id parameter in the response body. + # Do it in a UIA flow to make sure the username won't be picked back up from the + # UIA session data. + channel = self.make_request( + "POST", + "register", + {"username": username, "type": "m.login.password", "password": "foo"}, + ) + self.assertEqual(channel.code, 401) + + session = channel.json_body["session"] + channel = self.make_request( + "POST", + "register", + {"auth": {"session": session, "type": LoginType.DUMMY}}, ) - channel = self.make_request("POST", self.url, request_data) # Check that the registration succeeded (since we ignored the username parameter) # and that there's a user_id parameter in the response body. From f4a69b804bc0b78077406985f9f6707d8f11a7b1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 14 Jan 2022 11:43:12 +0000 Subject: [PATCH 5/9] Make test failure less noisy --- tests/rest/client/test_register.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index 0215967676df..38a6fb1b1ccd 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -768,7 +768,7 @@ def test_ignore_client_username(self): # Check that the registration succeeded (since we ignored the username parameter) # and that there's a user_id parameter in the response body. - self.assertEquals(200, channel.code, channel.result) + self.assertEquals(200, channel.code, channel.json_body) self.assertIn("user_id", channel.json_body) # Check that the user_id parameter from the response body differs from the From ffd629de2ccfed0709fc00ea002b17a6d85c3bb1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 Jan 2022 16:14:45 +0000 Subject: [PATCH 6/9] Incorporate comments from discussion --- synapse/config/registration.py | 11 +++++++---- synapse/handlers/register.py | 26 ++++++++++++++------------ synapse/rest/client/register.py | 20 ++++++-------------- tests/rest/client/test_register.py | 29 ++++++++++------------------- 4 files changed, 37 insertions(+), 49 deletions(-) diff --git a/synapse/config/registration.py b/synapse/config/registration.py index 57024cc6e8c8..ea9b50fe97e4 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -190,7 +190,7 @@ def read_config(self, config, **kwargs): # The success template used during fallback auth. self.fallback_success_template = self.read_template("auth_success.html") - self.ignore_client_username = config.get("ignore_client_username", False) + 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: @@ -449,12 +449,15 @@ def generate_config_section(self, generate_secrets=False, **kwargs): # #auto_join_rooms_for_guests: false - # Whether to ignore the username supplied by the client in the registration - # request and instead automatically generate one. + # 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. # - #ignore_client_username: true + #inhibit_user_in_use_error: true """ % locals() ) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index f08a516a7588..a719d5eef351 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,21 +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 ) - 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 e6165f0c1d23..6937c099e94e 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -339,7 +339,7 @@ def __init__(self, hs: "HomeServer"): ), ) - self.client_username_ignored = hs.config.registration.ignore_client_username + 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: @@ -347,11 +347,7 @@ async def on_GET(self, request: Request) -> Tuple[int, JsonDict]: 403, "Registration has been disabled", errcode=Codes.FORBIDDEN ) - # If we're ignoring client-supplied usernames when registering new users, we want - # to just tell the client any username is available, so it understands the - # registration will succeed if they provide this username. This is also in place - # to avoid leaking existing usernames on the server. - if self.client_username_ignored: + if self.inhibit_user_in_use_error: return 200, {"available": True} ip = request.getClientIP() @@ -431,7 +427,7 @@ def __init__(self, hs: "HomeServer"): self._refresh_tokens_enabled = ( hs.config.registration.refreshable_access_token_lifetime is not None ) - self._ignore_client_username = hs.config.registration.ignore_client_username + 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 @@ -468,7 +464,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # Pull out the provided username and do basic sanity checks early since # the auth layer will store these in sessions. desired_username = None - if not self._ignore_client_username and "username" in body: + if "username" in body: if not isinstance(body["username"], str) or len(body["username"]) > 512: raise SynapseError(400, "Invalid username") desired_username = body["username"] @@ -574,6 +570,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: 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 @@ -637,12 +634,7 @@ 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._ignore_client_username: - # Usernames will be stored in UIA sessions regardless of this setting, so - # if it's on we just don't try to retrieve the username from the session - # data. - desired_username = params.get("username", None) - + desired_username = params.get("username", None) guest_access_token = params.get("guest_access_token", None) if desired_username is not None: diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index 38a6fb1b1ccd..5e2e82723617 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -729,16 +729,16 @@ def test_reject_invalid_email(self): @override_config( { - "ignore_client_username": True, + "inhibit_user_in_use_error": True, } ) - def test_ignore_client_username(self): - """Tests that the 'ignore_client_username' configuration flag behaves + 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 + # 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)) @@ -748,34 +748,25 @@ def test_ignore_client_username(self): channel = self.make_request("GET", "register/available?username=" + username) self.assertEquals(200, channel.code, channel.result) - # Check that the registration succeeded (since we ignored the username parameter) - # and that there's a user_id parameter in the response body. - # Do it in a UIA flow to make sure the username won't be picked back up from the - # UIA session data. + # 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}}, ) - - # Check that the registration succeeded (since we ignored the username parameter) - # and that there's a user_id parameter in the response body. - self.assertEquals(200, channel.code, channel.json_body) - self.assertIn("user_id", channel.json_body) - - # Check that the user_id parameter from the response body differs from the - # username provided in the request, proving that the request parameter got - # successfully ignored. - user_id = UserID.from_string(channel.json_body["user_id"]) - self.assertNotEquals(user_id.localpart, username) + self.assertEqual(channel.code, 400, channel.json_body) + self.assertEqual(channel.json_body["errcode"], Codes.USER_IN_USE) class AccountValidityTestCase(unittest.HomeserverTestCase): From 540e0ca4572477675ca809741028231ed0131508 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 Jan 2022 16:20:01 +0000 Subject: [PATCH 7/9] Fixup changelog --- changelog.d/11743.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11743.feature b/changelog.d/11743.feature index 4c191cb42916..9809f48b9688 100644 --- a/changelog.d/11743.feature +++ b/changelog.d/11743.feature @@ -1 +1 @@ -Add a configuration flag to ignore client-supplied usernames upon registration. +Add a config flag to inhibit M_USER_IN_USE during registration. From 43af50cd0a05e61d334e72357cdb27fcef400f69 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 Jan 2022 16:23:17 +0000 Subject: [PATCH 8/9] Lint --- synapse/rest/client/register.py | 8 ++++++-- tests/rest/client/test_register.py | 1 - 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/synapse/rest/client/register.py b/synapse/rest/client/register.py index 6937c099e94e..c59dae7c0370 100644 --- a/synapse/rest/client/register.py +++ b/synapse/rest/client/register.py @@ -339,7 +339,9 @@ def __init__(self, hs: "HomeServer"): ), ) - self.inhibit_user_in_use_error = hs.config.registration.inhibit_user_in_use_error + 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: @@ -427,7 +429,9 @@ def __init__(self, hs: "HomeServer"): self._refresh_tokens_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._inhibit_user_in_use_error = ( + hs.config.registration.inhibit_user_in_use_error + ) self._registration_flows = _calculate_registration_flows( hs.config, self.auth_handler diff --git a/tests/rest/client/test_register.py b/tests/rest/client/test_register.py index 5e2e82723617..407dd32a7383 100644 --- a/tests/rest/client/test_register.py +++ b/tests/rest/client/test_register.py @@ -25,7 +25,6 @@ from synapse.appservice import ApplicationService from synapse.rest.client import account, account_validity, login, logout, register, sync from synapse.storage._base import db_to_json -from synapse.types import UserID from tests import unittest from tests.unittest import override_config From 3375b218868fc4a83ebb3d86b704cc873541b2b1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 20 Jan 2022 16:28:43 +0000 Subject: [PATCH 9/9] Config --- docs/sample_config.yaml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 8d16426d5ac2..a8a964c9efea 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1436,12 +1436,15 @@ account_threepid_delegates: # #auto_join_rooms_for_guests: false -# Whether to ignore the username supplied by the client in the registration -# request and instead automatically generate one. +# 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. # -#ignore_client_username: true +#inhibit_user_in_use_error: true ## Metrics ###