From 4de120f13ed37209defdf3e56844c24455f20782 Mon Sep 17 00:00:00 2001 From: frankbria Date: Tue, 28 Jul 2026 23:01:54 -0700 Subject: [PATCH 1/2] fix(security)!: derive JWT scopes from the user record and pin workspace ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #898 (P0.4). require_auth handed every JWT principal [read, write, admin] "for backward compatibility", so require_scope(SCOPE_ADMIN) — credential storage, GitHub PAT storage, PR merge — was decorative for anything holding a browser session, and users.is_superuser was never read. JWT scopes now come from the user row: read+write always, admin only for is_superuser (via getattr, so an unexpected principal shape fails closed to non-admin rather than 500). That alone would leave the product with no admin at all: fastapi-users forces is_superuser=False on registration, so nothing ever set it. Two paired fixes: - 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 (#336/#897). The count is re-checked, not assumed. - SchemaManager._ensure_bootstrap_superuser backfills admin to the earliest login-capable account when an upgraded instance has none, so this change cannot silently strip the operator's admin. Idempotent. Also closed, since they bypass or mirror the same gate: - create_api_key refuses an admin-scoped key from a non-superuser. Without it any signed-in user could mint themselves an admin key and escalate straight past the new derivation. - The API-key router mounts require_method_scope, so a scopes:["read"] key can no longer DELETE its owner's write/admin keys. - workspaces_registry.upsert makes ownership write-once (COALESCE order flipped), so user B re-registering user A's repo_path no longer takes the row over. A NULL owner is still claimable on first attribution. BREAKING CHANGE: a non-superuser session no longer holds admin scope. On a fresh install the bootstrap account is promoted automatically and on an existing one the backfill grants admin to the earliest real account, so no operator action is needed — but a second, non-superuser account now gets 403 on credential storage, PAT storage and PR merge. deploy/README.md documents granting admin to another account. --- CLAUDE.md | 4 +- codeframe/auth/api_key_router.py | 28 +++- codeframe/auth/dependencies.py | 23 ++- codeframe/auth/manager.py | 40 ++++++ codeframe/auth/router.py | 18 ++- .../workspace_registry_repository.py | 12 +- codeframe/platform_store/schema_manager.py | 54 ++++++- deploy/README.md | 7 + tests/auth/test_api_key_router_scopes.py | 133 ++++++++++++++++++ tests/auth/test_jwt_scope_derivation.py | 84 +++++++++++ tests/auth/test_registration_bootstrap.py | 56 ++++++++ tests/e2e/seed_workspace.py | 5 +- .../test_bootstrap_superuser_backfill.py | 98 +++++++++++++ .../test_workspace_registry_repository.py | 24 ++++ tests/ui/test_v2_scope_enforcement.py | 32 +++++ 15 files changed, 592 insertions(+), 26 deletions(-) create mode 100644 tests/auth/test_api_key_router_scopes.py create mode 100644 tests/auth/test_jwt_scope_derivation.py create mode 100644 tests/platform_store/test_bootstrap_superuser_backfill.py diff --git a/CLAUDE.md b/CLAUDE.md index e4164e56..91905a74 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,7 +54,9 @@ Issue **import + traceability** (#565) is **complete**: `POST /api/v2/integratio **Phase 3.5C is complete** — `CaptureGlitchModal` form (description/markdown, source, scope, gate obligations, severity, expiry) reachable from the PROOF9 page and the persistent sidebar "Capture Glitch" button. REQ detail view (`/proof/[req_id]`) ships markdown description rendering, `ProofScope` metadata display, obligations table with `Latest Run` column, sortable/filterable evidence history, and empty-state CTA. Backend: `ScopeOut` model on `RequirementResponse`. Issues #568, #569. -**v2 API auth enforcement (#336) is complete** — all 22 v2 REST routers require auth (`require_auth`: JWT Bearer or `X-API-Key`) via router-level dependencies in `server.py`; env-gated `CODEFRAME_AUTH_REQUIRED` (default **ON**; set `false` for local dev — read at request time). Streams never carry the JWT in the URL (#745): an authenticated `POST /auth/stream-ticket` (write scope; `has_scope`, so admin implies write) mints a 60s **single-use** ticket (`codeframe/auth/stream_tickets.py`, in-process store — multi-worker caveat documented in the module), redeemed as `?ticket=` **only** on the two SSE routes (allowlist `_QUERY_TICKET_PATHS` in `codeframe/auth/dependencies.py`) and by `authenticate_websocket` for the two WS routes; `?token=` is no longer accepted anywhere. Frontend fetches a fresh ticket per (re)connect via `fetchStreamTicket()`/`withStreamTicket()`; `useEventSource`/`useTerminalSocket` take an async `buildUrl` re-resolved on every retry. `/auth/register` admits only the bootstrap first user (403 after; seeded `!DISABLED!` admin excluded; in-process lock closes the TOCTOU window). Web UI: `/login` page (sign-in + create-first-account), proactive client-side auth guard in `AppLayout` (token-present → allow; no token → `checkAuthAccess` probe → allow only on explicit 2xx, else fail closed to `/login`; #651, #783), axios Bearer interceptor for reactive 401→`/login` redirect, SSE/WS hooks probe the `require_auth`-gated `/api/v2/settings/keys` (which respects `CODEFRAME_AUTH_REQUIRED`) on stream failure to catch token expiry (#651), SSE hooks append a fresh stream ticket, sidebar logout; `/auth/*` proxied in `next.config.js`. Backend tests run auth-off via root `tests/conftest.py` `setdefault`; `tests/ui/test_v2_auth_enforcement.py` opts back in. +**v2 API auth enforcement (#336) is complete** — all 22 v2 REST routers require auth (`require_auth`: JWT Bearer or `X-API-Key`) via router-level dependencies in `server.py`; env-gated `CODEFRAME_AUTH_REQUIRED` (default **ON**; set `false` for local dev — read at request time). Streams never carry the JWT in the URL (#745): an authenticated `POST /auth/stream-ticket` (write scope; `has_scope`, so admin implies write) mints a 60s **single-use** ticket (`codeframe/auth/stream_tickets.py`, in-process store — multi-worker caveat documented in the module), redeemed as `?ticket=` **only** on the two SSE routes (allowlist `_QUERY_TICKET_PATHS` in `codeframe/auth/dependencies.py`) and by `authenticate_websocket` for the two WS routes; `?token=` is no longer accepted anywhere. Frontend fetches a fresh ticket per (re)connect via `fetchStreamTicket()`/`withStreamTicket()`; `useEventSource`/`useTerminalSocket` take an async `buildUrl` re-resolved on every retry. `/auth/register` admits only the bootstrap first user (403 after; seeded `!DISABLED!` admin excluded; in-process lock closes the TOCTOU window), and `UserManager.on_after_register` promotes that sole account to `is_superuser`. + +**Scopes are real, not decorative (#898)** — a JWT principal's scopes now derive from its user row: `[read, write]` always, plus `admin` only when `is_superuser`. So `require_scope(SCOPE_ADMIN)` (credential storage, GitHub PAT storage, PR merge) genuinely refuses a non-superuser browser session. Corollaries: only a superuser may mint an `admin`-scoped API key (`api_key_router.create_api_key`, else any user could self-escalate); the API-key router carries a router-level `require_method_scope` so a read-scope key can no longer DELETE keys; `SchemaManager._ensure_bootstrap_superuser` backfills admin to the earliest login-capable account when an upgraded instance has none (otherwise the change silently strips the operator's admin). The auth-disabled synthetic principal is unchanged — it still carries all scopes, being the single-operator local opt-out. Workspace registry ownership is write-once in the same change: `upsert` never reassigns a non-NULL `owner_user_id`, so user B cannot take over user A's `repo_path` by re-registering it. Web UI: `/login` page (sign-in + create-first-account), proactive client-side auth guard in `AppLayout` (token-present → allow; no token → `checkAuthAccess` probe → allow only on explicit 2xx, else fail closed to `/login`; #651, #783), axios Bearer interceptor for reactive 401→`/login` redirect, SSE/WS hooks probe the `require_auth`-gated `/api/v2/settings/keys` (which respects `CODEFRAME_AUTH_REQUIRED`) on stream failure to catch token expiry (#651), SSE hooks append a fresh stream ticket, sidebar logout; `/auth/*` proxied in `next.config.js`. Backend tests run auth-off via root `tests/conftest.py` `setdefault`; `tests/ui/test_v2_auth_enforcement.py` opts back in. **PROOF9 merge gate is enforced (#731)** — `POST /api/v2/pr/{n}/merge` blocks while open (non-waived) requirements exist (409 with a blocking-requirement summary; ledger failure → explicit 500, never a silent pass-through). Explicit bypass: `override: true` + `override_reason` in `MergePRRequest`; the override is persisted to the `pr_merge_overrides` ledger table (actor from `require_auth`, reason, bypassed requirements, timestamp) and surfaced as `merge_override` on `GET /api/v2/pr/history` items. `cf pr merge` enforces the same gate on the cwd workspace (`--override --reason "..."` to bypass, same audit record; no workspace in cwd → no gate). Scope is workspace-global open requirements — the same condition as the frontend `canMerge` in `PRStatusPanel`. Known follow-ups (not blockers): per-branch scope filtering, `vacuous_pass` distinction (#556), and no frontend override UI yet (API/CLI-only). diff --git a/codeframe/auth/api_key_router.py b/codeframe/auth/api_key_router.py index fc25ac26..af26ce5e 100644 --- a/codeframe/auth/api_key_router.py +++ b/codeframe/auth/api_key_router.py @@ -15,10 +15,15 @@ from fastapi import APIRouter, Depends, HTTPException, Request, status from pydantic import BaseModel, field_validator -from codeframe.auth.dependencies import get_current_user, require_auth +from codeframe.auth.dependencies import ( + get_current_user, + require_auth, + require_method_scope, +) from codeframe.auth.models import User from codeframe.auth.api_keys import ( validate_scopes, + SCOPE_ADMIN, SCOPE_READ, SCOPE_WRITE, ) @@ -27,7 +32,15 @@ logger = logging.getLogger(__name__) -router = APIRouter(prefix="/api/auth/api-keys", tags=["auth", "api-keys"]) +# The method-scope guard applies to the whole router (#898). Without it this +# router proved authentication only, so a scopes:["read"] key could DELETE its +# owner's write/admin keys — a mutation from a credential that proved read +# access. GET needs read; POST/DELETE need write. +router = APIRouter( + prefix="/api/auth/api-keys", + tags=["auth", "api-keys"], + dependencies=[Depends(require_method_scope)], +) # ============================================================================= @@ -151,12 +164,23 @@ async def create_api_key( The full API key is returned only once. Store it securely - it cannot be retrieved again. + A key can never grant more than its creator holds: only an ``is_superuser`` + account may mint an ``admin``-scoped key (#898). Otherwise the new + JWT-scope derivation would be a formality — any signed-in user could issue + themselves an admin key and use it to store credentials or merge PRs. + Args: body: API key configuration (name, scopes, optional expiration) Returns: Created API key details including the full key (shown once) """ + if SCOPE_ADMIN in body.scopes and not current_user.is_superuser: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Only an admin account may create an admin-scoped API key", + ) + service = get_api_key_service(request) result = service.create_api_key( diff --git a/codeframe/auth/dependencies.py b/codeframe/auth/dependencies.py index ecb5313f..7c53bacf 100644 --- a/codeframe/auth/dependencies.py +++ b/codeframe/auth/dependencies.py @@ -4,8 +4,9 @@ - JWT Bearer tokens (existing FastAPI Users integration) - API keys via X-API-Key header (new for programmatic access) -API keys use scope-based permissions (read, write, admin). -JWT tokens get full permissions for backward compatibility. +Both credential types use scope-based permissions (read, write, admin). API-key +scopes are stored on the key; JWT scopes are derived from the user record — +read+write always, admin only for ``is_superuser`` accounts (issue #898). """ import logging @@ -377,11 +378,18 @@ def _resolve(principal: Dict[str, Any]) -> Dict[str, Any]: # Fall back to JWT if jwt_user is not None: + # Scopes come from the user record (#898). Handing every session + # [read, write, admin] made require_scope(SCOPE_ADMIN) decorative: + # anyone with a browser token could store credentials and merge PRs. + # getattr, not attribute access: a principal that somehow lacks the + # column must fail closed to non-admin rather than 500. + scopes = [SCOPE_READ, SCOPE_WRITE] + if getattr(jwt_user, "is_superuser", False): + scopes.append(SCOPE_ADMIN) return _resolve({ "type": "jwt", "user_id": jwt_user.id, - # JWT users get all scopes for backward compatibility - "scopes": [SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN], + "scopes": scopes, "user": jwt_user, }) @@ -414,9 +422,10 @@ async def require_method_scope( Safe methods (GET/HEAD/OPTIONS) require the ``read`` scope; mutating methods (POST/PUT/PATCH/DELETE) require ``write``. Admin-only routes layer - their own ``Depends(require_scope("admin"))`` on top. JWT principals and the - auth-disabled synthetic principal both carry all scopes, so this only - constrains scoped API keys — a read-only key can no longer mutate state. + their own ``Depends(require_scope("admin"))`` on top. JWT principals carry + read+write (admin only when ``is_superuser``, #898) and the auth-disabled + synthetic principal carries all scopes, so in practice this guard constrains + scoped API keys — a read-only key can no longer mutate state. """ required = SCOPE_READ if request.method.upper() in _SAFE_METHODS else SCOPE_WRITE if not has_scope(auth, required): diff --git a/codeframe/auth/manager.py b/codeframe/auth/manager.py index d4324e68..9bab10bd 100644 --- a/codeframe/auth/manager.py +++ b/codeframe/auth/manager.py @@ -20,6 +20,12 @@ # Get configuration from environment DEFAULT_SECRET = "CHANGE-ME-IN-PRODUCTION" +# Placeholder password for the seeded bootstrap admin (id=1). It cannot match +# any bcrypt hash, so that account can never log in and must not be counted as +# a real user. Written by SchemaManager._ensure_default_admin_user; re-exported +# by auth.router, which cannot own it (this module must not import that one). +DISABLED_PASSWORD = "!DISABLED!" + def _read_auth_secret() -> str: """Read ``AUTH_SECRET`` from the env, treating blank/whitespace as unset. @@ -167,6 +173,40 @@ async def on_after_register(self, user: User, request: Optional[Request] = None) "User registered", extra={"user_id": user.id, "email": user.email} ) + await self._promote_if_bootstrap_user(user) + + async def _promote_if_bootstrap_user(self, user: User) -> None: + """Grant ``is_superuser`` to the instance's first real account (#898). + + Admin scope derives from ``is_superuser``, and + ``fastapi_users.get_register_router`` forces the field to ``False`` on + every registration — so without this no principal would ever hold admin + and credential storage, GitHub PAT storage and PR merge would be + permanently 403. + + ``/auth/register`` already admits exactly one login-capable account + (issues #336, #897), so that account is the operator. The count is + re-checked here rather than assumed: if another registration path is + ever added, only a genuinely sole account is promoted. + """ + from sqlalchemy import func, select + + session = getattr(self.user_db, "session", None) + if session is None: # pragma: no cover - non-SQLAlchemy user_db + return + + result = await session.execute( + select(func.count()) + .select_from(User) + .where(User.hashed_password != DISABLED_PASSWORD) + ) + if result.scalar_one() != 1: + return + + await self.user_db.update(user, {"is_superuser": True}) + logger.info( + "Promoted bootstrap user to superuser", extra={"user_id": user.id} + ) async def on_after_login( self, user: User, request: Optional[Request] = None, response=None diff --git a/codeframe/auth/router.py b/codeframe/auth/router.py index f8820c0a..30e8d51f 100644 --- a/codeframe/auth/router.py +++ b/codeframe/auth/router.py @@ -11,7 +11,12 @@ from sqlalchemy import func, select from codeframe.auth.schemas import UserCreate, UserRead, UserUpdate -from codeframe.auth.manager import auth_backend, fastapi_users, get_async_session_maker +from codeframe.auth.manager import ( + DISABLED_PASSWORD, + auth_backend, + fastapi_users, + get_async_session_maker, +) from codeframe.auth.models import User from codeframe.auth.api_key_router import router as api_key_router from codeframe.auth.dependencies import require_auth @@ -30,11 +35,10 @@ class StreamTicketResponse(BaseModel): expires_in: int -# Placeholder password for the seeded bootstrap admin (id=1). It cannot match -# any bcrypt hash, so that account can never log in. It is therefore NOT a real -# account and does not close the registration window. See SchemaManager -# ._ensure_default_admin_user. -_DISABLED_PASSWORD = "!DISABLED!" +# ``DISABLED_PASSWORD`` (imported from auth.manager) is the placeholder password +# for the seeded bootstrap admin (id=1). It cannot match any bcrypt hash, so that +# account can never log in. It is therefore NOT a real account and does not close +# the registration window. See SchemaManager._ensure_default_admin_user. # Serializes the bootstrap registration check-then-create window. The count # check (here) and the user INSERT (fastapi-users route handler) run in @@ -198,7 +202,7 @@ async def allow_registration(request: Request): result = await session.execute( select(func.count()) .select_from(User) - .where(User.hashed_password != _DISABLED_PASSWORD) + .where(User.hashed_password != DISABLED_PASSWORD) ) real_user_count = result.scalar_one() diff --git a/codeframe/platform_store/repositories/workspace_registry_repository.py b/codeframe/platform_store/repositories/workspace_registry_repository.py index 3fb6de81..436b2e9d 100644 --- a/codeframe/platform_store/repositories/workspace_registry_repository.py +++ b/codeframe/platform_store/repositories/workspace_registry_repository.py @@ -61,12 +61,12 @@ def upsert( -- COALESCE so a refresh that omits name/tech_stack (None) keeps the -- previously-stored value instead of nulling it. name = COALESCE(excluded.name, workspaces_registry.name), - -- COALESCE so a refresh that omits the owner (None) keeps the - -- recorded owner instead of nulling it (#720). A refresh with a - -- *different* non-None owner still overwrites; that's safe only - -- because path-based tenant isolation (#655) stops user B from - -- re-registering a path they don't own. - owner_user_id = COALESCE(excluded.owner_user_id, workspaces_registry.owner_user_id), + -- Ownership is write-once: an already-recorded owner is never + -- reassigned (#898), so user B re-registering user A's + -- repo_path cannot take the row over. A refresh that omits the + -- owner still keeps it (#720), and a row left ownerless by an + -- auth-disabled run is still claimable on first attribution. + owner_user_id = COALESCE(workspaces_registry.owner_user_id, excluded.owner_user_id), tech_stack = COALESCE(excluded.tech_stack, workspaces_registry.tech_stack), last_opened_at = excluded.last_opened_at """, diff --git a/codeframe/platform_store/schema_manager.py b/codeframe/platform_store/schema_manager.py index 18eef2ac..be4add9c 100644 --- a/codeframe/platform_store/schema_manager.py +++ b/codeframe/platform_store/schema_manager.py @@ -9,6 +9,12 @@ logger = logging.getLogger(__name__) +# Password placeholder for the seeded bootstrap admin (id=1). It cannot match any +# bcrypt hash, so that account can never log in and must never be counted as a +# real user. Mirrors ``codeframe.auth.manager.DISABLED_PASSWORD``, which this +# layer must not import (auth depends on platform_store, not the reverse). +_DISABLED_PASSWORD = "!DISABLED!" + class SchemaManager: """Manages database schema creation and migrations. @@ -54,6 +60,9 @@ def create_schema(self) -> None: # Ensure default admin user exists self._ensure_default_admin_user() + # Backfill admin for instances upgraded across issue #898 + self._ensure_bootstrap_superuser() + def _create_auth_tables(self, cursor: sqlite3.Cursor) -> None: """Create authentication tables (fastapi-users compatible).""" cursor.execute( @@ -320,8 +329,9 @@ def _ensure_default_admin_user(self) -> None: id, email, name, hashed_password, is_active, is_superuser, is_verified, email_verified ) - VALUES (1, 'admin@localhost', 'Admin User', '!DISABLED!', 1, 1, 1, 1) - """ + VALUES (1, 'admin@localhost', 'Admin User', ?, 1, 1, 1, 1) + """, + (_DISABLED_PASSWORD,), ) user_created = cursor.rowcount > 0 @@ -332,3 +342,43 @@ def _ensure_default_admin_user(self) -> None: ) self.conn.commit() + + def _ensure_bootstrap_superuser(self) -> None: + """Give the operator admin back after issue #898 (upgrade backfill). + + Admin scope now derives from ``users.is_superuser``, and every account + registered before that change has it set to 0 (fastapi-users forces the + field False on registration). An in-place upgrade would therefore strip + admin from the only human on the instance, with no in-product way to + restore it. + + So: when no *login-capable* superuser exists, promote the earliest + login-capable account. The seeded id=1 admin holds is_superuser=1 but + carries a password placeholder that cannot match any bcrypt hash, so it + never counts on either side of the check. Idempotent — the second run + finds a superuser and does nothing. + + ponytail: earliest-id heuristic, not an ownership record. Fine while + registration admits exactly one account (#336/#897); revisit if the + product ever grows multi-user signup. + """ + cursor = self.conn.cursor() + cursor.execute( + """ + UPDATE users SET is_superuser = 1 + WHERE id = ( + SELECT MIN(id) FROM users WHERE hashed_password != ? + ) + AND NOT EXISTS ( + SELECT 1 FROM users + WHERE is_superuser = 1 AND hashed_password != ? + ) + """, + (_DISABLED_PASSWORD, _DISABLED_PASSWORD), + ) + if cursor.rowcount > 0: + logger.info( + "Granted admin (is_superuser) to the earliest login-capable " + "account: this instance had none after the scope change (#898)." + ) + self.conn.commit() diff --git a/deploy/README.md b/deploy/README.md index ef1c30ab..c9afd4be 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -64,6 +64,13 @@ Registration still closes permanently once the first real account exists, token or not. The seeded `admin@localhost` row has a disabled password, cannot log in, and does not count as that account. +That first account is also the instance's **admin** (`is_superuser`), and it is +the only one — admin scope is what gates credential storage, GitHub PAT storage +and PR merge (issue #898). A session for any later, non-superuser account gets +`[read, write]` and is refused on those endpoints. Grant admin to another +account by setting `is_superuser = 1` on its `users` row in the control-plane +DB; there is no in-product promotion flow yet. + **Set the token before the first deploy** — it is in `.env.production.example` / `.env.staging.example`: diff --git a/tests/auth/test_api_key_router_scopes.py b/tests/auth/test_api_key_router_scopes.py new file mode 100644 index 00000000..0a591332 --- /dev/null +++ b/tests/auth/test_api_key_router_scopes.py @@ -0,0 +1,133 @@ +"""The API-key management router enforces scope by method (issue #898 / P0.4). + +The router was mounted with a bare ``require_auth``, so a ``scopes: ["read"]`` +key could DELETE any of its owner's other keys — including the write/admin ones +— which is a privilege *reduction* attack (revoke the keys you can't use) and, +worse, an unscoped mutation from a credential that proved read access only. +""" + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from codeframe.auth import router as auth_router +from codeframe.auth.api_keys import SCOPE_READ, SCOPE_WRITE +from codeframe.auth.manager import reset_auth_engine +from codeframe.core.api_key_service import ApiKeyService +from codeframe.platform_store.database import Database +from tests.conftest import setup_test_user + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def client_and_keys(tmp_path, monkeypatch): + db_path = tmp_path / "state.db" + monkeypatch.setenv("DATABASE_PATH", str(db_path)) + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "true") + reset_auth_engine() + + db = Database(db_path) + db.initialize() + setup_test_user(db, user_id=1) + svc = ApiKeyService(db) + keys = { + "read": svc.create_api_key(user_id=1, name="r", scopes=[SCOPE_READ]), + "write": svc.create_api_key(user_id=1, name="w", scopes=[SCOPE_READ, SCOPE_WRITE]), + } + db.close() + + app = FastAPI() + app.include_router(auth_router.router) + yield TestClient(app, raise_server_exceptions=False), keys + reset_auth_engine() + + +def _hdr(key) -> dict: + return {"X-API-Key": key.key} + + +class TestApiKeyRouterScopes: + def test_read_key_may_list(self, client_and_keys): + client, keys = client_and_keys + resp = client.get("/api/auth/api-keys", headers=_hdr(keys["read"])) + assert resp.status_code == 200, resp.text + + def test_read_key_forbidden_on_revoke(self, client_and_keys): + """The core regression: a read-scope key must not revoke a key.""" + client, keys = client_and_keys + resp = client.delete( + f"/api/auth/api-keys/{keys['write'].id}", headers=_hdr(keys["read"]) + ) + assert resp.status_code == 403, resp.text + + # And the target key really is still active. + listed = client.get("/api/auth/api-keys", headers=_hdr(keys["read"])).json() + assert [k for k in listed if k["id"] == keys["write"].id][0]["is_active"] is True + + def test_write_key_may_revoke(self, client_and_keys): + client, keys = client_and_keys + resp = client.delete( + f"/api/auth/api-keys/{keys['read'].id}", headers=_hdr(keys["write"]) + ) + assert resp.status_code == 200, resp.text + assert resp.json()["revoked"] is True + + def test_read_key_forbidden_on_create(self, client_and_keys): + """Creation already required a JWT; the method guard fires first and 403s.""" + client, keys = client_and_keys + resp = client.post( + "/api/auth/api-keys", + headers=_hdr(keys["read"]), + json={"name": "escalate", "scopes": ["admin"]}, + ) + assert resp.status_code in (401, 403), resp.text + + +class TestAdminKeyMintingRequiresSuperuser: + """A key must not grant more than its creator holds (#898). + + Without this the JWT-scope derivation is a formality: any signed-in + non-superuser could mint themselves an ``admin`` key and use it to store + credentials or merge PRs. + """ + + def _jwt(self): + from tests.conftest import create_test_jwt_token + + return {"Authorization": f"Bearer {create_test_jwt_token(user_id=1)}"} + + def _promote(self, tmp_path): + db = Database(tmp_path / "state.db") + db.initialize() + db.conn.execute("UPDATE users SET is_superuser = 1 WHERE id = 1") + db.conn.commit() + db.close() + + def test_non_superuser_cannot_mint_admin_key(self, client_and_keys): + client, _ = client_and_keys + resp = client.post( + "/api/auth/api-keys", + headers=self._jwt(), + json={"name": "escalate", "scopes": ["admin"]}, + ) + assert resp.status_code == 403, resp.text + + def test_non_superuser_may_mint_a_write_key(self, client_and_keys): + client, _ = client_and_keys + resp = client.post( + "/api/auth/api-keys", + headers=self._jwt(), + json={"name": "ordinary", "scopes": ["read", "write"]}, + ) + assert resp.status_code == 201, resp.text + + def test_superuser_may_mint_admin_key(self, client_and_keys, tmp_path): + client, _ = client_and_keys + self._promote(tmp_path) + resp = client.post( + "/api/auth/api-keys", + headers=self._jwt(), + json={"name": "legit", "scopes": ["admin"]}, + ) + assert resp.status_code == 201, resp.text diff --git a/tests/auth/test_jwt_scope_derivation.py b/tests/auth/test_jwt_scope_derivation.py new file mode 100644 index 00000000..b82edb6d --- /dev/null +++ b/tests/auth/test_jwt_scope_derivation.py @@ -0,0 +1,84 @@ +"""JWT principals derive their scopes from the user record (issue #898 / P0.4). + +``require_auth`` used to hand every JWT principal ``[read, write, admin]`` "for +backward compatibility", which made the whole ``require_scope(SCOPE_ADMIN)`` +layer — credential storage, GitHub PAT storage, PR merge — decorative for +anything holding a browser session, and left ``users.is_superuser`` unread. + +Admin now comes from ``is_superuser``; the auth-disabled synthetic principal is +deliberately unchanged (it is the local single-operator opt-out). +""" + +import pytest + +from codeframe.auth.api_keys import SCOPE_ADMIN, SCOPE_READ, SCOPE_WRITE +from codeframe.auth.dependencies import require_auth +from codeframe.auth.models import User +from codeframe.auth.scopes import has_scope + +pytestmark = pytest.mark.v2 + + +def _user(user_id: int = 1, *, is_superuser: bool) -> User: + """A detached User row — require_auth only reads attributes off it.""" + return User( + id=user_id, + email="u@example.com", + hashed_password="x", + is_active=True, + is_superuser=is_superuser, + is_verified=True, + ) + + +class TestJwtScopeDerivation: + @pytest.mark.asyncio + async def test_non_superuser_jwt_gets_read_write_only(self, monkeypatch): + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "true") + auth = await require_auth(jwt_user=_user(is_superuser=False), api_key_auth=None) + + assert auth["type"] == "jwt" + assert auth["scopes"] == [SCOPE_READ, SCOPE_WRITE] + assert has_scope(auth, SCOPE_WRITE) is True + assert has_scope(auth, SCOPE_ADMIN) is False + + @pytest.mark.asyncio + async def test_superuser_jwt_gets_admin(self, monkeypatch): + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "true") + auth = await require_auth(jwt_user=_user(is_superuser=True), api_key_auth=None) + + assert auth["scopes"] == [SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN] + assert has_scope(auth, SCOPE_ADMIN) is True + + @pytest.mark.asyncio + async def test_api_key_scopes_still_win_over_jwt(self, monkeypatch): + """A read-only key presented alongside a superuser JWT stays read-only.""" + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "true") + auth = await require_auth( + jwt_user=_user(is_superuser=True), + api_key_auth={"type": "api_key", "user_id": 1, "scopes": [SCOPE_READ]}, + ) + + assert auth["type"] == "api_key" + assert has_scope(auth, SCOPE_ADMIN) is False + + @pytest.mark.asyncio + async def test_principal_without_the_flag_fails_closed(self, monkeypatch): + """A principal object lacking ``is_superuser`` degrades to non-admin + rather than raising — fail closed, never 500 and never escalate.""" + from types import SimpleNamespace + + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "true") + auth = await require_auth(jwt_user=SimpleNamespace(id=42), api_key_auth=None) + + assert auth["user_id"] == 42 + assert has_scope(auth, SCOPE_ADMIN) is False + + @pytest.mark.asyncio + async def test_auth_disabled_principal_keeps_admin(self, monkeypatch): + """The local no-auth opt-out is unchanged — it has no user record to read.""" + monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "false") + auth = await require_auth(jwt_user=None, api_key_auth=None) + + assert auth["type"] == "disabled" + assert has_scope(auth, SCOPE_ADMIN) is True diff --git a/tests/auth/test_registration_bootstrap.py b/tests/auth/test_registration_bootstrap.py index f2af247f..5298850e 100644 --- a/tests/auth/test_registration_bootstrap.py +++ b/tests/auth/test_registration_bootstrap.py @@ -141,6 +141,62 @@ async def _run(): ) +def _superuser_flag(db_path, email): + db = Database(db_path) + db.initialize() + row = db.conn.execute( + "SELECT is_superuser FROM users WHERE email = ?", (email,) + ).fetchone() + db.close() + return None if row is None else bool(row[0]) + + +class TestBootstrapUserBecomesSuperuser: + """#898 / P0.4 — admin scope now derives from ``users.is_superuser``. + + ``fastapi_users.get_register_router`` forces ``is_superuser=False`` on every + registration, so without this promotion no principal would ever hold admin + and credential storage / PAT storage / PR merge would be permanently 403. + The bootstrap route admits exactly one login-capable account (#336/#897), so + that account is the operator. + """ + + def test_bootstrap_user_is_promoted_to_superuser(self, auth_client): + resp = _register(auth_client) + assert resp.status_code in (200, 201), resp.text + assert _superuser_flag(auth_client.db_path, "first@example.com") is True + + def test_seeded_disabled_admin_does_not_block_promotion(self, auth_client): + """The seeded id=1 placeholder is already is_superuser=1 but cannot log + in, so it must not be mistaken for an existing admin.""" + _register(auth_client) + assert _superuser_flag(auth_client.db_path, "first@example.com") is True + + def test_registration_alongside_an_existing_real_user_does_not_promote( + self, auth_client, monkeypatch + ): + """Belt-and-braces: if a second registration path ever opens, only a + genuinely-sole account is promoted.""" + from codeframe.auth import router as auth_router_module + + _add_real_user(auth_client.db_path) + # Bypass the closed-registration gate to exercise the promotion guard + # itself rather than the route gate that normally precedes it. + async def _allow_anything(): + yield + + auth_client.app.dependency_overrides[auth_router_module.allow_registration] = ( + _allow_anything + ) + try: + resp = _register(auth_client, email="second@example.com") + assert resp.status_code in (200, 201), resp.text + finally: + auth_client.app.dependency_overrides.clear() + + assert _superuser_flag(auth_client.db_path, "second@example.com") is False + + class TestBootstrapTokenGate: """#897 — a configured token is mandatory, loopback included.""" diff --git a/tests/e2e/seed_workspace.py b/tests/e2e/seed_workspace.py index 09758c6b..f2fb3ac0 100644 --- a/tests/e2e/seed_workspace.py +++ b/tests/e2e/seed_workspace.py @@ -57,13 +57,16 @@ def seed_central_user(central_db_path: str) -> None: try: now = _now().isoformat() # id=1 is the seeded DISABLED admin; use a distinct id for our login user. + # is_superuser=1 mirrors a real install: the bootstrap first account is + # promoted to admin (#898), and admin scope is what admits credential + # storage, GitHub PAT storage and PR merge. conn.execute( """ INSERT OR IGNORE INTO users ( id, email, name, hashed_password, is_active, is_superuser, is_verified, email_verified, created_at, updated_at - ) VALUES (2, ?, 'E2E Test User', ?, 1, 0, 1, 1, ?, ?) + ) VALUES (2, ?, 'E2E Test User', ?, 1, 1, 1, 1, ?, ?) """, (TEST_USER_EMAIL, TEST_USER_HASH, now, now), ) diff --git a/tests/platform_store/test_bootstrap_superuser_backfill.py b/tests/platform_store/test_bootstrap_superuser_backfill.py new file mode 100644 index 00000000..c3718e83 --- /dev/null +++ b/tests/platform_store/test_bootstrap_superuser_backfill.py @@ -0,0 +1,98 @@ +"""Upgrade backfill: the operator keeps admin after #898 / P0.4. + +Admin scope now derives from ``users.is_superuser``. Every account registered +before this change has ``is_superuser = 0`` (fastapi-users forces it), so an +in-place upgrade would silently strip admin from the only human on the +instance — credential storage, GitHub PAT storage and PR merge would all start +403-ing with no way to fix it from the product. + +``SchemaManager`` therefore promotes the earliest login-capable account when +the instance has no login-capable superuser at all. The seeded ``!DISABLED!`` +admin (id=1) is is_superuser=1 but cannot log in, so it never counts. +""" + +import pytest + +from codeframe.platform_store.database import Database + +pytestmark = pytest.mark.v2 + +_HASH = "$2b$12$abcdefghijklmnopqrstuv" + + +def _add_user(db, user_id, *, is_superuser=0, password=_HASH): + db.conn.execute( + """ + INSERT OR REPLACE INTO users ( + id, email, name, hashed_password, + is_active, is_superuser, is_verified, email_verified + ) VALUES (?, ?, 'U', ?, 1, ?, 1, 1) + """, + (user_id, f"u{user_id}@example.com", password, is_superuser), + ) + db.conn.commit() + + +def _flags(db_path): + db = Database(db_path) + db.initialize() + rows = db.conn.execute( + "SELECT id, is_superuser FROM users ORDER BY id" + ).fetchall() + db.close() + return {row[0]: bool(row[1]) for row in rows} + + +class TestBootstrapSuperuserBackfill: + def test_promotes_the_only_real_user(self, tmp_path): + db_path = tmp_path / "state.db" + db = Database(db_path) + db.initialize() + _add_user(db, 2) + db.close() + + assert _flags(db_path)[2] is True + + def test_promotes_the_earliest_of_several(self, tmp_path): + db_path = tmp_path / "state.db" + db = Database(db_path) + db.initialize() + _add_user(db, 3) + _add_user(db, 2) + db.close() + + flags = _flags(db_path) + assert flags[2] is True + assert flags[3] is False + + def test_no_promotion_when_a_real_superuser_exists(self, tmp_path): + db_path = tmp_path / "state.db" + db = Database(db_path) + db.initialize() + _add_user(db, 2) + _add_user(db, 3, is_superuser=1) + db.close() + + flags = _flags(db_path) + assert flags[2] is False + assert flags[3] is True + + def test_seeded_disabled_admin_alone_promotes_nobody(self, tmp_path): + """A fresh install has only id=1 (!DISABLED!) — nothing to promote, and + no crash.""" + db_path = tmp_path / "state.db" + db = Database(db_path) + db.initialize() + db.close() + + assert _flags(db_path) == {1: True} + + def test_backfill_is_idempotent(self, tmp_path): + db_path = tmp_path / "state.db" + db = Database(db_path) + db.initialize() + _add_user(db, 2) + _add_user(db, 3) + db.close() + + assert _flags(db_path) == _flags(db_path) == {1: True, 2: True, 3: False} diff --git a/tests/platform_store/test_workspace_registry_repository.py b/tests/platform_store/test_workspace_registry_repository.py index 98d5ed1c..b57ce53b 100644 --- a/tests/platform_store/test_workspace_registry_repository.py +++ b/tests/platform_store/test_workspace_registry_repository.py @@ -179,6 +179,30 @@ def test_upsert_preserves_owner_on_ownerless_refresh(self, db_two_users): db.workspace_registry.upsert(repo_path="/p/alpha", name="alpha", owner_user_id=None) assert db.workspace_registry.get_by_path("/p/alpha")["owner_user_id"] == 1 + def test_upsert_refuses_to_reassign_an_existing_owner(self, db_two_users): + """Issue #898 / P0.4: ownership is not transferable by re-registering. + + Previously the ON CONFLICT clause took ``excluded.owner_user_id`` first, + so user B registering user A's ``repo_path`` silently took the row over. + """ + db = db_two_users + db.workspace_registry.upsert(repo_path="/p/alpha", name="alpha", owner_user_id=1) + + db.workspace_registry.upsert(repo_path="/p/alpha", name="alpha", owner_user_id=2) + + assert db.workspace_registry.get_by_path("/p/alpha")["owner_user_id"] == 1 + # And user B still cannot see or delete it. + assert db.workspace_registry.list_all(owner_user_id=2) == [] + + def test_upsert_claims_an_unowned_row(self, db_two_users): + """A row registered before auth was enforced (owner NULL) is claimable.""" + db = db_two_users + db.workspace_registry.upsert(repo_path="/p/alpha", name="alpha", owner_user_id=None) + + db.workspace_registry.upsert(repo_path="/p/alpha", name="alpha", owner_user_id=2) + + assert db.workspace_registry.get_by_path("/p/alpha")["owner_user_id"] == 2 + def test_delete_is_owner_scoped(self, db_two_users): db = db_two_users entry = db.workspace_registry.upsert(repo_path="/p/a", name="a", owner_user_id=1) diff --git a/tests/ui/test_v2_scope_enforcement.py b/tests/ui/test_v2_scope_enforcement.py index ed8c2491..d04d1cf4 100644 --- a/tests/ui/test_v2_scope_enforcement.py +++ b/tests/ui/test_v2_scope_enforcement.py @@ -161,6 +161,38 @@ def test_admin_key_allowed_on_pr_merge(self, scoped_app): assert r.status_code != 401 +class TestJwtIsNotAutomaticallyAdmin: + """Issue #898 / P0.4 — a browser session no longer implies admin. + + The ``scoped_app`` user (id=1) is seeded ``is_superuser = 0``, so its JWT + must be write-capable but admin-forbidden. Before this, every JWT carried + ``[read, write, admin]`` and the admin gate was decorative for anyone with a + session cookie's worth of token. + """ + + def _jwt(self): + from tests.conftest import create_test_jwt_token + + return {"Authorization": f"Bearer {create_test_jwt_token(user_id=1)}"} + + def test_non_superuser_jwt_forbidden_on_credential_storage(self, scoped_app): + app, _ = scoped_app + r = TestClient(app).put( + "/api/v2/settings/keys/openai", headers=self._jwt(), json={"value": "sk-x"} + ) + assert r.status_code == 403, r.text + + def test_non_superuser_jwt_forbidden_on_pr_merge(self, scoped_app): + app, _ = scoped_app + r = TestClient(app).post("/api/v2/pr/1/merge", headers=self._jwt(), json={}) + assert r.status_code == 403, r.text + + def test_non_superuser_jwt_still_allowed_on_write(self, scoped_app): + app, _ = scoped_app + r = TestClient(app).put("/api/v2/settings", headers=self._jwt(), json={}) + assert r.status_code not in (401, 403), r.text + + class TestPatchIsMutating: def test_read_key_forbidden_on_patch(self, scoped_app): app, keys = scoped_app From ef9359ee066f7fb87fa67bae9013530f2d700952 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 01:51:11 -0700 Subject: [PATCH 2/2] fix(security): close the API-key admin hole and harden the bootstrap promotion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #898. Both CodeRabbit and the opencode/GLM pass independently flagged the same top defect. - Clamp a key's scopes to its owner's live grant at *resolution* time (_scopes_within_owner_grant in get_api_key_auth), not only at creation. The creation guard alone left every pre-#898 admin key working — back then any signed-in user could mint one — so the fix gated new issuance while the existing population sailed past require_scope(SCOPE_ADMIN). Doing it on read needs no migration and, unlike one, also covers an owner demoted later. The key is clamped, not revoked: it keeps read/write. - Make the bootstrap promotion a single atomic UPDATE carrying both guards (sole login-capable account AND no login-capable superuser). _register_lock is an asyncio.Lock, so it does not serialize across uvicorn workers; with count-then-write, two racing first registrations could each see two users and neither promote — zero admins, no in-product way back. - Give '!DISABLED!' one definition, in platform_store (auth imports it; the reverse is not DAG-legal). Two copies had to stay byte-identical forever: the registration gate, the promotion and the backfill all compare against it, so drift would make fresh deploys unclaimable AND silently strip an upgraded deploy's admin, with no error. Pinned by a test. - create_api_key uses getattr for is_superuser, matching require_auth: a principal lacking the column must fail closed to 403, not raise into a 500 that leaves the route ungated. - Document that the backfill runs on every initialize(), so demoting the sole account does not stick. Deliberate — an instance with no reachable admin cannot store credentials or merge PRs. Demoting a non-earliest account still sticks. Both pinned by tests. Two test fixtures change premise, not strength: an admin-scoped key must now belong to a superuser, so test_v2_scope_enforcement mints its admin key for a new superuser (user 2, leaving user 1 non-superuser for the JWT tests) and test_stream_ticket_endpoint promotes its user. Both still assert exactly what they asserted before. --- codeframe/auth/api_key_router.py | 4 +- codeframe/auth/dependencies.py | 39 ++++- codeframe/auth/manager.py | 61 +++++--- .../workspace_registry_repository.py | 11 +- codeframe/platform_store/schema_manager.py | 25 ++- tests/auth/test_api_key_owner_scope_clamp.py | 145 ++++++++++++++++++ tests/auth/test_stream_ticket_endpoint.py | 5 + .../test_bootstrap_superuser_backfill.py | 53 +++++++ tests/ui/test_v2_scope_enforcement.py | 10 +- 9 files changed, 320 insertions(+), 33 deletions(-) create mode 100644 tests/auth/test_api_key_owner_scope_clamp.py diff --git a/codeframe/auth/api_key_router.py b/codeframe/auth/api_key_router.py index af26ce5e..5a1f1b36 100644 --- a/codeframe/auth/api_key_router.py +++ b/codeframe/auth/api_key_router.py @@ -175,7 +175,9 @@ async def create_api_key( Returns: Created API key details including the full key (shown once) """ - if SCOPE_ADMIN in body.scopes and not current_user.is_superuser: + # getattr, matching require_auth: a principal lacking the column must fail + # closed to 403, never raise into a 500 that leaves the route ungated. + if SCOPE_ADMIN in body.scopes and not getattr(current_user, "is_superuser", False): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Only an admin account may create an admin-scoped API key", diff --git a/codeframe/auth/dependencies.py b/codeframe/auth/dependencies.py index 7c53bacf..9f08b879 100644 --- a/codeframe/auth/dependencies.py +++ b/codeframe/auth/dependencies.py @@ -257,6 +257,43 @@ async def get_current_user_optional( # ============================================================================= +def _scopes_within_owner_grant(db: Any, key_record: Dict[str, Any]) -> list: + """Clamp a key's stored scopes to what its owner currently holds (#898). + + ``admin`` is the only scope tied to the user record, so this drops it when + the owning account is not ``is_superuser``. Enforcing it here rather than at + creation alone makes the invariant *continuous*: it also covers admin keys + persisted before the creation guard existed, and a key whose owner is later + demoted — neither of which a one-time migration would keep true. + + Fails closed: a missing user row or an unreadable ``is_superuser`` drops + admin rather than granting it. + """ + scopes = list(key_record.get("scopes") or []) + if SCOPE_ADMIN not in scopes: + return scopes + + try: + row = db.conn.execute( + "SELECT is_superuser FROM users WHERE id = ?", (key_record["user_id"],) + ).fetchone() + is_superuser = bool(row[0]) if row is not None else False + except Exception as e: + logger.error(f"Could not verify API key owner's admin status: {e}") + is_superuser = False + + if is_superuser: + return scopes + + logger.warning( + "API key %s carries admin scope but its owner (user_id=%s) is not a " + "superuser; admin dropped for this request (#898).", + key_record.get("id"), + key_record.get("user_id"), + ) + return [s for s in scopes if s != SCOPE_ADMIN] + + async def get_api_key_auth( api_key: Optional[str] = Security(api_key_header), request: Request = None, @@ -321,7 +358,7 @@ async def get_api_key_auth( return { "type": "api_key", "user_id": key_record["user_id"], - "scopes": key_record["scopes"], + "scopes": _scopes_within_owner_grant(db, key_record), "key_id": key_record["id"], } diff --git a/codeframe/auth/manager.py b/codeframe/auth/manager.py index 9bab10bd..b0d7cc8d 100644 --- a/codeframe/auth/manager.py +++ b/codeframe/auth/manager.py @@ -15,17 +15,19 @@ from codeframe.auth.models import User +# Re-exported, never redefined. The placeholder password for the seeded +# bootstrap admin (id=1) is owned by the layer that writes it +# (SchemaManager._ensure_default_admin_user). Two independent copies would have +# to stay byte-identical forever: the registration gate, the bootstrap +# promotion and the admin backfill all compare against it, so any drift makes +# fresh deploys unclaimable and silently strips an upgraded deploy's admin. +from codeframe.platform_store.schema_manager import DISABLED_PASSWORD + logger = logging.getLogger(__name__) # Get configuration from environment DEFAULT_SECRET = "CHANGE-ME-IN-PRODUCTION" -# Placeholder password for the seeded bootstrap admin (id=1). It cannot match -# any bcrypt hash, so that account can never log in and must not be counted as -# a real user. Written by SchemaManager._ensure_default_admin_user; re-exported -# by auth.router, which cannot own it (this module must not import that one). -DISABLED_PASSWORD = "!DISABLED!" - def _read_auth_secret() -> str: """Read ``AUTH_SECRET`` from the env, treating blank/whitespace as unset. @@ -185,28 +187,47 @@ async def _promote_if_bootstrap_user(self, user: User) -> None: permanently 403. ``/auth/register`` already admits exactly one login-capable account - (issues #336, #897), so that account is the operator. The count is - re-checked here rather than assumed: if another registration path is - ever added, only a genuinely sole account is promoted. + (issues #336, #897), so that account is the operator. Two guards, both + evaluated *inside* a *single atomic UPDATE* rather than read-then-write: + + - this must be the only login-capable account, and + - no login-capable superuser may exist yet. + + Atomicity matters because ``_register_lock`` is an ``asyncio.Lock`` — + in-process only, so it does not serialize across uvicorn/gunicorn + workers. With count-then-write, two racing first-time registrations can + each observe two users and neither promote, leaving the instance with + zero admins and no in-product way back. Letting the database arbitrate + removes that window. """ - from sqlalchemy import func, select + from sqlalchemy import text session = getattr(self.user_db, "session", None) if session is None: # pragma: no cover - non-SQLAlchemy user_db return result = await session.execute( - select(func.count()) - .select_from(User) - .where(User.hashed_password != DISABLED_PASSWORD) - ) - if result.scalar_one() != 1: - return - - await self.user_db.update(user, {"is_superuser": True}) - logger.info( - "Promoted bootstrap user to superuser", extra={"user_id": user.id} + text( + "UPDATE users SET is_superuser = 1 " + "WHERE id = :uid" + " AND (SELECT COUNT(*) FROM users" + " WHERE hashed_password != :disabled) = 1" + " AND NOT EXISTS (" + " SELECT 1 FROM users" + " WHERE is_superuser = 1 AND hashed_password != :disabled" + " )" + ), + {"uid": user.id, "disabled": DISABLED_PASSWORD}, ) + await session.commit() + + if result.rowcount: + # Keep the in-memory row consistent with what was just written, so + # the registration response does not report is_superuser=false. + await session.refresh(user) + logger.info( + "Promoted bootstrap user to superuser", extra={"user_id": user.id} + ) async def on_after_login( self, user: User, request: Optional[Request] = None, response=None diff --git a/codeframe/platform_store/repositories/workspace_registry_repository.py b/codeframe/platform_store/repositories/workspace_registry_repository.py index 436b2e9d..4310d1ec 100644 --- a/codeframe/platform_store/repositories/workspace_registry_repository.py +++ b/codeframe/platform_store/repositories/workspace_registry_repository.py @@ -33,14 +33,19 @@ def upsert( """Register (or refresh) a workspace by its repo path. Idempotent on ``repo_path``: a second call for the same path updates the - existing row (name/tech_stack/owner + ``last_opened_at``) instead of - creating a duplicate, preserving the original ``id`` and ``created_at``. + existing row (name/tech_stack + ``last_opened_at``) instead of creating a + duplicate, preserving the original ``id`` and ``created_at``. + + Ownership is **write-once** (#898): a conflict never reassigns an + existing ``owner_user_id``. Only an ownerless row (``NULL``, e.g. one + registered while auth was disabled) can still be claimed. Args: repo_path: Absolute path to the repository (unique key). name: Human-readable display name (defaults to last path segment). tech_stack: Natural-language tech stack description. - owner_user_id: Owning user id (nullable until auth is enforced). + owner_user_id: Owning user id. Recorded on insert, or on a refresh of + a row that has no owner yet; ignored when the row already has one. Returns: The registry entry as a dict. diff --git a/codeframe/platform_store/schema_manager.py b/codeframe/platform_store/schema_manager.py index be4add9c..58aaf2a9 100644 --- a/codeframe/platform_store/schema_manager.py +++ b/codeframe/platform_store/schema_manager.py @@ -11,9 +11,17 @@ # Password placeholder for the seeded bootstrap admin (id=1). It cannot match any # bcrypt hash, so that account can never log in and must never be counted as a -# real user. Mirrors ``codeframe.auth.manager.DISABLED_PASSWORD``, which this -# layer must not import (auth depends on platform_store, not the reverse). -_DISABLED_PASSWORD = "!DISABLED!" +# real user. +# +# THE single definition — ``codeframe.auth`` imports this one (that direction is +# DAG-legal; the reverse is not). It must not be duplicated: the registration +# gate, the bootstrap promotion and the admin backfill all compare against it, +# so two copies drifting apart would silently make a fresh deploy unclaimable +# AND strip an upgraded deploy's only admin, with no error anywhere. +DISABLED_PASSWORD = "!DISABLED!" + +# Back-compat alias for readers of this module's private name. +_DISABLED_PASSWORD = DISABLED_PASSWORD class SchemaManager: @@ -358,10 +366,15 @@ def _ensure_bootstrap_superuser(self) -> None: never counts on either side of the check. Idempotent — the second run finds a superuser and does nothing. - ponytail: earliest-id heuristic, not an ownership record. Fine while - registration admits exactly one account (#336/#897); revisit if the - product ever grows multi-user signup. + Note this runs on *every* ``initialize()``, not once, so demoting the + only login-capable account does not stick. That is deliberate: an + instance without a reachable admin cannot store credentials or merge + PRs and has no in-product way back. Demoting a non-earliest account + still sticks, since an admin then exists. """ + # ponytail: earliest-id heuristic, not an ownership record. Fine while + # registration admits exactly one account (#336/#897); revisit if the + # product ever grows multi-user signup. cursor = self.conn.cursor() cursor.execute( """ diff --git a/tests/auth/test_api_key_owner_scope_clamp.py b/tests/auth/test_api_key_owner_scope_clamp.py new file mode 100644 index 00000000..209e4fcc --- /dev/null +++ b/tests/auth/test_api_key_owner_scope_clamp.py @@ -0,0 +1,145 @@ +"""A key never grants more than its owner currently holds (issue #898 / P0.4). + +The creation-time guard in ``api_key_router`` blocks a non-superuser from +*minting* an admin key, but that alone leaves two holes: + +- keys already persisted with ``admin`` before the guard existed (every + pre-#898 database — any signed-in user could mint one), and +- a key whose owner is later demoted, which would keep admin forever. + +``get_api_key_auth`` therefore clamps the stored scopes against the owner's +live ``is_superuser`` on every request. That is continuous, needs no migration, +and makes a demotion take effect immediately. +""" + +import pytest + +from codeframe.auth.api_keys import SCOPE_ADMIN, SCOPE_READ, SCOPE_WRITE +from codeframe.auth.dependencies import ( + _scopes_within_owner_grant, + get_api_key_auth, +) +from codeframe.core.api_key_service import ApiKeyService +from codeframe.platform_store.database import Database +from tests.conftest import setup_test_user + +pytestmark = pytest.mark.v2 + + +class _FakeState: + pass + + +class _FakeApp: + def __init__(self, db): + self.state = _FakeState() + self.state.db = db + + +class _FakeRequest: + def __init__(self, db): + self.app = _FakeApp(db) + self.state = _FakeState() + + +@pytest.fixture +def db(tmp_path, monkeypatch): + monkeypatch.setenv("DATABASE_PATH", str(tmp_path / "state.db")) + database = Database(tmp_path / "state.db") + database.initialize() + setup_test_user(database, user_id=1) # is_superuser = 0 + yield database + database.close() + + +def _set_superuser(db, value): + db.conn.execute("UPDATE users SET is_superuser = ? WHERE id = 1", (int(value),)) + db.conn.commit() + + +async def _resolve(db, key): + return await get_api_key_auth(api_key=key, request=_FakeRequest(db)) + + +class TestOwnerScopeClamp: + @pytest.mark.asyncio + async def test_admin_dropped_when_owner_is_not_superuser(self, db): + key = ApiKeyService(db).create_api_key( + user_id=1, name="legacy", scopes=[SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN] + ) + + auth = await _resolve(db, key.key) + + assert auth is not None + assert SCOPE_ADMIN not in auth["scopes"] + # The rest of the key still works — it is clamped, not revoked. + assert auth["scopes"] == [SCOPE_READ, SCOPE_WRITE] + + @pytest.mark.asyncio + async def test_admin_honored_when_owner_is_superuser(self, db): + key = ApiKeyService(db).create_api_key( + user_id=1, name="legit", scopes=[SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN] + ) + _set_superuser(db, True) + + auth = await _resolve(db, key.key) + + assert SCOPE_ADMIN in auth["scopes"] + + @pytest.mark.asyncio + async def test_demotion_takes_effect_immediately(self, db): + """The case a one-time migration could never cover.""" + key = ApiKeyService(db).create_api_key( + user_id=1, name="k", scopes=[SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN] + ) + _set_superuser(db, True) + assert SCOPE_ADMIN in (await _resolve(db, key.key))["scopes"] + + _set_superuser(db, False) + + assert SCOPE_ADMIN not in (await _resolve(db, key.key))["scopes"] + + @pytest.mark.asyncio + async def test_non_admin_key_is_untouched(self, db): + key = ApiKeyService(db).create_api_key( + user_id=1, name="rw", scopes=[SCOPE_READ, SCOPE_WRITE] + ) + + assert (await _resolve(db, key.key))["scopes"] == [SCOPE_READ, SCOPE_WRITE] + + +class TestClampFailsClosed: + """The clamp is exercised directly here: an *orphan* key is unreachable (a + foreign key on ``api_keys.user_id`` guarantees the owner row exists), so the + branches worth pinning are the ones a real DB can still produce.""" + + def _key_record(self): + return { + "id": "k1", + "user_id": 1, + "scopes": [SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN], + } + + def test_unreadable_owner_drops_admin(self): + """A transient read failure must drop admin, not grant it.""" + + class _Boom: + class conn: + @staticmethod + def execute(*_a, **_k): + raise RuntimeError("transient DB failure") + + scopes = _scopes_within_owner_grant(_Boom(), self._key_record()) + + assert scopes == [SCOPE_READ, SCOPE_WRITE] + + def test_absent_owner_row_drops_admin(self): + class _NoRow: + class conn: + @staticmethod + def execute(*_a, **_k): + return type("C", (), {"fetchone": staticmethod(lambda: None)})() + + scopes = _scopes_within_owner_grant(_NoRow(), self._key_record()) + + assert SCOPE_ADMIN not in scopes diff --git a/tests/auth/test_stream_ticket_endpoint.py b/tests/auth/test_stream_ticket_endpoint.py index 3234eccf..86732f45 100644 --- a/tests/auth/test_stream_ticket_endpoint.py +++ b/tests/auth/test_stream_ticket_endpoint.py @@ -29,6 +29,11 @@ def auth_client(tmp_path, monkeypatch): db = Database(db_path) db.initialize() setup_test_user(db, user_id=1) + # Superuser: TestStreamTicketScopeEnforcement mints an admin-scoped key for + # this user, and since #898 a key's scopes are clamped to what its owner + # holds — an admin key owned by a non-superuser is no longer admin. + db.conn.execute("UPDATE users SET is_superuser = 1 WHERE id = 1") + db.conn.commit() db.close() app = FastAPI() diff --git a/tests/platform_store/test_bootstrap_superuser_backfill.py b/tests/platform_store/test_bootstrap_superuser_backfill.py index c3718e83..8a9ac006 100644 --- a/tests/platform_store/test_bootstrap_superuser_backfill.py +++ b/tests/platform_store/test_bootstrap_superuser_backfill.py @@ -43,6 +43,19 @@ def _flags(db_path): return {row[0]: bool(row[1]) for row in rows} +def test_disabled_password_has_exactly_one_definition(): + """The registration gate, the bootstrap promotion and this backfill all + compare against the placeholder. Two copies drifting apart would make fresh + deploys unclaimable and silently strip an upgraded deploy's only admin — + with no exception and no log line. So pin them to one object. + """ + from codeframe.auth import manager + from codeframe.platform_store import schema_manager + + assert manager.DISABLED_PASSWORD is schema_manager.DISABLED_PASSWORD + assert schema_manager._DISABLED_PASSWORD is schema_manager.DISABLED_PASSWORD + + class TestBootstrapSuperuserBackfill: def test_promotes_the_only_real_user(self, tmp_path): db_path = tmp_path / "state.db" @@ -87,6 +100,46 @@ def test_seeded_disabled_admin_alone_promotes_nobody(self, tmp_path): assert _flags(db_path) == {1: True} + def test_a_demoted_sole_account_is_re_promoted(self, tmp_path): + """Intentional, and worth pinning: the backfill runs on every + ``initialize()``, so demoting the only login-capable account does not + stick. The invariant it defends — an instance always has exactly one + reachable admin — outranks honoring a self-demotion that would lock the + operator out of credential storage and PR merge with no way back. + + Demoting a *non-earliest* account still sticks (an admin exists), which + is the case that actually matters for revoking someone's access. + """ + db_path = tmp_path / "state.db" + db = Database(db_path) + db.initialize() + _add_user(db, 2) + db.close() + assert _flags(db_path)[2] is True + + db = Database(db_path) + db.initialize() + db.conn.execute("UPDATE users SET is_superuser = 0 WHERE id = 2") + db.conn.commit() + db.close() + + assert _flags(db_path)[2] is True # re-promoted: it is the sole account + + def test_demoting_a_non_earliest_account_sticks(self, tmp_path): + db_path = tmp_path / "state.db" + db = Database(db_path) + db.initialize() + _add_user(db, 2) + _add_user(db, 3, is_superuser=1) + db.conn.execute("UPDATE users SET is_superuser = 0 WHERE id = 3") + db.conn.execute("UPDATE users SET is_superuser = 1 WHERE id = 2") + db.conn.commit() + db.close() + + flags = _flags(db_path) + assert flags[3] is False # stays demoted — user 2 is already admin + assert flags[2] is True + def test_backfill_is_idempotent(self, tmp_path): db_path = tmp_path / "state.db" db = Database(db_path) diff --git a/tests/ui/test_v2_scope_enforcement.py b/tests/ui/test_v2_scope_enforcement.py index d04d1cf4..9b678827 100644 --- a/tests/ui/test_v2_scope_enforcement.py +++ b/tests/ui/test_v2_scope_enforcement.py @@ -41,12 +41,18 @@ def scoped_app(tmp_path, monkeypatch): db = Database(db_path) db.initialize() + # Two users: 1 is an ordinary account (is_superuser=0) and 2 is the + # instance admin. Since #898 a key's scopes are clamped to what its owner + # holds, so an admin-scoped key must belong to a superuser — the read/write + # keys stay on user 1 so the method-scope tests are unaffected. db.conn.execute( """ INSERT OR REPLACE INTO users ( id, email, name, hashed_password, is_active, is_superuser, is_verified, email_verified - ) VALUES (1, 'test@example.com', 'Test', '!DISABLED!', 1, 0, 1, 1) + ) VALUES + (1, 'test@example.com', 'Test', '!DISABLED!', 1, 0, 1, 1), + (2, 'admin@example.com', 'Admin', '!DISABLED!', 1, 1, 1, 1) """ ) db.conn.commit() @@ -55,7 +61,7 @@ def scoped_app(tmp_path, monkeypatch): keys = { "read": svc.create_api_key(user_id=1, name="r", scopes=[SCOPE_READ]).key, "write": svc.create_api_key(user_id=1, name="w", scopes=[SCOPE_READ, SCOPE_WRITE]).key, - "admin": svc.create_api_key(user_id=1, name="a", scopes=[SCOPE_ADMIN]).key, + "admin": svc.create_api_key(user_id=2, name="a", scopes=[SCOPE_ADMIN]).key, } db.close()