fix(dashboard): count extra_editors when deciding who may edit a dashboard - #42708
Conversation
…board The server's is_editor unions the explicit editor list with subjects resolved by EXTRA_EDITORS_RESOLVER (security/manager.py), and the dashboard GET attaches those ids as `extra_editors` after the response schema is dumped, so they survive the `columns` projection. The frontend predicate `canUserEditDashboard` checked `editors` alone, so anyone whose editorship came only through the resolver saw no Edit button on a dashboard the API would happily let them write to. Widen the shared predicate to editors union extra_editors, matching the server, and type the field on Dashboard. `dash_edit_perm` is derived from this function at hydrate time, so every consumer of that flag is corrected together. Only deployments that configure EXTRA_EDITORS_RESOLVER are affected; behaviour is unchanged everywhere else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dosubot
Bot
added
authentication:access-control
Code Review Agent Run #0e627bActionable 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 #42708 +/- ##
==========================================
- Coverage 65.43% 65.43% -0.01%
==========================================
Files 2810 2810
Lines 159460 159466 +6
Branches 36396 36398 +2
==========================================
Hits 104350 104350
- Misses 53068 53074 +6
Partials 2042 2042
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:
|
…ments
Review follow-ups from a clean-code + tidy-first + react panel:
- New test pins the true union: non-matching explicit editors plus a
matching extra_editors grant. The existing cases all used editors: [],
so a refactor that consulted extra_editors only when editors is empty
would have stayed green. Also covers the resolver-returns-nothing shape.
- The doc and test comments narrated the bug history ("hid the Edit
button"); they now state the timeless contract — the predicate must
stay in step with the server's is_editor — per the project's
timeless-comments rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #96b0c3Actionable 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 |
2 participants
SUMMARY
The server's
is_editor(superset/security/manager.py) unions the explicit editor list with subjects resolved by a deployment'sEXTRA_EDITORS_RESOLVER, and the dashboard GET attaches those ids asextra_editorsafter the response schema is dumped (superset/dashboards/api.py:614), so they survive thecolumnsprojection. The frontend predicatecanUserEditDashboardcheckededitorsalone.Result: anyone whose editorship comes only through the resolver sees no Edit button on a dashboard the API would happily let them write to. This bites exactly — and only — in deployments that configure the resolver.
This PR widens the shared predicate in
permissionUtils.tstoeditors ∪ extra_editors, matching the server, and types the field onDashboard. Sincedash_edit_permis derived from this function at hydrate time, every consumer of that flag is corrected together. Behaviour is unchanged for deployments without the resolver.Split out of #41551, where the gap was found during review (thanks @kgabryje) — it is a platform-wide fix independent of version history and shouldn't ride inside a feature-flagged PR.
Disclosure: this change was developed with AI assistance (Claude), on behalf of and reviewed by @mikebridge.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no visual change except the Edit button appearing for users the API already authorizes.
TESTING INSTRUCTIONS
EXTRA_EDITORS_RESOLVERinsuperset_config.pyto return a subject id for a non-admin test user on some dashboard, e.g. a resolver granting a specific role's subject.PUT /api/v1/dashboard/{id}succeeds for this user. After: Edit button shows and editing works.npm run test -- permissionUtils.test.ts— includes cases for editorship granted throughextra_editorsand forextra_editorsbelonging to other subjects.ADDITIONAL INFORMATION
Scope boundary (from review): this PR fixes the dashboard single-entity surface only, deliberately. Two adjacent gaps are known and out of scope here:
extra_editors(superset/models/slice.py,superset/charts/api.py), and the in-flight version-history PR feat(versioning): version-history UI #41551 already fixes the Save-modal/menu gates via its sharedcanOverwriteSlicehelper. The remaining chart gap after both merge is the editable-titlecanEditgate inExploreChartHeader, tracked as a follow-up.DashboardCard/ChartCard/list views) cannot be fixed client-side: the list endpoints never attachextra_editors(only the single-entity GETs do, post-dump). Extra editors see hover-edit hidden on cards while the entity itself is editable once opened — attaching per-rowextra_editorsin list payloads has a resolver-cost × page-size question and needs a backend decision.