fix(security)!: enforce real scopes and tenancy (#898, P0.4)

[frankbria]

Closes #898 (P0.4 — critical / security, filed by the SaaS launch review).

The bug behind the bug

require_auth handed every JWT principal [read, write, admin] "for backward compatibility". That made the entire require_scope(SCOPE_ADMIN) layer — credential storage, GitHub PAT storage, PR merge — decorative for anything holding a browser session, and left users.is_superuser as dead data.

The obvious fix (derive admin from is_superuser) breaks the product on its own: fastapi_users.get_register_router hard-forces is_superuser=False on every registration, and the only row that ever carries 1 is the seeded !DISABLED! placeholder that can never log in. Derive naively and the instance has zero admin principals — Settings, PAT storage and PR merge become permanently 403 with no in-product way out.

So this PR is the derivation plus the two things that have to be true for it to ship.

Changes

1. JWT scopes come from the user row (auth/dependencies.py)
[read, write] always, admin only when is_superuser. Read via getattr(..., False) so an unexpected principal shape fails closed to non-admin rather than 500. The auth-disabled synthetic principal is deliberately untouched — it is the single-operator local opt-out and has no user record to read.

2. Somebody has to be admin

• UserManager.on_after_register promotes the account when it is the instance's sole login-capable user — exactly the bootstrap first user /auth/register admits (Enforce authentication across the v2 API (routers, WS/SSE, and web-UI login) #336/[P0.3] Gate bootstrap /auth/register behind an out-of-band secret #897). The count is re-checked rather than assumed, so if another registration path is ever added only a genuinely sole account is promoted.
• SchemaManager._ensure_bootstrap_superuser backfills admin to the earliest login-capable account when an upgraded instance has none. Idempotent. Without it, deploying this commit silently strips the operator's admin.

3. The escalation path that would have made #1 a formality (auth/api_key_router.py)
create_api_key refuses an admin-scoped key from a non-superuser. Not in the issue's criteria, but without it any signed-in user mints themselves an admin key and walks straight past the new derivation. A key can never grant more than its creator holds.

4. API-key router enforces scope by method (auth/api_key_router.py)
Router-level require_method_scope. GET needs read, POST/DELETE need write — a scopes:["read"] key can no longer DELETE its owner's write/admin keys.

5. Workspace ownership is write-once (workspace_registry_repository.py)
COALESCE argument order flipped, so an already-recorded owner_user_id is never reassigned: user B re-registering user A's repo_path no longer takes the row over. A NULL owner (left by an auth-disabled run) is still claimable on first attribution.

Acceptance criteria

Criterion	Where
JWT scopes derive from the user record; non-superuser JWT gets 403 on an admin endpoint	tests/auth/test_jwt_scope_derivation.py, tests/ui/test_v2_scope_enforcement.py::TestJwtIsNotAutomaticallyAdmin
API-key router enforces write on create/revoke; read-scope key gets 403 on DELETE	tests/auth/test_api_key_router_scopes.py
Registry upsert refuses to change owner_user_id; user B re-registering user A's path	tests/platform_store/test_workspace_registry_repository.py::TestOwnerScoping

Plus tests/auth/test_registration_bootstrap.py::TestBootstrapUserBecomesSuperuser and tests/platform_store/test_bootstrap_superuser_backfill.py for the two admin-provisioning paths.

Testing

• 377 passed, 0 failed on the blast radius: tests/auth, tests/platform_store, the v2 scope/auth/registry/settings/github/PR-merge/credential-isolation UI routers, test_api_key_service, test_api_key_commands.
• A further 1457 passed in a broad incidental run. Its single failure, test_config_reload_integration.py::test_full_reload_cycle, is a 0.1s-poll file-watcher deadline that flaked under an hour of fsync-saturated IO; it passes in isolation and touches none of the paths here (same class as [P1.34] Parallel-execution barrier timeout is load-sensitive — flakes under full-suite runs #976).
• ruff check clean; strict mypy clean on all changed modules.

BREAKING CHANGE

A non-superuser session no longer holds admin scope. No operator action is required — fresh installs promote the bootstrap account automatically, and existing installs get the backfill. But a second, non-superuser account now gets 403 on credential storage, PAT storage and PR merge.

Known limitations

• Granting admin to an additional account means setting is_superuser = 1 on its users row directly; there is no in-product promotion flow. Documented in deploy/README.md.

• The registry still lets a second user refresh name/tech_stack on a row they don't own — metadata, not ownership, and hosted-mode path confinement ([P7.0.1] Workspace path allowlist — prevent authenticated cross-tenant RCE (M1) #655) already blocks cross-tenant reach. Deliberately out of scope.

• API keys with admin scope minted by a non-superuser before this change keep working. Fixed in ef9359e after both reviewers flagged it: a key's scopes are now clamped to its owner's live grant on every request, so legacy admin keys and later-demoted owners are both covered without a migration.

• on_after_register's sole-user check is in-process; the [P0.3] Gate bootstrap /auth/register behind an out-of-band secret #897 registration lock carries the same documented multi-worker caveat.

• The bootstrap-superuser backfill runs on every Database.initialize(), so demoting the sole
  login-capable account does not stick. Deliberate — an instance with no reachable admin cannot store
  credentials or merge PRs and has no in-product way back. Demoting a non-earliest account still
  sticks. Documented in _ensure_bootstrap_superuser and pinned by tests.

[github-actions]

Claude finished @frankbria's task in 5m 38s —— View job

---

🔍 GLM bug-hunting review — complete

Reviewed PR #987 for concrete defects only (logic errors, security, data loss, races, breaking changes). Style, architecture, naming, and test coverage are out of scope (covered elsewhere).

• Gather PR context (diff, comments)
• Review auth/dependencies.py scope derivation
• Review auth/manager.py bootstrap promotion + schema_manager.py backfill
• Review auth/api_key_router.py scope-by-method + admin-key guard
• Review workspace_registry_repository.py COALESCE flip
• Cross-check callers/callees for each hunk
• Post summary

✅ GLM review: no defects found.

What was verified (each against read source, not the diff alone)

Hunk	Verdict
dependencies.py scope derivation	getattr(jwt_user, "is_superuser", False) fails closed on an unexpected principal; API-key-precedence and the auth-disabled all-scopes opt-out are correctly untouched.
manager.py _promote_if_bootstrap_user	getattr(self.user_db, "session", None) is sound — SQLAlchemyUserDatabase exposes .session; the count runs in the same flushed transaction as the user INSERT (count==1 holds for the first real user), so promotion persists atomically with the create. count != 1 guard correctly no-ops for a non-sole account.
schema_manager.py _ensure_bootstrap_superuser	SQL is correct across all cases: no real users → id = (SELECT MIN…) is NULL → no row matched (no crash, no spurious update); existing real superuser → NOT EXISTS short-circuits; idempotent on repeat create_schema(). Parameterizes the seeded insert.
api_key_router.py	Admin-key guard keys on current_user.is_superuser (freshly DB-loaded, not the derived dict). Router-level require_method_scope never blocks a JWT path (every session has read+write) and correctly 403s a read-scope key on POST/DELETE; create_api_key still mandates a JWT via get_current_user, so keys can't mint keys.
workspace_registry_repository.py COALESCE flip	COALESCE(existing, excluded) is the correct write-once semantics — a NULL owner is still claimable on first attribution, a set owner is never reassigned. Production caller _register_workspace passes the live principal id; no legitimate ownership-transfer flow is broken (hosted path confinement is the real tenant gate, as the PR notes).

No logic error, authz bypass, null path, race, or breaking-contract change met the failure-scenario bar.

[frankbria]

Third-party review round — 2 reviewers, 1 shared top finding

Both CodeRabbit and an opencode/GLM hostile-security pass independently landed on the same defect, which I had only listed as a known limitation. They were right to escalate it, so it's fixed in ef9359e.

The shared finding: pre-existing admin keys bypassed the whole fix

CodeRabbit (critical) — "identify persisted admin-scoped keys whose owners are not superusers"
opencode (H1) — "the API-key-side fix gates only new issuance, not the existing population"

Before #898, create_api_key had no is_superuser check, so any signed-in user could mint a scopes:["admin"] key. Gating creation leaves every one of those still working against require_scope(SCOPE_ADMIN) — credential storage, PAT storage, PR merge. The PR's own contract was unmet.

Both reviewers proposed a one-time migration to strip admin from those rows. I went a step further and clamped at resolution time (_scopes_within_owner_grant in get_api_key_auth) instead:

• no migration needed — it's correct on the first request after upgrade;
• it also covers an owner demoted later, which a one-time migration can never keep true;
• the key is clamped, not revoked, so it keeps read/write.

Demonstrated live below.

Also fixed

#	Finding	Fix
opencode M1	_promote_if_bootstrap_user was count-then-write; _register_lock is an asyncio.Lock, so two racing first registrations across uvicorn workers could each see two users and neither promote → zero admins	Single atomic UPDATE carrying both guards (sole login-capable account AND no login-capable superuser) — the DB arbitrates
opencode M2	'!DISABLED!' defined independently in two modules; silent drift makes fresh deploys unclaimable and strips an upgraded deploy's admin, with no error	One definition in platform_store (auth imports it — the reverse isn't DAG-legal), pinned by a test
opencode L1	create_api_key used direct attribute access where require_auth uses getattr	getattr(..., False) — fail closed to 403, never 500
CodeRabbit minor	upsert docstring still described ownership as mutable	Rewritten to state write-once
CodeRabbit minor	ponytail: marker sat in a docstring	Moved to a code comment, matching the existing convention in ui/dependencies.py

opencode reported no CRITICAL findings, and confirmed "the JWT-side escalation (the headline defect) is correctly closed."

One behavioral property I found while demoing, now documented and pinned

The backfill runs on every Database.initialize(), so demoting the sole login-capable account doesn't stick. That's deliberate — an instance with no reachable admin can't store credentials or merge PRs and has no in-product way back — and demoting a non-earliest account still sticks, which is the case that matters for revoking access. Both are now tests, and it's called out in the _ensure_bootstrap_superuser docstring.

Demo — live app, real HTTP, real SQLite, 17/17

CRITERION 1  JWT scopes derive from the user record
  ✓ PUT /api/v2/settings/keys/openai  (admin-scoped: credential storage)   403
  ✓ POST /api/v2/pr/1/merge          (admin-scoped)                        403
  ✓ GET  /api/v2/settings            (read — passes the scope gate)
  ✓ POST /api/v2/pr/1/merge          (owner promoted → now admits admin)
CRITERION 2  API-key router enforces write scope
  ✓ DELETE /api/auth/api-keys/<write-key>  with a read-scope key           403
  ✓   …and the target key survived (is_active)
  ✓ DELETE /api/auth/api-keys/<read-key>   with a write-scope key          200
  ✓ POST /api/auth/api-keys  scopes=[admin] as non-superuser               403
  ✓ POST /api/auth/api-keys  scopes=[read,write] as non-superuser          201
  ✓ POST admin endpoint with a pre-existing admin key (owner NOT superuser) 403
  ✓   …same key still works for read (clamped, not revoked)                200
  ✓   …owner promoted → same key now admits admin
CRITERION 3  Workspace registry ownership is write-once
  ✓ owner_user_id after B's takeover attempt                               1
  ✓ B's owner-scoped listing is empty                                      []
  ✓ an ownerless row is still claimable (owner NULL → 2)                   2
BONUS  upgrade backfill — an existing install keeps its admin
  ✓ earliest login-capable account (id=1) promoted on restart
  ✓ the later account (id=2) NOT promoted
17/17 checks passed

Test-fixture changes — premise, not strength

Two fixtures assumed an admin-scoped key can belong to a non-superuser, which is exactly what this PR outlaws. test_v2_scope_enforcement now mints its admin key for a new superuser (user 2; user 1 stays non-superuser for the JWT tests) and test_stream_ticket_endpoint promotes its user. Both still assert precisely what they asserted before.

Verification

ruff clean, strict mypy clean. Locally: 1421 passed in a broad run whose only failure was test_parallel_respects_max_parallel — the load-sensitive parallel barrier of #976, which passes in isolation (verified) after 71 minutes of fsync-saturated IO. An earlier run's lone failure was test_config_reload_integration::test_full_reload_cycle, a 0.1s-poll file-watcher deadline of the same class, also verified passing in isolation. Neither touches the auth, registry or schema paths in this PR.

[github-actions]

Claude finished @frankbria's task in 6m 48s —— View job

---

🔍 GLM bug-hunting review — complete

Reviewed PR #987 for concrete defects only (logic errors, security, data loss, races, breaking changes). Style, architecture, naming, and test coverage are out of scope (covered elsewhere). This pass covers the latest commit (ef9359e) — i.e. the resolution-time clamp, the atomic-UPDATE promotion, the single DISABLED_PASSWORD definition, and the getattr admin guard — not just the original diff.

• Gather PR context (diff, comments)
• Review auth/dependencies.py scope derivation + _scopes_within_owner_grant clamp
• Review auth/manager.py atomic-UPDATE promotion + schema_manager.py backfill
• Review auth/api_key_router.py scope-by-method + admin-key guard
• Review workspace_registry_repository.py COALESCE flip
• Cross-check callers/callees for each hunk
• Post summary

✅ GLM review: no defects found.

What was verified (each against read source, not the diff alone)

Hunk	Verdict
dependencies.py JWT scope derivation	getattr(jwt_user, "is_superuser", False) fails closed on an unexpected principal; API-key-precedence and the auth-disabled all-scopes opt-out are correctly untouched.
dependencies.py _scopes_within_owner_grant (new in ef9359e)	key_record["scopes"] is JSON-decoded to a list in APIKeyRepository._row_to_api_key, so SCOPE_ADMIN not in scopes is a real membership test (not a string substring). db.conn is the sync sqlite3.Connection (confirmed in Database.__init__). Bare except fails closed to drop-admin (never grant). key_record["user_id"] is always set (keys are minted only via create_api_key → get_current_user, a JWT), and a NULL would still fail closed.
api_key_router.py	Router-level require_method_scope never blocks a JWT path (sessions always carry read+write) and never blocks an admin-only key — has_scope is hierarchical (admin→write→read, auth/scopes.py), so ["admin"] satisfies the write check. The admin-key guard keys on current_user.is_superuser freshly loaded via get_current_user; create_api_key still mandates a JWT, so keys can't mint keys.
manager.py atomic-UPDATE promotion	getattr(self.user_db, "session", None) is the request's AsyncSession (fastapi-users SQLAlchemyUserDatabase.session). SQLite permits subqueries on the UPDATE target table (confirmed by the PR's live 17/17 demo, which exercises this exact statement), so both guards resolve in one statement; the COUNT excludes the seeded id=1 via hashed_password != DISABLED_PASSWORD, and the same-transaction flush means the new row is visible to the COUNT. result.rowcount gates the refresh, so non-bootstrap registrations no-op cleanly. The CLI cf auth register path POSTs to the web /auth/register route (auth_commands.py:409/416), so it routes through on_after_register too — no promotion gap on the CLI bootstrap path.
schema_manager.py backfill	SQL is correct across all cases (no real users → MIN(id) is NULL → no row matched; existing real superuser → NOT EXISTS short-circuits; idempotent on repeat create_schema()). _DISABLED_PASSWORD is a back-compat alias of the single DISABLED_PASSWORD source — no second literal to drift (manager.py and router.py both import it). schema_manager.py imports only sqlite3/logging, so the new auth.manager → platform_store.schema_manager edge adds no import cycle.
workspace_registry_repository.py COALESCE flip	COALESCE(workspaces_registry.owner_user_id, excluded.owner_user_id) is the correct write-once semantics — a NULL owner is still claimable on first attribution, a set owner is never reassigned. The INSERT side still passes the live principal id, so no legitimate ownership-transfer flow regresses (hosted path confinement #655 remains the real tenant gate).

No logic error, authz bypass, null path, race, or breaking-contract change met the failure-scenario bar.