From 31d15c6ab79e18cf243ff49854381ed2e2df83d4 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 1 Jul 2019 19:27:52 +0100 Subject: [PATCH 01/14] Move get_or_create_user to test code This is only used in tests, so... --- synapse/handlers/register.py | 51 ------------------------- tests/handlers/test_register.py | 68 ++++++++++++++++++++++++++++----- 2 files changed, 59 insertions(+), 60 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index e487b90c0821..0194140e4b41 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -505,57 +505,6 @@ def _submit_captcha(self, ip_addr, private_key, challenge, response): ) defer.returnValue(data) - @defer.inlineCallbacks - def get_or_create_user(self, requester, localpart, displayname, password_hash=None): - """Creates a new user if the user does not exist, - else revokes all previous access tokens and generates a new one. - - Args: - localpart : The local part of the user ID to register. If None, - one will be randomly generated. - Returns: - A tuple of (user_id, access_token). - Raises: - RegistrationError if there was a problem registering. - - NB this is only used in tests. TODO: move it to the test package! - """ - if localpart is None: - raise SynapseError(400, "Request must include user id") - yield self.auth.check_auth_blocking() - need_register = True - - try: - yield self.check_username(localpart) - except SynapseError as e: - if e.errcode == Codes.USER_IN_USE: - need_register = False - else: - raise - - user = UserID(localpart, self.hs.hostname) - user_id = user.to_string() - token = self.macaroon_gen.generate_access_token(user_id) - - if need_register: - yield self.register_with_store( - user_id=user_id, - token=token, - password_hash=password_hash, - create_profile_with_displayname=user.localpart, - ) - 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)) - @defer.inlineCallbacks def get_or_register_3pid_guest(self, medium, address, inviter_user_id): """Get a guest access token for a 3PID, creating a guest account if diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 4edce7af435f..1c7ded73973f 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -18,7 +18,7 @@ from twisted.internet import defer from synapse.api.constants import UserTypes -from synapse.api.errors import ResourceLimitError, SynapseError +from synapse.api.errors import Codes, ResourceLimitError, SynapseError from synapse.handlers.register import RegistrationHandler from synapse.types import RoomAlias, UserID, create_requester @@ -67,7 +67,7 @@ def test_user_is_created_and_logged_in_if_doesnt_exist(self): user_id = frank.to_string() requester = create_requester(user_id) result_user_id, result_token = self.get_success( - self.handler.get_or_create_user(requester, frank.localpart, "Frankie") + self.get_or_create_user(requester, frank.localpart, "Frankie") ) self.assertEquals(result_user_id, user_id) self.assertTrue(result_token is not None) @@ -87,7 +87,7 @@ def test_if_user_exists(self): user_id = frank.to_string() requester = create_requester(user_id) result_user_id, result_token = self.get_success( - self.handler.get_or_create_user(requester, local_part, None) + self.get_or_create_user(requester, local_part, None) ) self.assertEquals(result_user_id, user_id) self.assertTrue(result_token is not None) @@ -95,9 +95,7 @@ def test_if_user_exists(self): def test_mau_limits_when_disabled(self): self.hs.config.limit_usage_by_mau = False # Ensure does not throw exception - self.get_success( - self.handler.get_or_create_user(self.requester, "a", "display_name") - ) + self.get_success(self.get_or_create_user(self.requester, "a", "display_name")) def test_get_or_create_user_mau_not_blocked(self): self.hs.config.limit_usage_by_mau = True @@ -105,7 +103,7 @@ def test_get_or_create_user_mau_not_blocked(self): return_value=defer.succeed(self.hs.config.max_mau_value - 1) ) # Ensure does not throw exception - self.get_success(self.handler.get_or_create_user(self.requester, "c", "User")) + self.get_success(self.get_or_create_user(self.requester, "c", "User")) def test_get_or_create_user_mau_blocked(self): self.hs.config.limit_usage_by_mau = True @@ -113,7 +111,7 @@ def test_get_or_create_user_mau_blocked(self): return_value=defer.succeed(self.lots_of_users) ) self.get_failure( - self.handler.get_or_create_user(self.requester, "b", "display_name"), + self.get_or_create_user(self.requester, "b", "display_name"), ResourceLimitError, ) @@ -121,7 +119,7 @@ def test_get_or_create_user_mau_blocked(self): return_value=defer.succeed(self.hs.config.max_mau_value) ) self.get_failure( - self.handler.get_or_create_user(self.requester, "b", "display_name"), + self.get_or_create_user(self.requester, "b", "display_name"), ResourceLimitError, ) @@ -232,3 +230,55 @@ def test_register_not_support_user(self): def test_invalid_user_id_length(self): invalid_user_id = "x" * 256 self.get_failure(self.handler.register(localpart=invalid_user_id), SynapseError) + + @defer.inlineCallbacks + def get_or_create_user(self, requester, localpart, displayname, password_hash=None): + """Creates a new user if the user does not exist, + else revokes all previous access tokens and generates a new one. + + XXX: this used to be in the main codebase, but was only used by this file, + so got moved here. TODO: get rid of it, probably + + Args: + localpart : The local part of the user ID to register. If None, + one will be randomly generated. + Returns: + A tuple of (user_id, access_token). + Raises: + RegistrationError if there was a problem registering. + """ + if localpart is None: + raise SynapseError(400, "Request must include user id") + yield self.hs.get_auth().check_auth_blocking() + need_register = True + + try: + yield self.handler.check_username(localpart) + except SynapseError as e: + if e.errcode == Codes.USER_IN_USE: + need_register = False + else: + raise + + user = UserID(localpart, self.hs.hostname) + user_id = user.to_string() + token = self.macaroon_generator.generate_access_token(user_id) + + 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) + + if displayname is not None: + # logger.info("setting user display name: %s -> %s", user_id, displayname) + yield self.hs.get_profile_handler().set_displayname( + user, requester, displayname, by_admin=True + ) + + defer.returnValue((user_id, token)) From 0163b453b0dbd89cba6d9acbd80979ea8a6ef76f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 1 Jul 2019 22:30:26 +0100 Subject: [PATCH 02/14] use opaque strings as access tokens --- scripts-dev/dump_macaroon.py | 2 +- synapse/handlers/auth.py | 18 +++---- synapse/handlers/register.py | 8 ++- .../schema/delta/55/access_token_expiry.sql | 0 tests/api/test_auth.py | 49 ------------------- tests/handlers/test_auth.py | 14 ++---- tests/handlers/test_register.py | 2 +- 7 files changed, 18 insertions(+), 75 deletions(-) create mode 100644 synapse/storage/schema/delta/55/access_token_expiry.sql diff --git a/scripts-dev/dump_macaroon.py b/scripts-dev/dump_macaroon.py index 22b30fa78e43..b75391ff6f39 100755 --- a/scripts-dev/dump_macaroon.py +++ b/scripts-dev/dump_macaroon.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python2 +#!/usr/bin/env python from __future__ import print_function diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index c8c1ed324656..5f8cd8ebb1c9 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -578,7 +578,9 @@ def get_access_token_for_user_id(self, user_id, device_id=None): StoreError if there was a problem storing the token. """ logger.info("Logging in user %s on device %s", user_id, device_id) - access_token = yield self.issue_access_token(user_id, device_id) + access_token = self.generate_access_token() + yield self.store.add_access_token_to_user(user_id, access_token, device_id) + yield self.auth.check_auth_blocking(user_id) # the device *should* have been registered before we got here; however, @@ -831,11 +833,9 @@ def _check_local_password(self, user_id, password): defer.returnValue(None) defer.returnValue(user_id) - @defer.inlineCallbacks - def issue_access_token(self, user_id, device_id=None): - access_token = self.macaroon_gen.generate_access_token(user_id) - yield self.store.add_access_token_to_user(user_id, access_token, device_id) - defer.returnValue(access_token) + def generate_access_token(self): + """Generates an opaque string, for use as an access token""" + return stringutils.random_string(32) @defer.inlineCallbacks def validate_short_term_login_token_and_get_user_id(self, login_token): @@ -1052,8 +1052,7 @@ class MacaroonGenerator(object): hs = attr.ib() - def generate_access_token(self, user_id, extra_caveats=None): - extra_caveats = extra_caveats or [] + def generate_guest_access_token(self, user_id): macaroon = self._generate_base_macaroon(user_id) macaroon.add_first_party_caveat("type = access") # Include a nonce, to make sure that each login gets a different @@ -1061,8 +1060,7 @@ def generate_access_token(self, user_id, extra_caveats=None): macaroon.add_first_party_caveat( "nonce = %s" % (stringutils.random_string_with_symbols(16),) ) - for caveat in extra_caveats: - macaroon.add_first_party_caveat(caveat) + macaroon.add_first_party_caveat("guest = true") return macaroon.serialize() def generate_short_term_login_token(self, user_id, duration_in_ms=(2 * 60 * 1000)): diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 0194140e4b41..5e64c4b8577c 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -208,7 +208,7 @@ def register( token = None if generate_token: - token = self.macaroon_gen.generate_access_token(user_id) + token = self._auth_handler.generate_access_token() yield self.register_with_store( user_id=user_id, token=token, @@ -238,7 +238,7 @@ def register( 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) + token = self._auth_handler.generate_access_token() if default_display_name is None: default_display_name = localpart try: @@ -668,9 +668,7 @@ def register_device(self, user_id, device_id, initial_display_name, is_guest=Fal user_id, device_id, initial_display_name ) if is_guest: - access_token = self.macaroon_gen.generate_access_token( - user_id, ["guest = true"] - ) + access_token = self.macaroon_gen.generate_guest_access_token(user_id) else: access_token = yield self._auth_handler.get_access_token_for_user_id( user_id, device_id=device_id diff --git a/synapse/storage/schema/delta/55/access_token_expiry.sql b/synapse/storage/schema/delta/55/access_token_expiry.sql new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index d4e75b5b2e72..811ce136d710 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -239,55 +239,6 @@ def test_get_guest_user_from_macaroon(self): self.assertTrue(is_guest) self.store.get_user_by_id.assert_called_with(user_id) - @defer.inlineCallbacks - def test_cannot_use_regular_token_as_guest(self): - USER_ID = "@percy:matrix.org" - self.store.add_access_token_to_user = Mock() - - token = yield self.hs.handlers.auth_handler.issue_access_token( - USER_ID, "DEVICE" - ) - self.store.add_access_token_to_user.assert_called_with(USER_ID, token, "DEVICE") - - def get_user(tok): - if token != tok: - return None - return { - "name": USER_ID, - "is_guest": False, - "token_id": 1234, - "device_id": "DEVICE", - } - - self.store.get_user_by_access_token = get_user - self.store.get_user_by_id = Mock(return_value={"is_guest": False}) - - # check the token works - request = Mock(args={}) - request.args[b"access_token"] = [token.encode("ascii")] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - requester = yield self.auth.get_user_by_req(request, allow_guest=True) - self.assertEqual(UserID.from_string(USER_ID), requester.user) - self.assertFalse(requester.is_guest) - - # add an is_guest caveat - mac = pymacaroons.Macaroon.deserialize(token) - mac.add_first_party_caveat("guest = true") - guest_tok = mac.serialize() - - # the token should *not* work now - request = Mock(args={}) - request.args[b"access_token"] = [guest_tok.encode("ascii")] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - - with self.assertRaises(AuthError) as cm: - yield self.auth.get_user_by_req(request, allow_guest=True) - - self.assertEqual(401, cm.exception.code) - self.assertEqual("Guest access token used for regular user", cm.exception.msg) - - self.store.get_user_by_id.assert_called_with(USER_ID) - @defer.inlineCallbacks def test_blocking_mau(self): self.hs.config.limit_usage_by_mau = False diff --git a/tests/handlers/test_auth.py b/tests/handlers/test_auth.py index b204a0700d25..e45d53f76afa 100644 --- a/tests/handlers/test_auth.py +++ b/tests/handlers/test_auth.py @@ -44,18 +44,10 @@ def setUp(self): self.small_number_of_users = 1 self.large_number_of_users = 100 - def test_token_is_a_macaroon(self): - token = self.macaroon_generator.generate_access_token("some_user") - # Check that we can parse the thing with pymacaroons - macaroon = pymacaroons.Macaroon.deserialize(token) - # The most basic of sanity checks - if "some_user" not in macaroon.inspect(): - self.fail("some_user was not in %s" % macaroon.inspect()) - def test_macaroon_caveats(self): self.hs.clock.now = 5000 - token = self.macaroon_generator.generate_access_token("a_user") + token = self.macaroon_generator.generate_guest_access_token("a_user") macaroon = pymacaroons.Macaroon.deserialize(token) def verify_gen(caveat): @@ -70,11 +62,15 @@ def verify_type(caveat): def verify_nonce(caveat): return caveat.startswith("nonce =") + def verify_guest(caveat): + return caveat == "guest = true" + v = pymacaroons.Verifier() v.satisfy_general(verify_gen) v.satisfy_general(verify_user) v.satisfy_general(verify_type) v.satisfy_general(verify_nonce) + v.satisfy_general(verify_guest) v.verify(macaroon, self.hs.config.macaroon_secret_key) @defer.inlineCallbacks diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 1c7ded73973f..69e0eb61426b 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -262,7 +262,7 @@ def get_or_create_user(self, requester, localpart, displayname, password_hash=No user = UserID(localpart, self.hs.hostname) user_id = user.to_string() - token = self.macaroon_generator.generate_access_token(user_id) + token = self.hs.get_auth().generate_access_token(user_id) if need_register: yield self.handler.register_with_store( From 912fbeaefdff3560518d346d11f60a8536654628 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 2 Jul 2019 09:57:12 +0100 Subject: [PATCH 03/14] changelog --- changelog.d/5587.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5587.misc diff --git a/changelog.d/5587.misc b/changelog.d/5587.misc new file mode 100644 index 000000000000..8463bd520a2a --- /dev/null +++ b/changelog.d/5587.misc @@ -0,0 +1 @@ +Switch to using opaque strings for access tokens instead of macaroons. \ No newline at end of file From 2953aa70de430f7a7d71d437e0db226b319530e2 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 2 Jul 2019 11:08:41 +0100 Subject: [PATCH 04/14] fix changelog --- changelog.d/{5587.misc => 5588.misc} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{5587.misc => 5588.misc} (100%) diff --git a/changelog.d/5587.misc b/changelog.d/5588.misc similarity index 100% rename from changelog.d/5587.misc rename to changelog.d/5588.misc From 6b562ce9fc8b08eb1ffcbd9585d9767d0ed4e5ab Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 2 Jul 2019 11:13:12 +0100 Subject: [PATCH 05/14] access token length --- synapse/handlers/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 5f8cd8ebb1c9..f82e3ce69087 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -835,7 +835,7 @@ def _check_local_password(self, user_id, password): def generate_access_token(self): """Generates an opaque string, for use as an access token""" - return stringutils.random_string(32) + return stringutils.random_string(20) @defer.inlineCallbacks def validate_short_term_login_token_and_get_user_id(self, login_token): From 25d9aa8294e25adc4b3d91673b39dce425942d59 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 4 May 2021 18:30:16 +0100 Subject: [PATCH 06/14] back out extraneous changes --- tests/handlers/test_register.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 748de4339791..734b6dc57592 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -466,7 +466,9 @@ def test_register_not_support_user(self): 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 + ) def test_spam_checker_deny(self): """A spam checker can deny registration, which results in an error.""" @@ -560,8 +562,6 @@ async def get_or_create_user( one will be randomly generated. Returns: A tuple of (user_id, access_token). - Raises: - RegistrationError if there was a problem registering. """ if localpart is None: raise SynapseError(400, "Request must include user id") From 57c73c222f0cf6559bfceaa0f522c24cf47d03c5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 4 May 2021 18:38:29 +0100 Subject: [PATCH 07/14] fix bad merge --- tests/handlers/test_register.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 734b6dc57592..a1b318a8273c 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -501,7 +501,7 @@ def check_registration_for_spam( user_id = self.get_success(self.handler.register_user(localpart="user")) # Get an access token. - token = self.macaroon_generator.generate_access_token(user_id) + token = self.hs.get_auth().generate_access_token() self.get_success( self.store.add_access_token_to_user( user_id=user_id, token=token, device_id=None, valid_until_ms=None @@ -578,7 +578,7 @@ async def get_or_create_user( user = UserID(localpart, self.hs.hostname) user_id = user.to_string() - token = self.hs.get_auth().generate_access_token(user_id) + token = self.hs.get_auth().generate_access_token() if need_register: await self.handler.register_with_store( From abda8cd26804568d8286065a309da1dbb618dc7e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 5 May 2021 19:14:06 +0100 Subject: [PATCH 08/14] lint --- tests/handlers/test_register.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index a1b318a8273c..c52206223dd6 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -17,7 +17,6 @@ from synapse.api.auth import Auth from synapse.api.constants import UserTypes from synapse.api.errors import Codes, ResourceLimitError, SynapseError -from synapse.handlers.register import RegistrationHandler from synapse.spam_checker_api import RegistrationBehaviour from synapse.types import RoomAlias, UserID, create_requester From 19ebc19c9a31ec03ee8e94cbe32c9efe53fc41bd Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 6 May 2021 15:59:08 +0100 Subject: [PATCH 09/14] fix tests --- tests/api/test_auth.py | 61 --------------------------------- tests/handlers/test_auth.py | 2 -- tests/handlers/test_register.py | 4 +-- 3 files changed, 2 insertions(+), 65 deletions(-) diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index c0ed64f78430..755dcbef8759 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -253,67 +253,6 @@ def test_get_guest_user_from_macaroon(self): self.assertTrue(user_info.is_guest) self.store.get_user_by_id.assert_called_with(user_id) - def test_cannot_use_regular_token_as_guest(self): - USER_ID = "@percy:matrix.org" - self.store.add_access_token_to_user = simple_async_mock(None) - self.store.get_device = simple_async_mock(None) - - token = self.get_success( - self.hs.get_auth_handler().get_access_token_for_user_id( - USER_ID, "DEVICE", valid_until_ms=None - ) - ) - self.store.add_access_token_to_user.assert_called_with( - user_id=USER_ID, - token=token, - device_id="DEVICE", - valid_until_ms=None, - puppets_user_id=None, - ) - - async def get_user(tok): - if token != tok: - return None - return TokenLookupResult( - user_id=USER_ID, - is_guest=False, - token_id=1234, - device_id="DEVICE", - ) - - self.store.get_user_by_access_token = get_user - self.store.get_user_by_id = simple_async_mock({"is_guest": False}) - - # check the token works - request = Mock(args={}) - request.args[b"access_token"] = [token.encode("ascii")] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - requester = self.get_success( - self.auth.get_user_by_req(request, allow_guest=True) - ) - self.assertEqual(UserID.from_string(USER_ID), requester.user) - self.assertFalse(requester.is_guest) - - # add an is_guest caveat - mac = pymacaroons.Macaroon.deserialize(token) - mac.add_first_party_caveat("guest = true") - guest_tok = mac.serialize() - - # the token should *not* work now - request = Mock(args={}) - request.args[b"access_token"] = [guest_tok.encode("ascii")] - request.requestHeaders.getRawHeaders = mock_getRawHeaders() - - cm = self.get_failure( - self.auth.get_user_by_req(request, allow_guest=True), - InvalidClientCredentialsError, - ) - - self.assertEqual(401, cm.value.code) - self.assertEqual("Guest access token used for regular user", cm.value.msg) - - self.store.get_user_by_id.assert_called_with(USER_ID) - def test_blocking_mau(self): self.auth_blocking._limit_usage_by_mau = False self.auth_blocking._max_mau_value = 50 diff --git a/tests/handlers/test_auth.py b/tests/handlers/test_auth.py index 4331656c6559..3142d7dbc4e0 100644 --- a/tests/handlers/test_auth.py +++ b/tests/handlers/test_auth.py @@ -36,8 +36,6 @@ def prepare(self, reactor, clock, hs): self.large_number_of_users = 100 def test_macaroon_caveats(self): - self.hs.clock.now = 5000 - token = self.macaroon_generator.generate_guest_access_token("a_user") macaroon = pymacaroons.Macaroon.deserialize(token) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index c52206223dd6..718d3b2ba87a 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -500,7 +500,7 @@ def check_registration_for_spam( user_id = self.get_success(self.handler.register_user(localpart="user")) # Get an access token. - token = self.hs.get_auth().generate_access_token() + token = self.hs.get_auth_handler().generate_access_token() self.get_success( self.store.add_access_token_to_user( user_id=user_id, token=token, device_id=None, valid_until_ms=None @@ -577,7 +577,7 @@ async def get_or_create_user( user = UserID(localpart, self.hs.hostname) user_id = user.to_string() - token = self.hs.get_auth().generate_access_token() + token = self.hs.get_auth_handler().generate_access_token() if need_register: await self.handler.register_with_store( From 7843281525fd6489b1719a6dccdab34851d35131 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 6 May 2021 16:03:36 +0100 Subject: [PATCH 10/14] lint --- tests/api/test_auth.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 755dcbef8759..1b0a81575739 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -21,13 +21,11 @@ from synapse.api.errors import ( AuthError, Codes, - InvalidClientCredentialsError, InvalidClientTokenError, MissingClientTokenError, ResourceLimitError, ) from synapse.storage.databases.main.registration import TokenLookupResult -from synapse.types import UserID from tests import unittest from tests.test_utils import simple_async_mock From 55265e8e1c8d4acc93672f1e94c886e8ec179e92 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 10 May 2021 23:02:42 +0100 Subject: [PATCH 11/14] Extend token format --- synapse/handlers/auth.py | 20 +++++++++++++++++--- synapse/util/stringutils.py | 20 ++++++++++++++++++++ tests/handlers/test_register.py | 4 ++-- tests/util/test_stringutils.py | 8 +++++++- 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 04209acb166f..8a6666a4ade6 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -17,6 +17,7 @@ import time import unicodedata import urllib.parse +from binascii import crc32 from typing import ( TYPE_CHECKING, Any, @@ -34,6 +35,7 @@ import attr import bcrypt import pymacaroons +import unpaddedbase64 from twisted.web.server import Request @@ -66,6 +68,7 @@ from synapse.util.async_helpers import maybe_awaitable from synapse.util.macaroons import get_value_from_macaroon, satisfy_expiry from synapse.util.msisdn import phone_number_to_msisdn +from synapse.util.stringutils import base62_encode from synapse.util.threepids import canonicalise_email if TYPE_CHECKING: @@ -808,10 +811,12 @@ async def get_access_token_for_user_id( logger.info( "Logging in user %s as %s%s", user_id, puppets_user_id, fmt_expiry ) + target_user_id_obj = UserID.from_string(puppets_user_id) else: logger.info( "Logging in user %s on device %s%s", user_id, device_id, fmt_expiry ) + target_user_id_obj = UserID.from_string(user_id) if ( not is_appservice_ghost @@ -819,7 +824,7 @@ async def get_access_token_for_user_id( ): await self.auth.check_auth_blocking(user_id) - access_token = self.generate_access_token() + access_token = self.generate_access_token(target_user_id_obj) await self.store.add_access_token_to_user( user_id=user_id, token=access_token, @@ -1192,9 +1197,18 @@ async def _check_local_password(self, user_id: str, password: str) -> Optional[s return None return user_id - def generate_access_token(self): + def generate_access_token(self, for_user: UserID) -> str: """Generates an opaque string, for use as an access token""" - return stringutils.random_string(20) + + # we use the following format for access tokens: + # syt___ + + b64local = unpaddedbase64.encode_base64(for_user.localpart.encode("utf-8")) + random_string = stringutils.random_string(20) + base = f"syt_{b64local}_{random_string}" + + crc = base62_encode(crc32(base.encode("ascii")), minwidth=6) + return f"{base}_{crc}" async def validate_short_term_login_token( self, login_token: str diff --git a/synapse/util/stringutils.py b/synapse/util/stringutils.py index cd82777f80eb..4f25cd1d26ae 100644 --- a/synapse/util/stringutils.py +++ b/synapse/util/stringutils.py @@ -220,3 +220,23 @@ def strtobool(val: str) -> bool: return False else: raise ValueError("invalid truth value %r" % (val,)) + + +_BASE62 = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" + + +def base62_encode(num: int, minwidth: int = 1) -> str: + """Encode a number using base62 + + Args: + num: number to be encoded + minwidth: width to pad to, if the number is small + """ + res = "" + while num: + num, rem = divmod(num, 62) + res = _BASE62[rem] + res + + # pad to minimum width + pad = "0" * (minwidth - len(res)) + return pad + res diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 718d3b2ba87a..205a84a9b2b4 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -500,7 +500,7 @@ def check_registration_for_spam( user_id = self.get_success(self.handler.register_user(localpart="user")) # Get an access token. - token = self.hs.get_auth_handler().generate_access_token() + token = "testtok" self.get_success( self.store.add_access_token_to_user( user_id=user_id, token=token, device_id=None, valid_until_ms=None @@ -577,7 +577,7 @@ async def get_or_create_user( user = UserID(localpart, self.hs.hostname) user_id = user.to_string() - token = self.hs.get_auth_handler().generate_access_token() + token = self.hs.get_auth_handler().generate_access_token(user) if need_register: await self.handler.register_with_store( diff --git a/tests/util/test_stringutils.py b/tests/util/test_stringutils.py index f7fecd9cf301..ad4dd7f0078f 100644 --- a/tests/util/test_stringutils.py +++ b/tests/util/test_stringutils.py @@ -13,7 +13,7 @@ # limitations under the License. from synapse.api.errors import SynapseError -from synapse.util.stringutils import assert_valid_client_secret +from synapse.util.stringutils import assert_valid_client_secret, base62_encode from .. import unittest @@ -45,3 +45,9 @@ def test_client_secret_regex(self): for client_secret in bad: with self.assertRaises(SynapseError): assert_valid_client_secret(client_secret) + + def test_base62_encode(self): + self.assertEqual("0", base62_encode(0)) + self.assertEqual("10", base62_encode(62)) + self.assertEqual("1c", base62_encode(100)) + self.assertEqual("001c", base62_encode(100, minwidth=4)) From 7bfa66ddf5d73985aa5de34c19f529e8ee797e79 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 11 May 2021 12:34:53 +0100 Subject: [PATCH 12/14] fix mau tests --- tests/handlers/test_auth.py | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/tests/handlers/test_auth.py b/tests/handlers/test_auth.py index 3142d7dbc4e0..5f3350e490da 100644 --- a/tests/handlers/test_auth.py +++ b/tests/handlers/test_auth.py @@ -16,12 +16,17 @@ import pymacaroons from synapse.api.errors import AuthError, ResourceLimitError +from synapse.rest import admin from tests import unittest from tests.test_utils import make_awaitable class AuthTestCase(unittest.HomeserverTestCase): + servlets = [ + admin.register_servlets, + ] + def prepare(self, reactor, clock, hs): self.auth_handler = hs.get_auth_handler() self.macaroon_generator = hs.get_macaroon_generator() @@ -35,6 +40,8 @@ def prepare(self, reactor, clock, hs): self.small_number_of_users = 1 self.large_number_of_users = 100 + self.user1 = self.register_user("a_user", "pass") + def test_macaroon_caveats(self): token = self.macaroon_generator.generate_guest_access_token("a_user") macaroon = pymacaroons.Macaroon.deserialize(token) @@ -64,10 +71,10 @@ def verify_guest(caveat): def test_short_term_login_token_gives_user_id(self): token = self.macaroon_generator.generate_short_term_login_token( - "a_user", "", 5000 + self.user1, "", 5000 ) res = self.get_success(self.auth_handler.validate_short_term_login_token(token)) - self.assertEqual("a_user", res.user_id) + self.assertEqual(self.user1, res.user_id) self.assertEqual("", res.auth_provider_id) # when we advance the clock, the token should be rejected @@ -79,22 +86,22 @@ def test_short_term_login_token_gives_user_id(self): def test_short_term_login_token_gives_auth_provider(self): token = self.macaroon_generator.generate_short_term_login_token( - "a_user", auth_provider_id="my_idp" + self.user1, auth_provider_id="my_idp" ) res = self.get_success(self.auth_handler.validate_short_term_login_token(token)) - self.assertEqual("a_user", res.user_id) + self.assertEqual(self.user1, res.user_id) self.assertEqual("my_idp", res.auth_provider_id) def test_short_term_login_token_cannot_replace_user_id(self): token = self.macaroon_generator.generate_short_term_login_token( - "a_user", "", 5000 + self.user1, "", 5000 ) macaroon = pymacaroons.Macaroon.deserialize(token) res = self.get_success( self.auth_handler.validate_short_term_login_token(macaroon.serialize()) ) - self.assertEqual("a_user", res.user_id) + self.assertEqual(self.user1, res.user_id) # add another "user_id" caveat, which might allow us to override the # user_id. @@ -110,7 +117,7 @@ def test_mau_limits_disabled(self): # Ensure does not throw exception self.get_success( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ) ) @@ -128,7 +135,7 @@ def test_mau_limits_exceeded_large(self): self.get_failure( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ), ResourceLimitError, ) @@ -156,7 +163,7 @@ def test_mau_limits_parity(self): # If not in monthly active cohort self.get_failure( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ), ResourceLimitError, ) @@ -173,7 +180,7 @@ def test_mau_limits_parity(self): ) self.get_success( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ) ) self.get_success( @@ -191,7 +198,7 @@ def test_mau_limits_not_exceeded(self): # Ensure does not raise exception self.get_success( self.auth_handler.get_access_token_for_user_id( - "user_a", device_id=None, valid_until_ms=None + self.user1, device_id=None, valid_until_ms=None ) ) @@ -206,6 +213,6 @@ def test_mau_limits_not_exceeded(self): def _get_macaroon(self): token = self.macaroon_generator.generate_short_term_login_token( - "user_a", "", 5000 + self.user1, "", 5000 ) return pymacaroons.Macaroon.deserialize(token) From 3b66b115c34e1486fde95efb6df9f7dd9882b5ba Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 11 May 2021 13:03:52 +0100 Subject: [PATCH 13/14] fix registration test --- tests/handlers/test_register.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 205a84a9b2b4..bd4319052338 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -48,10 +48,6 @@ def prepare(self, reactor, clock, hs): self.mock_distributor = Mock() self.mock_distributor.declare("registered_user") self.mock_captcha_client = Mock() - self.macaroon_generator = Mock( - generate_access_token=Mock(return_value="secret") - ) - self.hs.get_macaroon_generator = Mock(return_value=self.macaroon_generator) self.handler = self.hs.get_registration_handler() self.store = self.hs.get_datastore() self.lots_of_users = 100 @@ -67,8 +63,8 @@ def test_user_is_created_and_logged_in_if_doesnt_exist(self): self.get_or_create_user(requester, frank.localpart, "Frankie") ) self.assertEquals(result_user_id, user_id) - self.assertTrue(result_token is not None) - self.assertEquals(result_token, "secret") + self.assertIsInstance(result_token, str) + self.assertGreater(len(result_token), 20) def test_if_user_exists(self): store = self.hs.get_datastore() From b0c61c5b0fa9e37079ce688cfec313be2fa6c5b9 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 12 May 2021 14:36:48 +0100 Subject: [PATCH 14/14] Update 5588.misc --- changelog.d/5588.misc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/5588.misc b/changelog.d/5588.misc index 8463bd520a2a..b8f52a212c6e 100644 --- a/changelog.d/5588.misc +++ b/changelog.d/5588.misc @@ -1 +1 @@ -Switch to using opaque strings for access tokens instead of macaroons. \ No newline at end of file +Reduce the length of Synapse's access tokens.