fix: authorize ownership and validate before deleting in DELETE /documents

app/routes/document_routes.py

@@ -318,22 +318,58 @@ async def get_documents_by_ids(request: Request, ids: list[str] = Query(...)):
 
 
 @router.delete("/documents")
-async def delete_documents(request: Request, document_ids: List[str] = Body(...)):
+async def delete_documents(
+    request: Request,
+    document_ids: List[str] = Body(...),
+    entity_id: str = None,
+):
+    user_authorized = get_user_id(request, entity_id)
     try:
         if isinstance(vector_store, AsyncPgVector):
             existing_ids = await vector_store.get_filtered_ids(
                 document_ids, executor=request.app.state.thread_pool
             )
-            await vector_store.delete(
-                ids=document_ids, executor=request.app.state.thread_pool
+            documents = await vector_store.get_documents_by_ids(
+                document_ids, executor=request.app.state.thread_pool
             )
         else:
             existing_ids = vector_store.get_filtered_ids(document_ids)
-            vector_store.delete(ids=document_ids)
+            documents = vector_store.get_documents_by_ids(document_ids)
 
+        # Validate existence BEFORE deleting anything. Previously the delete ran
+        # first and the 404 check came after, so a request mixing valid and
+        # unknown ids destroyed the valid rows and still returned "not found".
         if not all(id in existing_ids for id in document_ids):
             raise HTTPException(status_code=404, detail="One or more IDs not found")
 
+        # Ownership check: every chunk must be unowned or owned by the requester,
+        # so a caller cannot delete another tenant's documents by file_id.
+        unauthorized = sorted(
+            {
+                doc.metadata.get("file_id")
+                for doc in documents
+                if doc.metadata.get("user_id") not in (None, user_authorized)
+            }
+        )
+        if unauthorized:
+            logger.warning(
+                "Unauthorized delete attempt by user %s for file_ids %s",
+                user_authorized,
+                unauthorized,
+            )
+            raise HTTPException(
+                status_code=403,
+                detail="Not authorized to delete one or more of the specified documents",
+            )
+
+        # Only now perform the destructive delete.
+        if isinstance(vector_store, AsyncPgVector):
+            await vector_store.delete(
+                ids=document_ids, executor=request.app.state.thread_pool
+            )
+        else:
+            vector_store.delete(ids=document_ids)
+
         file_count = len(document_ids)
         return {
             "message": f"Documents for {file_count} file{'s' if file_count > 1 else ''} deleted successfully"

tests/test_main.py

@@ -279,3 +279,53 @@ def test_extract_text_from_file(tmp_path, auth_headers):
     assert json_data["file_id"] == "test_text_123"
     assert json_data["filename"] == "test_text_extraction.txt"
     assert json_data["known_type"] is True  # text files are known types
+
+
+def test_delete_documents_rejects_other_users(auth_headers, monkeypatch):
+    """DELETE must refuse (403) to delete documents owned by another user and
+    must not call delete (regression: there was no ownership check)."""
+    from app.services.vector_store.async_pg_vector import AsyncPgVector
+
+    async def other_user_docs(self, ids, executor=None):
+        return [
+            Document(page_content="x", metadata={"file_id": i, "user_id": "someoneelse"})
+            for i in ids
+        ]
+
+    monkeypatch.setattr(AsyncPgVector, "get_documents_by_ids", other_user_docs)
+
+    delete_calls = []
+
+    async def spy_delete(self, ids=None, collection_only=False, executor=None):
+        delete_calls.append(ids)
+
+    monkeypatch.setattr(AsyncPgVector, "delete", spy_delete)
+
+    response = client.request(
+        "DELETE", "/documents", json=["testid1"], headers=auth_headers
+    )
+    assert response.status_code == 403, response.text
+    assert delete_calls == [], "delete must not run for unauthorized request"
+
+
+def test_delete_documents_validates_before_deleting(auth_headers, monkeypatch):
+    """Unknown ids must 404 without destroying the valid rows.
+
+    Regression: delete ran before the existence check, so a request mixing
+    valid and unknown ids destroyed the valid rows and still returned 404.
+    """
+    from app.services.vector_store.async_pg_vector import AsyncPgVector
+
+    delete_calls = []
+
+    async def spy_delete(self, ids=None, collection_only=False, executor=None):
+        delete_calls.append(ids)
+
+    monkeypatch.setattr(AsyncPgVector, "delete", spy_delete)
+
+    # "ghost" is not among the dummy's known ids (testid1/testid2)
+    response = client.request(
+        "DELETE", "/documents", json=["testid1", "ghost"], headers=auth_headers
+    )
+    assert response.status_code == 404, response.text
+    assert delete_calls == [], "delete must not run when an id is missing"