fix(local): revoke the grant a ChatGPT sign-in replaces

docs/architecture/chatgpt-subscription.md

@@ -15,7 +15,7 @@ Every URL is in one file, [`endpoints.py`](../../surfsense_local/backend/modules
 |---|---|
 | Authorize | `https://auth.openai.com/api/accounts/authorize` |
 | Token exchange and refresh | `https://auth.openai.com/api/accounts/oauth/token` |
-| Revocation, on sign-out and delete | `POST https://auth.openai.com/api/accounts/oauth/revoke` |
+| Revocation, on sign-out, delete and signing in again | `POST https://auth.openai.com/api/accounts/oauth/revoke` |
 | ID-token keys | `https://auth.openai.com/.well-known/jwks.json` |
 | Plan's models | `GET https://api.openai.com/v1/models` |
 | Answers | `POST https://api.openai.com/v1/responses` |
@@ -27,7 +27,7 @@ Every URL is in one file, [`endpoints.py`](../../surfsense_local/backend/modules
 3. The authorize URL asks for `client_id=dynamic_agent_client`, so OpenAI issues a client for this user on first sign-in, or, for a connection that still holds a sign-in, its issued client with `id_token_hint`, OpenAI's returning-user path. It goes with `agent_name_hint=SurfSense`, an `ext_agent_host_id`, scopes `openid profile email offline_access resource.invoke chatgpt.tokens.use.direct`, `resource=https://api.openai.com/v1`, `state`, `nonce` and an S256 PKCE challenge. The host id is `urn:uuid:` over an HMAC of the install secret, so it is stable for the install and stored nowhere ([`host_id.py`](../../surfsense_local/backend/modules/llm/subscriptions/chatgpt/host_id.py)).
 4. The renderer opens it through Electron, which allows `auth.openai.com/api/accounts/authorize` only with a `redirect_uri` of `http://127.0.0.1:<port>/callback`, so a crafted link cannot send the code anywhere else.
 5. The callback must carry the flow's `state`, and a renewal must name the connection's own client: one naming another fails the flow and leaves the connection as it was. The code is exchanged with the issued `client_id` and the PKCE verifier, and the result is refused unless it grants `chatgpt.tokens.use.direct` and carries a refresh token. The ID token's RS256 signature is checked against the JWKS, then its issuer, audience, expiry (two minutes of leeway) and nonce. Its `sub` and `email` are kept.
-6. The tokens are saved on a new connection, or on the one being signed in again, and the flow reads `signed_in`. The renderer polls `GET /llm/connections/chatgpt/sign-in/{flow_id}` every second. A flow not finished in five minutes fails and frees its port. `DELETE` on the flow cancels it.
+6. The tokens are saved on a new connection, or on the one being signed in again, and the flow reads `signed_in`; a revocation of tokens it replaced, when there is one, runs after that. The renderer polls `GET /llm/connections/chatgpt/sign-in/{flow_id}` every second. A flow not finished in five minutes fails and frees its port. `DELETE` on the flow cancels it.
 
 Flows live in the API's memory ([`flows.py`](../../surfsense_local/backend/modules/llm/subscriptions/chatgpt/flows.py)): only the API signs in, and a restart only abandons a sign-in in progress.
 
@@ -39,7 +39,8 @@ A ChatGPT connection is a `provider_connections` row with `auth_kind = 'chatgpt'
 - `GET /llm/connections` adds `auth_kind`, `signed_in` and `account_email`; no token leaves the API.
 - `PUT` on a ChatGPT connection renames it and changes nothing else.
 - `DELETE /llm/connections/{id}/sign-in` signs out: the tokens go, the connection and its selection stay. Signing in again registers a new client, since the issued one went with the tokens.
-- Signing out, or deleting the connection, also revokes the refresh token at the issuer's `revocation_endpoint` (`/api/accounts/oauth/revoke`, RFC 7009, as a public client) once the local change has committed ([`revocation.py`](../../surfsense_local/backend/modules/llm/subscriptions/chatgpt/revocation.py)). It is best effort: a failure is logged and the sign-out stands. It is skipped when the sign-in host has been turned off since, and when this install can no longer decrypt the tokens (a keychain reset or a backup restored elsewhere), so a lost key never blocks the sign-out or delete that recovers from it.
+- Signing out or deleting the connection also revokes the refresh token at the issuer's `revocation_endpoint` (`/api/accounts/oauth/revoke`, RFC 7009, as a public client) once the local change has committed ([`revocation.py`](../../surfsense_local/backend/modules/llm/subscriptions/chatgpt/revocation.py)). It is best effort: a failure is logged without the token, and the sign-out or delete stands. It is skipped when the sign-in host has been turned off since, and when this install can no longer decrypt the tokens (a keychain reset or a backup restored elsewhere), so a lost key never blocks the sign-out or delete that recovers from it.
+- Signing in again revokes the tokens it replaces only when they belong to another client, after the new ones are saved and the flow reads `signed_in`, under the same guards. The returning path keeps the client, and RFC 7009 lets a server end the whole grant when one of its refresh tokens is revoked, which would take the new session with it.
 - It serves `text_gen` only. What a connection serves is one rule, [`serves.py`](../../surfsense_local/backend/modules/llm/connections/serves.py), keyed by `auth_kind`: selection refuses any other slot even with `allow_unlisted`, the image and speech tests answer `422`, image and speech resolution refuse it, and `GET /llm/connections` reports it as `serves`.
 
 ## Tokens
@@ -84,7 +85,7 @@ Every model list reads `serves` rather than deciding: the Settings sections, the
 
 ## Known gaps
 
-- **Sign in again** on a connection still signed in overwrites its tokens through `save_sign_in()` without revoking the grant they replace.
+- Signing in again as the same client leaves the replaced refresh token to expire on its own: whether revoking it would end OpenAI's whole grant is unknown.
 - A plan's refusal inside the stream (`response.failed`) reaches the agent's screen as `unknown`, not as `subscription_limit` or `subscription_sign_in`.
 - Built against OpenAI's documentation and a fake server; not yet run against a real ChatGPT account.
 - Signing in again after signing out registers a new client, not the returning-user path, because signing out drops the client id with the tokens.

surfsense_local/backend/modules/llm/subscriptions/chatgpt/flows.py

@@ -9,7 +9,7 @@
 import httpx
 from sqlalchemy.orm import Session
 
-from modules.llm.subscriptions.chatgpt import account
+from modules.llm.subscriptions.chatgpt import account, revocation
 from modules.llm.subscriptions.chatgpt.endpoints import (
     AGENT_NAME,
     DYNAMIC_CLIENT,
@@ -122,10 +122,13 @@ async def _finish(
             async with asyncio.timeout(SIGN_IN_SECONDS):
                 query = await loopback.callback()
             tokens = await _tokens(query, pkce, redirect_uri, returning)
-            flow.connection_id = await asyncio.to_thread(
+            flow.connection_id, stale = await asyncio.to_thread(
                 _save, session_factory, tokens, label, connection_id
             )
             flow.status = "signed_in"
+            # After signed_in: the saved sign-in stands whatever OpenAI answers.
+            if stale is not None:
+                await asyncio.to_thread(revocation.revoke, stale)
         except TimeoutError:
             _fail(flow, "The sign-in was not finished in time.")
         except (
@@ -210,13 +213,31 @@ def _save(
     tokens: TokenSet,
     label: str | None,
     connection_id: int | None,
-) -> int:
+) -> tuple[int, TokenSet | None]:
+    """The connection's id, and the replaced tokens when they are another
+    client's grant to end once this commits."""
     with session_factory() as session:
+        replaced = (
+            None
+            if connection_id is None
+            else revocation.tokens_to_revoke(
+                account.chatgpt_connection(session, connection_id)
+            )
+        )
         saved = account.save_sign_in(
             session, tokens, label=label, connection_id=connection_id
         )
+        # The returning path keeps the client, and revoking its old refresh
+        # token may end the whole grant, the new session's with it (RFC 7009 §2.1).
+        stale = (
+            replaced
+            if replaced is not None
+            and replaced.client_id != tokens.client_id
+            and revocation.may_revoke(session)
+            else None
+        )
         session.commit()
-        return saved
+    return saved, stale
 
 
 def _fail(flow: Flow, message: str) -> None:

surfsense_local/backend/tests/integration/llm/chatgpt/test_sign_in_routes.py

@@ -6,6 +6,7 @@
 """
 
 import asyncio
+import time
 from urllib.parse import parse_qs, urlsplit
 
 import httpx
@@ -16,7 +17,10 @@
 from modules.llm.models import ProviderConnection
 from modules.llm.providers.types import Message
 from modules.llm.resolution import resolve_generation
+from modules.llm.subscriptions.chatgpt import revocation
 from modules.llm.subscriptions.chatgpt.endpoints import get_endpoints
+from modules.llm.subscriptions.chatgpt.token_set import TokenSet
+from modules.llm.subscriptions.chatgpt.tokens import read_tokens, write_tokens
 from shared.db import create_session_factory
 
 from .fake_openai import EMAIL, ISSUED_CLIENT, FakeOpenAI
@@ -295,6 +299,149 @@ async def test_a_sign_in_host_turned_off_is_not_called_to_sign_out(
     assert "Could not revoke" not in caplog.text
 
 
+async def test_signing_in_again_as_the_same_client_revokes_nothing(
+    client: AsyncClient, fake_openai: FakeOpenAI
+) -> None:
+    """The returning path reuses the client, and revoking its old token may end the new grant."""
+    connection_id = (await _sign_in(client, label="ChatGPT"))["connection_id"]
+
+    again = await _sign_in(client, connection_id=connection_id)
+    await asyncio.sleep(0.3)
+
+    assert again["status"] == "signed_in"
+    assert fake_openai.revocations == []
+
+
+async def test_signing_in_again_over_tokens_that_cannot_be_decrypted_settles(
+    client: AsyncClient, engine: Engine, fake_openai: FakeOpenAI
+) -> None:
+    """A lost key must not keep the sign-in that recovers from it from finishing."""
+    connection_id = (await _sign_in(client, label="ChatGPT"))["connection_id"]
+    _scramble_tokens(engine, connection_id)
+
+    again = await _sign_in(client, connection_id=connection_id)
+    await asyncio.sleep(0.3)
+
+    assert again["status"] == "signed_in"
+    assert fake_openai.revocations == []
+
+
+def _store_old_client(engine: Engine, connection_id: int) -> None:
+    """Another client's live tokens, as left by a registration this sign-in replaces."""
+    with create_session_factory(engine)() as session:
+        write_tokens(
+            session.get(ProviderConnection, connection_id),
+            TokenSet(
+                client_id="app_old",
+                access_token="at-old",
+                refresh_token="rt-old",
+                id_token="id-old",
+                expires_at=time.time() + 3600,
+                account="user-1",
+            ),
+        )
+        session.commit()
+
+
+async def _sign_in_over_another_client(
+    client: AsyncClient, engine: Engine, connection_id: int
+) -> dict:
+    """Started over unreadable tokens, so it registers anew; another client's
+    tokens land before the browser comes back."""
+    _scramble_tokens(engine, connection_id)
+    started = await client.post(
+        "/llm/connections/chatgpt/sign-in", json={"connection_id": connection_id}
+    )
+    _store_old_client(engine, connection_id)
+    await _browse(started.json()["authorize_url"])
+    return await _settled(client, started.json()["flow_id"])
+
+
+async def _revoked(fake_openai: FakeOpenAI) -> list[str]:
+    """The revoke runs after the sign-in reads signed_in; wait for it."""
+    for _ in range(60):
+        if fake_openai.revocations:
+            break
+        await asyncio.sleep(0.05)
+    return [form["token"] for form in fake_openai.revocations]
+
+
+def _stored_refresh(engine: Engine, connection_id: int) -> str:
+    with create_session_factory(engine)() as session:
+        return read_tokens(session.get(ProviderConnection, connection_id)).refresh_token
+
+
+async def test_a_sign_in_that_replaces_another_clients_tokens_revokes_them(
+    client: AsyncClient, engine: Engine, fake_openai: FakeOpenAI
+) -> None:
+    """A different client's grant is a different grant, so ending it is safe."""
+    connection_id = (await _sign_in(client, label="ChatGPT"))["connection_id"]
+
+    again = await _sign_in_over_another_client(client, engine, connection_id)
+
+    assert again["status"] == "signed_in"
+    assert await _revoked(fake_openai) == ["rt-old"]
+    assert fake_openai.revocations[0]["client_id"] == "app_old"
+
+
+async def test_a_failed_revocation_keeps_the_new_sign_in(
+    client: AsyncClient,
+    engine: Engine,
+    fake_openai: FakeOpenAI,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """OpenAI being down costs the old grant's end, never the new tokens."""
+    connection_id = (await _sign_in(client, label="ChatGPT"))["connection_id"]
+    fake_openai.revocation_down = True
+
+    again = await _sign_in_over_another_client(client, engine, connection_id)
+    for _ in range(60):
+        if "Could not revoke" in caplog.text:
+            break
+        await asyncio.sleep(0.05)
+
+    assert again["status"] == "signed_in"
+    assert _stored_refresh(engine, connection_id) == fake_openai.issued_refresh[-1]
+    assert "Could not revoke" in caplog.text
+    assert "rt-old" not in caplog.text
+
+
+async def test_the_old_token_is_revoked_only_once_the_new_one_is_stored(
+    client: AsyncClient,
+    engine: Engine,
+    fake_openai: FakeOpenAI,
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """Revoking first would leave a failed save with neither grant."""
+    connection_id = (await _sign_in(client, label="ChatGPT"))["connection_id"]
+    seen: list[str] = []
+    monkeypatch.setattr(
+        revocation,
+        "revoke",
+        lambda tokens: seen.append(_stored_refresh(engine, connection_id)),
+    )
+
+    await _sign_in_over_another_client(client, engine, connection_id)
+    for _ in range(60):
+        if seen:
+            break
+        await asyncio.sleep(0.05)
+
+    assert seen == [fake_openai.issued_refresh[-1]]
+
+
+async def test_signing_in_again_after_signing_out_revokes_nothing_more(
+    client: AsyncClient, fake_openai: FakeOpenAI
+) -> None:
+    """The sign-out already ended that grant; there is nothing left to replace."""
+    connection_id = (await _sign_in(client, label="ChatGPT"))["connection_id"]
+    await client.delete(f"/llm/connections/{connection_id}/sign-in")
+
+    await _sign_in(client, connection_id=connection_id)
+
+    assert len(fake_openai.revocations) == 1
+
+
 async def test_a_label_already_taken_is_refused_before_the_browser_opens(
     client: AsyncClient, fake_openai: FakeOpenAI
 ) -> None: