Merge pull request #2032 from Open-Source-Legal/claude/eager-ritchie-wfd5yi

Permissioning audit follow-ups (#1986): conversation/message visibility parity, safe manager fallback, datacell crash

Author: JSv4

changelog.d/1986-perm-followups.fixed.md

@@ -0,0 +1,11 @@
+- Fixed an `AttributeError` crash in `Datacell._validate_manual_entry`
+  (`opencontractserver/extracts/models.py`, issue #1986 item 7). The "required"
+  check in the missing-`value` branch called
+  `self.column.validation_config.get("required")` *before* the
+  `config = self.column.validation_config or {}` guard, so validating a
+  manual-entry datacell with no `value` key on a column whose `validation_config`
+  is `None` (a nullable JSONField — legitimately null on explicitly-cleared or
+  legacy rows) raised `'NoneType' object has no attribute 'get'`. The `or {}`
+  guard is now resolved once at the top of the method and reused, so a config-less
+  column validates cleanly (nothing is required) while a column that *does* set
+  `required` still raises the expected `ValidationError`.

changelog.d/1986-perm-followups.security.md

@@ -0,0 +1,30 @@
+- Permissioning audit follow-ups from issue #1986 (filter/check parity + safe
+  error handling):
+  - **Anonymous chat-message visibility now follows the conversation
+    bifurcation (item 2).** `ChatMessageQuerySet.visible_to_user`
+    (`opencontractserver/conversations/models.py`) previously filtered the
+    anonymous branch on `conversation__is_public=True` alone, which exposed a
+    public **CHAT**'s messages even though `ConversationQuerySet.visible_to_user`
+    keeps the conversation itself hidden from anonymous users (anonymous can
+    only see THREADs). The branch now delegates to
+    `Conversation.objects.visible_to_user`, so message visibility inherits the
+    single-source-of-truth CHAT/THREAD rules — no public-CHAT message leak, and
+    messages in context-inherited public-resource threads are correctly visible.
+  - **Conversation and chat-message querysets now honour group-level READ
+    grants (item 3).** `ConversationQuerySet` / `ChatMessageQuerySet`
+    `visible_to_user` consulted only the USER object-permission tables. Because
+    `ConversationManager.user_can(READ)` / `ChatMessageManager.user_can(READ)`
+    route back through `visible_to_user`, a group-only READ grant was both
+    invisible in lists AND denied by `user_can(READ)` — even though non-READ
+    writes (via `_default_user_can`) honoured the same group grant. Both
+    querysets now join the `*groupobjectpermission` tables (mirroring
+    `BaseVisibilityManager`), closing the same group-grant gap PR #1985 fixed for
+    annotations / relationships / extracts.
+  - **`BaseVisibilityManager.visible_to_user` no longer swallows unexpected
+    errors (item 4).** The guardian-lookup fallback caught
+    `(ImportError, Exception)` — equivalent to a bare `except Exception` — so any
+    error silently degraded the queryset to creator/public filtering,
+    under-disclosing guardian-granted rows while hiding the defect
+    (`opencontractserver/shared/Managers.py`). The catch is narrowed to
+    `(ImportError, LookupError)` so a genuinely-absent permission table still
+    falls back gracefully, but programming/database errors now propagate.

opencontractserver/conversations/models.py

@@ -223,7 +223,15 @@ def visible_to_user(
             )
             return queryset.filter(anon_filter).distinct().order_by("-created")
 
-        # Get explicitly permitted conversation IDs via guardian
+        # Get explicitly permitted conversation IDs via guardian (user- AND
+        # group-level READ grants). ``ConversationManager.user_can(READ)``
+        # routes back through this queryset, so the filter IS the check — but
+        # non-READ writes delegate to ``_default_user_can``, which honours group
+        # grants by default. Consulting only the USER object-permission table
+        # here therefore made a group-only READ grant entirely non-functional:
+        # invisible in lists AND denied by ``user_can(READ)`` (issue #1986
+        # item 3 — the same filter/check group-grant gap PR #1985 closed for
+        # annotations / relationships / extracts).
         model_name = self.model._meta.model_name
         app_label = self.model._meta.app_label
 
@@ -236,9 +244,28 @@ def visible_to_user(
         except LookupError:
             permitted_ids = []
 
+        # Group-level READ grants, resolved in their own ``try`` so a missing
+        # group table never discards the already-resolved user-level grants.
+        # The lazy ``values_list`` keeps both lookups as SQL subqueries
+        # (no extra round-trips).
+        try:
+            group_permission_model_name = f"{model_name}groupobjectpermission"
+            group_permission_model_type = apps.get_model(
+                app_label, group_permission_model_name
+            )
+            group_permitted_ids = group_permission_model_type.objects.filter(
+                permission__codename=f"read_{model_name}",
+                group_id__in=user.groups.values_list("id", flat=True),
+            ).values_list("content_object_id", flat=True)
+        except LookupError:
+            group_permitted_ids = []
+
         # Base conditions: apply to BOTH CHAT and THREAD types
         base_conditions = (
-            Q(creator_id=user.id) | Q(is_public=True) | Q(id__in=permitted_ids)
+            Q(creator_id=user.id)
+            | Q(is_public=True)
+            | Q(id__in=permitted_ids)
+            | Q(id__in=group_permitted_ids)
         )
 
         # CHAT type: base conditions only (restrictive)
@@ -374,9 +401,25 @@ def visible_to_user(
 
         # Superusers are computed like any other user (scoped admin access, 2026-05) — no blanket bypass.
 
-        # Anonymous users only see messages in public conversations
+        # Anonymous users see messages only in conversations THEY can see.
+        # Delegating to ``Conversation.objects.visible_to_user`` keeps the
+        # CHAT/THREAD bifurcation the single source of truth (issue #1986
+        # item 2): for an anonymous viewer that is public THREADs plus context
+        # inheritance on public corpuses/documents, and NEVER a CHAT. The
+        # previous ``conversation__is_public=True`` shortcut both leaked a
+        # public *CHAT*'s messages (the conversation itself stays hidden via
+        # ``ConversationQuerySet.visible_to_user``, so the messages were the
+        # odd surface out) and missed messages in context-inherited
+        # public-resource threads. This mirrors the authenticated branch
+        # below, which already routes message visibility through conversation
+        # visibility.
         if user.is_anonymous:
-            return queryset.filter(conversation__is_public=True).order_by("created")
+            visible_conversation_ids = Conversation.objects.visible_to_user(
+                user
+            ).values_list("id", flat=True)
+            return queryset.filter(
+                conversation_id__in=visible_conversation_ids
+            ).order_by("created")
 
         # Primary visibility: messages in visible conversations
         # This inherits the bifurcated CHAT/THREAD permission logic
@@ -386,7 +429,12 @@ def visible_to_user(
         ).values_list("id", flat=True)
         conversation_visible = Q(conversation_id__in=visible_conversation_ids)
 
-        # Additional conditions for explicit message-level access
+        # Additional conditions for explicit message-level access (user- AND
+        # group-level guardian grants). Adding the group branch mirrors the
+        # conversation queryset's group-grant fix (issue #1986 item 3) so a
+        # message shared via a Django group is visible in lists, not only to
+        # direct user grantees. Both branches preserve the existing
+        # any-message-permission shape (a grant of any codename implies READ).
         from django.apps import apps
 
         try:
@@ -401,9 +449,22 @@ def visible_to_user(
         except LookupError:
             has_permission = Q(pk__in=[])
 
+        try:
+            group_permission_model = apps.get_model(
+                "conversations", "chatmessagegroupobjectpermission"
+            )
+            has_group_permission = Q(
+                id__in=group_permission_model.objects.filter(
+                    group_id__in=user.groups.values_list("id", flat=True)
+                ).values_list("content_object_id", flat=True)
+            )
+        except LookupError:
+            has_group_permission = Q(pk__in=[])
+
         base_conditions = (
             Q(creator=user)  # User created the message
-            | has_permission  # User has explicit permission on message
+            | has_permission  # User has explicit user-level permission
+            | has_group_permission  # User has a group-level permission
         )
 
         # Moderator conditions: user can moderate the conversation

opencontractserver/extracts/models.py

@@ -404,16 +404,20 @@ def _validate_manual_entry(self) -> None:
         if not self.data:
             self.data = {}
 
+        # ``validation_config`` is a nullable JSONField, so resolve the ``{}``
+        # fallback ONCE before reading any key — calling ``.get(...)`` on a
+        # ``None`` config is the AttributeError fixed in issue #1986 item 7.
+        config = self.column.validation_config or {}
+
         if "value" not in self.data:
-            if self.column.validation_config.get("required"):
+            if config.get("required"):
                 raise django.core.exceptions.ValidationError(
                     f"{self.column.name} is required"
                 )
             return
 
         value = self.data["value"]
         data_type = self.column.data_type
-        config = self.column.validation_config or {}
 
         # Skip further validation if value is None
         if value is None:

opencontractserver/shared/Managers.py

@@ -338,8 +338,19 @@ def visible_to_user(
 
             return queryset
 
-        except (ImportError, Exception) as e:
-            # Fall back to creator/public check only if Guardian not available or error
+        except (ImportError, LookupError) as e:
+            # Narrow, intentional fallback (issue #1986 item 4): degrade to
+            # creator/public filtering ONLY when the guardian permission tables
+            # are genuinely absent for this model — ``ImportError`` (guardian /
+            # an app not installed) or ``LookupError`` (``apps.get_model`` cannot
+            # resolve the permission model). The previous ``(ImportError,
+            # Exception)`` envelope was equivalent to a bare ``except Exception``
+            # (``ImportError`` ⊂ ``Exception``): ANY error — a programming bug, a
+            # database error, a malformed grant row — was swallowed and the
+            # queryset silently dropped every guardian-granted row, UNDER-
+            # disclosing (a fail-open-to-less-data security smell) while hiding
+            # the defect. Such errors now propagate so they surface in tests and
+            # monitoring instead of quietly narrowing a user's visible set.
             logger.debug(
                 f"Could not use Guardian permissions for {self.model.__name__}: {e}. "
                 f"Using creator/public filtering only."