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

Conversation

DMRobertson
Copy link
Contributor

This was present in Synapse's "internal" auth, but was not available when delegating auth to an OIDC provider until now.

I haven't tested this end-to-end. But there isn't any variation in the way we write to the DB. So I'm fairly confident this should work. (Unless I'm missing something @sandhose?)

@DMRobertson DMRobertson marked this pull request as ready for review November 22, 2023 13:40
@DMRobertson DMRobertson requested a review from a team as a code owner November 22, 2023 13:40
@DMRobertson
Copy link
Contributor Author

As a side-effect, this should reinstate MAU limiting in Synapse. Endpoints like /login and /register that are handled by the auth service won't be limited though.

Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

Tested locally and it indeeds tracks MAU.

I although noticed that the special __oidc_admin virtual user gets tracked, which shouldn't be the case.

synapse/api/auth/msc3861_delegated.py Show resolved Hide resolved
)
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.

@DMRobertson
Copy link
Contributor Author

I although noticed that the special __oidc_admin virtual user gets tracked, which shouldn't be the case.

Good spot, thank you. 45ffd5f should fix.

Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

Tried locally, worked as expected

@DMRobertson DMRobertson merged commit 32a59a6 into develop Nov 23, 2023
41 checks passed
@DMRobertson DMRobertson deleted the dmr/oidc-mau branch November 23, 2023 12:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants