fix(Audit): Retrieving records via /api/v1/audit/ with a master API key results in error - #8275
fix(Audit): Retrieving records via /api/v1/audit/ with a master API key results in error#8275bardock-2393 wants to merge 1 commit into
Conversation
…ey results in error `AllAuditLogViewSet` assumed `request.user` was an `FFAdminUser` and filtered audit logs by joining through `UserOrganisation`. Master API Key authentication supplies an `APIKeyUser`, which is not a database row, so Django raised `TypeError: Field 'id' expected a number but got <APIKeyUser>` and the endpoint returned a 500. Formalise `get_admin_organisations()` on `UserABC` and implement it for `APIKeyUser`, returning the key's organisation when the key is an admin key and nothing otherwise. `AllAuditLogViewSet` now resolves administered organisations through that interface rather than assuming a concrete user model, which keeps the existing org-admin visibility semantics for both authentication methods. The same assumption broke the six Platform Hub endpoints, which already called `get_admin_organisations()` on `request.user` and raised `AttributeError` for master API keys. Those are fixed by the same change.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8275 +/- ##
==========================================
- Coverage 98.73% 98.59% -0.15%
==========================================
Files 1567 1567
Lines 62379 62416 +37
==========================================
- Hits 61591 61538 -53
- Misses 788 878 +90 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Listing audit log records with a Master API Key returns a 500 instead of the records. Anyone driving Flagsmith through the Terraform provider, a CI script, or a SIEM/compliance pipeline cannot read the audit log at all with the credential they already use for everything else — the endpoint fails before it gets as far as checking what the key is allowed to see.
The endpoint assumed the caller was always a signed-in user, and looked up their organisations by following a database relation that only exists for real user accounts. A Master API Key is not a user account, so that lookup fell over.
/api/v1/audit/now returns audit logs for Master API Key callers instead of erroring.type: ignorecomments in the surrounding code.Closes #6583
Review effort: 2/5
How did you test this code?
Added tests that fail on
mainand pass with this change:/api/v1/audit/with an admin Master API Key returns the organisation's records. Without the fix this reproduces the reportedTypeError: Field 'id' expected a number but got <APIKeyUser>exactly./api/v1/audit/with a non-admin Master API Key returns an empty list, matching the existing behaviour for non-admin users./api/v1/audit/with an admin Master API Key belonging to a different organisation returns an empty list, so the fix does not leak records across organisations.AttributeError: 'APIKeyUser' object has no attribute 'get_admin_organisations'.The existing tests covering user-authenticated access to this endpoint (regular user, and admin of another organisation) still pass unchanged, which is what confirms the visibility rules did not shift.
Also ran the full backend suite (4518 passed, 48 skipped — the only errors were InfluxDB connection failures from that service not running locally, in tests unrelated to this change),
mypyin strict mode across the codebase, andpre-commitover the changed files.