Skip to content

Commit 7b189db

Browse files
committed
Tighten source privacy gates to READ-codename grants (parity with user_can)
Review follow-up that turned out to be a real parity bug, not a docstring question: the privacy-gate builders matched ANY guardian row on the source analysis/extract, while user_can recursion resolves READ via the read codename — so an update_analysis-only grantee saw private rows in lists while user_can(READ) denied them (the same filter/check violation class this PR closes elsewhere). visible_analyses_for / visible_extracts_for now filter permission__codename=read_analysis / read_extract on both the user- and group-level tables; pinned by test_update_only_source_grant_does_not_unlock_lists. Also from the review round: - _source_privacy_recursion_passes docstring documents the deleted-source posture (created_by_* FKs are on_delete=SET_NULL, so deletion turns rows into normal rows; the is-None branch is a fail-closed race guard, not a permanent lock) - ExtractManager.user_can carries an equivalence note for its None/anonymous guard vs the visible_to_user surface - get_corpus_annotations privacy exclusion now explains at the call site why it is intentionally unconditional (old anonymous guard was the leak) - changelog fragment updated for the grant-shape tightening
1 parent eec7f04 commit 7b189db

6 files changed

Lines changed: 74 additions & 13 deletions

File tree

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
- **Relationship privacy recursion implemented (closes the Phase-C deferral from issue #1655).** `RelationshipManager.user_can` and `RelationshipManager.visible_to_user` (`opencontractserver/shared/Managers.py`) now enforce `created_by_analysis` / `created_by_extract` source permissions exactly like annotations: non-creators need the requested permission on the source Analysis/Extract in addition to the doc+corpus MIN. Previously a user with doc+corpus grants could see and write analysis-/extract-private relationships through the manager surfaces. The recursion logic is shared with `AnnotationManager` via the new module-level `_source_privacy_recursion_passes`. Parity pinned by new fixtures/tests in `opencontractserver/tests/permissioning/test_authorization_invariants.py` (`RelationshipAuthorizationInvariantsTestCase` — the old `test_no_privacy_widening_via_created_by_analysis` sentinel that documented the deferral is replaced by real privacy invariants) and `opencontractserver/tests/permissioning/test_relationship_privacy_scoping.py`.
22
- **Extracts locked down to never be anonymous-visible.** `ExtractService.get_visible_extracts` / `check_extract_permission` (`opencontractserver/extracts/services/extract_service.py`) previously exposed `is_public` extracts in public corpora to anonymous callers (drift copied from the analysis service, reachable through the un-gated `extracts` GraphQL resolver), contradicting the documented "Extract — never" anonymous contract. A new `ExtractManager` (`opencontractserver/extracts/models.py`) denies anonymous users on both `visible_to_user` and `user_can` so the rule holds at the manager layer too (e.g. agent tools listing extracts). User-visible consequence: anonymous viewers of a public corpus now see `totalExtracts: 0` in `corpusStats` (the resolver's counts are per-viewer by contract). Two legacy tests that pinned the old semantic were updated to the agreed behavior: `test_query_optimizer_methods.py::test_get_visible_extracts_anonymous_only_public` (now `..._anonymous_sees_nothing`) and `test_stats.py::test_public_corpus_stats_query` (anonymous branch). New coverage: `opencontractserver/tests/permissioning/test_extract_anonymous_lockdown.py`.
3-
- **Group-granted analysis/extract permissions now unlock private rows in list queries.** The `created_by_*` privacy gates in `AnnotationQuerySet.visible_to_user` (`opencontractserver/shared/QuerySets.py`), `AnnotationService.get_document_annotations` / `get_corpus_annotations`, and `RelationshipService.get_document_relationships` consulted only the USER object-permission tables, so a viewer whose source grant arrived via a Django group passed `user_can` (which resolves group grants by default) but never saw the rows in lists — a filter/check parity drift. All gates now route through the new shared builders in `opencontractserver/utils/source_visibility.py`, which join the group object-permission tables. The same parity class also covered `Extract.is_public`: the inline gates omitted the flag for extracts (while honouring it for analyses) even though `user_can`'s recursion grants READ on public sources — `visible_extracts_for` now includes `is_public` on the authenticated branch (no practical exposure today: no user-facing flow sets the flag). Tests: `test_source_visibility_group_grants.py` + new invariant fixtures incl. `test_public_extract_source_passes_both_surfaces`.
3+
- **Group-granted analysis/extract permissions now unlock private rows in list queries.** The `created_by_*` privacy gates in `AnnotationQuerySet.visible_to_user` (`opencontractserver/shared/QuerySets.py`), `AnnotationService.get_document_annotations` / `get_corpus_annotations`, and `RelationshipService.get_document_relationships` consulted only the USER object-permission tables, so a viewer whose source grant arrived via a Django group passed `user_can` (which resolves group grants by default) but never saw the rows in lists — a filter/check parity drift. All gates now route through the new shared builders in `opencontractserver/utils/source_visibility.py`, which join the group object-permission tables. Two more drifts in the same parity class were closed in the builders: (a) `Extract.is_public` — the inline gates omitted the flag for extracts (while honouring it for analyses) even though `user_can`'s recursion grants READ on public sources; `visible_extracts_for` now includes `is_public` on the authenticated branch (no practical exposure today: no user-facing flow sets the flag); (b) grant shape — the inline gates matched ANY guardian row on the source, so an `update_analysis`-only grantee saw private rows in lists while failing `user_can(READ)` (which resolves the read codename); the builders now filter `permission__codename="read_analysis"` / `"read_extract"`. Tests: `test_source_visibility_group_grants.py` + new invariant fixtures incl. `test_public_extract_source_passes_both_surfaces` and `test_update_only_source_grant_does_not_unlock_lists`.
44
- **Anonymous privacy leak in corpus-wide annotation listing fixed.** `AnnotationService.get_corpus_annotations` applied the `created_by_*` privacy exclusion only to authenticated users — anonymous viewers of a public corpus could see analysis-/extract-private annotations on public documents. The exclusion now applies unconditionally (anonymous resolves to public-analyses-only / no extracts via `source_visibility`).

opencontractserver/annotations/services/annotation_service.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -743,6 +743,12 @@ def get_corpus_annotations(
743743
# reach. The shared builders honour user- AND group-level guardian
744744
# grants (parity with ``user_can``'s privacy recursion) and encode
745745
# the anonymous rules (public analyses only; never extracts).
746+
#
747+
# Intentionally UNCONDITIONAL — no ``if not user.is_anonymous``
748+
# guard. The old guard skipped privacy filtering entirely for
749+
# anonymous viewers, leaking analysis-/extract-private annotations
750+
# on public corpora (2026-06 audit). Anonymous handling lives inside
751+
# the builders instead.
746752
from opencontractserver.utils.source_visibility import (
747753
visible_analyses_for,
748754
visible_extracts_for,

opencontractserver/extracts/models.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,10 @@ def user_can(
224224
resolve_user_for_user_can,
225225
)
226226

227+
# Equivalent shape to the ``visible_to_user`` guard above: the
228+
# resolver maps None / unresolvable ids to None and passes
229+
# AnonymousUser through, so both surfaces deny exactly the same
230+
# caller set (pinned by ExtractAuthorizationInvariantsTestCase).
227231
resolved = resolve_user_for_user_can(user)
228232
if resolved is None or getattr(resolved, "is_anonymous", True):
229233
return False

opencontractserver/shared/Managers.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -674,6 +674,13 @@ def _source_privacy_recursion_passes(
674674
``and permission == READ`` for readability rather than relying on the
675675
flow-sensitive equivalence.
676676
677+
Deleted-source posture: both FKs are ``on_delete=SET_NULL``, so deleting
678+
the source Analysis/Extract NULLs the FK and the row becomes a normal
679+
(non-private) row — deletion does NOT permanently lock rows. The
680+
``source is None`` branches below fire only in the race window where the
681+
id column is read before the SET_NULL lands; they fail closed for that
682+
window, by design.
683+
677684
Performance note: the FK descriptors (``instance.created_by_analysis``
678685
/ ``created_by_extract``) hit the database once each per call when the
679686
relations aren't prefetched. Bulk callers SHOULD

opencontractserver/tests/permissioning/test_authorization_invariants.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1667,6 +1667,43 @@ def test_public_extract_source_passes_both_surfaces(self):
16671667
"(parity with user_can's recursion)",
16681668
)
16691669

1670+
def test_update_only_source_grant_does_not_unlock_lists(self):
1671+
"""An UPDATE-only grant on the source analysis must NOT clear the
1672+
privacy gate (2026-06 audit follow-up): ``user_can``'s recursion
1673+
resolves READ through the read codename, so the list gates must
1674+
match — the old any-permission-row match let an
1675+
``update_analysis``-only grantee see private rows in lists while
1676+
``user_can(READ)`` denied them (filter/check parity violation).
1677+
"""
1678+
from opencontractserver.annotations.models import Annotation
1679+
1680+
update_only = User.objects.create_user(
1681+
username="ann_update_only", email="auo@inv.test", password="x"
1682+
)
1683+
set_permissions_for_obj_to_user(
1684+
update_only, self.corpus, [PermissionTypes.READ]
1685+
)
1686+
set_permissions_for_obj_to_user(
1687+
update_only, self.document, [PermissionTypes.READ]
1688+
)
1689+
# UPDATE-only on the source — deliberately NO READ.
1690+
set_permissions_for_obj_to_user(
1691+
update_only, self.analysis, [PermissionTypes.UPDATE]
1692+
)
1693+
1694+
fresh = Annotation.objects.get(pk=self.private_via_analysis.pk)
1695+
self.assertFalse(
1696+
fresh.user_can(update_only, PermissionTypes.READ),
1697+
"user_can(READ) must deny an UPDATE-only source grantee",
1698+
)
1699+
self.assertFalse(
1700+
Annotation.objects.visible_to_user(update_only)
1701+
.filter(pk=self.private_via_analysis.pk)
1702+
.exists(),
1703+
"list gate must NOT be cleared by a non-READ source grant "
1704+
"(parity with user_can's read-codename recursion)",
1705+
)
1706+
16701707
def test_is_public_does_not_grant_writes(self):
16711708
"""SECURITY: ``is_public=True`` grants a non-creator READ but never
16721709
UPDATE/DELETE — the write asymmetry must hold for Annotation."""

opencontractserver/utils/source_visibility.py

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,15 @@
2020
- **extracts** — NONE. Extracts are never visible to anonymous users, at
2121
any layer (see ``ExtractManager`` and ``ExtractService``).
2222
23-
Grant-shape note: a *any-permission-row* match on the guardian tables
24-
(rather than a ``read_*`` codename filter) is deliberately preserved from
25-
the original inline implementations — holders of any explicit grant on the
26-
source object clear the privacy gate.
23+
Grant-shape note: the gates match guardian rows carrying the ``read_*``
24+
codename specifically — NOT any-permission rows. The original inline
25+
implementations matched any grant, which was itself a parity drift:
26+
``user_can``'s privacy recursion resolves READ through
27+
``Analysis.objects.user_can`` / ``Extract.objects.user_can``, whose
28+
guardian branch checks the read codename — so an ``update_analysis``-only
29+
grantee cleared the old list gates while failing ``user_can(READ)``
30+
(2026-06 audit follow-up; pinned by
31+
``test_update_only_source_grant_does_not_unlock_lists``).
2732
"""
2833

2934
from __future__ import annotations
@@ -54,11 +59,12 @@ def visible_analyses_for(user: Any) -> QuerySet:
5459

5560
visible = Analysis.objects.filter(Q(is_public=True) | Q(creator=user))
5661

57-
user_grant_ids = AnalysisUserObjectPermission.objects.filter(user=user).values_list(
58-
"content_object_id", flat=True
59-
)
62+
user_grant_ids = AnalysisUserObjectPermission.objects.filter(
63+
user=user, permission__codename="read_analysis"
64+
).values_list("content_object_id", flat=True)
6065
group_grant_ids = AnalysisGroupObjectPermission.objects.filter(
61-
group_id__in=user.groups.values_list("id", flat=True)
66+
group_id__in=user.groups.values_list("id", flat=True),
67+
permission__codename="read_analysis",
6268
).values_list("content_object_id", flat=True)
6369

6470
return (
@@ -102,11 +108,12 @@ def visible_extracts_for(user: Any) -> QuerySet:
102108

103109
visible = Extract.objects.filter(Q(creator=user) | Q(is_public=True))
104110

105-
user_grant_ids = ExtractUserObjectPermission.objects.filter(user=user).values_list(
106-
"content_object_id", flat=True
107-
)
111+
user_grant_ids = ExtractUserObjectPermission.objects.filter(
112+
user=user, permission__codename="read_extract"
113+
).values_list("content_object_id", flat=True)
108114
group_grant_ids = ExtractGroupObjectPermission.objects.filter(
109-
group_id__in=user.groups.values_list("id", flat=True)
115+
group_id__in=user.groups.values_list("id", flat=True),
116+
permission__codename="read_extract",
110117
).values_list("content_object_id", flat=True)
111118

112119
return (

0 commit comments

Comments
 (0)