fix(revenue-analytics): add access-control checks to taxonomy and joins viewsets#73009
Conversation
|
😎 This pull request was merged. |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/revenue_analytics/backend/test/test_api_access_control.py:11-14
**Missing EE Module Leaves Undefined Name**
When this test is collected in an environment without the EE package, the import error is swallowed but every test later calls `AccessControl.objects.create`. The suite then fails with `NameError` instead of skipping this EE-only module.
```suggestion
AccessControl = pytest.importorskip("ee.models.rbac.access_control").AccessControl
```
Reviews (1): Last reviewed commit: "fix(revenue-analytics): add access-contr..." | Re-trigger Greptile |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a missing access-control check to two revenue-analytics endpoints, mirroring the exact pattern already used by the module's query runners; well-tested (403/200 cases covered), and the one Greptile-flagged test-hygiene nit matches an existing repo-wide convention (try/except ImportError for the EE-only model) rather than a real risk.
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 13L, 1F substantive, 80L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (80L, 2F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ 4f472f3 · reviewed head 6c5fcc5 |
|
Retaining stamphog approval — delta since last review classified as |
|
Retaining stamphog approval — delta since last review classified as |
🤖 CI report
|
|
Retaining stamphog approval — delta since last review classified as |
1 participant
Problem
RevenueAnalyticsTaxonomyViewSetandRevenueAnalyticsJoinViewSetusescope_object = "INTERNAL", which opts them out of therevenue_analyticsresource access-control check the rest of the module enforces. A member whoserevenue_analyticsaccess is restricted can still hit both endpoints.Changes
Enforce
revenue_analyticsaccess in both actions, matching the query runners:valuesrequiresviewercreaterequireseditorExplicit
check_access_level_for_resource(...)raisingPermissionDenied, keptscope_object = "INTERNAL". Instances without the access-control feature already default to access, so nothing changes for them.How did you test this code?
Added
test_api_access_control.py: a restricted member gets 403 on both endpoints, aviewergets 200 on taxonomy and 403 on the joins write (so the check is what blocks, not project access). These need ClickHouse, which my local test harness couldn't reach this session, so they run in CI. Locally I confirmed the underlyingcheck_access_level_for_resourcebehavior via the existing RBAC tests, plus ruff and mypy on the changed file.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
I (Claude) wrote this with Tom. Invoked
/improving-drf-endpointsand/writing-tests. Chose the explicit access check over flippingscope_objectto"revenue_analytics", since no other viewset uses that value and the explicit check mirrorsvalidate_query_runner_accesswithout relying on the scope machinery. Fixed both viewsets together, same root cause.