Tighten Admin Validations + IDOR Prevention

[JSv4]

Summary

Comprehensive security audit of Auth0 integration and analyzer callback handling. Twelve real fixes across three layers: defense-in-depth allowlist for is_superuser JWT elevation, IDOR-safe error messages in GraphQL mutations, and SHA-256 hashing of analyzer callback tokens to prevent credential leakage via database reads.

Key Changes

Auth0 Privilege Escalation Prevention

• Defense-in-depth allowlist for is_superuser: Added AUTH0_SUPERUSER_SUB_ALLOWLIST environment variable. JWT claim sync now refuses to elevate is_superuser to True unless the user's Auth0 sub (Django username) appears in the allowlist, regardless of verified token claims. Blocks misconfigured tenants sourcing admin claims from user-writable user_metadata.
• Reduced admin claims cache TTL: Changed ADMIN_CLAIMS_CACHE_TTL from 300 to 30 seconds for tighter privilege revocation SLA.
• System check for empty allowlist: Added opencontractserver/users/checks.py to warn at startup when Auth0 is enabled but allowlist is empty.
• Documentation hardening: Added security warnings in docs/configuration/authentication.md about sourcing claims from app_metadata (not user_metadata) and detailed allowlist setup instructions.

IDOR-Safe GraphQL Mutations

• Unified error messages: AddRelationship, CreateMetadataColumn, and UpdateMetadataColumn mutations now return identical error messages for both "does not exist" and "no permission" cases, preventing enumeration attacks.
• Annotation filtering via visible_to_user: Source and target annotations filtered through Annotation.objects.visible_to_user(user) with count validation to catch both missing and unauthorized IDs without echoing them back.
• Corpus/Column visibility checks: Corpus and Column lookups now use visible_to_user() querysets; non-existent and inaccessible resources collapse into the same error branch.
• Generic error logging: ORM and constraint exceptions logged server-side with logger.exception() but return generic "Error creating X" messages to clients.
• Bad global ID handling: Unparseable global IDs treated as indistinguishable from not-found to keep IDOR surface flat.

Analyzer Callback Token Security

• SHA-256 hashing of callback tokens: Replaced plaintext UUID callback_token field with callback_token_hash (CharField, max_length=64). Database never stores plaintext; a DB read alone cannot let an attacker forge analyzer callbacks.
• Token rotation on submission: Analysis.rotate_callback_token() generates fresh secrets.token_urlsafe(32) plaintext on each submission, invalidating any in-flight callbacks bound to previous tokens.
• Constant-time verification: Analysis.verify_callback_token(candidate) uses hmac.compare_digest() to prevent timing leaks.
• Migration 0021: Backfills existing plaintext tokens as SHA-256 hashes so in-flight analyzers continue working; reverse migration is lossy (generates fresh UUIDs) as hashes cannot be recovered.
• Redacted logging: Analyzer submission payloads logged with callback token redacted to avoid plaintext leakage in logs.

Supporting Changes

• Updated opencontractserver/analyzer/views.py to call verify_callback_token() instead of direct string comparison.
• Updated test fixtures in test_analyzers.py and test_security_hardening.py to mint plaintext tokens via rotate_callback_token() before submission.
• Added AUTH0_CREATE_NEW_USERS environment variable (default True) to control auto-provisioning of new Auth0 users.
• Documented User.email as informational (no unique=True constraint); User.username (Auth0 sub) is the only identity field.
• Added JWKS stale-grace window (_JWKS_STALE_GRACE = 3600 seconds) to bound how long a compromised key

https://claude.ai/code/session_01DEQ8YPfGpzKHr1VviKjdpX

[claude]

Code Review: Security Hardening — Auth0 Permissioning Audit & Callback Token Hashing

This is a well-scoped, defence-in-depth security PR. The changes are correct in intent and largely well-implemented. Below is a detailed breakdown.

---

Overall Assessment

Positive

• All three attack surfaces (privilege escalation, IDOR enumeration, credential storage) are addressed coherently and consistently.
• Constant-time comparison (hmac.compare_digest) is correctly used in verify_callback_token.
• Migration is correct: forward path hashes existing tokens so in-flight analyzers continue working; reverse path is documented as intentionally lossy.
• Unified error messages across IDOR-sensitive paths are applied consistently and the logic is clearly commented.
• System check (users.W001) surfaces operator misconfiguration at startup rather than silently failing at runtime.
• Redacted logging of callback tokens is a nice follow-through detail.

---

Issues & Suggestions

Medium — Document permission gap in AddRelationship (annotation_mutations.py)

The new code validates that source/target annotations are visible to the user and that the user has CREATE on the corpus. However document_pk is used in Relationship.objects.create(document_id=document_pk) without any visibility check on the document itself.

If a document belongs to a different corpus the user cannot see, they could still create a relationship referencing it by guessing the ID — as long as the corpus they have CREATE on accepts it. Recommend adding:

try:
    Document.objects.visible_to_user(user).get(pk=document_pk)
except Document.DoesNotExist:
    return AddRelationship(ok=False, relationship=None, message=not_found_msg)

Low — rotate_callback_token() requires caller to remember save() (models.py)

The method mutates self.callback_token_hash but defers save() to the caller. Both existing call-sites do this correctly, but it is an easy footgun for future callers. Consider always saving inside the method when the instance has a PK, or at minimum rename to generate_callback_token() to signal the object is in a dirty state:

def rotate_callback_token(self) -> str:
    plaintext = secrets.token_urlsafe(32)
    self.callback_token_hash = self._hash_callback_token(plaintext)
    if self.pk:
        self.save(update_fields=["callback_token_hash"])
    return plaintext

Low — System check not tagged with Tags.security (checks.py)

@register() without a tag works, but @register(Tags.security) would categorize it correctly in manage.py check --tag security and CI security-focused check runs:

from django.core.checks import Tags, Warning, register

@register(Tags.security)
def check_auth0_superuser_allowlist(...):

Low — N+1 queries in migration backfill (0021_hash_analysis_callback_token.py)

backfill_token_hashes issues one UPDATE per analysis row. For large tables this is slow in a migration window. A bulk_update approach in batches of ~500 would be more robust at scale. Minor for this table size, but worth improving.

Minor — logger.exception redundancy in broad-except blocks

In the new exception handlers, e.g.:

logger.exception("Error creating relationship: %s", e)

logger.exception already appends the full traceback including the exception message, so passing e as a format argument is redundant noise. Just:

logger.exception("Error creating relationship")

Minor — _JWKS_STALE_GRACE is not configurable via settings

_JWKS_STALE_GRACE = 3600 follows the same module-constant pattern as _JWKS_CACHE_TTL, which is fine. Environments with slower Auth0 failover might want to tune this, but a settings.JWKS_STALE_GRACE_SECONDS override is not required for this PR — just flagging for a potential follow-up.

---

Correctness Checks

• count() != len(set(source_pks)) correctly handles duplicate IDs in the input without echoing them back. ✓
• hmac.compare_digest operates on two hex strings of equal length (64 chars each), so the constant-time guarantee holds. ✓
• _can_serve_stale is only ever called inside _jwks_cache_lock, satisfying its documented precondition. ✓
• callback_token_hash CharField(max_length=64) is exactly right for SHA-256 hex output. ✓
• ADMIN_CLAIMS_CACHE_TTL = 30 correctly propagates to the constant in opencontractserver/constants/auth.py and all doc references. ✓
• The operator migration note in CHANGELOG ("populate AUTH0_SUPERUSER_SUB_ALLOWLIST BEFORE deploy") is present and prominent. ✓

---

Test Coverage

• test_correct_token_accepted correctly replaces the old UUID-based test and exercises the rotate_callback_token → verify_callback_token round-trip. ✓
• The existing test_same_error_for_missing_vs_wrong_token test covers the 403-equality assertion. ✓
• Gap: No tests for the allowlist enforcement in sync_admin_claims_from_payload (the F1 fix). A test asserting is_superuser stays False when the sub is absent from the allowlist, and flips to True when added, would be valuable. The blocked-elevation warning log path is also untested.
• Gap: No tests for the users.W001 system check itself. Django system checks are straightforward to unit-test with call_checks() or the check registry.

---

Summary

The core security fixes (F1–F10) are correctly implemented and well-motivated. The document-visibility gap in AddRelationship and the rotate_callback_token save footgun are the two items I would want resolved before merge. The test coverage gaps for the allowlist enforcement and the system check are lower priority but would increase confidence in the F1 fix long-term.

[codecov]

Codecov Report

❌ Patch coverage is 93.87755% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
opencontractserver/users/checks.py	90.90%	2 Missing ⚠️
opencontractserver/users/tasks.py	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

Code Review: Tighten Admin Validations + IDOR Prevention

This is a well-structured, defense-in-depth security PR. The changes are clearly motivated, consistently applied, and come with good test coverage. Below is a thorough review.

---

Overview

The PR addresses three distinct security layers:

1. Auth0 privilege escalation — allowlist gating for is_superuser JWT claim sync
2. IDOR oracle elimination — unified error messages and visible_to_user() filtering in GraphQL mutations
3. Callback token credential storage — SHA-256 hashing of Analysis.callback_token at rest

All three areas contain real security improvements and the changes are generally correct. The following notes cover bugs, edge cases, and minor style issues.

---

Findings

1. _can_serve_stale reads outside the lock — potential race condition

File: config/graphql_auth0_auth/utils.py

_can_serve_stale reads _jwks_cache["data"] and _jwks_cache["expires_at"] without holding _jwks_cache_lock. It is only called from inside the with _jwks_cache_lock: block in _get_cached_jwks, so this is correct in the current call graph — but the docstring says "Caller must already hold _jwks_cache_lock" in a way that's easy to miss. Consider either asserting the lock or making it a private helper with a more prominent guard comment. As written it is safe but fragile.

2. ADMIN_CLAIMS_CACHE_TTL = 30 may be too aggressive for high-traffic deployments

File: opencontractserver/constants/auth.py

Dropping from 300 s to 30 s means every JWT-authenticated API call triggers a cache miss every 30 seconds per user and fires the claim-sync ORM write path. For a deployment with many concurrent users this is a 10× increase in write pressure on the auth path. The security motivation is sound, but consider making this configurable via an env var (like AUTH0_ADMIN_CLAIMS_CACHE_TTL) so operators can tune the trade-off without a code change.

3. visible_to_user + explicit user_has_permission_for_obj double-check in AddRelationship

File: config/graphql/annotation_mutations.py, lines ~506-519

After filtering the corpus through Corpus.objects.visible_to_user(user), the code then calls user_has_permission_for_obj(user, corpus, PermissionTypes.CREATE, ...) separately. This is redundant: visible_to_user already filters for read visibility, and the explicit permission check correctly adds the CREATE gate — but it uses the already-fetched corpus object, which is fine. The only concern is that visible_to_user may return a corpus where the user has READ but not CREATE, and the subsequent check handles this correctly. This is actually the right pattern per the CLAUDE.md permissioning guide. No bug here — just noting it's intentional.

4. Migration 0021 reverse path uses save() instead of update()

File: opencontractserver/analyzer/migrations/0021_hash_analysis_callback_token.py, line ~42

for analysis in Analysis.objects.all():
    analysis.callback_token = uuid.uuid4()
    analysis.save(update_fields=["callback_token"])

save() in a migration RunPython block uses the historical model, which may trigger signals or other side effects. Prefer the same Analysis.objects.filter(pk=...).update(callback_token=uuid.uuid4()) pattern used in the forward path. However, since this is explicitly documented as a lossy/best-effort reverse and no production deployment should rely on it, this is low severity.

5. backfill_token_hashes iterates row-by-row — may be slow on large tables

File: opencontractserver/analyzer/migrations/0021_hash_analysis_callback_token.py, lines ~26-31

for pk, raw_token in Analysis.objects.values_list("id", "callback_token"):
    if raw_token is None:
        continue
    digest = hashlib.sha256(str(raw_token).encode("utf-8")).hexdigest()
    Analysis.objects.filter(pk=pk).update(callback_token_hash=digest)

This fires one UPDATE per row. For tables with thousands of Analysis rows this could be very slow. Since SHA-256 cannot be expressed in a single SQL expression portably, the per-row approach is acceptable — but consider batching with bulk_update or chunking the queryset (iterator(chunk_size=500)) to reduce memory pressure during migration.

6. verify_callback_token returns False for empty callback_token_hash — correct but undocumented edge case

File: opencontractserver/analyzer/models.py

The new rotate_callback_token doesn't call save(), and a newly-created Analysis has callback_token_hash = "". The verify_callback_token guard on empty hash is correct. However, the analyzer submission path in opencontractserver/utils/analyzer.py now correctly calls rotate_callback_token() + save() — but only at submission time. If run_analysis() is called and then the process crashes before save(), the hash field is updated in memory but not persisted. This is pre-existing fragility (the old UUID field had the same issue) and not introduced by this PR, just worth noting.

7. AUTH0_CREATE_NEW_USERS setting is referenced in docs but not in base.py

File: docs/configuration/authentication.md, config/settings/base.py

The documentation Step 7 and env-var table mention AUTH0_CREATE_NEW_USERS, but there's no corresponding env.bool("AUTH0_CREATE_NEW_USERS", default=True) in config/settings/base.py. If the Auth0 backend doesn't currently read this setting, the doc is aspirational rather than functional. Is this intended to be a future follow-up, or was the settings entry accidentally omitted?

8. System check check_auth0_superuser_allowlist fires even in test environments

File: opencontractserver/users/checks.py

The check fires whenever USE_AUTH0=True. In CI runs that set USE_AUTH0=True for Auth0-path tests, this will produce a users.W001 warning on every test run unless the allowlist is populated. Consider adding a settings.TESTING guard or using Django's silenced_system_checks in test settings. This is minor but could be surprising.

9. AddRelationship — relationship_label_pk is no longer validated

File: config/graphql/annotation_mutations.py

The old code passed relationship_label_pk directly to Relationship.objects.create(relationship_label_id=...). If the relationship_label_id global ID is invalid or doesn't exist, the create() call will raise an IntegrityError or ValueError caught by the broad except Exception block and returned as a generic "Error creating relationship." message. This is acceptable from a security standpoint (no IDOR leak), but it's a slight UX regression compared to a cleaner validation. This is a reasonable trade-off given the security goals.

10. test_correct_token_accepted test name change is correct

File: opencontractserver/tests/test_security_hardening.py

The old test name test_correct_token_uuid_type_accepted was accurate for the old UUID field. The new name test_correct_token_accepted is correct. Good cleanup.

---

Positive Highlights

• Unified error messages are applied consistently across AddRelationship, CreateMetadataColumn, UpdateMetadataColumn, and Datacell mutations. The pattern of defining not_found_msg at the top of each mutation and reusing it is clean and prevents accidental divergence.
• hmac.compare_digest in verify_callback_token is exactly the right tool for constant-time comparison here.
• secrets.token_urlsafe(32) gives 256 bits of entropy — stronger than the previous UUID v4 (122 bits). Good upgrade.
• Token rotation on submit invalidates old tokens with no extra state. Elegant design.
• Migration commentary is thorough and correctly warns operators about the deployment ordering requirement.
• TestAddRelationshipDocumentIDOR covers both the attack path and the happy path in the same class. The use of assertIn("not found", data["message"].lower()) instead of an exact string match is appropriately loose.
• CHANGELOG entry is detailed with file paths, fix numbering, and the explicit list of verified false positives. This level of audit transparency is excellent.
• Operator action required callout in the PR description and changelog is critical and well-placed.

---

Summary

This is a high-quality security PR. The identified issues are mostly minor (race condition documentation, one missing settings entry, migration performance). The three core security improvements — allowlist gating for superuser elevation, IDOR-safe mutations, and hashed callback tokens — are all correctly implemented with tests.

Recommended action before merge:

1. Clarify whether AUTH0_CREATE_NEW_USERS is fully wired up or a documentation placeholder (issue Bump mypy from 0.971 to 0.982 #7 above).
2. Confirm CI test settings won't generate noisy users.W001 warnings (issue Bump crispy-bootstrap5 from 0.6 to 0.7 #8).
3. Consider making ADMIN_CLAIMS_CACHE_TTL env-configurable (issue Bump traefik from v2.8.7 to v2.9.1 in /compose/production/traefik #2) if write pressure is a concern.

The deployment note about populating AUTH0_SUPERUSER_SUB_ALLOWLIST before deploy is critical and is already well-documented in both the PR description and docs — good.

[claude]

Code Review: Tighten Admin Validations + IDOR Prevention

This is a well-structured security PR covering three real vulnerability classes. The fixes are targeted, the migration is safe, and the test coverage for the new paths is thorough. A few items worth addressing before merge:

---

Issues

1. relationship_label_pk is never visibility-checked (IDOR gap in AddRelationship)

After the refactor, source_annotations, target_annotations, corpus, and document are all routed through visible_to_user(). But relationship_label_pk is decoded from the global ID and passed directly into Relationship.objects.create(relationship_label_id=relationship_label_pk) without any existence or visibility check.

# config/graphql/annotation_mutations.py ~line 480
relationship_label_pk = from_global_id(relationship_label_id)[1]
# ... no AnnotationLabel.objects.visible_to_user(user).filter(pk=relationship_label_pk).exists() check

An unauthenticated or low-privilege caller can enumerate AnnotationLabel IDs by sending requests and distinguishing between "relationship created" vs. the broad except returning a generic error (i.e., IntegrityError vs. no error). The commit message explicitly says document_id was an IDOR oracle "without this check" — the same logic applies to labels. Suggest adding:

if not AnnotationLabel.objects.visible_to_user(user).filter(pk=relationship_label_pk).exists():
    return AddRelationship(ok=False, relationship=None, message=not_found_msg)

---

2. source_pks / target_pks are strings in id__in filter

from_global_id(graphene_id)[1] returns a string (e.g. "42"). The id__in=source_pks filter works via Django's implicit type coercion, but set(source_pks) is then comparing string-deduped values against source_annotations.count() which uses integer PKs under the hood. This is fine in practice because Django coerces correctly and both sides get the same deduplication, but the implicit behavior is fragile. Using int(pk) on the parsed IDs would make the intent explicit and prevent surprises if the model ever changes to a non-integer PK.

---

3. Test gap: F7 Datacell IDOR fixes are untested

ApproveDatacell, RejectDatacell, and EditDatacell now catch Datacell.DoesNotExist explicitly and use generic error messages. No new tests were added for these paths. Given the established pattern in test_security_hardening.py, it would be consistent to add a test verifying that accessing a datacell belonging to another user returns the generic "not found" message rather than a permission-distinguishable error.

---

4. _can_serve_stale assertion is not thread-owner-specific

assert (
    _jwks_cache_lock.locked()
), "_can_serve_stale must be called while holding _jwks_cache_lock"

threading.Lock.locked() returns True if the lock is held by any thread, not necessarily the calling thread. Thread B holding the lock while Thread A mistakenly calls _can_serve_stale() without acquiring it would pass the assert. The docstring describes this limitation well, but if this helper ever escapes the current call sites the guard won't catch the misuse. Minor, but worth documenting or using _jwks_cache_lock.acquire(blocking=False) + immediate release as a more robust check.

---

Observations / Questions

AUTH0_ADMIN_CLAIMS_CACHE_TTL vs. AUTH0_ADMIN_CLAIMS_CACHE_TTL naming collision: base.py introduces AUTH0_ADMIN_CLAIMS_CACHE_TTL as a Django setting, but _sync_admin_claims_cached reads it via getattr(settings, "AUTH0_ADMIN_CLAIMS_CACHE_TTL", ...). The test overrides it as AUTH0_ADMIN_CLAIMS_CACHE_TTL=7 — this is consistent. Looks correct, just confirming intentional naming.

AUTH0_JWT dict introduction: auth0_settings reads AUTH0_JWT as a flat dict and overlays it onto DEFAULTS. Previously AUTH0_JWT was not set in base.py at all, so AUTH0_CREATE_NEW_USERS always resolved to the hardcoded True in DEFAULTS. Now it correctly picks up the env var. Any override_settings(AUTH0_JWT={...}) in tests would completely replace the dict (no deep-merge), but a search of the test suite confirms no existing tests override it — clean.

10x TTL reduction (300s → 30s) performance impact: Understood tradeoff — the privilege revocation window matters. Worth a note in the operator documentation (or the existing troubleshooting section) that high-traffic APIs will see roughly 10x more Django cache writes on the /graphql/ path. Not blocking, just worth calling out for load-tested deployments.

Migration 0021 performance on large tables: The per-row UPDATE loop is memory-safe but will be slow for deployments with many Analysis rows. On a large production table this migration could lock rows for a non-trivial window. Not a correctness issue since Analysis rows are typically low-cardinality, but worth flagging in the migration's module docstring.

---

What's Done Well

• Constant-time verification via hmac.compare_digest in verify_callback_token is correct and appreciated.
• Token rotation on submission (invalidating in-flight callbacks) is a good design choice, and the redacted logging line prevents plaintext leakage.
• The allowlist system check (users.W001) silenced in test.py is exactly the right pattern.
• TestAddRelationshipDocumentIDOR is a well-structured IDOR test that covers both the attack path and the happy path — sets a good template for the Datacell gap above.
• _can_serve_stale docstring is unusually thorough about the lock invariant, which is the right level of care for a subtly shared helper.

[claude]

Code Review: Tighten Admin Validations + IDOR Prevention

This is a high-quality security hardening PR. The changes are well-scoped, consistently applied, and the test suite is thorough. Several issues worth raising are below.

---

What's Working Well

• Callback token hashing (models.py, utils/analyzer.py): rotate_callback_token() / verify_callback_token() with hmac.compare_digest is exactly the right pattern. CharField(max_length=64) is precisely sized for SHA-256 hex. Redacted logging at submit time is a nice detail.
• IDOR-safe mutations: The visible_to_user() + unified error message pattern is applied consistently across AddRelationship, CreateMetadataColumn, and UpdateMetadataColumn. The document_id visibility check in AddRelationship closes the residual IDOR surface correctly.
• Allowlist gate (config/graphql_auth0_auth/utils.py): The allowlist check fires only on is_superuser=True elevation and correctly lets demotion pass through without requiring a sub to be listed.
• System check (users/checks.py): Surfacing the empty-allowlist case at startup is far better than a silent runtime failure. Silencing it in test settings via SILENCED_SYSTEM_CHECKS is clean.
• Migration 0021: Forward path correctly hashes existing tokens so in-flight analyzers survive the upgrade. The reverse path is intentionally lossy and clearly documented.
• Test coverage: TestAuth0SuperuserAllowlist, TestAuth0SuperuserAllowlistSystemCheck, TestAnalysisCallbackTokenHelpers, TestCanServeStale, and TestAddRelationshipDocumentIDOR collectively pin the new security invariants well.

---

Issues

Low — assert disabled by python -O in security-critical helper

_can_serve_stale (config/graphql_auth0_auth/utils.py) uses:

assert _jwks_cache_lock.locked(), "_can_serve_stale must be called while holding _jwks_cache_lock"

assert is silently stripped when Python runs with -O or -OO. For a lock-ordering invariant in a security-critical module, prefer an explicit runtime guard:

if not _jwks_cache_lock.locked():
    raise RuntimeError("_can_serve_stale must be called while holding _jwks_cache_lock")

Most Django production deployments don't use -O, so this is low priority, but it's better practice for security code.

Low — rotate_callback_token() defers save() to the caller (models.py:269-284)

Both current call sites call save(update_fields=["callback_token_hash"]) correctly, but a future caller could easily forget. Since this is a credential rotation, the failure mode (stale hash in DB, plaintext lost) is silent and hard to debug. Either auto-save when the instance has a PK, or add a prominent warning in the docstring:

def rotate_callback_token(self) -> str:
    plaintext = secrets.token_urlsafe(32)
    self.callback_token_hash = self._hash_callback_token(plaintext)
    if self.pk:
        self.save(update_fields=["callback_token_hash"])
    return plaintext

Low — System check not tagged with Tags.security (users/checks.py:11)

from django.core.checks import Tags, Warning, register

@register(Tags.security)
def check_auth0_superuser_allowlist(...):

This makes it show up in manage.py check --tag security and in security-focused CI check pipelines.

Low — Migration backfill issues one UPDATE per row (0021_hash_analysis_callback_token.py:26-33)

backfill_token_hashes iterates with iterator(chunk_size=500) (good for memory) but still issues one SQL UPDATE per row. For tables with thousands of rows this will be slow in a migration window. A bulk_update or batched UPDATE … SET … WHERE id IN (…) approach would be more robust:

from django.db import connection

def backfill_token_hashes(apps, schema_editor):
    Analysis = apps.get_model("analyzer", "Analysis")
    batch = []
    for pk, raw_token in Analysis.objects.exclude(callback_token__isnull=True).values_list("id", "callback_token").iterator(chunk_size=500):
        digest = hashlib.sha256(str(raw_token).encode("utf-8")).hexdigest()
        batch.append((pk, digest))
        if len(batch) == 500:
            _flush_batch(Analysis, batch)
            batch.clear()
    if batch:
        _flush_batch(Analysis, batch)

def _flush_batch(Analysis, batch):
    for pk, digest in batch:
        Analysis.objects.filter(pk=pk).update(callback_token_hash=digest)

(True bulk_update requires the historical model to support it; the loop approach with chunked flushes is still an improvement if you add batch flushing.)

Minor — Redundant exception argument to logger.exception

logger.exception() automatically appends the current exception's traceback and message. Passing e as a format argument doubles the exception text in the log:

# Current (redundant):
logger.exception("Error creating relationship: %s", e)

# Clean:
logger.exception("Error creating relationship")

This applies to all new logger.exception call sites in annotation_mutations.py and extract_mutations.py.

Minor — AUTH0_JWT dict assignment in base.py may clobber existing keys

AUTH0_JWT = {
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

If any other code path (local/production settings overrides, third-party package) had previously added keys to AUTH0_JWT, this assignment silently loses them. If this is a pure addition (the key didn't exist before), it's fine — but worth a quick grep to confirm no other settings file sets AUTH0_JWT.

---

Correctness Spot-Checks

Check	Result
count() != len(set(source_pks)) handles duplicate IDs in input without echoing them	✓
hmac.compare_digest compares two equal-length hex strings (64 chars each) — constant-time guarantee holds	✓
Migration forward path hashes before dropping column; reverse is documented as lossy	✓
ADMIN_CLAIMS_CACHE_TTL = 30 propagates to auth.py, _sync_admin_claims_cached, and all doc references	✓
_can_serve_stale is only invoked inside with _jwks_cache_lock: in _get_cached_jwks	✓
Allowlist gate only blocks elevation (is_superuser_claim=True); demotion is unaffected	✓
SILENCED_SYSTEM_CHECKS = ["users.W001"] in test settings prevents noise; allowlist tests use override_settings and call the check function directly	✓

---

Summary

No blocking issues. The assert → RuntimeError change and the @register(Tags.security) tag are the most actionable items. Everything else is low/minor polish. Ready to merge once these are addressed or consciously accepted.

[claude]

Code Review — PR #1641: Tighten Admin Validations + IDOR Prevention

This is a solid, well-scoped security hardening PR. The changes are targeted, the rationale is clearly documented, and the test coverage is genuinely good. Below are observations ranging from blocking concerns to minor style notes.

---

Overview

Three distinct security layers are addressed:

1. Auth0 privilege escalation — allowlist gate for is_superuser JWT claim sync
2. GraphQL IDOR oracles — unified error messages + visible_to_user querysets
3. Analyzer callback credential storage — SHA-256 hashing of callback tokens

All three areas have correct core logic. The issues below are mostly about edge cases, defence-in-depth gaps, and a few correctness risks.

---

Potential Bugs / Correctness Issues

1. _can_serve_stale assert races under real (non-test) concurrency

File: config/graphql_auth0_auth/utils.py:_can_serve_stale

assert (
    _jwks_cache_lock.locked()
), "_can_serve_stale must be called while holding _jwks_cache_lock"

threading.Lock.locked() returns True if any thread holds the lock, not necessarily the calling thread. A different thread acquiring the lock while this thread is running a (hypothetical) non-lock-holding call would cause the assert to pass incorrectly. This is fine for today's single call-site (under with _jwks_cache_lock:), but the docstring says "callers MUST already hold the lock" — the assert doesn't actually enforce that. Consider replacing the assert with a comment, or using a threading.RLock and tracking the owner thread explicitly. At minimum, the docstring should note this limitation so a future refactor doesn't misplace trust in the assert.

2. Migration backfill uses individual-row UPDATE inside a Python loop

File: opencontractserver/analyzer/migrations/0021_hash_analysis_callback_token.py:backfill_token_hashes

for pk, raw_token in queryset.iterator(chunk_size=500):
    digest = hashlib.sha256(str(raw_token).encode("utf-8")).hexdigest()
    Analysis.objects.filter(pk=pk).update(callback_token_hash=digest)

iterator(chunk_size=500) keeps the fetch cursor server-side, but each iteration still fires a separate UPDATE query. On a table with thousands of rows this is N round-trips. Consider collecting digests into a batch and using bulk_update:

to_update = []
for pk, raw_token in queryset.iterator(chunk_size=500):
    obj = Analysis(pk=pk)
    obj.callback_token_hash = hashlib.sha256(str(raw_token).encode()).hexdigest()
    to_update.append(obj)
    if len(to_update) >= 500:
        Analysis.objects.bulk_update(to_update, ["callback_token_hash"])
        to_update.clear()
if to_update:
    Analysis.objects.bulk_update(to_update, ["callback_token_hash"])

This is low-risk to leave as-is for most deployments, but flagging it since migration runtimes matter.

3. AddRelationship — relationship_label visibility is unchecked

File: config/graphql/annotation_mutations.py

The PR closes the annotation, corpus, and document IDOR holes, but relationship_label_pk is used directly in Relationship.objects.create(relationship_label_id=relationship_label_pk, ...) without a visibility check. An attacker who can create relationships in their own corpus could probe private AnnotationLabel IDs by supplying them as relationship_label_id and observing whether the create succeeds or fails with a DB constraint vs. the generic error. This is a narrower surface than the fixed paths, but worth addressing for consistency.

4. AUTH0_JWT dict in config/settings/base.py — potential key collision

File: config/settings/base.py

AUTH0_JWT = {
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

If AUTH0_JWT was already defined earlier in the same settings module (or in an inherited base), this assignment silently replaces the entire dict rather than merging. Check whether any existing AUTH0_JWT keys (e.g. JWT_AUTH_HEADER_PREFIX, audience settings) are expected — if so, use .update() or dict-merge syntax.

---

Security Observations

5. Allowlist applies to is_superuser only — is_staff IDOR still present

File: config/graphql_auth0_auth/utils.py:sync_admin_claims_from_payload

The PR description notes "The allowlist applies to is_superuser only — is_staff is still gated by the JWT claim alone." This is a deliberate decision and documented, so it's not a bug. But is_staff grants Django admin access, which can still be dangerous if sourced from user-writable user_metadata. Consider documenting this explicitly in the code comment at the allowlist gate (not just in the docs), or applying a second, lower-severity allowlist for is_staff in a follow-up.

6. verify_callback_token does not guard against callback_token_hash being set to an empty string by a write path other than rotate_callback_token

File: opencontractserver/analyzer/models.py

def verify_callback_token(self, candidate: str | None) -> bool:
    if not candidate or not self.callback_token_hash:
        return False

The guard is correct: an empty hash always returns False. But the CharField(blank=True, default="") means any code that calls analysis.callback_token_hash = "" and saves will silently disable the token, without requiring rotate_callback_token. The field is editable=False on the model level, but Django admin / direct ORM writes bypass this. A pre_save signal or a custom clean() that refuses to blank a non-empty hash would close this. Minor, but worth noting.

---

Code Quality / Style

7. TestAuth0SuperuserAllowlistSystemCheck bypasses SILENCED_SYSTEM_CHECKS

File: opencontractserver/tests/test_admin_auth.py

The test_check_warns_when_allowlist_empty test calls check_auth0_superuser_allowlist(None) directly rather than going through Django's check runner, so SILENCED_SYSTEM_CHECKS = ["users.W001"] in test.py has no effect here — the test works correctly. But the comment in test.py ("Individual tests that exercise the allowlist behaviour override AUTH0_SUPERUSER_SUB_ALLOWLIST explicitly") doesn't account for the direct-call path. A brief cross-reference comment would remove ambiguity for future readers.

8. test_admin_claims_cached_noop_when_auth0_disabled assertion is underspecified

File: opencontractserver/tests/test_admin_auth.py

The test verifies that _sync_admin_claims_cached does nothing when USE_AUTH0=False, but it uses override_settings(USE_AUTH0=False) when the early-return guard in _sync_admin_claims_cached actually checks settings.USE_AUTH0. Confirm that _sync_admin_claims_cached truly has a USE_AUTH0 guard — if the guard is upstream in the middleware/decorator rather than in the function itself, the test may be passing for the wrong reason.

9. f-string + %s logging mixed style

File: opencontractserver/utils/analyzer.py:266-269

logger.info(
    f"submit_corpus_documents_to_analyzer() - submit data to gremlin: "
    f"{redacted_submission}"
)

The codebase uses %-style lazy formatting elsewhere in this same PR (logger.exception("Error creating relationship: %s", e)). This f-string forces dict-to-string serialization at call time regardless of log level. Prefer logger.info("... gremlin: %s", redacted_submission) for consistency and to avoid the eager format cost.

10. restore_plaintext_tokens fires one UPDATE per row — same as forward path

File: opencontractserver/analyzer/migrations/0021_hash_analysis_callback_token.py:restore_plaintext_tokens

The reverse migration generates and sets a UUID per row in a loop. Same N-round-trips concern as item #2 above. bulk_update would help, though reverse migrations are rarely performance-critical.

11. logger.exception vs logger.error — exception context included unintentionally?

File: config/graphql/annotation_mutations.py:554, config/graphql/extract_mutations.py

logger.exception(...) logs the full stack trace at ERROR level. This is intentional and good for debugging. But for the ORM except Exception as e: blocks that are expected to trigger on bad user input (e.g. a malformed global ID that slipped past the early parse try/except), a full traceback in logs on every bad request may be noisy. Consider logger.error(...) with exc_info=True only for truly unexpected paths.

---

Summary

#	Severity	Area	Action
1	Low	_can_serve_stale lock assert	Clarify or remove misleading assert
2	Low	Migration backfill N+1	Use bulk_update for production-scale safety
3	Medium	AddRelationship relationship_label visibility	Add visible_to_user check for consistency
4	Medium	AUTH0_JWT dict assignment	Verify no existing keys are silently dropped
5	Info	is_staff allowlist gap	Document in code or defer to follow-up
6	Low	callback_token_hash = "" write bypass	Consider clean() guard
7–11	Info	Style / test clarity	Optional polish

Overall this is well-thought-out work. The IDOR fixes, token hashing, and allowlist mechanism are all correct in their core logic. Items 3 and 4 are the most worth addressing before merge.

[claude]

Code Review — PR #1641: Tighten Admin Validations + IDOR Prevention

Overall this is a well-scoped, defense-in-depth security PR with clear rationale for each fix. The IDOR-safe error messaging, token hashing, and allowlist gate are all solid. A few concerns worth addressing before merge, ordered by severity.

---

Critical / High

1. threading.Lock.locked() does not prove the calling thread holds the lock

_can_serve_stale contains:

assert _jwks_cache_lock.locked(), "_can_serve_stale must be called while holding _jwks_cache_lock"

Lock.locked() returns True if any thread holds the lock, not specifically the calling thread. If Thread A holds it and Thread B erroneously calls _can_serve_stale, the assertion passes anyway, providing false security. Flip it to a RuntimeError so it can't be silently disabled with -O, and accept that perfect thread-identity checking requires a threading.local sentinel:

if not _jwks_cache_lock.locked():
    raise RuntimeError("_can_serve_stale must be called while holding _jwks_cache_lock")

This is better than assert (not strippable) even though it still can't definitively prove this thread holds the lock. The real fix is to restructure so _can_serve_stale only exists inside _get_cached_jwks's with block.

2. ApproveDatacell / RejectDatacell lack ownership checks

The PR refactors the exception handling in these mutations but the underlying queryset is:

obj = Datacell.objects.get(pk=pk)

There is no creator=info.context.user guard (unlike EditDatacell, which does have it). Any authenticated user can approve or reject any datacell. The IDOR message fix is correct, but the underlying authorization gap remains. Either add visible_to_user(user) filtering or add the creator guard — and add a test that a non-owner gets an appropriate error rather than silently mutating another user's data.

---

Medium

3. AUTH0_SUPERUSER_SUB_ALLOWLIST empty default is a silent deploy-time footgun

The docs and CHANGELOG do call this out, but the behavior is asymmetric: the system check (users.W001) emits a warning at startup — warnings do not block startup or CI. An operator who deploys without reading the changelog, dismisses the warning, and hits production will find that all Auth0 superusers are demoted silently on their next request. Consider:

• Promoting users.W001 to an Error-level check (not Warning) in non-DEBUG settings, or
• Adding a --check step to the deployment runbook that fails the deploy if users.W001 is present.

At minimum the hint message should say explicitly: "Existing superusers will be demoted on their NEXT REQUEST, not just on next login."

4. N+1 queries in AddRelationship

source_annotations = Annotation.objects.visible_to_user(user).filter(id__in=source_pks)
target_annotations = Annotation.objects.visible_to_user(user).filter(id__in=target_pks)
if source_annotations.count() != len(set(source_pks)) or target_annotations.count() != len(set(target_pks)):
    ...
relationship.target_annotations.set(target_annotations)
relationship.source_annotations.set(source_annotations)

This is four database round-trips (two .count(), then two QuerySet re-evaluations inside .set()). The querysets are lazy, so .count() and .set() are each separate SQL calls. Consider materializing once via list() or values_list('id', flat=True) and comparing lengths:

visible_source_ids = set(Annotation.objects.visible_to_user(user).filter(id__in=source_pks).values_list('id', flat=True))
if visible_source_ids != set(source_pks): ...

Then pass the already-fetched IDs to .set().

5. AUTH0_JWT dict assignment may stomp an existing key

# config/settings/base.py
AUTH0_JWT = {
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

If an AUTH0_JWT dict is already defined earlier in the settings (e.g. in another settings include or in the existing base.py body that wasn't shown in the diff), this assignment would silently drop all other keys in that dict. Confirm the existing AUTH0_JWT contents and use .update() or dict unpacking rather than reassignment if there are existing keys.

6. AUTH0_CREATE_NEW_USERS consumption not visible in diff

The PR description says this is "wired into AUTH0_JWT settings so get_auth0_user_from_token actually honours operator-disabled provisioning," but the diff for utils.py only shows changes to _get_cached_jwks, _can_serve_stale, and sync_admin_claims_from_payload. The actual consumption in get_auth0_user_from_token isn't visible in the diff. If the function wasn't updated to read settings.AUTH0_JWT["AUTH0_CREATE_NEW_USERS"], the new env var would be a no-op. Please verify (or add a test that setting AUTH0_CREATE_NEW_USERS=False actually prevents user creation).

---

Low / Nitpick

7. Import inside settings module body

# config/settings/base.py
from opencontractserver.constants.auth import (
    ADMIN_CLAIMS_CACHE_TTL as _DEFAULT_ADMIN_CLAIMS_CACHE_TTL,
)
AUTH0_ADMIN_CLAIMS_CACHE_TTL = env.int("AUTH0_ADMIN_CLAIMS_CACHE_TTL", default=_DEFAULT_ADMIN_CLAIMS_CACHE_TTL)

This is valid Python but it creates a settings-time import of application code, which can trigger circular imports if Django hasn't fully initialized. The safer approach is to hard-code the default (default=30) in the env declaration and keep the constant as the source of truth via a comment.

8. Rotation-on-every-submit vs. pre-existing in-flight callbacks

run_analysis calls rotate_callback_token() every time an analysis is submitted. The PR description acknowledges this "invalidates any in-flight callbacks bound to previous tokens." If an analyzer worker takes a long time and the analysis is re-submitted (e.g. retry logic), the re-submission invalidates the first worker's token. This may be intentional but is worth a short inline comment.

9. Test assertion self.assertTrue(result) for sync_admin_claims_from_payload

In test_blocks_superuser_when_allowlist_empty and test_blocks_superuser_for_user_not_in_allowlist:

result = sync_admin_claims_from_payload(self.user, payload)
self.assertTrue(result)

If the user starts as is_superuser=False and the allowlist overrides the claim back to False, no DB save happens and needs_save=False may be returned. Whether self.assertTrue(False) would fail the test depends on what the function actually returns in this path. If this test passes, the assertion is coincidentally passing (perhaps needs_save is True for a different field), but the assertion intent is unclear. Suggest asserting the final DB state instead:

self.assertFalse(self.user.is_superuser)  # already done below — the assertTrue is redundant or misleading

10. SHA-256 token hashing without salt (informational)

Using unsalted SHA-256 for the callback token is acceptable here because secrets.token_urlsafe(32) produces ~256 bits of entropy — far beyond rainbow-table range. No action needed, but a one-line comment would help future maintainers not "fix" it by adding bcrypt.

---

Positives Worth Calling Out

• The backfill migration is well-structured: iterator(chunk_size=500) bounds memory, the reverse migration is documented as intentionally lossy, and the comment explains why hashes can't be recovered.
• The verify_callback_token / rotate_callback_token split on the model is clean — separates "generate credentials" from "verify credentials" and keeps the DB boundary clear.
• redacted_submission for logging avoids plaintext token leakage in log aggregators — good operational hygiene.
• The comprehensive test suite for new paths (JWKS stale grace, allowlist gate, token helper edges) significantly raises the regression barrier.
• The CHANGELOG entry with F-numbered findings and explicit "Verified false positives" section is unusually thorough and useful for reviewers.

[claude]

Code Review — PR #1641: Tighten Admin Validations + IDOR Prevention

This is a well-scoped security hardening PR covering three independent attack surfaces. The overall approach is sound and the changelog is unusually thorough. Below are findings organized by severity.

---

High Priority

1. Non-integer PKs from from_global_id can cause unhandled exceptions

In annotation_mutations.py, after the global-ID parsing try/except block, source_pks may contain non-integer strings (e.g., "not-an-int") if from_global_id succeeds but returns a non-numeric PK. The subsequent .count() call is outside any try/except:

# This .count() triggers a DB query — Django will try to cast each pk to BigInt.
# A non-integer pk (e.g. "not-an-int") raises ValueError or DataError here,
# which is currently unhandled.
if source_annotations.count() != len(set(source_pks)) ...

The test test_garbage_global_ids_yield_not_found_message implicitly acknowledges this — it uses if relation_data is not None: to guard against the case where a GraphQL error is returned instead of a mutation response. Consider adding integer validation of source_pks/target_pks/*_pk after parsing, or wrapping the entire annotation-filtering block in a try/except that maps to not_found_msg.

2. AnnotationLabel.objects.visible_to_user(user) — confirm the manager exists

The new AddRelationship code adds:

if (
    not AnnotationLabel.objects.visible_to_user(user)
    .filter(pk=relationship_label_pk)
    .exists()
):

This is a good security improvement, but AnnotationLabel is not referenced in the CLAUDE.md's list of models with visible_to_user. If AnnotationLabel inherits from BaseOCModel and has guardian permission tables this is fine, but if it doesn't, this will either raise AttributeError at runtime or silently return an empty queryset (blocking all relationship creation). The test suite covering the test_inaccessible_relationship_label_yields_not_found case should verify the happy path too — confirm test_allows_relationship_on_visible_document also exercises the label check path end-to-end.

---

Medium Priority

3. AUTH0_JWT assignment may overwrite existing keys

In config/settings/base.py:

AUTH0_JWT = {
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

If AUTH0_JWT was previously a multi-key dict (or is set in a derived settings file), this overwrites the entire dict instead of merging. Consider:

AUTH0_JWT = {
    **getattr(locals(), "AUTH0_JWT", {}),
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

Or verify AUTH0_JWT is not defined anywhere else in the settings hierarchy.

4. AUTH0_CREATE_NEW_USERS may not be consumed

The PR description and CHANGELOG say AUTH0_CREATE_NEW_USERS is "wired into get_auth0_user_from_token", but the diff doesn't show any change to get_auth0_user_from_token reading from settings.AUTH0_JWT["AUTH0_CREATE_NEW_USERS"]. If the consuming code isn't updated, the setting is documented but inert — operators would set it expecting an effect that doesn't exist.

5. Migration lacks atomic = False for large-table safety

0021_hash_analysis_callback_token.py runs the Python-level backfill inside the default transaction wrapper. On deployments with many Analysis rows, the long-running transaction (even with iterator(chunk_size=500)) can hold locks that block other writes. Consider adding atomic = False to the Migration class if the table is expected to be large in production.

---

Low Priority

6. _can_serve_stale thread-safety is fragile by design

The docstring correctly warns that callers must hold _jwks_cache_lock. This pattern is safe today (single call site inside the with _jwks_cache_lock: block) but is a footgun for future refactors — the function reads shared mutable state and has no runtime enforcement. A future refactor could silently introduce a race. One alternative: pass the cache snapshot as parameters (data, expires_at) rather than reading global state, making the contract explicit in the function signature.

7. Minor N+1 in AddRelationship

source_annotations = Annotation.objects.visible_to_user(user).filter(id__in=source_pks)
...
if source_annotations.count() != len(set(source_pks)):  # COUNT query
    ...
relationship.source_annotations.set(source_annotations)  # SELECT query again

.count() and .set() each hit the DB separately on the same queryset. For typical relationship sizes this is negligible, but calling .values_list("id", flat=True) once and comparing sets would avoid the extra round-trip. Not a blocker.

8. ADMIN_CLAIMS_CACHE_TTL constant vs. env-var naming

opencontractserver/constants/auth.py defines ADMIN_CLAIMS_CACHE_TTL = 30. The settings entry is AUTH0_ADMIN_CLAIMS_CACHE_TTL. The constant file no longer matches the env-var name (missing AUTH0_ prefix). The constant is still used as the default in the getattr fallback, so behavior is correct, but the naming mismatch may confuse future operators looking at the constants file.

---

Positive Observations

• The unified not_found_msg pattern in AddRelationship, CreateMetadataColumn, and UpdateMetadataColumn is clean and consistent with the CLAUDE.md IDOR prevention guidance.
• Analysis.verify_callback_token using hmac.compare_digest is correct; moving hmac import to the model and out of the view is a good encapsulation.
• The migration backfill batching (bulk_update in chunks of 500) and the iterator(chunk_size=500) on the reverse path are both correct and memory-safe.
• _JWKS_STALE_GRACE bound on the stale-cache fallback is a genuine improvement — the original code allowed indefinitely stale keys after an Auth0 outage.
• The system check (users.W001) surfacing the empty allowlist at startup is a good operator-facing safeguard, and silencing it in test.py with a comment explaining how TestAuth0SuperuserAllowlistSystemCheck bypasses it is the right approach.
• The CHANGELOG and docs/configuration/authentication.md updates are detailed and accurate — including the "operator action required before deploy" callout.
• Test coverage is solid: rotation/verify edges, empty-hash fresh rows, stale-grace boundary conditions, and IDOR scenarios all have direct tests.

---

Summary: Items 1 and 2 should be addressed before merge. Items 3–5 are medium-risk but worth a follow-up. The security model described in the PR is correct and the implementation is generally high quality.

[claude]

Code Review: PR #1641 — Tighten Admin Validations + IDOR Prevention

This is a well-scoped security hardening PR addressing real vulnerabilities across three layers. The implementation is careful and the test coverage is comprehensive. Below are findings ranging from bugs to minor style notes.

---

Overview

The PR ships twelve security fixes:

• Auth0: Defense-in-depth allowlist for is_superuser JWT elevation + tighter 30s cache TTL + system check
• IDOR: Unified error messages in AddRelationship, CreateMetadataColumn, UpdateMetadataColumn, and Datacell mutations
• Credential storage: SHA-256 hashing of Analysis.callback_token (never stores plaintext)

The overall approach is correct and the new patterns are consistent with the project's visible_to_user conventions documented in CLAUDE.md.

---

Bugs / Correctness Issues

1. ApproveDatacell / RejectDatacell still fetch without owner filter — test assertion is misleading

ApproveDatacell.mutate and RejectDatacell.mutate both call:

obj = Datacell.objects.get(pk=pk)

There is no creator filter here (unlike EditDatacell, which correctly uses get(pk=pk, creator=info.context.user)). Any authenticated user can therefore approve or reject any datacell by its ID.

The new test test_approve_datacell_owned_by_other_user_returns_not_found contains an incorrect doc-comment:

# (the queryset filters by creator, so DoesNotExist is raised)

The queryset does not filter by creator for these two mutations. This test will pass only if there is an unrelated mechanism (e.g. a manager-level filter) that happens to block the cross-user lookup — which would be fragile and unintentional. Either the test is wrong, or this pre-existing IDOR in ApproveDatacell/RejectDatacell is being mischaracterized as fixed. Worth an explicit look before merge.

Suggested fix: Add creator=info.context.user to both Datacell.objects.get(pk=pk) calls, matching EditDatacell's pattern, and update the error messages accordingly.

2. test_garbage_global_ids_yield_not_found_message — soft assertion can silently pass

data = result.get("data") or {}
relation_data = data.get("addRelationship")
if relation_data is not None:
    self.assertFalse(relation_data["ok"])

If relation_data is None (e.g. the mutation raises a GraphQL-level error instead of returning ok=False), the test body is never entered and the test trivially passes. This should be an unconditional assertion.

---

Security Notes

3. SHA-256 without a server-side secret key

Analysis._hash_callback_token uses plain SHA-256:

hashlib.sha256(plaintext.encode("utf-8")).hexdigest()

This is fine given that secrets.token_urlsafe(32) produces ~256 bits of entropy, making offline cracking infeasible. If the project ever moves to shorter tokens, switching to hmac.new(SECRET_KEY, plaintext, sha256).hexdigest() would add a second factor against database-dump attacks. Not a blocker at the current token length.

4. _can_serve_stale — threading caveat is well-documented but worth a follow-up

The docstring correctly calls out that Python's threading.Lock cannot assert ownership, and that threading.RLock would be needed for a runtime guarantee. As written, the single call site inside _get_cached_jwks is safe. A brief inline comment at that call site (e.g. # _can_serve_stale requires _jwks_cache_lock to be held) would prevent a future caller from invoking it outside the lock. Low severity but a clear footgun given the docstring's own warning.

---

Design / Style

5. AUTH0_JWT dict defined as a single-key assignment in base.py

AUTH0_JWT = {
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

If AUTH0_JWT was already defined earlier in the same settings class (or in a mixin) with other keys, this assignment silently drops those keys. A safer pattern when extending an existing dict is:

AUTH0_JWT = {
    **getattr(cls, "AUTH0_JWT", {}),
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

Should be verified that this is a net-new key, not an override of an existing dict.

6. from opencontractserver.constants.auth import ... inside the settings class body

from opencontractserver.constants.auth import (
    ADMIN_CLAIMS_CACHE_TTL as _DEFAULT_ADMIN_CLAIMS_CACHE_TTL,
)
AUTH0_ADMIN_CLAIMS_CACHE_TTL = env.int(...)

This is a module-level import inside what appears to be a Base settings class body. This is unusual and could produce circular-import issues during Django startup if opencontractserver imports from django.conf.settings at load time. Consider hoisting this import to the file's top-level imports or computing the default inline.

7. Double permission check in CreateMetadataColumn / UpdateMetadataColumn

After visible_to_user(user).get(pk=...), there is a redundant explicit user_has_permission_for_obj(user, corpus, PermissionTypes.UPDATE) call. visible_to_user only grants READ access, so the UPDATE check is logically necessary. However, when visible_to_user returns the object but the user lacks UPDATE permission, the returned not_found_msg says "you do not have permission to update it" — which is accurate but subtly reveals that the object exists. This is a deliberate trade-off (avoid a second round-trip to the DB for the error text), and the unified message still prevents enumeration. Worth a brief comment explaining that the message is intentionally vague despite the two-step logic.

---

Test Coverage

The test suite additions are thorough:

• TestAuth0SuperuserAllowlist covers the empty-allowlist, excluded-sub, and demotion paths.
• TestAnalysisCallbackTokenHelpers covers all edges including rotation invalidation, unsaved-instance behavior, and None/empty inputs.
• TestCanServeStale mirrors the production lock acquisition shape correctly.
• TestAddRelationshipDocumentIDOR covers both the happy path and the blocked IDOR path.

The only gap is the ApproveDatacell/RejectDatacell ownership issue noted in #1 above.

---

Summary

Severity	Finding
Bug	ApproveDatacell/RejectDatacell lack creator filter; new test's doc-comment is incorrect (#1)
Minor	test_garbage_global_ids has a soft assertion that can silently pass (#2)
Low	AUTH0_JWT dict assignment may silently discard existing keys (#5)
Low	Import inside settings class body risks circular imports (#6)
Nit	_can_serve_stale call site should note lock-must-be-held invariant (#4)

The Auth0 allowlist, JWKS stale-grace, IDOR-safe messages, and callback token hashing are all solid. Resolving finding #1 (and ideally #2) before merge would close the remaining IDOR surface in the datacell mutations.

[claude]

Code Review: Tighten Admin Validations + IDOR Prevention

This is a well-scoped security hardening PR covering three distinct attack surfaces. The overall design is sound. Notes below are organized by severity.

Potential Bugs / Must-Fix

1. AUTH0_JWT dict assignment may clobber existing keys

The new code in config/settings/base.py does a plain dict assignment for AUTH0_JWT. If AUTH0_JWT was already defined with other keys in a parent settings file, this silently drops them. Either confirm the variable is brand-new to this codebase, or use a merge pattern. This cannot be ruled out from the diff alone.

2. AnnotationLabel.objects.visible_to_user(user) — manager may not exist

config/graphql/annotation_mutations.py now calls AnnotationLabel.objects.visible_to_user(user).filter(...). The visible_to_user method is provided by BaseVisibilityManager. If AnnotationLabel does not inherit BaseOCModel (or lacks that manager), this raises AttributeError at runtime and produces an unhandled 500 — worse than the original permission leak. Please confirm AnnotationLabel has the manager, or fall back to an explicit guardian check.

3. test_approve_datacell_owned_by_other_user_returns_not_found — comment vs. code mismatch

The test docstring says "the queryset filters by creator" as the reason DoesNotExist is raised, but the diff for ApproveDatacell and RejectDatacell only shows exception-clause changes. If those mutations do not include creator=info.context.user in the get() call, any authenticated user can approve or reject any datacell. Please show the full get() call in context, or add an inline comment confirming the creator filter exists.

---

Design / Architecture Observations

4. Double visibility check in CreateMetadataColumn / UpdateMetadataColumn

The pattern (visible_to_user().get() followed by an explicit user_has_permission_for_obj UPDATE check) is correct and intentional: the first avoids leaking whether the corpus exists at all, the second enforces the UPDATE permission specifically. A brief inline comment explaining the two-layer design would prevent a future reader from collapsing it into just the explicit permission check, which would reintroduce the enumeration oracle.

5. Allowlist demotion is a silent breaking change on upgrade

With an empty (default) allowlist, every existing Auth0-synced superuser is demoted on their next API request within 30 seconds — no grace period, no dry-run mode. The users.W001 check fires at startup but is a Warning. Consider elevating to Critical when User.objects.filter(is_superuser=True).exists(), or adding a management command listing affected users before deploy.

6. _can_serve_stale threading footgun

The docstring correctly notes that no runtime assert can guarantee the caller holds _jwks_cache_lock. Since the helper is only called from inside "with _jwks_cache_lock:", this is safe today. Adding a short comment at the call site would make the invariant harder to violate during future refactors.

7. _JWKS_STALE_GRACE = 3600 seconds

A 1-hour stale window means a compromised key that Auth0 has rotated can still verify tokens for up to one hour if Auth0 goes down immediately after the rotation. This is a documented availability-vs-security trade-off worth a one-line operator note for deployments with aggressive key-rotation policies.

---

Minor Notes

8. Migration concurrency window — AddField with default="" followed by RunPython backfill means rows briefly have callback_token_hash="". Safe because verify_callback_token returns False for an empty hash.

9. Nested try/except in CreateMetadataColumn — valid and intentional; a one-line comment explaining the two-layer structure would help readers who might try to flatten it.

10. Module-level import inside settings class body (config/settings/base.py) — the import of ADMIN_CLAIMS_CACHE_TTL runs at settings-parse time; verify opencontractserver is on sys.path before app initialization in all deployment environments.

---

What is Well Done

• Token security primitives: SHA-256 plus hmac.compare_digest plus secrets.token_urlsafe(32) are all correct choices. Raw SHA-256 (not bcrypt/argon2) is appropriate here because the tokens carry approximately 256 bits of entropy.
• Rotation on submit: minting a fresh plaintext in run_analysis and redacting the log line closes the log-leakage path cleanly.
• Migration efficiency: iterator(chunk_size=500) plus bulk_update avoids the N-round-trip footgun common in security backfill migrations. The reverse migration's "intentionally lossy" comment is the right posture.
• IDOR count-comparison pattern: visible_to_user().filter(id__in=...).count() != len(set(pks)) catches both missing and unauthorized IDs without echoing them. A clean, reusable idiom.
• Test coverage: TestAnalysisCallbackTokenHelpers, TestAuth0SuperuserAllowlist, TestAddRelationshipDocumentIDOR, and TestCanServeStale cover the edge cases well, including unsaved-instance behavior and stale-grace boundary conditions.

---

Summary: Items 1 and 2 need confirmation before merge (potential runtime breakage). Item 3 needs clarification. Everything else is advisory. The security intent is solid throughout.

[claude]

Code Review: PR #1641 — Tighten Admin Validations + IDOR Prevention

This is a well-scoped security hardening PR that addresses real vulnerabilities across three distinct layers. The fixes are correctly designed, the test coverage is strong, and the documentation is thorough. Below are detailed findings.

---

Overview

The PR delivers:

1. Auth0 privilege-escalation prevention via AUTH0_SUPERUSER_SUB_ALLOWLIST + system check
2. IDOR-safe error messages across six GraphQL mutations
3. SHA-256 hashing of analyzer callback tokens stored at rest

All twelve stated fixes are verified in the diff. No false fixes were introduced.

---

Positives

• Constant-time token comparison (hmac.compare_digest) is correctly preserved in verify_callback_token.
• secrets.token_urlsafe(32) is the right primitive — ~256 bits, URL-safe, cryptographically secure.
• Migration backfill is well-structured: batched via bulk_update, atomic by default, and the reverse migration is honestly documented as lossy. The iterator(chunk_size=500) in the reverse path is a nice addition.
• Test coverage is excellent. TestAnalysisCallbackTokenHelpers, TestAuth0SuperuserAllowlist, TestAuth0SuperuserAllowlistSystemCheck, TestCanServeStale, and TestAddRelationshipDocumentIDOR are all solid. The unsaved-instance test for rotate_callback_token is a good edge-case catch.
• System check severity escalation (W001 → E001 when existing superusers exist) is exactly the right behavior and will save operators from silent lockouts.
• IDOR surface flattening is consistently applied: not-found and no-permission paths converge to the same message and the same log level (server-side logger.exception, generic string to caller).

---

Issues

1. AUTH0_JWT dict in config/settings/base.py — possible clobber

AUTH0_JWT = {
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

If AUTH0_JWT is already defined earlier in base.py (or in a parent settings file that base.py inherits from), this assignment silently drops all existing keys. A safer pattern is:

AUTH0_JWT = {
    **getattr(globals(), "AUTH0_JWT", {}),
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

or, once the full existing dict is confirmed empty, a comment documenting this is the first and only assignment. Worth verifying this doesn't shadow any existing AUTH0_JWT config in graphql_auth0_auth.

2. Inline import inside settings class body (style)

# config/settings/base.py (inside the class body)
from opencontractserver.constants.auth import (
    ADMIN_CLAIMS_CACHE_TTL as _DEFAULT_ADMIN_CLAIMS_CACHE_TTL,
)

Imports at class-body scope in settings files are unusual and may confuse tools that introspect settings (e.g., django-environ, django-stubs). The constant could be imported at the module top level instead and referenced in the class body without the alias.

3. TOCTOU window in AddRelationship annotation count check

# annotation_mutations.py
source_annotations = Annotation.objects.visible_to_user(user).filter(id__in=source_pks)
target_annotations = Annotation.objects.visible_to_user(user).filter(id__in=target_pks)
if source_annotations.count() != len(set(source_pks)) or ...:
    return ...
# ... later ...
relationship.source_annotations.set(source_annotations)

There's a read-then-use gap: the count() queries and the .set() call are not inside a single transaction.atomic() block. A concurrent annotation deletion between the count check and the set() would create a relationship with missing endpoints. This is not a security issue — it can only happen under race conditions with legitimate deletions — but wrapping the entire creation block in with transaction.atomic() would make the invariant explicit.

4. _can_serve_stale lock assumption is documented but not enforced

The docstring correctly warns that the helper must be called under _jwks_cache_lock, and honestly acknowledges a runtime assertion isn't feasible with threading.Lock. The PR description notes this is known. Considered for future: a refactor to threading.RLock with an owner-thread assertion would harden this, but is not a blocker for this PR given the single call site.

5. AnnotationLabel.visible_to_user — confirm manager exists

# annotation_mutations.py
if not AnnotationLabel.objects.visible_to_user(user).filter(pk=relationship_label_pk).exists():

This assumes AnnotationLabel has a visible_to_user manager (from BaseVisibilityManager). The test for the inaccessible-label path passes, which confirms the manager exists, but it's worth a quick grep to confirm AnnotationLabel extends BaseOCModel and therefore inherits the manager — especially since the migration path does not touch AnnotationLabel.

---

Minor Nits

• test_reject_datacell_not_found and test_edit_datacell_not_found lack docstrings while test_approve_datacell_not_found has one. Trivial but inconsistent.
• The _base_variables helper in TestAddRelationshipDocumentIDOR is a nice DRY touch; consider using a similar pattern in TestAnalysisCallbackTokenHelpers.setUp to reduce the FK setup boilerplate.
• SILENCED_SYSTEM_CHECKS = ["users.W001", "users.E001"] in test.py — silencing E001 makes sense (test DB has superusers from migrations), and the comment explaining why is correct.

---

Summary

Approve with the AUTH0_JWT clobber question resolved. All three security fix areas are correctly implemented and adequately tested. The migration is production-safe for tables of any normal size. The IDOR unification is complete and consistent across the affected mutations. The only item that needs explicit confirmation before merge is whether the AUTH0_JWT = {...} assignment in base.py is additive or a clobber of an existing key.

[claude]

Code Review: Tighten Admin Validations + IDOR Prevention

This is a solid, well-structured security hardening PR. The changes are logically organized across three independent layers, test coverage is comprehensive, and the documentation is thorough. Below are observations organized by severity.

---

Overview

Three independent security fixes:

1. Auth0 privilege escalation — allowlist gate on JWT-driven is_superuser elevation
2. IDOR-safe GraphQL mutations — unified error messages + visible_to_user() filtering
3. Analyzer callback token hashing — SHA-256 + constant-time compare replaces plaintext UUID storage

---

Issues Worth Fixing Before Merge

1. rotate_callback_token() auto-save is a footgun for unsaved instances with dirty fields

opencontractserver/analyzer/models.py

if self.pk is not None:
    self.save(update_fields=["callback_token_hash"])

The docstring only warns about the "unsaved instance" case (pk is None). It doesn't warn about the reverse: calling rotate_callback_token() on an instance that has a pk but also has other unsaved mutations. In that case, save(update_fields=["callback_token_hash"]) discards those mutations silently — the caller's subsequent save() won't include the hash update because update_fields would override it. Consider adding a _skip_save=False parameter for contexts that manage their own save(), or at minimum extend the warning in the docstring to cover this case.

2. Import inside settings class scope may cause circular imports

config/settings/base.py:

from opencontractserver.constants.auth import (
    ADMIN_CLAIMS_CACHE_TTL as _DEFAULT_ADMIN_CLAIMS_CACHE_TTL,
)

This import is inside the Base settings class body. It runs at class definition time, pulling in opencontractserver.constants.auth before some Django internals are ready. It works today, but it's fragile — any future import added to opencontractserver/constants/auth.py that transitively depends on a Django model or app will cause a bootstrap crash. A safer pattern is to inline the numeric default (30) and add a comment referencing the constant's source.

3. cast(Any, headers) proliferation

The cast(Any, headers) workaround is copy-pasted into 4+ files (docling_parser_rest.py, docxodus_parser.py, cohere_reranker.py, users/tasks.py). CLAUDE.md's DRY principle applies here. Options:

• Annotate headers as dict[str, str | bytes] at declaration (the actual type requests accepts), eliminating the need for a cast entirely.
• Create a RequestHeaders = dict[str, str | bytes] type alias in a utility file and import it at each site.

Either removes the repeated cast and its repeated explanatory comment.

---

Performance Note

30-second cache TTL at scale

Reducing ADMIN_CLAIMS_CACHE_TTL from 300 → 30 seconds means the claim-sync write path fires ~10× more often for active users. At low traffic this is immaterial; at scale (many concurrent authed requests) it becomes DB write pressure on every N-th request per user per 30 seconds. The PR correctly makes this configurable via AUTH0_ADMIN_CLAIMS_CACHE_TTL, which is the right escape hatch. Worth noting this in a deployment runbook or CHANGELOG entry for operators who profile request latency.

---

Minor / Nitpicks

4. Migration is correct but the "atomic by default" assumption should be explicit

opencontractserver/analyzer/migrations/0021_hash_analysis_callback_token.py

The comment says "Atomic by design. Add/backfill/drop must be one transaction." PostgreSQL supports DDL transactions, so this is true by default. But consider adding atomic = True explicitly to the Migration class to make the intent self-documenting and guard against any future ATOMIC_REQUESTS = False override.

5. Silencing users.E001 (Critical) in test settings

config/settings/test.py:

SILENCED_SYSTEM_CHECKS = ["users.W001", "users.E001"]

The comment correctly explains that the check tests call the function directly and bypass silencing. The concern is cosmetic: silencing E001 (Critical) in the shared test config means a CI run that inadvertently creates a superuser row in the test DB during setUpTestData in another test class won't surface a startup critical. The mitigation (direct function calls in the check tests) is sound. Add a brief comment explaining that E001 is silenced because the Critical severity is verified by TestAuth0SuperuserAllowlistSystemCheck.test_check_escalates_to_critical_with_existing_superuser calling check_auth0_superuser_allowlist directly.

6. _JWKS_STALE_GRACE = 3600 is a long grace window

A one-hour stale-serve window for JWKS means a compromised Auth0 signing key can still be used to forge tokens for up to an hour after the key is rotated out of the JWKS endpoint. This is a deliberate availability-vs-security tradeoff (as noted in the PR), but the value should be documented alongside the constant with an explicit note about what it protects against and why 3600 was chosen vs a shorter value.

---

What's Done Well

• SHA-256 + hmac.compare_digest() is the correct choice for this credential pattern. The plaintext never touches the DB, the backfill migration correctly hashes the existing UUID strings so in-flight callbacks keep working, and token rotation on each submission is the right lifecycle.
• System checks (users.W001 / users.E001) are an excellent addition — surfacing the misconfigured allowlist at startup before an admin login mysteriously fails is exactly the right UX.
• visible_to_user() pattern is used correctly in all mutated resolvers, consistent with CLAUDE.md conventions.
• Unified error messages across AddRelationship, CreateMetadataColumn, and UpdateMetadataColumn properly close the enumeration oracle without leaking whether an object exists or is just inaccessible.
• Test coverage is comprehensive — the unit tests on rotate_callback_token / verify_callback_token cover the important edges (empty hash, None candidate, rotation invalidation, unsaved instance). The IDOR tests are structured as proper attacker/victim pairs.
• Logging changes — switching from f-strings to %-style and redacting the callback token in the submission log are both correct.
• AUTH0_CREATE_NEW_USERS env var is a useful operational escape hatch for locked-down deployments.

---

Summary

Approve with the rotate_callback_token() footgun (item 1) addressed — that one is likely to bite a future caller. Items 2 and 3 are worth fixing before merge to avoid fragility and drift. The remaining items are notes/suggestions that can be addressed in follow-up. The core security logic is sound.

[claude]

Code Review — PR #1641: Tighten Admin Validations + IDOR Prevention

This is a well-scoped security hardening PR with clear reasoning behind each change. The three-layer approach (Auth0 privilege escalation, IDOR-safe mutations, and hashed callback tokens) is coherent and defensible. Below are observations organized by severity.

---

Bugs / Correctness Issues

UpdateMetadataColumn — pre-existing indentation bug fixed (implicit, worth calling out)
The diff shows the original code had a return statement that was not indented inside the if not user_has_permission_for_obj(...) block — the permission check was effectively dead code. This PR silently fixes that. The fix is correct, but it may be worth noting in the PR description since the old code would have allowed any authenticated user to update any column they could resolve.

checks.py — Python version compatibility (Python < 3.10)

def check_auth0_superuser_allowlist(
    app_configs: Any, **kwargs: Any
) -> list[Warning | Critical]:

The Warning | Critical union syntax in the return annotation requires Python 3.10+ to evaluate at runtime. Without from __future__ import annotations at the top of the file, importing checks.py on Python 3.8/3.9 will raise TypeError: unsupported operand type(s) for |: 'type' and 'type'. Since the migration file correctly includes from __future__ import annotations, this appears to be an oversight. Quick fix:

from __future__ import annotations

AUTH0_JWT dict in base.py — verify it is actually consumed
The PR adds:

AUTH0_JWT = {
    "AUTH0_CREATE_NEW_USERS": env.bool("AUTH0_CREATE_NEW_USERS", default=True),
}

The comment says this is consumed by get_auth0_user_from_token via settings.AUTH0_JWT, but the utils.py diff doesn't show any new code path reading from settings.AUTH0_JWT["AUTH0_CREATE_NEW_USERS"]. If the existing get_auth0_user_from_token doesn't already read this key, the new env variable is silently ignored. Please confirm with a grep or pointer to the call site.

---

Performance Considerations

ADMIN_CLAIMS_CACHE_TTL: 300 → 30 seconds — 10× write rate increase
For deployments with many active admin/staff users this means 10× more frequent cache.set + sync_admin_claims_from_payload calls on the hot auth path. The new AUTH0_ADMIN_CLAIMS_CACHE_TTL env override mitigates this for operators who need the old behaviour. Worth noting in the deployment notes that existing deployments should set AUTH0_ADMIN_CLAIMS_CACHE_TTL=300 if the write load is a concern.

Migration 0021 — atomic by default, large-table risk
The migration is atomic by default (Django default), meaning AddField + RunPython(backfill) + RemoveField all run in a single transaction. The comment acknowledges write-lock risk for large Analysis tables (10k+ rows). For deployments with many rows, consider whether atomic = False on the Migration class with a manual two-step deploy is warranted. The current approach is safe for typical analyzer record counts but could cause multi-second table locks in edge cases.

---

Security Observations (All Positive)

• Allowlist + system check escalation (W001 → E001 when existing superusers exist) is a smart operational safety net. Blocking manage.py check --deploy prevents accidentally locking out the only admin account on upgrade.
• hmac.compare_digest in verify_callback_token is correct constant-time usage. The empty-hash guard correctly prevents bypass via null/empty token.
• rotate_callback_token auto-save when self.pk is not None prevents the race where plaintext is transmitted but the hash is not stored. The docstring clearly warns about the unsaved-instance case.
• int() cast on global IDs in AddRelationship is a good addition — it collapses non-integer decoded IDs into the IDOR-safe not_found_msg branch rather than letting them reach the queryset boundary.
• _can_serve_stale lock hazard docstring is appropriately thorough. The single-call-site argument is valid for now.
• Redacted logging in analyzer.py (%-style format + [REDACTED] token in the log copy) is a correct operational security improvement.

---

Minor Code Quality Notes

• checks.py: The import get_user_model inside the try block is unconventional. Moving it to module level would be cleaner, but the current approach is safe since it avoids import-time side effects during makemigrations.
• migration 0021: The import uuid inside restore_plaintext_tokens is consistent with Django migration conventions — fine.
• test_metadata_columns_graphql.py: The assertion update from assertIn("don't have permission", ...) to the new IDOR-safe message is correct and necessary.
• test_admin_auth.py — TestAuth0SuperuserAllowlistSystemCheck.setUp(): Calling User.objects.filter(is_superuser=True).update(is_superuser=False) in setUp() is safe under TestCase transaction isolation, but could mask flakiness if future tests rely on migration-created superuser rows. Low risk.

---

Summary

Approve with minor comments. The security reasoning is sound, the code is well-commented, and the test coverage (32 new test methods across 5 test files) covers the critical paths including negative cases and edge cases. Three items to follow up before merge:

1. Add from __future__ import annotations to checks.py (runtime breakage on Python < 3.10).
2. Confirm AUTH0_CREATE_NEW_USERS is actually read from settings.AUTH0_JWT somewhere in get_auth0_user_from_token.
3. Consider a deployment note about ADMIN_CLAIMS_CACHE_TTL changing from 300 → 30 seconds for existing deployments.