Make the owner of an outgoing post a database guarantee #110

Closed

robbertbos wants to merge 7 commits from fase-1-fundament into main

pull from: fase-1-fundament

merge into: robbertbos:main

robbertbos:main

robbertbos:renovate/nldd

robbertbos:remove-pending-cleanup-label

robbertbos:fase-2b-kabel-v2

robbertbos:dev-fake-mm-file-fixtures-pr

robbertbos:queued-count-consistent-formatting

robbertbos:renovate-auto-refresh-release-sha

robbertbos:dev-fake-mm-file-fixtures

Conversation 0 Commits 7 Files changed 11 +947 -58

robbertbos commented 2026-07-12 14:44:03 +00:00

Owner

Copy link

Fase 1 van "nieuw bericht opstellen en plannen". Deze PR bouwt nog geen feature: hij dicht een structurele zwakte waar de rest van dat werk bovenop komt te staan, en die vandaag al bestaat.

Het probleem

De tick-loop die geplande berichten verstuurt, leidt de afzender af uit de kaart, niet uit de rij die hij gaat versturen:

card = await s.get(_Card, card_id)
user_id = card.user_id          # wie verstuurt
account_id = card.account_id    # met welk token

Dat werkt alleen zolang outgoing_posts.user_id gelijk is aan cards.user_id. Er was geen enkele databasegarantie dat dat zo is: geen composite FK, geen check-constraint. Het is een applicatie-invariant die op drie plekken met de hand werd volgehouden (_ensure_card, en de card_id_map in de import).

Niet exploiteerbaar vandaag - alle drie de plekken doen het goed. Maar de veiligheid was geen eigenschap van het schema, en de runner draait buiten het requestpad, dus zonder require_user-vangrail. Eén endpoint dat de eigenaarscheck vergeet, of één handmatig bewerkte importbundel, en de runner verstuurt de tekst van gebruiker A met het token van gebruiker B. Geen FK, geen constraint en geen test zou aanslaan.

Vanaf fase 4 heeft een rij geen kaart meer om zijn identiteit uit af te leiden, dus dit moest eerst.

Wat er verandert

Twee composite foreign keys. PRAGMA foreign_keys=ON staat aan voor elke SQLite-verbinding, dus ze worden op beide databases afgedwongen:

cards:              UNIQUE (id, user_id)
connected_accounts: UNIQUE (id, user_id)

outgoing_posts:
  FOREIGN KEY (card_id,    user_id) REFERENCES cards(id, user_id)
  FOREIGN KEY (account_id, user_id) REFERENCES connected_accounts(id, user_id)

Daarmee is het account waarvan het token een rij verstuurt, per databasegarantie een account van de eigenaar van die rij. NULL-kolommen slaan de controle over (MATCH SIMPLE), dus een rij zonder account - en straks zonder kaart - past er gewoon in.

De enkelvoudige FK's blijven staan. Ze zijn overbodig naast de composite variant, maar die op card_id is naamloos aangemaakt in het initiële schema, en een naamloze constraint is op SQLite niet te droppen zonder de tabel te herbouwen in een migratie die bij het opstarten draait. Dat weegt niet op tegen de redundantie die het zou besparen.

De runner leest zijn afzender van de rij. De terugval op card.account_id verdwijnt: een rij zonder account faalt nu zichtbaar met "geen Mattermost-account gekoppeld" in plaats van een identiteit te lenen van een naburig record. En de groepering keyt op (card_id, account_id), niet op de kaart alleen - de worker bouwt één client per groep, dus een groep moet single-account zijn.

Migraties worden voortaan getest. Dat gebeurde niet: conftest.py bouwt het schema met create_all, dus een revisie die fout was, ontbrak of afweek van de modellen bleef groen. De nieuwe harness draait upgrade / downgrade / upgrade tegen een echte database, vergelijkt het gemigreerde schema met de modellen via Alembic's eigen compare_metadata, en schrijft er ook echt rijen in. CI draait hem ook tegen Postgres: batch-mode bouwt de tabel opnieuw op op SQLite en doet ALTER TABLE op Postgres, dus een revisie kan op de één slagen en op de ander falen.

Wat de review vond, en wat ik ermee deed

Ik heb er vier adversariële reviewlenzen op gezet (migratieveiligheid, security, correctheid, testkwaliteit) en de bevindingen daarna opnieuw laten weerleggen. 13 van de 14 zijn weerlegd. Wat overbleef:

• Ik had een voetzoeker ingebouwd. De testfixture deed DROP SCHEMA public CASCADE op wat WAGGLE_TEST_MIGRATION_URL aanwees, en mijn eigen docstring zei: wijs 'm naar de dev-database uit compose.dev.yaml. Wie de instructie volgde, gooide zijn dev-data weg - en de test slaagde gewoon. Er zit nu een guard op, zoals dev_seed.py die ook heeft.

• Drie vals-groene tests. De reviewer bewees het met mutaties: haal ux_cards_id_user uit de revisie en álle tests slaagden nog steeds, terwijl elke INSERT in het gemigreerde schema faalde met "foreign key mismatch" (een composite FK zonder bijpassende unique key op de ouder is geldige DDL die simpelweg elke rij weigert). Datzelfde gold voor de backfill: sloop de hele lus, test blijft groen - want op SQLite dwingt Alembic's eigen engine geen foreign keys af, dus de constraint "raiset" helemaal niet. Beide mutaties doden nu tests.

• Mijn comment beloofde te veel. Ik schreef dat de FK's "één eigenaar én één account" per groep garanderen. Ze garanderen één eigenaar. Dat er ook één account is, leunde op een ongeschreven invariant. In plaats van de comment te repareren heb ik de oorzaak weggenomen: account_id zit nu in de groeperingssleutel.

De grootste verdenking - dat de SQLite-tabelherbouw de partiële index ux_outgoing_one_draft_per_card zou droppen - is empirisch weerlegd: hij overleeft mét zijn WHERE-clausule, net als elke andere index en constraint op de drie tabellen. Er staat nu een test op.

Poorten

• 1334 tests groen, 100,00% backend-dekking.
• Migratietests groen op SQLite én Postgres.
• pre-commit run --all-files groen, tsc schoon, eslint 0 errors.

Let op bij review

De backfill in de revisie draait bij het opstarten van de applicatie. Faalt hij, dan start de pod niet. Hij repareert daarom in plaats van te blokkeren: een rij waarvan de eigenaar afwijkt van de kaart wordt uitgelijnd (nul rijen verwacht - de runner verstuurde zo'n rij toch al met het token van de kaart-eigenaar), en een account dat niet van de eigenaar is wordt genulld in plaats van de upgrade te laten crashen. Zo'n rij belandt zichtbaar in Verzendproblemen.

Fase 1 van "nieuw bericht opstellen en plannen". Deze PR bouwt nog geen feature: hij dicht een structurele zwakte waar de rest van dat werk bovenop komt te staan, en die vandaag al bestaat. ## Het probleem De tick-loop die geplande berichten verstuurt, leidt de afzender af uit de **kaart**, niet uit de **rij die hij gaat versturen**: ```python card = await s.get(_Card, card_id) user_id = card.user_id # wie verstuurt account_id = card.account_id # met welk token ``` Dat werkt alleen zolang `outgoing_posts.user_id` gelijk is aan `cards.user_id`. Er was **geen enkele databasegarantie** dat dat zo is: geen composite FK, geen check-constraint. Het is een applicatie-invariant die op drie plekken met de hand werd volgehouden (`_ensure_card`, en de `card_id_map` in de import). Niet exploiteerbaar vandaag - alle drie de plekken doen het goed. Maar de veiligheid was geen eigenschap van het schema, en de runner draait buiten het requestpad, dus zonder `require_user`-vangrail. Eén endpoint dat de eigenaarscheck vergeet, of één handmatig bewerkte importbundel, en de runner verstuurt de tekst van gebruiker A met het token van gebruiker B. Geen FK, geen constraint en geen test zou aanslaan. Vanaf fase 4 heeft een rij geen kaart meer om zijn identiteit uit af te leiden, dus dit moest eerst. ## Wat er verandert **Twee composite foreign keys.** `PRAGMA foreign_keys=ON` staat aan voor elke SQLite-verbinding, dus ze worden op beide databases afgedwongen: ``` cards: UNIQUE (id, user_id) connected_accounts: UNIQUE (id, user_id) outgoing_posts: FOREIGN KEY (card_id, user_id) REFERENCES cards(id, user_id) FOREIGN KEY (account_id, user_id) REFERENCES connected_accounts(id, user_id) ``` Daarmee is **het account waarvan het token een rij verstuurt, per databasegarantie een account van de eigenaar van die rij**. NULL-kolommen slaan de controle over (MATCH SIMPLE), dus een rij zonder account - en straks zonder kaart - past er gewoon in. De enkelvoudige FK's blijven staan. Ze zijn overbodig naast de composite variant, maar die op `card_id` is naamloos aangemaakt in het initiële schema, en een naamloze constraint is op SQLite niet te droppen zonder de tabel te herbouwen in een migratie die bij het **opstarten** draait. Dat weegt niet op tegen de redundantie die het zou besparen. **De runner leest zijn afzender van de rij.** De terugval op `card.account_id` verdwijnt: een rij zonder account faalt nu zichtbaar met "geen Mattermost-account gekoppeld" in plaats van een identiteit te lenen van een naburig record. En de groepering keyt op `(card_id, account_id)`, niet op de kaart alleen - de worker bouwt één client per groep, dus een groep moet single-account zijn. **Migraties worden voortaan getest.** Dat gebeurde niet: `conftest.py` bouwt het schema met `create_all`, dus een revisie die fout was, ontbrak of afweek van de modellen bleef groen. De nieuwe harness draait `upgrade` / `downgrade` / `upgrade` tegen een echte database, vergelijkt het gemigreerde schema met de modellen via Alembic's eigen `compare_metadata`, en schrijft er ook echt rijen in. CI draait hem ook tegen Postgres: batch-mode bouwt de tabel opnieuw op op SQLite en doet `ALTER TABLE` op Postgres, dus een revisie kan op de één slagen en op de ander falen. ## Wat de review vond, en wat ik ermee deed Ik heb er vier adversariële reviewlenzen op gezet (migratieveiligheid, security, correctheid, testkwaliteit) en de bevindingen daarna opnieuw laten weerleggen. 13 van de 14 zijn weerlegd. Wat overbleef: - **Ik had een voetzoeker ingebouwd.** De testfixture deed `DROP SCHEMA public CASCADE` op wat `WAGGLE_TEST_MIGRATION_URL` aanwees, en mijn eigen docstring zei: wijs 'm naar de dev-database uit `compose.dev.yaml`. Wie de instructie volgde, gooide zijn dev-data weg - en de test slaagde gewoon. Er zit nu een guard op, zoals `dev_seed.py` die ook heeft. - **Drie vals-groene tests.** De reviewer bewees het met mutaties: haal `ux_cards_id_user` uit de revisie en álle tests slaagden nog steeds, terwijl elke INSERT in het gemigreerde schema faalde met "foreign key mismatch" (een composite FK zonder bijpassende unique key op de ouder is geldige DDL die simpelweg elke rij weigert). Datzelfde gold voor de backfill: sloop de hele lus, test blijft groen - want op SQLite dwingt Alembic's eigen engine geen foreign keys af, dus de constraint "raiset" helemaal niet. Beide mutaties doden nu tests. - **Mijn comment beloofde te veel.** Ik schreef dat de FK's "één eigenaar én één account" per groep garanderen. Ze garanderen één eigenaar. Dat er ook één account is, leunde op een ongeschreven invariant. In plaats van de comment te repareren heb ik de oorzaak weggenomen: `account_id` zit nu in de groeperingssleutel. De grootste verdenking - dat de SQLite-tabelherbouw de partiële index `ux_outgoing_one_draft_per_card` zou droppen - is **empirisch weerlegd**: hij overleeft mét zijn `WHERE`-clausule, net als elke andere index en constraint op de drie tabellen. Er staat nu een test op. ## Poorten - 1334 tests groen, **100,00% backend-dekking**. - Migratietests groen op **SQLite én Postgres**. - `pre-commit run --all-files` groen, `tsc` schoon, `eslint` 0 errors. ## Let op bij review De backfill in de revisie draait bij het **opstarten van de applicatie**. Faalt hij, dan start de pod niet. Hij repareert daarom in plaats van te blokkeren: een rij waarvan de eigenaar afwijkt van de kaart wordt uitgelijnd (nul rijen verwacht - de runner verstuurde zo'n rij toch al met het token van de kaart-eigenaar), en een account dat niet van de eigenaar is wordt genulld in plaats van de upgrade te laten crashen. Zo'n rij belandt zichtbaar in Verzendproblemen.

robbertbos added 7 commits 2026-07-12 14:44:03 +00:00

Make the owner of an outgoing post a database guarantee ... 3d8d0ec1a3

The scheduled runner derives the sender from the card - card.user_id and
card.account_id - not from the row it is about to send. Nothing enforced that
those agree: no composite FK, no check constraint, only an application invariant
upheld by hand in three places (_ensure_card, and the card_id_map in the import).

One endpoint forgetting the ownership check, or one hand-edited import bundle,
and the runner would send one user's text with another user's token. No FK, no
constraint and no test would catch it. The tick-loop runs outside the request
path, so it has no require_user guard rail either.

Two composite foreign keys close it: a row's card belongs to the row's owner, and
so does the account whose token sends it. NULL columns skip the check (MATCH
SIMPLE), so a row without an account - or later without a card - still fits.

The single-column FKs stay. They are subsumed, but the card_id one was created
unnamed in the initial schema and dropping an unnamed constraint on SQLite means
rebuilding the table in a migration that runs at startup. Not worth it.

Runner: take the sender from the row, not from the card ... bc1f1c3fa0

The card is not what we are sending, and from the card-less message on a row
will not have one at all. The composite FKs from the previous commit are what
make reading the account off the row safe: the account is provably the row
owner's.

The fallback to card.account_id goes with it. A row without an account now fails
visibly with "geen Mattermost-account gekoppeld" instead of borrowing an
identity from a neighbouring record.

Migrate to the composite tenant FKs, and start testing migrations at all ... a8552dff9b

Nothing in the suite ran a migration: conftest builds its schema with
create_all, so a revision that was wrong, missing, or out of step with the
models still showed green. test_migrations.py runs upgrade/downgrade/upgrade
against a real database and asserts the migrated schema matches the models. CI
now runs it against Postgres too - batch mode rebuilds the table on SQLite and
emits ALTER TABLE on Postgres, so a revision can pass on one and fail on the
other, and this one runs at application startup.

The revision backfills before it constrains: it aligns the row owner with the
card owner (zero rows expected - the runner has always sent with the card
owner's token), fills account_id off the card, and nulls an account that is not
the row owner's rather than blocking the upgrade. A hard failure here is a pod
that will not boot; a nulled account is a row that lands visibly in
Verzendproblemen.

Document that multi-tenant isolation is in scope, not out of it ... 759bd8a32d

docs/security.md still described a "single-user single-tenant" app and put
multi-tenant isolation explicitly out of scope. That has not been true since the
OIDC login, and next to that text the cross-tenant test in this PR reads as
pointless.

Test the migration's data backfill and the SQLite table rebuild ... bb98ab5ca9

alembic/versions/ sits outside the coverage source, so the 100% gate says
nothing about the riskiest code in the revision: the backfill loop that runs at
application startup. If it leaves behind a row the new FKs reject, the pod does
not boot. The test seeds the three rows the old schema allowed and the new one
must not, then upgrades across the revision and checks each was repaired.

Also asserts the batch rebuild keeps ux_outgoing_one_draft_per_card WITH its
WHERE clause. That index is declared outside __table_args__; a rebuild that
forgot the predicate would turn "one draft per card" into "one row per card" and
reject every second scheduled post, and a column diff would not show it.

The fixture now empties the database before each test. On Postgres all tests
share one, so a test that needs to start below head silently started at head -
you cannot upgrade backwards - and the result depended on test order.
pytest-randomly is installed.

Stop the migration test from being able to wipe your dev database ... 3513f3929d

_drop_everything ran DROP SCHEMA public CASCADE on whatever WAGGLE_TEST_MIGRATION_URL
pointed at, and the docstring told you to point it at
postgresql://waggle:waggle@localhost:5432/waggle - which is verbatim the compose.dev.yaml
database that CLAUDE.md documents for local Postgres testing. Follow the instructions,
lose your dev data, and the test still passes so nothing tells you.

It now refuses any database whose name does not look like a throwaway, the way
dev_seed.py already refuses a non-local target.

Three more holes the review found, all of them false greens:

- test_migrated_schema_matches_the_models compared column NAMES. Delete the
  ux_cards_id_user constraint from the revision and every test still passed - while
  every INSERT into the migrated schema failed with "foreign key mismatch", because a
  composite FK whose parent lacks the matching unique key is valid DDL that simply
  rejects every row. It now uses alembic's own compare_metadata, and that mutation kills
  four tests.

- Nothing ever wrote a row into a MIGRATED schema: the constraint tests build theirs with
  create_all and the migration tests only read DDL, so the two halves never met. There is
  now a test that inserts a legitimate row and a stolen one.

- "If the backfill misses a row, creating the FKs raises" is not true on SQLite: batch
  mode rebuilds the table and alembic's engine never turns FK enforcement on, so deleting
  the whole backfill loop left the test green. It now ends with a foreign_key_check.

Group due rows by (card_id, account_id), not by card alone ... 3a8cce9bcb

The worker builds one Mattermost client per group and reuses it for every row in
it, so a group has to be single-account. I had written that the composite FKs
guarantee that. They do not: they guarantee a card group is one *owner*. That it
is also one *account* rests on account_id being copied off the card at creation
and never changed - an invariant nothing enforces.

Rather than correct the comment, put account_id in the key. The grouping then
cannot go wrong even if that invariant ever stops holding, and it is what the
card-less message needs anyway.

robbertbos referenced this pull request 2026-07-12 15:58:00 +00:00

Give every parked attachment an owner, a ceiling, and the server's own limit #111

robbertbos referenced this pull request 2026-07-15 20:53:01 +00:00

Make the owner of an outgoing post a database guarantee #123

robbertbos closed this pull request 2026-07-15 20:53:01 +00:00

Some checks failed

Hide all checks

CI / pre-commit (pull_request) Successful in 1m10s

Required

Details

CI / frontend-test (pull_request) Successful in 4m13s

Required

Details

CI / release-scripts (pull_request) Successful in 9s

Required

Details

security-scan / Python SCA (pip-audit) (pull_request) Successful in 57s

Required

Details

security-scan / Python SAST (bandit) (pull_request) Successful in 34s

Required

Details

security-scan / JS SCA (npm audit) (pull_request) Successful in 40s

Required

Details

CI / backend-test (pull_request) Successful in 6m56s

Required

Details

security-scan / Filesystem scan (trivy fs) (pull_request) Successful in 22s

Required

Details

security-scan / SBOM (trivy) (pull_request) Successful in 15s

Required

Details

CI / backend-test-postgres (pull_request) Successful in 8m27s

Required

Details

test-build / build (backend) (pull_request) Successful in 1m49s

Details

test-build / build (frontend) (pull_request) Successful in 2m1s

Details

CI / e2e (pull_request) Successful in 8m12s

Required

Details

test-build / build (pull_request) Has been cancelled

Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

robbertbos/waggle!110

No description provided.