fix(dashboard-export): include and re-attach roles in import/export (#21000)#40136
fix(dashboard-export): include and re-attach roles in import/export (#21000)#40136rusackas wants to merge 3 commits into
Conversation
Closes #21000 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
authentication:RBAC
Code Review Agent Run #3edc95Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40136 +/- ##
==========================================
- Coverage 64.17% 64.16% -0.01%
==========================================
Files 2590 2590
Lines 138087 138099 +12
Branches 32039 32043 +4
==========================================
+ Hits 88615 88617 +2
- Misses 47947 47955 +8
- Partials 1525 1527 +2
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:
|
|
The flagged issue is correct: the test's docstring states that dashboards with no roles 'must not emit an empty roles key', but the assertion allows both missing 'roles' and 'roles: []', contradicting the requirement. To resolve, update the assertion to strictly check that 'roles' is not in the result, ensuring no 'roles' key is emitted for unrestricted dashboards. tests/unit_tests/commands/dashboard/export_test.py |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dashboards restricted via `DASHBOARD_RBAC` had their role assignments silently dropped from export bundles, so a round-trip recreated the dashboard with no access restriction — a 'least privilege' dashboard became 'all roles can access' on import. `Dashboard.export_fields` doesn't include `roles` (it's a many-to-many relationship, not a column), so `export_to_dict` skipped it entirely. This change: - **Export** (`export.py:_file_content`): emit `roles` as a list of role *names* in the YAML when the dashboard has any. Names rather than IDs because IDs are environment-local — names are the only thing that can cross environments. Omits the key entirely when there are no role restrictions (older import code treats "missing" as "no restriction"; an empty list could confuse importers that distinguish the two states). - **Import** (`importers/v1/utils.py:import_dashboard`): pop `roles` before handing config to `import_from_dict` (the standard SQLAlchemy path doesn't resolve role names into role objects), then resolve each name via `security_manager.find_role` and assign `dashboard.roles` after creation. Roles that don't exist in the destination are skipped with a warning rather than failing the whole import. Companion regression tests (added in this PR's earlier commit) cover both the populated-roles and no-roles cases on the export side. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rusackas
changed the title
|
Bito Automatic Review Failed - Technical Failure |
rusackas
requested review from
betodealmeida,
dpgaspar,
eschutho,
hainenber and
mistercrunch
|
Hi @rusackas ERROR:superset.commands.importers.v1.utils:Schema validation failed for dashboards/Unicode_Test_25.yaml (prefix: dashboards): {'roles': ['Unknown field.']} Schema validation fails need to be added for ImportV1DashboardSchema(Schema) |
|
Ahh, interesting. Thanks for testing. I'll investigate soon, but feel free to push a commit if you spit the issue. |
3 participants
SUMMARY
Closes #21000.
Dashboards restricted via
DASHBOARD_RBAChad their role assignments silently dropped from export bundles, so a round-trip recreated the dashboard with no access restriction — a "least privilege" dashboard became "all roles can access" on import.Dashboard.export_fieldsdoesn't includeroles(it's a many-to-many relationship, not a column), soexport_to_dictskipped it entirely.This PR:
Export (
export.py:_file_content): emitrolesas a list of role names in the YAML when the dashboard has any. Names rather than IDs because IDs are environment-local — names are the only thing that can cross environments. Omits the key entirely when there are no role restrictions (older import code treats "missing" as "no restriction"; an empty list could confuse importers that distinguish the two states).Import (
importers/v1/utils.py:import_dashboard): poprolesbefore handing config toimport_from_dict(the standard SQLAlchemy path doesn't resolve role names into role objects), then resolve each name viasecurity_manager.find_roleand assigndashboard.rolesafter creation. Roles that don't exist in the destination are skipped with a warning rather than failing the whole import — admins may need to create them before the access restriction takes effect.Locks in regression tests (added in this PR's earlier commit):
test_file_content_includes_roles_for_dashboard_with_role_restrictionstest_file_content_omits_roles_field_when_dashboard_has_no_roles(strict —"roles" not in result)Security note
This is a security improvement: a round-trip that silently lost role restrictions is the kind of footgun that turns a "least privilege" config into a "public" one without any visible change. Bundles that already exist in the wild without a
roleskey continue to import as "no restriction," matching prior behavior. Bundles with aroleskey are interpreted as a restriction request.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
DASHBOARD_RBAC(for the bug to manifest in practice)🤖 Generated with Claude Code