From 7932d4e9f7e140ef05a3099e18120101019ec3e1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 17 Dec 2020 12:04:14 +0000 Subject: [PATCH 1/4] Don't MAU limit AS ghost users --- synapse/api/auth_blocking.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/synapse/api/auth_blocking.py b/synapse/api/auth_blocking.py index 9c227218e02b..d8088f524ac7 100644 --- a/synapse/api/auth_blocking.py +++ b/synapse/api/auth_blocking.py @@ -36,6 +36,7 @@ def __init__(self, hs): self._limit_usage_by_mau = hs.config.limit_usage_by_mau self._mau_limits_reserved_threepids = hs.config.mau_limits_reserved_threepids self._server_name = hs.hostname + self._track_appservice_user_ips = hs.config.appservice.track_appservice_user_ips async def check_auth_blocking( self, @@ -76,6 +77,12 @@ async def check_auth_blocking( # We never block the server from doing actions on behalf of # users. return + elif requester.app_service and not self._track_appservice_user_ips: + # If we're authenticated as an appservice then we only block + # auth if `track_appservice_user_ips` is set, as that option + # implicitly means that application services are part of MAU + # limits. + return # Never fail an auth check for the server notices users or support user # This can be a problem where event creation is prohibited due to blocking From 35be2600902189392aaa6a212058853e9e39aeba Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 17 Dec 2020 12:05:18 +0000 Subject: [PATCH 2/4] Newsfile --- changelog.d/8962.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8962.bugfix diff --git a/changelog.d/8962.bugfix b/changelog.d/8962.bugfix new file mode 100644 index 000000000000..af1a5e4c3a9d --- /dev/null +++ b/changelog.d/8962.bugfix @@ -0,0 +1 @@ +Fix bug where application services couldn't register new ghost users if the server had reached its MAU limit. From 4c33796b20f934a43f4f09a2bac6653c18d72b69 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 17 Dec 2020 12:55:21 +0000 Subject: [PATCH 3/4] Correctly handle AS registerations and add test --- synapse/handlers/auth.py | 8 ++++- synapse/handlers/register.py | 7 +++- synapse/replication/http/login.py | 12 +++++-- synapse/rest/client/v2_alpha/register.py | 14 ++++++-- tests/test_mau.py | 45 ++++++++++++++++++++++-- 5 files changed, 77 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index c7dc07008a1b..3b8ac4325b56 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -709,6 +709,7 @@ async def get_access_token_for_user_id( device_id: Optional[str], valid_until_ms: Optional[int], puppets_user_id: Optional[str] = None, + is_appservice_ghost: bool = False, ) -> str: """ Creates a new access token for the user with the given user ID. @@ -725,6 +726,7 @@ async def get_access_token_for_user_id( we should always have a device ID) valid_until_ms: when the token is valid until. None for no expiry. + is_appservice_ghost: Whether the user is an application ghost user Returns: The access token for the user's session. Raises: @@ -745,7 +747,11 @@ async def get_access_token_for_user_id( "Logging in user %s on device %s%s", user_id, device_id, fmt_expiry ) - await self.auth.check_auth_blocking(user_id) + if ( + not is_appservice_ghost + or self.hs.config.appservice.track_appservice_user_ips + ): + await self.auth.check_auth_blocking(user_id) access_token = self.macaroon_gen.generate_access_token(user_id) await self.store.add_access_token_to_user( diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 0d85fd08684f..039aff10619c 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -630,6 +630,7 @@ async def register_device( device_id: Optional[str], initial_display_name: Optional[str], is_guest: bool = False, + is_appservice_ghost: bool = False, ) -> Tuple[str, str]: """Register a device for a user and generate an access token. @@ -651,6 +652,7 @@ async def register_device( device_id=device_id, initial_display_name=initial_display_name, is_guest=is_guest, + is_appservice_ghost=is_appservice_ghost, ) return r["device_id"], r["access_token"] @@ -672,7 +674,10 @@ async def register_device( ) else: access_token = await self._auth_handler.get_access_token_for_user_id( - user_id, device_id=registered_device_id, valid_until_ms=valid_until_ms + user_id, + device_id=registered_device_id, + valid_until_ms=valid_until_ms, + is_appservice_ghost=is_appservice_ghost, ) return (registered_device_id, access_token) diff --git a/synapse/replication/http/login.py b/synapse/replication/http/login.py index 4c81e2d784bd..36071feb36df 100644 --- a/synapse/replication/http/login.py +++ b/synapse/replication/http/login.py @@ -36,7 +36,9 @@ def __init__(self, hs): self.registration_handler = hs.get_registration_handler() @staticmethod - async def _serialize_payload(user_id, device_id, initial_display_name, is_guest): + async def _serialize_payload( + user_id, device_id, initial_display_name, is_guest, is_appservice_ghost + ): """ Args: device_id (str|None): Device ID to use, if None a new one is @@ -48,6 +50,7 @@ async def _serialize_payload(user_id, device_id, initial_display_name, is_guest) "device_id": device_id, "initial_display_name": initial_display_name, "is_guest": is_guest, + "is_appservice_ghost": is_appservice_ghost, } async def _handle_request(self, request, user_id): @@ -56,9 +59,14 @@ async def _handle_request(self, request, user_id): device_id = content["device_id"] initial_display_name = content["initial_display_name"] is_guest = content["is_guest"] + is_appservice_ghost = content["is_appservice_ghost"] device_id, access_token = await self.registration_handler.register_device( - user_id, device_id, initial_display_name, is_guest + user_id, + device_id, + initial_display_name, + is_guest, + is_appservice_ghost=is_appservice_ghost, ) return 200, {"device_id": device_id, "access_token": access_token} diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index a89ae6ddf9d4..722d993811fe 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -655,9 +655,13 @@ async def _do_appservice_registration(self, username, as_token, body): user_id = await self.registration_handler.appservice_register( username, as_token ) - return await self._create_registration_details(user_id, body) + return await self._create_registration_details( + user_id, body, is_appservice_ghost=True, + ) - async def _create_registration_details(self, user_id, params): + async def _create_registration_details( + self, user_id, params, is_appservice_ghost=False + ): """Complete registration of newly-registered user Allocates device_id if one was not given; also creates access_token. @@ -674,7 +678,11 @@ async def _create_registration_details(self, user_id, params): device_id = params.get("device_id") initial_display_name = params.get("initial_device_display_name") device_id, access_token = await self.registration_handler.register_device( - user_id, device_id, initial_display_name, is_guest=False + user_id, + device_id, + initial_display_name, + is_guest=False, + is_appservice_ghost=is_appservice_ghost, ) result.update({"access_token": access_token, "device_id": device_id}) diff --git a/tests/test_mau.py b/tests/test_mau.py index c5ec6396a770..26548b46110c 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -19,6 +19,7 @@ from synapse.api.constants import LoginType from synapse.api.errors import Codes, HttpResponseException, SynapseError +from synapse.appservice import ApplicationService from synapse.rest.client.v2_alpha import register, sync from tests import unittest @@ -75,6 +76,44 @@ def test_simple_deny_mau(self): self.assertEqual(e.code, 403) self.assertEqual(e.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + def test_as_ignores_mau(self): + """Test that application services can still create users when the MAU + limit has been reached. + """ + + # Create and sync so that the MAU counts get updated + token1 = self.create_user("kermit1") + self.do_sync_for_user(token1) + token2 = self.create_user("kermit2") + self.do_sync_for_user(token2) + + # check we're testing what we think we are: there should be two active users + self.assertEqual(self.get_success(self.store.get_monthly_active_count()), 2) + + # We've created and activated two users, we shouldn't be able to + # register new users + with self.assertRaises(SynapseError) as cm: + self.create_user("kermit3") + + e = cm.exception + self.assertEqual(e.code, 403) + self.assertEqual(e.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + + # Cheekily add an application service that we use to register a new user + # with. + as_token = "foobartoken" + self.store.services_cache.append( + ApplicationService( + token=as_token, + hostname=self.hs.hostname, + id="SomeASID", + sender="@as_sender:test", + namespaces={"users": [{"regex": "@as_*", "exclusive": True}]}, + ) + ) + + self.create_user("as_kermit4", token=as_token) + def test_allowed_after_a_month_mau(self): # Create and sync so that the MAU counts get updated token1 = self.create_user("kermit1") @@ -192,7 +231,7 @@ def test_tracked_but_not_limited(self): self.reactor.advance(100) self.assertEqual(2, self.successResultOf(count)) - def create_user(self, localpart): + def create_user(self, localpart, token=None): request_data = json.dumps( { "username": localpart, @@ -201,7 +240,9 @@ def create_user(self, localpart): } ) - request, channel = self.make_request("POST", "/register", request_data) + request, channel = self.make_request( + "POST", "/register", request_data, access_token=token + ) if channel.code != 200: raise HttpResponseException( From 14eab1b4d2cf837c6b0925da7194cc5940e9401c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 17 Dec 2020 16:14:13 +0000 Subject: [PATCH 4/4] Update tests/test_mau.py Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- tests/test_mau.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_mau.py b/tests/test_mau.py index 26548b46110c..586294446b9a 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -78,7 +78,8 @@ def test_simple_deny_mau(self): def test_as_ignores_mau(self): """Test that application services can still create users when the MAU - limit has been reached. + limit has been reached. This only works when application service + user ip tracking is disabled. """ # Create and sync so that the MAU counts get updated