diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b02b9c6..f5ddc48 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,6 +26,30 @@ jobs: lint-and-test: runs-on: ubuntu-latest + # A real PostgreSQL for tests/test_audit_postgres.py. The rest of the suite + # builds its schema from the models on in-memory SQLite and never runs the + # migrations. That is how migration 005's append-only trigger on + # audit_events could refuse the ON DELETE SET NULL cascade of every free + # account deletion (fixed in migration 012), and migration 001 could fail on + # an empty database, without a failing test. The major version is the one + # docker-compose.cloud.yml runs (the test checks it). Pinned by digest + # like the veraPDF image: to move it, take the digest from + # `docker buildx imagetools inspect postgres:`. Trust auth is fine for + # a throwaway database that only this job's steps can reach. + services: + postgres: + image: postgres:16.15-alpine@sha256:721873c34ceb9f8d8fc265984940dc982404c105f19ad51be9fdc5970a6080ea + env: + POSTGRES_DB: filemorph_test + POSTGRES_HOST_AUTH_METHOD: trust + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -h localhost -U postgres" + --health-interval 5s + --health-timeout 5s + --health-retries 10 + steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -182,6 +206,8 @@ jobs: run: pip-audit -r requirements.lock --ignore-vuln PYSEC-2026-1325 --ignore-vuln CVE-2026-55073 - name: Run tests + env: + FILEMORPH_TEST_POSTGRES_URL: postgresql+asyncpg://postgres@localhost:5432/filemorph_test run: pytest tests/ -v --tb=short # requirements.lock is what the image installs, so it has to keep diff --git a/.github/workflows/deps-latest.yml b/.github/workflows/deps-latest.yml index 4b85c29..490ee41 100644 --- a/.github/workflows/deps-latest.yml +++ b/.github/workflows/deps-latest.yml @@ -34,6 +34,21 @@ jobs: test-latest: runs-on: ubuntu-latest + # The same PostgreSQL service as ci.yml's lint-and-test; see the comment there. + services: + postgres: + image: postgres:16.15-alpine@sha256:721873c34ceb9f8d8fc265984940dc982404c105f19ad51be9fdc5970a6080ea + env: + POSTGRES_DB: filemorph_test + POSTGRES_HOST_AUTH_METHOD: trust + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -h localhost -U postgres" + --health-interval 5s + --health-timeout 5s + --health-retries 10 + steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -57,4 +72,6 @@ jobs: run: pip install -r requirements-dev.txt - name: Run tests + env: + FILEMORPH_TEST_POSTGRES_URL: postgresql+asyncpg://postgres@localhost:5432/filemorph_test run: pytest tests/ -v --tb=short diff --git a/CHANGELOG.md b/CHANGELOG.md index 50905d7..2893e8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,43 @@ Versions follow [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Fixed — deleting a free account works on PostgreSQL + +`DELETE /api/v1/auth/account` failed with a 500 on PostgreSQL for every +account without a Stripe customer id, and nothing was deleted. Deleting the +`users` row makes Postgres null `audit_events.actor_user_id` through the +foreign key's `ON DELETE SET NULL`, which it carries out as an UPDATE, and +the audit log's append-only trigger (migration 005) refused every UPDATE, so +the whole deletion rolled back. Every account has audit events naming it — +at the latest the deletion request itself — so every such deletion failed. +Migration 012 lets exactly that change through: `actor_user_id` goes from a +value to NULL, nothing else in the row changes, and the account it named no +longer exists. Every other UPDATE or DELETE of an audit event is still +refused, including a direct UPDATE that nulls a live account's ID, and the +hash chain still verifies. Accounts with a Stripe customer id were not +affected: their `users` row is kept for the tax-retention period, so nothing +cascades. + +The test suite runs on SQLite, which has no such trigger, so no test noticed. +The `lint-and-test` job and the weekly `deps-latest` run now start a +PostgreSQL 16 service, and `tests/test_audit_postgres.py` migrates an empty +database with Alembic (which surfaced the migration 001 failure below), +deletes an account through the API, and checks that its audit events are +kept without the account ID, the chain verifies, and every other change to an +event is refused. A guard fails if either workflow stops providing the +database. The TOM annex and the vendor security questionnaire name migration +012 next to 005. + +### Fixed — a fresh Cloud Edition database can be migrated + +On an empty PostgreSQL database, `alembic upgrade head`, which the container +entrypoint runs on every Cloud Edition start, failed in the first migration +with `type "tier_enum" already exists`: migration 001 created its two enum +types itself and then again with their tables. A fresh Cloud Edition install +therefore never started; the container kept restarting. Migration 001 now +leaves creating the types to the tables. A database that already has the +schema is not affected, since Alembic does not run migration 001 again. + ### Added — online cancellation without login: "Cancel contracts here" (§ 312k BGB) German consumer law (§ 312k BGB) requires a subscription sold online to be diff --git a/alembic/versions/001_baseline.py b/alembic/versions/001_baseline.py index 8be7cf3..c4e3477 100644 --- a/alembic/versions/001_baseline.py +++ b/alembic/versions/001_baseline.py @@ -35,9 +35,10 @@ def _uuid_col(name: str, *args, **kwargs) -> sa.Column: def upgrade() -> None: - tier_enum.create(op.get_bind(), checkfirst=True) - job_status_enum.create(op.get_bind(), checkfirst=True) - + # No explicit tier_enum/job_status_enum.create() here: op.create_table + # creates each enum type with its first table, without checkfirst, so a + # type created beforehand made a fresh Postgres fail with "type already + # exists". op.create_table( "users", _uuid_col("id", primary_key=True), diff --git a/alembic/versions/012_audit_actor_erasure.py b/alembic/versions/012_audit_actor_erasure.py new file mode 100644 index 0000000..72cdc25 --- /dev/null +++ b/alembic/versions/012_audit_actor_erasure.py @@ -0,0 +1,89 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Let an account deletion null ``audit_events.actor_user_id``. + +Revision ID: 012_audit_actor_erasure +Revises: 011_ai_usage +Create Date: 2026-09-29 + +Migration 005 gave ``audit_events.actor_user_id`` an ``ON DELETE SET NULL`` +foreign key, so that deleting an account (GDPR Art. 17) keeps its audit rows +but drops their link to the person. The same migration made the table +append-only with a trigger that raises on every UPDATE. Postgres carries out +SET NULL as an UPDATE of the referencing rows, so the trigger refused it and +the whole ``DELETE FROM users`` failed. Every registered account has audit +rows naming it (registration, login), so self-service deletion of a free +account failed with a 500 and deleted nothing. + +The trigger function now lets exactly that change through: ``actor_user_id`` +goes from a value to NULL, nothing else in the row changes, and the ``users`` +row it named no longer exists. Only deleting that account produces this +combination. A direct UPDATE of a live account's rows, and every other +UPDATE or DELETE, is still refused. The rest of the row is +compared as a whole (``to_jsonb(row) - 'actor_user_id'``) rather than column +by column, so a column added later is covered too and dropping one cannot +break the function. ``actor_user_id`` is not an input to ``record_hash``, so +``verify_chain`` still passes. + +Postgres only, like the trigger itself; SQLite (the test harness) never had +it. ``tests/test_audit_postgres.py`` runs this against a real Postgres in CI. +""" + +from __future__ import annotations + +from typing import Sequence, Union + +from alembic import op + +revision: str = "012_audit_actor_erasure" +down_revision: Union[str, None] = "011_ai_usage" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# CREATE OR REPLACE keeps the function's identity, so migration 005's two +# triggers (audit_events_no_update, audit_events_no_delete) stay bound to it; +# a later exception has to re-issue the whole function. NEW is read only for +# an UPDATE; a DELETE trigger has none. The lookup names public.users because +# a session's temporary tables come first in the search path, so a temporary +# "users" could answer it otherwise. The migrations create the tables in the +# connecting role's current schema, public unless configured otherwise; where +# they live elsewhere, the lookup fails and the deletion is refused. +_ALLOW_ACTOR_ERASURE = """ +CREATE OR REPLACE FUNCTION audit_events_block_modification() +RETURNS TRIGGER AS $$ +BEGIN + IF TG_OP = 'UPDATE' THEN + IF OLD.actor_user_id IS NOT NULL + AND NEW.actor_user_id IS NULL + AND (to_jsonb(NEW) - 'actor_user_id') = (to_jsonb(OLD) - 'actor_user_id') + AND NOT EXISTS (SELECT 1 FROM public.users WHERE id = OLD.actor_user_id) THEN + RETURN NEW; + END IF; + END IF; + RAISE EXCEPTION + 'audit_events is append-only; % is not permitted', + TG_OP; +END; +$$ LANGUAGE plpgsql; +""" + +# Migration 005's function, verbatim. +_BLOCK_EVERYTHING = """ +CREATE OR REPLACE FUNCTION audit_events_block_modification() +RETURNS TRIGGER AS $$ +BEGIN + RAISE EXCEPTION + 'audit_events is append-only; % is not permitted', + TG_OP; +END; +$$ LANGUAGE plpgsql; +""" + + +def upgrade() -> None: + if op.get_bind().dialect.name == "postgresql": + op.execute(_ALLOW_ACTOR_ERASURE) + + +def downgrade() -> None: + if op.get_bind().dialect.name == "postgresql": + op.execute(_BLOCK_EVERYTHING) diff --git a/app/core/audit.py b/app/core/audit.py index 45f3f2e..8f33b59 100644 --- a/app/core/audit.py +++ b/app/core/audit.py @@ -11,7 +11,9 @@ Three properties hold: 1. **Append-only at the database layer.** Migration 005 installs a - Postgres trigger that raises on UPDATE / DELETE. SQLite (test + Postgres trigger that raises on UPDATE / DELETE; the one exception + (migration 012) is the ``ON DELETE SET NULL`` cascade that clears + ``actor_user_id`` when that account is deleted. SQLite (test harness only) skips the trigger; we cover the SQLite path with a "verify rejects tampering" test instead. 2. **Forward chain.** ``record_hash[i] = SHA256(record_hash[i-1] || diff --git a/app/db/models.py b/app/db/models.py index 5425728..25f14c8 100644 --- a/app/db/models.py +++ b/app/db/models.py @@ -279,7 +279,9 @@ class AuditEvent(Base): Append-only enforcement at the database layer (Postgres trigger in migration 005) means even a compromised application credential - cannot UPDATE or DELETE rows. SQLite (test harness only) skips the + cannot UPDATE or DELETE rows. The one exception (migration 012) is + the ``ON DELETE SET NULL`` cascade that clears ``actor_user_id`` + when the account it names is deleted. SQLite (test harness only) skips the trigger — the migration scopes the trigger to ``dialect.name == 'postgresql'``. @@ -307,6 +309,10 @@ class AuditEvent(Base): DateTime(timezone=True), nullable=False, server_default=func.now() ) event_type: Mapped[str] = mapped_column(String(64), nullable=False) + # Nulled by the database when the user is deleted (see the class + # docstring). A ``User.audit_events`` relationship would need + # ``passive_deletes="all"``: otherwise the ORM nulls the rows itself + # while the user still exists, and the trigger refuses the deletion. actor_user_id: Mapped[Optional[uuid.UUID]] = mapped_column( UUID(as_uuid=True), ForeignKey("users.id", ondelete="SET NULL"), diff --git a/docs/dpa-tom-annex.md b/docs/dpa-tom-annex.md index d786d34..3474d58 100644 --- a/docs/dpa-tom-annex.md +++ b/docs/dpa-tom-annex.md @@ -146,8 +146,10 @@ physical assets of its own. ### Input control (Eingabekontrolle) - Tamper-evident audit log: SHA-256 hash chain, Postgres append-only - trigger — `app/core/audit.py`, Migration 005; the `verify_chain` - helper detects retroactive edits from a SQL dump alone. Compatible with + trigger — `app/core/audit.py`, Migrations 005 and 012 (the one change + the trigger allows: a hard account deletion nulls the account ID on + that account's events); the `verify_chain` helper detects retroactive + edits from a SQL dump alone. Compatible with ISO 27001 A.12.4.1 / BORA §50 / BeurkG §39a. It records registration, login, email verification, password reset and account deletion; subscription and payment events, including the withdrawal waiver at diff --git a/docs/vendor-security-questionnaire.md b/docs/vendor-security-questionnaire.md index c737aaa..557eccb 100644 --- a/docs/vendor-security-questionnaire.md +++ b/docs/vendor-security-questionnaire.md @@ -304,8 +304,8 @@ and renews certificates automatically. - **API keys:** SHA-256 hashes only — `app/core/security.py`. Raw key is shown once at creation and never logged. - **Audit log:** plain Postgres rows protected by an append-only - trigger and a SHA-256 hash chain — `app/core/audit.py`, Migration - 005. Backups protect the integrity of the at-rest copy; the chain + trigger and a SHA-256 hash chain — `app/core/audit.py`, Migrations + 005 and 012. Backups protect the integrity of the at-rest copy; the chain detects retroactive edits from a SQL dump alone. - **Database backups:** operator-side. The Compliance Edition deployment encrypts backups at rest and stores them off-site; the AGPL operator @@ -573,7 +573,10 @@ classification. **No file content** is logged. The regression guard is Yes. The audit log is a SHA-256 hash chain over append-only Postgres rows protected by a database trigger -(`app/core/audit.py`, Migration 005). The `verify_chain` helper +(`app/core/audit.py`, Migrations 005 and 012). The one change the +trigger allows is the database nulling the account ID on an account's +events when that account is hard-deleted (Art. 17 GDPR); the chain +still verifies afterwards. The `verify_chain` helper detects retroactive edits from a SQL dump alone — compatible with ISO 27001 A.12.4.1, BORA §50, and BeurkG §39a expectations. diff --git a/tests/test_audit_postgres.py b/tests/test_audit_postgres.py new file mode 100644 index 0000000..ca0c3be --- /dev/null +++ b/tests/test_audit_postgres.py @@ -0,0 +1,247 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +"""The audit log's append-only trigger, on a real PostgreSQL. + +The rest of the suite runs on in-memory SQLite, where the trigger from +migration 005 does not exist. That hid a GDPR Art. 17 bug: the trigger also +refused the ``ON DELETE SET NULL`` cascade that clears +``audit_events.actor_user_id``, so ``DELETE /api/v1/auth/account`` failed on +Postgres for every free account (each has audit rows from registration and +login) and deleted nothing. Migration 012 lets exactly that cascade through. + +These tests run ``alembic upgrade head`` against the database in +``FILEMORPH_TEST_POSTGRES_URL`` (an asyncpg URL) and skip without it. Every +test empties the tables, so the database name must contain ``test``. CI's +``lint-and-test`` job provides a Postgres service; +``test_ci_runs_these_tests_on_postgres`` keeps it that way. +""" + +from __future__ import annotations + +import asyncio +import os +import re +import subprocess +import sys +from pathlib import Path +from unittest.mock import AsyncMock + +import pytest +import yaml +from sqlalchemy import func, select, text +from sqlalchemy.engine import make_url +from sqlalchemy.exc import DBAPIError +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine +from sqlalchemy.pool import NullPool + +from app.core import audit as audit_module +from app.core import email as email_mod +from app.core.audit import verify_chain +from app.db.base import get_db +from app.db.models import AuditEvent, User +from app.main import app + +_ROOT = Path(__file__).resolve().parent.parent +_ENV = "FILEMORPH_TEST_POSTGRES_URL" +_URL = os.environ.get(_ENV, "") +_PASSWORD = "initial-password" + +needs_postgres = pytest.mark.skipif(not _URL, reason=f"{_ENV} is not set") + + +@pytest.fixture(scope="module") +def pg_factory(): + """Migrate the database to head; point the app and the audit writer at it.""" + database = make_url(_URL).database or "" + assert "test" in database, f"{_ENV} must name a throwaway test database, not {database!r}" + migrated = subprocess.run( + [sys.executable, "-m", "alembic", "upgrade", "head"], + cwd=_ROOT, + env={**os.environ, "DATABASE_URL": _URL}, + capture_output=True, + text=True, + encoding="utf-8", + timeout=300, + ) + assert migrated.returncode == 0, migrated.stdout + migrated.stderr + + # NullPool: every session opens its own connection on the event loop that + # uses it. TestClient's loop and the asyncio.run() loops below differ. + engine = create_async_engine(_URL, poolclass=NullPool) + factory = async_sessionmaker(engine, expire_on_commit=False, class_=AsyncSession) + + async def _override_get_db(): + async with factory() as session: + yield session + + app.dependency_overrides[get_db] = _override_get_db + original = audit_module.AsyncSessionLocal + audit_module.AsyncSessionLocal = factory + yield factory + audit_module.AsyncSessionLocal = original + app.dependency_overrides.pop(get_db, None) + + +@pytest.fixture +def pg(pg_factory): + """Empty tables for each test. TRUNCATE fires no row triggers, so the + append-only trigger does not refuse the cleanup.""" + + async def _truncate(): + async with pg_factory() as s: + await s.execute(text("TRUNCATE audit_events, users RESTART IDENTITY CASCADE")) + await s.commit() + + asyncio.run(_truncate()) + return pg_factory + + +@needs_postgres +def test_deleting_a_free_account_nulls_its_audit_actor(client, pg, monkeypatch): + """Registration, login and the deletion request all name the account in + the audit log; deleting it must clear that name, keep the rows and leave + the hash chain intact.""" + monkeypatch.setattr(email_mod, "send_email", AsyncMock()) + email = "erase-me@example.com" + res = client.post("/api/v1/auth/register", json={"email": email, "password": _PASSWORD}) + assert res.status_code == 201, res.text + token = res.json()["access_token"] + res = client.post("/api/v1/auth/login", json={"email": email, "password": _PASSWORD}) + assert res.status_code == 200, res.text + + res = client.request( + "DELETE", + "/api/v1/auth/account", + json={"password": _PASSWORD, "confirm_email": email, "confirm_word": "DELETE"}, + headers={"Authorization": f"Bearer {token}"}, + ) + assert res.status_code == 204, res.text + + async def _state(): + async with pg() as s: + users = await s.scalar(select(func.count()).select_from(User)) + events = (await s.execute(select(AuditEvent).order_by(AuditEvent.id))).scalars().all() + return users, events, await verify_chain(s) + + users, events, broken_at = asyncio.run(_state()) + assert users == 0 + assert {e.event_type for e in events} >= { + "auth.register.success", + "auth.login.success", + "auth.account_deletion.requested", + "auth.account_deletion.completed", + } + assert [e.actor_user_id for e in events] == [None] * len(events) + assert broken_at is None + + +@needs_postgres +@pytest.mark.parametrize( + "statement", + [ + "UPDATE audit_events SET payload_json = '{}' WHERE id = :id", + "UPDATE audit_events SET actor_user_id = NULL WHERE id = :id", + "UPDATE audit_events SET actor_user_id = :other WHERE id = :id", + "DELETE FROM audit_events WHERE id = :id", + ], + ids=["payload", "null-live-actor", "reassign-actor", "delete"], +) +def test_trigger_refuses_every_other_change(pg, statement): + """Only the cascade of an account deletion may clear an actor. A direct + UPDATE that nulls the id of a live account is refused like any other.""" + + async def _run(): + async with pg() as s: + owner = User(email="owner@example.com", password_hash="unused") + other = User(email="other@example.com", password_hash="unused") + s.add_all([owner, other]) + await s.commit() + await audit_module.record_event("auth.login.success", actor_user_id=owner.id, db=s) + event_id = await s.scalar(select(AuditEvent.id)) + params = ( + {"id": event_id, "other": other.id} if ":other" in statement else {"id": event_id} + ) + with pytest.raises(DBAPIError, match="append-only"): + await s.execute(text(statement), params) + + asyncio.run(_run()) + + +@needs_postgres +def test_temporary_users_table_cannot_stand_in(pg): + """The check looks up public.users. A temporary "users" table, which the + search path finds first, must not make a live account look deleted.""" + + async def _run(): + async with pg() as s: + owner = User(email="owner@example.com", password_hash="unused") + s.add(owner) + await s.commit() + await audit_module.record_event("auth.login.success", actor_user_id=owner.id, db=s) + await s.execute(text("CREATE TEMPORARY TABLE users (id uuid)")) + with pytest.raises(DBAPIError, match="append-only"): + await s.execute(text("UPDATE audit_events SET actor_user_id = NULL")) + + asyncio.run(_run()) + + +@needs_postgres +def test_cascade_may_change_nothing_but_the_actor(pg): + """Because of the foreign key, only deleting the account reaches the check + with the account already gone. A second trigger that alters the row on the + way stands in for any other change, and the whole deletion is refused.""" + + async def _run(): + async with pg() as s: + owner = User(email="owner@example.com", password_hash="unused") + s.add(owner) + await s.commit() + await audit_module.record_event("auth.login.success", actor_user_id=owner.id, db=s) + # Rolled back with the failed DELETE below (transactional DDL). Fires + # before audit_events_no_update: same event, alphabetical order. + await s.execute( + text( + "CREATE FUNCTION test_touch_audit_event() RETURNS trigger AS $$ BEGIN " + "NEW.event_type := 'touched'; RETURN NEW; END; $$ LANGUAGE plpgsql" + ) + ) + await s.execute( + text( + "CREATE TRIGGER audit_events_a_touch BEFORE UPDATE ON audit_events " + "FOR EACH ROW EXECUTE FUNCTION test_touch_audit_event()" + ) + ) + with pytest.raises(DBAPIError, match="append-only"): + await s.execute(text("DELETE FROM users WHERE id = :id"), {"id": owner.id}) + + asyncio.run(_run()) + + +_TEST_JOBS = {"ci.yml": "lint-and-test", "deps-latest.yml": "test-latest"} + + +def _test_job(workflow: str) -> dict: + path = _ROOT / ".github" / "workflows" / workflow + return yaml.safe_load(path.read_text(encoding="utf-8"))["jobs"][_TEST_JOBS[workflow]] + + +@pytest.mark.parametrize("workflow", sorted(_TEST_JOBS)) +def test_ci_runs_these_tests_on_postgres(workflow): + """Without the service and the URL, every test above skips and nothing + says so. That is how the trigger went untested until migration 012.""" + job = _test_job(workflow) + image = job["services"]["postgres"]["image"] + assert re.fullmatch(r"postgres:[\w.-]+@sha256:[0-9a-f]{64}", image), ( + f"pin the Postgres service image by digest, got {image!r}" + ) + assert image == _test_job("ci.yml")["services"]["postgres"]["image"], ( + f"{workflow} tests on a different Postgres than ci.yml" + ) + compose = (_ROOT / "docker-compose.cloud.yml").read_text(encoding="utf-8") + major = re.search(r"image:\s*postgres:(\d+)", compose).group(1) + assert image.startswith(f"postgres:{major}."), ( + f"{workflow} tests {image}, docker-compose.cloud.yml runs postgres:{major}" + ) + pytest_steps = [step for step in job["steps"] if "pytest" in step.get("run", "")] + assert pytest_steps, f"{workflow} no longer runs pytest" + for step in pytest_steps: + assert _ENV in step.get("env", {}), f"{step.get('name')!r} does not set {_ENV}"