Phase 0's API half (see docs/PLAN.md): a working FastAPI app — register, login, /me, logout —
backed by a Postgres schema where row-level security is real and independently proven, not just
declared in a migration comment.
The core design decision: request-scoped queries run as velodrome_app (NOBYPASSRLS), but
identity has to be established beforeapp.user_id can be set for RLS to apply — so the handful
of pre-identity lookups (login by email, session by token hash, invite by code hash) run as a
second role, velodrome_auth (BYPASSRLS), used nowhere else. Full rationale in velodrome/db.py's module docstring and apps/api/README.md.
This lands as one PR rather than several because the migration, the models, and the auth service
are three views of the same invariant — reviewing them apart wouldn't actually be easier, just
disconnected. Diff is ~1,700 lines of authored content (the rest is the uv.lock lockfile), over
the usual ~400-line guideline in CLAUDE.md — flagging that honestly rather than pretending
otherwise.
How this was verified
Not just "tests pass" — everything below was run for real against a live postgis/postgis:16-3.4
container and a built Docker image, which is how 7 real bugs got caught before they'd have hit CI
(details in the commit message):
ruff check, ruff format --check, mypy --strict — all clean
pytest — 12/12 passing against real Postgres, no mocked DB behaviour
Full alembic upgrade head -> downgrade -1 -> upgrade head cycle, run twice: once standalone,
once inside a two-database cluster specifically to catch a Postgres-roles-are-cluster-wide bug
that only shows up that way
alembic check — clean, after fixing both the PostGIS/TIGER false-positive noise and two genuine
model/migration drifts it surfaced once that noise was filtered out
Docker image builds; the actual container was run against real Postgres and exercised over real
HTTP — /healthz, /api/v1/openapi.json, a full register → cookie → authenticated GET /me
round trip — not just the test suite in isolation
CI is green (pending this push)
Tests added for the behaviour that changed
Verified manually — see above; this one leaned unusually hard on manual verification because
auth/RLS is exactly the area CLAUDE.md flags as "a mistake exposes another user's data"
Invariants
Raw ingested bytes remain immutable; derived tables stay rebuildable — n/a, no ingestion yet
No stored odometer added; wear still derived from installs — n/a, no components yet
Physical quantities stored as SI integers — n/a this PR, but users.unit_system defaults to imperial for display only, per the invariant
New user-owned tables have user_id + RLS policy + repository scope — sessions and api_tokens both do; users itself has an own-row policy (see PR body below for why login
still works); invites is scoped by created_by
No secret can reach a response model, log line, or error message — password_hash/ token_hash/code_hash never appear in any schemas/ model; verified by reading every
response schema, not just by convention
One deliberate deviation worth a second look:users itself is not scoped by the same
"every user-owned table has an RLS policy restricting it to its own user" pattern in the usual
sense — it has an own-row id = current_setting('app.user_id') policy, but the login flow (by
definition, pre-authentication) reads it via the velodrome_auth bypass role instead. This is
necessary — there's no way to scope a lookup-by-email query to a user_id you don't have yet — but
it's exactly the kind of auth-adjacent judgment call CLAUDE.md says should get scrutiny rather
than a rubber stamp.
Risks and follow-ups
Deliberately deferred, not forgotten: per-IP/per-account login rate limiting (docs/PLAN.md
mentions it; needs its own design pass rather than a bolt-on here) and the procrastinate
worker container (nothing to run yet — arrives with the ingestion pipeline).
ci.yml's api and migrations jobs were updated to provision the two runtime roles this code
actually needs, replacing the single placeholder DATABASE_URL that existed before any real
code did. Touches .gitea/workflows/ci.yml outside apps/api/ — necessary since the old env
vars would never have worked against this schema, not scope creep.
apps/api/README.md has the full "why two database connections" explanation for anyone touching
this later, including the exact environment variables and a local dev quickstart.
## What and why
Phase 0's API half (see `docs/PLAN.md`): a working FastAPI app — register, login, `/me`, logout —
backed by a Postgres schema where row-level security is real and independently proven, not just
declared in a migration comment.
**The core design decision:** request-scoped queries run as `velodrome_app` (`NOBYPASSRLS`), but
identity has to be established *before* `app.user_id` can be set for RLS to apply — so the handful
of pre-identity lookups (login by email, session by token hash, invite by code hash) run as a
second role, `velodrome_auth` (`BYPASSRLS`), used nowhere else. Full rationale in
`velodrome/db.py`'s module docstring and `apps/api/README.md`.
This lands as one PR rather than several because the migration, the models, and the auth service
are three views of the same invariant — reviewing them apart wouldn't actually be easier, just
disconnected. Diff is ~1,700 lines of authored content (the rest is the `uv.lock` lockfile), over
the usual ~400-line guideline in `CLAUDE.md` — flagging that honestly rather than pretending
otherwise.
## How this was verified
Not just "tests pass" — everything below was run for real against a live `postgis/postgis:16-3.4`
container and a built Docker image, which is how 7 real bugs got caught before they'd have hit CI
(details in the commit message):
- `ruff check`, `ruff format --check`, `mypy --strict` — all clean
- `pytest` — 12/12 passing against real Postgres, no mocked DB behaviour
- Full `alembic upgrade head -> downgrade -1 -> upgrade head` cycle, run **twice**: once standalone,
once inside a two-database cluster specifically to catch a Postgres-roles-are-cluster-wide bug
that only shows up that way
- `alembic check` — clean, after fixing both the PostGIS/TIGER false-positive noise and two genuine
model/migration drifts it surfaced once that noise was filtered out
- Docker image builds; the actual container was run against real Postgres and exercised over real
HTTP — `/healthz`, `/api/v1/openapi.json`, a full register → cookie → authenticated `GET /me`
round trip — not just the test suite in isolation
- [x] CI is green (pending this push)
- [x] Tests added for the behaviour that changed
- [x] Verified manually — see above; this one leaned unusually hard on manual verification because
auth/RLS is exactly the area `CLAUDE.md` flags as "a mistake exposes another user's data"
## Invariants
- [ ] Raw ingested bytes remain immutable; derived tables stay rebuildable — n/a, no ingestion yet
- [ ] No stored odometer added; wear still derived from installs — n/a, no components yet
- [x] Physical quantities stored as SI integers — n/a this PR, but `users.unit_system` defaults to
`imperial` for *display only*, per the invariant
- [x] New user-owned tables have `user_id` + RLS policy + repository scope — `sessions` and
`api_tokens` both do; `users` itself has an own-row policy (see PR body below for why login
still works); `invites` is scoped by `created_by`
- [x] No secret can reach a response model, log line, or error message — `password_hash`/
`token_hash`/`code_hash` never appear in any `schemas/` model; verified by reading every
response schema, not just by convention
- [x] Migration survives `upgrade -> downgrade -1 -> upgrade` — verified twice, see above
**One deliberate deviation worth a second look:** `users` itself is *not* scoped by the same
"every user-owned table has an RLS policy restricting it to its own user" pattern in the usual
sense — it has an own-row `id = current_setting('app.user_id')` policy, but the login flow (by
definition, pre-authentication) reads it via the `velodrome_auth` bypass role instead. This is
necessary — there's no way to scope a lookup-by-email query to a user_id you don't have yet — but
it's exactly the kind of auth-adjacent judgment call `CLAUDE.md` says should get scrutiny rather
than a rubber stamp.
## Risks and follow-ups
- **Deliberately deferred, not forgotten:** per-IP/per-account login rate limiting (`docs/PLAN.md`
mentions it; needs its own design pass rather than a bolt-on here) and the `procrastinate`
worker container (nothing to run yet — arrives with the ingestion pipeline).
- `ci.yml`'s `api` and `migrations` jobs were updated to provision the two runtime roles this code
actually needs, replacing the single placeholder `DATABASE_URL` that existed before any real
code did. Touches `.gitea/workflows/ci.yml` outside `apps/api/` — necessary since the old env
vars would never have worked against this schema, not scope creep.
- `apps/api/README.md` has the full "why two database connections" explanation for anyone touching
this later, including the exact environment variables and a local dev quickstart.
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>
BBergle
merged commit ea6e17d39f into main2026-09-21 08:26:12 -04:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
What and why
Phase 0's API half (see
docs/PLAN.md): a working FastAPI app — register, login,/me, logout —backed by a Postgres schema where row-level security is real and independently proven, not just
declared in a migration comment.
The core design decision: request-scoped queries run as
velodrome_app(NOBYPASSRLS), butidentity has to be established before
app.user_idcan be set for RLS to apply — so the handfulof pre-identity lookups (login by email, session by token hash, invite by code hash) run as a
second role,
velodrome_auth(BYPASSRLS), used nowhere else. Full rationale invelodrome/db.py's module docstring andapps/api/README.md.This lands as one PR rather than several because the migration, the models, and the auth service
are three views of the same invariant — reviewing them apart wouldn't actually be easier, just
disconnected. Diff is ~1,700 lines of authored content (the rest is the
uv.locklockfile), overthe usual ~400-line guideline in
CLAUDE.md— flagging that honestly rather than pretendingotherwise.
How this was verified
Not just "tests pass" — everything below was run for real against a live
postgis/postgis:16-3.4container and a built Docker image, which is how 7 real bugs got caught before they'd have hit CI
(details in the commit message):
ruff check,ruff format --check,mypy --strict— all cleanpytest— 12/12 passing against real Postgres, no mocked DB behaviourFull
alembic upgrade head -> downgrade -1 -> upgrade headcycle, run twice: once standalone,once inside a two-database cluster specifically to catch a Postgres-roles-are-cluster-wide bug
that only shows up that way
alembic check— clean, after fixing both the PostGIS/TIGER false-positive noise and two genuinemodel/migration drifts it surfaced once that noise was filtered out
Docker image builds; the actual container was run against real Postgres and exercised over real
HTTP —
/healthz,/api/v1/openapi.json, a full register → cookie → authenticatedGET /meround trip — not just the test suite in isolation
CI is green (pending this push)
Tests added for the behaviour that changed
Verified manually — see above; this one leaned unusually hard on manual verification because
auth/RLS is exactly the area
CLAUDE.mdflags as "a mistake exposes another user's data"Invariants
users.unit_systemdefaults toimperialfor display only, per the invariantuser_id+ RLS policy + repository scope —sessionsandapi_tokensboth do;usersitself has an own-row policy (see PR body below for why loginstill works);
invitesis scoped bycreated_bypassword_hash/token_hash/code_hashnever appear in anyschemas/model; verified by reading everyresponse schema, not just by convention
upgrade -> downgrade -1 -> upgrade— verified twice, see aboveOne deliberate deviation worth a second look:
usersitself is not scoped by the same"every user-owned table has an RLS policy restricting it to its own user" pattern in the usual
sense — it has an own-row
id = current_setting('app.user_id')policy, but the login flow (bydefinition, pre-authentication) reads it via the
velodrome_authbypass role instead. This isnecessary — there's no way to scope a lookup-by-email query to a user_id you don't have yet — but
it's exactly the kind of auth-adjacent judgment call
CLAUDE.mdsays should get scrutiny ratherthan a rubber stamp.
Risks and follow-ups
docs/PLAN.mdmentions it; needs its own design pass rather than a bolt-on here) and the
procrastinateworker container (nothing to run yet — arrives with the ingestion pipeline).
ci.yml'sapiandmigrationsjobs were updated to provision the two runtime roles this codeactually needs, replacing the single placeholder
DATABASE_URLthat existed before any realcode did. Touches
.gitea/workflows/ci.ymloutsideapps/api/— necessary since the old envvars would never have worked against this schema, not scope creep.
apps/api/README.mdhas the full "why two database connections" explanation for anyone touchingthis later, including the exact environment variables and a local dev quickstart.