Address review: permission check before validation, reject text=None

- Move LabelSet.objects.get + permission check ahead of all field
  validation so a non-owner can't distinguish 'reached validation' from
  'denied by permission' via different error messages.  This is
  defense-in-depth on top of the existing IDOR-safe deny pattern.
- Tighten the text guard from 'is not None and not strip()' to
  'not (text and text.strip())' so the model default ('Text Label')
  cannot apply when the client omits the argument entirely (GraphQL
  passes None for an unsupplied optional argument).
- Normalise color='' to None before validate_color so callers who
  explicitly pass an empty color receive the model default rather
  than triggering a misleading validation error.
- Add regression tests:
  * test_omitted_text_is_rejected pins the None-text path
  * test_permission_denied_message_does_not_leak_validation_state
    verifies validation-order indistinguishability for non-owners

Author: JSv4

config/graphql/label_mutations.py

@@ -237,27 +237,13 @@ def mutate(
         obj = None
         obj_id = None
 
-        # Reject blank ``text`` early.  ``text`` is GraphQL-optional so the
-        # model default ("Text Label") would silently apply, which produces
-        # surprising labels for clients that forgot to pass a value or
-        # passed an empty string.  Django's ``blank=False`` is form-only
-        # and is NOT enforced by ``objects.create()``.
-        if text is not None and not text.strip():
-            return CreateLabelForLabelsetMutation(
-                obj=None,
-                obj_id=None,
-                message="Label text cannot be blank.",
-                ok=False,
-            )
-
-        # Validate color format (defense in depth)
-        is_valid_color, color_error = validate_color(color)
-        if not is_valid_color:
-            return CreateLabelForLabelsetMutation(
-                obj=None, obj_id=None, message=color_error, ok=False
-            )
-
         try:
+            # Permission check FIRST — runs before any field validation
+            # so a non-owner can't distinguish "reached validation" from
+            # "denied by permission" via different error messages
+            # (defense-in-depth; consistent with the IDOR-safe deny
+            # pattern documented in
+            # ``docs/permissioning/consolidated_permissioning_guide.md``).
             labelset = LabelSet.objects.get(pk=from_global_id(labelset_id)[1])
             if not user_has_permission_for_obj(
                 info.context.user,
@@ -267,6 +253,34 @@ def mutate(
             ):
                 # Generic deny path — same message and code path as not-found
                 raise LabelSet.DoesNotExist()
+
+            # ``text`` is GraphQL-optional so without this guard the model
+            # default ("Text Label") would silently apply, producing
+            # surprising labels for clients that forgot to pass a value
+            # or passed whitespace.  Django's ``blank=False`` is
+            # form-only and NOT enforced by ``objects.create()``.  The
+            # ``not (text and text.strip())`` form covers ``None``,
+            # empty string, and whitespace-only in one expression.
+            if not (text and text.strip()):
+                return CreateLabelForLabelsetMutation(
+                    obj=None,
+                    obj_id=None,
+                    message="Label text is required and cannot be blank.",
+                    ok=False,
+                )
+
+            # Validate color format (defense in depth).  ``color=""`` is
+            # normalised to ``None`` so the model default ("#FFFFFF")
+            # applies, matching the silent-default behaviour clients
+            # already get from omitting the argument.
+            if color == "":
+                color = None
+            is_valid_color, color_error = validate_color(color)
+            if not is_valid_color:
+                return CreateLabelForLabelsetMutation(
+                    obj=None, obj_id=None, message=color_error, ok=False
+                )
+
             logger.debug("CreateLabelForLabelsetMutation - mutate / Labelset", labelset)
             # Drop None and empty-string values so model field defaults
             # apply (description, color, icon, text are NOT NULL with

opencontractserver/tests/test_label_mutations.py

@@ -378,3 +378,56 @@ def test_blank_text_is_rejected(self) -> None:
         self.assertFalse(data["ok"])
         self.assertIn("blank", data["message"].lower())
         self.assertEqual(self.labelset.annotation_labels.count(), 0)
+
+    def test_omitted_text_is_rejected(self) -> None:
+        """A client that omits ``text`` entirely must NOT receive a
+        ``"Text Label"`` row by default — the model default is hidden
+        behind a required-field guard.
+        """
+        # GraphQL doesn't support omitting an argument from variables,
+        # but the resolver default is ``text=None``; build a custom
+        # variables dict that simulates the omission by passing None
+        # via JSON null (the GraphQL coercion that sends ``null`` for an
+        # unsupplied optional argument).
+        variables = self._create_variables("placeholder")
+        variables["text"] = None
+
+        result = self.client.execute(
+            CREATE_LABEL_FOR_LABELSET_MUTATION,
+            variables=variables,
+        )
+
+        self.assertIsNone(result.get("errors"))
+        data = result["data"]["createAnnotationLabelForLabelset"]
+        self.assertFalse(data["ok"])
+        self.assertIn("blank", data["message"].lower())
+        self.assertEqual(self.labelset.annotation_labels.count(), 0)
+        self.assertFalse(
+            AnnotationLabel.objects.filter(text="Text Label").exists()
+        )
+
+    def test_permission_denied_message_does_not_leak_validation_state(
+        self,
+    ) -> None:
+        """A non-owner caller hitting validation should still get the
+        same generic 404-style denial as if the labelset didn't exist —
+        the permission check runs FIRST so blank-text vs
+        permission-denied are indistinguishable to outsiders.
+        """
+        # ``other_user`` has no permission on ``self.labelset``.  Send
+        # an *invalid* mutation (blank text) — the response must look
+        # like a "does not exist", NOT like "blank text".
+        variables = self._create_variables("   ")
+
+        result = self.other_client.execute(
+            CREATE_LABEL_FOR_LABELSET_MUTATION,
+            variables=variables,
+        )
+
+        self.assertIsNone(result.get("errors"))
+        data = result["data"]["createAnnotationLabelForLabelset"]
+        self.assertFalse(data["ok"])
+        # Must NOT mention "blank" — that would leak that the caller's
+        # request was structurally valid but rejected by permission.
+        self.assertNotIn("blank", data["message"].lower())
+        self.assertIn("does not exist", data["message"].lower())