From 7562d887e159f404c8d752271310f4432f246656 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 12 May 2021 15:04:51 +0100 Subject: Change the format of access tokens away from macaroons (#5588) --- tests/api/test_auth.py | 63 ----------------------------------------- tests/handlers/test_auth.py | 43 +++++++++++++++------------- tests/handlers/test_register.py | 12 +++----- tests/util/test_stringutils.py | 8 +++++- 4 files changed, 34 insertions(+), 92 deletions(-) (limited to 'tests') diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index c0ed64f784..1b0a815757 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -21,13 +21,11 @@ from synapse.api.constants import UserTypes 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 @@ -253,67 +251,6 @@ class AuthTestCase(unittest.HomeserverTestCase): 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 fe7e9484fd..5f3350e490 100644 --- a/tests/handlers/test_auth.py +++ b/tests/handlers/test_auth.py @@ -16,12 +16,17 @@ from unittest.mock import Mock 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,16 +40,10 @@ class AuthTestCase(unittest.HomeserverTestCase): 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()) + self.user1 = self.register_user("a_user", "pass") def test_macaroon_caveats(self): - 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): @@ -59,19 +58,23 @@ class AuthTestCase(unittest.HomeserverTestCase): 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) 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 @@ -83,22 +86,22 @@ class AuthTestCase(unittest.HomeserverTestCase): 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. @@ -114,7 +117,7 @@ class AuthTestCase(unittest.HomeserverTestCase): # 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 ) ) @@ -132,7 +135,7 @@ class AuthTestCase(unittest.HomeserverTestCase): 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, ) @@ -160,7 +163,7 @@ class AuthTestCase(unittest.HomeserverTestCase): # 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, ) @@ -177,7 +180,7 @@ class AuthTestCase(unittest.HomeserverTestCase): ) 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( @@ -195,7 +198,7 @@ class AuthTestCase(unittest.HomeserverTestCase): # 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 ) ) @@ -210,6 +213,6 @@ class AuthTestCase(unittest.HomeserverTestCase): def _get_macaroon(self): token = self.macaroon_generator.generate_short_term_login_token( - "user_a", "", 5000 + self.user1, "", 5000 ) return pymacaroons.Macaroon.deserialize(token) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 608f8f3d33..bd43190523 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -48,10 +48,6 @@ class RegistrationTestCase(unittest.HomeserverTestCase): 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 @@ class RegistrationTestCase(unittest.HomeserverTestCase): 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() @@ -500,7 +496,7 @@ class RegistrationTestCase(unittest.HomeserverTestCase): 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 = "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 +573,7 @@ class RegistrationTestCase(unittest.HomeserverTestCase): 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_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 f7fecd9cf3..ad4dd7f007 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 @@ class StringUtilsTestCase(unittest.TestCase): 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)) -- cgit 1.5.1 From a683028d81606708f686b890c0a44f5a20b54798 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 12 May 2021 16:05:28 +0200 Subject: Correctly ratelimit invites when creating a room (#9968) * Correctly ratelimit invites when creating a room Also allow ratelimiting for more than one action at a time. --- changelog.d/9968.bugfix | 1 + synapse/api/ratelimiting.py | 22 +++++++++++---- synapse/handlers/room.py | 27 +++++++++++++----- synapse/handlers/room_member.py | 25 +++++++++++++++++ tests/api/test_ratelimiting.py | 57 ++++++++++++++++++++++++++++++++++++++ tests/rest/client/v1/test_rooms.py | 37 +++++++++++++++++++++++++ 6 files changed, 157 insertions(+), 12 deletions(-) create mode 100644 changelog.d/9968.bugfix (limited to 'tests') diff --git a/changelog.d/9968.bugfix b/changelog.d/9968.bugfix new file mode 100644 index 0000000000..39e75f9956 --- /dev/null +++ b/changelog.d/9968.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.27.0 preventing users and appservices exempt from ratelimiting from creating rooms with many invitees. diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py index 2244b8a340..b9a10283f4 100644 --- a/synapse/api/ratelimiting.py +++ b/synapse/api/ratelimiting.py @@ -57,6 +57,7 @@ class Ratelimiter: rate_hz: Optional[float] = None, burst_count: Optional[int] = None, update: bool = True, + n_actions: int = 1, _time_now_s: Optional[int] = None, ) -> Tuple[bool, float]: """Can the entity (e.g. user or IP address) perform the action? @@ -76,6 +77,9 @@ class Ratelimiter: burst_count: How many actions that can be performed before being limited. Overrides the value set during instantiation if set. update: Whether to count this check as performing the action + n_actions: The number of times the user wants to do this action. If the user + cannot do all of the actions, the user's action count is not incremented + at all. _time_now_s: The current time. Optional, defaults to the current time according to self.clock. Only used by tests. @@ -124,17 +128,20 @@ class Ratelimiter: time_delta = time_now_s - time_start performed_count = action_count - time_delta * rate_hz if performed_count < 0: - # Allow, reset back to count 1 - allowed = True + performed_count = 0 time_start = time_now_s - action_count = 1.0 - elif performed_count > burst_count - 1.0: + + # This check would be easier read as performed_count + n_actions > burst_count, + # but performed_count might be a very precise float (with lots of numbers + # following the point) in which case Python might round it up when adding it to + # n_actions. Writing it this way ensures it doesn't happen. + if performed_count > burst_count - n_actions: # Deny, we have exceeded our burst count allowed = False else: # We haven't reached our limit yet allowed = True - action_count += 1.0 + action_count = performed_count + n_actions if update: self.actions[key] = (action_count, time_start, rate_hz) @@ -182,6 +189,7 @@ class Ratelimiter: rate_hz: Optional[float] = None, burst_count: Optional[int] = None, update: bool = True, + n_actions: int = 1, _time_now_s: Optional[int] = None, ): """Checks if an action can be performed. If not, raises a LimitExceededError @@ -201,6 +209,9 @@ class Ratelimiter: burst_count: How many actions that can be performed before being limited. Overrides the value set during instantiation if set. update: Whether to count this check as performing the action + n_actions: The number of times the user wants to do this action. If the user + cannot do all of the actions, the user's action count is not incremented + at all. _time_now_s: The current time. Optional, defaults to the current time according to self.clock. Only used by tests. @@ -216,6 +227,7 @@ class Ratelimiter: rate_hz=rate_hz, burst_count=burst_count, update=update, + n_actions=n_actions, _time_now_s=time_now_s, ) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index fb4823a5cc..835d874cee 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -32,7 +32,14 @@ from synapse.api.constants import ( RoomCreationPreset, RoomEncryptionAlgorithms, ) -from synapse.api.errors import AuthError, Codes, NotFoundError, StoreError, SynapseError +from synapse.api.errors import ( + AuthError, + Codes, + LimitExceededError, + NotFoundError, + StoreError, + SynapseError, +) from synapse.api.filtering import Filter from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion from synapse.events import EventBase @@ -126,10 +133,6 @@ class RoomCreationHandler(BaseHandler): self.third_party_event_rules = hs.get_third_party_event_rules() - self._invite_burst_count = ( - hs.config.ratelimiting.rc_invites_per_room.burst_count - ) - async def upgrade_room( self, requester: Requester, old_room_id: str, new_version: RoomVersion ) -> str: @@ -676,8 +679,18 @@ class RoomCreationHandler(BaseHandler): invite_3pid_list = [] invite_list = [] - if len(invite_list) + len(invite_3pid_list) > self._invite_burst_count: - raise SynapseError(400, "Cannot invite so many users at once") + if invite_list or invite_3pid_list: + try: + # If there are invites in the request, see if the ratelimiting settings + # allow that number of invites to be sent from the current user. + await self.room_member_handler.ratelimit_multiple_invites( + requester, + room_id=None, + n_invites=len(invite_list) + len(invite_3pid_list), + update=False, + ) + except LimitExceededError: + raise SynapseError(400, "Cannot invite so many users at once") await self.event_creation_handler.assert_accepted_privacy_policy(requester) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 20700fc5a8..9a092da715 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -163,6 +163,31 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): async def forget(self, user: UserID, room_id: str) -> None: raise NotImplementedError() + async def ratelimit_multiple_invites( + self, + requester: Optional[Requester], + room_id: Optional[str], + n_invites: int, + update: bool = True, + ): + """Ratelimit more than one invite sent by the given requester in the given room. + + Args: + requester: The requester sending the invites. + room_id: The room the invites are being sent in. + n_invites: The amount of invites to ratelimit for. + update: Whether to update the ratelimiter's cache. + + Raises: + LimitExceededError: The requester can't send that many invites in the room. + """ + await self._invites_per_room_limiter.ratelimit( + requester, + room_id, + update=update, + n_actions=n_invites, + ) + async def ratelimit_invite( self, requester: Optional[Requester], diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py index fa96ba07a5..dcf0110c16 100644 --- a/tests/api/test_ratelimiting.py +++ b/tests/api/test_ratelimiting.py @@ -230,3 +230,60 @@ class TestRatelimiter(unittest.HomeserverTestCase): # Shouldn't raise for _ in range(20): self.get_success_or_raise(limiter.ratelimit(requester, _time_now_s=0)) + + def test_multiple_actions(self): + limiter = Ratelimiter( + store=self.hs.get_datastore(), clock=None, rate_hz=0.1, burst_count=3 + ) + # Test that 4 actions aren't allowed with a maximum burst of 3. + allowed, time_allowed = self.get_success_or_raise( + limiter.can_do_action(None, key="test_id", n_actions=4, _time_now_s=0) + ) + self.assertFalse(allowed) + + # Test that 3 actions are allowed with a maximum burst of 3. + allowed, time_allowed = self.get_success_or_raise( + limiter.can_do_action(None, key="test_id", n_actions=3, _time_now_s=0) + ) + self.assertTrue(allowed) + self.assertEquals(10.0, time_allowed) + + # Test that, after doing these 3 actions, we can't do any more action without + # waiting. + allowed, time_allowed = self.get_success_or_raise( + limiter.can_do_action(None, key="test_id", n_actions=1, _time_now_s=0) + ) + self.assertFalse(allowed) + self.assertEquals(10.0, time_allowed) + + # Test that after waiting we can do only 1 action. + allowed, time_allowed = self.get_success_or_raise( + limiter.can_do_action( + None, + key="test_id", + update=False, + n_actions=1, + _time_now_s=10, + ) + ) + self.assertTrue(allowed) + # The time allowed is the current time because we could still repeat the action + # once. + self.assertEquals(10.0, time_allowed) + + allowed, time_allowed = self.get_success_or_raise( + limiter.can_do_action(None, key="test_id", n_actions=2, _time_now_s=10) + ) + self.assertFalse(allowed) + # The time allowed doesn't change despite allowed being False because, while we + # don't allow 2 actions, we could still do 1. + self.assertEquals(10.0, time_allowed) + + # Test that after waiting a bit more we can do 2 actions. + allowed, time_allowed = self.get_success_or_raise( + limiter.can_do_action(None, key="test_id", n_actions=2, _time_now_s=20) + ) + self.assertTrue(allowed) + # The time allowed is the current time because we could still repeat the action + # once. + self.assertEquals(20.0, time_allowed) diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index a3694f3d02..7c4bdcdfdd 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -463,6 +463,43 @@ class RoomsCreateTestCase(RoomBase): ) self.assertEquals(400, channel.code) + @unittest.override_config({"rc_invites": {"per_room": {"burst_count": 3}}}) + def test_post_room_invitees_ratelimit(self): + """Test that invites sent when creating a room are ratelimited by a RateLimiter, + which ratelimits them correctly, including by not limiting when the requester is + exempt from ratelimiting. + """ + + # Build the request's content. We use local MXIDs because invites over federation + # are more difficult to mock. + content = json.dumps( + { + "invite": [ + "@alice1:red", + "@alice2:red", + "@alice3:red", + "@alice4:red", + ] + } + ).encode("utf8") + + # Test that the invites are correctly ratelimited. + channel = self.make_request("POST", "/createRoom", content) + self.assertEqual(400, channel.code) + self.assertEqual( + "Cannot invite so many users at once", + channel.json_body["error"], + ) + + # Add the current user to the ratelimit overrides, allowing them no ratelimiting. + self.get_success( + self.hs.get_datastore().set_ratelimit_for_user(self.user_id, 0, 0) + ) + + # Test that the invites aren't ratelimited anymore. + channel = self.make_request("POST", "/createRoom", content) + self.assertEqual(200, channel.code) + class RoomTopicTestCase(RoomBase): """ Tests /rooms/$room_id/topic REST events. """ -- cgit 1.5.1