diff --git a/.claude/rules/file-map.md b/.claude/rules/file-map.md index b5c2369c..3e729a6e 100644 --- a/.claude/rules/file-map.md +++ b/.claude/rules/file-map.md @@ -274,6 +274,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 467eef12..ffe587a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,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). - **Every installation-wide scope carries a reason (R-5b).** A unit test walks `src/`, `mcp_servers/` and `plugins/` and fails on a function that passes `ALL_WORKSPACES` without a recorded reason, on a reason whose function no longer passes it, and on an import that renames it. The 44 functions that pass it today were each checked against every caller that can reach them; none lets a workspace user reach another workspace's rows. - **GraphRAG's query expansion reads only the caller's workspace (R-12).** Entities are shared across workspaces, so with `GRAPH_RAG_ENABLED` on, the 1-hop lookup added names that co-occurred in any workspace's documents, retired ones included, to the caller's query. A co-occurrence now counts only through a live document of the scope. GraphRAG is off by default. - **A password change ends the account's other sessions (`SECURITY.md` item 13).** A diff --git a/CLAUDE.md b/CLAUDE.md index 09e08fe2..b3ba4816 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 ca4947a8..ae1f3491 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -334,7 +334,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). Includes a user: purge has no last-admin check, so the only active admin, with no memberships, can be hard-deleted (found by the review of #525) | +| 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` | **Fixed** — the three routes validate a Pydantic body, so a string `"false"` no longer passes the last-admin rule (Postgres stored it as false), and the rule counts active admins only. The env-var admin's bootstrap check still counts live admins. `tests/unit/test_admin_user_bodies_are_validated.py`, `tests/integration/test_admin_count_is_of_active_admins.py`. Known residual: the rule reads the count and writes on separate connections, so the last two active admins deactivating each other at the same moment both succeed; the repair is SQL by hand (older than R-10) | | 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) | **Fixed** — a co-occurrence counts only through a live document of the caller's workspace; retired documents contributed too. `tests/integration/test_graph_expansion_scope.py` | diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 87f2d271..39c1eaf3 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1925,7 +1925,7 @@ From `SECURITY.md`'s QP-9 register, in this order: migration 0024). 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 5077ff48..cc32871a 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 ebadcb99..89fb623a 100644 --- a/src/db/users.py +++ b/src/db/users.py @@ -10,7 +10,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__) @@ -228,11 +228,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 94598f3b..fa72dadf 100644 --- a/src/routes_fastapi/auth_routes.py +++ b/src/routes_fastapi/auth_routes.py @@ -308,6 +308,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 62f65559..977298c0 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 9b3ee52c..0c8ff7e8 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 @@ -158,7 +158,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 9523475f..57fa41c8 100644 --- a/tests/unit/test_db_users.py +++ b/tests/unit/test_db_users.py @@ -345,15 +345,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 @@ -362,7 +363,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]