🐾 fix(api): treat a NULL owner as unclaimed, not foreign
The previous commit's guard rejected any library whose owner_username didn't match the caller — including NULL. On the live instance 12 of 16 workspace libraries have no owner (created before owner stamping, or via the plain /libraries/ endpoint, which never sets one), so that turned a silent false 204 into a hard 409 and made them undeletable. Strictly worse: the orphan became permanent rather than merely unrecorded. A NULL owner is unclaimed. Only a *different* non-null owner is foreign, and only that returns 409. Verified against live Neo4j on umbriel with throwaway libraries: NULL owner deletes (204, node gone), caller-owned deletes (204, node gone), other-owned is refused (409, node survives), absent stays 204. Test data cleaned up; the 30 real libraries were untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -213,14 +213,24 @@ def workspace_detail_or_delete(request, workspace_id):
|
|||||||
except Library.DoesNotExist:
|
except Library.DoesNotExist:
|
||||||
lib = None
|
lib = None
|
||||||
|
|
||||||
# Cross-user reads look like "not found" — don't disclose existence
|
# A NULL owner_username is *unclaimed*, not someone else's: workspace
|
||||||
# across users. DELETE is handled separately below: telling the caller
|
# libraries created before owner stamping (and any created through the
|
||||||
# "deleted" about a library we did not touch is what silently orphans
|
# plain /libraries/ endpoint, which never sets it) have no owner. Treating
|
||||||
# libraries, and an orphan holds its globally-unique name forever.
|
# those as foreign would make them permanently undeletable through this
|
||||||
unowned = lib is not None and lib.owner_username != request.user.username
|
# endpoint — the very orphans this view is trying not to create.
|
||||||
|
#
|
||||||
|
# Cross-user reads still look like "not found"; DELETE is handled
|
||||||
|
# separately below, because telling the caller "deleted" about a library
|
||||||
|
# we did not touch is what silently orphans libraries, and an orphan holds
|
||||||
|
# its globally-unique name forever.
|
||||||
|
foreign = (
|
||||||
|
lib is not None
|
||||||
|
and lib.owner_username
|
||||||
|
and lib.owner_username != request.user.username
|
||||||
|
)
|
||||||
|
|
||||||
if request.method == "GET":
|
if request.method == "GET":
|
||||||
if lib is None or unowned:
|
if lib is None or foreign:
|
||||||
return Response(
|
return Response(
|
||||||
{"detail": "Workspace not found."},
|
{"detail": "Workspace not found."},
|
||||||
status=status.HTTP_404_NOT_FOUND,
|
status=status.HTTP_404_NOT_FOUND,
|
||||||
@@ -231,7 +241,7 @@ def workspace_detail_or_delete(request, workspace_id):
|
|||||||
if lib is None:
|
if lib is None:
|
||||||
return Response(status=status.HTTP_204_NO_CONTENT)
|
return Response(status=status.HTTP_204_NO_CONTENT)
|
||||||
|
|
||||||
if unowned:
|
if foreign:
|
||||||
# Still opaque about ownership, but never a false success: the
|
# Still opaque about ownership, but never a false success: the
|
||||||
# caller must not record this workspace as cleaned up.
|
# caller must not record this workspace as cleaned up.
|
||||||
logger.warning(
|
logger.warning(
|
||||||
|
|||||||
@@ -284,6 +284,34 @@ class WorkspaceDeleteOwnershipTests(TestCase):
|
|||||||
self.assertEqual(response.status_code, 204)
|
self.assertEqual(response.status_code, 204)
|
||||||
cascade.assert_called_once_with(lib)
|
cascade.assert_called_once_with(lib)
|
||||||
|
|
||||||
|
def test_unclaimed_library_is_deleted(self):
|
||||||
|
"""A NULL owner is unclaimed, not foreign.
|
||||||
|
|
||||||
|
Workspace libraries predating owner stamping (and any created via the
|
||||||
|
plain /libraries/ endpoint, which never sets an owner) have
|
||||||
|
owner_username=None. Refusing those would make them permanently
|
||||||
|
undeletable through this endpoint — orphans by construction.
|
||||||
|
"""
|
||||||
|
from library.models import Library
|
||||||
|
|
||||||
|
lib = self._library(None)
|
||||||
|
with (
|
||||||
|
patch("neomodel.sync_.match.NodeSet.get", return_value=lib),
|
||||||
|
patch(
|
||||||
|
"library.api.workspaces.delete_library_cascade",
|
||||||
|
return_value={
|
||||||
|
"library_uid": "lib_1",
|
||||||
|
"name": "Assistant",
|
||||||
|
"item_count": 0,
|
||||||
|
"orphans_deleted": 0,
|
||||||
|
},
|
||||||
|
) as cascade,
|
||||||
|
):
|
||||||
|
response = self.client.delete("/library/api/workspaces/ws_a/")
|
||||||
|
|
||||||
|
self.assertEqual(response.status_code, 204)
|
||||||
|
cascade.assert_called_once_with(lib)
|
||||||
|
|
||||||
def test_unowned_library_get_still_looks_absent(self):
|
def test_unowned_library_get_still_looks_absent(self):
|
||||||
"""Ownership must stay opaque on reads — 404, not 409."""
|
"""Ownership must stay opaque on reads — 404, not 409."""
|
||||||
from library.models import Library
|
from library.models import Library
|
||||||
|
|||||||
Reference in New Issue
Block a user