From 32e4420a6678f1cfa4a3e488cff2d219ca326950 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Thu, 3 May 2018 04:20:25 +0100 Subject: [PATCH] improve mxid & displayname selection for register_mxid_from_3pid * [x] strip invalid characters from generated mxid * [x] append numbers to disambiguate clashing mxids * [x] generate displayanames from 3pids using a dodgy heuristic * [x] get rid of the create_profile_with_localpart and instead explicitly set displaynames so they propagate correctly --- synapse/handlers/profile.py | 2 +- synapse/handlers/register.py | 45 ++++++++++++++------- synapse/rest/client/v2_alpha/register.py | 50 +++++++++++++++++++++--- synapse/storage/registration.py | 14 +------ synapse/types.py | 12 ++++++ 5 files changed, 90 insertions(+), 33 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 7c5591056d..b9898733dc 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -208,7 +208,7 @@ class ProfileHandler(BaseHandler): @defer.inlineCallbacks def set_displayname(self, target_user, requester, new_displayname, by_admin=False): """target_user is the user whose displayname is to be changed; - auth_user is the user attempting to make this change.""" + requester is the user attempting to make this change.""" if not self.hs.is_mine(target_user): raise SynapseError(400, "User is not hosted on this Home Server") diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 5da03e0a0b..9a1af53dfa 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -113,6 +113,7 @@ class RegistrationHandler(BaseHandler): generate_token=True, guest_access_token=None, make_guest=False, + display_name=None, admin=False, ): """Registers a new client on the server. @@ -128,6 +129,7 @@ class RegistrationHandler(BaseHandler): since it offers no means of associating a device_id with the access_token. Instead you should call auth_handler.issue_access_token after registration. + display_name (str): The displayname to set for this user, if any Returns: A tuple of (user_id, access_token). Raises: @@ -165,13 +167,20 @@ class RegistrationHandler(BaseHandler): password_hash=password_hash, was_guest=was_guest, make_guest=make_guest, - create_profile_with_localpart=( - # If the user was a guest then they already have a profile - None if was_guest else user.localpart - ), admin=admin, ) + if display_name is not None: + display_name = ( + # If the user was a guest then they already have a profile + None if was_guest else user.localpart + ) + + if display_name: + yield self.profile_handler.set_displayname( + user_id, user_id, display_name, by_admin=True, + ) + if self.hs.config.user_directory_search_all_users: profile = yield self.store.get_profileinfo(localpart) yield self.user_directory_handler.handle_local_profile_change( @@ -196,8 +205,12 @@ class RegistrationHandler(BaseHandler): token=token, password_hash=password_hash, make_guest=make_guest, - create_profile_with_localpart=user.localpart, ) + + yield self.profile_handler.set_displayname( + user_id, user_id, user.localpart, by_admin=True, + ) + except SynapseError: # if user id is taken, just generate another user = None @@ -241,8 +254,12 @@ class RegistrationHandler(BaseHandler): user_id=user_id, password_hash="", appservice_id=service_id, - create_profile_with_localpart=user.localpart, ) + + yield self.profile_handler.set_displayname( + user_id, user_id, user.localpart, by_admin=True, + ) + defer.returnValue(user_id) @defer.inlineCallbacks @@ -288,7 +305,10 @@ class RegistrationHandler(BaseHandler): user_id=user_id, token=token, password_hash=None, - create_profile_with_localpart=user.localpart, + ) + + yield self.profile_handler.set_displayname( + user_id, user_id, user.localpart, by_admin=True, ) except Exception as e: yield self.store.add_access_token_to_user(user_id, token) @@ -443,18 +463,15 @@ class RegistrationHandler(BaseHandler): user_id=user_id, token=token, password_hash=password_hash, - create_profile_with_localpart=user.localpart, ) + if displayname is not None: + yield self.profile_handler.set_displayname( + user_id, user_id, displayname, by_admin=True, + ) else: yield self._auth_handler.delete_access_tokens_for_user(user_id) yield self.store.add_access_token_to_user(user_id=user_id, token=token) - if displayname is not None: - logger.info("setting user display name: %s -> %s", user_id, displayname) - yield self.profile_handler.set_displayname( - user, requester, displayname, by_admin=True, - ) - defer.returnValue((user_id, token)) def auth_handler(self): diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 9171dcee06..7dc0cea076 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -17,7 +17,7 @@ from twisted.internet import defer import synapse -import synapse.types +from synapse import types from synapse.api.auth import get_access_token_from_request, has_access_token from synapse.api.constants import LoginType from synapse.api.errors import SynapseError, Codes, UnrecognizedRequestError @@ -31,6 +31,8 @@ from ._base import client_v2_patterns, interactive_auth_handler import logging import hmac +import re +from string import capwords from hashlib import sha1 from synapse.util.async import run_on_reactor from synapse.util.ratelimitutils import FederationRateLimiter @@ -222,6 +224,8 @@ class RegisterRestServlet(RestServlet): raise SynapseError(400, "Invalid username") desired_username = body['username'] + desired_display_name = None + appservice = None if has_access_token(request): appservice = yield self.auth.get_appservice_by_req(request) @@ -374,9 +378,10 @@ class RegisterRestServlet(RestServlet): # reset it first to avoid folks picking their own username. desired_username = None - # we should always have an auth_result 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 + # 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 ( ( @@ -388,7 +393,41 @@ class RegisterRestServlet(RestServlet): ) ): address = auth_result[login_type]['address'] - desired_username = address.replace('@', '-').lower() + desired_username = types.strip_invalid_mxid_characters( + address.replace('@', '-').lower() + ) + + # find a unique mxid for the account, suffixing numbers + # if needed + while True: + try: + yield 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 + + # XXX: a nasty heuristic to turn an email address into + # a displayname, as part of register_mxid_from_3pid + parts = address.replace('.', ' ').split('@') + desired_display_name = ( + capwords(parts[0]) + + " [" + capwords(parts[1].split(' ')[0]) + "]" + ) if desired_username is not None: yield self.registration_handler.check_username( @@ -431,6 +470,7 @@ class RegisterRestServlet(RestServlet): password=new_password, guest_access_token=guest_access_token, generate_token=False, + display_name=desired_display_name, ) # remember that we've now registered that user account, and with diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 6b557ca0cf..384c9977c1 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -128,7 +128,7 @@ class RegistrationStore(RegistrationWorkerStore, def register(self, user_id, token=None, password_hash=None, was_guest=False, make_guest=False, appservice_id=None, - create_profile_with_localpart=None, admin=False): + admin=False): """Attempts to register an account. Args: @@ -142,8 +142,6 @@ class RegistrationStore(RegistrationWorkerStore, make_guest (boolean): True if the the new user should be guest, false to add a regular user account. appservice_id (str): The ID of the appservice registering the user. - create_profile_with_localpart (str): Optionally create a profile for - the given localpart. Raises: StoreError if the user_id could not be registered. """ @@ -156,7 +154,6 @@ class RegistrationStore(RegistrationWorkerStore, was_guest, make_guest, appservice_id, - create_profile_with_localpart, admin ) @@ -169,7 +166,6 @@ class RegistrationStore(RegistrationWorkerStore, was_guest, make_guest, appservice_id, - create_profile_with_localpart, admin, ): now = int(self.clock.time()) @@ -234,14 +230,6 @@ class RegistrationStore(RegistrationWorkerStore, (next_id, user_id, token,) ) - if create_profile_with_localpart: - # set a default displayname serverside to avoid ugly race - # between auto-joins and clients trying to set displaynames - txn.execute( - "INSERT INTO profiles(user_id, displayname) VALUES (?,?)", - (create_profile_with_localpart, create_profile_with_localpart) - ) - self._invalidate_cache_and_stream( txn, self.get_user_by_id, (user_id,) ) diff --git a/synapse/types.py b/synapse/types.py index cc7c182a78..46be4a071d 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -229,6 +229,18 @@ def contains_invalid_mxid_characters(localpart): return any(c not in mxid_localpart_allowed_characters for c in localpart) +def strip_invalid_mxid_characters(localpart): + """Removes any invalid characters from an mxid + + Args: + localpart (basestring): the localpart to be stripped + + Returns: + localpart (basestring): the localpart having been stripped + """ + return filter(lambda c: c not in mxid_localpart_allowed_characters, localpart) + + class StreamToken( namedtuple("Token", ( "room_key",