Gate Create/RemoveLabelForLabelset on UPDATE permission

Previous commit pinned the Remove mutation to creator-only, but per
docs/permissioning/consolidated_permissioning_guide.md the actual model is
"anyone with edit rights to a LabelSet can add/delete/edit labels". LabelSet
has full guardian permission tables (LabelSetUserObjectPermission, group
perms, update_labelset codename), so the right check is
user_has_permission_for_obj(..., PermissionTypes.UPDATE,
include_group_permissions=True).

Both CreateLabelForLabelsetMutation and RemoveLabelsFromLabelsetMutation now
fetch the labelset by pk and gate mutation on that check. On denial we raise
LabelSet.DoesNotExist so the response message and code path are identical to
the not-found case (no IDOR information leak).

Tests: replaced the prior "rejects-on-public" test with one that pins
"is_public grants READ only", and added a positive test confirming a
non-creator with explicit UPDATE permission can remove labels.

Author: claude

CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security
 
-- **`RemoveLabelsFromLabelsetMutation` allowed any authenticated user to mutate public labelsets** (`config/graphql/label_mutations.py:283-303`): The resolver fetched the target labelset with `Q(creator=user) | Q(is_public=True)` and then unconditionally called `labelset.annotation_labels.remove(*label_pks)`. `is_public` is intended to expose a labelset for **use**, not for mutation, and the companion `CreateLabelForLabelsetMutation` (line 234) was already creator-only — so any logged-in user could strip labels from another user's public labelset, breaking shared-labelset workflows. Restricted the lookup to `pk=..., creator=user`, removed the now-unused `Q` import, and inverted the regression test (`opencontractserver/tests/test_label_mutations.py:174` — renamed to `test_remove_labels_rejects_non_owner_even_for_public_labelset`) so the rejection path is pinned going forward.
+- **Labelset-membership mutations now enforce the documented edit-rights model** (`config/graphql/label_mutations.py`, `CreateLabelForLabelsetMutation` and `RemoveLabelsFromLabelsetMutation`): Both resolvers previously made the wrong scope call. `Remove` used `Q(creator=user) | Q(is_public=True)` with no further check, so any logged-in user could strip labels from another user's *public* labelset. `Create` was creator-only, so a collaborator with explicit `update_labelset` guardian permission (or a group grant) could not add labels even though the consolidated permissioning guide says edit rights on a `LabelSet` should permit add/remove/edit of its labels. Both now fetch the labelset by pk and gate mutation on `user_has_permission_for_obj(user, labelset, PermissionTypes.UPDATE, include_group_permissions=True)`, raising `LabelSet.DoesNotExist` on denial so the response message and code path are identical to the not-found case (no IDOR information leak). Test coverage in `opencontractserver/tests/test_label_mutations.py`: `test_remove_labels_rejects_non_owner_even_when_labelset_is_public` (replaces the prior permissive test) confirms `is_public` alone does not grant write; `test_remove_labels_allows_non_owner_with_explicit_update_permission` pins the edit-rights model. The unused `Q` import was removed.
 - **Cross-corpus structural-annotation leak in `CoreAnnotationVectorStore`** (`opencontractserver/llms/vector_stores/core_vector_stores.py:296-326,371-413`): The corpus-wide retrieval path (`corpus_id` set, `document_id=None`) returned every structural annotation in the database regardless of corpus. Two collaborating defects caused the leak:
   1. `Q(structural=True)` in the corpus-only branch had **no corpus constraint** — parser-produced structural annotations have `Annotation.document_id = corpus_id = NULL`, so corpus membership is only knowable through `structural_set → Document.structural_annotation_set (reverse FK) → DocumentPath.corpus_id`, a join the previous code did not perform.
   2. The `check_corpus_deletion` block (default `True`) added `Q(document_id__in=active_doc_ids)`, and `__in` lookups never match `NULL`, so structural annotations were silently dropped on the production-default path. Bypassing this filter with `check_corpus_deletion=False` exposed defect #1 directly.

config/graphql/label_mutations.py

@@ -19,7 +19,10 @@
 from config.graphql.validation_utils import validate_color
 from opencontractserver.annotations.models import AnnotationLabel, LabelSet
 from opencontractserver.types.enums import PermissionTypes
-from opencontractserver.utils.permissioning import set_permissions_for_obj_to_user
+from opencontractserver.utils.permissioning import (
+    set_permissions_for_obj_to_user,
+    user_has_permission_for_obj,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -229,9 +232,15 @@ def mutate(root, info, labelset_id, text, description, color, icon, label_type):
             )
 
         try:
-            labelset = LabelSet.objects.get(
-                pk=from_global_id(labelset_id)[1], creator=info.context.user
-            )
+            labelset = LabelSet.objects.get(pk=from_global_id(labelset_id)[1])
+            if not user_has_permission_for_obj(
+                info.context.user,
+                labelset,
+                PermissionTypes.UPDATE,
+                include_group_permissions=True,
+            ):
+                # Generic deny path — same message and code path as not-found
+                raise LabelSet.DoesNotExist
             logger.debug("CreateLabelForLabelsetMutation - mutate / Labelset", labelset)
             obj = AnnotationLabel.objects.create(
                 text=text,
@@ -288,9 +297,15 @@ def mutate(root, info, label_ids, labelset_id):
             label_pks = list(
                 map(lambda graphene_id: from_global_id(graphene_id)[1], label_ids)
             )
-            labelset = LabelSet.objects.get(
-                pk=from_global_id(labelset_id)[1], creator=user
-            )
+            labelset = LabelSet.objects.get(pk=from_global_id(labelset_id)[1])
+            if not user_has_permission_for_obj(
+                user,
+                labelset,
+                PermissionTypes.UPDATE,
+                include_group_permissions=True,
+            ):
+                # Generic deny path — same message and code path as not-found
+                raise LabelSet.DoesNotExist
             labelset.annotation_labels.remove(*label_pks)
             ok = True
             message = "Success"

opencontractserver/tests/test_label_mutations.py

@@ -171,12 +171,11 @@ def test_remove_labels_empty_list_is_noop(self) -> None:
         remaining = set(self.labelset.annotation_labels.values_list("pk", flat=True))
         self.assertEqual(remaining, {self.label_a.pk, self.label_b.pk, self.label_c.pk})
 
-    def test_remove_labels_rejects_non_owner_even_for_public_labelset(self) -> None:
-        """Public labelsets are read-only via this mutation for non-creators.
+    def test_remove_labels_rejects_non_owner_even_when_labelset_is_public(self) -> None:
+        """``is_public`` grants READ only — it does not grant UPDATE.
 
-        ``is_public`` exposes a labelset for use, not for mutation. Only the
-        creator may remove labels from it; matches the creator-only lookup in
-        ``CreateLabelForLabelsetMutation``.
+        Without an explicit guardian UPDATE grant a non-creator must not be
+        able to mutate the labelset's membership, regardless of ``is_public``.
         """
 
         self.labelset.is_public = True
@@ -197,3 +196,33 @@ def test_remove_labels_rejects_non_owner_even_for_public_labelset(self) -> None:
         # Nothing should have changed
         remaining = set(self.labelset.annotation_labels.values_list("pk", flat=True))
         self.assertEqual(remaining, {self.label_a.pk, self.label_b.pk, self.label_c.pk})
+
+    def test_remove_labels_allows_non_owner_with_explicit_update_permission(
+        self,
+    ) -> None:
+        """A non-creator who has been granted UPDATE on the labelset may remove labels.
+
+        Pins the documented model: anyone with edit rights to a LabelSet can
+        add/remove labels — not just the creator. See
+        ``docs/permissioning/consolidated_permissioning_guide.md``.
+        """
+
+        set_permissions_for_obj_to_user(
+            self.other_user, self.labelset, [PermissionTypes.UPDATE]
+        )
+
+        variables = {
+            "labelIds": [to_global_id("AnnotationLabelType", self.label_a.id)],
+            "labelsetId": to_global_id("LabelSetType", self.labelset.id),
+        }
+
+        result = self.other_client.execute(REMOVE_LABELS_MUTATION, variables=variables)
+
+        self.assertIsNone(result.get("errors"))
+        data = result["data"]["removeAnnotationLabelsFromLabelset"]
+        self.assertTrue(
+            data["ok"],
+            msg=f"Mutation did not succeed. Message: {data['message']}",
+        )
+        remaining = set(self.labelset.annotation_labels.values_list("pk", flat=True))
+        self.assertEqual(remaining, {self.label_b.pk, self.label_c.pk})