From 3d07230ab24be3867a6dd8089cd1de8b0ec9406c Mon Sep 17 00:00:00 2001 From: Mitchell Currie Date: Tue, 21 Jul 2026 14:27:37 +1000 Subject: [PATCH 1/4] Implement project management --- README.md | 5 +- STATUS.md | 2 +- smolrouter/app.py | 174 ++++++- smolrouter/container.py | 16 +- smolrouter/facade_key_cli.py | 7 +- smolrouter/project_key_manager.py | 437 ++++++++++++++++++ smolrouter/templates/project_detail.html | 79 ++++ smolrouter/templates/projects.html | 40 ++ tests/unit/test_project_key_manager.py | 557 +++++++++++++++++++++++ 9 files changed, 1300 insertions(+), 17 deletions(-) create mode 100644 smolrouter/project_key_manager.py create mode 100644 tests/unit/test_project_key_manager.py diff --git a/README.md b/README.md index 540ce16..8cdf840 100644 --- a/README.md +++ b/README.md @@ -250,7 +250,8 @@ For OpenAI-compatible providers, a configured provider `api_key` takes precedenc - Use `-C/--config` or `ROUTES_CONFIG` for deterministic routing config selection. - Use `SMOLROUTER_SECRETS` for deterministic secrets file selection; otherwise SmolRouter searches `./secrets.yaml`, the user config dir, then the site config dir. - Use `SMOLROUTER_FACADE_KEYS` for dedicated facade-key storage (`./facade_keys.yaml`, user config dir, then site config dir). -- Provision facade keys with `python -m smolrouter.manage_facade_keys create --project --routes-config ` (generates `srk-...` secrets; default append behavior avoids clobbering old secrets). +- Manage projects and facade API keys from **Projects** in the Web UI. Project metadata stays in the routes YAML/JSON and secrets stay in `facade_keys.yaml`; new keys are shown once for copying, while existing keys are represented only by SHA-256 fingerprints. Saving project metadata normalizes the routes file, so YAML comments and hand formatting may not be retained. +- For non-interactive automation, `python -m smolrouter.manage_facade_keys create --project --routes-config ` remains available and uses the same locked file-management path as the UI. - Use `BLOB_STORAGE_PATH` explicitly in production if you want a location other than the safe default under `~/.smolrouter/blob_storage`. - For dev checkouts, `BLOB_STORAGE_PATH=./blob_storage` keeps blobs next to the repo as before. - `docker-compose.yml` mounts `./logs:/app/logs`, so rotated ERROR logs persist across container restarts. @@ -263,7 +264,7 @@ Configure `request_rate_limits` in the routes config. The anonymous and project Requests with invalid or mismatched local keys receive `401` before consuming a bucket. An admitted request consumes capacity before its JSON body is parsed or sent upstream. A rejected request receives `429` with OpenAI-style error code `anonymous_rate_limit_exceeded` or `project_rate_limit_exceeded`, a whole-second `Retry-After`, and policy/bucket-class headers that contain no project key or identity. Redis failures fail closed with `503` and code `rate_limiter_unavailable`; production startup also verifies the limiter and refuses FakeRedis when limiting is enabled. -The Projects UI and API expose each configured project's effective two-window policy and whether it comes from the project default or a whole-project override. They do not expose live Redis counters. Request graphs, filters, and drilldowns only cover logs still present in the configured request-log retention window, so they are not authoritative rate-limit accounting. +The Projects UI can create and delete projects and create or revoke their facade API keys. It also exposes each configured project's effective two-window policy and whether it comes from the project default or a whole-project override. Stored secrets are never returned after creation; the UI and read APIs expose only full SHA-256 fingerprints and counts. They do not expose live Redis counters. Request graphs, filters, and drilldowns only cover logs still present in the configured request-log retention window, so they are not authoritative rate-limit accounting. ## Testing diff --git a/STATUS.md b/STATUS.md index e2a19b0..b53bf75 100644 --- a/STATUS.md +++ b/STATUS.md @@ -6,7 +6,7 @@ Google GenAI provider has extra features like key status tracking and exhaustion detection. Need to ensure consistent IA across all providers (OpenAI, Anthropic, Ollama). Consider: Should other providers have similar detailed status tracking? Location: Provider interfaces and dashboard consistency. - 🔴 Facade-key identity and attribution (Phase A complete, Phase B first milestone complete) - Treat facade API keys as first-class SmolRouter identity for request attribution, soft quota accounting, routing/QoS defaults, and future batch approval policy. Phase A groundwork is in place: facade-key config/secret registry, validation, and container plumbing. Phase B now accepts local facade keys on the OpenAI write path (`/v1/chat/completions`, `/v1/completions`, `/v1/responses`) via `Authorization: Bearer srk-...`, keeps `X-SmolRouter-Key` as a transitional alias, exempts those write routes from JWT-in-`Authorization` collision, threads identity through `ClientContext`, persists a generic request subject (`identity_kind`, `identity_subject_id`, `identity_display_name`) on request creation, surfaces project identity in dashboard/client/request-detail views, and adds read-only `/projects` inventory for configured facade-key identities plus `/projects/{id}` drilldown backed by a Redis identity-recency index. Facade-key provisioning now has an operator CLI (`python -m smolrouter.manage_facade_keys create`) and dedicated secret boundary (`facade_keys.yaml` / `SMOLROUTER_FACADE_KEYS`) as the single runtime source of truth for facade-key secrets. Future deliberate gaps: no quota ledger yet, no hard enforcement, no Ollama compatibility-path parity, no `/v1/models` or `/api/tags` identity resolution yet, and no broader dashboard/browser auth redesign or provider-BYOK transport cleanup yet. Historical traffic-only identities are reachable via drilldown when known, but are not listed as inventory. Full design: `docs/PROPOSAL_facade_key_identity_accounting.md`. + Treat facade API keys as first-class SmolRouter identity for request attribution, soft quota accounting, routing/QoS defaults, and future batch approval policy. Phase A groundwork is in place: facade-key config/secret registry, validation, and container plumbing. Phase B now accepts local facade keys on the OpenAI write path (`/v1/chat/completions`, `/v1/completions`, `/v1/responses`) via `Authorization: Bearer srk-...`, keeps `X-SmolRouter-Key` as a transitional alias, exempts those write routes from JWT-in-`Authorization` collision, threads identity through `ClientContext`, persists a generic request subject (`identity_kind`, `identity_subject_id`, `identity_display_name`) on request creation, and surfaces project identity in dashboard/client/request-detail views. The `/projects` UI now creates and deletes file-backed projects and creates/revokes facade API keys with one-time secret display; the CLI remains an automation fallback and shares the same locked mutation path. Project metadata remains in routes YAML/JSON, dedicated secrets remain in `facade_keys.yaml` / `SMOLROUTER_FACADE_KEYS`, and read views expose only key fingerprints. Future deliberate gaps: no quota ledger yet, no hard enforcement, no Ollama compatibility-path parity, no `/v1/models` or `/api/tags` identity resolution yet, and no broader dashboard/browser auth redesign or provider-BYOK transport cleanup yet. Historical traffic-only identities are reachable via drilldown when known, but are not listed as inventory. Full design: `docs/PROPOSAL_facade_key_identity_accounting.md`. - 🟡 Facade-key request/token accounting and soft quotas Build on facade-key identity first, then add approximate request/token accounting and soft quota state for local project/use-case keys. Keep semantics honest: soft/best-effort, streaming-aware, and fail-open on accounting gaps. This should precede broad provider-key accounting convergence because the main product question is "which local project/use case consumed the budget?" not just "which downstream secret was used?". Full design: `docs/PROPOSAL_facade_key_identity_accounting.md`. diff --git a/smolrouter/app.py b/smolrouter/app.py index 2db3028..59a6293 100644 --- a/smolrouter/app.py +++ b/smolrouter/app.py @@ -16,10 +16,11 @@ from datetime import datetime, timezone from urllib.parse import quote, urlparse -from fastapi import FastAPI, Request, WebSocket, WebSocketDisconnect, HTTPException +from fastapi import Depends, FastAPI, Request, WebSocket, WebSocketDisconnect, HTTPException from fastapi.responses import StreamingResponse, JSONResponse, HTMLResponse from fastapi.templating import Jinja2Templates from fastapi.staticfiles import StaticFiles +from pydantic import BaseModel, ConfigDict import httpx from contextlib import asynccontextmanager @@ -48,6 +49,7 @@ from smolrouter.container import initialize_container from smolrouter.config_paths import normalize_provider_file_references, resolve_routes_config_path from smolrouter.facade_keys import RequestIdentity +from smolrouter.project_key_manager import ProjectKeyManagementError, facade_key_id from smolrouter.redis_config import is_fake_redis from smolrouter.request_rate_limits import ( RateLimitDecision, @@ -1407,7 +1409,7 @@ async def _handle_request_without_local_facade_key( ) -def _resolve_request_identity( +async def _resolve_request_identity( active_container: Any, request: Request, presented_key: Optional[str] = None, @@ -1416,7 +1418,7 @@ def _resolve_request_identity( if not presented_key: return None - registry = active_container.get_facade_key_registry() + registry = await asyncio.to_thread(active_container.get_facade_key_registry) resolved = registry.resolve_secret(presented_key) if resolved is None or resolved.identity is None: raise HTTPException(status_code=401, detail=_invalid_local_facade_key_detail()) @@ -1450,6 +1452,7 @@ def _build_project_inventory_entry( secret_count: int = 0, effective_rate_limit: Any = None, rate_limits_enabled: Optional[bool] = None, + keys: Optional[list[dict[str, str]]] = None, ) -> dict[str, Any]: if config is None: display_name = None @@ -1481,6 +1484,7 @@ def _build_project_inventory_entry( "enabled": enabled, "display_name": display_name, "secret_count": int(secret_count), + "keys": list(keys or []), "default_class": default_class, "tags": list(tags), "quota": quota_payload, @@ -1498,13 +1502,14 @@ async def _get_facade_key_registry_data() -> dict[str, Any]: if active_container is None: return {} - registry = active_container.get_facade_key_registry() + registry = await asyncio.to_thread(active_container.get_facade_key_registry) rate_limit_config = _request_rate_limit_config(active_container) return { key_id: { "config": config, "secret_count": len(registry.get_secrets(key_id)), + "keys": [{"key_id": facade_key_id(secret)} for secret in registry.get_secrets(key_id)], "effective_rate_limit": config.rate_limit or rate_limit_config.project_default, "rate_limits_enabled": rate_limit_config.enabled, } @@ -1524,6 +1529,7 @@ async def _load_facade_key_inventory() -> dict[str, dict[str, Any]]: secret_count=entry["secret_count"], effective_rate_limit=entry["effective_rate_limit"], rate_limits_enabled=entry["rate_limits_enabled"], + keys=entry["keys"], ) for project_id, entry in sorted(registry_data.items(), key=lambda item: item[0]) } @@ -1711,7 +1717,7 @@ def _build_request_tracking_headers(log_entry: Any) -> Dict[str, str]: return headers -def _resolve_presented_facade_identity( +async def _resolve_presented_facade_identity( active_container: Any, request: Request, presented_facade_key: Optional[str], @@ -1719,12 +1725,12 @@ def _resolve_presented_facade_identity( resolved_facade_key: Optional[str], ) -> Optional[RequestIdentity]: if presented_facade_key and presented_authorization_facade_key: - header_identity = _resolve_request_identity( + header_identity = await _resolve_request_identity( active_container, request, presented_key=presented_facade_key, ) - authorization_identity = _resolve_request_identity( + authorization_identity = await _resolve_request_identity( active_container, request, presented_key=presented_authorization_facade_key, @@ -1733,7 +1739,7 @@ def _resolve_presented_facade_identity( raise HTTPException(status_code=401, detail="Mismatched local facade key values") return authorization_identity - return _resolve_request_identity(active_container, request, presented_key=resolved_facade_key) + return await _resolve_request_identity(active_container, request, presented_key=resolved_facade_key) async def _prepare_proxy_request_context( @@ -1801,7 +1807,7 @@ async def _prepare_proxy_request_context( ) try: - identity = _resolve_presented_facade_identity( + identity = await _resolve_presented_facade_identity( active_container, request, presented_facade_key, @@ -2523,6 +2529,29 @@ async def proxy_ollama_request(path: str, request: Request) -> JSONResponse | St } +class _StrictMutationBody(BaseModel): + model_config = ConfigDict(extra="forbid") + + +class ProjectCreateBody(_StrictMutationBody): + project_id: str + display_name: Optional[str] = None + tags: Optional[list[str]] = None + + +class ProjectDeleteBody(_StrictMutationBody): + project_id: str + + +class ProjectKeyCreateBody(_StrictMutationBody): + project_id: str + + +class ProjectKeyDeleteBody(_StrictMutationBody): + project_id: str + key_id: str + + @app.post("/v1/chat/completions", responses=OPENAI_PROXY_ROUTE_RESPONSES) async def chat_completions(request: Request): return await proxy_request("/v1/chat/completions", request) @@ -2869,6 +2898,133 @@ async def api_projects(request: Request): raise HTTPException(status_code=500, detail="Failed to load project inventory") +def _single_raw_header(request: Request, name: bytes) -> str: + values = [value.decode("latin-1") for key, value in request.scope.get("headers", []) if key.lower() == name] + if len(values) != 1 or not values[0].strip(): + raise HTTPException(status_code=403, detail=f"Exactly one {name.decode()} header is required") + return values[0].strip() + + +def _canonical_origin(value: str) -> tuple[str, str, int]: + try: + parsed = urlparse(value) + if ( + parsed.scheme not in {"http", "https"} + or not parsed.hostname + or parsed.username is not None + or parsed.password is not None + or parsed.path not in {"", "/"} + or parsed.params + or parsed.query + or parsed.fragment + ): + raise ValueError + return parsed.scheme, parsed.hostname.lower(), parsed.port or (443 if parsed.scheme == "https" else 80) + except ValueError as exc: + raise HTTPException(status_code=403, detail="Malformed Origin header") from exc + + +def _check_project_mutation_request(request: Request) -> None: + get_webui_security().check_webui_access(request) + content_type = request.headers.get("content-type", "").split(";", 1)[0].strip().lower() + if content_type != "application/json": + raise HTTPException(status_code=415, detail="Content-Type must be application/json") + origin = _single_raw_header(request, b"origin") + _single_raw_header(request, b"host") + if origin.lower() == "null" or _canonical_origin(origin) != _canonical_origin(str(request.base_url).rstrip("/")): + raise HTTPException(status_code=403, detail="Origin must exactly match the request origin") + + +async def _project_key_manager(request: Request): + active_container = await _get_active_container(False) + if active_container is None: + raise HTTPException(status_code=500, detail="Project/key management is unavailable") + try: + return active_container.get_project_key_manager() + except Exception as exc: + raise HTTPException(status_code=500, detail="Project/key management is unavailable") from exc + + +def _management_error(exc: ProjectKeyManagementError) -> HTTPException: + return HTTPException(status_code=exc.status_code, detail=str(exc)) + + +def _project_mutation_guard(request: Request) -> None: + _check_project_mutation_request(request) + + +def _mutation_response(payload: dict[str, Any], status_code: int = 200) -> JSONResponse: + return JSONResponse( + payload, + status_code=status_code, + headers={"Cache-Control": "no-store, no-cache", "Pragma": "no-cache"}, + ) + + +@app.post("/api/project-management/projects", status_code=201) +async def create_managed_project( + request: Request, + body: ProjectCreateBody, + _guard: None = Depends(_project_mutation_guard), +): + manager = await _project_key_manager(request) + try: + project = await asyncio.to_thread(manager.create_project, body.project_id, body.display_name, body.tags) + return _mutation_response({"project": project}, 201) + except ProjectKeyManagementError as exc: + raise _management_error(exc) + + +@app.delete("/api/project-management/projects") +async def delete_managed_project( + request: Request, + body: ProjectDeleteBody, + _guard: None = Depends(_project_mutation_guard), +): + manager = await _project_key_manager(request) + try: + registry = await asyncio.to_thread(manager.get_registry) + if not registry.has_key(body.project_id): + historical = await RequestLog.get_by_identity("facade_key", body.project_id, limit=1) + if historical: + raise HTTPException(status_code=409, detail="Historical-only projects cannot be deleted") + await asyncio.to_thread(manager.delete_project, body.project_id) + return _mutation_response({"deleted": True, "project_id": body.project_id}) + except ProjectKeyManagementError as exc: + raise _management_error(exc) + + +@app.post("/api/project-management/keys", status_code=201) +async def create_managed_key( + request: Request, + body: ProjectKeyCreateBody, + _guard: None = Depends(_project_mutation_guard), +): + manager = await _project_key_manager(request) + try: + secret, key_id = await asyncio.to_thread(manager.create_key, body.project_id) + return _mutation_response( + {"project_id": body.project_id, "key_id": key_id, "secret": secret}, + 201, + ) + except ProjectKeyManagementError as exc: + raise _management_error(exc) + + +@app.delete("/api/project-management/keys") +async def delete_managed_key( + request: Request, + body: ProjectKeyDeleteBody, + _guard: None = Depends(_project_mutation_guard), +): + manager = await _project_key_manager(request) + try: + await asyncio.to_thread(manager.revoke_key, body.project_id, body.key_id) + return _mutation_response({"deleted": True, "project_id": body.project_id, "key_id": body.key_id}) + except ProjectKeyManagementError as exc: + raise _management_error(exc) + + @app.get("/api/projects/{project_id:path}", responses=PROJECT_DETAIL_API_RESPONSES) async def api_project_detail(request: Request, project_id: str, limit: int = 100): get_webui_security().check_webui_access(request) diff --git a/smolrouter/container.py b/smolrouter/container.py index 0f6605e..a97d698 100644 --- a/smolrouter/container.py +++ b/smolrouter/container.py @@ -17,6 +17,7 @@ from .interfaces import ClientContext from .facade_keys import FacadeKeyRegistry, RequestIdentity, load_facade_key_registry +from .project_key_manager import ProjectKeyManager from .request_rate_limits import ( RequestRateLimitConfig, RequestRateLimitPolicy, @@ -99,9 +100,15 @@ class SmolRouterContainer: """ def __init__(self, config: Optional[SmolRouterConfig] = None): + uses_default_config = config is None self.config = config or self._create_default_config() + self._project_key_manager = ProjectKeyManager() if uses_default_config else None if self.config.facade_key_registry is None: - self.config.facade_key_registry = load_facade_key_registry(self.config.facade_keys) + self.config.facade_key_registry = ( + self._project_key_manager.get_registry() + if self._project_key_manager is not None + else load_facade_key_registry(self.config.facade_keys) + ) self._providers = None self._mediator = None self._cache = None @@ -422,8 +429,15 @@ def create_client_context( ) def get_facade_key_registry(self) -> FacadeKeyRegistry: + if self._project_key_manager is not None: + return self._project_key_manager.get_registry() return self.config.facade_key_registry or load_facade_key_registry(self.config.facade_keys) + def get_project_key_manager(self) -> ProjectKeyManager: + if self._project_key_manager is None: + raise RuntimeError("Project/key management is unavailable for an injected static configuration") + return self._project_key_manager + def get_request_rate_limit_config(self) -> RequestRateLimitConfig: """Return the validated global request-rate-limit configuration.""" return self.config.request_rate_limit_config diff --git a/smolrouter/facade_key_cli.py b/smolrouter/facade_key_cli.py index 9da3c8b..9762b73 100644 --- a/smolrouter/facade_key_cli.py +++ b/smolrouter/facade_key_cli.py @@ -16,10 +16,9 @@ append_facade_key_secret, generate_facade_key_secret, load_facade_key_secrets_from_path, - reload_facade_key_secrets, resolve_facade_key_config_path, - save_facade_key_secrets, ) +from .project_key_manager import ProjectKeyManager def configure_create_parser(parser: argparse.ArgumentParser) -> argparse.ArgumentParser: parser.add_argument("--routes-config", default=os.getenv("ROUTES_CONFIG")) @@ -120,8 +119,8 @@ def run_create(args: argparse.Namespace) -> int: print(f"project_id={project_id}, generated_secret={generated}") return 0 - save_facade_key_secrets(secret_file_path, updated) - reload_facade_key_secrets() + manager = ProjectKeyManager(routes_path=routes_path, secrets_path=secret_file_path) + generated, _key_id = manager.create_key(project_id, generated) print(f"Generated facade key for project: {project_id}") print(f"secret (copy exactly once): {generated}") print(f"wrote facade-key secrets to: {secret_file_path}") diff --git a/smolrouter/project_key_manager.py b/smolrouter/project_key_manager.py new file mode 100644 index 0000000..6452e50 --- /dev/null +++ b/smolrouter/project_key_manager.py @@ -0,0 +1,437 @@ +"""Thread- and process-safe file-backed project and facade-key management.""" + +from __future__ import annotations + +import copy +import hashlib +import json +import os +import re +import secrets +import tempfile +import threading +from contextlib import contextmanager +from pathlib import Path +from typing import Any, Iterator, Mapping + +import yaml +from platformdirs import user_config_dir + +from .config_paths import resolve_routes_config_path +from .facade_key_store import ( + generate_facade_key_secret, + load_facade_key_secrets_from_path, + reload_facade_key_secrets, + resolve_facade_key_config_path, +) +from .facade_keys import FacadeKeyRegistry + +if os.name == "nt": + import msvcrt +else: + import fcntl + + +PROJECT_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._/-]{0,127}$") +_PROCESS_LOCK = threading.RLock() + + +class ProjectKeyManagementError(RuntimeError): + status_code = 500 + + +class ProjectKeyValidationError(ProjectKeyManagementError): + status_code = 422 + + +class ProjectKeyNotFoundError(ProjectKeyManagementError): + status_code = 404 + + +class ProjectKeyConflictError(ProjectKeyManagementError): + status_code = 409 + + +def facade_key_id(secret: str) -> str: + return f"sha256:{hashlib.sha256(secret.encode('utf-8')).hexdigest()}" + + +def _revision(path: Path) -> tuple[Any, ...]: + try: + stat = path.stat() + return (True, stat.st_dev, stat.st_ino, stat.st_size, stat.st_mtime_ns) + except FileNotFoundError: + return (False, None, None, None, None) + + +def _content_hash(path: Path) -> str | None: + try: + return hashlib.sha256(path.read_bytes()).hexdigest() + except FileNotFoundError: + return None + + +def _reject_symlink(path: Path) -> None: + if path.is_symlink(): + raise ProjectKeyManagementError(f"Managed file may not be a symlink: {path}") + + +def _read_mapping(path: Path) -> dict[str, Any]: + _reject_symlink(path) + if not path.is_file(): + return {} + try: + raw = path.read_text(encoding="utf-8") + parsed = json.loads(raw) if path.suffix.lower() == ".json" else yaml.safe_load(raw) + except (OSError, ValueError, yaml.YAMLError) as exc: + raise ProjectKeyManagementError(f"Unable to read managed configuration {path}: {exc}") from exc + if parsed is None: + return {} + if not isinstance(parsed, dict): + raise ProjectKeyManagementError(f"Managed configuration must be a mapping: {path}") + return {str(key): value for key, value in parsed.items()} + + +def _serialize(path: Path, payload: Mapping[str, Any]) -> bytes: + if path.suffix.lower() == ".json": + return (json.dumps(payload, indent=2, sort_keys=True) + "\n").encode() + return yaml.safe_dump(dict(payload), sort_keys=True).encode() + + +def _atomic_replace( + path: Path, + payload: Mapping[str, Any], + *, + mode: int, + expected_hash: str | None, + preserve_mode: bool = False, +) -> None: + _reject_symlink(path) + if _content_hash(path) != expected_hash: + raise ProjectKeyConflictError(f"Managed configuration changed while it was being updated: {path}") + path.parent.mkdir(parents=True, exist_ok=True) + existing_mode = (path.stat().st_mode & 0o777) if preserve_mode and path.exists() else mode + fd, tmp_name = tempfile.mkstemp(prefix=f".{path.name}-", suffix=".tmp", dir=path.parent) + try: + if hasattr(os, "fchmod"): + os.fchmod(fd, existing_mode) + else: + os.chmod(tmp_name, existing_mode) + with os.fdopen(fd, "wb") as handle: + handle.write(_serialize(path, payload)) + handle.flush() + os.fsync(handle.fileno()) + os.replace(tmp_name, path) + os.chmod(path, existing_mode) + _fsync_directory(path.parent) + except BaseException: + try: + os.close(fd) + except OSError: + pass + try: + os.unlink(tmp_name) + except OSError: + pass + raise + + +def _fsync_directory(path: Path) -> None: + """Persist a directory entry where the platform supports directory fsync.""" + try: + directory_fd = os.open(path, os.O_RDONLY) + except OSError: + if os.name == "nt": + return + raise + try: + try: + os.fsync(directory_fd) + except OSError: + if os.name != "nt": + raise + finally: + os.close(directory_fd) + + +def _lock_file(lock_fd: int) -> None: + if os.name != "nt": + fcntl.flock(lock_fd, fcntl.LOCK_EX) + return + if os.fstat(lock_fd).st_size == 0: + os.write(lock_fd, b"\0") + os.fsync(lock_fd) + os.lseek(lock_fd, 0, os.SEEK_SET) + msvcrt.locking(lock_fd, msvcrt.LK_LOCK, 1) + + +def _unlock_file(lock_fd: int) -> None: + if os.name != "nt": + fcntl.flock(lock_fd, fcntl.LOCK_UN) + return + os.lseek(lock_fd, 0, os.SEEK_SET) + msvcrt.locking(lock_fd, msvcrt.LK_UNLCK, 1) + + +def _validate_project_id(project_id: Any) -> str: + raw_value = str(project_id or "") + value = raw_value.strip() + segments = value.split("/") + if ( + raw_value != value + or not PROJECT_ID_RE.fullmatch(value) + or value.startswith("/") + or value.endswith("/") + or "//" in value + or any(segment in {".", ".."} for segment in segments) + ): + raise ProjectKeyValidationError( + "project_id must be 1-128 characters, start with an alphanumeric character, " + "and contain only letters, numbers, '.', '_', '-', and well-formed '/' segments" + ) + return value + + +def _project_config(display_name: Any = None, tags: Any = None) -> dict[str, Any]: + result: dict[str, Any] = {"enabled": True} + if display_name is not None: + if not isinstance(display_name, str) or not display_name.strip() or len(display_name.strip()) > 100: + raise ProjectKeyValidationError("display_name must be a non-empty string of at most 100 characters") + result["display_name"] = display_name.strip() + if tags is not None: + if not isinstance(tags, list) or len(tags) > 20: + raise ProjectKeyValidationError("tags must be a list containing at most 20 values") + cleaned: list[str] = [] + for tag in tags: + if not isinstance(tag, str) or not tag.strip() or len(tag.strip()) > 40: + raise ProjectKeyValidationError("each tag must be a non-empty string of at most 40 characters") + if tag.strip() in cleaned: + raise ProjectKeyValidationError("tags must be unique") + cleaned.append(tag.strip()) + result["tags"] = cleaned + return result + + +class ProjectKeyManager: + """Own the authoritative route metadata, key file, locks, and registry snapshot.""" + + def __init__(self, routes_path: Path | str | None = None, secrets_path: Path | str | None = None): + self.routes_path = ( + Path(routes_path).expanduser().absolute() + if routes_path is not None + else resolve_routes_config_path(os.getenv("ROUTES_CONFIG")) + ) + resolved_secrets = Path(secrets_path).expanduser().absolute() if secrets_path else resolve_facade_key_config_path() + self.secrets_path = resolved_secrets or (Path(user_config_dir("smolrouter")) / "facade_keys.yaml").resolve() + lock_seed = hashlib.sha256(f"{self.routes_path}\0{self.secrets_path}".encode()).hexdigest() + self.lock_path = self.secrets_path.parent / f".project-keys-{lock_seed}.lock" + self._snapshot: FacadeKeyRegistry | None = None + self._snapshot_revision: tuple[Any, ...] | None = None + self._snapshot_error: Exception | None = None + + @contextmanager + def _locked(self) -> Iterator[None]: + with _PROCESS_LOCK: + self.lock_path.parent.mkdir(parents=True, exist_ok=True) + lock_fd = os.open(self.lock_path, os.O_CREAT | os.O_RDWR, 0o600) + acquired = False + try: + _lock_file(lock_fd) + acquired = True + yield + finally: + if acquired: + _unlock_file(lock_fd) + os.close(lock_fd) + + def revision(self) -> tuple[Any, ...]: + return (_revision(self.routes_path), _revision(self.secrets_path)) + + def _read_sources(self) -> tuple[dict[str, Any], dict[str, list[str]]]: + routes = _read_mapping(self.routes_path) + raw_projects = routes.get("facade_keys", {}) + if not isinstance(raw_projects, dict): + raise ProjectKeyManagementError("routes facade_keys must be a mapping") + _reject_symlink(self.secrets_path) + try: + key_map = load_facade_key_secrets_from_path(self.secrets_path) + except (OSError, ValueError, yaml.YAMLError) as exc: + raise ProjectKeyManagementError(f"Unable to read managed facade keys: {exc}") from exc + for project_id, values in key_map.items(): + if len(values) != len(set(values)): + raise ProjectKeyManagementError( + f"Facade key secrets contain duplicate values for project {project_id!r}" + ) + return routes, key_map + + def _recover_after_delete_failure( + self, + project_id: str, + old_keys: Mapping[str, list[str]], + wrote_keys: bool, + original_error: Exception, + ) -> None: + """Reconcile the snapshot with the exact disk state after a partial delete.""" + try: + disk_routes, disk_keys, disk_revision = self._read_stable_sources() + disk_projects = disk_routes.get("facade_keys", {}) + if wrote_keys and project_id in disk_projects: + try: + _atomic_replace( + self.secrets_path, + old_keys, + mode=0o600, + expected_hash=_content_hash(self.secrets_path), + ) + except Exception: + # A configured project without keys is valid and fail-closed. + pass + disk_routes, disk_keys, disk_revision = self._read_stable_sources() + self._install_snapshot(disk_routes, disk_keys, disk_revision) + except Exception as recovery_error: + self._snapshot = None + self._snapshot_revision = self.revision() + self._snapshot_error = ProjectKeyManagementError( + f"Project deletion recovery failed after {type(original_error).__name__}" + ) + raise self._snapshot_error from recovery_error + + def _read_stable_sources( + self, + *, + attempts: int = 3, + ) -> tuple[dict[str, Any], dict[str, list[str]], tuple[Any, ...]]: + for _attempt in range(attempts): + before = self.revision() + routes, keys = self._read_sources() + after = self.revision() + if before == after: + return routes, keys, after + raise ProjectKeyManagementError("Managed configuration changed repeatedly while it was being read") + + def _install_snapshot( + self, + routes: Mapping[str, Any], + keys: Mapping[str, list[str]], + revision: tuple[Any, ...], + ) -> FacadeKeyRegistry: + registry = FacadeKeyRegistry.from_sources(routes.get("facade_keys", {}), keys) + self._snapshot = registry + self._snapshot_revision = revision + self._snapshot_error = None + return registry + + def _reload_snapshot_from_disk(self) -> FacadeKeyRegistry: + try: + routes, keys, revision = self._read_stable_sources() + return self._install_snapshot(routes, keys, revision) + except Exception as exc: + self._snapshot = None + self._snapshot_revision = self.revision() + self._snapshot_error = exc + raise + + def get_registry(self) -> FacadeKeyRegistry: + revision = self.revision() + if self._snapshot is not None and self._snapshot_revision == revision and self._snapshot_error is None: + return self._snapshot + with self._locked(): + revision = self.revision() + if self._snapshot is not None and self._snapshot_revision == revision and self._snapshot_error is None: + return self._snapshot + try: + return self._reload_snapshot_from_disk() + except Exception as exc: + self._snapshot = None + self._snapshot_revision = revision + self._snapshot_error = exc + raise + + def list_keys(self, project_id: str) -> list[dict[str, str]]: + registry = self.get_registry() + return [{"key_id": facade_key_id(value)} for value in registry.get_secrets(project_id)] + + def create_project(self, project_id: Any, display_name: Any = None, tags: Any = None) -> dict[str, Any]: + project_id = _validate_project_id(project_id) + config = _project_config(display_name, tags) + with self._locked(): + routes_hash = _content_hash(self.routes_path) + routes, keys = self._read_sources() + projects = routes.setdefault("facade_keys", {}) + if project_id in projects: + raise ProjectKeyConflictError("Project already exists") + projects[project_id] = config + FacadeKeyRegistry.from_sources(projects, keys) + _atomic_replace(self.routes_path, routes, mode=0o600, expected_hash=routes_hash, preserve_mode=True) + self._reload_snapshot_from_disk() + return {"project_id": project_id, **config} + + def delete_project(self, project_id: Any) -> None: + project_id = str(project_id or "").strip() + with self._locked(): + routes_hash = _content_hash(self.routes_path) + keys_hash = _content_hash(self.secrets_path) + routes, keys = self._read_sources() + projects = routes.get("facade_keys", {}) + if project_id not in projects: + raise ProjectKeyNotFoundError("Project not found") + old_keys = copy.deepcopy(keys) + projects.pop(project_id) + keys.pop(project_id, None) + FacadeKeyRegistry.from_sources(projects, keys) + wrote_keys = keys != old_keys + try: + if wrote_keys: + _atomic_replace(self.secrets_path, keys, mode=0o600, expected_hash=keys_hash) + _atomic_replace(self.routes_path, routes, mode=0o600, expected_hash=routes_hash, preserve_mode=True) + except Exception as exc: + self._recover_after_delete_failure(project_id, old_keys, wrote_keys, exc) + raise + reload_facade_key_secrets() + self._reload_snapshot_from_disk() + + def create_key(self, project_id: Any, secret: str | None = None) -> tuple[str, str]: + project_id = str(project_id or "").strip() + with self._locked(): + routes_hash = _content_hash(self.routes_path) + keys_hash = _content_hash(self.secrets_path) + routes, keys = self._read_sources() + projects = routes.get("facade_keys", {}) + if project_id not in projects: + raise ProjectKeyNotFoundError("Project not found") + secret = secret or generate_facade_key_secret() + if any(secret in existing for existing in keys.values()): + raise ProjectKeyConflictError("Generated key already exists") + keys.setdefault(project_id, []).append(secret) + FacadeKeyRegistry.from_sources(projects, keys) + if _content_hash(self.routes_path) != routes_hash: + raise ProjectKeyConflictError("Routes configuration changed while it was being updated") + _atomic_replace(self.secrets_path, keys, mode=0o600, expected_hash=keys_hash) + reload_facade_key_secrets() + self._reload_snapshot_from_disk() + return secret, facade_key_id(secret) + + def revoke_key(self, project_id: Any, key_id: Any) -> None: + project_id = str(project_id or "").strip() + key_id = str(key_id or "").strip() + with self._locked(): + routes_hash = _content_hash(self.routes_path) + keys_hash = _content_hash(self.secrets_path) + routes, keys = self._read_sources() + projects = routes.get("facade_keys", {}) + if project_id not in projects: + raise ProjectKeyNotFoundError("Project not found") + matches = [value for value in keys.get(project_id, []) if secrets.compare_digest(facade_key_id(value), key_id)] + if not matches: + raise ProjectKeyNotFoundError("Key not found") + if len(matches) != 1: + raise ProjectKeyConflictError("Key fingerprint is ambiguous") + keys[project_id] = [value for value in keys[project_id] if value != matches[0]] + if not keys[project_id]: + keys.pop(project_id) + FacadeKeyRegistry.from_sources(projects, keys) + if _content_hash(self.routes_path) != routes_hash: + raise ProjectKeyConflictError("Routes configuration changed while it was being updated") + _atomic_replace(self.secrets_path, keys, mode=0o600, expected_hash=keys_hash) + reload_facade_key_secrets() + self._reload_snapshot_from_disk() diff --git a/smolrouter/templates/project_detail.html b/smolrouter/templates/project_detail.html index 9dd87f0..79d8678 100644 --- a/smolrouter/templates/project_detail.html +++ b/smolrouter/templates/project_detail.html @@ -102,6 +102,12 @@ padding: 2rem; font-size: 0.95rem; } + .management-panel { background:#fff; border:1px solid #e5e7eb; border-radius:10px; padding:1rem 1.5rem; margin-bottom:1rem; } + .management-row { display:flex; gap:.6rem; align-items:center; flex-wrap:wrap; } + .management-message { color:#b91c1c; min-height:1.2rem; margin-top:.5rem; } + .secret-panel { display:none; background:#fffbeb; border:1px solid #f59e0b; border-radius:8px; padding:1rem; margin-top:.75rem; } + .secret-value, .fingerprint { font-family:"Monaco","Menlo",monospace; overflow-wrap:anywhere; } + .danger { background:#b91c1c !important; color:#fff !important; } {% include 'responsive_shell.html' %} @@ -131,6 +137,35 @@

{{ project.display_name or project.id }}

+ {% if project.configured %} +
+

API keys

+
+
+ Copy this key now. It will not be shown again. +
+ + +
+ + + + + {% for key in project["keys"] %} + + {% else %} + + {% endfor %} + +
FingerprintAction
{{ key.key_id }}
No active API keys.
+
+
+

Delete project

+

Type {{ project.id }} to confirm. Active API keys will be revoked first.

+
+ +
+ {% endif %}
Secret Count
@@ -206,5 +241,49 @@

Recent Facade-Key Requests

+ {% if project.configured %} + + {% endif %} diff --git a/smolrouter/templates/projects.html b/smolrouter/templates/projects.html index c89878b..69e4f91 100644 --- a/smolrouter/templates/projects.html +++ b/smolrouter/templates/projects.html @@ -63,6 +63,11 @@ font-size: 0.72rem; padding: 0.15rem 0.5rem; } + .management-panel { background:#fff; border:1px solid #e5e7eb; border-radius:10px; padding:1rem 1.5rem; margin-bottom:1rem; } + .management-form { display:flex; gap:.6rem; flex-wrap:wrap; align-items:flex-end; } + .management-form label { display:flex; flex-direction:column; gap:.25rem; font-size:.8rem; color:#475569; } + .management-form input { min-width:190px; padding:.55rem; border:1px solid #cbd5e1; border-radius:6px; } + .management-message { margin-top:.6rem; color:#b91c1c; min-height:1.2rem; } {% include 'responsive_shell.html' %} @@ -70,6 +75,17 @@ {% include 'navigation.html' %}
+
+

Create project

+
+ + + + +
+ +

Projects remain file-backed. Saving through this page normalizes the routes YAML/JSON, so YAML comments and hand formatting may not be retained.

+
{{ projects|length }}
@@ -185,5 +201,29 @@

Facade-Key Projects

+ diff --git a/tests/unit/test_project_key_manager.py b/tests/unit/test_project_key_manager.py new file mode 100644 index 0000000..a236946 --- /dev/null +++ b/tests/unit/test_project_key_manager.py @@ -0,0 +1,557 @@ +import hashlib +import json +import multiprocessing +import stat +from concurrent.futures import ThreadPoolExecutor +from unittest.mock import AsyncMock, Mock, patch + +import pytest +import yaml + +import smolrouter.app as app_module +import smolrouter.project_key_manager as manager_module +from smolrouter.database import RequestLog +from smolrouter.interfaces import ClientContext +from smolrouter.project_key_manager import ( + ProjectKeyConflictError, + ProjectKeyManager, + ProjectKeyManagementError, + ProjectKeyValidationError, + facade_key_id, +) + + +def _process_create_keys(routes_path, secrets_path, start): + manager = ProjectKeyManager(routes_path, secrets_path) + for number in range(start, start + 4): + manager.create_key("project-a", f"srk-process-{number}") + + +def _manager(tmp_path, routes_text="facade_keys: {}\n"): + routes = tmp_path / "routes.yaml" + routes.write_text(routes_text, encoding="utf-8") + routes.chmod(0o640) + return ProjectKeyManager(routes, tmp_path / "facade_keys.yaml"), routes + + +def test_create_project_key_revoke_and_delete_are_immediately_visible(tmp_path): + manager, routes = _manager(tmp_path) + created = manager.create_project("team/project", "Project", ["team"]) + assert created["enabled"] is True + assert stat.S_IMODE(routes.stat().st_mode) == 0o640 + + secret, key_id = manager.create_key("team/project", "srk-test-secret") + assert secret == "srk-test-secret" + assert key_id == "sha256:" + hashlib.sha256(secret.encode()).hexdigest() + assert manager.get_registry().resolve_secret(secret).identity.subject_id == "team/project" + assert stat.S_IMODE(manager.secrets_path.stat().st_mode) == 0o600 + + manager.revoke_key("team/project", key_id) + assert manager.get_registry().resolve_secret(secret) is None + manager.delete_project("team/project") + assert "team/project" not in yaml.safe_load(routes.read_text())["facade_keys"] + + +@pytest.mark.parametrize("project_id", ["", "/bad", "bad/", "bad//id", "bad/../id", " bad", "a" * 129]) +def test_new_project_ids_are_strictly_validated(tmp_path, project_id): + manager, _ = _manager(tmp_path) + with pytest.raises(ProjectKeyValidationError): + manager.create_project(project_id) + + +def test_manager_rejects_symlinked_managed_file(tmp_path): + target = tmp_path / "target.yaml" + target.write_text("facade_keys: {}\n") + routes = tmp_path / "routes.yaml" + routes.symlink_to(target) + manager = ProjectKeyManager(routes, tmp_path / "facade_keys.yaml") + with pytest.raises(ProjectKeyManagementError, match="symlink"): + manager.get_registry() + + +def test_manager_rejects_symlinked_secrets_file(tmp_path): + manager, _ = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + target = tmp_path / "target-keys.yaml" + target.write_text("project-a: [srk-live]\n", encoding="utf-8") + manager.secrets_path.symlink_to(target) + with pytest.raises(ProjectKeyManagementError, match="symlink"): + manager.get_registry() + + +@pytest.mark.parametrize( + ("routes_name", "routes_text", "error"), + [ + ("routes.yaml", "facade_keys: []\n", "facade_keys must be a mapping"), + ("routes.yaml", "[broken", "Unable to read managed configuration"), + ("routes.json", '{"facade_keys": []}\n', "facade_keys must be a mapping"), + ("routes.json", "{broken", "Unable to read managed configuration"), + ], +) +def test_manager_rejects_malformed_routes_sources(tmp_path, routes_name, routes_text, error): + routes = tmp_path / routes_name + routes.write_text(routes_text, encoding="utf-8") + manager = ProjectKeyManager(routes, tmp_path / "facade_keys.yaml") + with pytest.raises(ProjectKeyManagementError, match=error): + manager.get_registry() + + +def test_json_routes_preserve_unrelated_fields(tmp_path): + routes = tmp_path / "routes.json" + routes.write_text('{"providers": [{"name": "local"}], "facade_keys": {}}\n', encoding="utf-8") + manager = ProjectKeyManager(routes, tmp_path / "facade_keys.yaml") + manager.create_project("project-a") + stored = json.loads(routes.read_text(encoding="utf-8")) + assert stored["providers"] == [{"name": "local"}] + assert stored["facade_keys"]["project-a"] == {"enabled": True} + + +def test_existing_duplicate_secrets_fail_closed_instead_of_becoming_unrevokeable(tmp_path): + manager, _ = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.secrets_path.write_text("project-a: [srk-dup, srk-dup]\n", encoding="utf-8") + with pytest.raises(ProjectKeyManagementError, match="duplicate values"): + manager.get_registry() + + +@pytest.mark.parametrize("content", ["[broken", "[]\n", "project-a: {nested: value}\n"]) +def test_manager_rejects_malformed_secrets_sources(tmp_path, content): + manager, _ = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.secrets_path.write_text(content, encoding="utf-8") + with pytest.raises(ProjectKeyManagementError, match="Unable to read managed facade keys"): + manager.get_registry() + + +def test_external_invalid_change_never_serves_stale_credentials(tmp_path): + manager, routes = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-live") + assert manager.get_registry().resolve_secret("srk-live") is not None + routes.write_text("facade_keys: []\n", encoding="utf-8") + with pytest.raises(ProjectKeyManagementError): + manager.get_registry() + with pytest.raises(ProjectKeyManagementError): + manager.get_registry() + + +def test_external_replacement_during_read_never_pairs_stale_credentials_with_new_revision(tmp_path, monkeypatch): + manager, routes = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-old") + manager._snapshot = None + real_read = manager._read_sources + replaced = False + + def replace_after_stale_parse(): + nonlocal replaced + result = real_read() + if not replaced: + replaced = True + routes.write_text("facade_keys: {}\n", encoding="utf-8") + manager.secrets_path.write_text("{}\n", encoding="utf-8") + return result + + monkeypatch.setattr(manager, "_read_sources", replace_after_stale_parse) + registry = manager.get_registry() + assert registry.resolve_secret("srk-old") is None + assert manager.get_registry().resolve_secret("srk-old") is None + assert manager._snapshot_revision == manager.revision() + + +def test_threaded_key_creation_does_not_lose_updates(tmp_path): + manager, _ = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + with ThreadPoolExecutor(max_workers=4) as pool: + list(pool.map(lambda number: manager.create_key("project-a", f"srk-{number}"), range(12))) + assert len(manager.get_registry().get_secrets("project-a")) == 12 + assert len(manager.list_keys("project-a")) == 12 + + +def test_process_key_creation_does_not_lose_updates(tmp_path): + manager, routes = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + context = multiprocessing.get_context("fork") + processes = [ + context.Process(target=_process_create_keys, args=(routes, manager.secrets_path, start)) + for start in (0, 4, 8) + ] + for process in processes: + process.start() + for process in processes: + process.join(10) + assert process.exitcode == 0 + assert len(manager.get_registry().get_secrets("project-a")) == 12 + + +def test_external_edit_between_read_and_replace_returns_conflict(tmp_path, monkeypatch): + manager, routes = _manager(tmp_path) + original_read = manager._read_sources + + def racing_read(): + result = original_read() + routes.write_text("facade_keys:\n externally-added: {}\n", encoding="utf-8") + return result + + monkeypatch.setattr(manager, "_read_sources", racing_read) + with pytest.raises(ProjectKeyConflictError): + manager.create_project("project-a") + assert "externally-added" in routes.read_text(encoding="utf-8") + + +def test_revoke_requires_exact_full_fingerprint(tmp_path): + manager, _ = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + _, key_id = manager.create_key("project-a", "srk-live") + assert facade_key_id("srk-live") == key_id + with pytest.raises(Exception, match="Key not found"): + manager.revoke_key("project-a", key_id[:-1]) + + +def test_duplicate_secret_is_rejected_before_it_can_make_revocation_ambiguous(tmp_path): + manager, _ = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-live") + with pytest.raises(ProjectKeyConflictError, match="already exists"): + manager.create_key("project-a", "srk-live") + assert manager.get_registry().get_secrets("project-a") == ("srk-live",) + + +def test_project_delete_rolls_keys_back_when_routes_write_fails(tmp_path): + manager, routes = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-live") + real_replace = manager_module._atomic_replace + + def fail_routes(path, payload, **kwargs): + if path == routes: + raise OSError("routes write failed") + return real_replace(path, payload, **kwargs) + + with patch("smolrouter.project_key_manager._atomic_replace", side_effect=fail_routes): + with pytest.raises(OSError, match="routes write failed"): + manager.delete_project("project-a") + + assert manager.get_registry().resolve_secret("srk-live") is not None + assert yaml.safe_load(routes.read_text(encoding="utf-8"))["facade_keys"] == {"project-a": {}} + + +def test_project_delete_keeps_committed_files_consistent_when_directory_fsync_fails(tmp_path): + manager, routes = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-live") + real_fsync = manager_module._fsync_directory + calls = 0 + + def fail_after_routes_replace(path): + nonlocal calls + calls += 1 + if calls == 2: + raise OSError("routes directory fsync failed") + return real_fsync(path) + + with patch("smolrouter.project_key_manager._fsync_directory", side_effect=fail_after_routes_replace): + with pytest.raises(OSError, match="routes directory fsync failed"): + manager.delete_project("project-a") + + assert yaml.safe_load(routes.read_text(encoding="utf-8"))["facade_keys"] == {} + assert yaml.safe_load(manager.secrets_path.read_text(encoding="utf-8")) == {} + assert manager.get_registry().get_config("project-a") is None + + +def test_project_delete_recovers_when_secret_directory_fsync_fails_after_replace(tmp_path): + manager, routes = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-live") + real_fsync = manager_module._fsync_directory + calls = 0 + + def fail_after_secret_replace(path): + nonlocal calls + calls += 1 + if calls == 1: + raise OSError("secret directory fsync failed") + return real_fsync(path) + + with patch("smolrouter.project_key_manager._fsync_directory", side_effect=fail_after_secret_replace): + with pytest.raises(OSError, match="secret directory fsync failed"): + manager.delete_project("project-a") + + assert yaml.safe_load(routes.read_text(encoding="utf-8"))["facade_keys"] == {"project-a": {}} + assert manager.get_registry().resolve_secret("srk-live") is not None + + +def test_project_delete_rollback_failure_installs_fail_closed_disk_snapshot(tmp_path): + manager, routes = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-live") + real_replace = manager_module._atomic_replace + secret_writes = 0 + + def fail_routes_and_rollback(path, payload, **kwargs): + nonlocal secret_writes + if path == routes: + raise OSError("routes write failed") + secret_writes += 1 + if secret_writes == 2: + raise OSError("rollback failed") + return real_replace(path, payload, **kwargs) + + with patch("smolrouter.project_key_manager._atomic_replace", side_effect=fail_routes_and_rollback): + with pytest.raises(OSError, match="routes write failed"): + manager.delete_project("project-a") + + assert manager.get_registry().get_config("project-a") is not None + assert manager.get_registry().resolve_secret("srk-live") is None + + +def test_atomic_replace_falls_back_when_fchmod_is_unavailable(tmp_path, monkeypatch): + manager, routes = _manager(tmp_path) + monkeypatch.delattr(manager_module.os, "fchmod") + manager.create_project("project-a") + assert yaml.safe_load(routes.read_text(encoding="utf-8"))["facade_keys"]["project-a"]["enabled"] is True + + +@pytest.mark.asyncio +async def test_management_api_requires_same_origin_json_and_secret_is_no_store(async_client, tmp_path): + manager, _ = _manager(tmp_path) + container = Mock() + container.get_project_key_manager.return_value = manager + + async def active(_legacy): + return container + + security = Mock() + security.check_webui_access.return_value = None + headers = {"Origin": "http://test", "Content-Type": "application/json"} + with patch("smolrouter.app._get_active_container", active), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + denied = await async_client.post( + "/api/project-management/projects", + json={"project_id": "project-a"}, + headers={"Origin": "https://evil.example"}, + ) + wrong_type = await async_client.post( + "/api/project-management/projects", + content='{"project_id":"project-a"}', + headers={"Origin": "http://test", "Content-Type": "text/plain"}, + ) + created = await async_client.post( + "/api/project-management/projects", json={"project_id": "project-a"}, headers=headers + ) + keyed = await async_client.post( + "/api/project-management/keys", json={"project_id": "project-a"}, headers=headers + ) + + assert denied.status_code == 403 + assert wrong_type.status_code == 415 + assert created.status_code == 201 + assert keyed.status_code == 201 + payload = keyed.json() + assert payload["secret"].startswith("srk-") + assert payload["key_id"].startswith("sha256:") and len(payload["key_id"]) == 71 + assert keyed.headers["cache-control"] == "no-store, no-cache" + assert keyed.headers["pragma"] == "no-cache" + assert manager.get_registry().resolve_secret(payload["secret"]) is not None + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "headers", + [ + {"Content-Type": "application/json"}, + {"Origin": "null", "Content-Type": "application/json"}, + {"Origin": "http://test/path", "Content-Type": "application/json"}, + [("Origin", "http://test"), ("Origin", "http://test"), ("Content-Type", "application/json")], + [("Host", "test"), ("Host", "evil.example"), ("Origin", "http://test"), ("Content-Type", "application/json")], + ], +) +async def test_management_api_rejects_missing_null_malformed_or_duplicate_origin(async_client, tmp_path, headers): + manager, _ = _manager(tmp_path) + container = Mock() + container.get_project_key_manager.return_value = manager + + async def active(_legacy): + return container + + security = Mock() + security.check_webui_access.return_value = None + with patch("smolrouter.app._get_active_container", active), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + response = await async_client.post( + "/api/project-management/projects", + content='{"project_id":"project-a"}', + headers=headers, + ) + assert response.status_code == 403 + assert manager.get_registry().get_config("project-a") is None + + +@pytest.mark.asyncio +async def test_management_api_accepts_default_origin_port_and_rejects_extra_fields(async_client, tmp_path): + manager, _ = _manager(tmp_path) + container = Mock() + container.get_project_key_manager.return_value = manager + + async def active(_legacy): + return container + + security = Mock() + security.check_webui_access.return_value = None + with patch("smolrouter.app._get_active_container", active), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + extra = await async_client.post( + "/api/project-management/projects", + json={"project_id": "project-a", "unexpected": True}, + headers={"Origin": "http://test:80"}, + ) + created = await async_client.post( + "/api/project-management/projects", + json={"project_id": "project-a"}, + headers={"Origin": "http://test:80"}, + ) + assert extra.status_code == 422 + assert created.status_code == 201 + + +@pytest.mark.asyncio +async def test_management_api_key_revoke_and_project_delete_update_runtime_immediately(async_client, tmp_path): + manager, _ = _manager(tmp_path) + container = Mock() + container.get_project_key_manager.return_value = manager + + async def active(_legacy): + return container + + security = Mock() + security.check_webui_access.return_value = None + headers = {"Origin": "http://test", "Content-Type": "application/json"} + with patch("smolrouter.app._get_active_container", active), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + assert ( + await async_client.post( + "/api/project-management/projects", json={"project_id": "project-a"}, headers=headers + ) + ).status_code == 201 + created_key = await async_client.post( + "/api/project-management/keys", json={"project_id": "project-a"}, headers=headers + ) + secret = created_key.json()["secret"] + key_id = created_key.json()["key_id"] + assert manager.get_registry().resolve_secret(secret) is not None + revoked = await async_client.request( + "DELETE", + "/api/project-management/keys", + json={"project_id": "project-a", "key_id": key_id}, + headers=headers, + ) + deleted = await async_client.request( + "DELETE", + "/api/project-management/projects", + json={"project_id": "project-a"}, + headers=headers, + ) + assert revoked.status_code == 200 + assert deleted.status_code == 200 + assert manager.get_registry().resolve_secret(secret) is None + assert manager.get_registry().get_config("project-a") is None + + +@pytest.mark.asyncio +async def test_created_and_revoked_key_updates_actual_inference_auth_path(async_client, tmp_path, monkeypatch): + manager, _ = _manager(tmp_path) + container = Mock() + container.get_project_key_manager.return_value = manager + container.get_facade_key_registry.side_effect = manager.get_registry + container.create_client_context.side_effect = lambda **kwargs: ClientContext(**kwargs) + container.route_request = AsyncMock(return_value=({"choices": []}, 200, "provider:test", None)) + + async def active(_legacy): + return container + + security = Mock() + security.check_webui_access.return_value = None + monkeypatch.setattr(app_module, "_is_legacy_proxy_mode", lambda: False) + headers = {"Origin": "http://test", "Content-Type": "application/json"} + with patch("smolrouter.app._get_active_container", active), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + await async_client.post( + "/api/project-management/projects", json={"project_id": "project-a"}, headers=headers + ) + created = await async_client.post( + "/api/project-management/keys", json={"project_id": "project-a"}, headers=headers + ) + secret = created.json()["secret"] + key_id = created.json()["key_id"] + accepted = await async_client.post( + "/v1/chat/completions", + json={"model": "gpt-4", "messages": []}, + headers={app_module.FACADE_KEY_HEADER: secret}, + ) + await async_client.request( + "DELETE", + "/api/project-management/keys", + json={"project_id": "project-a", "key_id": key_id}, + headers=headers, + ) + rejected = await async_client.post( + "/v1/chat/completions", + json={"model": "gpt-4", "messages": []}, + headers={app_module.FACADE_KEY_HEADER: secret}, + ) + + assert accepted.status_code == 200 + assert rejected.status_code == 401 + assert rejected.json()["detail"] == "Invalid SmolRouter facade key" + + +@pytest.mark.asyncio +async def test_management_api_rejects_deleting_historical_only_project(async_client, tmp_path): + manager, _ = _manager(tmp_path) + container = Mock() + container.get_project_key_manager.return_value = manager + + async def active(_legacy): + return container + + await RequestLog.create( + source_ip="127.0.0.1", + method="POST", + path="/v1/chat/completions", + identity_kind="facade_key", + identity_subject_id="historical-only", + request_id="historical-only-management-test", + ) + security = Mock() + security.check_webui_access.return_value = None + with patch("smolrouter.app._get_active_container", active), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + response = await async_client.request( + "DELETE", + "/api/project-management/projects", + json={"project_id": "historical-only"}, + headers={"Origin": "http://test", "Content-Type": "application/json"}, + ) + assert response.status_code == 409 + + +@pytest.mark.asyncio +async def test_project_ui_has_management_controls_but_never_secret_values(async_client, tmp_path): + manager, _ = _manager(tmp_path, "facade_keys:\n project-a: {}\n") + manager.create_key("project-a", "srk-never-render-this") + container = Mock() + container.get_facade_key_registry.side_effect = manager.get_registry + limits = Mock(enabled=False, project_default=None) + container.get_request_rate_limit_config.return_value = limits + + async def active(_legacy): + return container + + security = Mock() + security.check_webui_access.return_value = None + with patch("smolrouter.app._get_active_container", active), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + listing = await async_client.get("/projects") + detail = await async_client.get("/projects/project-a") + assert listing.status_code == 200 and "Create project" in listing.text + assert detail.status_code == 200 and "Create API key" in detail.text and "Delete project" in detail.text + assert "Copy key" in detail.text + assert "navigator.clipboard.writeText" in detail.text + assert "Copy failed. Select and copy the key manually" in detail.text + assert "pagehide" in detail.text + assert "sha256:" in detail.text + assert "srk-never-render-this" not in listing.text + detail.text From 120239f3578f3c45a09328637064bea72aaa81e8 Mon Sep 17 00:00:00 2001 From: Mitchell Currie Date: Tue, 21 Jul 2026 14:51:16 +1000 Subject: [PATCH 2/4] Address project management review feedback --- smolrouter/app.py | 70 ++++++++++++---- smolrouter/project_key_manager.py | 45 ++++++---- smolrouter/templates/project_detail.html | 2 +- tests/unit/test_project_key_manager.py | 102 +++++++++++++++++++++++ 4 files changed, 185 insertions(+), 34 deletions(-) diff --git a/smolrouter/app.py b/smolrouter/app.py index 59a6293..1515b68 100644 --- a/smolrouter/app.py +++ b/smolrouter/app.py @@ -12,7 +12,7 @@ from dataclasses import dataclass from enum import Enum from pathlib import Path -from typing import Any, AsyncIterator, Dict, Optional, Tuple +from typing import Annotated, Any, AsyncIterator, Dict, Optional, Tuple from datetime import datetime, timezone from urllib.parse import quote, urlparse @@ -2528,6 +2528,33 @@ async def proxy_ollama_request(path: str, request: Request) -> JSONResponse | St 404: {"description": PROJECT_NOT_FOUND_DETAIL}, } +PROJECT_MANAGEMENT_BASE_RESPONSES = { + 401: {"description": "Web UI authentication required"}, + 403: {"description": "Web UI access denied or request origin rejected"}, + 415: {"description": "Content-Type must be application/json"}, + 422: {"description": "Invalid mutation request"}, + 500: {"description": "Project/key management unavailable or durable mutation failed"}, +} +CREATE_PROJECT_RESPONSES = { + **PROJECT_MANAGEMENT_BASE_RESPONSES, + 409: {"description": "Project already exists or configuration changed concurrently"}, +} +DELETE_PROJECT_RESPONSES = { + **PROJECT_MANAGEMENT_BASE_RESPONSES, + 404: {"description": PROJECT_NOT_FOUND_DETAIL}, + 409: {"description": "Historical-only project or configuration conflict"}, +} +CREATE_KEY_RESPONSES = { + **PROJECT_MANAGEMENT_BASE_RESPONSES, + 404: {"description": PROJECT_NOT_FOUND_DETAIL}, + 409: {"description": "Configuration conflict"}, +} +DELETE_KEY_RESPONSES = { + **PROJECT_MANAGEMENT_BASE_RESPONSES, + 404: {"description": "Project or key not found"}, + 409: {"description": "Ambiguous key fingerprint or configuration conflict"}, +} + class _StrictMutationBody(BaseModel): model_config = ConfigDict(extra="forbid") @@ -2935,7 +2962,7 @@ def _check_project_mutation_request(request: Request) -> None: raise HTTPException(status_code=403, detail="Origin must exactly match the request origin") -async def _project_key_manager(request: Request): +async def _project_key_manager(): active_container = await _get_active_container(False) if active_container is None: raise HTTPException(status_code=500, detail="Project/key management is unavailable") @@ -2953,6 +2980,9 @@ def _project_mutation_guard(request: Request) -> None: _check_project_mutation_request(request) +ProjectMutationGuard = Annotated[None, Depends(_project_mutation_guard)] + + def _mutation_response(payload: dict[str, Any], status_code: int = 200) -> JSONResponse: return JSONResponse( payload, @@ -2961,13 +2991,16 @@ def _mutation_response(payload: dict[str, Any], status_code: int = 200) -> JSONR ) -@app.post("/api/project-management/projects", status_code=201) +@app.post( + "/api/project-management/projects", + status_code=201, + responses=CREATE_PROJECT_RESPONSES, +) async def create_managed_project( - request: Request, body: ProjectCreateBody, - _guard: None = Depends(_project_mutation_guard), + _guard: ProjectMutationGuard, ): - manager = await _project_key_manager(request) + manager = await _project_key_manager() try: project = await asyncio.to_thread(manager.create_project, body.project_id, body.display_name, body.tags) return _mutation_response({"project": project}, 201) @@ -2975,13 +3008,12 @@ async def create_managed_project( raise _management_error(exc) -@app.delete("/api/project-management/projects") +@app.delete("/api/project-management/projects", responses=DELETE_PROJECT_RESPONSES) async def delete_managed_project( - request: Request, body: ProjectDeleteBody, - _guard: None = Depends(_project_mutation_guard), + _guard: ProjectMutationGuard, ): - manager = await _project_key_manager(request) + manager = await _project_key_manager() try: registry = await asyncio.to_thread(manager.get_registry) if not registry.has_key(body.project_id): @@ -2994,13 +3026,16 @@ async def delete_managed_project( raise _management_error(exc) -@app.post("/api/project-management/keys", status_code=201) +@app.post( + "/api/project-management/keys", + status_code=201, + responses=CREATE_KEY_RESPONSES, +) async def create_managed_key( - request: Request, body: ProjectKeyCreateBody, - _guard: None = Depends(_project_mutation_guard), + _guard: ProjectMutationGuard, ): - manager = await _project_key_manager(request) + manager = await _project_key_manager() try: secret, key_id = await asyncio.to_thread(manager.create_key, body.project_id) return _mutation_response( @@ -3011,13 +3046,12 @@ async def create_managed_key( raise _management_error(exc) -@app.delete("/api/project-management/keys") +@app.delete("/api/project-management/keys", responses=DELETE_KEY_RESPONSES) async def delete_managed_key( - request: Request, body: ProjectKeyDeleteBody, - _guard: None = Depends(_project_mutation_guard), + _guard: ProjectMutationGuard, ): - manager = await _project_key_manager(request) + manager = await _project_key_manager() try: await asyncio.to_thread(manager.revoke_key, body.project_id, body.key_id) return _mutation_response({"deleted": True, "project_id": body.project_id, "key_id": body.key_id}) diff --git a/smolrouter/project_key_manager.py b/smolrouter/project_key_manager.py index 6452e50..ddf61e3 100644 --- a/smolrouter/project_key_manager.py +++ b/smolrouter/project_key_manager.py @@ -3,6 +3,7 @@ from __future__ import annotations import copy +import errno import hashlib import json import os @@ -33,6 +34,12 @@ PROJECT_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._/-]{0,127}$") +PROJECT_NOT_FOUND_MESSAGE = "Project not found" +_DIRECTORY_FSYNC_UNSUPPORTED_ERRNOS = { + errno.EINVAL, + getattr(errno, "ENOTSUP", errno.EINVAL), + getattr(errno, "EOPNOTSUPP", errno.EINVAL), +} _PROCESS_LOCK = threading.RLock() @@ -138,17 +145,19 @@ def _atomic_replace( def _fsync_directory(path: Path) -> None: """Persist a directory entry where the platform supports directory fsync.""" + if os.name == "nt": + return try: directory_fd = os.open(path, os.O_RDONLY) - except OSError: - if os.name == "nt": + except OSError as exc: + if exc.errno in _DIRECTORY_FSYNC_UNSUPPORTED_ERRNOS: return raise try: try: os.fsync(directory_fd) - except OSError: - if os.name != "nt": + except OSError as exc: + if exc.errno not in _DIRECTORY_FSYNC_UNSUPPORTED_ERRNOS: raise finally: os.close(directory_fd) @@ -173,18 +182,24 @@ def _unlock_file(lock_fd: int) -> None: msvcrt.locking(lock_fd, msvcrt.LK_UNLCK, 1) -def _validate_project_id(project_id: Any) -> str: - raw_value = str(project_id or "") - value = raw_value.strip() +def _has_invalid_project_path(value: str) -> bool: segments = value.split("/") - if ( - raw_value != value - or not PROJECT_ID_RE.fullmatch(value) - or value.startswith("/") + return ( + value.startswith("/") or value.endswith("/") or "//" in value or any(segment in {".", ".."} for segment in segments) - ): + ) + + +def _is_valid_new_project_id(raw_value: str, value: str) -> bool: + return raw_value == value and PROJECT_ID_RE.fullmatch(value) is not None and not _has_invalid_project_path(value) + + +def _validate_project_id(project_id: Any) -> str: + raw_value = str(project_id or "") + value = raw_value.strip() + if not _is_valid_new_project_id(raw_value, value): raise ProjectKeyValidationError( "project_id must be 1-128 characters, start with an alphanumeric character, " "and contain only letters, numbers, '.', '_', '-', and well-formed '/' segments" @@ -374,7 +389,7 @@ def delete_project(self, project_id: Any) -> None: routes, keys = self._read_sources() projects = routes.get("facade_keys", {}) if project_id not in projects: - raise ProjectKeyNotFoundError("Project not found") + raise ProjectKeyNotFoundError(PROJECT_NOT_FOUND_MESSAGE) old_keys = copy.deepcopy(keys) projects.pop(project_id) keys.pop(project_id, None) @@ -398,7 +413,7 @@ def create_key(self, project_id: Any, secret: str | None = None) -> tuple[str, s routes, keys = self._read_sources() projects = routes.get("facade_keys", {}) if project_id not in projects: - raise ProjectKeyNotFoundError("Project not found") + raise ProjectKeyNotFoundError(PROJECT_NOT_FOUND_MESSAGE) secret = secret or generate_facade_key_secret() if any(secret in existing for existing in keys.values()): raise ProjectKeyConflictError("Generated key already exists") @@ -420,7 +435,7 @@ def revoke_key(self, project_id: Any, key_id: Any) -> None: routes, keys = self._read_sources() projects = routes.get("facade_keys", {}) if project_id not in projects: - raise ProjectKeyNotFoundError("Project not found") + raise ProjectKeyNotFoundError(PROJECT_NOT_FOUND_MESSAGE) matches = [value for value in keys.get(project_id, []) if secrets.compare_digest(facade_key_id(value), key_id)] if not matches: raise ProjectKeyNotFoundError("Key not found") diff --git a/smolrouter/templates/project_detail.html b/smolrouter/templates/project_detail.html index 79d8678..78fad5d 100644 --- a/smolrouter/templates/project_detail.html +++ b/smolrouter/templates/project_detail.html @@ -162,7 +162,7 @@

API keys

Delete project

Type {{ project.id }} to confirm. Active API keys will be revoked first.

-
+
{% endif %} diff --git a/tests/unit/test_project_key_manager.py b/tests/unit/test_project_key_manager.py index a236946..b9fc04d 100644 --- a/tests/unit/test_project_key_manager.py +++ b/tests/unit/test_project_key_manager.py @@ -1,3 +1,4 @@ +import errno import hashlib import json import multiprocessing @@ -7,6 +8,7 @@ import pytest import yaml +from fastapi import HTTPException import smolrouter.app as app_module import smolrouter.project_key_manager as manager_module @@ -59,6 +61,59 @@ def test_new_project_ids_are_strictly_validated(tmp_path, project_id): manager.create_project(project_id) +@pytest.mark.parametrize( + "project_id", + ["a", "Project_01", "team/project", "team.alpha/project-2", "0/child.name"], +) +def test_new_project_ids_accept_supported_corpus(tmp_path, project_id): + manager, _ = _manager(tmp_path) + assert manager.create_project(project_id)["project_id"] == project_id + + +@pytest.mark.parametrize("phase", ["open", "fsync"]) +def test_directory_fsync_ignores_only_supported_platform_errors(tmp_path, phase): + open_mock = Mock(return_value=42) + fsync_mock = Mock() + close_mock = Mock() + if phase == "open": + open_mock.side_effect = OSError(errno.ENOTSUP, "unsupported") + else: + fsync_mock.side_effect = OSError(errno.EOPNOTSUPP, "unsupported") + with patch.object(manager_module.os, "open", open_mock), patch.object( + manager_module.os, "fsync", fsync_mock + ), patch.object(manager_module.os, "close", close_mock): + manager_module._fsync_directory(tmp_path) + if phase == "open": + fsync_mock.assert_not_called() + close_mock.assert_not_called() + else: + close_mock.assert_called_once_with(42) + + +@pytest.mark.parametrize("phase", ["open", "fsync"]) +def test_directory_fsync_reraises_unexpected_errors(tmp_path, phase): + open_mock = Mock(return_value=42) + fsync_mock = Mock() + close_mock = Mock() + if phase == "open": + open_mock.side_effect = OSError(errno.EACCES, "denied") + else: + fsync_mock.side_effect = OSError(errno.EIO, "failed") + with patch.object(manager_module.os, "open", open_mock), patch.object( + manager_module.os, "fsync", fsync_mock + ), patch.object(manager_module.os, "close", close_mock), pytest.raises(OSError): + manager_module._fsync_directory(tmp_path) + if phase == "fsync": + close_mock.assert_called_once_with(42) + + +def test_directory_fsync_on_windows_returns_before_open(tmp_path): + open_mock = Mock(side_effect=AssertionError("directory open must not run on Windows")) + with patch.object(manager_module.os, "name", "nt"), patch.object(manager_module.os, "open", open_mock): + manager_module._fsync_directory(tmp_path) + open_mock.assert_not_called() + + def test_manager_rejects_symlinked_managed_file(tmp_path): target = tmp_path / "target.yaml" target.write_text("facade_keys: {}\n") @@ -343,6 +398,52 @@ async def active(_legacy): assert manager.get_registry().resolve_secret(payload["secret"]) is not None +@pytest.mark.asyncio +async def test_management_api_returns_500_when_manager_dependency_is_unavailable(async_client): + async def unavailable(_legacy): + return None + + security = Mock() + security.check_webui_access.return_value = None + with patch("smolrouter.app._get_active_container", unavailable), patch( + "smolrouter.app.get_webui_security", return_value=security + ): + response = await async_client.post( + "/api/project-management/projects", + json={"project_id": "project-a"}, + headers={"Origin": "http://test"}, + ) + assert response.status_code == 500 + assert response.json()["detail"] == "Project/key management is unavailable" + + +@pytest.mark.asyncio +async def test_management_api_preserves_webui_authentication_401(async_client): + security = Mock() + security.check_webui_access.side_effect = HTTPException(status_code=401, detail="authentication required") + with patch("smolrouter.app.get_webui_security", return_value=security): + response = await async_client.post( + "/api/project-management/projects", + json={"project_id": "project-a"}, + headers={"Origin": "http://test"}, + ) + assert response.status_code == 401 + + +def test_management_openapi_declares_mutation_error_responses(): + app_module.app.openapi_schema = None + schema = app_module.app.openapi()["paths"] + expected_base = {"401", "403", "415", "422", "500"} + expected_by_operation = { + ("/api/project-management/projects", "post"): {"409"}, + ("/api/project-management/projects", "delete"): {"404", "409"}, + ("/api/project-management/keys", "post"): {"404", "409"}, + ("/api/project-management/keys", "delete"): {"404", "409"}, + } + for (path, method), extra in expected_by_operation.items(): + assert expected_base | extra <= set(schema[path][method]["responses"]) + + @pytest.mark.asyncio @pytest.mark.parametrize( "headers", @@ -549,6 +650,7 @@ async def active(_legacy): detail = await async_client.get("/projects/project-a") assert listing.status_code == 200 and "Create project" in listing.text assert detail.status_code == 200 and "Create API key" in detail.text and "Delete project" in detail.text + assert 'for="delete-confirmation"' in detail.text assert "Copy key" in detail.text assert "navigator.clipboard.writeText" in detail.text assert "Copy failed. Select and copy the key manually" in detail.text From 4967ba429c310a544ef6a448f83d9f88c82e4c0f Mon Sep 17 00:00:00 2001 From: Mitchell Currie Date: Tue, 21 Jul 2026 15:47:37 +1000 Subject: [PATCH 3/4] Address remaining project management review feedback --- smolrouter/facade_key_cli.py | 13 +++- smolrouter/project_key_manager.py | 45 ++++++++---- smolrouter/templates/project_detail.html | 13 ++-- smolrouter/templates/project_mutation.html | 19 +++++ smolrouter/templates/projects.html | 5 +- tests/unit/test_facade_key_cli.py | 84 ++++++++++++++++++++++ tests/unit/test_project_key_manager.py | 67 +++++++++++++++++ 7 files changed, 218 insertions(+), 28 deletions(-) create mode 100644 smolrouter/templates/project_mutation.html diff --git a/smolrouter/facade_key_cli.py b/smolrouter/facade_key_cli.py index 9762b73..a30a9ff 100644 --- a/smolrouter/facade_key_cli.py +++ b/smolrouter/facade_key_cli.py @@ -18,7 +18,7 @@ load_facade_key_secrets_from_path, resolve_facade_key_config_path, ) -from .project_key_manager import ProjectKeyManager +from .project_key_manager import ProjectKeyManagementError, ProjectKeyManager def configure_create_parser(parser: argparse.ArgumentParser) -> argparse.ArgumentParser: parser.add_argument("--routes-config", default=os.getenv("ROUTES_CONFIG")) @@ -120,11 +120,18 @@ def run_create(args: argparse.Namespace) -> int: return 0 manager = ProjectKeyManager(routes_path=routes_path, secrets_path=secret_file_path) - generated, _key_id = manager.create_key(project_id, generated) + try: + generated, _key_id = manager.create_key(project_id, generated) + except (ProjectKeyManagementError, OSError) as exc: + print(f"Failed to create facade key for project {project_id}: {exc}") + return 4 print(f"Generated facade key for project: {project_id}") print(f"secret (copy exactly once): {generated}") print(f"wrote facade-key secrets to: {secret_file_path}") - print(f"secret_count: {len(updated.get(project_id, []))}") + try: + print(f"secret_count: {len(manager.list_keys(project_id))}") + except (ProjectKeyManagementError, OSError) as exc: + print(f"secret_count: unavailable ({exc})") return 0 diff --git a/smolrouter/project_key_manager.py b/smolrouter/project_key_manager.py index ddf61e3..4364b6b 100644 --- a/smolrouter/project_key_manager.py +++ b/smolrouter/project_key_manager.py @@ -207,23 +207,39 @@ def _validate_project_id(project_id: Any) -> str: return value +def _normalize_display_name(display_name: Any) -> str: + if not isinstance(display_name, str): + raise ProjectKeyValidationError("display_name must be a non-empty string of at most 100 characters") + normalized = display_name.strip() + if not normalized or len(normalized) > 100: + raise ProjectKeyValidationError("display_name must be a non-empty string of at most 100 characters") + return normalized + + +def _normalize_tag(tag: Any) -> str: + if not isinstance(tag, str): + raise ProjectKeyValidationError("each tag must be a non-empty string of at most 40 characters") + normalized = tag.strip() + if not normalized or len(normalized) > 40: + raise ProjectKeyValidationError("each tag must be a non-empty string of at most 40 characters") + return normalized + + +def _normalize_tags(tags: Any) -> list[str]: + if not isinstance(tags, list) or len(tags) > 20: + raise ProjectKeyValidationError("tags must be a list containing at most 20 values") + normalized = [_normalize_tag(tag) for tag in tags] + if len(normalized) != len(set(normalized)): + raise ProjectKeyValidationError("tags must be unique") + return normalized + + def _project_config(display_name: Any = None, tags: Any = None) -> dict[str, Any]: result: dict[str, Any] = {"enabled": True} if display_name is not None: - if not isinstance(display_name, str) or not display_name.strip() or len(display_name.strip()) > 100: - raise ProjectKeyValidationError("display_name must be a non-empty string of at most 100 characters") - result["display_name"] = display_name.strip() + result["display_name"] = _normalize_display_name(display_name) if tags is not None: - if not isinstance(tags, list) or len(tags) > 20: - raise ProjectKeyValidationError("tags must be a list containing at most 20 values") - cleaned: list[str] = [] - for tag in tags: - if not isinstance(tag, str) or not tag.strip() or len(tag.strip()) > 40: - raise ProjectKeyValidationError("each tag must be a non-empty string of at most 40 characters") - if tag.strip() in cleaned: - raise ProjectKeyValidationError("tags must be unique") - cleaned.append(tag.strip()) - result["tags"] = cleaned + result["tags"] = _normalize_tags(tags) return result @@ -265,6 +281,9 @@ def revision(self) -> tuple[Any, ...]: def _read_sources(self) -> tuple[dict[str, Any], dict[str, list[str]]]: routes = _read_mapping(self.routes_path) raw_projects = routes.get("facade_keys", {}) + if raw_projects is None: + raw_projects = {} + routes["facade_keys"] = raw_projects if not isinstance(raw_projects, dict): raise ProjectKeyManagementError("routes facade_keys must be a mapping") _reject_symlink(self.secrets_path) diff --git a/smolrouter/templates/project_detail.html b/smolrouter/templates/project_detail.html index 78fad5d..42f6cf3 100644 --- a/smolrouter/templates/project_detail.html +++ b/smolrouter/templates/project_detail.html @@ -241,6 +241,7 @@

Recent Facade-Key Requests

+ {% include 'project_mutation.html' %} {% if project.configured %} diff --git a/smolrouter/templates/projects.html b/smolrouter/templates/projects.html index 69e4f91..d96a52d 100644 --- a/smolrouter/templates/projects.html +++ b/smolrouter/templates/projects.html @@ -201,6 +201,7 @@

Facade-Key Projects

+ {% include 'project_mutation.html' %}