diff --git a/changelog.d/1943-corpus-file-tools-followup.changed.md b/changelog.d/1943-corpus-file-tools-followup.changed.md new file mode 100644 index 0000000000..f48c6e64ca --- /dev/null +++ b/changelog.d/1943-corpus-file-tools-followup.changed.md @@ -0,0 +1,29 @@ +- **Corpus file-tool review polish (PR #1940 follow-up, issue #1943 items 3–5).** + - **`require_user` helper (`opencontractserver/llms/tools/core_tools/_helpers.py`).** + The 5-line "no user injected / id not found → `PermissionError`" guard that + `rename_document` and `delete_document` each repeated is now a single + `require_user(user_id, caller)` helper (companion to the existing + `get_user_or_none`), preserving the same per-caller error messages. + Read-only tools keep using `get_user_or_none` (which tolerates anonymous + access) (item 4). + - **`delete_document` tool description (`opencontractserver/llms/tools/tool_registry.py`).** + The description now spells out that deletion needs the corpus DELETE tier — + *higher* than the write permission `rename_document`/`move_document` use — + and instructs the LLM to report a `permission denied` failure rather than + retry-loop on it. A WRITE-but-not-DELETE user still passes the coarse + framework `requires_write_permission` gate and is correctly rejected by + `DocumentLifecycleService.soft_delete_document`; tightening this to a + dedicated `requires_delete_permission` flag (so the tool is filtered out + before such a user ever sees it) remains a tracked follow-up (item 3). + - **Direct unit test for `sanitize_corpus_filename`** + (`opencontractserver/tests/test_shared_utils.py`). The canonical + corpus-filename sanitiser was only exercised indirectly through the rename + integration paths; it now has a focused test covering empty/None → + fallback, all-special-chars → underscores, path-separator collapse (no + directory traversal), no run-collapsing, leading-dot preservation, and + truncation at `MAX_FILENAME_LENGTH` (item 5). + - **Not changed (item 2):** `CorpusDocumentService._check_document_in_corpus` + was kept private. It was deliberately renamed from public to underscore- + prefixed in PR #1685 to signal its "NO PERMISSION CHECK" contract; its + cross-service callers all live inside `corpuses.services` and gate corpus + READ upstream, so the leading underscore (service-internal use) is correct. diff --git a/changelog.d/1943-corpus-file-tools-followup.fixed.md b/changelog.d/1943-corpus-file-tools-followup.fixed.md new file mode 100644 index 0000000000..faef2fc8db --- /dev/null +++ b/changelog.d/1943-corpus-file-tools-followup.fixed.md @@ -0,0 +1,13 @@ +- **`rename_document` agent tool: dropped an extra query and closed a status race (PR #1940 follow-up, issue #1943 item 1).** + `FolderDocumentService.rename_document` + (`opencontractserver/corpuses/services/folder_documents.py`) now returns a + 4-tuple `(success, error, new_path, changed)` — the new `changed` boolean is + `True` only for a real rename and `False` for a no-op (sanitised name already + matched) or any failure. The tool + (`opencontractserver/llms/tools/core_tools/documents.py`) previously + snapshotted the pre-rename `DocumentPath.path` and compared it to the + service's returned path to label the response `"renamed"` vs `"unchanged"`. + That extra `DocumentPath` read is gone, and so is the snapshot-vs-service race + window where a concurrent rename could mislabel the `status` (the underlying + data was always safe — the service's `select_for_update` guarantees that; + only the tool-layer status string could drift). diff --git a/opencontractserver/corpuses/services/folder_documents.py b/opencontractserver/corpuses/services/folder_documents.py index 1f3ca646e2..3a06b52a2a 100644 --- a/opencontractserver/corpuses/services/folder_documents.py +++ b/opencontractserver/corpuses/services/folder_documents.py @@ -327,7 +327,7 @@ def rename_document( new_name: str, *, request: Any = None, - ) -> tuple[bool, str, str | None]: + ) -> tuple[bool, str, str | None, bool]: """Rename a document's file within a corpus (change the filename only). Creates a new immutable :class:`DocumentPath` history node whose @@ -358,10 +358,14 @@ def rename_document( new_name: Desired new filename (last path segment only). Returns: - ``(success, error_message, new_path)``. ``new_path`` is the + ``(success, error_message, new_path, changed)``. ``new_path`` is the resulting path string on success (possibly disambiguated, e.g. - ``/Legal/Q3_Summary_1.pdf``) and ``None`` on failure. A no-op - rename (the name is unchanged) returns ``(True, "", )``. + ``/Legal/Q3_Summary_1.pdf``) and ``None`` on failure. ``changed`` is + ``True`` only when the filename actually changed and ``False`` for a + no-op (the sanitised name already matched the current filename) or + any failure — letting callers distinguish a real rename from a no-op + without re-reading the pre-rename path. A no-op rename returns + ``(True, "", , False)``. Validations: - User has corpus UPDATE permission @@ -377,14 +381,15 @@ def rename_document( False, "Permission denied: You do not have write access to this corpus", None, + False, ) # Validate document belongs to corpus if not CorpusDocumentService._check_document_in_corpus(document, corpus): - return False, "Document does not belong to this corpus", None + return False, "Document does not belong to this corpus", None, False if not new_name or not new_name.strip(): - return False, "New name must not be empty", None + return False, "New name must not be empty", None, False sanitized = sanitize_corpus_filename(new_name.strip()) @@ -407,7 +412,7 @@ def rename_document( ) if not current: - return False, "No active document path found", None + return False, "No active document path found", None, False # Split the current path into "/" + "". # Stored paths always start with "/", so rpartition yields a @@ -423,7 +428,7 @@ def rename_document( # No-op: the file already has this name. if new_filename == current_filename: - return True, "", current.path + return True, "", current.path, False base_path = f"{directory}{new_filename}" @@ -436,7 +441,7 @@ def rename_document( user=user, ) except ValueError as exc: - return False, str(exc), None + return False, str(exc), None, False except IntegrityError as exc: logger.warning( "IntegrityError renaming document %s in corpus %s after " @@ -445,7 +450,7 @@ def rename_document( corpus.id, exc, ) - return False, f"{PATH_CONFLICT_MSG}, please retry: {exc}", None + return False, f"{PATH_CONFLICT_MSG}, please retry: {exc}", None, False logger.info( "Renamed document %s in corpus %s from %r to %r by user %s", @@ -455,7 +460,7 @@ def rename_document( chosen_path, user.id, ) - return True, "", chosen_path + return True, "", chosen_path, True @classmethod def move_documents_to_folder( diff --git a/opencontractserver/llms/tools/core_tools/_helpers.py b/opencontractserver/llms/tools/core_tools/_helpers.py index 65ae757364..4bfd0c18a8 100644 --- a/opencontractserver/llms/tools/core_tools/_helpers.py +++ b/opencontractserver/llms/tools/core_tools/_helpers.py @@ -54,6 +54,31 @@ def get_user_or_none(user_id: int | None): return None +def require_user(user_id: int | None, caller: str): + """Resolve ``user_id`` to a ``User`` or raise ``PermissionError``. + + The write tools (e.g. ``rename_document`` / ``delete_document``) require an + authenticated, existing user. This collapses the two distinct guard + branches those tools repeated into one helper, preserving the same + ``caller``-specific messages for each failure mode: + + * ``user_id is None`` (no user injected at all) → + ``" requires an authenticated user."`` + * a stale/unknown id (no matching row) → ``"User not found."`` + + Read-only tools that tolerate anonymous access keep using + :func:`get_user_or_none` instead (it returns ``None`` rather than raising). + + Return type is intentionally inferred — see :func:`get_user_or_none`. + """ + if user_id is None: + raise PermissionError(f"{caller} requires an authenticated user.") + user = get_user_or_none(user_id) + if user is None: + raise PermissionError(f"User {user_id} not found.") + return user + + def clamp_limit(limit: int | None, default: int, maximum: int) -> int: """Clamp a caller-supplied ``limit`` into ``[1, maximum]``. diff --git a/opencontractserver/llms/tools/core_tools/documents.py b/opencontractserver/llms/tools/core_tools/documents.py index 1686444de2..b5c1afcb42 100644 --- a/opencontractserver/llms/tools/core_tools/documents.py +++ b/opencontractserver/llms/tools/core_tools/documents.py @@ -44,7 +44,12 @@ from opencontractserver.corpuses.models import Corpus from opencontractserver.documents.models import Document, DocumentPath -from ._helpers import _db_sync_to_async, clamp_limit, get_user_or_none +from ._helpers import ( + _db_sync_to_async, + clamp_limit, + get_user_or_none, + require_user, +) logger = logging.getLogger(__name__) @@ -323,11 +328,7 @@ def rename_document( ValueError: If the document/corpus is not accessible or the rename fails. """ - if user_id is None: - raise PermissionError("rename_document requires an authenticated user.") - user = get_user_or_none(user_id) - if user is None: - raise PermissionError(f"User {user_id} not found.") + user = require_user(user_id, "rename_document") corpus = Corpus.objects.visible_to_user(user).filter(pk=corpus_id).first() if corpus is None: @@ -341,19 +342,13 @@ def rename_document( f"Document with id={document_id} does not exist or is not accessible." ) - # Snapshot the pre-rename path so we can tell a real rename from a no-op - # (the service returns the unchanged path for both — see its docstring). - previous_path = ( - DocumentPath.objects.filter( - document=document, corpus=corpus, is_current=True, is_deleted=False - ) - .values_list("path", flat=True) - .first() - ) - from opencontractserver.corpuses.services import FolderDocumentService - success, error, new_path = FolderDocumentService.rename_document( + # The service reports whether the filename actually changed via ``changed``, + # so the tool no longer snapshots the pre-rename path to infer a no-op — this + # drops an extra query and closes the snapshot/service race that could mislabel + # a concurrent rename's status. + success, error, new_path, changed = FolderDocumentService.rename_document( user=user, document=document, corpus=corpus, @@ -362,18 +357,16 @@ def rename_document( if not success: raise ValueError(f"Rename failed: {error}") - unchanged = new_path == previous_path return { - "status": "unchanged" if unchanged else "renamed", + "status": "renamed" if changed else "unchanged", "document_id": document_id, "corpus_id": corpus_id, "path": new_path, "message": ( - f"Document {document_id} already named '{new_path}' in corpus " + f"Document {document_id} renamed to '{new_path}' in corpus {corpus_id}." + if changed + else f"Document {document_id} already named '{new_path}' in corpus " f"{corpus_id}; no change made." - if unchanged - else f"Document {document_id} renamed to '{new_path}' in " - f"corpus {corpus_id}." ), } @@ -425,11 +418,7 @@ def delete_document( ValueError: If the document/corpus is not accessible or the delete fails. """ - if user_id is None: - raise PermissionError("delete_document requires an authenticated user.") - user = get_user_or_none(user_id) - if user is None: - raise PermissionError(f"User {user_id} not found.") + user = require_user(user_id, "delete_document") corpus = Corpus.objects.visible_to_user(user).filter(pk=corpus_id).first() if corpus is None: diff --git a/opencontractserver/llms/tools/tool_registry.py b/opencontractserver/llms/tools/tool_registry.py index 6d4f5433d8..7c2bbd276f 100644 --- a/opencontractserver/llms/tools/tool_registry.py +++ b/opencontractserver/llms/tools/tool_registry.py @@ -588,7 +588,13 @@ def to_dict(self) -> dict: "Soft-delete a document from the current corpus (move it to the " "corpus trash). The document is NOT permanently erased — it keeps " "its history and can be restored from the corpus trash. Requires " - "delete permission on the corpus." + "the corpus DELETE permission, which is a HIGHER tier than the " + "write/update permission the other file tools (rename_document, " + "move_document) need: a user may be able to rename or move files " + "yet still lack delete rights. If this tool fails with a " + "'permission denied' error, the user cannot delete in this corpus " + "— report that and do NOT retry, as repeating the call will keep " + "failing." ), category=ToolCategory.CORPUS, requires_corpus=True, @@ -598,8 +604,11 @@ def to_dict(self) -> dict: # the framework layer; the authoritative DELETE check lives in # ``DocumentLifecycleService.soft_delete_document`` (defense in depth), # so a WRITE-but-not-DELETE user passes this gate and is then correctly - # rejected by the service. If a ``requires_delete_permission`` flag is - # ever added, switch this entry to it. + # rejected by the service. The description above tells the LLM not to + # retry-loop on that rejection. Tightening this to a dedicated + # ``requires_delete_permission`` flag (so WRITE-only users never see the + # tool) is tracked as a near-term follow-up; switch this entry to it + # when that flag lands. requires_write_permission=True, parameters=(("document_id", "ID of the document to delete", True),), ), diff --git a/opencontractserver/tests/test_shared_utils.py b/opencontractserver/tests/test_shared_utils.py index a226f4155a..c5bcd85bd8 100644 --- a/opencontractserver/tests/test_shared_utils.py +++ b/opencontractserver/tests/test_shared_utils.py @@ -9,6 +9,7 @@ from django.test import TestCase, override_settings +from opencontractserver.constants.document_processing import MAX_FILENAME_LENGTH from opencontractserver.shared.db_utils import table_has_column from opencontractserver.shared.defaults import ( create_model_icon_path, @@ -25,7 +26,10 @@ sanitize_slug, validate_user_slug_or_raise, ) -from opencontractserver.shared.utils import calc_oc_file_path +from opencontractserver.shared.utils import ( + calc_oc_file_path, + sanitize_corpus_filename, +) class TestDefaultFunctions(TestCase): @@ -324,6 +328,65 @@ def test_filename_with_uuid(self): self.assertEqual(result, "uploadfiles/exports/abc-123-def.pdf") +class TestSanitizeCorpusFilename(TestCase): + """Direct unit tests for shared/utils.py sanitize_corpus_filename. + + This is the canonical sanitiser for the filename segment of a + ``DocumentPath.path`` (shared by ``Corpus.add_document``, the text-import + tool and ``FolderDocumentService.rename_document``). It is exercised + indirectly through the rename integration paths, but its invariants — + fallback on empty, special-chars-to-underscore, no directory traversal, + truncation, and leading-dot preservation — deserve an explicit test. + """ + + def test_allowed_chars_pass_through(self): + # Alphanumerics plus -, _ and . survive unchanged. + self.assertEqual( + sanitize_corpus_filename("Report-v2_final.pdf"), "Report-v2_final.pdf" + ) + + def test_empty_input_uses_fallback(self): + self.assertEqual(sanitize_corpus_filename(""), "untitled") + + def test_none_input_uses_fallback(self): + # ``(name or "")`` coerces None before slicing, so None -> fallback too. + self.assertEqual(sanitize_corpus_filename(None), "untitled") + + def test_custom_fallback(self): + self.assertEqual(sanitize_corpus_filename("", fallback="doc"), "doc") + + def test_all_special_chars_become_underscores(self): + # Disallowed chars are replaced (never dropped), so an all-special name + # yields underscores, not the fallback. + self.assertEqual(sanitize_corpus_filename("!!!!"), "____") + + def test_path_separators_collapse_to_underscore(self): + # Slashes (fwd and back) map to "_" so a sanitised name can never + # traverse directories. + self.assertEqual(sanitize_corpus_filename("a/b\\c"), "a_b_c") + + def test_runs_are_not_collapsed(self): + # Each disallowed char maps to its own underscore (documented: runs are + # intentionally NOT collapsed to a single separator). + self.assertEqual(sanitize_corpus_filename("My File"), "My__File") + + def test_leading_dot_preserved(self): + # A leading dot is an allowed char, so dotfiles keep their dot. + self.assertEqual(sanitize_corpus_filename(".gitignore"), ".gitignore") + + def test_truncated_to_max_length(self): + result = sanitize_corpus_filename("a" * (MAX_FILENAME_LENGTH + 50)) + self.assertEqual(len(result), MAX_FILENAME_LENGTH) + self.assertEqual(result, "a" * MAX_FILENAME_LENGTH) + + def test_truncation_happens_before_sanitisation(self): + # The raw input is sliced to MAX_FILENAME_LENGTH first, then each + # surviving char is mapped — so an over-long all-special string yields + # exactly MAX_FILENAME_LENGTH underscores, not more. + result = sanitize_corpus_filename("*" * (MAX_FILENAME_LENGTH + 10)) + self.assertEqual(result, "_" * MAX_FILENAME_LENGTH) + + class TestVectorSearchMixinDimensionMapping(TestCase): """Tests for VectorSearchViaEmbeddingMixin._dimension_to_field mapping."""