BBergleandClaude Opus 5
ddea750792
Phase 0's API half: a working FastAPI app with register/login/me/logout, backed by a Postgres schema where row-level security is real and independently proven, not just declared. The core design decision, and the reason this lands as one PR instead of several: request-scoped queries run as `velodrome_app` (NOBYPASSRLS), but looking up identity in the first place — login by email, a session by its token hash — has to happen *before* app.user_id can be set, so those specific lookups run as a second role, `velodrome_auth` (BYPASSRLS), used nowhere else in the codebase. See velodrome/db.py's module docstring and apps/api/README.md for the full rationale. This is genuinely one reviewable unit: the migration, the models, and the auth service only make sense evaluated together, since they're three views of the same invariant. tests/test_auth.py::test_rls_blocks_cross_user_session_reads is the test worth reading first — it doesn't trust the RLS policy SQL because it reads correctly, it proves isolation by registering two users and confirming a scoped read of `sessions` for user A returns exactly one row, never two. Bugs found and fixed while actually running this against real Postgres (everything below was verified against a live postgis/postgis:16-3.4 container and a built Docker image, not just read for correctness): - CREATE ROLE's PASSWORD clause is DDL, not DML — it doesn't accept bind parameters (`PASSWORD $1` is a syntax error). Fixed with dollar-quoting. - Postgres roles are cluster-wide, not per-database — a second database in the same cluster hit "role already exists" on a plain CREATE ROLE. Fixed with a DO block catching duplicate_object. - A bare `Mapped[datetime]` on the ORM models infers a naive timestamp, silently disagreeing with the migration's correct `DateTime(timezone=True)` — asyncpg rejected the mismatch at insert time. Fixed once, at the declarative Base level via type_annotation_map, rather than per-column. - The session cookie's `secure` flag was gated on `!= "development"`, so anything else — including local testing and a real deploy running temporarily without TLS in front — got a Secure cookie no HTTP client will ever send back, breaking every authenticated request after login with no visible error. Gated on `== "production"` instead. - `alembic check` initially flagged every PostGIS/TIGER-installed table (dozens of them) as drift, because they're not in our metadata. A schema-based denylist doesn't work — reflected foreign tables come back with schema=None regardless of their real schema. Fixed with an allowlist keyed on target_metadata.tables instead, which is also more robust against future PostGIS versions adding more tables. - The migration itself was missing `nullable=False` on three timestamp columns that the ORM model assumed were never null — a genuine model/migration drift that alembic check caught once the PostGIS noise above was filtered out. Fixed in 0001 directly, since it's never shipped. - Two indexes the migration creates explicitly weren't declared on the ORM models, causing the same kind of drift. Added index=True to match. Deliberately deferred, not forgotten: per-IP/per-account login rate limiting (docs/PLAN.md mentions it; Phase 0's bar is a working skeleton, and this needs its own design pass) and the procrastinate job runner / worker container (nothing to run yet — arrives with the ingestion pipeline). ci.yml updated to match: the api and migrations jobs now provision the same two runtime roles this code actually needs, replacing the single placeholder DATABASE_URL from before any code existed. Verified: ruff check, ruff format --check, and mypy --strict all clean. 12/12 pytest passing against a real Postgres. Full alembic upgrade -> downgrade -1 -> upgrade cycle run twice (once standalone, once inside a two-database cluster to specifically catch the role-collision bug). alembic check clean. Docker image builds and serves real traffic — register and an authenticated GET /me both exercised against the actual built container, not just the test suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
90 lines
3.4 KiB
Python
90 lines
3.4 KiB
Python
"""Alembic environment.
|
|
|
|
Deliberately independent of velodrome.config.Settings: migrations run as a privileged/owner
|
|
connection (VELODROME_DATABASE_URL_MIGRATE — a superuser or schema-owning role, which trivially
|
|
satisfies "BYPASSRLS" since superusers always bypass RLS), never as either of the two runtime
|
|
roles the app itself uses. Reading env vars directly here, rather than importing the app's
|
|
settings, keeps a migration-only CI job from needing every runtime env var the app requires.
|
|
"""
|
|
|
|
import asyncio
|
|
import os
|
|
|
|
import sqlalchemy as sa
|
|
from sqlalchemy.engine import Connection
|
|
from sqlalchemy.ext.asyncio import async_engine_from_config
|
|
|
|
from alembic import context
|
|
from velodrome.models import Base
|
|
|
|
config = context.config
|
|
target_metadata = Base.metadata
|
|
|
|
|
|
# postgis/postgis ships an entire pre-installed schema of its own (PostGIS core tables plus the
|
|
# TIGER geocoder's tiger/topology schemas — dozens of tables) that Alembic never created and
|
|
# doesn't manage. Without this filter, `alembic check`/autogenerate sees every single one as
|
|
# "should be dropped" simply because it's not in our SQLAlchemy metadata — which would make CI's
|
|
# `alembic check` step permanently useless (always red, for reasons that have nothing to do with
|
|
# an actual drift).
|
|
#
|
|
# A denylist keyed on schema name is NOT reliable here: reflected foreign tables can come back
|
|
# with `schema=None` on their Table object regardless of which schema they actually live in on
|
|
# the server (confirmed against a real postgis/postgis:16-3.4 container — tables that `\dt`
|
|
# clearly shows under the `tiger` schema still reflect with schema=None). An allowlist is the
|
|
# robust version of the same idea: only ever compare tables OUR metadata declares, so a future
|
|
# PostGIS/TIGER version adding more foreign tables can never cause a false positive here.
|
|
def include_object(
|
|
object: sa.schema.SchemaItem, name: str | None, type_: str, reflected: bool, compare_to: object
|
|
) -> bool:
|
|
if type_ == "table":
|
|
return name in target_metadata.tables
|
|
return True
|
|
|
|
|
|
def _migrate_url() -> str:
|
|
url = os.environ.get("VELODROME_DATABASE_URL_MIGRATE")
|
|
if not url:
|
|
# Local-dev convenience only — every real environment (CI, deploy/) sets this explicitly.
|
|
url = "postgresql+asyncpg://postgres:postgres@localhost:5432/velodrome"
|
|
return url
|
|
|
|
|
|
def run_migrations_offline() -> None:
|
|
context.configure(
|
|
url=_migrate_url(),
|
|
target_metadata=target_metadata,
|
|
literal_binds=True,
|
|
dialect_opts={"paramstyle": "named"},
|
|
include_object=include_object,
|
|
)
|
|
with context.begin_transaction():
|
|
context.run_migrations()
|
|
|
|
|
|
def _do_run_migrations(connection: Connection) -> None:
|
|
context.configure(
|
|
connection=connection,
|
|
target_metadata=target_metadata,
|
|
include_object=include_object,
|
|
)
|
|
with context.begin_transaction():
|
|
context.run_migrations()
|
|
|
|
|
|
async def run_migrations_online() -> None:
|
|
configuration = config.get_section(config.config_ini_section) or {}
|
|
configuration["sqlalchemy.url"] = _migrate_url()
|
|
connectable = async_engine_from_config(configuration, prefix="sqlalchemy.")
|
|
|
|
async with connectable.connect() as connection:
|
|
await connection.run_sync(_do_run_migrations)
|
|
|
|
await connectable.dispose()
|
|
|
|
|
|
if context.is_offline_mode():
|
|
run_migrations_offline()
|
|
else:
|
|
asyncio.run(run_migrations_online())
|