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..5a1f1b36 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,25 @@ 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) """ + # 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", + ) + 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..9f08b879 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 @@ -256,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, @@ -320,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"], } @@ -377,11 +415,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 +459,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..b0d7cc8d 100644 --- a/codeframe/auth/manager.py +++ b/codeframe/auth/manager.py @@ -15,6 +15,14 @@ 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 @@ -167,6 +175,59 @@ 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. 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 text + + session = getattr(self.user_db, "session", None) + if session is None: # pragma: no cover - non-SQLAlchemy user_db + return + + result = await session.execute( + 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/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..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. @@ -61,12 +66,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..58aaf2a9 100644 --- a/codeframe/platform_store/schema_manager.py +++ b/codeframe/platform_store/schema_manager.py @@ -9,6 +9,20 @@ 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. +# +# 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: """Manages database schema creation and migrations. @@ -54,6 +68,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 +337,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 +350,48 @@ 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. + + 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( + """ + 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_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_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/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/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..8a9ac006 --- /dev/null +++ b/tests/platform_store/test_bootstrap_superuser_backfill.py @@ -0,0 +1,151 @@ +"""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} + + +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" + 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_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) + 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..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() @@ -161,6 +167,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