fix(security): surface extra_editors in dashboard/chart lists - #43392
fix(security): surface extra_editors in dashboard/chart lists#43392EnxDev wants to merge 1 commit into
Conversation
dosubot
Bot
added
authentication:access-control
Code Review Agent Run #6fc504Actionable 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43392 +/- ##
==========================================
- Coverage 66.84% 66.82% -0.03%
==========================================
Files 2876 2876
Lines 164132 164257 +125
Branches 37875 37921 +46
==========================================
+ Hits 109719 109764 +45
- Misses 52238 52306 +68
- Partials 2175 2187 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| """Attach ``extra_editors`` to each row, matching the single-object GET.""" | ||
| super().pre_get_list(data) | ||
| ids = data.get("ids", []) | ||
| extra_editors_by_id = get_extra_editors_by_pk(Slice, ids) |
There was a problem hiding this comment.
Suggestion: The chart list path performs one EXTRA_EDITORS_RESOLVER call for each chart returned by the page through get_extra_editors_by_pk. This multiplies resolver latency with page size and propagates any resolver exception as a list-request failure, unlike the previous list behavior. Use a bounded or batch-aware resolution strategy, or handle per-chart resolver failures without aborting the entire response. [performance]
Severity Level: Major ⚠️
- ⚠️ Configured chart lists incur resolver work per returned chart.
- ❌ Resolver failure can make chart listings return an API error.
- ⚠️ Large chart pages amplify custom resolver latency.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/api.py
**Line:** 420:420
**Comment:**
*Performance: The chart list path performs one `EXTRA_EDITORS_RESOLVER` call for each chart returned by the page through `get_extra_editors_by_pk`. This multiplies resolver latency with page size and propagates any resolver exception as a list-request failure, unlike the previous list behavior. Use a bounded or batch-aware resolution strategy, or handle per-chart resolver failures without aborting the entire response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| """Attach ``extra_editors`` to each row, matching the single-object GET.""" | ||
| super().pre_get_list(data) | ||
| ids = data.get("ids", []) | ||
| extra_editors_by_id = get_extra_editors_by_pk(Dashboard, ids) |
There was a problem hiding this comment.
Suggestion: The list endpoint now invokes EXTRA_EDITORS_RESOLVER once for every dashboard in the page through get_extra_editors_by_pk. A resolver that performs I/O or is computationally expensive therefore turns a single list request into one resolver execution per row, multiplying latency and potentially causing the entire listing to fail if any resolver invocation raises. Use a bounded or batch-aware resolution strategy, or isolate resolver failures so one problematic resource does not break the whole list response. [performance]
Severity Level: Major ⚠️
- ⚠️ Configured dashboard lists incur resolver work per row.
- ❌ Resolver failure can make dashboard listings return an API error.
- ⚠️ Large dashboard pages amplify custom resolver latency.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 441:441
**Comment:**
*Performance: The list endpoint now invokes `EXTRA_EDITORS_RESOLVER` once for every dashboard in the page through `get_extra_editors_by_pk`. A resolver that performs I/O or is computationally expensive therefore turns a single list request into one resolver execution per row, multiplying latency and potentially causing the entire listing to fail if any resolver invocation raises. Use a bounded or batch-aware resolution strategy, or isolate resolver failures so one problematic resource does not break the whole list response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
I'm not familiar with this implementation detail, but I think I understand the logic and the fix here.
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
Reviewed this as a consistency fix rather than an authorization change: server-side is_editor/raise_for_editorship already union editors with the resolver output, so this only surfaces extra_editors on list responses and unions it into the client-side edit affordance, granting no new capability. It is also not a new exposure, since the single-object GET already returns extra_editors to anyone who can GET the object and list rows are access-filtered to that same audience; get_extra_editors_by_pk scopes its re-query to the page's already-filtered ids, so skipping the visibility filter avoids double-filtering without widening the set. Whole path is gated on EXTRA_EDITORS_RESOLVER, so stock deployments are unaffected. One nit: pre_get_list pairs rows via zip(result, ids), which relies on FAB keeping those aligned; a brief comment noting that assumption would help. LGTM once rebased on master (currently conflicting only because master moved in the same files).
3 participants
SUMMARY
extra_editors— editorship a deployment grants indirectly viaEXTRA_EDITORS_RESOLVER— was attached only to single-objectGETresponses (DashboardRestApi.get/ChartRestApi.get), never to list responses.DashboardCard,ChartCard, and the Dashboard/Chart list pages also checkededitorsonly. Combined, a resolver-granted editor saw the Edit affordance disabled on cards/list rows even though the server'sis_editoralready let them save once the dashboard/chart was open — a UI-affordance gap, not an authorization bug (confirmed viaraise_for_editorship/is_editor, which already unioneditorswith the resolver output).superset/security/manager.py: newget_extra_editors_by_pkbatch-resolvesextra editors for a page of rows in one query, re-querying by the primary
keys FAB already selected (list responses only expose serialized rows to
pre_get_list, not model instances).superset/dashboards/api.py/superset/charts/api.py: a newpre_get_listattachesextra_editorsper row, gated onEXTRA_EDITORS_RESOLVERbeing configured — zero extra cost when it isn't.permissionUtils.ts'sisUserEditorOrAdminnow takes an optionalextraEditorsargument;DashboardCard,ChartCard,DashboardList, andChartListpassextra_editorsalongsideeditors.DatasetListis untouched — datasets never gotEXTRA_EDITORS_RESOLVERsupport, so its
editors-only check is already correct.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
extra_editors_before_after.mp4
TESTING INSTRUCTIONS
pytest tests/integration_tests/dashboards/api_tests.py -k extra_editors— 4 new tests: list responses omit/include
extra_editorsfordashboards and charts depending on whether
EXTRA_EDITORS_RESOLVERisconfigured.
npm run test -- permissionUtils.test.ts— new cases forisUserEditorOrAdmin'sextraEditorsunion/precedence.EXTRA_EDITORS_RESOLVER, grant a non-editor usereditorship on a dashboard they can view but aren't listed as an editor
of, then confirm the dashboard list/card Edit affordance is enabled for
them.
ADDITIONAL INFORMATION