global logging for PermissionDenied DRF errors

[sravfeyn]

Product Description

https://dimagi.atlassian.net/browse/CI-430

Followup on #878 to enable such logging for all API requests where permission is denied.

Technical Summary

Safety Assurance

Safety story

Automated test coverage

Tested flow locally to see it gets logged.

Tested flow locally to see logs.

QA Plan

NA

Labels & Review

• The set of people pinged as reviewers is appropriate for the level of risk of the change

[coderabbitai]

Walkthrough

Removed a custom OpportunityViewSetPermission from commcare_connect/opportunity/api/views.py and reverted OpportunityViewSet to use DRF's IsAuthenticated. Added commcare_connect/utils/exceptions.drf_permission_denied_handler to log PermissionDenied events (user id, auth method, request URL, sanitized headers) and delegate to DRF's default handler. Wired that handler into REST_FRAMEWORK via the EXCEPTION_HANDLER setting in config/settings/base.py.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Add a temporary permission class that logs info to sentry #878: Removes the per-view logging permission class and shifts permission-denied logging to a centralized DRF exception handler.

Suggested reviewers

• calellowitz

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: implementing global logging for DRF PermissionDenied errors across all API requests.
Description check	✅ Passed	The description provides relevant context, referencing a follow-up to PR 878 and linking to the related ticket, indicating the work is intended to enable logging across all API requests with PermissionDenied errors.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch sr/drf-exception

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8687d9c and cb1f749.

📒 Files selected for processing (3)

• commcare_connect/opportunity/api/views.py
• commcare_connect/utils/exceptions.py
• config/settings/base.py

🧰 Additional context used

🪛 Ruff (0.14.10)

commcare_connect/opportunity/api/views.py

35-35: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: pytest

🔇 Additional comments (2)

commcare_connect/opportunity/api/views.py (1)

33-38: LGTM!

Simplifying to use IsAuthenticated directly is appropriate now that the custom logging logic has been centralized in the exception handler. The static analysis hint about ClassVar can be safely ignored here—this is standard DRF convention for permission_classes.

config/settings/base.py (1)

321-321: Configuration is correct.

The EXCEPTION_HANDLER setting correctly points to the new centralized handler. Ensure the issues flagged in commcare_connect/utils/exceptions.py (missing return for non-PermissionDenied exceptions, sensitive header logging) are addressed before merging.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

commcare_connect/utils/exceptions.py (1)

23-23: Minor: Redundant parentheses in isinstance check.

The tuple wrapper around a single type is unnecessary.

🔎 Suggested fix

-    if isinstance(exc, (PermissionDenied)) and request is not None:
+    if isinstance(exc, PermissionDenied) and request is not None:

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb1f749 and babfa62.

📒 Files selected for processing (1)

• commcare_connect/utils/exceptions.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: pytest

🔇 Additional comments (3)

commcare_connect/utils/exceptions.py (3)

1-6: LGTM!

Imports are minimal and appropriate. Logger setup follows Python best practices using __name__.

---

10-19: Header sanitization addresses the security concern.

This correctly redacts sensitive headers before logging, fixing the previously flagged security issue. The substring matching approach is reasonable for catching variations like X-Api-Key or Auth-Token.

Note: The substring "key" may be slightly broad (matching headers like X-Monkey-Header), but erring on the side of over-redaction is safer for security.

---

21-39: Critical fix applied: All exceptions now delegate correctly.

The return statement on line 39 is now outside the if block, ensuring all exception types (not just PermissionDenied) are properly handled by DRF's default handler. This fixes the previously flagged critical issue.

The safe attribute access patterns (getattr, hasattr) are well implemented.