From 0f52670b0d5851f8caf2493d1cb7f385928da3d7 Mon Sep 17 00:00:00 2001 From: jwvanderstam Date: Sun, 8 Nov 2026 18:41:12 +0100 Subject: [PATCH] =?UTF-8?q?fix(security):=20R-9=20=E2=80=94=20a=20purge=20?= =?UTF-8?q?destroys=20only=20what=20was=20retired=20first?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single-object purges of a document, conversation, workspace, connector and user checked references and nothing else, so a live row could be hard-deleted in one call, skipping Clark-Wilson's reversible step, and the only active admin could be purged (found again by the review of #525). Each purge now locks its row and refuses unless it is retired, inside its own transaction (`require_retired`); the services turn that into a 409 for a live row and a 404 for a missing one (`purge_refusals`), and every purge route sends it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KkWPqAcWbxfqz1aCP6Qsdx --- .claude/rules/file-map.md | 2 + CHANGELOG.md | 6 + CLAUDE.md | 2 +- SECURITY.md | 2 +- docs/ROADMAP.md | 2 +- src/db/connection.py | 22 ++++ src/db/connectors.py | 3 +- src/db/conversations.py | 3 +- src/db/documents.py | 5 +- src/db/users.py | 5 +- src/db/workspaces.py | 3 +- src/routes_fastapi/auth_routes.py | 2 + src/routes_fastapi/connector_routes.py | 2 + src/routes_fastapi/memory_routes.py | 6 +- src/routes_fastapi/workspace_routes.py | 2 + src/services/connectors.py | 5 +- src/services/conversations.py | 4 +- src/services/documents.py | 4 +- src/services/errors.py | 16 +++ src/services/users.py | 5 +- src/services/workspaces.py | 5 +- .../test_purge_requires_retired.py | 121 ++++++++++++++++++ tests/unit/test_db_advanced.py | 4 +- tests/unit/test_db_users.py | 11 +- tests/unit/test_db_workspaces.py | 11 +- tests/unit/test_purge_preconditions.py | 4 +- tests/unit/test_purge_requires_retired.py | 98 ++++++++++++++ tests/unit/test_repository_scopes.py | 2 +- 28 files changed, 322 insertions(+), 35 deletions(-) create mode 100644 tests/integration/test_purge_requires_retired.py create mode 100644 tests/unit/test_purge_requires_retired.py diff --git a/.claude/rules/file-map.md b/.claude/rules/file-map.md index 8a221bc0..db00c728 100644 --- a/.claude/rules/file-map.md +++ b/.claude/rules/file-map.md @@ -265,6 +265,8 @@ Full module index for LocalChat. **Keep this current** — update in the same co | `tests/unit/test_jwt_library_is_pyjwt.py` | P2-6 — a `python-jose`-signed token still verifies after the swap to PyJWT (the fixture cannot be re-made once jose is uninstalled), and neither lock pins `python-jose` or `ecdsa` again | | `tests/unit/test_local_folder_least_privilege.py` | P0-3 — the `local_folder` connector reaches only inside `CONNECTOR_LOCAL_ROOTS`, and only a global admin may create or repoint one. Covers `/etc`, `..`, a symlink out of an allowed root, and a sibling sharing a path prefix (`commonpath`, not `startswith`) | | `tests/unit/test_object_authorization_matrix.py` | P0-1 — every route addressing an object by id is scoped to a workspace. Walks the AST of `src/` and fails on any call to a workspace-scoped database method that omits `scope=`, so a *new* route cannot repeat C1/C2 | +| `tests/unit/test_purge_requires_retired.py` | R-9 (QP-9 review) — `require_retired` locks the row and refuses a live or missing one, `purge_refusals` makes that a 409 or a 404, and all five purge routes send it | +| `tests/integration/test_purge_requires_retired.py` | R-9 against Postgres, for the document, conversation, connector, workspace and user purges: a live row is refused and survives, a missing one is not found, a retired one is destroyed | | `tests/unit/test_purge_preconditions.py` | The Clark-Wilson purge TPs — a cited conversation or a user with memberships is refused before any DELETE | | `tests/unit/test_token_accounting.py` | OBS-1a — a streamed chat against `fake_ollama` adds exactly the tokens its `done` object reported, the cloud path adds a canned OpenAI-compatible stream's tokens and its cost at the configured prices, a call without both prices still counts tokens, and no counter carries more than the model and path labels | | `tests/unit/test_degradation_paths.py` | P2-4b — the exception paths that change behaviour: the four narrowed handlers absorb a malformed timestamp and let anything else escape, and the settings page still renders when its stats do not (the bug P2-4b itself introduced) | diff --git a/CHANGELOG.md b/CHANGELOG.md index 1840adf3..9546bb50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ OpenAI, `https://api.openai.com/v1`) and drop any `provider/` prefix from `CLOUD `CLOUD_PRICE_OUTPUT_PER_MTOK`; without both, only tokens are counted. A provider that is not OpenAI-shaped (Anthropic's native API, Vertex) is no longer reachable, as ADR-4 accepted. +- **A purge destroys only what was retired first (R-9).** The single-object purges of a + document, conversation, workspace, connector and user checked references and nothing + else, so a live row could be hard-deleted in one call, skipping the reversible step + before it, and the only active admin could be purged. Each purge now locks the row and + refuses unless it is retired: a live row is a 409, a missing one a 404 (a missing + document used to report success, a missing connector a 409 about synced documents). ## [3.5.0] — 2026-10-31 diff --git a/CLAUDE.md b/CLAUDE.md index e35cafa3..7ac0305d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -128,7 +128,7 @@ Hard-deleting a CDI while other data still holds its ID breaks IVPs — citation `conversation_messages` has neither (a message goes with its conversation). - A "delete" TP sets `deleted_at`; it never issues `DELETE FROM` on a CDI. - All SELECT queries on CDIs filter `WHERE deleted_at IS NULL`. -- A hard purge is a *separate, explicitly authorized TP* with a precondition: no active references exist. It is never the default operation. +- A hard purge is a *separate, explicitly authorized TP* with a precondition: the row is retired, and no active references exist. It is never the default operation. **Audit trail** — state changes on CDIs must be traceable: who changed what, when. `deleted_at` + `deleted_by` is the minimum required. diff --git a/SECURITY.md b/SECURITY.md index 4ce76631..d213a5cb 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -309,7 +309,7 @@ fails on the code before it. | R-6 | Medium | `GET /settings` rendered installation-wide admin statistics for any caller | **Fixed** (#507) — `tests/unit/test_settings_page_stats_are_admin_only.py` | | R-7 | Low | A connector's sync history and manual sync took the id alone | **Fixed** (#510) — `TestAConnectorOutsideTheScopeIsNotFound` | | R-8 | Low | Feedback and annotations accept conversation and chunk ids from another workspace; a viewer can make any document un-purgeable | **Fixed** — every id a rating or annotation names is looked up in the caller's scope first, the global tier's chunks included except for a workspace API key; the message id too, which the review did not name. `tests/integration/test_object_authorization_over_the_wire.py`, `tests/integration/test_rating_ids_are_in_scope.py` | -| R-9 | Low | Single-object purges do not require the row to be retired first | **Open** (admin-only) | +| R-9 | Low | Single-object purges do not require the row to be retired first | **Fixed** — each of the five purges locks its row and refuses unless it is retired, inside its own transaction: a live row is a 409 and survives, a missing one a 404. A user purge thereby cannot reach an active admin. `tests/unit/test_purge_requires_retired.py`, `tests/integration/test_purge_requires_retired.py` | | R-10 | Low | Admin user routes take raw dicts; the last-admin count ignores `is_active` | **Open** (admin-only) | | R-11 | Low | The image's source is owned by the runtime user | **Open** | | R-12 | Low | GraphRAG's entity expansion is not scoped (off by default) | **Open** | diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index c99fa414..b07fee55 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1924,7 +1924,7 @@ From `SECURITY.md`'s QP-9 register, in this order: 2. Item 13: a password change ends the account's other sessions (`tokens_valid_after`). 3. R-10: admin user routes take raw dicts; the last-admin count ignores `is_active`. 4. R-5b: a ratchet over the `ALL_WORKSPACES` call sites, each with a recorded reason. -5. R-9, R-11, R-12: purge preconditions, the image's file ownership, GraphRAG's scope. +5. R-9 ✅, R-11, R-12: purge preconditions, the image's file ownership, GraphRAG's scope. #### QP-11 — Replace `litellm` (ADR-4) ✅ (built 2026-11-07, taken ahead of the rest of QP-10) diff --git a/src/db/connection.py b/src/db/connection.py index 0ca34e9a..33e86f98 100644 --- a/src/db/connection.py +++ b/src/db/connection.py @@ -252,6 +252,28 @@ class DatabaseUnavailableError(Exception): pass +class NotRetiredError(Exception): + """A purge named a row that is missing, or still live: Destroy follows Retire (R-9).""" + + def __init__(self, table: str, *, missing: bool) -> None: + super().__init__(f"{table} row is {'missing' if missing else 'not retired'}") + self.missing = missing + + +def require_retired(cur: Any, table: str, row_id: object) -> None: + """Lock the row and refuse unless it is retired, inside the purge's own transaction. + + Locking it means a restore cannot slip in between this check and the DELETE. + """ + cur.execute( + sql.SQL("SELECT deleted_at IS NOT NULL FROM {} WHERE id = %s FOR UPDATE").format(sql.Identifier(table)), + (row_id,), + ) + row = cur.fetchone() + if row is None or not row[0]: + raise NotRetiredError(table, missing=row is None) + + # ============================================================================ # PGVECTOR TYPE ADAPTERS FOR PSYCOPG3 # ============================================================================ diff --git a/src/db/connectors.py b/src/db/connectors.py index 6be4ab73..951c6b54 100644 --- a/src/db/connectors.py +++ b/src/db/connectors.py @@ -14,7 +14,7 @@ from ..utils.logging_config import get_logger from ..utils.scope import ALL_WORKSPACES, Scope, scope_predicate -from .connection import DatabaseConnection, DatabaseUnavailableError +from .connection import DatabaseConnection, DatabaseUnavailableError, require_retired logger = get_logger(__name__) @@ -164,6 +164,7 @@ def purge(self, connector_id: str) -> bool: raise DatabaseUnavailableError("Cannot purge connector: DB not connected") with self._db.get_connection(scope=ALL_WORKSPACES) as conn: with conn.cursor() as cur: + require_retired(cur, "connectors", connector_id) cur.execute( "SELECT 1 FROM documents WHERE source_id = %s AND deleted_at IS NULL LIMIT 1", (connector_id,), diff --git a/src/db/conversations.py b/src/db/conversations.py index b54eee60..750c59e4 100644 --- a/src/db/conversations.py +++ b/src/db/conversations.py @@ -7,7 +7,7 @@ from ..utils.encryption import encrypt as _encrypt from ..utils.logging_config import get_logger, sanitize_log_value from ..utils.scope import ALL_WORKSPACES, Scope, scope_predicate -from .connection import DatabaseConnection, DatabaseUnavailableError +from .connection import DatabaseConnection, DatabaseUnavailableError, require_retired logger = get_logger(__name__) @@ -364,6 +364,7 @@ def purge(self, conversation_id: str) -> bool: with self._db.get_connection(scope=ALL_WORKSPACES) as conn: with conn.cursor() as cursor: + require_retired(cursor, "conversations", conversation_id) cursor.execute( "SELECT 1 FROM memories WHERE source_conv = %s LIMIT 1", (conversation_id,), diff --git a/src/db/documents.py b/src/db/documents.py index 189b4678..961375e0 100644 --- a/src/db/documents.py +++ b/src/db/documents.py @@ -9,7 +9,7 @@ from ..utils.sanitization import escape_sql_like from ..utils.scope import ALL_WORKSPACES, Scope, scope_predicate from ..utils.text import content_terms -from .connection import DatabaseConnection, DatabaseUnavailableError +from .connection import DatabaseConnection, DatabaseUnavailableError, require_retired if TYPE_CHECKING: import psycopg @@ -232,7 +232,7 @@ def retire( def purge(self, doc_id: int) -> bool: """ - Hard-delete only when no chunk_stats row exists for any of this document's chunks. + Hard-delete a retired document only when no chunk_stats row exists for any of its chunks. Returns False when citations block the purge; True on success. """ if not self._db.is_connected: @@ -241,6 +241,7 @@ def purge(self, doc_id: int) -> bool: logger.debug("Purge requested for document ID: %s", doc_id) with self._db.get_connection(scope=ALL_WORKSPACES) as conn: with conn.cursor() as cursor: + require_retired(cursor, "documents", doc_id) # Citation precondition: any chunk that was retrieved blocks the purge. cursor.execute( """ diff --git a/src/db/users.py b/src/db/users.py index b8c9e575..9e6ea276 100644 --- a/src/db/users.py +++ b/src/db/users.py @@ -9,7 +9,7 @@ from ..utils.logging_config import get_logger, sanitize_log_value from ..utils.scope import ALL_WORKSPACES -from .connection import DatabaseConnection, DatabaseUnavailableError +from .connection import DatabaseConnection, DatabaseUnavailableError, require_retired logger = get_logger(__name__) @@ -207,11 +207,12 @@ def retire(self, user_id: str, deleted_by: str | None = None) -> bool: return bool(cur.rowcount > 0) def purge(self, user_id: str) -> bool: - """Hard-delete a user if no workspace memberships exist. Returns False when blocked.""" + """Hard-delete a retired user if no workspace memberships exist. Returns False when blocked.""" if not self._db.is_connected: raise DatabaseUnavailableError("Cannot purge user: DB not connected") with self._db.get_connection(scope=ALL_WORKSPACES) as conn: with conn.cursor() as cur: + require_retired(cur, "users", user_id) cur.execute( "SELECT 1 FROM workspace_members WHERE user_id = %s LIMIT 1", (user_id,), diff --git a/src/db/workspaces.py b/src/db/workspaces.py index 8749733a..79b51a57 100644 --- a/src/db/workspaces.py +++ b/src/db/workspaces.py @@ -10,7 +10,7 @@ from ..utils.logging_config import get_logger from ..utils.scope import ALL_WORKSPACES -from .connection import DatabaseConnection, DatabaseUnavailableError +from .connection import DatabaseConnection, DatabaseUnavailableError, require_retired logger = get_logger(__name__) @@ -189,6 +189,7 @@ def purge(self, workspace_id: str) -> bool: raise DatabaseUnavailableError("Cannot purge workspace: DB not connected") with self._db.get_connection(scope=ALL_WORKSPACES) as conn: with conn.cursor() as cur: + require_retired(cur, "workspaces", workspace_id) cur.execute( "SELECT 1 FROM documents WHERE workspace_id = %s AND deleted_at IS NULL LIMIT 1", (workspace_id,), diff --git a/src/routes_fastapi/auth_routes.py b/src/routes_fastapi/auth_routes.py index 334fc70f..6d6d5469 100644 --- a/src/routes_fastapi/auth_routes.py +++ b/src/routes_fastapi/auth_routes.py @@ -303,6 +303,8 @@ def purge_user(user_id: str, users: Users, admin_user_id: Annotated[str, Depends status_code=409, ) return {"success": True} + except ServiceError as exc: + return _refused(exc) except Exception: logger.exception("[Users] purge error") return JSONResponse({"success": False, "message": _ERR_INTERNAL}, status_code=500) diff --git a/src/routes_fastapi/connector_routes.py b/src/routes_fastapi/connector_routes.py index e6a9ed61..5225fd29 100644 --- a/src/routes_fastapi/connector_routes.py +++ b/src/routes_fastapi/connector_routes.py @@ -171,6 +171,8 @@ def purge_connector( status_code=409, ) return {"success": True} + except ServiceError as exc: + return JSONResponse({"success": False, "message": exc.message}, status_code=exc.status_code) except Exception: logger.exception("[Connectors] purge error") return JSONResponse({"success": False, "message": _ERR_INTERNAL}, status_code=500) diff --git a/src/routes_fastapi/memory_routes.py b/src/routes_fastapi/memory_routes.py index 11c29f1a..e30b68a1 100644 --- a/src/routes_fastapi/memory_routes.py +++ b/src/routes_fastapi/memory_routes.py @@ -10,6 +10,7 @@ from ..security_fastapi import get_current_user_id, require_admin_dep from ..services.conversations import ConversationsService +from ..services.errors import ServiceError from ..utils.logging_config import get_logger from ..utils.workspace import get_scope, get_workspace_id from ._authz import deny as _deny @@ -195,7 +196,10 @@ def purge_conversation( conversations: Conversations, _admin: Annotated[str, Depends(require_admin_dep)], ) -> Any: - purged = conversations.purge(conversation_id) + try: + purged = conversations.purge(conversation_id) + except ServiceError as exc: + return JSONResponse({"success": False, "message": exc.message}, status_code=exc.status_code) if not purged: return JSONResponse( { diff --git a/src/routes_fastapi/workspace_routes.py b/src/routes_fastapi/workspace_routes.py index b38c19be..e2e170e0 100644 --- a/src/routes_fastapi/workspace_routes.py +++ b/src/routes_fastapi/workspace_routes.py @@ -181,6 +181,8 @@ def purge_workspace( status_code=409, ) return {"success": True} + except ServiceError as exc: + return JSONResponse({"success": False, "message": exc.message}, status_code=exc.status_code) except Exception: logger.exception("[Workspaces] purge error") return JSONResponse({"success": False, "message": _ERR_INTERNAL}, status_code=500) diff --git a/src/services/connectors.py b/src/services/connectors.py index cebcbc30..f09bd8f3 100644 --- a/src/services/connectors.py +++ b/src/services/connectors.py @@ -22,7 +22,7 @@ from ..utils.logging_config import get_logger from ..utils.scope import Scope -from .errors import ServiceError +from .errors import ServiceError, purge_refusals if TYPE_CHECKING: from ..connectors.base import BaseConnector @@ -167,7 +167,8 @@ def retire(self, connector_id: str, actor: str | None, scope: Scope) -> bool: return retired def purge(self, connector_id: str) -> bool: - return self._db.connectors.purge(connector_id) + with purge_refusals("Connector"): + return self._db.connectors.purge(connector_id) def sync_history(self, connector_id: str, limit: int, scope: Scope) -> list[dict[str, Any]] | None: """The connector's sync log, or None when *scope* has no such connector. diff --git a/src/services/conversations.py b/src/services/conversations.py index 22194fa3..c33a85d2 100644 --- a/src/services/conversations.py +++ b/src/services/conversations.py @@ -11,6 +11,7 @@ from typing import TYPE_CHECKING, Any from ..utils.scope import Scope +from .errors import purge_refusals if TYPE_CHECKING: from ..db import Database @@ -57,7 +58,8 @@ def retire_all(self, actor: str | None, scope: Scope) -> int: return self._db.conversations.retire_all(deleted_by=_actor(actor), scope=scope) def purge(self, conversation_id: str) -> bool: - return self._db.conversations.purge(conversation_id) + with purge_refusals("Conversation"): + return self._db.conversations.purge(conversation_id) def _actor(user_id: str | None) -> str | None: diff --git a/src/services/documents.py b/src/services/documents.py index 3ea246f8..145fd3d2 100644 --- a/src/services/documents.py +++ b/src/services/documents.py @@ -15,7 +15,7 @@ from ..db.connection import DatabaseUnavailableError from ..utils.logging_config import get_logger from ..utils.scope import Scope -from .errors import ServiceError +from .errors import ServiceError, purge_refusals if TYPE_CHECKING: from ..db import Database @@ -80,7 +80,7 @@ def retire_all(self, actor: str | None, scope: Scope) -> int: return self._db.documents.retire_all(scope=scope, deleted_by=_actor(actor)) def purge(self, doc_id: int) -> bool: - with _database_required(): + with _database_required(), purge_refusals("Document"): return self._db.documents.purge(doc_id) def purge_all(self) -> int: diff --git a/src/services/errors.py b/src/services/errors.py index f690b4fe..aca4d2ea 100644 --- a/src/services/errors.py +++ b/src/services/errors.py @@ -6,6 +6,11 @@ from __future__ import annotations +from collections.abc import Iterator +from contextlib import contextmanager + +from ..db.connection import NotRetiredError + class ServiceError(Exception): """A refusal the caller should see. ``message`` is safe to send as-is.""" @@ -14,3 +19,14 @@ def __init__(self, status_code: int, message: str) -> None: super().__init__(message) self.status_code = status_code self.message = message + + +@contextmanager +def purge_refusals(what: str) -> Iterator[None]: + """A purge of a missing row is a 404; of a live one, a 409 (R-9).""" + try: + yield + except NotRetiredError as exc: + if exc.missing: + raise ServiceError(404, f"{what} not found") from exc + raise ServiceError(409, f"{what} must be retired before it is purged") from exc diff --git a/src/services/users.py b/src/services/users.py index 3e29bd1b..09b404f5 100644 --- a/src/services/users.py +++ b/src/services/users.py @@ -15,7 +15,7 @@ from ..security_fastapi import verify_credentials_db from ..utils.logging_config import get_logger -from .errors import ServiceError +from .errors import ServiceError, purge_refusals from .passwords import hash_user_password, verify_user_password from .workspaces import WorkspacesService @@ -139,7 +139,8 @@ def retire_user(self, user_id: str, actor: str) -> bool: return self._db.users.retire(user_id, deleted_by=actor) def purge_user(self, user_id: str) -> bool: - return self._db.users.purge(user_id) + with purge_refusals("User"): + return self._db.users.purge(user_id) def user_workspaces(self, user_id: str) -> list[dict[str, Any]]: workspaces: list[dict[str, Any]] = self._db.members.for_user(user_id) diff --git a/src/services/workspaces.py b/src/services/workspaces.py index feb5cbce..059deaa3 100644 --- a/src/services/workspaces.py +++ b/src/services/workspaces.py @@ -14,7 +14,7 @@ from ..db.workspaces import LastOwnerError from ..utils.logging_config import get_logger from ..utils.scope import Scope -from .errors import ServiceError +from .errors import ServiceError, purge_refusals if TYPE_CHECKING: from ..db import Database @@ -86,7 +86,8 @@ def default_id(self) -> str | None: return self._db.workspaces.default_id() def purge(self, workspace_id: str) -> bool: - return self._db.workspaces.purge(workspace_id) + with purge_refusals("Workspace"): + return self._db.workspaces.purge(workspace_id) def ontology(self, top_n: int, scope: Scope) -> dict[str, Any]: return self._db.entities.workspace_ontology(top_n=top_n, scope=scope) diff --git a/tests/integration/test_purge_requires_retired.py b/tests/integration/test_purge_requires_retired.py new file mode 100644 index 00000000..d9c9ff10 --- /dev/null +++ b/tests/integration/test_purge_requires_retired.py @@ -0,0 +1,121 @@ +"""R-9 (QP-9 review) against Postgres — a purge destroys only what was retired first. + +Each single-object purge checked its references and nothing else, so a live document, +conversation, workspace, connector or user could be hard-deleted in one call, skipping +the reversible step Clark-Wilson puts before it. Now the purge locks the row and refuses +unless it is retired: a live row is a 409 and survives, a missing one is a 404, and a +retired one with nothing citing it is still destroyed. +""" + +from __future__ import annotations + +import uuid +from collections.abc import Iterator +from types import SimpleNamespace +from typing import Any +from unittest.mock import MagicMock + +import pytest + +from src.services.connectors import ConnectorsService +from src.services.conversations import ConversationsService +from src.services.documents import DocumentsService +from src.services.errors import ServiceError +from src.services.users import UsersService +from src.services.workspaces import WorkspacesService +from src.utils.scope import ALL_WORKSPACES + +pytestmark = [pytest.mark.integration, pytest.mark.db] + + +@pytest.fixture(scope="module") +def database() -> Any: + from src.db import Database + + db = Database() + ok, message = db.initialize() + if not ok or not db.is_connected: + pytest.skip(f"PostgreSQL is not available: {message}") + from src.app_bootstrap import _run_alembic_migrations + + _run_alembic_migrations() + return db + + +@pytest.fixture() +def workspace(database: Any) -> Iterator[str]: + ws = database.workspaces.create(f"r9-{uuid.uuid4().hex[:8]}", owner_id=None) + yield ws + database.workspaces.retire(ws) + + +def _tag() -> str: + return uuid.uuid4().hex[:8] + + +KINDS = { + "document": SimpleNamespace( + table="documents", + create=lambda db, ws: db.documents.insert(f"r9-{_tag()}.txt", "x", workspace_id=ws, scope=ws), + retire=lambda db, row, ws: db.documents.retire(row, scope=ws), + purge=lambda db, row: DocumentsService(db).purge(row), + ), + "conversation": SimpleNamespace( + table="conversations", + create=lambda db, ws: db.conversations.create("r9", workspace_id=ws), + retire=lambda db, row, ws: db.conversations.retire(row, scope=ws), + purge=lambda db, row: ConversationsService(db).purge(row), + ), + "connector": SimpleNamespace( + table="connectors", + create=lambda db, ws: db.connectors.create("webhook", f"r9-{_tag()}", {}, workspace_id=ws), + retire=lambda db, row, ws: db.connectors.retire(row, scope=ws), + purge=lambda db, row: ConnectorsService(db, MagicMock()).purge(row), + ), + "workspace": SimpleNamespace( + table="workspaces", + create=lambda db, ws: db.workspaces.create(f"r9-inner-{_tag()}", owner_id=None), + retire=lambda db, row, ws: db.workspaces.retire(row), + purge=lambda db, row: WorkspacesService(db).purge(row), + ), + "user": SimpleNamespace( + table="users", + create=lambda db, ws: db.users.create(f"r9-{_tag()}", "x", role="user"), + retire=lambda db, row, ws: db.users.retire(row), + purge=lambda db, row: UsersService(db).purge_user(row), + ), +} +MISSING = {"documents": 2_000_000_000, "conversations": str(uuid.uuid4()), "connectors": str(uuid.uuid4()), + "workspaces": str(uuid.uuid4()), "users": str(uuid.uuid4())} + + +def _exists(database: Any, table: str, row: Any) -> bool: + with database.get_connection(scope=ALL_WORKSPACES) as conn: + found = conn.execute(f"SELECT 1 FROM {table} WHERE id = %s", (row,)).fetchone() # noqa: S608 — table is a literal from KINDS + return found is not None + + +@pytest.mark.parametrize("kind", KINDS) +def test_a_live_row_is_refused_and_survives(database, workspace, kind): + k = KINDS[kind] + row = k.create(database, workspace) + with pytest.raises(ServiceError) as refused: + k.purge(database, row) + assert (refused.value.status_code, _exists(database, k.table, row)) == (409, True) + + +@pytest.mark.parametrize("kind", KINDS) +def test_a_retired_row_is_purged(database, workspace, kind): + """The permitted case: Retire, then Destroy.""" + k = KINDS[kind] + row = k.create(database, workspace) + k.retire(database, row, workspace) + assert (k.purge(database, row), _exists(database, k.table, row)) == (True, False) + + +@pytest.mark.parametrize("kind", KINDS) +def test_a_missing_row_is_not_found(database, kind): + k = KINDS[kind] + with pytest.raises(ServiceError) as refused: + k.purge(database, MISSING[k.table]) + assert refused.value.status_code == 404 diff --git a/tests/unit/test_db_advanced.py b/tests/unit/test_db_advanced.py index bd40b43b..d3aaa18b 100644 --- a/tests/unit/test_db_advanced.py +++ b/tests/unit/test_db_advanced.py @@ -573,7 +573,7 @@ def test_purge_succeeds_and_deletes_rows_when_no_citations(self): """purge_document returns True and issues DELETEs when no citations exist.""" db = self._make_db() cursor = MagicMock() - cursor.fetchone.return_value = (False,) # no citations + cursor.fetchone.side_effect = [(True,), (False,)] # retired, no citations conn = self._make_conn_ctx(cursor) with patch.object(db, 'get_connection') as mock_gc: @@ -582,7 +582,7 @@ def test_purge_succeeds_and_deletes_rows_when_no_citations(self): result = db.documents.purge(5) assert result is True - calls = [call[0][0].upper() for call in cursor.execute.call_args_list] + calls = [str(call[0][0]).upper() for call in cursor.execute.call_args_list] assert any('DELETE' in sql for sql in calls) def _make_conn_ctx(self, cursor): diff --git a/tests/unit/test_db_users.py b/tests/unit/test_db_users.py index dff49ac0..19f111e8 100644 --- a/tests/unit/test_db_users.py +++ b/tests/unit/test_db_users.py @@ -335,15 +335,16 @@ def test_raises_when_db_unavailable(self): m.purge("uid") def test_blocked_when_workspace_memberships_exist(self): - m, _, cur = _users_mixin(fetchone_return=(1,)) + m, _, cur = _users_mixin() + cur.fetchone.side_effect = [(True,), (1,)] # retired, but a membership row result = m.purge("uid") assert result is False - # Only the membership-check SELECT should run, not a DELETE. - assert cur.execute.call_count == 1 + # Only the retired check and the membership check run, not a DELETE. + assert cur.execute.call_count == 2 def test_hard_deletes_when_no_memberships(self): m, _, cur = _users_mixin() - cur.fetchone.return_value = None # no membership row found + cur.fetchone.side_effect = [(True,), None] # retired, no membership row cur.rowcount = 1 result = m.purge("uid") assert result is True @@ -352,7 +353,7 @@ def test_hard_deletes_when_no_memberships(self): def test_returns_false_when_delete_matches_no_row(self): m, _, cur = _users_mixin() - cur.fetchone.return_value = None + cur.fetchone.side_effect = [(True,), None] cur.rowcount = 0 assert m.purge("uid") is False diff --git a/tests/unit/test_db_workspaces.py b/tests/unit/test_db_workspaces.py index d7349d31..6f0eef9d 100644 --- a/tests/unit/test_db_workspaces.py +++ b/tests/unit/test_db_workspaces.py @@ -259,21 +259,22 @@ def test_raises_when_db_unavailable(self): m.workspaces.purge("wid") def test_blocked_when_live_documents_exist(self): - m, _, cur = _workspaces_mixin(fetchone_return=(1,)) + m, _, cur = _workspaces_mixin() + cur.fetchone.side_effect = [(True,), (1,)] # retired, but a live document result = m.workspaces.purge("wid") assert result is False - assert cur.execute.call_count == 1 + assert cur.execute.call_count == 2 def test_blocked_when_live_conversations_exist(self): m, _, cur = _workspaces_mixin() - cur.fetchone.side_effect = [None, (1,)] # no docs, but a live conversation + cur.fetchone.side_effect = [(True,), None, (1,)] # retired, no docs, but a live conversation result = m.workspaces.purge("wid") assert result is False - assert cur.execute.call_count == 2 + assert cur.execute.call_count == 3 def test_hard_deletes_when_no_live_references(self): m, _, cur = _workspaces_mixin() - cur.fetchone.side_effect = [None, None] + cur.fetchone.side_effect = [(True,), None, None] cur.rowcount = 1 result = m.workspaces.purge("wid") assert result is True diff --git a/tests/unit/test_purge_preconditions.py b/tests/unit/test_purge_preconditions.py index 9abf9744..d80e3d6f 100644 --- a/tests/unit/test_purge_preconditions.py +++ b/tests/unit/test_purge_preconditions.py @@ -26,9 +26,9 @@ def _mixin(cls, *, cites: bool, rowcount: int = 1, connected: bool = True): - """A mixin whose first SELECT answers "something still references this" or not.""" + """A mixin whose row is retired (R-9), and whose next SELECT answers "something still references this" or not.""" cur = MagicMock() - cur.fetchone.return_value = (1,) if cites else None + cur.fetchone.side_effect = [(True,), (1,) if cites else None] cur.rowcount = rowcount cur.__enter__ = MagicMock(return_value=cur) cur.__exit__ = MagicMock(return_value=False) diff --git a/tests/unit/test_purge_requires_retired.py b/tests/unit/test_purge_requires_retired.py new file mode 100644 index 00000000..0de8207d --- /dev/null +++ b/tests/unit/test_purge_requires_retired.py @@ -0,0 +1,98 @@ +"""R-9 (QP-9 review) — a purge destroys only what was retired first. + +``require_retired`` locks the row and refuses a live or missing one inside the purge's +transaction; ``purge_refusals`` turns that into a 409 or a 404; each purge route sends +it. Against Postgres, for all five purges: ``tests/integration/test_purge_requires_retired.py``. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from src.db.connection import NotRetiredError, require_retired +from src.services.errors import ServiceError, purge_refusals +from tests.utils.auth import admin_headers, authenticated_state + +pytestmark = pytest.mark.unit + +ROW = "77777777-7777-7777-7777-777777777777" + + +def _cursor(answer: tuple[bool] | None) -> MagicMock: + cur = MagicMock() + cur.fetchone.return_value = answer + return cur + + +class TestTheCheck: + @pytest.mark.parametrize(("answer", "missing"), [(None, True), ((False,), False)], ids=["missing", "live"]) + def test_a_missing_or_live_row_is_refused(self, answer, missing): + with pytest.raises(NotRetiredError) as refused: + require_retired(_cursor(answer), "users", ROW) + assert refused.value.missing is missing + + def test_a_retired_row_passes(self): + assert require_retired(_cursor((True,)), "users", ROW) is None + + def test_the_row_is_locked_until_the_purge_commits(self): + """Without the lock a restore could land between the check and the DELETE.""" + cur = _cursor((True,)) + require_retired(cur, "users", ROW) + statement = cur.execute.call_args.args[0].as_string(None) + assert statement == 'SELECT deleted_at IS NOT NULL FROM "users" WHERE id = %s FOR UPDATE' + + +class TestTheRefusal: + @pytest.mark.parametrize(("missing", "status", "message"), [ + (True, 404, "User not found"), + (False, 409, "User must be retired before it is purged"), + ]) + def test_it_becomes_a_404_or_a_409(self, missing, status, message): + with pytest.raises(ServiceError) as refused, purge_refusals("User"): + raise NotRetiredError("users", missing=missing) + assert (refused.value.status_code, refused.value.message) == (status, message) + + +def _client(router_module: str, prefix: str = "/api") -> tuple[TestClient, MagicMock]: + import importlib + + router = importlib.import_module(router_module).router + state = authenticated_state(role="admin") + app = FastAPI() + app.include_router(router, prefix=prefix) + app.state = state + client = TestClient(app, raise_server_exceptions=True) + client.headers.update(admin_headers()) + return client, state.db + + +ROUTES = { + "user": ("src.routes_fastapi.auth_routes", "/api", f"/api/users/{ROW}/purge", "users"), + "conversation": ("src.routes_fastapi.memory_routes", "/api", f"/api/conversations/{ROW}/purge", "conversations"), + "workspace": ("src.routes_fastapi.workspace_routes", "/api", f"/api/workspaces/{ROW}/purge", "workspaces"), + "connector": ("src.routes_fastapi.connector_routes", "/api", f"/api/connectors/{ROW}/purge", "connectors"), + "document": ("src.routes_fastapi.document_routes", "/api/documents", "/api/documents/5/purge", "documents"), +} + + +class TestEveryPurgeRouteSendsIt: + @pytest.mark.parametrize("kind", ROUTES) + def test_a_live_row_is_a_409(self, kind): + module, prefix, path, repo = ROUTES[kind] + client, db = _client(module, prefix) + getattr(db, repo).purge.side_effect = NotRetiredError(repo, missing=False) + resp = client.delete(path) + assert (resp.status_code, resp.json()["message"]) == ( + 409, f"{kind.capitalize()} must be retired before it is purged") + + @pytest.mark.parametrize("kind", ROUTES) + def test_a_missing_row_is_a_404(self, kind): + module, prefix, path, repo = ROUTES[kind] + client, db = _client(module, prefix) + getattr(db, repo).purge.side_effect = NotRetiredError(repo, missing=True) + resp = client.delete(path) + assert (resp.status_code, resp.json()["message"]) == (404, f"{kind.capitalize()} not found") diff --git a/tests/unit/test_repository_scopes.py b/tests/unit/test_repository_scopes.py index 5d409e02..468beea4 100644 --- a/tests/unit/test_repository_scopes.py +++ b/tests/unit/test_repository_scopes.py @@ -100,7 +100,7 @@ class TestConnectorWorkerPathsAreInstallationWide: """The sync worker and an admin's purge act for no request, so they carry the flag.""" def test_purge(self): - host = _Host(fetchone=None, rowcount=1) + host = _Host(fetchone=(True,), rowcount=1) ConnectorsRepository(host).purge("c-1") assert host.scopes == [ALL_WORKSPACES]