diff --git a/backend/druks/services/base.py b/backend/druks/services/base.py index 3b61e903..dcd49031 100644 --- a/backend/druks/services/base.py +++ b/backend/druks/services/base.py @@ -118,6 +118,11 @@ class Service: # identity_scopes join the consent ask. identity_endpoint: ClassVar[str] = "" identity_scopes: ClassVar[tuple[str, ...]] = () + # The identity fact that names the provider account — "sub" for Google, + # "id" for GitHub. When set, a fresh sign-in that matches an existing + # connection for the same owner updates that row; a revoked row becomes + # live again. When empty, each fresh sign-in creates a new connection. + identity_key: ClassVar[str] = "" def __init_subclass__(cls, **kwargs: Any) -> None: super().__init_subclass__(**kwargs) diff --git a/backend/druks/services/models.py b/backend/druks/services/models.py index b9d3b5fd..2569e3e6 100644 --- a/backend/druks/services/models.py +++ b/backend/druks/services/models.py @@ -115,6 +115,26 @@ def list_for_account(cls, provider: str, account_id: str) -> "list[OauthConnecti ) ) + @classmethod + def get_for_identity( + cls, provider: str, account_id: str, key: str, value: Any + ) -> "OauthConnection | None": + # A live match wins; among revoked matches, the latest consent wins. + return ( + db_session() + .scalars( + select(cls) + .where( + cls.provider == provider, + cls.account_id == account_id, + cls.identity[key].astext == str(value), + ) + .order_by(cls.revoked_at.is_(None).desc(), cls.connected_at.desc()) + .limit(1) + ) + .first() + ) + @classmethod def list_for_provider( cls, provider: str, *, include_revoked: bool = False diff --git a/backend/druks/services/routes.py b/backend/druks/services/routes.py index 26320f89..3c0405bc 100644 --- a/backend/druks/services/routes.py +++ b/backend/druks/services/routes.py @@ -126,15 +126,22 @@ async def oauth_callback(state: str = "", code: str = "", error: str = "") -> Re raise HTTPException(status_code=400, detail=f"No OAuth service {provider!r}.") granted = tokens.get("scope", "").split() or pending["scopes"] identity = await service.get_identity(tokens["access_token"]) - reconsent = bool(pending["connection_id"]) - if reconsent: - row = OauthConnection.get(pending["connection_id"]) + connection_id = pending["connection_id"] + # Reconsent names an existing row by id; a declared identity key + # matches a fresh sign-in to one. Both make a revoked row live again. + row = None + if connection_id: + row = OauthConnection.get(connection_id) if not row: raise HTTPException( status_code=400, detail="The connection was removed while consent was open." ) - # Reconsent names the row, so it also returns a revoked one to life. - # A fresh sign-in never does — it always creates a new connection. + elif service.identity_key and (value := identity.get(service.identity_key)): + row = OauthConnection.get_for_identity( + provider, pending["account_id"], service.identity_key, value + ) + reconsent = bool(row) + if row: row.reconnect(refresh_token=tokens["refresh_token"], scopes=granted, identity=identity) # A token cached before this consent must not serve the new one. await OauthClient(provider=provider).evict_access_token(row.id) diff --git a/backend/tests/test_services.py b/backend/tests/test_services.py index 98a87acd..a45bd06a 100644 --- a/backend/tests/test_services.py +++ b/backend/tests/test_services.py @@ -3,14 +3,17 @@ import html import json from types import SimpleNamespace +from urllib.parse import parse_qsl, urlparse import httpx import pytest +from druks.accounts.models import Account from druks.core.apis.github import GitHubClient, get_github_client from druks.core.webhooks.github import GitHubEvents from druks.database import db_session from druks.services.exceptions import ServiceNotConnectedError -from druks.services.models import ServiceIdentity +from druks.services.models import OauthConnection, ServiceIdentity +from druks.services.oauth import OauthClient from druks.testing import make_settings from fastapi import HTTPException from fastapi.testclient import TestClient @@ -575,6 +578,7 @@ class Acme(Service): token_endpoint = "https://acme.test/token" identity_endpoint = "https://acme.test/whoami" identity_scopes = ("openid",) + identity_key = "sub" class Settings(BaseModel): client_id: str @@ -604,6 +608,36 @@ def handler(request: httpx.Request) -> httpx.Response: return Acme +@pytest.fixture +def keyed_acme(acme, monkeypatch): + async def get_identity(service, access_token): + assert service is acme + assert access_token == "at-1" + return {"sub": "account-1", "email": "op@acme.test"} + + monkeypatch.setattr(acme, "get_identity", classmethod(get_identity)) + return acme + + +@pytest.fixture +def oauth_events(monkeypatch): + events = [] + + async def record(name, **kwargs): + events.append((name, kwargs)) + + monkeypatch.setattr("druks.services.routes.publish", record) + return events + + +def _complete_oauth_sign_in(client: TestClient, provider: str = "acme") -> None: + consent = client.get(f"/api/oauth/{provider}/connect", follow_redirects=False) + state = dict(parse_qsl(urlparse(consent.headers["location"]).query))["state"] + response = client.get("/api/oauth/callback", params={"state": state, "code": "c-1"}) + + assert response.status_code == 200 + + def test_oauth_connect_redirects_to_consent_with_the_scope_union( tmp_path, acme, druks_db, monkeypatch ): @@ -723,6 +757,165 @@ def test_oauth_connect_rejects_an_unknown_reconnect_target(tmp_path, acme, druks assert client.get("/api/oauth/acme/connect?connection=zzz").status_code == 404 +def test_fresh_sign_in_with_matching_identity_resurrects_revoked_connection( + tmp_path, keyed_acme, druks_db, oauth_events +): + from druks.testing import configure_app_for_test + + ServiceIdentity.connect( + keyed_acme.name, identity={"client_id": "id-1"}, secrets={"client_secret": "sec-1"} + ) + account = Account.get_or_create("op@example.com") + connection = OauthConnection.create( + provider=keyed_acme.name, + account_id=account.id, + refresh_token="rt-old", + scopes=["profile.read"], + identity={"sub": "account-1"}, + ) + connection_id = connection.id + connection.revoke("user") + settings = make_settings(tmp_path, urls={"endpoint": "https://druks.example"}) + + with TestClient(configure_app_for_test(settings=settings)) as client: + _complete_oauth_sign_in(client, keyed_acme.name) + + db_session().expire_all() + resurrected = OauthConnection.get(connection_id) + assert resurrected + assert not resurrected.revoked_at + assert not resurrected.revoked_reason + assert resurrected.refresh_token.decrypt() == "rt-1" + assert [row.id for row in OauthConnection.list_for_provider(keyed_acme.name)] == [connection_id] + assert len(OauthConnection.list_for_provider(keyed_acme.name, include_revoked=True)) == 1 + assert oauth_events == [ + ( + "oauth.connected", + { + "provider": keyed_acme.name, + "connection_id": connection_id, + "account_id": account.id, + "reconsent": True, + }, + ) + ] + + +def test_matching_fresh_sign_in_lands_on_live_connection_and_evicts_cached_token( + tmp_path, keyed_acme, druks_db, oauth_events, monkeypatch +): + from druks.testing import configure_app_for_test + + ServiceIdentity.connect( + keyed_acme.name, identity={"client_id": "id-1"}, secrets={"client_secret": "sec-1"} + ) + account = Account.get_or_create("op@example.com") + connection = OauthConnection.create( + provider=keyed_acme.name, + account_id=account.id, + refresh_token="rt-old", + scopes=["profile.read"], + identity={"sub": "account-1"}, + ) + connection_id = connection.id + evicted_connection_ids = [] + + async def record_eviction(oauth_client, evicted_connection_id): + assert oauth_client.provider == keyed_acme.name + evicted_connection_ids.append(evicted_connection_id) + + monkeypatch.setattr(OauthClient, "evict_access_token", record_eviction) + settings = make_settings(tmp_path, urls={"endpoint": "https://druks.example"}) + + with TestClient(configure_app_for_test(settings=settings)) as client: + _complete_oauth_sign_in(client, keyed_acme.name) + + db_session().expire_all() + reconnected = OauthConnection.get(connection_id) + assert reconnected + assert reconnected.refresh_token.decrypt() == "rt-1" + assert len(OauthConnection.list_for_provider(keyed_acme.name, include_revoked=True)) == 1 + assert evicted_connection_ids == [connection_id] + assert oauth_events[-1][1]["connection_id"] == connection_id + assert oauth_events[-1][1]["reconsent"] is True + + +def test_fresh_sign_in_with_live_and_revoked_identity_matches_lands_on_live_connection( + tmp_path, keyed_acme, druks_db, oauth_events +): + from druks.testing import configure_app_for_test + + ServiceIdentity.connect( + keyed_acme.name, identity={"client_id": "id-1"}, secrets={"client_secret": "sec-1"} + ) + account = Account.get_or_create("op@example.com") + live = OauthConnection.create( + provider=keyed_acme.name, + account_id=account.id, + refresh_token="rt-live-old", + scopes=["profile.read"], + identity={"sub": "account-1"}, + ) + live_id = live.id + revoked = OauthConnection.create( + provider=keyed_acme.name, + account_id=account.id, + refresh_token="rt-revoked-old", + scopes=["profile.read"], + identity={"sub": "account-1"}, + ) + revoked_id = revoked.id + revoked.revoke("user") + settings = make_settings(tmp_path, urls={"endpoint": "https://druks.example"}) + + with TestClient(configure_app_for_test(settings=settings)) as client: + _complete_oauth_sign_in(client, keyed_acme.name) + + db_session().expire_all() + reconnected = OauthConnection.get(live_id) + still_revoked = OauthConnection.get(revoked_id) + assert reconnected + assert still_revoked + assert reconnected.refresh_token.decrypt() == "rt-1" + assert still_revoked.revoked_at + assert not still_revoked.refresh_token + assert len(OauthConnection.list_for_provider(keyed_acme.name, include_revoked=True)) == 2 + assert oauth_events[-1][1]["connection_id"] == live_id + assert oauth_events[-1][1]["reconsent"] is True + + +def test_fresh_sign_in_without_the_declared_identity_fact_creates_a_new_connection( + tmp_path, acme, druks_db, oauth_events +): + from druks.testing import configure_app_for_test + + ServiceIdentity.connect( + acme.name, identity={"client_id": "id-1"}, secrets={"client_secret": "sec-1"} + ) + account = Account.get_or_create("op@example.com") + revoked = OauthConnection.create( + provider=acme.name, + account_id=account.id, + refresh_token="rt-old", + scopes=["profile.read"], + identity={"sub": "account-1"}, + ) + revoked_id = revoked.id + revoked.revoke("user") + settings = make_settings(tmp_path, urls={"endpoint": "https://druks.example"}) + + with TestClient(configure_app_for_test(settings=settings)) as client: + _complete_oauth_sign_in(client, acme.name) + + db_session().expire_all() + [created] = OauthConnection.list_for_provider(acme.name) + assert created.id != revoked_id + assert len(OauthConnection.list_for_provider(acme.name, include_revoked=True)) == 2 + assert OauthConnection.get(revoked_id).revoked_at + assert oauth_events[-1][1]["connection_id"] == created.id + assert oauth_events[-1][1]["reconsent"] is False + + def test_fresh_sign_in_after_revoke_creates_a_new_connection(tmp_path, acme, druks_db, monkeypatch): from urllib.parse import parse_qsl, urlparse @@ -755,8 +948,8 @@ def sign_in(): [first] = OauthConnection.list_for_provider("acme") assert client.delete(f"/api/oauth/connections/{first.id}").status_code == 204 - # A fresh sign-in never reuses a row, even for the same provider - # account. The revoked row stays behind as history. + # The identity facts do not include "sub", so the sign-in cannot + # match the existing row. The revoked row stays as history. sign_in() [live] = OauthConnection.list_for_provider("acme") assert live.id != first.id @@ -795,7 +988,7 @@ async def record(name, **kwargs): [connection] = OauthConnection.list_for_provider("acme") assert client.delete(f"/api/oauth/connections/{connection.id}").status_code == 204 - # Reconsent names the row, so it is the one way back to life. + # Reconsent names the row and makes the revoked consent live again. reconnect = client.get( f"/api/oauth/acme/connect?connection={connection.id}", follow_redirects=False ) diff --git a/docs/writing-an-extension.md b/docs/writing-an-extension.md index 13ac6e44..058be4c3 100644 --- a/docs/writing-an-extension.md +++ b/docs/writing-an-extension.md @@ -781,6 +781,13 @@ account's facts (email, username, name). Druks calls it once at consent and shows the facts as the connection's label in Settings. `identity_scopes` are the scopes that call needs; they join the consent ask. +Declare `identity_key` when one identity fact names the provider account: +`"sub"` for Google, `"id"` for GitHub. A fresh sign-in that matches an +existing connection for the same owner updates that row instead of +creating a second one. A revoked row that matches becomes live again and +keeps its id. Without the declaration, each fresh sign-in creates a new +connection. + Some providers have no such endpoint, or return the facts in a different shape. Override `get_identity` for them: @@ -805,6 +812,7 @@ class GoogleOauth(Service): extra_authorize_params = {"access_type": "offline", "prompt": "consent"} identity_endpoint = "https://openidconnect.googleapis.com/v1/userinfo" identity_scopes = ("openid", "email") + identity_key = "sub" class Settings(BaseModel): client_id: str = Field(title="Client ID") @@ -851,11 +859,13 @@ identity. Your rows never need tombstone copies of either. Your UI starts a sign-in by opening `/api/oauth/acme/connect` — the platform runs the consent with the union of every installed extension's declared scopes and stores the connection for the signed-in user. A fresh -sign-in always creates a new connection, even for a provider account that -was connected before. To widen an existing connection's scopes, open +sign-in creates a new connection, unless the service's `identity_key` +matches it to an existing connection for the same owner. To widen an +existing connection's scopes, open `/api/oauth/acme/connect?connection=`; reconsent replaces its tokens. -Reconsent names the row, so it also returns a revoked connection to life -under its old id — the only way a row comes back. +Reconsent names the row, so it also makes a revoked connection live again +under its old id. A fresh sign-in that matches the `identity_key` does the +same. Add `?next=/app/night_watch/accounts` to land the user back on your page after consent instead of the generic "connected" page. `next` must be a bare path starting with `/` — a URL with a scheme or host is @@ -864,12 +874,13 @@ rejected, so the door can never redirect off the box. Register serves every service. React to sign-ins with the signal machinery. The platform publishes -`oauth.connected` when a consent completes. `reconsent` is true when it -replaced an existing connection's tokens, including a revoked row that -returned to life. It publishes `oauth.disconnected` when a connection is -revoked — by the user, or because the service's client credentials were -replaced. Revocation is a state, not a deletion: your subscriber can still -read the connection it is told about. Subscribe in `subscribers.py`: +`oauth.connected` when a consent completes. `reconsent` is true when the +consent replaced an existing connection's tokens. This happens on +reconsent by id and on an `identity_key` match, for a live or a revoked +row. It publishes `oauth.disconnected` when a connection is revoked — +by the user, or because the service's client credentials were replaced. +Revocation is a state, not a deletion: your subscriber can still read the +connection it is told about. Subscribe in `subscribers.py`: ```python from druks.signals import subscribe