Align query optimizers and visibility filtering

[JSv4]

Document the two-layer permission filtering architecture:

• visible_to_user() manager methods for standard visibility
• Query optimizers for complex composite permissions

Include comprehensive remediation plan with prioritized action items:

• High: DocumentManager inheritance audit, metadata permission docs
• Medium: Shared permission utility, request-level caching
• Low: Logging cleanup, index recommendations, deprecation removal

[claude]

Pull Request Review: Query Permission Architecture Documentation

This PR adds comprehensive documentation for OpenContracts' two-layer permission filtering architecture and proposes a remediation plan for identified technical debt. Overall, this is excellent documentation work that will significantly improve maintainability and developer onboarding. Here's my detailed feedback:

---

✅ Strengths

1. Comprehensive and Well-Structured

• Clear separation between visible_to_user() manager methods and Query Optimizers
• Excellent use of tables, code examples, and visual hierarchies
• Progressive disclosure: overview → patterns → specific implementations → best practices

2. Actionable Remediation Plan

• Well-prioritized (High/Medium/Low) action items
• Each item includes specific file paths, line numbers, and scope estimates
• Realistic implementation phases with clear dependencies

3. Strong Security Focus

• Documents IDOR protection patterns (H2, Pattern 5)
• Highlights the critical DocumentManager inheritance issue (H1)
• Emphasizes the MIN (most restrictive) permission model

4. Performance Awareness

• Detailed N+1 query prevention patterns (Pattern 3)
• Request-level caching documentation (M3)
• Bulk permission check examples

---

🔴 Critical Issues

C1: DocumentManager Permission Inheritance (Mirrors H1)

CONFIRMED ISSUE: Looking at the code:

• DocumentManager (line 269 in Managers.py) extends BaseVisibilityManager
• But returns DocumentQuerySet (line 276) which extends PermissionQuerySet (line 187 in QuerySets.py)
• PermissionQuerySet.visible_to_user() (lines 137-184) has commented-out guardian permission checks and only checks is_public and creator

Impact: Documents shared via set_permissions_for_obj_to_user() will NOT appear in Document.objects.visible_to_user(user) results, potentially breaking the entire sharing feature.

Verification needed: The inheritance chain suggests BaseVisibilityManager.visible_to_user() should win (manager method > queryset method), but this needs immediate testing:

# Test case from remediation plan
document = Document.objects.create(creator=user_a, is_public=False)
set_permissions_for_obj_to_user(user_b, document, [PermissionTypes.READ])
assert document in Document.objects.visible_to_user(user_b)  # Will this pass?

Recommendation: This should be Priority 0 (before High). Add the test script in docs/test_scripts/test_document_guardian_permissions.md and run it ASAP.

---

🟡 Medium-Priority Issues

M1: Missing Integration with Existing Documentation

The new documentation should reference and be referenced by:

1. docs/permissioning/consolidated_permissioning_guide.md (currently 26k+ tokens) - Add a section pointing to the new query patterns doc
2. CLAUDE.md - Update the "Documentation Locations" section to include the new files
3. Consider adding a brief "see also" at the top of query_permission_patterns.md pointing to the consolidated guide

Locations to update:

• CLAUDE.md line ~301 (Documentation Locations section)
• consolidated_permissioning_guide.md (add cross-reference in Overview section)

M2: Metadata Permission Model Underdocumented

The documentation mentions (query_permission_patterns.md lines 324-345) that metadata uses a different permission model than annotations, but:

1. This isn't mentioned in the existing consolidated_permissioning_guide.md
2. The rationale ("metadata schemas are corpus-level features") should be more prominent
3. Should document which specific mutations use this model

Recommendation:

• Add to H2 action items: Update the consolidated guide immediately
• This divergence is a potential security review gap that needs prominent documentation

M3: Test Coverage Claims Need Verification

The remediation plan (line 268) references test locations:

• opencontractserver/tests/test_permissions.py - Does not exist (verified via Glob)
• opencontractserver/tests/test_graphql_permissions.py - Does not exist

Related tests that DO exist:

• test_annotation_permission_mutations.py
• test_document_relationship_permissions.py
• test_mention_permissions.py
• test_annotation_privacy.py

Recommendation: Update the remediation plan to reference actual test files, or create placeholders as part of H1's action items.

---

🟢 Minor Issues / Suggestions

L1: Code Reference Format

Per CLAUDE.md (lines 235-241), code references should use file_path:line_number format. The docs do this well in most places, but consider adding references for:

• Pattern examples (e.g., "See implementation at opencontractserver/shared/Managers.py:40")
• Each optimizer in the table (lines 111-119)

L2: Changelog Update

Per CLAUDE.md (lines 196-215), significant documentation should be reflected in CHANGELOG.md. This architecture documentation should be added under "Technical Details" in the Unreleased section.

L3: Minor Typo/Clarity Issues

1. Line 84 in query_permission_patterns.md: "the user's Claude.ai/code" should be "Claude Code" or "claude.ai/code" (consistent with the introduction)
2. Line 267 in remediation plan: "Estimated Scope: 4-6 files modified, ~200 lines of code" - Good estimate, but should note this excludes test additions
3. Line 146 in query_permission_patterns.md: Consider adding a note about when to use select_related vs prefetch_related in optimizers

L4: Consider Adding a Decision Tree

A visual decision tree would help developers choose between patterns:

Do you need to query objects?
├─ Is this a top-level model with direct permissions?
│  └─ Use visible_to_user() [Pattern 1]
├─ Do permissions inherit from multiple sources?
│  └─ Use Query Optimizer [Pattern 2]
└─ Is this a one-off mutation permission check?
   └─ Use user_has_permission_for_obj() [See permissioning.py]

---

📊 Remediation Plan Assessment

Excellent Prioritization

The High → Medium → Low breakdown is spot-on:

• H1 is genuinely a security issue
• H2 is a documentation gap that could lead to security issues
• M1-M4 are code quality improvements that will pay dividends
• L1-L5 are nice-to-have cleanups

Suggested Reordering

Consider splitting H1 into two items:

1. H1a (Immediate): Write and run the test case to verify current behavior
2. H1b (High): Fix the issue if the test fails, or document the inheritance chain if it passes

Implementation Notes

The 4-phase plan (lines 241-260) is realistic. Consider:

• Adding time estimates ("2-4 hours" for H1, "1-2 days" for M1, etc.) - though CLAUDE.md discourages this for user-facing communication, internal planning docs can have estimates
• Actually, per CLAUDE.md line 132, never give time estimates. Ignore my previous suggestion. The phase-based breakdown is better.

---

🧪 Testing Recommendations

Before merging, I recommend:

1. Run the H1 verification test described above
2. Test the documented patterns by writing a simple script that exercises each pattern:
   • Create a document, share it, query with visible_to_user()
   • Create an annotation, query with AnnotationQueryOptimizer
   • Create metadata, verify the different permission model
3. Validate all line number references - a grep script can verify they're still accurate

---

🎯 Final Recommendation

Approve with conditions:

1. ✅ Merge this documentation as-is - it's valuable even with the issues noted
2. 🔴 Immediately create and run the H1 test case (before next sprint)
3. 🟡 Address M1-M3 within the next 2 sprints
4. 🟢 L1-L4 can be addressed opportunistically

This is high-quality work that demonstrates deep understanding of the codebase. The remediation plan is actionable and well-scoped. Great job! 🎉

---

📝 Checklist for Author

• Add cross-references to existing documentation (M1)
• Update CHANGELOG.md with "Added query permission architecture documentation"
• Create H1 test script in docs/test_scripts/
• Verify all line number references are accurate
• Update test file references in remediation plan (M3)
• Consider adding the decision tree diagram (L4)