Authorize metadata-column deletion on parent corpus and add regression test

[JSv4]

Motivation

• Fix an authorization bypass where deleteMetadataColumn only checked Column-level permissions, allowing a column creator or direct grantee to delete a corpus-scoped metadata column without having DELETE permission on the parent corpus.
• Metadata columns are corpus-scoped and deleting a column cascades Datacell values, so destructive schema changes must be authorized against the containing corpus.

Description

• Change DeleteMetadataColumn.mutate in config/graphql/extract_mutations.py to load the Column with select_related("fieldset__corpus"), require the column to belong to a corpus metadata schema, and authorize the operation with BaseService.require_permission(corpus, user, PermissionTypes.DELETE, request=info.context) instead of authorizing on the child Column.
• Preserve the existing is_manual_entry guard so extract-managed columns remain undeletable via this surface.
• Update opencontractserver/tests/test_metadata_columns_graphql.py to attach the test Fieldset to the Corpus, add a column_creator test user and client, and add test_column_creator_without_corpus_delete_cannot_delete to assert that a column creator with direct CRUD on the child Column but without corpus DELETE cannot delete the corpus metadata column.

Testing

• Ran python -m py_compile config/graphql/extract_mutations.py opencontractserver/tests/test_metadata_columns_graphql.py and parsed both files with ast.parse, which succeeded.
• Ran git diff --check (no issues found).
• Attempted pytest for DeleteMetadataColumnTestCase, but the test run was blocked by an environment dependency error (ModuleNotFoundError: No module named 'django'), so full integration tests could not be executed in this environment.

---

Codex Task

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review

This PR correctly fixes a privilege escalation bug where a user with direct Column-level permissions could delete a corpus-scoped metadata column without holding corpus-level DELETE. The fix is logically sound and the new regression test covers the bypass scenario. Two issues surfaced during review.

---

1. Service-layer regression in config/graphql/extract_mutations.py (lines 377-381) — Medium

The old code used BaseService.get_or_none(Column, pk, user, request=info.context), which delegates to Column.objects.visible_to_user(user).filter(pk=pk).first() — a READ-gated lookup. The replacement is a raw ORM call with no user filter. Any authenticated user can now fetch any Column object and traverse its fieldset → corpus chain before a single permission check fires.

In practice, the corpus DELETE gate on line 393 prevents actual deletion. But:

• This violates CLAUDE.md rule 7: "code with a user context in config/graphql/ must reach models through the service layer." CreateMetadataColumn and UpdateMetadataColumn in the same file both use BaseService.get_or_none(Column, ...) correctly — this is an inconsistency that will confuse the next reader.
• The E001 system check only scans for visible_to_user / user_can / user_has_permission_for_obj by name, so this raw call passes CI silently.
• It removes defence-in-depth: an authenticated user can now resolve the Column ORM object and learn its fieldset→corpus chain before hitting any auth gate, rather than getting None immediately.

Suggested fix — restore the visibility filter and layer the corpus check on top:

column = (
    Column.objects.select_related("fieldset__corpus")
    .visible_to_user(user)
    .filter(pk=from_global_id(column_id)[1])
    .first()
)
if column is None:
    return DeleteMetadataColumn(ok=False, message=not_found_msg)

corpus = getattr(column.fieldset, "corpus", None)
if corpus is None:
    return DeleteMetadataColumn(ok=False, message=not_found_msg)

if BaseService.require_permission(corpus, user, PermissionTypes.DELETE, request=info.context):
    return DeleteMetadataColumn(ok=False, message=not_found_msg)

---

2. Missing positive test for the new authorization model — Low

test_creator_can_delete grants the owner both corpus CRUD and column CRUD, so it passes under both the old (column-level) and new (corpus-level) code paths and cannot distinguish between them.

test_column_creator_without_corpus_delete_cannot_delete correctly covers the negative case. But there is no test for the positive case: corpus DELETE alone (without column-level DELETE) should be sufficient to delete. A future refactor reverting the check to column-level would not be caught by the test suite.

Suggested addition:

def test_corpus_delete_without_column_delete_can_delete(self):
    corpus_only_user = User.objects.create_user(username="del-corpus-only", password="x")
    set_permissions_for_obj_to_user(corpus_only_user, self.corpus, [PermissionTypes.DELETE])
    # No Column-level permissions granted to corpus_only_user

    new_column = Column.objects.create(
        fieldset=self.fieldset, name="Corpus Only Col",
        output_type="str", is_manual_entry=True, creator=self.user,
    )
    client = Client(schema, context_value=TestContext(corpus_only_user))
    result = client.execute(
        self.MUTATION,
        variables={"columnId": to_global_id("ColumnType", new_column.pk)},
    )
    payload = result["data"]["deleteMetadataColumn"]
    self.assertTrue(payload["ok"], payload["message"])
    self.assertFalse(Column.objects.filter(pk=new_column.pk).exists())

---

Minor note

The getattr(column.fieldset, "corpus", None) guard on line 385 is safe and correct. Fieldset.corpus is a nullable forward OneToOneField; with select_related("fieldset__corpus"), Django performs a LEFT OUTER JOIN and returns None (not RelatedObjectDoesNotExist) when no corpus is linked. No change needed there.

[claude]

Review: Authorize metadata-column deletion on parent corpus (#2085)

This PR fixes a real authorization bypass: DeleteMetadataColumn previously gated on a direct Column-level DELETE grant, so a column creator or explicit grantee could cascade-delete all Datacell values without holding DELETE on the parent corpus. The fix (require corpus-level DELETE), the IDOR-safe unified error message, and the regression tests are all on the right track. Three issues worth addressing before merge:

---

1. UpdateMetadataColumn has the same bypass — unfixed (MEDIUM)

config/graphql/extract_mutations.py, line 315–317:

if column is None or BaseService.require_permission(
    column, user, PermissionTypes.UPDATE, request=info.context
):

UpdateMetadataColumn still gates on the child Column rather than the parent corpus. A user granted change_column directly on a corpus-metadata column — without holding corpus UPDATE — can rename it, swap validation_config, or change data_type. This is the same class of bypass that this PR fixes for DELETE; schema mutations on a corpus-scoped object should require corpus-level authorization. The fix is the same pattern you applied here: traverse column.fieldset.corpus and call require_permission(corpus, UPDATE).

---

2. Two extra lazy DB queries per call (MINOR)

config/graphql/extract_mutations.py, line 387:

corpus = getattr(column.fieldset, "corpus", None)

BaseService.get_or_none fetches Column bare (no select_related). The column.fieldset access fires a second DB round-trip, and fieldset.corpus fires a third. The PR description explicitly mentions using select_related("fieldset__corpus") but the implementation doesn't do it. Since BaseService.get_or_none doesn't accept select_related, one alternative is:

column = (
    BaseService.filter_visible(Column, user, request=info.context)
    .select_related("fieldset__corpus")
    .filter(pk=from_global_id(column_id)[1])
    .first()
)

Or fetch the FK IDs via column.fieldset_id / column.fieldset.corpus_id without triggering the full object load.

---

3. New corpus is None guard has no test (MINOR)

config/graphql/extract_mutations.py, lines 387–389:

corpus = getattr(column.fieldset, "corpus", None)
if corpus is None:
    return DeleteMetadataColumn(ok=False, message=not_found_msg)

This is a new code path that the PR's setUp change (adding corpus=self.corpus to self.fieldset) inadvertently makes impossible to hit with existing tests. A simple test that creates a Fieldset with corpus=None and tries to delete a column from it would pin this branch.

---

Nit: getattr default is never reached

column.fieldset is a non-nullable FK so it always returns a Fieldset instance, never None. Fieldset.corpus is a forward OneToOneField(null=True) whose descriptor already returns None when corpus_id IS NULL. The getattr(..., None) default is therefore unreachable — column.fieldset.corpus is cleaner and equally correct.

---

Summary: Finding #1 is the most important — UpdateMetadataColumn is the mirror image of the bug this PR fixes and should be addressed in the same change. Findings #2 and #3 are low-risk but worth a quick pass before the PR lands.

[claude]

Review: Authorize metadata-column mutations on parent corpus

Solid fix. I reviewed the full diff plus the final state of config/graphql/extract_mutations.py and opencontractserver/tests/test_metadata_columns_graphql.py.

What's good

• The core fix is correct. Column/Fieldset use standalone django-guardian permission tables (no built-in corpus inheritance), so a direct creator/grantee DELETE/UPDATE on the child Column really could bypass corpus-level authorization for a corpus-scoped metadata schema. Re-authorizing against column.fieldset.corpus for both DeleteMetadataColumn and UpdateMetadataColumn closes that gap correctly.
• _get_metadata_column_with_corpus is good DRY — extracting the shared lookup means the corpus-scoped gate can't silently regress to column-scoped in only one of the two mutations, and select_related("fieldset__corpus") avoids extra round-trips.
• IDOR-safety is preserved. BaseService.filter_visible(Column, user).filter(pk=pk).first() is equivalent to get_for_user_or_none's READ fast-path (opencontractserver/shared/services/conventions.py does exactly visible_to_user(user).filter(pk=pk).first() for READ), so swapping get_or_none for the new helper in the last commit didn't weaken the READ gate. The unified "not found or no permission" response is preserved.
• Test coverage is thorough: both the vulnerable scenario (column-level grant without corpus-level grant → refused) and the inverse (corpus-level grant without column-level grant → succeeds) are pinned for both mutations, plus the nullable fieldset.corpus edge case. This is exactly the "prove the old code would fail this test" style of regression test the project wants.

Items worth addressing

1. Changelog fragment is now stale (changelog.d/2085-metadata-column-delete-authz.security.md):

   • It says the column lookup "stays READ-gated through the service layer (BaseService.get_or_none(Column, …))" — the final commit replaced that call with _get_metadata_column_with_corpus (BaseService.filter_visible(...)), so the fragment describes code that no longer exists.
   • More importantly, it only documents the DeleteMetadataColumn fix. The last commit ("Authorize UpdateMetadataColumn against parent corpus, not child Column") fixes the identical bug class in UpdateMetadataColumn — a user with direct change_column but no corpus UPDATE could rename/retype a metadata column — but there's no changelog fragment for it. Per CLAUDE.md's changelog rules, that's a distinct production security fix and should get its own bullet (or the existing fragment should be updated to cover both mutations).

2. Related, out-of-scope gap: UpdateFieldset (config/graphql/extract_mutations.py:644-690) still authorizes solely against require_permission(fieldset, user, UPDATE) — it never checks fieldset.corpus. Since a corpus's metadata schema is a Fieldset (see CreateMetadataColumn, which sets fieldset.corpus = corpus), a user holding a direct Fieldset-level UPDATE grant but lacking corpus UPDATE can still rename/redescribe a corpus's metadata schema via updateFieldset — the same privilege-escalation class this PR just fixed for Column, one level up the object graph. Worth a follow-up applying the same fieldset.corpus-gate pattern here (or noting explicitly why it's out of scope, if plain fieldsets and metadata-schema fieldsets are meant to be treated differently).

3. Behavior-change note (probably fine, but worth confirming): the helper now treats "fieldset has no linked corpus" the same as "not found" for both mutations. All columns created via CreateMetadataColumn are guaranteed to have fieldset.corpus set, so this shouldn't affect normal usage — just flagging in case legacy/manually-created data has a manual-entry column whose fieldset predates the Fieldset.corpus field, which would now become permanently un-updatable/undeletable via these two mutations.

None of these block the fix itself — the authorization change and its regression tests look correct and are a clear net security improvement. Items 1–2 are the ones I'd want resolved (or explicitly deferred) before merge; item 3 is just an FYI.