Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Keep track of user_ips and monthly_active_users when delegating auth #16672

Merged
merged 12 commits into from
Nov 23, 2023
1 change: 1 addition & 0 deletions changelog.d/16672.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Restore tracking of requests and monthly active users when delegating authentication to an [MSC3861](https://github.com/matrix-org/synapse/pull/16672) OIDC provider.
48 changes: 48 additions & 0 deletions synapse/api/auth/base.py
Original file line number Diff line number Diff line change
@@ -27,6 +27,8 @@
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 @@ def __init__(self, hs: "HomeServer"):
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 @@ async def get_appservice_user(
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things to note here.

  1. OIDC access tokens have a limited lifetime and refresh. This means we'll end up writing a row once per refresh period per active device to this table. I'm not super keen on this. But stale entries automatically get cleaned up from this table, so it might not be the worst thing in the world.

  2. We wondered if this might affect the user retention metrics. AFAICS these are based on the user_daily_visits table, which is updated with this query:

    sql = """
    INSERT INTO user_daily_visits (user_id, device_id, timestamp, user_agent)
    SELECT u.user_id, u.device_id, ?, MAX(u.user_agent)
    FROM user_ips AS u
    LEFT JOIN (
    SELECT user_id, device_id, timestamp FROM user_daily_visits
    WHERE timestamp = ?
    ) udv
    ON u.user_id = udv.user_id AND u.device_id=udv.device_id
    INNER JOIN users ON users.name=u.user_id
    WHERE ? <= last_seen AND last_seen < ?
    AND udv.timestamp IS NULL AND users.is_guest=0
    AND users.appservice_id IS NULL
    GROUP BY u.user_id, u.device_id
    """

That query groups on (user_ips.user_id, user_ips.device_id), so can handle the same device appearing twice in the user_ips table. (As indeed it may, for e.g. a mobile client moving between wifi and mobile internet connections.) So we haven't written a bug here.

  1. Both of these concerns would also affect people using non-OIDC refreshing access tokens. It's just that nobody really used them(?) after they got specced.

4 . A cleaner approach would be to change this table to be keyed off (user, device, ip) instead of today's (user, access_token, ip). But I'm punting that for the future.

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,
)
39 changes: 1 addition & 38 deletions synapse/api/auth/internal.py
Original file line number Diff line number Diff line change
@@ -22,7 +22,6 @@
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 @@ def __init__(self, hs: "HomeServer"):
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 @@ async def _wrapped_get_user_by_req(
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 @@ async def _wrapped_get_user_by_req(
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(
1 change: 1 addition & 0 deletions synapse/api/auth/msc3861_delegated.py
Original file line number Diff line number Diff line change
@@ -226,6 +226,7 @@ async def get_user_by_req(
# TODO: we probably want to assert the allow_guest inside this call
# 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)
await self._record_request(request, requester)

if not allow_guest and requester.is_guest:
raise OAuthInsufficientScopeError([SCOPE_MATRIX_API])
21 changes: 21 additions & 0 deletions synapse/storage/databases/main/client_ips.py
Original file line number Diff line number Diff line change
@@ -589,6 +589,27 @@ async def insert_client_ip(
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-":
10 changes: 1 addition & 9 deletions tests/config/test_oauth_delegation.py
Original file line number Diff line number Diff line change
@@ -22,15 +22,7 @@

from tests.server import get_clock, setup_test_homeserver
from tests.unittest import TestCase, skip_unless
from tests.utils import default_config

try:
import authlib # noqa: F401

HAS_AUTHLIB = True
except ImportError:
HAS_AUTHLIB = False

from tests.utils import HAS_AUTHLIB, default_config

# These are a few constants that are used as config parameters in the tests.
SERVER_NAME = "test"
88 changes: 76 additions & 12 deletions tests/handlers/test_oauth_delegation.py
Original file line number Diff line number Diff line change
@@ -13,7 +13,8 @@
# limitations under the License.

from http import HTTPStatus
from typing import Any, Dict, Union
from io import BytesIO
from typing import Any, Dict, Optional, Union
from unittest.mock import ANY, AsyncMock, Mock
from urllib.parse import parse_qs

@@ -25,6 +26,8 @@
from signedjson.sign import sign_json

from twisted.test.proto_helpers import MemoryReactor
from twisted.web.http_headers import Headers
from twisted.web.iweb import IResponse

from synapse.api.errors import (
AuthError,
@@ -33,23 +36,17 @@
OAuthInsufficientScopeError,
SynapseError,
)
from synapse.http.site import SynapseRequest
from synapse.rest import admin
from synapse.rest.client import account, devices, keys, login, logout, register
from synapse.server import HomeServer
from synapse.types import JsonDict
from synapse.types import JsonDict, UserID
from synapse.util import Clock

from tests.server import FakeChannel
from tests.test_utils import FakeResponse, get_awaitable_result
from tests.unittest import HomeserverTestCase, skip_unless
from tests.utils import mock_getRawHeaders

try:
import authlib # noqa: F401

HAS_AUTHLIB = True
except ImportError:
HAS_AUTHLIB = False

from tests.unittest import HomeserverTestCase, override_config, skip_unless
from tests.utils import HAS_AUTHLIB, mock_getRawHeaders

# These are a few constants that are used as config parameters in the tests.
SERVER_NAME = "test"
@@ -685,3 +682,70 @@ def test_admin_token(self) -> None:

# There should be no call to the introspection endpoint
self.http_client.request.assert_not_called()

@override_config({"mau_stats_only": True})
def test_request_tracking(self) -> None:
"""Using an access token should update the client_ips and MAU tables."""
# To start, there are no MAU users.
store = self.hs.get_datastores().main
mau = self.get_success(store.get_monthly_active_count())
self.assertEqual(mau, 0)

known_token = "token-token-GOOD-:)"

async def mock_http_client_request(
method: str,
uri: str,
data: Optional[bytes] = None,
headers: Optional[Headers] = None,
) -> IResponse:
"""Mocked auth provider response."""
assert method == "POST"
token = parse_qs(data)[b"token"][0].decode("utf-8")
if token == known_token:
return FakeResponse.json(
code=200,
payload={
"active": True,
"scope": MATRIX_USER_SCOPE,
"sub": SUBJECT,
"username": USERNAME,
},
)

return FakeResponse.json(code=200, payload={"active": False})

self.http_client.request = mock_http_client_request

EXAMPLE_IPV4_ADDR = "123.123.123.123"
EXAMPLE_USER_AGENT = "httprettygood"

# First test a known access token
channel = FakeChannel(self.site, self.reactor)
# type-ignore: FakeChannel is a mock of an HTTPChannel, not a proper HTTPChannel
req = SynapseRequest(channel, self.site) # type: ignore[arg-type]
req.client.host = EXAMPLE_IPV4_ADDR
req.requestHeaders.addRawHeader("Authorization", f"Bearer {known_token}")
req.requestHeaders.addRawHeader("User-Agent", EXAMPLE_USER_AGENT)
req.content = BytesIO(b"")
req.requestReceived(
b"GET",
b"/_matrix/client/v3/account/whoami",
b"1.1",
)
channel.await_result()
self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body)
self.assertEqual(channel.json_body["user_id"], USER_ID, channel.json_body)

# Expect to see one MAU entry, from the first request
mau = self.get_success(store.get_monthly_active_count())
self.assertEqual(mau, 1)

conn_infos = self.get_success(
store.get_user_ip_and_agents(UserID.from_string(USER_ID))
)
self.assertEqual(len(conn_infos), 1, conn_infos)
conn_info = conn_infos[0]
self.assertEqual(conn_info["access_token"], known_token)
self.assertEqual(conn_info["ip"], EXAMPLE_IPV4_ADDR)
self.assertEqual(conn_info["user_agent"], EXAMPLE_USER_AGENT)
8 changes: 1 addition & 7 deletions tests/rest/admin/test_jwks.py
Original file line number Diff line number Diff line change
@@ -19,13 +19,7 @@
from synapse.rest.synapse.client import build_synapse_client_resource_tree

from tests.unittest import HomeserverTestCase, override_config, skip_unless

try:
import authlib # noqa: F401

HAS_AUTHLIB = True
except ImportError:
HAS_AUTHLIB = False
from tests.utils import HAS_AUTHLIB


@skip_unless(HAS_AUTHLIB, "requires authlib")
8 changes: 1 addition & 7 deletions tests/rest/client/test_keys.py
Original file line number Diff line number Diff line change
@@ -30,13 +30,7 @@
from tests import unittest
from tests.http.server._base import make_request_with_cancellation_test
from tests.unittest import override_config

try:
import authlib # noqa: F401

HAS_AUTHLIB = True
except ImportError:
HAS_AUTHLIB = False
from tests.utils import HAS_AUTHLIB


class KeyQueryTestCase(unittest.HomeserverTestCase):
8 changes: 1 addition & 7 deletions tests/rest/test_well_known.py
Original file line number Diff line number Diff line change
@@ -16,13 +16,7 @@
from synapse.rest.well_known import well_known_resource

from tests import unittest

try:
import authlib # noqa: F401

HAS_AUTHLIB = True
except ImportError:
HAS_AUTHLIB = False
from tests.utils import HAS_AUTHLIB


class WellKnownTests(unittest.HomeserverTestCase):
7 changes: 7 additions & 0 deletions tests/utils.py
Original file line number Diff line number Diff line change
@@ -30,6 +30,13 @@
from synapse.storage.engines import create_engine
from synapse.storage.prepare_database import prepare_database

try:
import authlib # noqa: F401

HAS_AUTHLIB = True
except ImportError:
HAS_AUTHLIB = False

# set this to True to run the tests against postgres instead of sqlite.
#
# When running under postgres, we first create a base database with the name
Loading