Honour group object-permissions in QuerySet.visible_to_user (#1714)

[JSv4]

Summary

Closes #1714.

Manager.user_can resolves group object-permissions — _default_user_can
runs with include_group_permissions=True — but the visible_to_user filters
in opencontractserver/shared/QuerySets.py only joined the user
object-permission table (*userobjectpermission), never
*groupobjectpermission. A user whose only READ grant on a Document /
Annotation / Note was via a group therefore passed
obj.user_can(user, READ) yet was excluded from
Model.objects.visible_to_user(user) — authorized for a single object but
unable to ever see it in a list query. Corpus was already correct (its
PermissionedTreeQuerySet resolves group perms via guardian's
get_objects_for_user), which is what made this a genuine per-model drift.

Changes

• PermissionQuerySet.visible_to_user — OR in a subquery against
  {model}groupobjectpermission filtered by the user's group ids.
• DocumentQuerySet.visible_to_user — same, against
  documentgroupobjectpermission.
• AnnotationQuerySet.visible_to_user — add group subqueries to both the
  document- and corpus-visibility predicates (annotations have no own
  permission table; visibility is MIN(doc, corpus)).
• NoteQuerySet.visible_to_user — no code change; it composes
  Document.objects.visible_to_user / Corpus.objects.visible_to_user, so it
  inherits the fix transitively (docstring updated to say so).

Group ids resolve as a lazy values_list subquery, so the filter stays a
single round-trip — query count is independent of group-membership size.

Tests

New GroupObjectPermissionVisibilityTest in test_permissioned_querysets.py
pins, for a user whose sole grant is a group object-permission:

• filter/check equivalence (user_can(READ) == visible_to_user(...).exists())
  for Document, Annotation, and Note;
• the generic PermissionQuerySet fallback body honours group perms;
• the read/write asymmetry — a group READ grant never widens to UPDATE/DELETE;
• non-members stay excluded;
• the perf property — group resolution is a subquery, not a per-group
  round-trip.

Relationship to #1712

PR #1712 (Phase F, open) adds
DocumentAuthorizationInvariantsTestCase.test_group_shared_known_drift_user_can_vs_visible_to_user,
which pins the current (drifted) behaviour. This PR closes that drift.
The two PRs touch disjoint files (#1712: test_authorization_invariants*.py

• docs; this PR: QuerySets.py + test_permissioned_querysets.py), so they
  merge without conflict — but whichever lands second, that one pinned drift
  test must be converted into an equivalence assertion (its own docstring says
  so). The equivalence is already covered here by
  GroupObjectPermissionVisibilityTest.

Test plan

• GroupObjectPermissionVisibilityTest — 8/8 pass.
• test_permissioned_querysets.py + test_visibility_managers.py — fully
  green.
• test_authorization_invariants.py — 34 pre-existing environment errors
  (fileless test documents trigger an eager ingest_doc task on a fresh
  clone); identical 34 on the pristine baseline with this PR's changes
  stashed, so this change adds zero regressions. Unrelated to permission
  filtering.

Out of scope

BaseVisibilityManager.visible_to_user in shared/Managers.py has the
structurally identical user-table-only pattern; it is only reached
un-overridden by CorpusActionExecution, which #1714 did not flag. Left for a
separate change to keep this fix surgical.

---

Generated by Claude Code

[claude]

Code Review — PR #1724: Honour group object-permissions in QuerySet.visible_to_user

Overview

This PR fixes a genuine filter/check drift (issue #1714): Manager.user_can always resolved group object-permissions (include_group_permissions=True), but the queryset bodies in PermissionQuerySet, DocumentQuerySet, and AnnotationQuerySet only joined the *userobjectpermission table — never *groupobjectpermission. A user whose sole READ grant was via a group therefore passed user_can(READ) yet was invisible in every list query. The fix ORs in a lazy subquery against the group permission table in each affected queryset body.

The approach is correct and the implementation is clean. A few things worth noting:

---

Correctness

• The values_list subqueries are lazy (not evaluated at Python layer), so group_id__in=user.groups.values_list("id", flat=True) stays a SQL subquery throughout. Query count is independent of group membership size — confirmed by test_group_lookup_uses_subquery_not_per_group_round_trips. ✓
• AnnotationQuerySet.visible_to_user correctly applies group subqueries to both the document-visibility predicate and the corpus-visibility predicate (annotations have no own permission table; visibility is MIN(doc, corpus)). ✓
• NoteQuerySet.visible_to_user inherits the fix transitively with no code change. Docstring update is accurate. ✓

---

Code Quality

Minor: user_group_ids extraction is inconsistent across methods

In AnnotationQuerySet.visible_to_user the group ids are extracted to a named variable before the try/except blocks:

user_group_ids = user.groups.values_list("id", flat=True)

But in PermissionQuerySet.visible_to_user and DocumentQuerySet.visible_to_user the expression is inlined:

group_id__in=user.groups.values_list("id", flat=True),

This is purely a style inconsistency — inlining a lazy queryset is fine. Consider extracting to a local variable in the other two methods for readability parity. Not a blocker.

Minor: Duplicate comment block pattern

The same comment explaining the "why" for group grants appears three times (once per queryset body), with only the model name varying. This is unavoidable given the current architecture (each queryset body is independent), but worth acknowledging. Any future refactor that extracts a _group_permitted_ids(user, model) helper could collapse it.

LookupError fallback uses [] (Python list) not a queryset

In AnnotationQuerySet.visible_to_user, when LookupError fires for the corpus lookup:

except LookupError:
    corpus_permitted_ids = []
    corpus_group_permitted_ids = []

Django handles Q(corpus_id__in=[]) as WHERE FALSE, which is correct. This matches the pre-existing pattern for doc_permitted_ids, so it's not a regression introduced by this PR. Just confirming it's intentional.

---

Security

• Read/write asymmetry preserved: The filter only queries read_* codenames; write permissions go through a separate code path. test_group_read_grant_does_not_widen_to_writes pins this correctly. ✓
• Non-member exclusion: test_stranger_without_group_membership_stays_excluded checks that a user not in the group cannot see any of the group-shared objects, and that user_can(READ) agrees. ✓
• No IDOR risk introduced: The fix adds inclusive OR clauses, not replaces existing checks. The change can only widen visibility to correctly-authorized users; it cannot grant access to users not in any group with the relevant permission. ✓

---

Tests

The new GroupObjectPermissionVisibilityTest class is well-structured and covers the right invariants:

Test	What it pins
test_document_group_grant_appears_in_visible_to_user	Basic filter inclusion + control (unshared doc excluded)
test_document_group_filter_check_equivalence	user_can ↔ visible_to_user parity — the core invariant
test_group_read_grant_does_not_widen_to_writes	Security: READ grant ≠ UPDATE/DELETE
test_permission_queryset_generic_body_honors_group_perms	Generic fallback body tested directly
test_annotation_group_grant_filter_check_equivalence	Annotation filter/check parity
test_note_group_grant_filter_check_equivalence	Note transitive inheritance
test_stranger_without_group_membership_stays_excluded	Non-member exclusion across all three models
test_group_lookup_uses_subquery_not_per_group_round_trips	Performance: O(1) queries, not O(groups)

Using TestCase (not TransactionTestCase) is appropriate here — no signals or async ORM operations involved. ✓

One suggestion: test_note_group_grant_filter_check_equivalence asserts self.assertTrue(check) and self.assertTrue(in_filter) without custom failure messages, unlike the document and annotation equivalents. Adding messages consistent with the other tests would make failures easier to diagnose.

---

Out-of-Scope Item (Acknowledged)

The PR correctly flags that BaseVisibilityManager.visible_to_user in Managers.py (lines ~238–248) has the structurally identical drift — it only queries *userobjectpermission, never *groupobjectpermission. Models that reach BaseVisibilityManager without a concrete queryset override (e.g., CorpusActionExecution) still have the bug. This is left for a separate change, which is a reasonable call for surgical scope. A follow-up issue to track this would be worth filing.

---

Relationship with PR #1712

The PR body correctly notes that #1712's test_group_shared_known_drift_user_can_vs_visible_to_user pins the drifted (broken) behaviour and will need converting to an equivalence assertion once one of the two PRs lands. The two PRs touch disjoint files, so merge order shouldn't create conflicts — but whoever merges second should verify that pinned-drift test is updated. Consider adding a comment or TODO in test_authorization_invariants.py now to make this dependency explicit for the reviewer who merges second.

---

Summary

This is a high-quality, well-scoped fix. The bug is real and the approach is correct. Tests are thorough and cover the critical security and performance invariants. No blockers; the notes above are all minor or informational.

Verdict: Approve with minor suggestions. The out-of-scope BaseVisibilityManager drift and the #1712 coordination are the most important follow-ups to track.

[claude]

Code Review — PR #1724: Honour group object-permissions in QuerySet.visible_to_user

This is a clean, well-targeted fix for a genuine authorization bug. The root cause analysis is sound, the implementation is correct, and the test suite is excellent. A few observations below.

---

What this PR does well

Correctness of the fix
The filter/check drift identified in #1714 is real: _default_user_can runs with include_group_permissions=True (via guardian's get_user_obj_perms), but the visible_to_user querysets only joined the *userobjectpermission table. The lazy values_list("id", flat=True) subquery approach keeps group resolution in-DB — exactly right.

AnnotationQuerySet scoping
user_group_ids is computed once and reused across both the doc and corpus predicate lookups. Good — avoids redundant queryset construction within the same call.

Test quality
GroupObjectPermissionVisibilityTest is comprehensive:

• Filter/check equivalence across Document, Annotation, Note, and the generic PermissionQuerySet body
• Write asymmetry (group READ does not bleed to UPDATE/DELETE)
• Non-member exclusion
• Performance pin (subquery count independent of group count)

The conversion of the "known drift" test in test_authorization_invariants.py to a proper equivalence assertion is exactly right — that test was a tripwire by design and the docstring said so.

---

Issues / Observations

1. Silent user-perm loss if group model is missing (DocumentQuerySet)

In DocumentQuerySet.visible_to_user (lines ~345–374), the user-level and group-level lookups share a single try block:

try:
    permission_model = apps.get_model("documents", "documentuserobjectpermission")
    permitted_ids = ...
    group_permission_model = apps.get_model("documents", "documentgroupobjectpermission")  # ← new
    group_permitted_ids = ...
    return self.filter(...)
except LookupError:
    return self.filter(Q(creator=user) | Q(is_public=True)).distinct()

If documentgroupobjectpermission raises LookupError (e.g., guardian not fully migrated), the entire block falls through and permitted_ids — the user-level grants that were already successfully resolved — are silently discarded. The AnnotationQuerySet avoids this by using separate try blocks for doc and corpus models, with per-block fallback assignments. The same split would be more defensive here:

try:
    permission_model = apps.get_model("documents", "documentuserobjectpermission")
    permitted_ids = permission_model.objects.filter(...).values_list(...)
except LookupError:
    permitted_ids = []

try:
    group_permission_model = apps.get_model("documents", "documentgroupobjectpermission")
    group_permitted_ids = group_permission_model.objects.filter(...).values_list(...)
except LookupError:
    group_permitted_ids = []

return self.filter(
    Q(creator=user) | Q(is_public=True) | Q(id__in=permitted_ids) | Q(id__in=group_permitted_ids)
).distinct()

The PermissionQuerySet.visible_to_user fallback body has the same structure (single try block covering both model lookups). Same concern, same suggested fix. In practice the risk is very low because guardian always creates both tables together, but the AnnotationQuerySet pattern is already safer and worth following consistently.

2. BaseVisibilityManager.visible_to_user remains unfixed (noted, but track it)

The PR description explicitly calls this out as out of scope — only CorpusActionExecution reaches the unoverridden manager path. That's a reasonable call for a surgical fix. Worth opening a follow-up issue if #1714 doesn't already track it, so the remaining gap doesn't fade from view.

3. Minor: PermissionQuerySet.visible_to_user comment references stale line numbers

# Mirrors BaseVisibilityManager.visible_to_user lines 217-228.

This was already in the pre-PR code. The reference lines in Managers.py may have shifted. Not introduced by this PR, but easy to fix in passing since the comment is in touched code.

---

Security

No regressions introduced. The fix only widens visibility to correctly-authorized users (group-granted read). The non-member exclusion test explicitly pins that the fix doesn't over-broaden. Write-asymmetry test is correct.

---

Performance

The lazy values_list subquery is the right approach. The performance pin test test_group_lookup_uses_subquery_not_per_group_round_trips correctly validates that query count is constant regardless of group count. One small note: the test passes lightweight=True which skips prefetch — that's fine for the purpose of measuring the core query count, but documenting why in a comment would help a future reader understand it's not omitting important side-effects.

---

Summary

The fix is correct, well-tested, and the test conversion in test_authorization_invariants.py is exactly what the old pinned drift test asked for. The main actionable item before merge is splitting the shared try block in DocumentQuerySet.visible_to_user (and the PermissionQuerySet fallback) to avoid silent loss of user-level grants if the group model lookup ever fails. Everything else is minor polish.

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
opencontractserver/shared/QuerySets.py	84.61%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Code Review — PR #1714: Honour group object-permissions in QuerySet.visible_to_user

This is a well-targeted fix for a real authorization correctness bug. The root cause diagnosis is accurate, the approach is sound, and the test suite is genuinely excellent. Below are observations ranging from blocking concerns to minor style notes.

---

Summary

Manager.user_can has always resolved group object-permissions (include_group_permissions=True), but the three affected visible_to_user overrides only joined *userobjectpermission tables, never *groupobjectpermission. The fix correctly ORs in a lazy-subquery path for group grants on each model. NoteQuerySet inherits the fix transitively.

---

✅ What's Working Well

• Separate try/except blocks for user-level vs. group-level guardian tables is the right choice. A missing group table silently zeroes group_permitted_ids without discarding the already-resolved permitted_ids. The degradation test (test_visible_to_user_degrades_when_guardian_tables_missing) correctly pins this.
• user.groups.values_list("id", flat=True) as a lazy subquery — stays a single SQL round-trip regardless of group count. The performance test pins this invariant explicitly, which is exactly the right approach.
• AnnotationQuerySet gets the fix on both its doc and corpus visibility predicates, including the structural-annotation path. No group-permission gap left in the three-way OR construction.
• The CHANGELOG entry is thorough and references the affected files/issue.
• The drift-test conversion in test_authorization_invariants.py is clean — the assertFalse is correctly flipped to assertTrue with the old "pin known drift" language removed.

---

⚠️ Issues to Address

1. BaseVisibilityManager.visible_to_user in Managers.py — structurally identical gap (acknowledged as "Out of scope")

The PR notes this deliberately but it's worth calling out explicitly: CorpusActionExecution (and any other model that reaches visible_to_user via BaseVisibilityManager without a queryset override) still has the user-table-only pattern. Users with group-granted READ on those objects will still see the filter/check drift. Please open a follow-up issue immediately so this doesn't age silently — the "out of scope" callout in the PR description is good, but the issue tracker is where it needs to live.

2. Minor inconsistency in where user_group_ids is computed

In PermissionQuerySet and DocumentQuerySet, user_group_ids is computed inside the group-permissions try block:

try:
    user_group_ids = user.groups.values_list("id", flat=True)
    group_permission_model = apps.get_model(...)
    ...
except LookupError:
    group_permitted_ids = []

In AnnotationQuerySet, it's computed outside both try blocks:

user_group_ids = user.groups.values_list("id", flat=True)

try:
    doc_group_perm_model = ...

The AnnotationQuerySet approach is actually slightly better (computed once, reused for both doc and corpus lookups), but the inconsistency across the three bodies could confuse a future reader. Consider either pulling user_group_ids out of the try block in PermissionQuerySet/DocumentQuerySet to match, or adding a brief inline note. Doesn't affect correctness since user.groups.values_list(...) won't raise LookupError.

3. Repeated group-lookup pattern — DRY concern per CLAUDE.md

The same three-line pattern:

user_group_ids = user.groups.values_list("id", flat=True)
group_permission_model = apps.get_model(app_label, f"{model_name}groupobjectpermission")
group_permitted_ids = group_permission_model.objects.filter(
    permission__codename=f"read_{model_name}", group_id__in=user_group_ids
).values_list("content_object_id", flat=True)

appears three times (and a variation in AnnotationQuerySet for two models). CLAUDE.md calls for maximal DRY. A small private helper:

def _group_permitted_ids(app_label: str, model_name: str, user) -> QuerySet:
    """Return a lazy subquery of IDs readable by any group the user belongs to."""
    user_group_ids = user.groups.values_list("id", flat=True)
    group_perm_model = apps.get_model(app_label, f"{model_name}groupobjectpermission")
    return group_perm_model.objects.filter(
        permission__codename=f"read_{model_name}", group_id__in=user_group_ids
    ).values_list("content_object_id", flat=True)

could be extracted to the top of QuerySets.py (or a nearby util), wrapping each call site in its own try/except LookupError. That would make the per-queryset bodies readable and keep the fallback logic in one place. This is a "should" rather than a blocker, but given the existing duplication across three separate queryset bodies it crosses the CLAUDE.md threshold.

---

🔍 Nits / Minor Observations

• Test count: The PR description says "8/8 pass" but I count 9 test methods in GroupObjectPermissionVisibilityTest (including test_group_lookup_uses_subquery_not_per_group_round_trips). Worth correcting in the description.

• test_permission_queryset_generic_body_honors_group_perms: Directly instantiating PermissionQuerySet(model=Document, ...) bypasses DocumentQuerySet.visible_to_user (the override). This is intentional — it's testing the base class body. The comment makes this clear, and the design is correct.

• test_stranger_without_group_membership_stays_excluded: Good negative-path test. Consider also asserting self.stranger not in self.group.user_set.all() as a setUp guard so the test fails loudly if the fixture is ever accidentally modified.

---

Verdict

Approve with one blocking request: open a follow-up issue for the BaseVisibilityManager gap before merging (or reference an existing one if it already exists). The DRY helper is a "should-fix" that could reasonably be done in this PR or a fast follow. Everything else is nits.

The test suite quality — especially the performance invariant test and the degradation test — sets a high bar. The fix is correct and surgical.

[claude]

Code Review: Honour group object-permissions in QuerySet.visible_to_user (#1714)

Overview

This PR closes a genuine security/correctness drift: Manager.user_can resolves group object-permissions (include_group_permissions=True) but the three visible_to_user query-set methods only joined the *userobjectpermission table — so a group-shared user passed user_can(READ) but was silently excluded from every list query. The fix ORs in a lazy subquery against *groupobjectpermission, keeping group resolution as a SQL subquery (single round-trip). Corpus was already correct via get_objects_for_user; Note inherits the fix transitively. The scope is correctly surgical and the change set is small.

---

What's Good

Correctness and approach

• The lazy user.groups.values_list("id", flat=True) pattern is exactly right — Django inlines it as a correlated subquery, so the query count stays flat regardless of group-membership size.
• Separate try/except LookupError blocks for the user-level and group-level table lookups is the right defensive call: a missing group table no longer discards already-resolved user grants.
• NoteQuerySet needing no code change (inherits via composition) is correctly identified and clearly explained.
• The "known drift" test in test_authorization_invariants.py is cleanly converted from a pinned-asymmetry assertion to an equivalence assertion, which is the right thing to do once the fix lands.

Test quality
GroupObjectPermissionVisibilityTest covers all the important angles:

• Filter/check equivalence for Document, Annotation, and Note
• Read/write asymmetry (group READ must not widen to UPDATE/DELETE)
• Non-member exclusion
• Generic PermissionQuerySet fallback body
• Performance (query count independence of group-membership size)
• Graceful degradation when guardian tables are missing

The performance test (test_group_lookup_uses_subquery_not_per_group_round_trips) with CaptureQueriesContext is particularly good — it pins the SQL shape, not just the result.

---

Issues and Suggestions

Minor: user_group_ids placement inconsistency

In AnnotationQuerySet.visible_to_user, user_group_ids is computed before the try blocks (computed once, reused for both doc and corpus group lookups — good). But in PermissionQuerySet and DocumentQuerySet, it's computed inside the second try block alongside apps.get_model. This is functionally fine (the early-return guards above mean user is always an authenticated non-anonymous user at that point), but the inconsistency is a mild readability issue. The AnnotationQuerySet pattern of computing it once outside the try blocks is arguably cleaner and worth standardising.

Minor: Out-of-scope BaseVisibilityManager.visible_to_user — worth a TODO comment or issue

The PR correctly calls out that BaseVisibilityManager.visible_to_user in shared/Managers.py has the structurally identical user-table-only pattern and is left for a follow-up (only reaches CorpusActionExecution un-overridden, so lower urgency). Given it's the same bug class, it's worth either a # TODO(#1714-follow-up) comment at that method or a tracking issue to ensure it doesn't fall through the cracks.

Minor: TransactionTestCase import in test_corpus_objs_service.py

This appears to be a stray addition unrelated to the group-permission fix. If it's not needed by an existing test in that file, consider dropping it to keep the diff surgical.

Observation: empty-list fallback behaviour

When permitted_ids = [] (the except LookupError path), Django generates WHERE id IN (ARRAY[]) or optimises it to WHERE FALSE. This is correct, but it's worth noting that before the fix these paths returned early, while now they continue to build the filter. The resulting behaviour is equivalent but the SQL path differs. No action needed, just worth being aware of if query plan regressions are ever investigated.

Project convention: no Claude credits in PR body

The PR description ends with _Generated by [Claude Code](...session_...)_. Per CLAUDE.md: "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." Please remove that footer before merge.

---

Security

No concerns. The fix only adds visibility for users who already hold an explicit group READ grant — it does not widen access beyond what user_can was already authorizing. The write-asymmetry tests confirm group READ doesn't bleed into UPDATE/DELETE, and the non-member-exclusion test confirms the fix is bounded to actual group members.

---

Summary

This is a well-scoped, correctly implemented fix for a real permission system bug. The tests are thorough and the approach (lazy subquery, separate try blocks, surgical scope) is sound. The items above are all minor. The only blocking issue before merge is removing the Claude Code attribution from the PR body. The TransactionTestCase stray import and the BaseVisibilityManager follow-up are worth addressing but not merge-blockers.

Verdict: Approve with minor nits — clean up the Claude attribution footer, optionally track the BaseVisibilityManager gap in an issue.