Files
pad/internal/models/connected_apps.go
T
xarmian 905baaa010 feat(oauth): backfill session.Extra into oauth_connections + switch read path (TASK-1522) (#583)
* feat(oauth): backfill session.Extra into oauth_connections + switch read path (TASK-1522)

Phase C1 for PLAN-1519. Seeds existing OAuth grant chains into the new
connection tables (Phase A) and switches /console/connected-apps to
read from them, retiring the session.Extra parse on the read path.

Backfill (internal/store/oauth_connections_backfill.go)
- Walks oauth_access_tokens + oauth_refresh_tokens to find every
  distinct request_id chain (including refresh-only chains).
- Picks the newest token row per chain — its session.Extra drives
  the seeded shape, so a chain whose user re-scoped recently
  reflects the latest decision.
- Maps session.Extra shapes to the new tables per IDEA-1517 §2:
  no key → all_current=1; ["*"] → all_current=1; explicit slugs →
  all_current=0 + one join row per slug (added_by='user').
- Resolves slugs → workspace IDs; unresolved slugs (deleted /
  renamed workspace) are counted + logged at WARN, not fatal.
- Idempotent on every INSERT (OR IGNORE / ON CONFLICT DO NOTHING)
  so re-running on every startup is a cheap no-op once stable.
- Returns a BackfillOAuthConnectionsResult so the startup log
  reports chains_seen / connections_created / workspaces_added /
  unresolved_slugs — operators see fresh work and notice drift.

Read-path rewrite (internal/store/connected_apps.go)
- ListUserOAuthConnections projects AllowedWorkspaces from
  GetOAuthConnectionAccess (oauth_connection_workspaces JOIN
  workspaces) instead of parsing session.Extra strings.
- Hydrates Name + MayCreate + AllCurrent + IncludeFuture from
  oauth_connections so Phase D's mutation UI has them.
- Defensive fallback for chains without an oauth_connections row
  (any leftover the backfill missed): treats as legacy
  "any workspace, default-on flags" so the connection still
  renders. Backfill at startup keeps this branch unreachable in
  production.
- Retires parseAllowedWorkspacesFromSession; the new
  extractAllowedWorkspacesFromSessionExtra helper in
  oauth_connections_backfill.go is the only consumer of the
  session.Extra shape on the store side.

Model (internal/models/connected_apps.go)
- Adds Name / MayCreateWorkspaces / AllCurrentWorkspaces /
  IncludeFutureWorkspaces. AllowedWorkspaces semantics stay
  stable (nil = "any"; explicit slugs = chip list) so the
  existing DTO + frontend continue working unchanged. Phase D
  exposes the new fields on the wire.

Startup wiring (cmd/pad/main.go)
- After srv.SetOAuthServer / SetClaimSecret, run the backfill
  once. Non-fatal on error (partial state is consistent and the
  next run completes). Quiet at the Debug level on steady-state
  re-runs; INFO when fresh work landed.

Tests
- 8 BackfillOAuthConnections cases: empty DB, pre-TASK-952
  (no key), wildcard, explicit list, mixed resolvable/unresolved
  slugs, multi-row chain newest-row-wins, refresh-only chain,
  idempotent re-run (verified via post-run row count).
- TestExtractAllowedWorkspacesFromSessionExtra replaces the
  retired parseAllowedWorkspacesFromSession test — covers all
  three IDEA-1517 §2 input shapes + malformed/non-array
  defensive cases.
- TestListUserOAuthConnections_DeduplicatesChain +
  TestHandleListConnectedApps_DTOShapeAndAuditEnrichment updated
  to call BackfillOAuthConnections (the production startup
  hook) before asserting on AllowedWorkspaces — mirrors the
  real-world flow now that the read path no longer parses
  session.Extra inline.

Parent: PLAN-1519.

* fix(oauth): backfill counters reflect actual new rows per Codex review (round 1)

PR #583 Codex review round 1 flagged that the backfill counters
over-report on steady-state restarts:

- wasFreshlyInserted compared updated_at vs created_at — true for
  every untouched existing row, so every restart counted every
  pre-existing connection as "created."
- slugsAdded++ ran after AddConnectionWorkspace regardless of
  whether the INSERT OR IGNORE / ON CONFLICT DO NOTHING hit an
  existing row.

Net effect: startup logs "backfill complete" with non-zero counts
on every restart instead of the intended quiet "no-op" path —
making real fresh work indistinguishable from steady-state.

Fix: probe existence BEFORE the insert on both sides.

- backfillOneChain reads GetOAuthConnection first; only sets
  created=true and runs insertOAuthConnectionIfAbsent on a miss.
- Per-slug: IsConnectionWorkspaceAllowed pre-check; skip + don't
  increment when the row already exists.

Two cheap PK / indexed lookups per chain. Pre-Phase-C deployments
have small chain counts so the added cost is well below the scan
already running.

Removed the now-unused wasFreshlyInserted helper. Added an
assertion in TestBackfillOAuthConnections_Idempotent that both
ConnectionsCreated and WorkspacesAdded report 0 on the second
run — the regression guard for this exact finding.

Parent: PLAN-1519.

* fix(oauth): backfill skips slug re-seed on existing rows per Codex review (round 2)

PR #583 round 2 caught that the round-1 fix protected the parent
oauth_connections row from re-seed but left the join table
mutable from stale session.Extra:

When a user removes a workspace from their connection's allow-list
via Phase D's mutation UI (RemoveConnectionWorkspace), the next
server restart would re-run the backfill, find the parent row
intact, and re-INSERT the removed slug from the original
session.Extra. The user's removal would silently revert every
restart.

Fix: backfill is a one-shot seed. Once the parent row exists, the
new tables are authoritative — legacy session.Extra is frozen
reference data, not a reconciliation source. The slug loop only
runs when we just inserted a fresh parent row.

Added TestBackfillOAuthConnections_DoesNotResurrectRemovedWorkspace
as the regression guard: seeds two slugs, removes one, runs
backfill again, asserts the removed slug stays gone and the kept
slug is untouched.

Parent: PLAN-1519.

* fix(oauth): atomic per-chain backfill transaction per Codex review (round 3)

PR #583 round 3 caught that round 2's "only seed slugs on fresh
parent" gate introduced a permanent-partial-state risk: if the
process crashes (or AddConnectionWorkspace errors) between
inserting the parent row and finishing the slug loop, the next
backfill sees created=false, short-circuits the slug seeding, and
leaves the connection permanently scoped to a partial allow-list.

Fix: per-chain transaction. Parent insert + every slug insert
land in one BEGIN/COMMIT pair; any mid-loop failure rolls
everything back. The next backfill then sees the chain as un-seeded
and retries from scratch — preserving both round 2's
"no-resurrection of user-removed slugs" (existence probe inside
the tx) and round 3's "no permanent partial seed" (atomic commit).

Scope: per-chain (small tx), not whole-backfill. The original
no-transaction rationale was about lock-hold duration across
thousands of chains; that doesn't apply at chain granularity (one
parent + a handful of join rows = sub-millisecond hold).

Removed the now-unused insertOAuthConnectionIfAbsent helper; the
INSERTs live inline within the transaction.

Added TestBackfillOAuthConnections_AtomicOnMidLoopFailure as the
regression guard: forces a mid-loop INSERT failure via a duplicate
slug in session.Extra (which violates the join table's PK on the
second insert), asserts the parent row rolled back, then runs a
clean retry and verifies full seed completion.

Parent: PLAN-1519.

* fix(oauth): surface store errors from backfill + list path per Codex review (round 4)

PR #583 round 4 caught two silent-fallthrough paths that could
leak partial/incorrect state instead of failing loudly:

1. Backfill slug loop: GetWorkspaceBySlug errors were treated the
   same as "workspace not found" — both incremented slugsMissed
   and continued. A real I/O error mid-loop would commit a
   partial allow-list, and the next backfill's parent-exists
   short-circuit would make that partial scope permanent.
   Fix: distinguish (nil, nil) "not found" from (nil, err)
   "real failure" — return the error so the per-chain
   transaction rolls back and the next run retries cleanly.

2. ListUserOAuthConnections hydration: GetOAuthConnectionAccess
   and GetOAuthConnection errors collapsed into the "no
   oauth_connections row" defensive-fallback branch, returning
   the legacy "any workspace, default-on flags" shape. On a
   real store failure that silently broadens a user's scope —
   e.g. a connection the user explicitly removed a slug from
   would render as "Any workspace" until the store recovered.
   Fix: surface store errors from both calls; the defensive
   fallback path is now exclusively for HasConnection=false,
   not for error masking.

Both findings tighten the failure mode from "silently emit
broadened/partial state" to "surface the error so retries
happen against accurate data." Existing tests cover the happy
paths; the failure paths are exercised by I/O errors against
the same store interfaces (no new test added — the change is
"return err instead of swallow it" and the assertion of NOT
swallowing is the diff itself).

Parent: PLAN-1519.
2026-05-18 03:32:42 -04:00

102 lines
4.3 KiB
Go

package models
import "time"
// Connected-apps models (PLAN-943 TASK-954).
//
// A "connected app" is one OAuth client (Claude Desktop, Cursor, …)
// that the user has authorized to act on their behalf via the MCP
// surface. Each user-authorized OAuth flow produces one connection,
// identified by the OAuth grant chain's request_id (preserved across
// refresh-token rotations — see internal/store/oauth.go for the
// chain semantics). Revoking a connection invalidates every token in
// that chain so the next /mcp call from that client gets 401.
// CapabilityTier classifies the granted scopes into a coarse,
// user-readable bucket. Used by the connected-apps page so users
// don't have to read raw OAuth scope strings to understand what a
// given client can do.
type CapabilityTier string
const (
CapabilityTierReadOnly CapabilityTier = "read_only"
CapabilityTierReadWrite CapabilityTier = "read_write"
CapabilityTierFullAccess CapabilityTier = "full_access" // includes pad:admin
CapabilityTierUnknown CapabilityTier = "unknown" // empty / unparseable scopes
)
// OAuthConnection is one row of the connected-apps page. Joins
// oauth_access_tokens (the active grant) → oauth_clients (the DCR
// metadata). The audit-log enrichments (LastUsedAt + Calls30d) come
// from a separate aggregate query against mcp_audit_log so the page
// loads in two queries instead of N+1.
//
// One value per OAuth grant chain — the connections list deduplicates
// across refresh-token rotations because every chain member shares
// the same RequestID.
type OAuthConnection struct {
// RequestID is the OAuth grant chain identifier — preserved
// across refresh-token rotations and used as the public ID for
// revoke + audit drilldown ("connection_id" on the wire).
RequestID string
// ClientID / ClientName / LogoURL / RedirectURIs come from the
// DCR registration metadata (oauth_clients table). ClientName is
// what shows on the consent screen + connected-apps cards
// (e.g. "Claude Desktop").
ClientID string
ClientName string
LogoURL string
RedirectURIs []string
// AllowedWorkspaces is the workspace allow-list. Post-TASK-1522
// it's the projection of oauth_connection_workspaces rows joined
// to workspaces by ID (slug list, sorted). Two shapes the UI
// renders against:
// - nil — the connection's all_current_workspaces flag is on
// (covers every workspace the user is a member of). UI shows
// an "Any workspace" badge.
// - explicit slugs — all_current_workspaces=false; UI shows the
// slug list as chips.
// (Pre-TASK-1522 the same field also held ["*"] for wildcard
// tokens; the backfill normalizes those to the flag and the field
// is nil for them now. The wire DTO's nullable shape is unchanged
// so frontend code keeps working.)
AllowedWorkspaces []string
// Connection-level scope flags + name from oauth_connections
// (PLAN-1519 / TASK-1520 / IDEA-1517 §2). Pre-TASK-1522 these
// were unsourced (the field didn't exist); post-backfill every
// active connection has a row and the values mean what the page
// should render. Phase D's mutation UI reads + writes these.
//
// Name is empty string for backfilled rows that haven't been
// renamed yet — UI prompts on first connections-page visit.
Name string
MayCreateWorkspaces bool
AllCurrentWorkspaces bool
IncludeFutureWorkspaces bool
// GrantedScopes is the raw space-separated scope string fosite
// recorded at grant time. Surfaced in the per-connection expand
// drilldown alongside CapabilityTier so power users can see the
// exact policy.
GrantedScopes string
// CapabilityTier is the human-readable bucket derived from
// GrantedScopes (read_only / read_write / full_access / unknown).
CapabilityTier CapabilityTier
// ConnectedAt is the earliest requested_at across the chain —
// when the user originally authorized this connection. Survives
// refresh-token rotation because every chain member shares the
// same request_id and the MIN over the chain stays anchored.
ConnectedAt time.Time
// LastUsedAt + Calls30d come from MCPConnectionStatsForUser
// (TASK-960's audit-log aggregate). Nil LastUsedAt means "no
// audit entries yet" — the page renders "—" instead of a date.
LastUsedAt *time.Time
Calls30d int
}