Honour group object-permissions in BaseVisibilityManager (#1731)

[JSv4]

Summary

BaseVisibilityManager.visible_to_user only joined the *userobjectpermission guardian table, while Manager.user_can resolves group object-permissions (_default_user_can runs with include_group_permissions=True). A user whose only READ grant on a model using the un-overridden manager body — CorpusActionExecution — was via a group therefore passed obj.user_can(user, READ) yet was excluded from Model.objects.visible_to_user(user): authorized for the row but never able to see it in a list query.

This is the follow-up tracked by issue #1731. PR #1724 (issue #1714) closed the structurally identical drift in the shared/QuerySets.py bodies but deliberately left the manager body for a follow-up, since CorpusActionExecution is the only model reaching it un-overridden (CorpusActionExecutionManager.visible_to_user merely re-wraps super().visible_to_user(...) in a custom QuerySet).

• BaseVisibilityManager.visible_to_user now also resolves the *groupobjectpermission table, filtered by the requesting user's group ids, and ORs Q(id__in=<group subquery>) into the visibility filter — mirroring the QuerySet.visible_to_user ignores group object-permissions (Document/Annotation/Note) — filter/check drift vs Manager.user_can #1714 QuerySets.py fix.
• The user- and group-table lookups sit in separate try/except LookupError blocks, so a model missing the group table never discards already-resolved user-level grants.
• Group ids resolve as a lazy values_list subquery, keeping the lookup a single round-trip (query count independent of how many groups the user belongs to).
• New CorpusActionExecutionGroupVisibilityTest pins filter/check equivalence for a group-shared user, the read/write asymmetry (a group READ grant never widens to UPDATE/DELETE), non-member exclusion, and graceful degradation when the guardian tables cannot be resolved.

Closes #1731

Test plan

• docker compose -f test.yml run --rm django pytest opencontractserver/tests/test_corpus_action_execution.py::CorpusActionExecutionGroupVisibilityTest --keepdb
• docker compose -f test.yml run --rm django pytest opencontractserver/tests/test_corpus_action_execution.py::CorpusActionExecutionPermissionsTestCase --keepdb — no regression on existing visible_to_user tests
• docker compose -f test.yml run --rm django pytest opencontractserver/tests/permissioning/test_permissioned_querysets.py::GroupObjectPermissionVisibilityTest --keepdb — sibling QuerySet.visible_to_user ignores group object-permissions (Document/Annotation/Note) — filter/check drift vs Manager.user_can #1714 suite still green

Note: the backend suite could not be run in the authoring environment — the Docker image build is blocked by the network policy (pip/wget cannot verify TLS through the proxy). CI runs these tests.

---

Generated by Claude Code

[claude]

Code Review — PR #1740: Honour group object-permissions in BaseVisibilityManager

Overview

This PR closes issue #1731 — the follow-up to #1714 — by mirroring the group-permission fix from shared/QuerySets.py into BaseVisibilityManager.visible_to_user. The structural diagnosis is correct: user_can resolved group grants via _default_user_can(include_group_permissions=True) but the manager body only joined the *userobjectpermission table, creating a filter/check split for any user whose sole READ path is a group grant on CorpusActionExecution.

The fix is clean, the separation into two independent try/except LookupError blocks is the right call (prevents a missing group table from nuking user-level grants), and the lazy values_list subquery correctly avoids a per-group round-trip.

---

Issues

1. Misleading warning messages (minor correctness bug)

Both except LookupError branches log "Falling back to creator/public check", but that description is no longer accurate:

• If the user-level table lookup fails, group-level grants are still resolved — the effective filter is creator | public | group_permitted_ids, not just creator | public.
• If the group-level table lookup fails, user-level grants are still included — the effective filter is creator | public | user_permitted_ids.

These messages will mislead anyone diagnosing permission issues from logs. Suggested replacements:

# user-level except:
logger.warning(
    f"Permission model {app_label}.{permission_model_name} not found. "
    "User-level grants will be skipped; group-level grants (if resolvable) still apply."
)

# group-level except:
logger.warning(
    f"Group permission model {app_label}.{group_permission_model_name} not found. "
    "Group-level grants will be skipped; user-level grants still apply."
)

2. Degradation test covers both-tables-fail but not partial failure

test_visible_to_user_degrades_when_guardian_tables_missing patches both the user- and group-level tables to fail simultaneously. That exercises the crash-safety story, but the whole point of the two separate try blocks is the asymmetric failure case — where one table exists and one does not.

Worth adding a test that patches only the user-level table to fail and asserts that group grants still work, explicitly confirming that the two-block separation does what the PR description says:

def test_group_grants_survive_when_user_table_missing(self):
    real_get_model = apps.get_model
    def fake(app_label, model_name=None, *args, **kwargs):
        name = model_name if model_name is not None else app_label
        if 'userobjectpermission' in name and 'group' not in name:
            raise LookupError(name)
        return real_get_model(app_label, model_name, *args, **kwargs)
    with patch.object(apps, 'get_model', side_effect=fake):
        visible_ids = set(
            CorpusActionExecution.objects.visible_to_user(self.group_user)
            .values_list('pk', flat=True)
        )
    self.assertIn(self.group_exec.pk, visible_ids)  # group grants survive

Without this test the resilience claim is documented but not exercised.

---

Nitpicks

• CHANGELOG verbosity: the new entry is longer than the QuerySet.visible_to_user ignores group object-permissions (Document/Annotation/Note) — filter/check drift vs Manager.user_can #1714 entry it mirrors. The implementation details (lazy subquery, two try blocks) belong in the PR description; the changelog reader mainly needs what-was-broken / what-now-works. Consider trimming to match the length of other recent entries.

• Comment density in Managers.py: the new block has a 10-line comment explaining the rationale, then fallback-assignment comments, then a comment before the final filter(). The code is straightforward enough that these could be condensed to one sentence per block (CLAUDE.md: "only add one when the WHY is non-obvious"). The detail is better placed in the PR description.

---

Verdict

The bug fix is correct and the core test matrix (filter/check equivalence, write-escalation guard, non-member exclusion, graceful degradation) is solid. The two issues above are real:

• Issue 1 is a low-severity log-message bug that will cause confusion during operational debugging.
• Issue 2 is a missing test for the exact invariant the two-block design is supposed to guarantee.

Both are straightforward to address before merge. Everything else is cosmetic.

[claude]

Code Review — PR #1740: Honour group object-permissions in BaseVisibilityManager

Overview

This PR closes issue #1731 — the structural twin of #1714. Manager.user_can (via _default_user_can) has always resolved group object-permissions (include_group_permissions=True), but BaseVisibilityManager.visible_to_user only joined the *userobjectpermission table. A user with a READ grant via a group therefore passed user_can(READ) yet was excluded from list queries — authorized for an individual row but invisible in collection endpoints. The fix ORs in a *groupobjectpermission subquery, mirroring the identical repair already applied in shared/QuerySets.py for #1714.

---

What's Good

• Mirrors the canonical pattern exactly. The new group-permission block in Managers.py is structurally identical to the PermissionQuerySet.visible_to_user body in QuerySets.py (lines 286–303), which is the right way to enforce consistency across the two entry points.
• Correctly isolated try/except blocks. Splitting user- and group-table lookups into separate try/except LookupError guards means a missing group table cannot discard already-resolved user-level grants. This is the correct design.
• Lazy subquery for group IDs. user.groups.values_list("id", flat=True) passed to group_id__in= is evaluated as a SQL subquery by the ORM, keeping the round-trip count flat regardless of group membership size.
• Test suite is thorough. CorpusActionExecutionGroupVisibilityTest covers the primary regression, filter/check equivalence, read/write asymmetry (security), non-member exclusion, and degraded-table fallback. All four invariants are the right ones to pin.

---

Issues

Minor — Misleading fallback log message

The new LookupError handler for the group table logs:

logger.warning(
    f"Group permission model {app_label}."
    f"{group_permission_model_name} not found. Falling back "
    "to creator/public check."
)

The phrase "Falling back to creator/public check" is inaccurate — user-level grants (permitted_ids) are still applied; only the group-level supplement is zeroed out. Compare the analogous handler in QuerySets.py which uses no warning log and no fallback description at all. Suggested fix:

logger.warning(
    f"Group permission model {app_label}.{group_permission_model_name} not found. "
    "Group-level grants will not be considered for this model."
)

Minor — Test doesn't directly verify the isolated-try invariant it claims to test

test_visible_to_user_degrades_when_guardian_tables_missing patches apps.get_model to raise LookupError for both *userobjectpermission and *groupobjectpermission, then asserts the group-only user loses access. This correctly verifies "no crash on combined table failure," but the docstring claims it also verifies "a missing group table never discards already-resolved user grants" — which requires patching only the group table while leaving the user table intact.

Suggested additional assertion (or a second test method):

def test_user_grant_survives_when_only_group_table_is_missing(self):
    """User-level grants must survive even if the group table cannot be resolved."""
    from unittest.mock import patch
    from django.apps import apps

    real_get_model = apps.get_model

    def fake_get_model(app_label, model_name=None, *args, **kwargs):
        name = model_name if model_name is not None else app_label
        if "groupobjectpermission" in name:
            raise LookupError(f"simulated missing table: {name}")
        return real_get_model(app_label, model_name, *args, **kwargs)

    from guardian.shortcuts import assign_perm
    assign_perm("read_corpusactionexecution", self.owner, self.group_exec)

    with patch.object(apps, "get_model", side_effect=fake_get_model):
        visible_ids = set(
            CorpusActionExecution.objects.visible_to_user(self.owner)
            .values_list("pk", flat=True)
        )
    self.assertIn(self.group_exec.pk, visible_ids)

This is the unique safety guarantee of the separate-try design; it deserves a direct test.

Nit — CHANGELOG entry prose length

The new CHANGELOG bullet is extremely long (~450 words). CHANGELOG.md already has a thorough entry for the #1714 sibling; this one repeats much of that context. Consider trimming to 2–3 sentences and linking to the issue/prior entry rather than re-explaining the mechanism in full.

---

Summary

The implementation is correct, consistent with the established QuerySets.py pattern, and the tests cover all the critical invariants. The only changes worth making before merge are the misleading warning log message (easy one-liner fix) and optionally an additional test case that directly exercises the isolated-try guarantee the docstring describes. Everything else is a nit.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!