From f6e8e9bba5c2b99bd4b3bdaa5d59aa58d32a0a66 Mon Sep 17 00:00:00 2001 From: Robert Helewka Date: Fri, 7 Aug 2026 18:32:23 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=BE=20fix(api):=20treat=20a=20NULL=20o?= =?UTF-8?q?wner=20as=20unclaimed,=20not=20foreign?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- mnemosyne/library/api/workspaces.py | 24 +++++++++++----- .../library/tests/test_workspaces_api.py | 28 +++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/mnemosyne/library/api/workspaces.py b/mnemosyne/library/api/workspaces.py index 860cd63..3a7371b 100644 --- a/mnemosyne/library/api/workspaces.py +++ b/mnemosyne/library/api/workspaces.py @@ -213,14 +213,24 @@ def workspace_detail_or_delete(request, workspace_id): except Library.DoesNotExist: lib = None - # Cross-user reads look like "not found" — don't disclose existence - # across users. DELETE is handled separately below: 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. - unowned = lib is not None and lib.owner_username != request.user.username + # A NULL owner_username is *unclaimed*, not someone else's: workspace + # libraries created before owner stamping (and any created through the + # plain /libraries/ endpoint, which never sets it) have no owner. Treating + # those as foreign would make them permanently undeletable through this + # 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 lib is None or unowned: + if lib is None or foreign: return Response( {"detail": "Workspace not found."}, status=status.HTTP_404_NOT_FOUND, @@ -231,7 +241,7 @@ def workspace_detail_or_delete(request, workspace_id): if lib is None: return Response(status=status.HTTP_204_NO_CONTENT) - if unowned: + if foreign: # Still opaque about ownership, but never a false success: the # caller must not record this workspace as cleaned up. logger.warning( diff --git a/mnemosyne/library/tests/test_workspaces_api.py b/mnemosyne/library/tests/test_workspaces_api.py index 402b587..04e4262 100644 --- a/mnemosyne/library/tests/test_workspaces_api.py +++ b/mnemosyne/library/tests/test_workspaces_api.py @@ -284,6 +284,34 @@ class WorkspaceDeleteOwnershipTests(TestCase): self.assertEqual(response.status_code, 204) 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): """Ownership must stay opaque on reads — 404, not 409.""" from library.models import Library