feat(security): Phase 7c — app-side API-Key authentication with 3-scope keys + rate-limit + admin UI #88
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| """Phase 7c — api_keys table + audit columns on jobs + bootstrap-admin seed. | ||
|
|
||
| Revision ID: 20260517_phase7c_api_keys | ||
| Revises: 20260517_phase7b_datetime_tz | ||
| Create Date: 2026-05-17 | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import logging | ||
| import secrets | ||
|
|
||
| import bcrypt | ||
| from alembic import op | ||
| import sqlalchemy as sa | ||
|
|
||
| revision = "20260517_phase7c_api_keys" | ||
| down_revision = "20260517_phase7b_datetime_tz" | ||
| branch_labels = None | ||
| depends_on = None | ||
|
|
||
| _log = logging.getLogger(__name__) | ||
| _BOOTSTRAP_KEY_NAME = "bootstrap-admin" | ||
|
|
||
|
|
||
| def _generate_bootstrap_key() -> tuple[str, str, str]: | ||
| body = secrets.token_urlsafe(32) | ||
| plaintext = f"lh_{body}" | ||
| prefix = plaintext[:12] | ||
| hashed = bcrypt.hashpw(plaintext.encode(), bcrypt.gensalt(rounds=12)).decode() | ||
| return plaintext, prefix, hashed | ||
|
|
||
|
|
||
| def upgrade() -> None: | ||
| op.create_table( | ||
| "api_keys", | ||
| sa.Column("id", sa.Uuid, primary_key=True), | ||
| sa.Column("name", sa.String, nullable=False), | ||
| sa.Column("key_hash", sa.String, nullable=False), | ||
| sa.Column("key_prefix", sa.String, nullable=False), | ||
| sa.Column("scopes", sa.JSON, nullable=False), | ||
| sa.Column("allowed_printer_ids", sa.JSON, nullable=False), | ||
| sa.Column("rate_limit_per_minute", sa.Integer, nullable=False, server_default="60"), | ||
| sa.Column("enabled", sa.Boolean, nullable=False, server_default="1"), | ||
| sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), | ||
| sa.Column("last_used_at", sa.DateTime(timezone=True), nullable=True), | ||
| sa.Column("last_used_ip", sa.String, nullable=True), | ||
| sa.Column("expires_at", sa.DateTime(timezone=True), nullable=True), | ||
| sa.Column("notes", sa.String, nullable=True), | ||
| ) | ||
| op.create_index("ix_api_keys_name", "api_keys", ["name"]) | ||
| op.create_index("ix_api_keys_key_prefix", "api_keys", ["key_prefix"]) | ||
|
|
||
| with op.batch_alter_table("jobs") as batch_op: | ||
| batch_op.add_column(sa.Column("api_key_id", sa.Uuid, nullable=True)) | ||
| batch_op.add_column(sa.Column("source_ip", sa.String, nullable=True)) | ||
| op.create_index("ix_jobs_api_key_id", "jobs", ["api_key_id"]) | ||
|
|
||
| conn = op.get_bind() | ||
| count = conn.execute(sa.text("SELECT COUNT(*) FROM api_keys")).scalar() | ||
| if count == 0: | ||
| from datetime import UTC, datetime | ||
| from uuid import uuid4 | ||
| plaintext, prefix, hashed = _generate_bootstrap_key() | ||
| key_id = str(uuid4()) | ||
| now = datetime.now(UTC).isoformat() | ||
| conn.execute( | ||
| sa.text( | ||
| "INSERT INTO api_keys " | ||
| "(id, name, key_hash, key_prefix, scopes, allowed_printer_ids, " | ||
| " rate_limit_per_minute, enabled, created_at) " | ||
| "VALUES (:id, :name, :hash, :prefix, :scopes, :printers, " | ||
| " :rate, :enabled, :now)" | ||
| ), | ||
| { | ||
| "id": key_id, "name": _BOOTSTRAP_KEY_NAME, "hash": hashed, | ||
| "prefix": prefix, "scopes": json.dumps(["admin"]), | ||
| "printers": json.dumps([]), "rate": 60, "enabled": 1, "now": now, | ||
| }, | ||
| ) | ||
| _log.warning("BOOTSTRAP API KEY: %s (prefix: %s) — rotate via /admin/api-keys", plaintext, prefix) | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| op.drop_index("ix_jobs_api_key_id", table_name="jobs") | ||
| with op.batch_alter_table("jobs") as batch_op: | ||
| batch_op.drop_column("source_ip") | ||
| batch_op.drop_column("api_key_id") | ||
| op.drop_index("ix_api_keys_key_prefix", table_name="api_keys") | ||
| op.drop_index("ix_api_keys_name", table_name="api_keys") | ||
| op.drop_table("api_keys") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,243 @@ | ||||||||
| """REST CRUD endpoints for API key management — Phase 7c Step 8. | ||||||||
|
|
||||||||
| All endpoints require ``admin`` scope. | ||||||||
|
|
||||||||
| Routes | ||||||||
| ------ | ||||||||
| GET /api/admin/api-keys — list all keys (metadata only, no hashes/plaintexts) | ||||||||
| POST /api/admin/api-keys — create key, returns plaintext ONCE in response | ||||||||
| GET /api/admin/api-keys/{id} — single key metadata | ||||||||
| PATCH /api/admin/api-keys/{id} — update enabled/rate_limit/notes | ||||||||
| DELETE /api/admin/api-keys/{id} — revoke key (sets enabled=False) | ||||||||
| """ | ||||||||
|
|
||||||||
| from __future__ import annotations | ||||||||
|
|
||||||||
| from typing import Annotated | ||||||||
| from uuid import UUID | ||||||||
|
|
||||||||
| from fastapi import APIRouter, Depends, HTTPException, status | ||||||||
| from pydantic import BaseModel | ||||||||
| from sqlalchemy.ext.asyncio import AsyncSession | ||||||||
|
|
||||||||
| from app.auth.dependencies import AuthContext | ||||||||
| from app.auth.key_generator import generate_api_key | ||||||||
| from app.auth.scope_deps import require_admin | ||||||||
| from app.auth.verifier import invalidate_cache | ||||||||
| from app.db.session import get_session | ||||||||
| from app.models.api_key import ApiKey | ||||||||
| from app.repositories import api_keys as api_keys_repo | ||||||||
| from app.services.rate_limiter import _rate_limiter | ||||||||
|
|
||||||||
| router = APIRouter(prefix="/api/admin/api-keys", tags=["admin"]) | ||||||||
|
|
||||||||
| SessionDep = Annotated[AsyncSession, Depends(get_session)] | ||||||||
| AdminAuthDep = Annotated[AuthContext, Depends(require_admin)] | ||||||||
|
|
||||||||
|
|
||||||||
| # --------------------------------------------------------------------------- | ||||||||
| # Request / Response schemas | ||||||||
| # --------------------------------------------------------------------------- | ||||||||
|
|
||||||||
|
|
||||||||
| class ApiKeyCreate(BaseModel): | ||||||||
| name: str | ||||||||
| scopes: list[str] | ||||||||
| allowed_printer_ids: list[str] = [] | ||||||||
| rate_limit_per_minute: int = 60 | ||||||||
| notes: str | None = None | ||||||||
| expires_at: str | None = None # ISO-8601 string or null | ||||||||
|
|
||||||||
|
Comment on lines
+45
to
+52
|
||||||||
|
|
||||||||
| class ApiKeyCreateResponse(BaseModel): | ||||||||
| """Returned ONCE on creation — includes plaintext. Never return again.""" | ||||||||
|
|
||||||||
| key_id: UUID | ||||||||
| plaintext: str | ||||||||
| prefix: str | ||||||||
| name: str | ||||||||
| scopes: list[str] | ||||||||
|
|
||||||||
|
|
||||||||
| class ApiKeyRead(BaseModel): | ||||||||
| """Metadata-only view — no key_hash, no plaintext.""" | ||||||||
|
|
||||||||
| id: UUID | ||||||||
| name: str | ||||||||
| key_prefix: str | ||||||||
| scopes: list[str] | ||||||||
| allowed_printer_ids: list[str] | ||||||||
| rate_limit_per_minute: int | ||||||||
| enabled: bool | ||||||||
| created_at: str | ||||||||
| last_used_at: str | None | ||||||||
| last_used_ip: str | None | ||||||||
| expires_at: str | None | ||||||||
| notes: str | None | ||||||||
|
|
||||||||
|
|
||||||||
| class ApiKeyPatch(BaseModel): | ||||||||
| enabled: bool | None = None | ||||||||
| rate_limit_per_minute: int | None = None | ||||||||
| notes: str | None = None | ||||||||
| allowed_printer_ids: list[str] | None = None | ||||||||
|
|
||||||||
|
|
||||||||
| def _key_to_read(key: ApiKey) -> ApiKeyRead: | ||||||||
| return ApiKeyRead( | ||||||||
| id=key.id, | ||||||||
| name=key.name, | ||||||||
| key_prefix=key.key_prefix, | ||||||||
| scopes=key.scopes, | ||||||||
| allowed_printer_ids=key.allowed_printer_ids, | ||||||||
| rate_limit_per_minute=key.rate_limit_per_minute, | ||||||||
| enabled=key.enabled, | ||||||||
| created_at=key.created_at.isoformat() if key.created_at else "", | ||||||||
| last_used_at=key.last_used_at.isoformat() if key.last_used_at else None, | ||||||||
| last_used_ip=key.last_used_ip, | ||||||||
| expires_at=key.expires_at.isoformat() if key.expires_at else None, | ||||||||
| notes=key.notes, | ||||||||
| ) | ||||||||
|
|
||||||||
|
|
||||||||
| # --------------------------------------------------------------------------- | ||||||||
| # Routes | ||||||||
| # --------------------------------------------------------------------------- | ||||||||
|
|
||||||||
|
|
||||||||
| @router.get( | ||||||||
| "", | ||||||||
| response_model=list[ApiKeyRead], | ||||||||
| summary="List all API keys", | ||||||||
| description="Returns metadata for all API keys. key_hash and plaintext are never included.", | ||||||||
| ) | ||||||||
| async def list_api_keys(session: SessionDep, _auth: AdminAuthDep) -> list[ApiKeyRead]: | ||||||||
| result = await session.execute(__import__("sqlalchemy", fromlist=["select"]).select(ApiKey)) | ||||||||
|
There was a problem hiding this comment. Using
Suggested change
Owner
Author
There was a problem hiding this comment. Fixed in 7e0b1e4 (Fix D): replaced with top-level |
||||||||
| keys = list(result.scalars()) | ||||||||
| return [_key_to_read(k) for k in keys] | ||||||||
|
Comment on lines
+116
to
+119
|
||||||||
|
|
||||||||
|
|
||||||||
| @router.post( | ||||||||
| "", | ||||||||
| response_model=ApiKeyCreateResponse, | ||||||||
| status_code=status.HTTP_201_CREATED, | ||||||||
| summary="Create a new API key", | ||||||||
| description=( | ||||||||
| "Creates a new API key. The ``plaintext`` field in the response is the " | ||||||||
| "full key — it is shown ONCE and never stored. Copy it before closing " | ||||||||
| "this response. Subsequent GETs return only the prefix." | ||||||||
| ), | ||||||||
| ) | ||||||||
| async def create_api_key( | ||||||||
| body: ApiKeyCreate, | ||||||||
| session: SessionDep, | ||||||||
| _auth: AdminAuthDep, | ||||||||
| ) -> ApiKeyCreateResponse: | ||||||||
| plaintext, prefix, hashed = generate_api_key() | ||||||||
| key = ApiKey( | ||||||||
| name=body.name, | ||||||||
| key_hash=hashed, | ||||||||
| key_prefix=prefix, | ||||||||
| scopes=body.scopes, | ||||||||
| allowed_printer_ids=body.allowed_printer_ids, | ||||||||
| rate_limit_per_minute=body.rate_limit_per_minute, | ||||||||
| notes=body.notes, | ||||||||
| enabled=True, | ||||||||
| ) | ||||||||
| if body.expires_at: | ||||||||
| from datetime import datetime | ||||||||
|
|
||||||||
| key.expires_at = datetime.fromisoformat(body.expires_at) | ||||||||
|
|
||||||||
|
|
||||||||
| created = await api_keys_repo.create(session, key) | ||||||||
| return ApiKeyCreateResponse( | ||||||||
| key_id=created.id, | ||||||||
| plaintext=plaintext, | ||||||||
| prefix=prefix, | ||||||||
| name=created.name, | ||||||||
| scopes=created.scopes, | ||||||||
| ) | ||||||||
|
|
||||||||
|
|
||||||||
| @router.get( | ||||||||
| "/{key_id}", | ||||||||
| response_model=ApiKeyRead, | ||||||||
| summary="Get API key metadata", | ||||||||
| description="Returns metadata for a single API key. key_hash and plaintext are never included.", | ||||||||
| ) | ||||||||
| async def get_api_key( | ||||||||
| key_id: UUID, | ||||||||
| session: SessionDep, | ||||||||
| _auth: AdminAuthDep, | ||||||||
| ) -> ApiKeyRead: | ||||||||
| key = await api_keys_repo.get(session, key_id) | ||||||||
| if key is None: | ||||||||
| raise HTTPException( | ||||||||
| status_code=status.HTTP_404_NOT_FOUND, | ||||||||
| detail=f"API key {key_id} not found", | ||||||||
| ) | ||||||||
|
Comment on lines
+174
to
+176
There was a problem hiding this comment. The error response format here (a plain string detail) is inconsistent with the structured dictionary format used in |
||||||||
| return _key_to_read(key) | ||||||||
|
|
||||||||
|
|
||||||||
| @router.patch( | ||||||||
| "/{key_id}", | ||||||||
| response_model=ApiKeyRead, | ||||||||
| summary="Update API key metadata", | ||||||||
| description=( | ||||||||
| "Update ``enabled``, ``rate_limit_per_minute``, ``notes``, or " | ||||||||
| "``allowed_printer_ids``. Cannot change scopes or the key value itself — " | ||||||||
| "revoke and recreate for that." | ||||||||
| ), | ||||||||
| ) | ||||||||
| async def update_api_key( | ||||||||
| key_id: UUID, | ||||||||
| body: ApiKeyPatch, | ||||||||
| session: SessionDep, | ||||||||
| _auth: AdminAuthDep, | ||||||||
| ) -> ApiKeyRead: | ||||||||
| key = await api_keys_repo.get(session, key_id) | ||||||||
| if key is None: | ||||||||
| raise HTTPException( | ||||||||
| status_code=status.HTTP_404_NOT_FOUND, | ||||||||
| detail=f"API key {key_id} not found", | ||||||||
| ) | ||||||||
| if body.enabled is not None: | ||||||||
| key.enabled = body.enabled | ||||||||
| if body.rate_limit_per_minute is not None: | ||||||||
| key.rate_limit_per_minute = body.rate_limit_per_minute | ||||||||
| if body.notes is not None: | ||||||||
| key.notes = body.notes | ||||||||
| if body.allowed_printer_ids is not None: | ||||||||
| key.allowed_printer_ids = body.allowed_printer_ids | ||||||||
|
|
||||||||
| session.add(key) | ||||||||
| await session.commit() | ||||||||
| await session.refresh(key) | ||||||||
| return _key_to_read(key) | ||||||||
|
|
||||||||
|
|
||||||||
| @router.delete( | ||||||||
| "/{key_id}", | ||||||||
| status_code=status.HTTP_204_NO_CONTENT, | ||||||||
| summary="Revoke an API key", | ||||||||
| description=( | ||||||||
| "Sets ``enabled = False``. The key will be rejected on next use. " | ||||||||
| "The row is kept for audit purposes (jobs referencing this key_id " | ||||||||
| "remain intact)." | ||||||||
| ), | ||||||||
| ) | ||||||||
| async def revoke_api_key( | ||||||||
| key_id: UUID, | ||||||||
| session: SessionDep, | ||||||||
| _auth: AdminAuthDep, | ||||||||
| ) -> None: | ||||||||
| key = await api_keys_repo.revoke(session, key_id) | ||||||||
| if key is None: | ||||||||
| raise HTTPException( | ||||||||
| status_code=status.HTTP_404_NOT_FOUND, | ||||||||
| detail=f"API key {key_id} not found", | ||||||||
| ) | ||||||||
| # Invalidate bcrypt cache so the key is rejected immediately | ||||||||
| invalidate_cache(key.key_hash) | ||||||||
| # Clear rate-limiter bucket | ||||||||
| _rate_limiter.reset(key_id) | ||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ApiKeyCreatemodel should use thedatetimetype forexpires_atand include validation forrate_limit_per_minute. Usingstrfor dates requires manual parsing and bypasses Pydantic's automatic validation and error reporting (422 Unprocessable Entity). Additionally, a non-positive rate limit could cause a division-by-zero error in the rate limiter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7e0b1e4 (Fix E):
expires_atis nowdatetime | None— Pydantic parses and validates at request time, returning 422 on invalid input.rate_limit_per_minutenow usesField(ge=1, le=10000).