Keep track of `user_ips` and `monthly_active_users` when delegating auth (#16672)

* Describe `insert_client_ip`
* Pull out client_ips and MAU tracking to BaseAuth
* Define HAS_AUTHLIB once in tests

sick of copypasting

* Track ips and token usage when delegating auth
* Test that we track MAU and user_ips
* Don't track `__oidc_admin`

4 files changed, 74 insertions, 38 deletions

diff --git a/synapse/api/auth/base.py b/synapse/api/auth/base.py
index 9321d6f186..e2e3dc61b4 100644
--- a/synapse/api/auth/base.py
+++ b/synapse/api/auth/base.py
@@ -27,6 +27,8 @@ from synapse.api.errors import (
     UnstableSpecAuthError,
 )
 from synapse.appservice import ApplicationService
+from synapse.http import get_request_user_agent
+from synapse.http.site import SynapseRequest
 from synapse.logging.opentracing import trace
 from synapse.types import Requester, create_requester
 from synapse.util.cancellation import cancellable
@@ -45,6 +47,9 @@ class BaseAuth:
         self.store = hs.get_datastores().main
         self._storage_controllers = hs.get_storage_controllers()
 
+        self._track_appservice_user_ips = hs.config.appservice.track_appservice_user_ips
+        self._track_puppeted_user_ips = hs.config.api.track_puppeted_user_ips
+
     async def check_user_in_room(
         self,
         room_id: str,
@@ -349,3 +354,46 @@ class BaseAuth:
         return create_requester(
             effective_user_id, app_service=app_service, device_id=effective_device_id
         )
+
+    async def _record_request(
+        self, request: SynapseRequest, requester: Requester
+    ) -> None:
+        """Record that this request was made.
+
+        This updates the client_ips and monthly_active_user tables.
+        """
+        ip_addr = request.get_client_ip_if_available()
+
+        if ip_addr and (not requester.app_service or self._track_appservice_user_ips):
+            user_agent = get_request_user_agent(request)
+            access_token = self.get_access_token_from_request(request)
+
+            # XXX(quenting): I'm 95% confident that we could skip setting the
+            # device_id to "dummy-device" for appservices, and that the only impact
+            # would be some rows which whould not deduplicate in the 'user_ips'
+            # table during the transition
+            recorded_device_id = (
+                "dummy-device"
+                if requester.device_id is None and requester.app_service is not None
+                else requester.device_id
+            )
+            await self.store.insert_client_ip(
+                user_id=requester.authenticated_entity,
+                access_token=access_token,
+                ip=ip_addr,
+                user_agent=user_agent,
+                device_id=recorded_device_id,
+            )
+
+            # Track also the puppeted user client IP if enabled and the user is puppeting
+            if (
+                requester.user.to_string() != requester.authenticated_entity
+                and self._track_puppeted_user_ips
+            ):
+                await self.store.insert_client_ip(
+                    user_id=requester.user.to_string(),
+                    access_token=access_token,
+                    ip=ip_addr,
+                    user_agent=user_agent,
+                    device_id=requester.device_id,
+                )
diff --git a/synapse/api/auth/internal.py b/synapse/api/auth/internal.py
index 36ee9c8b8f..985cbb1278 100644
--- a/synapse/api/auth/internal.py
+++ b/synapse/api/auth/internal.py
@@ -22,7 +22,6 @@ from synapse.api.errors import (
     InvalidClientTokenError,
     MissingClientTokenError,
 )
-from synapse.http import get_request_user_agent
 from synapse.http.site import SynapseRequest
 from synapse.logging.opentracing import active_span, force_tracing, start_active_span
 from synapse.types import Requester, create_requester
@@ -48,8 +47,6 @@ class InternalAuth(BaseAuth):
         self._account_validity_handler = hs.get_account_validity_handler()
         self._macaroon_generator = hs.get_macaroon_generator()
 
-        self._track_appservice_user_ips = hs.config.appservice.track_appservice_user_ips
-        self._track_puppeted_user_ips = hs.config.api.track_puppeted_user_ips
         self._force_tracing_for_users = hs.config.tracing.force_tracing_for_users
 
     @cancellable
@@ -115,9 +112,6 @@ class InternalAuth(BaseAuth):
         Once get_user_by_req has set up the opentracing span, this does the actual work.
         """
         try:
-            ip_addr = request.get_client_ip_if_available()
-            user_agent = get_request_user_agent(request)
-
             access_token = self.get_access_token_from_request(request)
 
             # First check if it could be a request from an appservice
@@ -154,38 +148,7 @@ class InternalAuth(BaseAuth):
                             errcode=Codes.EXPIRED_ACCOUNT,
                         )
 
-            if ip_addr and (
-                not requester.app_service or self._track_appservice_user_ips
-            ):
-                # XXX(quenting): I'm 95% confident that we could skip setting the
-                # device_id to "dummy-device" for appservices, and that the only impact
-                # would be some rows which whould not deduplicate in the 'user_ips'
-                # table during the transition
-                recorded_device_id = (
-                    "dummy-device"
-                    if requester.device_id is None and requester.app_service is not None
-                    else requester.device_id
-                )
-                await self.store.insert_client_ip(
-                    user_id=requester.authenticated_entity,
-                    access_token=access_token,
-                    ip=ip_addr,
-                    user_agent=user_agent,
-                    device_id=recorded_device_id,
-                )
-
-                # Track also the puppeted user client IP if enabled and the user is puppeting
-                if (
-                    requester.user.to_string() != requester.authenticated_entity
-                    and self._track_puppeted_user_ips
-                ):
-                    await self.store.insert_client_ip(
-                        user_id=requester.user.to_string(),
-                        access_token=access_token,
-                        ip=ip_addr,
-                        user_agent=user_agent,
-                        device_id=requester.device_id,
-                    )
+            await self._record_request(request, requester)
 
             if requester.is_guest and not allow_guest:
                 raise AuthError(
diff --git a/synapse/api/auth/msc3861_delegated.py b/synapse/api/auth/msc3861_delegated.py
index 31bb035cc8..7373d81534 100644
--- a/synapse/api/auth/msc3861_delegated.py
+++ b/synapse/api/auth/msc3861_delegated.py
@@ -227,6 +227,10 @@ class MSC3861DelegatedAuth(BaseAuth):
             # so that we don't provision the user if they don't have enough permission:
             requester = await self.get_user_by_access_token(access_token, allow_expired)
 
+        # Do not record requests from MAS using the virtual `__oidc_admin` user.
+        if access_token != self._admin_token:
+            await self._record_request(request, requester)
+
         if not allow_guest and requester.is_guest:
             raise OAuthInsufficientScopeError([SCOPE_MATRIX_API])
 
diff --git a/synapse/storage/databases/main/client_ips.py b/synapse/storage/databases/main/client_ips.py
index c006129625..d4b14aaebe 100644
--- a/synapse/storage/databases/main/client_ips.py
+++ b/synapse/storage/databases/main/client_ips.py
@@ -589,6 +589,27 @@ class ClientIpWorkerStore(ClientIpBackgroundUpdateStore, MonthlyActiveUsersWorke
         device_id: Optional[str],
         now: Optional[int] = None,
     ) -> None:
+        """Record that `user_id` used `access_token` from this `ip` address.
+
+        This method does two things.
+
+        1. It queues up a row to be upserted into the `client_ips` table. These happen
+           periodically; see _update_client_ips_batch.
+        2. It immediately records this user as having taken action for the purposes of
+           MAU tracking.
+
+        Any DB writes take place on the background tasks worker, falling back to the
+        main process. If we're not that worker, this method emits a replication payload
+        to run this logic on that worker.
+
+        Two caveats to note:
+
+         - We only take action once per LAST_SEEN_GRANULARITY, to avoid spamming the
+           DB with writes.
+         - Requests using the sliding-sync proxy's user agent are excluded, as its
+           requests are not directly driven by end-users. This is a hack and we're not
+           very proud of it.
+        """
         # The sync proxy continuously triggers /sync even if the user is not
         # present so should be excluded from user_ips entries.
         if user_agent == "sync-v3-proxy-":