Phase D: standardize IDOR pattern via get_for_user_or_none (#1658)

[JSv4]

Closes #1658.

Summary

Phase D of the permission-centralization plan. Adds a generic IDOR-safe lookup helper and migrates non-corpus-scoped Model.objects.get(pk=X) + permission_check mutation sites onto it so "pk doesn't exist" and "pk exists but caller can't READ it" return the same unified response. Closes the differential-error-text enumeration channel called out in CLAUDE.md's "IDOR Prevention" section.

What landed

• get_for_user_or_none(model_cls, pk, user) in opencontractserver/utils/permissioning.py. Generic over the model TypeVar so callers preserve concrete attribute typing through the gate. Catches ValueError/TypeError from garbage pk inputs so malformed GraphQL ids can't bubble as 500s. Cross-references CorpusObjsService in the docstring — corpus-scoped doc lookups continue to route through PR Refactor: Split DocumentService and CorpusObjsService for cleaner architecture #1685's service so the two IDOR-safe surfaces don't drift.
• 10 unit tests in opencontractserver/tests/permissioning/test_get_for_user_or_none.py pin every branch (creator / public / missing pk / inaccessible pk / explicit grant / superuser / anonymous-private / anonymous-public / garbage pk / None pk).
• Migrated (non-corpus-scoped sites): extract_mutations.py (UpdateExtractMutation, CreateExtractIteration), analysis_mutations.py (DeleteAnalysisMutation — previously raised three different exception types), agent_mutations.py (Create/Update/Delete AgentConfiguration), corpus_mutations.py (SetCorpusVisibility, UpdateCorpusMutation embedder pre-check, UpdateCorpusDescription, DeleteCorpusMutation is_personal pre-check, StartCorpusFork, ReEmbedCorpus, CreateCorpusAction, AddTemplateToCorpus, ToggleCorpusMemory, AddDocumentsToCorpus, RemoveDocumentsFromCorpus), badge_mutations.py (AwardBadgeMutation Badge / Corpus / User-recipient), label_mutations.py (DeleteMultipleLabelMutation, CreateLabelForLabelsetMutation, RemoveLabelsFromLabelsetMutation), conversation_mutations.py (5 sites), document_mutations.py (RetryDocumentProcessing), document_relationship_mutations.py (CreateDocumentRelationship + UpdateDocumentRelationship).
• READ as a precondition for non-READ ops. A handful of mutations previously allowed PERMISSION-without-READ or UPDATE-without-READ to "work" because they fetched the row with a bare Model.objects.get(pk=...) and then checked the higher permission directly. After Phase D every migrated mutation routes the lookup through visible_to_user first — the caller must be able to see the row before any non-READ check fires. Three pinning tests were updated to grant [READ, PERMISSION] / [READ, UPDATE] instead of the higher permission alone (test_user_with_permission_perm_can_change_visibility, test_non_owner_with_explicit_update_permission_can_create, test_remove_labels_allows_non_owner_with_explicit_update_permission). test_invalid_id_format_returns_error updated to assert the unified ok=False response now that the helper swallows malformed-pk ValueErrors instead of letting them bubble as GraphQL errors.
• Superuser bypass added to UserProfileManager.visible_to_user (opencontractserver/users/models.py:51). Mirrors BaseVisibilityManager's superuser branch so admin / moderation surfaces (badge awarding) can route the recipient lookup through get_for_user_or_none and still reach private-profile users — without weakening the IDOR-safe pattern for non-superuser callers.
• CHANGELOG entry under ### Security.

Test plan

• opencontractserver/tests/permissioning/test_get_for_user_or_none.py — 10 tests pass (helper contract).
• Per-file regression on every migrated mutation file — all green.
• Comprehensive regression across permissioning/ + every affected test_*.py: 557 passed, 1 skipped, 0 failed (~11 min serial).
• Pre-commit (black / isort / flake8 / mypy) clean on every committed file.
• Manual Playwright check per CLAUDE.md auth pattern: non-existent pk vs other-user's pk vs valid pk all return the unified envelope on the migrated mutations.

Out of scope

• Read-only field resolvers already going through .visible_to_user(...).
• Internal services / query optimizers receiving already-validated instances.
• annotation_mutations.py — already uses visible_to_user(user).get(pk=X) throughout; no bare-get IDOR risk and the surface is large enough to deserve its own pass.
• UpdateMessageMutation — uses select_related that doesn't compose with the helper's .filter(pk=pk).first() chain; the site is already IDOR-safe via visible_to_user.

[claude]

Code Review — Phase D: IDOR-safe get_for_user_or_none helper

Overall verdict: Solid security improvement with a clean helper design, comprehensive tests, and consistent migration across ~30 mutation sites. Four issues worth addressing before merge, ranging from a performance regression to a subtle behavioral change.

---

What this does well

• get_for_user_or_none is well-designed. The TypeVar bound preserves concrete attribute typing for callers, the hasattr(manager, "visible_to_user") guard fails loudly on misconfigured models instead of silently bypassing IDOR protection, and the ValueError/TypeError catch on malformed pks is the right place to absorb untrusted GraphQL ids.
• 10-test suite pins every branch (creator / public / missing / inaccessible / explicit grant / superuser / anonymous private / anonymous public / garbage pk / None pk). This is exactly the contract that needs pinning for a security helper.
• READ-as-precondition rule is a meaningful security tightening: granting UPDATE/DELETE without READ no longer silently "works" by bypassing the visibility filter.
• Removing the print() debug statements from extract_mutations.py is good cleanup.
• Changelog entry is detailed and correctly scoped.

---

Issues

1. Performance regression — is_new=True stripped from all creation paths (significant)

set_permissions_for_obj_to_user has an is_new optimization: when True, it skips the upfront remove_perm sweep (7 guardian DB ops) because a freshly-created object has no prior permissions to clear. The PR removes is_new=True from every creation path it touches:

CreateAgentConfigurationMutation, CreateBadgeMutation, CreateThreadMutation, CreateThreadMessageMutation, ReplyToMessageMutation, CreateFieldset, CreateColumn, CreateMetadataColumn, SetMetadataValue (when created), CreateExtract, CreateExtractIteration, CreateLabelMutation, CreateLabelForLabelsetMutation, _clone_fieldset_for_iteration (called once per cloned column).

The last one is the worst case. _clone_fieldset_for_iteration creates a new fieldset and then creates N column clones in a loop, each calling set_permissions_for_obj_to_user without is_new=True. That's (N+1) × 7 extra remove_perm round-trips on every iteration clone. A fieldset with 20 columns now does 147 unnecessary DB ops that the old code avoided.

None of this is mentioned in the PR description, and none of the migrated sites are "sharing/downgrade flows" that need the full replace semantics — they're all creation paths. This looks like an accidental side-effect of the refactor.

Suggestion: restore is_new=True on all call sites where the object was just created (the is_new=True parameter is keyword-only, so adding it back is safe and non-breaking).

---

2. DeleteAnalysisMutation uses exception flow, not the unified response envelope (inconsistency)

Every other migrated mutation returns ok=False, message=... directly. DeleteAnalysisMutation instead does:

analysis = get_for_user_or_none(Analysis, analysis_pk, info.context.user)
if analysis is None:
    raise PermissionError(not_found_msg)

This was pre-existing for the lock and DELETE-permission checks, but routing the get_for_user_or_none result through raise rather than return means the caller gets a GraphQL errors array rather than an ok=False mutation envelope — different UX from every other mutation in this PR.

The PR description says one goal is "the same unified response", but for this mutation the surface is errors not data.deleteAnalysis.ok. Consider switching to return style (the mutation already has ok / message fields) or at minimum calling out that this mutation intentionally stays exception-based.

---

3. DeleteCorpusMutation double-lookup (minor)

obj = get_for_user_or_none(cls.IOSettings.model, id, info.context.user)
if obj is not None and obj.is_personal:
    raise GraphQLError(...)
return super().mutate(root, info, *args, **kwargs)  # does its own DB lookup

When obj is None the code falls through to super().mutate(), which will perform a second DB query on the same pk. The comment correctly identifies that the parent renders the not-found/no-permission response, but this means two lookups for every unauthorized or missing corpus. This could be simplified by short-circuiting when obj is None:

obj = get_for_user_or_none(cls.IOSettings.model, id, info.context.user)
if obj is None:
    return cls(ok=False, message="Corpus not found or you don't have permission")
if obj.is_personal:
    raise GraphQLError(...)
return super().mutate(root, info, *args, **kwargs)

(or whatever the parent's not-found response shape is). The current approach is correct but sub-optimal.

---

4. Behavioral change in AwardBadgeMutation for non-superuser awarders (needs confirmation)

The recipient lookup was:

recipient = User.objects.get(pk=recipient_pk)  # any valid pk worked

Now it's:

recipient = get_for_user_or_none(User, recipient_pk, awarder)

After adding the superuser bypass to UserProfileManager.visible_to_user, superuser awarders can still reach private-profile recipients. But non-superuser awarders (e.g. corpus owners with PERMISSION on a corpus) can no longer award badges to users with private profiles — they get "User not found" even for valid pks.

If the award-badge flow is superuser-only in practice this is fine. But if corpus owners or moderators can legitimately award badges, this is a breaking behavioral change. Please confirm the intended non-superuser awarding surface and add a test that documents it.

---

Minor nits

• StartCorpusFork comment acknowledges the redundant user_can(READ) after get_for_user_or_none ("kept for defense in depth"). That's reasonable, but the comment is long enough to slightly obscure the logic. A one-liner referencing the issue number would be less distracting.
• request=info.context is also stripped from the set_permissions_for_obj_to_user calls alongside is_new. The docstring says None is safe for paths that don't reuse the instance post-grant, which is true for creation paths. Just worth confirming no subsequent user_can fires on the same newly-created object within the same request (a quick grep shows none, so this is fine).

---

Test coverage

Existing test updates correctly reflect the READ precondition rule. The test_invalid_id_format_returns_error update is a nice self-documenting improvement — the new assertion is clearer than "GraphQL errors array is non-null". No gaps in the helper's own test suite.

The manual Playwright check is still unchecked — understandable for a PR this size, but worth doing before merge given that 30+ mutation surfaces changed.

[claude]

Code Review — Phase D: IDOR-safe get_for_user_or_none helper + mutation migration

Overall this is a well-scoped, well-documented security hardening PR. The central abstraction is clean, the TypeVar approach is correct, test coverage is solid, and the systematic migration across ~30 call sites is consistent. A few issues are worth addressing before merge.

---

✅ Strengths

• Helper design is correct. get_for_user_or_none cleanly collapses "doesn't exist" and "can't READ" into one None return. The TypeError guard on missing visible_to_user fails loudly for dev misconfiguration (right call). Catching ValueError/TypeError from the ORM on garbage pks is the right place for that guard.
• TypeVar usage. TypeVar("_T_Model", bound=django.db.models.Model) is the correct way to preserve concrete return types through a generic helper — callers get Extract | None rather than Model | None.
• Test coverage. The 10-test file pins every branch of the helper contract. The new badge test exercises both the non-superuser block and superuser bypass paths. The existing test updates are correct and clearly documented.
• READ-as-precondition rule. Enforcing READ before any higher-privilege op is the right semantic and the test updates (granting [READ, PERMISSION] / [READ, UPDATE] instead of the higher permission alone) correctly reflect the new contract.
• Docstring cross-reference to CorpusObjsService. This is the right way to prevent two parallel IDOR-safe surfaces from drifting.

---

🔴 Issues

1. DeleteCorpusMutation uses exception-for-flow-control (corpus_mutations.py:332–347)

obj = get_for_user_or_none(cls.IOSettings.model, id, info.context.user)
if obj is None:
    raise cls.IOSettings.model.DoesNotExist(
        f"{cls.IOSettings.model.__name__} matching pk={id} does not exist or is not visible to user."
    )

This is fragile. It relies on the parent DRFDeletion.mutate catching DoesNotExist and converting it to ok=False. If the parent's exception-handling behavior ever changes, this silently reverts to surfacing a GraphQL errors entry rather than the unified envelope — exactly the leak we're trying to prevent.

The canonical pattern used everywhere else in this PR is a direct return cls(ok=False, message=...). Is there a reason the parent class can't handle a pre-validated object being passed in, or is there a super().mutate(...) call that handles the is_personal check? If the is_personal guard is the only thing the parent adds, consider inlining it here and returning directly, rather than trusting the parent to swallow DoesNotExist.

2. from_global_id exceptions not fully guarded in analysis_mutations.py (line ~184)

analysis_pk = from_global_id(id)[1]
analysis = get_for_user_or_none(Analysis, analysis_pk, info.context.user)

from_global_id raises Exception on malformed base64. get_for_user_or_none only catches ValueError/TypeError from the ORM, not from from_global_id itself. This means a malformed id argument surfaces as a GraphQL errors entry rather than the ok=False envelope — the remaining enumeration channel the PR is trying to close.

The fix is consistent with the rest of the PR:

try:
    analysis_pk = from_global_id(id)[1]
except Exception:
    return DeleteAnalysisMutation(ok=False, message=not_found_msg)

3. ToggleCorpusMemory catches a narrower exception set than from_global_id can raise (corpus_mutations.py:~1506)

try:
    corpus_pk = from_global_id(corpus_id)[1]
except (ValueError, IndexError):
    return ToggleCorpusMemory(ok=False, message=not_found_msg, corpus=None)

from_global_id raises a bare Exception (via binascii.Error) on invalid base64, which is not a ValueError or IndexError. The other callsites in this file use except Exception: — this one should too.

---

🟡 Minor / Nits

4. StartCorpusFork redundant double-READ check (corpus_mutations.py:~520)

corpus = get_for_user_or_none(Corpus, corpus_pk, info.context.user)
if corpus is None or not corpus.user_can(info.context.user, PermissionTypes.READ, request=info.context):

get_for_user_or_none already enforces READ via visible_to_user. The user_can(READ) check is purely redundant — if the user can't READ, get_for_user_or_none returns None and the or short-circuits. The comment calls this "defense-in-depth" which is defensible, but it executes a second permission evaluation (with possible DB/cache hit) every time. If the intent is truly defense-in-depth, a code comment is sufficient; if it's just belt-and-suspenders, removing it makes the pattern consistent with the rest of the migrated callsites.

5. UserProfileManager.visible_to_user superuser bypass scope (users/models.py:73–76)

if getattr(user, "is_superuser", False):
    return self.filter(is_active=True)

This is the right call for the badge use-case and mirrors BaseVisibilityManager. Worth confirming: are there any read-only GraphQL query resolvers that call User.objects.visible_to_user(superuser) to populate a user-search or user-list field? If so, superusers will now see private-profile users in those query results too. That may be intentional (superusers should see everything), but it's a surface area change worth a grep to confirm no query-layer resolver is unexpectedly widened.

6. DeleteMultipleLabelMutation — creator check after get_for_user_or_none (label_mutations.py:~186–200)

label = get_for_user_or_none(AnnotationLabel, label_pk, user)
if label is None:
    return DeleteMultipleLabelMutation(ok=False, message="Label not found")
if label.read_only:
    return DeleteMultipleLabelMutation(ok=False, message="Cannot delete read-only labels")
if not user.is_superuser and label.creator_id != user.id:
    return DeleteMultipleLabelMutation(ok=False, message="Label not found")
label.delete()

The logic is correct but note the ordering: read_only is checked before the creator gate. A non-creator who can READ a public read-only label gets "Cannot delete read-only labels" rather than "Label not found". This is a mild information leak (reveals the label exists and is read-only). Swapping the order — creator check before read_only check — would make the non-creator path always return "Label not found" regardless. Small point, but it closes a residual differential-response channel.

---

Test plan note

The PR marks the Playwright check as incomplete:

• Manual Playwright check per CLAUDE.md auth pattern: non-existent pk vs other-user's pk vs valid pk all return the unified envelope on the migrated mutations.

Given that DeleteAnalysisMutation previously raised PermissionError → errors[] and now returns via ok=False, any existing frontend code that checks result.errors for that mutation will silently stop seeing permission denials. The Playwright check is worth completing before merge if the analysis delete flow is surfaced in the UI.

---

Summary

Category	Count
🔴 Issues to fix	3 (items 1–3)
🟡 Minor / nits	3 (items 4–6)
✅ No changes needed	—

The core abstraction is correct and the migration is systematic and well-tested. Items 1–3 are the ones worth resolving before merge; items 4–6 are lower-priority.

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
frontend/src/views/Corpuses.tsx	75.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Code Review — Phase D: IDOR-safe get_for_user_or_none helper + mutation migration

Overall this is a well-executed security hardening PR. The helper is correctly designed, the migration is thorough, and the test coverage is solid. A few items worth addressing before merge:

---

The core helper (opencontractserver/utils/permissioning.py)

Strengths:

• TypeVar-based generic preserves concrete return types through the gate — mypy and IDEs keep full attribute access
• None pk short-circuit and ValueError/TypeError catch prevent 500s on garbage GraphQL input
• hasattr(manager, 'visible_to_user') guard raises TypeError immediately on a misuse (calling error, not a runtime data error) — correct to raise here rather than return None
• Docstring cross-references the corpus-scoped alternative so future contributors pick the right tool

One concern — _default_manager + .first() ordering:

return manager.visible_to_user(user).filter(pk=pk).first()

.first() implicitly adds ORDER BY pk ASC unless the model's Meta.ordering is empty. For models with many rows and a guardian permissions join this is benign, but it's subtly different from .get(pk=pk) (which doesn't sort). No correctness risk, just worth knowing.

---

Potential info-leak in AddDocumentsToCorpus / RemoveDocumentsFromCorpus

# corpus_mutations.py ~411
corpus = get_for_user_or_none(Corpus, from_global_id(corpus_id)[1], user)

from_global_id(corpus_id)[1] is evaluated inline as the pk argument. If corpus_id is a malformed base64 string, from_global_id raises before get_for_user_or_none is called, and the exception falls through to the outer except Exception as e handler which returns:

return AddDocumentsToCorpus(message=f"Error on upload: {e}", ok=False)

That message echoes the raw exception text (including the malformed ID). All other migrated sites either wrap from_global_id in a dedicated try/except Exception before calling the helper, or pass the pk as a pre-decoded variable. Suggest extracting the pk decode:

try:
    corpus_pk = from_global_id(corpus_id)[1]
except Exception:
    return AddDocumentsToCorpus(message=not_found_msg, ok=False)
corpus = get_for_user_or_none(Corpus, corpus_pk, user)

Same issue exists in RemoveDocumentsFromCorpus.

---

DeleteCorpusMutation schema shape change is a frontend-breaking change

The old DRFDeletion-based implementation returned null for deleteCorpus on error (with a GraphQL errors entry). The new plain graphene.Mutation returns { ok, message }. Tests confirm the new shape, and the test updates in test_permissioned_querysets.py correctly reflect it. But any frontend code that patterns on the old null-response or errors field needs updating. Worth a CLAUDE.md note or a grep for deleteCorpus in the frontend if that hasn't happened yet.

---

label_mutations.py — raise LabelSet.DoesNotExist() as flow control

Both CreateLabelForLabelsetMutation and RemoveLabelsFromLabelsetMutation still use:

labelset = get_for_user_or_none(LabelSet, labelset_pk, info.context.user)
if labelset is None or not labelset.user_can(...):
    raise LabelSet.DoesNotExist()

This works because the enclosing try/except LabelSet.DoesNotExist handler converts it to an ok=False response. But it's the only migration site that doesn't return the envelope directly — inconsistent with the Phase D "return ok=False envelope" pattern and relies on exception-as-flow-control. Not a blocking issue, but a cleanup candidate for Phase E.

---

Behavioral change: READ as precondition for higher-permission ops

The PR is clear that this is intentional and the updated tests prove it. The places where it's tightest:

• SetCorpusVisibility: user now needs READ + PERMISSION (not just PERMISSION)
• RemoveLabelsFromLabelset / CreateLabelForLabelset: READ + UPDATE (not just UPDATE)
• AwardBadge recipient: now filtered through UserProfileManager.visible_to_user

The docstring and CHANGELOG document this well. One small note: test_non_owner_with_explicit_update_permission_can_create in test_label_mutations.py should explicitly assert that granting UPDATE without READ now fails (currently it only proves the READ+UPDATE path succeeds). Adding the negative case would pin the regression more firmly.

---

UserProfileManager.visible_to_user superuser bypass

if getattr(user, "is_superuser", False):
    return self.filter(is_active=True)

Correct ordering (anonymous → superuser → authenticated), getattr with default handles non-User types safely. The new test_non_superuser_cannot_award_to_private_profile_recipient test covers both the blocked non-superuser path and the allowed superuser path — good.

One edge: the comment says this mirrors BaseVisibilityManager's superuser branch. Confirming BaseVisibilityManager uses the same is_active=True filter (not is_active=True, is_deleted=False or similar) would give confidence the two are actually in sync.

---

Removed debug print() calls in extract_mutations.py

The removal of several print(f"Attempting to update extract...") statements scattered through UpdateExtractMutation is a silent cleanup win. Good catch.

---

Summary

Category	Assessment
Security	Solid — closes the differential-error-text IDOR channel consistently
Correctness	Good — no logic regressions found
Test coverage	Good — 10 new helper tests + regression on every migrated file
Blocking	AddDocumentsToCorpus / RemoveDocumentsFromCorpus inline from_global_id decode could leak error text through the outer except Exception handler
Non-blocking	label_mutations raise-as-flow-control inconsistency; DeleteCorpusMutation response-shape change needs frontend verification; negative test case for the READ-precondition regression

[claude]

Code Review — Phase D: IDOR-safe get_for_user_or_none (#1658)

Overview

This is a well-conceived security hardening PR. The core pattern — a TypeVar-generic helper that collapses "pk doesn't exist" and "pk exists but caller lacks READ" into a single None return — is exactly the right abstraction. The 10 pinned unit tests, the cross-reference to CorpusObjsService in the docstring (so the two IDOR-safe surfaces don't drift), and the hasattr(manager, 'visible_to_user') fail-fast guard are all strong choices. The DeleteCorpusMutation refactor away from DRFDeletion is necessary to get the consistent ok=False envelope, and the UserProfileManager superuser bypass mirrors the base class correctly.

Three issues worth addressing before merge, plus a handful of smaller observations.

---

Issues

1. UpdateExtractMutation — from_global_id is now unguarded (regression)

File: config/graphql/extract_mutations.py ~line 942

The original code wrapped from_global_id(id)[1] inside a try/except Extract.DoesNotExist. The Phase D rewrite moved from_global_id outside any try block:

# OLD — from_global_id inside the try
try:
    extract_pk = from_global_id(id)[1]
    extract = Extract.objects.get(pk=extract_pk)
except Extract.DoesNotExist:
    return UpdateExtractMutation(ok=False, message="Extract not found.", obj=None)

# NEW — from_global_id outside any guard
extract_pk = from_global_id(id)[1]          # ← no try/except
extract = get_for_user_or_none(Extract, extract_pk, user)

from_global_id raises binascii.Error (a ValueError subclass) on malformed base64. The helper only catches ValueError/TypeError inside filter(pk=pk).first() — it does not protect a naked from_global_id call that precedes it. A malformed id argument now surfaces as an unhandled exception / GraphQL errors entry rather than the unified ok=False envelope. Every other migrated site that faces the same risk either wraps from_global_id in its own try/except Exception (e.g. SetCorpusVisibility, DeleteAnalysisMutation, ToggleCorpusMemory, AddDocumentsToCorpus) or uses int() with (TypeError, ValueError) (CreateExtractIteration).

Fix: Wrap the from_global_id call as is done in the other migrated mutations:

try:
    extract_pk = from_global_id(id)[1]
except Exception:
    return UpdateExtractMutation(ok=False, message=extract_not_found_msg, obj=None)

---

2. DeleteCorpusMutation.is_personal path raises GraphQLError — breaks the ok=False envelope

File: config/graphql/corpus_mutations.py ~line 546

After Phase D the mutation's happy/sad path contract is:

{ data: { deleteCorpus: { ok: bool, message: str } }, errors: null }

The is_personal branch breaks this:

if obj.is_personal:
    raise GraphQLError(                      # ← surfaces as errors[], not data.ok
        "Cannot delete your personal 'My Documents' corpus. ..."
    )

Any frontend consumer that pattern-matches on data.deleteCorpus.ok will silently mishandle the one case that still returns errors[] instead of { ok: false, message: ... }. The existing tests in test_permissioned_querysets.py and test_permissioning.py now assert errors is absent for the standard unauthorised path, making this the only remaining path that can produce errors.

The PR description notes that is_personal is "observable to anyone who can READ the corpus" — this is true and the distinct message is fine — but there's no security reason the error has to travel via GraphQLError. Returning cls(ok=False, message=...) leaks nothing new and gives a uniform envelope:

if obj.is_personal:
    return cls(
        ok=False,
        message=(
            "Cannot delete your personal 'My Documents' corpus. "
            "This corpus is automatically managed and stores your uploaded documents."
        ),
    )

---

3. label_mutations.py — two migrated mutations still raise LabelSet.DoesNotExist for control flow

File: config/graphql/label_mutations.py lines 1128–1129 and 1141–1142

CreateLabelForLabelsetMutation and RemoveLabelsFromLabelsetMutation now use get_for_user_or_none for the lookup but immediately fall back to raise LabelSet.DoesNotExist() on failure:

labelset = get_for_user_or_none(LabelSet, labelset_pk, info.context.user)
if labelset is None or not labelset.user_can(..., PermissionTypes.UPDATE, ...):
    raise LabelSet.DoesNotExist()   # ← preserved from old code

This is the one pre-Phase D control-flow pattern that survived the migration. Its response shape depends entirely on how the enclosing except handles it, making it opaque compared to every other mutation in this PR. While this appears to be pre-existing behaviour rather than a Phase D regression, it's inconsistent enough that it warrants a follow-up issue (or a note in the PR description that it's deliberate).

---

Smaller observations

corpus_mutations.py — StartCorpusFork message inconsistency
The non-found branch returns "Corpus not found" rather than the unified "Corpus not found or you don't have permission" used everywhere else in this PR. Nitpick, but a search-and-grep later will find only the short form for this one mutation.

corpus_mutations.py — UpdateCorpusDescription tighter-than-READ gate
The check is if corpus is None or corpus.creator != user. A non-creator who holds a guardian UPDATE grant gets back "not found or you don't have permission to update it", which is correct (creator-only is intentional), but callers with a guardian UPDATE grant will be confused. The comment could say explicitly that this mutation is creator-only by design.

label_mutations.py — unguarded from_global_id inside the label-pk loop
label_pks = [int(from_global_id(gid)[1]) for gid in label_ids] — if any element of label_ids is malformed base64, from_global_id raises before get_for_user_or_none is reached. There's an outer except Exception that catches this, but it returns a generic message rather than the unified IDOR envelope. This is pre-existing behaviour, but worth noting since the loop is on the same hot path being hardened.

get_for_user_or_none — no test for a model that lacks visible_to_user
The helper raises TypeError with a clear message when _default_manager doesn't have visible_to_user. That's good behaviour, but there's no unit test pinning it. If someone accidentally calls the helper on a plain model, they'd get an exception in production, not a None. Consider adding one test case:

def test_raises_on_model_without_visibility_manager(self):
    from django.contrib.auth.models import Permission  # no visible_to_user
    with self.assertRaises(TypeError):
        get_for_user_or_none(Permission, 1, self.owner)

---

Test coverage — what's well covered

• All 10 branches of the helper itself (creator / public / missing / inaccessible / explicit grant / superuser / anonymous-private / anonymous-public / garbage pk / None pk) ✓
• DeleteCorpusMutation — regular user and collaborator paths ✓
• SetCorpusVisibility — invalid ID format now returns unified envelope ✓
• AwardBadgeMutation — superuser bypass for private-profile recipient ✓
• Label mutation tests updated for READ+UPDATE precondition ✓

What's missing: The is_personal delete path; a malformed-ID test for UpdateExtractMutation; the new lock check in DeleteCorpusMutation.

---

Summary

The design is sound and the bulk of the migration is clean. Items 1 and 2 above are actionable before merge (a 500 on malformed extract IDs is a real regression; a broken envelope shape on is_personal will silently misparse in the frontend). Item 3 is worth a follow-up issue if not fixed here.

[claude]

Code Review - Phase D: get_for_user_or_none + IDOR migration

Overall this is a well-scoped security hardening PR. The helper is clean, TypeVar generics are correct, the fail-loud guardrail for non-permissioned models is a nice touch, and the docstring cross-referencing CorpusObjsService sets the right boundary so the two IDOR-safe surfaces do not drift.

Issues

1. ReEmbedCorpus: from_global_id failure returns a distinct message (minor)
   corpus_mutations.py ~line 687 still returns 'Invalid corpus ID' on parse failure rather than the unified not-found message. Every other migrated site (SetCorpusVisibility, DeleteCorpusMutation, ToggleCorpusMemory, AddDocumentsToCorpus, RemoveDocumentsFromCorpus) routes parse failure through the unified message. Should be 'Corpus not found' to match the message used later in the same function for the non-creator case.

2. CreateAgentConfigurationMutation and AwardBadgeMutation: bare from_global_id inside outer try (low)
   In both mutations, corpus_pk decode is inside the outer try/except Exception block rather than its own pre-guarded try. A malformed corpus_id bubbles to the outer catch rather than the corpus-specific envelope. Not a security issue but worth aligning with the explicit try/except -> unified message pattern used in SetCorpusVisibility et al.

3. DeleteMultipleLabelMutation: reorder changes surface message (pre-existing but worth noting)
   The creator check now runs before the read-only check. A non-creator who can READ a public built-in label previously got 'Cannot delete read-only labels'; now they get 'Label not found'. Intentional (IDOR-correct) but worth noting in the PR body in case product needs to distinguish these states.

Observations

4. UpdateCorpusDescription: creator-only gate is now IDOR-collapsed (confirm with frontend)
   Users with a guardian UPDATE grant who are not the creator previously got a distinct error. They now get the unified envelope. Confirm no frontend logic renders a separate message for this case.

5. Removed print() debugging in UpdateExtractMutation (good cleanup).

6. DeleteCorpusMutation: DRFDeletion to graphene.Mutation is a response shape change. Previously returned null with errors[]. Now returns ok=false with a message field. Confirm no frontend code pattern-matches on data.deleteCorpus === null for this mutation.

7. test_raises_on_model_without_visibility_manager: Using django.contrib.auth.models.Permission as the sentinel is fragile. A purpose-built stub model would be more hermetic.

8. UserProfileManager superuser bypass: correct and well-commented inline, but worth documenting in docs/permissioning/consolidated_permissioning_guide.md so future contributors do not inadvertently revert it.

Positives

• TypeVar-generic signature preserves concrete model types through the gate - no type-safety lost.
• Catching ValueError/TypeError at the ORM level (not just from_global_id) means the IDOR contract holds even when callers decode the pk themselves.
• 10 unit tests in test_get_for_user_or_none.py cover every branch: creator, public, missing pk, inaccessible pk, explicit grant, superuser, anonymous-private, anonymous-public, garbage pk, None pk.
• Removing the GraphQLError/PermissionError/DoesNotExist three-way exception mix from DeleteAnalysisMutation is a meaningful simplification.
• is_personal corpus rejection now returns ok=False instead of raising GraphQLError, giving frontend a single response shape to pattern-match on.

[claude]

Phase D IDOR Review

Solid security refactor overall. The get_for_user_or_none helper is cleanly designed (TypeVar preserves concrete types, fail-loud TypeError for unmanaged models, garbage-pk guard is thorough), and the test suite at test_get_for_user_or_none.py pins all the key branches. The UpdateCorpusMutation embedder pre-check is a nice catch — the old bare Corpus.objects.get() without user filtering could have leaked corpus state to users who couldn't see the corpus at all.

A few things worth a look before merging:

---

1. from_global_id exception handling is inconsistent across migrated mutations

Several corpus and extract mutations explicitly wrap from_global_id in its own try/except Exception so a malformed base64 id surfaces through the unified ok=False envelope:

# corpus_mutations.py — explicit wrap
try:
    corpus_pk = from_global_id(corpus_id)[1]
except Exception:
    return SetCorpusVisibility(ok=False, message=not_found_msg)

But UpdateAgentConfigurationMutation, DeleteAgentConfigurationMutation, CreateThreadMutation (corpus and document arms), CreateThreadMessageMutation, ReplyToMessageMutation, DeleteConversationMutation, and DeleteMessageMutation all call from_global_id unwrapped inside the outer mutation-level except Exception as e. If the id is malformed base64, the outer catch runs and returns whatever generic message that handler produces — not the unified IDOR-safe message.

get_for_user_or_none does catch garbage pks after the ORM call (ValueError/TypeError from the .filter(pk=pk) step), but it can't help if from_global_id raises before the helper is even reached.

Suggested fix: wrap every from_global_id call at the top of the lookup chain with the same try/except Exception pattern used in the corpus and extract mutations, or add a small helper like decode_global_id_or_none(gid) that returns (None, None) on failure.

---

2. Error message inconsistency in CreateAgentConfigurationMutation

agent_mutations.py lines 56-62 (in the diff):

try:
    corpus_pk = from_global_id(corpus_id)[1]
except Exception:
    return CreateAgentConfigurationMutation(
        ok=False,
        message="Corpus not found",   # ← shorter message
        agent=None,
    )
corpus = get_for_user_or_none(Corpus, corpus_pk, user)
if corpus is None or not corpus.user_can(...):
    return CreateAgentConfigurationMutation(
        ok=False,
        message="Corpus not found or you don't have permission to create agents for it.",
        agent=None,
    )

The malformed-id path and the "not found / no permission" path return different strings. A caller could distinguish a malformed id from a genuinely missing/inaccessible corpus by diffing the message text — not a IDOR-in-the-pk-enumeration sense, but inconsistent with the unified-envelope goal. Both branches should emit the same string.

---

3. set_permissions_for_obj_to_user called with .id in the new test

test_permissioned_querysets.py line 363 (in the diff):

set_permissions_for_obj_to_user(
    self.owner.id,           # integer pk
    personal_corpus,
    [PermissionTypes.CRUD]
)

Every other call site in the test suite passes the User object itself (self.owner). If set_permissions_for_obj_to_user doesn't accept a bare integer, this test will silently pass but do nothing (or fail at setup). Worth confirming the function accepts either form, or change it to self.owner.

---

4. Misleading error message for malformed document ids in AddDocumentsToCorpus / RemoveDocumentsFromCorpus

try:
    corpus_pk = from_global_id(corpus_id)[1]
    doc_pks = [int(from_global_id(doc_id)[1]) for doc_id in document_ids]
except Exception:
    return AddDocumentsToCorpus(message=not_found_msg, ok=False)

not_found_msg says "Corpus not found or you do not have permission to add documents to it." If one of the document ids is malformed base64, the caller gets a corpus-scoped error message pointing at the wrong thing. Not a security issue, but confusing UX. Either split the id-decode blocks so each returns a message scoped to the offending id, or make the shared message more generic ("Invalid request").

---

5. DeleteCorpusMutation DRFDeletion replacement — verify no lost behavior

DeleteCorpusMutation was refactored from DRFDeletion to a plain graphene.Mutation that calls obj.delete() directly. DRFDeletion.mutate typically delegates to a DRF DestroyModelMixin, which may invoke perform_destroy (and any pre/post-delete signal hooks wired to the DRF view layer rather than the Django model layer). If the Corpus model relies only on Django ORM signals (pre_delete/post_delete), this is fine. If there were any view-layer hooks, they'd be silently dropped. Worth a quick grep for any perform_destroy override on the Corpus resource.

---

Minor nit — CreateExtractIteration pk exception narrowing

extract_mutations.py:

try:
    source_pk = int(from_global_id(source_extract_id)[1])
except (TypeError, ValueError):
    return CreateExtractIteration(ok=False, message=source_not_found_msg)

This catches TypeError/ValueError but not binascii.Error (the exception from_global_id raises on malformed base64, which is a subclass of ValueError). ValueError should cover it in practice, but it's worth confirming by checking the Python standard library — binascii.Error inherits from ValueError in CPython, so this is fine. The note here is just for reviewers: the coverage is correct.

---

What's working well

• TypeVar on get_for_user_or_none preserves concrete model types through the gate — callers don't lose .fieldset, .is_public, etc.
• The fail-loud TypeError guardrail for unmanaged models (test_raises_on_model_without_visibility_manager) is the right call.
• UserProfileManager.visible_to_user superuser bypass is well-scoped (is_active=True filter retained) and clearly documented.
• The UpdateCorpusMutation embedder pre-check now properly routes through visible_to_user, closing the quiet "has documents" state leak.
• Frontend handleDeleteCorpus update correctly branches on data?.deleteCorpus?.ok.
• Test coverage for the helper is thorough (creator / public / missing pk / inaccessible pk / explicit grant / superuser / anonymous-private / anonymous-public / garbage pk / None pk).

[claude]

Code Review — Phase D: IDOR-safe get_for_user_or_none (#1658)

This is a well-executed security improvement. The helper design is clean, the TypeVar binding preserves concrete types for callers, the fail-loud TypeError guardrail on models without visible_to_user is exactly right, and the test suite comprehensively pins the contract. Below are issues worth addressing before merge, followed by minor observations.

---

Bugs / Issues

1. badge_mutations.py — badge_id and user_id from_global_id calls are unguarded

In AwardBadgeMutation.mutate, lines like:

badge_pk = from_global_id(badge_id)[1]
badge = get_for_user_or_none(Badge, badge_pk, awarder)
...
recipient_pk = from_global_id(user_id)[1]
recipient = get_for_user_or_none(User, recipient_pk, awarder)

…are NOT wrapped in try/except. A malformed badge_id or user_id (bad base64) falls through to the outer except Exception handler, which returns a generic message instead of the unified IDOR-safe envelope. The corpus_id arm in the same mutation IS wrapped — the inconsistency means all three ids in this mutation should be guarded uniformly.

2. corpus_mutations.py — StartCorpusFork, CreateCorpusAction, AddTemplateToCorpus have bare from_global_id calls

# StartCorpusFork (line ~568 after diff):
corpus_pk = from_global_id(corpus_id)[1]

# CreateCorpusAction (line ~819):
corpus_pk = from_global_id(corpus_id)[1]

# AddTemplateToCorpus (lines ~1437-1438):
corpus_pk = from_global_id(corpus_id)[1]
template_pk = from_global_id(template_id)[1]

These all sit inside outer try/except Exception blocks that will catch a binascii.Error, but the resulting message will be f"Error on ...: {e}" rather than the unified "not found or you don't have permission" response. The PR description claims from_global_id decode is pre-guarded at every migrated site, but these sites are the counter-examples.

3. label_mutations.py — RemoveLabelsFromLabelsetMutation conflates label/labelset decode errors

try:
    labelset_pk = from_global_id(labelset_id)[1]
    label_pks = [int(from_global_id(gid)[1]) for gid in label_ids]
except Exception:
    # returns not_found_msg: "LabelSet matching query does not exist."

A malformed label_id (bad base64 or non-integer) hits the same except branch and returns a labelset-not-found error message. The two decodes should be in separate try blocks with distinct messages (or at minimum a label-specific message).

4. test_permissioning.py — __test_only_permissioned_user_can_delete_public_corpus mutation query may be missing ok/message fields

The assertion block is updated to result["data"]["deleteCorpus"]["ok"], but the diff doesn't show the mutation string being updated to request those fields. If the query string for this helper is still deleteCorpus { ... } without ok/message, GraphQL will return an empty object and the assertion will KeyError. Please confirm the mutation query in that helper includes both fields (or show the full diff for it).

---

Design / Consistency Observations

5. Error message inconsistency across migrated sites

The unified envelope intent is slightly undermined by varying message wording:

• "Corpus not found or you don't have permission" (SetCorpusVisibility)
• "Corpus not found or you don't have permission to fork it." (StartCorpusFork)
• "Corpus not found" (ReEmbedCorpus — notably, this is the shortest and least explanatory)

The IDOR contract is preserved regardless, but consistent phrasing across all mutation sites would make frontend error handling simpler and reduce future confusion about which message format to pattern-match on.

6. UpdateCorpusMutation preferred-embedder pre-check — bare from_global_id in new code

if "preferred_embedder" in kwargs:
    corpus_global_id = kwargs.get("id")
    if corpus_global_id:
        corpus_pk = from_global_id(corpus_global_id)[1]   # unguarded
        corpus = get_for_user_or_none(Corpus, corpus_pk, info.context.user)

A malformed id here would propagate as an unhandled exception. Since the parent super().mutate() handles not-found, the inconsistency is low-risk, but it's the one bare call in a file that otherwise wraps all others.

7. Superuser bypass in UserProfileManager — behavioral change deserves a migration note

Adding is_superuser bypass to UserProfileManager.visible_to_user is the right call for the badge-awarding use case and it's well-documented. However, any query or resolver that previously called User.objects.visible_to_user(superuser_user) and expected to see only the superuser's own profile + public profiles now silently returns all active users. The permissioning guide update covers this, but a CHANGELOG "Changed" entry (in addition to "Security") would help operators auditing the upgrade.

---

Minor / Polish

• extract_mutations.py CreateExtractIteration uses except (TypeError, ValueError) while the PR consistently uses except Exception elsewhere for from_global_id wrapping. binascii.Error is a subclass of ValueError in Python 3 so this works, but documenting the subclass relationship in the comment would prevent future uncertainty.
• analysis_mutations.py DeleteAnalysisMutation lock check: the inline comment "sh@t happens" survived the rewrite — worth cleaning before merge since it's now surfaced in the canonical error-handling path.
• Stray # Use the ReEmbedCorpus mutation instead... reference in UpdateCorpusMutation's new comment block (line ~223 after diff) is a slightly awkward mid-comment instruction that reads like it was ported from an old inline comment. Minor.

---

Test Coverage

The 10 unit tests in test_get_for_user_or_none.py are thorough and well-named. Particularly good: test_raises_on_model_without_visibility_manager as a guardrail regression test. The badge test_non_superuser_cannot_award_to_private_profile_recipient is a strong pinning test for the new superuser bypass behaviour.

Gap: there's no test for malformed badge_id / user_id in AwardBadgeMutation (which is issue #1 above). A test exercising base64-garbage input to each from_global_id arm would both catch the bug and pin the regression.

---

Summary

Issues 1–3 are concrete divergences from the stated IDOR guarantee ("same unified response for malformed IDs") and should be fixed before merge. Issue 4 may cause a test failure. The rest are polish and consistency improvements. The core helper and the broad mutation migration are solid.

[claude]

Code Review — Phase D: IDOR-safe get_for_user_or_none helper

This is a solid, well-scoped security hardening PR. The core helper is cleanly designed, the documentation is thorough, and the breadth of mutation sites migrated is impressive. Comments below are in rough priority order.

---

1. Inconsistent from_global_id exception guarding — medium risk

The PR description explicitly notes that from_global_id can raise a bare binascii.Error before get_for_user_or_none is reached. Several sites guard the decode inside their own try/except Exception block, but a handful were missed. In those cases a malformed base64 input falls through to the outer generic except Exception handler and returns a generic failure message rather than the unified IDOR-safe envelope — creating a small but detectable behavioural difference.

badge_mutations.py — AwardBadgeMutation (~line 375):
badge_pk = from_global_id(badge_id)[1] and recipient_pk = from_global_id(user_id)[1] are unguarded. The corpus_id arm was correctly wrapped, which makes the inconsistency easy to see.

# corpus_id — guarded (good)
try:
    corpus_pk = from_global_id(corpus_id)[1]
except Exception:
    return AwardBadgeMutation(ok=False, message="Corpus not found", user_badge=None)

# badge_id / user_id — NOT guarded
badge_pk = from_global_id(badge_id)[1]   # binascii.Error leaks to outer handler
recipient_pk = from_global_id(user_id)[1]

corpus_mutations.py — StartCorpusFork, CreateCorpusAction, AddTemplateToCorpus:
corpus_pk = from_global_id(corpus_id)[1] is inside the outer try block but has no specific guard. A malformed corpus id bubbles to the generic outer except Exception rather than returning the unified message.

document_relationship_mutations.py — UpdateDocumentRelationship (the else branch that re-decodes corpus_id):
Same pattern — corpus_pk = from_global_id(corpus_id)[1] is unguarded inside the outer try.

Suggested fix: apply the same try/except Exception guard that the PR already uses for agent_mutations, conversation_mutations, analysis_mutations, and corpus_mutations (for ReEmbedCorpus / ToggleCorpusMemory) uniformly to the remaining sites.

---

2. RemoveLabelsFromLabelsetMutation — both IDs decoded in the same try block — low risk / UX

try:
    labelset_pk = from_global_id(labelset_id)[1]
    label_pks = [int(from_global_id(gid)[1]) for gid in label_ids]
except Exception:
    return RemoveLabelsFromLabelsetMutation(message=not_found_msg, ok=False)

not_found_msg is the LabelSet-scoped message. A caller who supplies a valid labelset_id but a malformed entry in label_ids will receive "LabelSet matching query does not exist." — factually incorrect and slightly confusing. Consider splitting into two guards (labelset decode → label decode) with a label-specific message, following the pattern used for corpus/document ids in AddDocumentsToCorpus.

---

3. DeleteCorpusMutation — superuser lock bypass not inherited from DRFDeletion

The new inline implementation checks:

if obj.user_lock is not None and info.context.user.id != obj.user_lock_id:
    return cls(ok=False, message="Specified object is locked by another user...")

This blocks any caller who is not the lock-holder — including superusers. If DRFDeletion.mutate previously allowed superusers to force-delete a user-locked corpus (via the user_can superuser shortcut that fires later), that path is now unreachable because the lock check runs first. If that was relied on for admin moderation, it's a regression. The comment says "mirrors DRFDeletion" but DRFDeletion's implementation isn't visible in the diff. Worth verifying the superuser case matches the old behaviour.

The DeleteAnalysisMutation has the same structure, so if the old behaviour was consistent between the two, this may be intentional — just worth confirming.

---

4. test_delete_personal_corpus_returns_unified_envelope — possible argument type mismatch

set_permissions_for_obj_to_user(
    self.owner.id,  # integer pk, not a User instance
    personal_corpus,
    [PermissionTypes.CRUD]
)

Most other call sites in the test suite pass self.owner (a User instance). If set_permissions_for_obj_to_user accepts both an integer and a User object this is fine; if it expects a User, the permission grant will silently fail and the superuser-path assertion will be testing against a corpus without the expected permissions. Worth a quick check against the function signature.

---

5. UpdateExtractMutation — removed explicit user_can(READ) check for corpus and fieldset

The old code explicitly called corpus.user_can(user, PermissionTypes.READ, ...) and fieldset.user_can(user, PermissionTypes.READ, ...) after the get() call. In the new code get_for_user_or_none enforces READ via visible_to_user, so the explicit user_can is redundant and its removal is correct. Just noting this as intentional, not a bug.

---

Positive observations

• Helper design is solid. The TypeVar bound preserves concrete attribute types across the gate, the None-pk early-return prevents a useless ORM round-trip, the visible_to_user guard (hasattr check + TypeError) fails loudly for models that forgot to implement the manager, and the ValueError/TypeError catch prevents 500s from garbage input. All 10 branches are pinned in test_get_for_user_or_none.py.

• DeleteCorpusMutation migration from DRFDeletion is well-executed. The new shape (ok/message) is consistent with every other Phase D mutation, the frontend handler in Corpuses.tsx is correctly updated to branch on data.deleteCorpus.ok, and two Playwright CT tests (ok=false and ok=true) exercise both paths.

• "READ as precondition for non-READ ops" is a meaningful security tightening (not just cosmetic). The three updated tests (test_user_with_permission_perm_can_change_visibility, test_non_owner_with_explicit_update_permission_can_create, test_remove_labels_allows_non_owner_with_explicit_update_permission) correctly document the new requirement with docstring explanations, so the behaviour change doesn't look like a test fix but rather a deliberate invariant.

• UserProfileManager.visible_to_user superuser bypass is minimal and well-justified. The new test test_non_superuser_cannot_award_to_private_profile_recipient pins both the blocked and allowed paths.

• Debug print() statements removed from UpdateExtractMutation — appreciated cleanup.

• CHANGELOG entry is well-structured and gives enough detail for a future auditor to trace the change.

---

Summary

The only items I'd flag before merge are #1 (inconsistent from_global_id guarding) — particularly in badge_mutations.py where the corpus arm was guarded but badge/user arms weren't, which looks like an incomplete pass — and a quick double-check on #3 (superuser lock bypass). Everything else is either low-priority or confirming intentional behaviour.

[claude]

Code Review — Phase D: IDOR-safe get_for_user_or_none (#1658)

Overview

This is a well-scoped, well-executed security hardening PR. The central abstraction — get_for_user_or_none — is the right design: a single generic helper that collapses the "doesn't exist" / "exists but inaccessible" distinction at the ORM boundary, with a TypeVar that preserves concrete model typing for callers. The systematic migration across 20+ mutation sites, the 10-test contract suite, and the CHANGELOG/docs coverage make this easy to verify and future-proof.

Findings below range from a confirmed bug (missing GraphQL query field update) to minor style nits.

---

Issues

1. DELETE_CORPUS GraphQL mutation query not in diff — possible missing field update (medium)

DeleteCorpusMutation migrated from DRFDeletion (which returns the model instance or null) to a graphene.Mutation returning { ok, message }. The frontend handleDeleteCorpus fix correctly reads data.deleteCorpus.ok / data.deleteCorpus.message, and the Playwright mock uses those fields — but there is no change to frontend/src/graphql/mutations.ts (or wherever DELETE_CORPUS is defined) in this diff.

If the DELETE_CORPUS document still selects only the corpus object (the old DRFDeletion shape) or nothing at all, Apollo will silently drop ok/message from the response and data?.deleteCorpus?.ok will be undefined (falsy), sending every delete through the error toast — including successful ones.

Please confirm the DELETE_CORPUS mutation document already requests ok and message, or add that selection as part of this PR.

---

2. Unguarded from_global_id in UpdateDocumentRelationship (low–medium)

Most migrated sites explicitly guard from_global_id in a try/except before calling the helper. The UpdateDocumentRelationship.mutate corpus branch (around the original line 335) does not:

corpus_pk = from_global_id(corpus_id)[1]   # no guard
corpus = get_for_user_or_none(Corpus, corpus_pk, info.context.user)

A malformed base64 corpus_id falls through to the outer except Exception, which returns a generic response — not the unified IDOR-safe message. The risk is low (the outer handler prevents a 500) but inconsistent with every other migrated site, and worth aligning.

---

3. PR number in source comment — will rot (style / CLAUDE.md)

corpus_mutations.py — AddDocumentsToCorpus and RemoveDocumentsFromCorpus both contain:

# (IDOR review on PR #1693)

CLAUDE.md says "Don't reference the current task, fix, or callers since those belong in the PR description and rot as the codebase evolves." The security rationale is already captured in the surrounding comment block; the PR reference can be dropped.

---

4. set_permissions_for_obj_to_user called with self.owner.id (integer) in one test (minor)

test_delete_personal_corpus_returns_unified_envelope in test_permissioned_querysets.py:

set_permissions_for_obj_to_user(
    self.owner.id,   # integer PK, not User object
    personal_corpus,
    [PermissionTypes.CRUD]
)

Every other call in the same test class passes the user object directly. Worth normalising to self.owner for readability.

---

Observations / Minor Points

• except Exception breadth on from_global_id: binascii.Error (the common failure mode) is a subclass of ValueError, so except ValueError would suffice in most cases. That said, graphql_relay's exception hierarchy is not part of any public contract, so the broader catch is defensible for security-critical code.

• Two from_global_id calls in one try block (AddTemplateToCorpus): A malformed template_id returns "You don't have permission to add templates to this corpus" — slightly misleading. Not a security issue (it's the safe path), but a separate guard per id would give cleaner UX. Low priority.

• UpdateCorpusMutation ternary is redundant: get_for_user_or_none already returns None when pk is None, so the if corpus_pk is not None else None ternary can be replaced with a direct call.

---

Strengths

• The TypeVar-generic signature preserves concrete attribute typing across the gate without extra casting.
• The fail-loud TypeError for models lacking visible_to_user is a good developer guardrail, and the test pins the contract.
• Reordering the creator vs. read_only gate in DeleteMultipleLabelMutation (creator check first) so non-creators never see "Cannot delete read-only labels" is exactly the right IDOR thinking.
• The superuser bypass on UserProfileManager.visible_to_user is minimal and precisely documented in both code and docs.
• Test coverage is thorough: all 10 contract branches for the helper, regression on the DeleteCorpusMutation migration, new badge+private-profile test, updated corpus/label tests.
• CHANGELOG and permissioning guide updated accurately.

The one item that needs resolution before merge is issue 1 (the DELETE_CORPUS query field coverage). Everything else is low/minor.