From ddd876a90c6a5e8b660efc0eadfd64d4f929c53a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Jul 2019 13:08:50 +0100 Subject: [PATCH 1/2] Remove access-token support from RegistrationHandler.register Nothing uses this now, so we can remove the dead code, and clean up the API. Since we're changing the shape of the return value anyway, we take the opportunity to give the method a better name. --- changelog.d/5641.misc | 1 + synapse/handlers/register.py | 27 ++---------- synapse/module_api/__init__.py | 10 +---- synapse/replication/http/register.py | 6 --- synapse/rest/admin/__init__.py | 3 +- synapse/rest/client/v1/login.py | 14 ++----- synapse/rest/client/v2_alpha/register.py | 11 +++-- tests/handlers/test_register.py | 53 +++++++++++++----------- 8 files changed, 44 insertions(+), 81 deletions(-) create mode 100644 changelog.d/5641.misc diff --git a/changelog.d/5641.misc b/changelog.d/5641.misc new file mode 100644 index 000000000000..c2fbad2b5bdb --- /dev/null +++ b/changelog.d/5641.misc @@ -0,0 +1 @@ +Remove access-token support from RegistrationHandler.register. diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 853020180be5..261b09d007a2 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -138,11 +138,10 @@ def check_username(self, localpart, guest_access_token=None, assigned_user_id=No ) @defer.inlineCallbacks - def register( + def register_user( self, localpart=None, password=None, - generate_token=True, guest_access_token=None, make_guest=False, admin=False, @@ -160,11 +159,6 @@ def register( password (unicode) : The password to assign to this user so they can login again. This can be None which means they cannot login again via a password (e.g. the user is an application service user). - generate_token (bool): Whether a new access token should be - generated. Having this be True should be considered deprecated, - 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. user_type (str|None): type of user. One of the values from api.constants.UserTypes, or None for a normal user. default_display_name (unicode|None): if set, the new user's displayname @@ -172,7 +166,7 @@ def register( address (str|None): the IP address used to perform the registration. bind_emails (List[str]): list of emails to bind to this account. Returns: - A tuple of (user_id, access_token). + Deferred[str]: user_id Raises: RegistrationError if there was a problem registering. """ @@ -206,12 +200,8 @@ def register( elif default_display_name is None: default_display_name = localpart - token = None - if generate_token: - token = self.macaroon_gen.generate_access_token(user_id) yield self.register_with_store( user_id=user_id, - token=token, password_hash=password_hash, was_guest=was_guest, make_guest=make_guest, @@ -230,21 +220,17 @@ def register( else: # autogen a sequential user ID attempts = 0 - token = None user = None while not user: localpart = yield self._generate_user_id(attempts > 0) user = UserID(localpart, self.hs.hostname) user_id = user.to_string() yield self.check_user_id_not_appservice_exclusive(user_id) - if generate_token: - token = self.macaroon_gen.generate_access_token(user_id) if default_display_name is None: default_display_name = localpart try: yield self.register_with_store( user_id=user_id, - token=token, password_hash=password_hash, make_guest=make_guest, create_profile_with_displayname=default_display_name, @@ -254,7 +240,6 @@ def register( # if user id is taken, just generate another user = None user_id = None - token = None attempts += 1 if not self.hs.config.user_consent_at_registration: yield self._auto_join_rooms(user_id) @@ -272,7 +257,7 @@ def register( # Bind email to new account yield self._register_email_threepid(user_id, threepid_dict, None, False) - defer.returnValue((user_id, token)) + defer.returnValue(user_id) @defer.inlineCallbacks def _auto_join_rooms(self, user_id): @@ -534,7 +519,6 @@ def _join_user_to_room(self, requester, room_identifier): def register_with_store( self, user_id, - token=None, password_hash=None, was_guest=False, make_guest=False, @@ -548,9 +532,6 @@ def register_with_store( Args: user_id (str): The desired user ID to register. - token (str): The desired access token to use for this user. If this - is not None, the given access token is associated with the user - id. password_hash (str|None): Optional. The password hash for this user. was_guest (bool): Optional. Whether this is a guest account being upgraded to a non-guest account. @@ -586,7 +567,6 @@ def register_with_store( if self.hs.config.worker_app: return self._register_client( user_id=user_id, - token=token, password_hash=password_hash, was_guest=was_guest, make_guest=make_guest, @@ -599,7 +579,6 @@ def register_with_store( else: return self.store.register( user_id=user_id, - token=token, password_hash=password_hash, was_guest=was_guest, make_guest=make_guest, diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index a0be2c5ca30e..7bb020cb45be 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -103,7 +103,6 @@ def register(self, localpart, displayname=None, emails=[]): _, access_token = yield self.register_device(user_id) defer.returnValue((user_id, access_token)) - @defer.inlineCallbacks def register_user(self, localpart, displayname=None, emails=[]): """Registers a new user with given localpart and optional displayname, emails. @@ -115,15 +114,10 @@ def register_user(self, localpart, displayname=None, emails=[]): Returns: Deferred[str]: user_id """ - user_id, _ = yield self.hs.get_registration_handler().register( - localpart=localpart, - default_display_name=displayname, - bind_emails=emails, - generate_token=False, + return self.hs.get_registration_handler().register_user( + localpart=localpart, default_display_name=displayname, bind_emails=emails ) - defer.returnValue(user_id) - def register_device(self, user_id, device_id=None, initial_display_name=None): """Register a device for a user and generate an access token. diff --git a/synapse/replication/http/register.py b/synapse/replication/http/register.py index f81a0f1b8f54..2bf2173895ec 100644 --- a/synapse/replication/http/register.py +++ b/synapse/replication/http/register.py @@ -38,7 +38,6 @@ def __init__(self, hs): @staticmethod def _serialize_payload( user_id, - token, password_hash, was_guest, make_guest, @@ -51,9 +50,6 @@ def _serialize_payload( """ Args: user_id (str): The desired user ID to register. - token (str): The desired access token to use for this user. If this - is not None, the given access token is associated with the user - id. password_hash (str|None): Optional. The password hash for this user. was_guest (bool): Optional. Whether this is a guest account being upgraded to a non-guest account. @@ -68,7 +64,6 @@ def _serialize_payload( address (str|None): the IP address used to perform the regitration. """ return { - "token": token, "password_hash": password_hash, "was_guest": was_guest, "make_guest": make_guest, @@ -85,7 +80,6 @@ def _handle_request(self, request, user_id): yield self.registration_handler.register_with_store( user_id=user_id, - token=content["token"], password_hash=content["password_hash"], was_guest=content["was_guest"], make_guest=content["make_guest"], diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index 9843a902c69d..6888ae559002 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -219,11 +219,10 @@ def on_POST(self, request): register = RegisterRestServlet(self.hs) - (user_id, _) = yield register.registration_handler.register( + user_id = yield register.registration_handler.register_user( localpart=body["username"].lower(), password=body["password"], admin=bool(admin), - generate_token=False, user_type=user_type, ) diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index b13043cc6408..0d05945f0aa6 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -314,10 +314,8 @@ def do_jwt_login(self, login_submission): registered_user_id = yield self.auth_handler.check_user_exists(user_id) if not registered_user_id: - registered_user_id, _ = ( - yield self.registration_handler.register( - localpart=user, generate_token=False - ) + registered_user_id = yield self.registration_handler.register_user( + localpart=user ) result = yield self._register_device_with_callback( @@ -505,12 +503,8 @@ def on_successful_auth( user_id = UserID(localpart, self._hostname).to_string() registered_user_id = yield self._auth_handler.check_user_exists(user_id) if not registered_user_id: - registered_user_id, _ = ( - yield self._registration_handler.register( - localpart=localpart, - generate_token=False, - default_display_name=user_display_name, - ) + registered_user_id = yield self._registration_handler.register_user( + localpart=localpart, default_display_name=user_display_name ) login_token = self._macaroon_gen.generate_short_term_login_token( diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 5c120e4dd5d8..f327999e5990 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -464,11 +464,10 @@ def on_POST(self, request): Codes.THREEPID_IN_USE, ) - (registered_user_id, _) = yield self.registration_handler.register( + registered_user_id = yield self.registration_handler.register_user( localpart=desired_username, password=new_password, guest_access_token=guest_access_token, - generate_token=False, threepid=threepid, address=client_addr, ) @@ -542,8 +541,8 @@ def _do_shared_secret_registration(self, username, password, body): if not compare_digest(want_mac, got_mac): raise SynapseError(403, "HMAC incorrect") - (user_id, _) = yield self.registration_handler.register( - localpart=username, password=password, generate_token=False + user_id = yield self.registration_handler.register_user( + localpart=username, password=password ) result = yield self._create_registration_details(user_id, body) @@ -577,8 +576,8 @@ def _create_registration_details(self, user_id, params): def _do_guest_registration(self, params, address=None): if not self.hs.config.allow_guest_access: raise SynapseError(403, "Guest access is disabled") - user_id, _ = yield self.registration_handler.register( - generate_token=False, make_guest=True, address=address + user_id = yield self.registration_handler.register_user( + make_guest=True, address=address ) # we don't allow guests to specify their own device_id, because diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 1c7ded73973f..8197f26d4f4d 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -129,21 +129,21 @@ def test_register_mau_blocked(self): return_value=defer.succeed(self.lots_of_users) ) self.get_failure( - self.handler.register(localpart="local_part"), ResourceLimitError + self.handler.register_user(localpart="local_part"), ResourceLimitError ) self.store.get_monthly_active_count = Mock( return_value=defer.succeed(self.hs.config.max_mau_value) ) self.get_failure( - self.handler.register(localpart="local_part"), ResourceLimitError + self.handler.register_user(localpart="local_part"), ResourceLimitError ) def test_auto_create_auto_join_rooms(self): room_alias_str = "#room:test" self.hs.config.auto_join_rooms = [room_alias_str] - res = self.get_success(self.handler.register(localpart="jeff")) - rooms = self.get_success(self.store.get_rooms_for_user(res[0])) + user_id = self.get_success(self.handler.register_user(localpart="jeff")) + rooms = self.get_success(self.store.get_rooms_for_user(user_id)) directory_handler = self.hs.get_handlers().directory_handler room_alias = RoomAlias.from_string(room_alias_str) room_id = self.get_success(directory_handler.get_association(room_alias)) @@ -154,25 +154,25 @@ def test_auto_create_auto_join_rooms(self): def test_auto_create_auto_join_rooms_with_no_rooms(self): self.hs.config.auto_join_rooms = [] frank = UserID.from_string("@frank:test") - res = self.get_success(self.handler.register(frank.localpart)) - self.assertEqual(res[0], frank.to_string()) - rooms = self.get_success(self.store.get_rooms_for_user(res[0])) + user_id = self.get_success(self.handler.register_user(frank.localpart)) + self.assertEqual(user_id, frank.to_string()) + rooms = self.get_success(self.store.get_rooms_for_user(user_id)) self.assertEqual(len(rooms), 0) def test_auto_create_auto_join_where_room_is_another_domain(self): self.hs.config.auto_join_rooms = ["#room:another"] frank = UserID.from_string("@frank:test") - res = self.get_success(self.handler.register(frank.localpart)) - self.assertEqual(res[0], frank.to_string()) - rooms = self.get_success(self.store.get_rooms_for_user(res[0])) + user_id = self.get_success(self.handler.register_user(frank.localpart)) + self.assertEqual(user_id, frank.to_string()) + rooms = self.get_success(self.store.get_rooms_for_user(user_id)) self.assertEqual(len(rooms), 0) def test_auto_create_auto_join_where_auto_create_is_false(self): self.hs.config.autocreate_auto_join_rooms = False room_alias_str = "#room:test" self.hs.config.auto_join_rooms = [room_alias_str] - res = self.get_success(self.handler.register(localpart="jeff")) - rooms = self.get_success(self.store.get_rooms_for_user(res[0])) + user_id = self.get_success(self.handler.register_user(localpart="jeff")) + rooms = self.get_success(self.store.get_rooms_for_user(user_id)) self.assertEqual(len(rooms), 0) def test_auto_create_auto_join_rooms_when_support_user_exists(self): @@ -180,8 +180,8 @@ def test_auto_create_auto_join_rooms_when_support_user_exists(self): self.hs.config.auto_join_rooms = [room_alias_str] self.store.is_support_user = Mock(return_value=True) - res = self.get_success(self.handler.register(localpart="support")) - rooms = self.get_success(self.store.get_rooms_for_user(res[0])) + user_id = self.get_success(self.handler.register_user(localpart="support")) + rooms = self.get_success(self.store.get_rooms_for_user(user_id)) self.assertEqual(len(rooms), 0) directory_handler = self.hs.get_handlers().directory_handler room_alias = RoomAlias.from_string(room_alias_str) @@ -209,27 +209,31 @@ def test_auto_create_auto_join_where_no_consent(self): # When:- # * the user is registered and post consent actions are called - res = self.get_success(self.handler.register(localpart="jeff")) - self.get_success(self.handler.post_consent_actions(res[0])) + user_id = self.get_success(self.handler.register_user(localpart="jeff")) + self.get_success(self.handler.post_consent_actions(user_id)) # Then:- # * Ensure that they have not been joined to the room - rooms = self.get_success(self.store.get_rooms_for_user(res[0])) + rooms = self.get_success(self.store.get_rooms_for_user(user_id)) self.assertEqual(len(rooms), 0) def test_register_support_user(self): - res = self.get_success( - self.handler.register(localpart="user", user_type=UserTypes.SUPPORT) + user_id = self.get_success( + self.handler.register_user(localpart="user", user_type=UserTypes.SUPPORT) ) - self.assertTrue(self.store.is_support_user(res[0])) + d = self.store.is_support_user(user_id) + self.assertTrue(self.get_success(d)) def test_register_not_support_user(self): - res = self.get_success(self.handler.register(localpart="user")) - self.assertFalse(self.store.is_support_user(res[0])) + user_id = self.get_success(self.handler.register_user(localpart="user")) + d = self.store.is_support_user(user_id) + self.assertFalse(self.get_success(d)) def test_invalid_user_id_length(self): invalid_user_id = "x" * 256 - self.get_failure(self.handler.register(localpart=invalid_user_id), SynapseError) + self.get_failure( + self.handler.register_user(localpart=invalid_user_id), SynapseError + ) @defer.inlineCallbacks def get_or_create_user(self, requester, localpart, displayname, password_hash=None): @@ -267,13 +271,12 @@ def get_or_create_user(self, requester, localpart, displayname, password_hash=No if need_register: yield self.handler.register_with_store( user_id=user_id, - token=token, password_hash=password_hash, create_profile_with_displayname=user.localpart, ) else: yield self.hs.get_auth_handler().delete_access_tokens_for_user(user_id) - yield self.store.add_access_token_to_user(user_id=user_id, token=token) + 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) From b2429b193c6deb09b368fdea43ea6bd44cadd353 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 8 Jul 2019 19:00:35 +0100 Subject: [PATCH 2/2] update changelog --- changelog.d/5641.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/5641.misc b/changelog.d/5641.misc index c2fbad2b5bdb..1899bc963d4e 100644 --- a/changelog.d/5641.misc +++ b/changelog.d/5641.misc @@ -1 +1 @@ -Remove access-token support from RegistrationHandler.register. +Remove access-token support from RegistrationHandler.register, and rename it.