diff --git a/apps/api/src/cora/infrastructure/schema_version.py b/apps/api/src/cora/infrastructure/schema_version.py index 0829c60902..6a643d4b9d 100644 --- a/apps/api/src/cora/infrastructure/schema_version.py +++ b/apps/api/src/cora/infrastructure/schema_version.py @@ -74,7 +74,7 @@ class SchemaCheck: expected: str -EXPECTED_SCHEMA_VERSION: Final = "20260810000000" +EXPECTED_SCHEMA_VERSION: Final = "20260810120000" """The newest migration this build was written against. Hand-maintained, and deliberately not derived at runtime: the image does diff --git a/apps/api/tests/architecture/test_entries_table_grants.py b/apps/api/tests/architecture/test_entries_table_grants.py index be96ae68b0..0f0d06f6ae 100644 --- a/apps/api/tests/architecture/test_entries_table_grants.py +++ b/apps/api/tests/architecture/test_entries_table_grants.py @@ -8,13 +8,12 @@ PRIVILEGES" (see e.g. `20260621040000_init_entries_run_feed_heartbeats.sql`); that claim is FALSE for tables (the role-init migration's `ALTER DEFAULT PRIVILEGES` covers sequences only, per -`20260512230000_init_role_cora_app.sql`), so several tables created -this way carry no working grant at all. `_GRANDFATHERED` lists the -tables already affected; fixing them is a separate, already-tracked -follow-up (a GRANT-only migration against a live production database), -not something to silently paper over here. This test's job is to make -sure the mistake stops recurring: every table NOT on that list must -carry an explicit GRANT. +`20260512230000_init_role_cora_app.sql`). Five tables created this way +carried no working grant at all; `20260810120000_grant_cora_app_entries_table_access.sql` +closed the gap with a purely additive GRANT-only migration, so +`_GRANDFATHERED` is empty again. This test's job is to make sure the +mistake stops recurring: every entries_*/events table must carry an +explicit GRANT. """ from __future__ import annotations @@ -29,49 +28,82 @@ r"CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?([a-zA-Z_][a-zA-Z0-9_]*)", re.IGNORECASE, ) - -# Tables confirmed (2026-08-10, alongside the enclosure permit probe -# trail's own migration review) to rely on the false ALTER DEFAULT -# PRIVILEGES claim and carry no working GRANT today. Do not add to this -# list going forward: a new table belongs in a migration with its own -# explicit GRANT, per the assertion message below. -_GRANDFATHERED = frozenset( - { - "entries_run_readings", - "entries_operation_procedure_steps", - "entries_run_feed_heartbeats", - "entries_operation_procedure_diagnostics", - "entries_operation_procedure_outcomes", - } +_RENAME_TABLE_RE = re.compile( + r"ALTER\s+TABLE\s+([a-zA-Z_][a-zA-Z0-9_]*)\s+RENAME\s+TO\s+([a-zA-Z_][a-zA-Z0-9_]*)", + re.IGNORECASE, ) +# Closed: the five tables that relied on the false ALTER DEFAULT +# PRIVILEGES claim all got an explicit GRANT in +# 20260810120000_grant_cora_app_entries_table_access.sql. Do not add to +# this list going forward: a new table belongs in a migration with its +# own explicit GRANT, per the assertion message below. +_GRANDFATHERED: frozenset[str] = frozenset() + def _all_migration_text() -> str: return "\n".join(f.read_text() for f in tracked_migration_files()) -def _append_only_tables_created() -> set[str]: - out: set[str] = set() +def _append_only_tables_created() -> dict[str, frozenset[str]]: + """Every CURRENTLY append-only table, keyed by its CURRENT identifier + and mapped to every name it has ever held. + + Follows `ALTER TABLE ... RENAME TO ...` across ALL tables' migration + history, not just tables already matching `entries_`/`events`, then + filters to that prefix only on the final (current) name. A table can + enter the append-only family through a rename whose OLD name never + matched the prefix: `entries_conduit_verdicts` was created as + `observations_conduit_traversals`, renamed to + `entries_conduit_traversals`, then renamed again to its current name. + Gating the rename-follow on "old name already tracked as entries_/ + events" would silently drop that chain the moment the origin name + fell outside the prefix, exactly the same blind spot this function + exists to close for the `entries_run_readings` case (see below), just + one hop earlier. + + The same walk gives the full lineage, not just the current name, + which matters for the GRANT search: a privilege attaches to the + table's OID, not its name, so a GRANT issued under an OLD name (e.g. + `entries_decision_reasonings`, before it became + `entries_decision_inferences`) remains valid forever and a rename + never needs it re-issued under the new name. And the current-name + requirement matters for correctness in the other direction: a GRANT + written TODAY must target the table's current name (e.g. + `entries_run_observations`, not the dead `entries_run_readings`), the + only name that actually exists in the database by the time a later + migration runs. + """ + lineage: dict[str, set[str]] = {} for path in tracked_migration_files(): - for match in _CREATE_TABLE_RE.finditer(path.read_text()): + text = path.read_text() + for match in _CREATE_TABLE_RE.finditer(text): name = match.group(1) - if name == "events" or name.startswith("entries_"): - out.add(name) - return out + lineage.setdefault(name, {name}) + for match in _RENAME_TABLE_RE.finditer(text): + old_name, new_name = match.group(1), match.group(2) + if old_name in lineage: + names = lineage.pop(old_name) + names.add(new_name) + lineage[new_name] = names + return { + name: frozenset(names) + for name, names in lineage.items() + if name == "events" or name.startswith("entries_") + } @pytest.mark.architecture def test_every_new_entries_table_has_cora_app_grant() -> None: - """Pattern accepted: `GRANT ... ON [TABLE] ... TO cora_app`. - - Tables on `_GRANDFATHERED` are skipped: they predate this test and - fixing them is a separate production migration, not a test change. - A table that is renamed off the grandfathered list (its CREATE TABLE - name changes) is NOT exempt under its new name; only the exact - grandfathered identifiers are excused. + """Pattern accepted: `GRANT ... ON [TABLE] ... TO cora_app`, + where `` is any name in the table's rename lineage (see + `_append_only_tables_created`), not just its current one. Tables on + `_GRANDFATHERED` are skipped: they predate this test and fixing them + is a separate production migration, not a test change. """ haystack = _all_migration_text() - tables = _append_only_tables_created() - _GRANDFATHERED + lineages = _append_only_tables_created() + tables = set(lineages) - _GRANDFATHERED assert tables, ( "No non-grandfathered append-only tables found; either the schema " "is empty, table-name detection is wrong, or _GRANDFATHERED has " @@ -80,11 +112,14 @@ def test_every_new_entries_table_has_cora_app_grant() -> None: missing: list[str] = [] for table in sorted(tables): - pattern = re.compile( - rf"GRANT\b[^;]*\bON\s+(?:TABLE\s+)?{re.escape(table)}\b[^;]*\bTO\s+[^;]*cora_app\b", - re.IGNORECASE | re.DOTALL, - ) - if not pattern.search(haystack): + if not any( + re.search( + rf"GRANT\b[^;]*\bON\s+(?:TABLE\s+)?{re.escape(name)}\b[^;]*\bTO\s+[^;]*cora_app\b", + haystack, + re.IGNORECASE | re.DOTALL, + ) + for name in lineages[table] + ): missing.append(table) assert not missing, ( diff --git a/apps/api/tests/integration/test_cora_app_role_revoke_postgres.py b/apps/api/tests/integration/test_cora_app_role_revoke_postgres.py index 574c716dfa..7d9d904deb 100644 --- a/apps/api/tests/integration/test_cora_app_role_revoke_postgres.py +++ b/apps/api/tests/integration/test_cora_app_role_revoke_postgres.py @@ -1,4 +1,5 @@ -"""Integration test: cora_app role cannot UPDATE / DELETE append-only tables. +"""Integration test: cora_app role cannot UPDATE / DELETE append-only tables, +and DOES have SELECT + INSERT on every entries_* table. Foundation hardening. The migration `20260512230000_init_role_cora_app.sql` creates a `cora_app` database @@ -15,6 +16,11 @@ review (Postgres now refuses them at the role boundary) - The classic event-sourcing failure mode where "events are immutable" is documented but not enforced + - A GRANT statement that exists in migration text but that Postgres + does not actually honor (`test_entries_table_grants.py` is a regex + match against SQL source, not a proof the database accepts it; five + tables carried a false ALTER DEFAULT PRIVILEGES claim for months + with a passing test suite before `20260810120000_...sql` fixed it) The fixtures in `conftest.py` connect as the testcontainers superuser so the rest of the suite can TRUNCATE between tests. @@ -192,6 +198,86 @@ async def test_cora_app_cannot_update_or_delete_entries_tables( await conn.execute(f"DELETE FROM {table}") +_ENTRIES_TABLE_INSERTS: dict[str, tuple[str, int]] = { + "entries_run_observations": ( + """ + INSERT INTO entries_run_observations ( + event_id, run_id, logbook_id, actor_id, command_name, + channel_name, value, sampling_procedure, sampled_at, + occurred_at, correlation_id + ) VALUES ($1, $2, $3, $4, 'TestCmd', 'test-channel', 1.0, + 'periodic', now(), now(), $5) + """, + 5, + ), + "entries_operation_procedure_activities": ( + """ + INSERT INTO entries_operation_procedure_activities ( + event_id, procedure_id, logbook_id, actor_id, command_name, + step_kind, payload, sampled_at, occurred_at, correlation_id + ) VALUES ($1, $2, $3, $4, 'TestCmd', 'SetpointStep', '{}'::jsonb, + now(), now(), $5) + """, + 5, + ), + "entries_run_feed_heartbeats": ( + """ + INSERT INTO entries_run_feed_heartbeats ( + event_id, run_id, source_id, heartbeat_at + ) VALUES ($1, $2, 'test-source', now()) + """, + 2, + ), + "entries_operation_procedure_diagnostics": ( + """ + INSERT INTO entries_operation_procedure_diagnostics ( + event_id, procedure_id, logbook_id, iteration_index, + model_ref, payload, sampled_at, occurred_at, correlation_id + ) VALUES ($1, $2, $3, 1, 'test-model', '{}'::jsonb, now(), now(), $4) + """, + 4, + ), + "entries_operation_procedure_outcomes": ( + """ + INSERT INTO entries_operation_procedure_outcomes ( + event_id, procedure_id, logbook_id, iteration_index, + point, measurements, succeeded, sampled_at, occurred_at, + correlation_id + ) VALUES ($1, $2, $3, 1, '{}'::jsonb, '{}'::jsonb, true, now(), now(), $4) + """, + 4, + ), +} +"""The five tables `20260810120000_grant_cora_app_entries_table_access.sql` +fixed, by their CURRENT (post-rename) name, each mapped to a minimal +valid INSERT and its UUID-parameter count. Column shapes copied from +each table's own CREATE TABLE migration; `is_simulated` and other +DEFAULT-bearing columns are omitted since a bare INSERT already proves +the grant.""" + + +@pytest.mark.integration +@pytest.mark.parametrize("table", sorted(_ENTRIES_TABLE_INSERTS)) +async def test_cora_app_can_select_and_insert_entries_tables( + cora_app_pool: asyncpg.Pool, + table: str, +) -> None: + """The regression this test exists for: each of these five tables' + migration header claimed cora_app already had SELECT + INSERT via + ALTER DEFAULT PRIVILEGES, a claim that was false (that clause covers + sequences only). `test_entries_table_grants.py` proves a GRANT + statement is present in migration text; only a real INSERT against a + `cora_app`-credentialed pool proves Postgres actually honors it.""" + sql, param_count = _ENTRIES_TABLE_INSERTS[table] + event_id = uuid4() + params = [event_id, *(uuid4() for _ in range(param_count - 1))] + + async with cora_app_pool.acquire() as conn: + await conn.execute(sql, *params) + rows = await conn.fetch(f"SELECT event_id FROM {table} WHERE event_id = $1", event_id) + assert len(rows) == 1 + + @pytest.mark.integration async def test_cora_app_can_mutate_idempotency_keys( cora_app_pool: asyncpg.Pool, diff --git a/infra/atlas/migrations/20260810120000_grant_cora_app_entries_table_access.sql b/infra/atlas/migrations/20260810120000_grant_cora_app_entries_table_access.sql new file mode 100644 index 0000000000..6df614065e --- /dev/null +++ b/infra/atlas/migrations/20260810120000_grant_cora_app_entries_table_access.sql @@ -0,0 +1,38 @@ +-- Fixes a latent permission gap: five entries_* tables were created +-- with a header comment claiming cora_app "gets SELECT + INSERT via +-- ALTER DEFAULT PRIVILEGES in 20260512230000_init_role_cora_app.sql". +-- That claim is false. The role-init migration's +-- `ALTER DEFAULT PRIVILEGES ... GRANT USAGE, SELECT ON SEQUENCES` +-- covers SEQUENCES only, never TABLES, so cora_app has never had a +-- working grant on any of these tables. Confirmed by grepping every +-- migration for a GRANT naming each table plus cora_app: none exists. +-- +-- Affected tables, named by their CURRENT identifier (two of the five +-- were renamed after creation, so the GRANT below must target the name +-- the table actually holds by this point in migration history, not the +-- name it was created under): +-- entries_run_readings -> entries_run_observations +-- (20260610020000_rename_entries_run_readings_to_entries_run_observations.sql) +-- entries_operation_procedure_steps -> entries_operation_procedure_activities +-- (20260610030000_rename_entries_operation_procedure_steps_to_entries_operation_procedure_activities.sql) +-- entries_run_feed_heartbeats (unrenamed) +-- entries_operation_procedure_diagnostics (unrenamed) +-- entries_operation_procedure_outcomes (unrenamed) +-- +-- Each already carries its own REVOKE UPDATE, DELETE, TRUNCATE (a +-- privilege revocation attaches to the table object and survives a +-- later rename), so only the missing GRANT is added here. +-- +-- Purely additive and non-destructive: a GRANT cannot lock a table or +-- fail against existing rows. Currently dormant, not live, because the +-- pilot's DATABASE_URL still connects as the owner role (cora), not +-- cora_app; this closes the gap before anything switches to the +-- restricted role. See tests/architecture/test_entries_table_grants.py, +-- whose _GRANDFATHERED allowlist these five names are removed from in +-- the same change as this migration. + +GRANT SELECT, INSERT ON entries_run_observations TO cora_app; +GRANT SELECT, INSERT ON entries_operation_procedure_activities TO cora_app; +GRANT SELECT, INSERT ON entries_run_feed_heartbeats TO cora_app; +GRANT SELECT, INSERT ON entries_operation_procedure_diagnostics TO cora_app; +GRANT SELECT, INSERT ON entries_operation_procedure_outcomes TO cora_app; diff --git a/infra/atlas/migrations/atlas.sum b/infra/atlas/migrations/atlas.sum index febbb34e5a..edb9274ab9 100644 --- a/infra/atlas/migrations/atlas.sum +++ b/infra/atlas/migrations/atlas.sum @@ -1,4 +1,4 @@ -h1:bdALzYDPVBnS+sQ6HlFKoV4ZwWy02BiLdJGYVeBbgGg= +h1:Hh2aacDC3e9ka8TTvNmeD8Zw7LlHWQSMl8waDuzuWL0= 20260509120000_init_events.sql h1:GmgCZKfaqXu1m96/cKAks2vhaLWTdEaHTLkFtUo9FXg= 20260509170000_init_idempotency.sql h1:Nbu8DIE4Sv1WiHw3G22+tYffPhKc5Jryw3PMK8wB2zY= 20260510010000_add_event_id.sql h1:RbtYP6uMnOB20zhJ9dNXUi4YVqbmlEzf562pmygnRW8= @@ -162,3 +162,4 @@ h1:bdALzYDPVBnS+sQ6HlFKoV4ZwWy02BiLdJGYVeBbgGg= 20260729120000_add_proj_data_dataset_summary_checksum.sql h1:Zk3BmIBMjck0Q4OkKzP1wU93NLWC2b5IRV0iM0ihlX0= 20260809190000_split_enclosure_permit_transition_and_source_times.sql h1:xjTv+1Eq6Ujg40E4y0mOK5nIlT8pXKhdp3ve7No+uvQ= 20260810000000_init_entries_enclosure_permit_probes.sql h1:jSvy8jXhHwZbq1A4AMqKEL3uviaJtlyGLCGvISfevd4= +20260810120000_grant_cora_app_entries_table_access.sql h1:LCngXYBMYN5aS7fy/3Sa8G1C/9KNG+J4GFf5Xh4X2kM=