fix(views): enforce per-chart access check in legacy form_data endpoint#40497
fix(views): enforce per-chart access check in legacy form_data endpoint#40497sha174n wants to merge 11 commits into
Conversation
The legacy /api/v1/form_data/?slice_id=N endpoint returned slc.form_data
verbatim with only the broad @has_access_api gate, while the modern
ChartRestApi.get returns 404 to callers without datasource_access on the
chart's underlying dataset. This brings query_form_data to the same bar
by invoking security_manager.raise_for_access(chart=slc) before reading
form_data.
To avoid introducing a status-code-based existence oracle (where a
non-existent slice_id returns 200 + {} while a forbidden one returns
403), the SupersetSecurityException is caught and normalised to the
same 404 ChartRestApi.get uses; callers cannot distinguish "missing"
from "forbidden".
The new test mirrors the canonical test_get_chart_no_data_access
pattern (Gamma user, "Girl Name Cloud" chart, 404 assertion) and adds
defence-in-depth body assertions so a future regression that returned
a partial form_data dict in an error envelope is still caught.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40497 +/- ##
==========================================
- Coverage 63.97% 63.96% -0.02%
==========================================
Files 2654 2649 -5
Lines 142753 142416 -337
Branches 32833 32736 -97
==========================================
- Hits 91325 91094 -231
+ Misses 49870 49765 -105
+ Partials 1558 1557 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
api:charts
Code Review Agent Run #b8d346Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The pr_comments.csv file contains only 1 line of data (header) and no actual review comments. I cannot analyze or count any suggestions or comments without content to work with. |
Reuse ChartDAO.get_by_id_or_uuid so the endpoint applies the same ChartFilter (dataset-scoped) as the modern ChartRestApi.get, and return 404 for both missing and unauthorised slice IDs so callers cannot enumerate chart existence via status code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Code Review Agent Run #77ec94Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
The legacy
Api.query_form_data(GET /api/v1/form_data/?slice_id=)returned
slc.form_dataverbatim with only@has_access_apias thegate, which is satisfied by any authenticated user (and by
Publicrole on deployments using
PUBLIC_ROLE_LIKE). The siblingApi.query()already validates viaquery_context.raise_for_access(), and the modernChartRestApi.getreturns 404 to callers without
datasource_access. This bringsquery_form_datato the same bar.To avoid introducing a status-code-based existence oracle (where a
non-existent
slice_idreturns 200 +{}while a forbidden onereturns 403), the
SupersetSecurityExceptionis caught andnormalised to the same 404
ChartRestApi.getuses, so callerscannot distinguish
missingfromforbidden.BEFORE/AFTER SCREENSHOTS
N/A (server-side authorisation change).
TESTING INSTRUCTIONS
The added
test_query_form_data_no_data_accessmirrors the canonicaltest_get_chart_no_data_accesspattern (Gamma user, "Girl Name Cloud"chart, 404 assertion) and adds defence-in-depth body assertions so a
regression that returned a partial form_data dict inside an error
envelope is still caught.
ADDITIONAL INFORMATION