[mikebridge]

SUMMARY

When a dashboard is saved, created or copied with a layout, Superset rebuilds the dashboard's chart list from the chart references in that layout. Those references weren't checked on the server, so a layout could add any chart to the dashboard.

The server now accepts a chart the layout adds only if the user can read it. Charts already on the dashboard, archived ones included, are always accepted, so saving a dashboard you can edit keeps working even if it contains charts you couldn't add yourself. When copying, the source dashboard's own charts are accepted the same way. A layout that adds a chart the user can't read is rejected with a 422 that names the chart IDs, and nothing is saved.

The check sits where the layout becomes the chart list (DashboardDAO.set_dash_metadata), so it covers dashboard update, create and copy, as well as the MCP tools that save layouts through the same path.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A (API behaviour only).

TESTING INSTRUCTIONS

• pytest tests/integration_tests/dashboards/api_tests.py -k layout_chart_access
• As a non-admin user who edits a dashboard: saving the dashboard as is, adding one of your own charts, and copying the dashboard all still work. A layout that references a chart you can't read is rejected with a 422, and the dashboard is unchanged.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.57%. Comparing base (700c614) to head (79f43d4).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #45538       +/-   ##
===========================================
+ Coverage   65.50%   84.57%   +19.06%     
===========================================
  Files        3070     3060       -10     
  Lines      205501   204640      -861     
  Branches    48306    47903      -403     
===========================================
+ Hits       134614   173066    +38452     
+ Misses      67804    28280    -39524     
- Partials     3083     3294      +211     

Flag	Coverage Δ
hive	34.49% <17.85%> (-0.01%)	⬇️
mysql	52.52% <100.00%> (+0.01%)	⬆️
postgres	52.53% <100.00%> (+0.01%)	⬆️
presto	36.22% <17.85%> (-0.01%)	⬇️
python	87.73% <100.00%> (+34.95%)	⬆️
sqlite	52.29% <100.00%> (+0.01%)	⬆️
unit	82.45% <67.85%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codeant-ai-for-open-source]

Suggestion: On non-admin saves, each unallowed chart reaches can_access_chart without preloaded editors or viewers, causing two lazy-load queries per chart and slowing large layout saves.

Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Performance

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset/daos/dashboard.py
**Line:** 716:719
**Comment:**
	*Performance: On non-admin saves, each unallowed chart reaches `can_access_chart` without preloaded editors or viewers, causing two lazy-load queries per chart and slowing large layout saves.

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

👍 | 👎

[sadpandajoe]

Agreed—can_access_chart reads Slice.editors / Slice.viewers lazily and the Slice query here doesn't preload them, so a non-admin save adding N readable charts the caller doesn't own issues roughly 2N extra SELECTs (existing members short-circuit, so unchanged layouts are unaffected). prefetch_chart_access() already solves this for dashboard reads; should the new-chart candidates use it?

[codeant-ai-for-open-source]

Yes. Reuse prefetch_chart_access() for the charts not already in allowed_chart_ids before calling can_access_chart. That batches the editor/viewer loads while preserving the existing authorization check and its behavior; already-allowed members remain skipped. This is a worthwhile fix for layouts with many newly referenced charts.

[mikebridge]

Applied. prefetch_chart_access() loads the dashboard's existing members, and those are the charts this check skips, so I used the same pattern scoped to the new charts instead: one selectinload for editors and viewers before the access check, skipped for admins.

[codeant-ai-for-open-source]

  CodeAnt PR Risk

🟢  Low Risk

• The PR appears safe to merge; editor and viewer relationships for new charts are batch-loaded before access checks, avoiding per-chart lazy-load queries.

Assessed commit: 79f43d4878e1

[fitzee]

No blocking issues; LGTM. The check sits on the one path that rebuilds dashboard.slices from a layout, so PUT, POST, /copy/ and the MCP tools that go through those commands are all covered. The bypasses I tried are rejected and the existing-member and archived-member cases still save. What's left is a UX edge case and some sibling paths outside this PR's scope.

What I checked

• Coverage. set_dash_metadata is the only place a layout becomes membership. UpdateDashboardCommand (superset/commands/dashboard/update.py:156), CreateDashboardCommand (superset/commands/dashboard/create.py:68), and copy_dashboard (superset/daos/dashboard.py:1068) all route through it. The MCP add_chart_to_existing_dashboard, generate_dashboard and duplicate_dashboard tools use those commands or that DAO method. A raw position_json PUT with no positions key only reconciles the layout and never changes membership, so it can't be used to stage charts that a later save re-attaches.

• ID forms. The check reads the same _layout_chart_id coercion as the membership query. A foreign chart referenced as "4" or 4.0 gets the same 422 as 4 (run locally). Only type == "CHART" nodes add membership, so other node types can't smuggle in a chart.

• Dangling and archived references. IDs that don't exist never reach the check. They are still turned into placeholders and the save returns 200, so dashboards with hard-deleted charts aren't locked out. Archived members pass because _member_chart_ids reads dashboard_slices directly.

• Copy. The copy endpoint already requires editorship of the source (superset/commands/dashboard/copy.py), so allowing the source's members is sound. In the duplicate_slices path every chart node must already map to a clone (superset/daos/dashboard.py:1050-1058), so that path's allowed_chart_ids adds nothing in practice: the creator is an editor of every clone.

• Error response and existence oracle. The 422 echoes back only IDs the caller sent, and only ones that exist, archived included. The caller learns whether an ID exists, but chart IDs are sequential and this is much less than what the old behaviour leaked (it attached the chart). Acceptable.

• Tests. Mutation-tested locally on SQLite. These mutations are all caught:

  • removing the raise;
  • dropping the member allowance;
  • excluding archived members from _member_chart_ids;
  • dropping the copy-source allowance.

  Only the duplicate_slices allowance survives mutation; see the last point below.

Non-blocking

1. A stale layout reference to a detached chart now blocks every layout save (superset/daos/dashboard.py:716-719). Repro, confirmed locally:

   1. Dashboard D (editor: Gamma) has chart C (owned by Admin; Gamma can't read it) in both its layout and its membership.
   2. Admin detaches C with PUT /api/v1/chart/<C> {"dashboards": []}. The dashboard_slices row goes, but D's position_json still has the CHART-<C> node.
   3. Gamma re-saves D's layout unchanged and gets 422 The layout references charts that are not available: <C>.

   Every layout save then fails until Gamma finds and deletes that tile, and the error gives only a numeric ID. Rejecting the save is the right security outcome; before this PR, the save silently re-attached C, undoing the chart owner's detach. But the UX could be better. Since the builder only offers charts the user can list, one option is to handle an unreadable chart that isn't a member the same way as a dangling one: turn it into a placeholder and don't attach it. That also removes the existence oracle. If you keep the 422, a test for this drift case would document the intent.

2. Sibling paths that still attach charts without this check. These are outside the PR's stated scope, but worth tracking:

   • Import with overwrite=False. superset/commands/dashboard/importers/v1/__init__.py:292-325 links any existing chart whose UUID appears in the bundle, because import_chart returns the existing alive chart without any permission check. This needs import permission and knowledge of the chart's UUID.
   • Version restore. superset/versioning/restore.py:747-761 re-attaches historical members with no access check. In the drift scenario above, Gamma can restore a version from before Admin's detach and get C back. That works around both the chart owner's removal and this check.
   • Native filter targets. native_filter_configuration targets feed the same dashboard-viewer and embedded-guest datasource grant (superset/security/manager.py:5527-5545). An editor can point a target's datasetId at any dataset through json_metadata, and nothing checks dataset access when the dashboard is written. Same threat model as this PR, different field.

3. Performance (agrees with the bot comment, but it's minor). can_access_chart lazy-loads editors, viewers and the datasource for each new chart that isn't a member. Admins short-circuit, and normal saves add 0-2 charts. Only an API or MCP create with a large layout from a non-admin pays roughly N×3 queries. If you want to fix it, selectinload(Slice.editors, Slice.viewers) on the current_slices query at superset/daos/dashboard.py:714 is enough.

4. Test gaps.

   • POST /api/v1/dashboard/ isn't covered. Locally it correctly returns 422 for a foreign chart and 201 for the user's own chart.
   • The duplicate_slices copy path isn't covered.
   • Nothing covers the string or float chartId forms.

   Adding the POST case to the parameterized test is cheap.

5. Interaction with #45528 and #45530. #45528 changes the clone loop to clone only the layout's charts. Every chart node in that path is still remapped to a clone first, so the set(old_to_new_slice_ids.values()) allowance stays correct either way. Expect a textual conflict near superset/daos/dashboard.py:1068. The version-fork UI (createDashboardFromSnapshot) already swaps out charts that /api/v1/chart/ doesn't return. Its list filter matches can_access_chart well enough that forking an old version shouldn't start hitting this 422.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	79f43d4
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ae2848fc5b5117d74e764a2
😎 Deploy Preview	https://deploy-preview-45538--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[fitzee]

Re-reviewed at ebb6ca1c: Still LGTM with no blockers. The new pre-check in validate() is sound and reopens no bypass, and #45543 and #45530 only conflict textually. The detached-chart 422 from our first review hasn't been addressed or replied to.

The PR's own delta since 802bc6ca is two commits, 72d657d4 and ebb6ca1c. They answer sadpandajoe's three comments (report-email side effect, readable-not-edited test, prefetch). CI: 58 checks pass, 14 skipped.

Prior items

• Detached chart blocks every layout save: unchanged, no reply. Re-ran the repro at ebb6ca1c:

  1. Admin sends PUT /api/v1/chart/<C> {"dashboards": []}.
  2. Gamma re-saves the unchanged layout and gets 422 The layout references charts that are not available: <C>.

  Only the dashboard_slices rows are honoured: _member_chart_ids at superset/daos/dashboard.py:302-316, used at :559 and :753-756. That means the bypass stays closed. Leaving a tile in the layout doesn't keep or re-attach a chart the caller can't read. The UX point stands, and there's still no test that documents the intent.

• Sibling paths: still open, out of scope.

  • Import with overwrite=False links charts by UUID.
  • Version restore re-attaches past members.
  • Native filter datasetId targets are written unchecked.

  The PR doesn't touch any of these.

• Performance: partly fixed. superset/daos/dashboard.py:332-339 now loads editors and viewers with one selectinload for non-member candidates and skips it for admins. I confirmed that 1 and 6 added charts both issue 2 editor and 2 viewer queries. The datasource-permission EXISTS queries inside can_access_chart are still per chart; see new item 2.

• Test gaps: partly closed.

  • Added: the readable-not-edited case (chart on a dataset granted to Gamma) for both PUT and copy.
  • Still untested:
    • POST /api/v1/dashboard/. Locally it returns 422 for a foreign chart, 201 for Gamma's own chart and 201 for a readable chart.
    • String and float chartId. Locally "<id>" and <id>.0 both return 422 and membership is unchanged.
    • The duplicate_slices copy path.

• Surviving mutant: still survives. No test covers the duplicate_slices branch of the copy allowance at superset/daos/dashboard.py:1109-1113. It's harmless, because every chart node in that path is already remapped to a clone.

• #45528: now merges cleanly with this head (git merge-tree). The :1068 conflict I predicted no longer happens.

New in this push

1. The pre-check runs before side effects and is correct (superset/commands/dashboard/update.py:213-227, superset/daos/dashboard.py:539-559).
   • validate() now refuses an unreadable new layout chart before process_tab_diff() / process_native_filter_diff() deactivate reports and send email.
   • It uses the same _reject_malformed_chart_nodes, _layout_chart_id, skip_visibility_filter and member allowance as the set_dash_metadata backstop. So it can't accept something the backstop would reject, or the reverse, on PUT.
   • The backstop still covers create, copy and MCP.
   • Mutation check: replacing the validate() call with pass fails only test_update_dashboard_layout_chart_access_rejects_before_side_effects. The backstop catches the other cases.
   • The UpdateDashboardNativeFiltersCommand, ...ChartCustomizationsCommand and ...ColorsConfigCommand subclasses inherit validate(), but their payloads have no json_metadata, so the check does nothing there.
2. Minor: on PUT the access check now runs twice for each newly added non-member chart (superset/commands/dashboard/update.py:213 and superset/daos/dashboard.py:753).
   • Measured with a non-admin PUT that adds dataset-granted charts:
     • With both checks, going from 1 to 6 added charts adds about 23 statements per chart (104 → 217).
     • With the backstop disabled, it adds about 15 per chart (93 → 166).
   • The extra cost is mostly the per-chart permission EXISTS queries from raise_for_access.
   • Normal saves add 0-2 charts, so this is non-blocking. Skipping the backstop when the command has already validated, or caching per datasource, would remove it.

Cross-PR merge check

• #45543 (e30773a1): textual conflict only, semantically compatible.
  • The conflict is in set_dash_metadata (superset/daos/dashboard.py:753-757 against #45543's _install_membership_baseline / kept_archived block) and in api_tests.py.
  • The natural resolution runs _raise_for_unreadable_new_charts, then _install_membership_baseline(dashboard), then dashboard.slices = current_slices + kept_archived.
  • Both PRs read the stored dashboard_slices rows as the baseline. kept_archived only keeps existing archived members, so it never adds a chart the check would have had to judge.
  • With that resolution, the layout-access and archived integration tests passed (9) and tests/unit_tests/dao/dashboard_test.py passed (66).
• #45530 (d4b45d97): still textual only.
  • The conflict is the set_dash_metadata call in copy_dashboard (superset/daos/dashboard.py:1105-1114) plus api_tests.py. Resolve by keeping #45530's snapshot-metadata and _copy_chart_metadata lines and adding this PR's allowed_chart_ids= kwarg.
  • No bypass via clones: #45530 clones only original_dash.slices members that appear in the layout, so the old_to_new_slice_ids.values() allowance can't include a clone of a chart the caller can't read.
  • Effect on version copies: copying a version whose layout references a chart that is no longer a live member and that the caller can't read now returns 422. That matches the intent of this PR.
  • Merged tree: 18 copy and layout-access integration tests passed, and tests/unit_tests/dao/ passed (201). Two older copy tests errored in the SQLite example-data fixture (too many SQL variables), which is unrelated to either PR.

[mikebridge]

AI-assisted reply on Mike's behalf.

@fitzee: thanks for the second pass. Pushed in 5005764, then a master merge (fe6e6a6).

Detached chart still in the layout. This is intended for this PR, and there's now a test for it (test_update_dashboard_layout_detached_chart).

• Only dashboard_slices rows count as membership. A stale tile neither keeps a chart a member nor lets a save re-attach it.
• A caller who can't read the chart gets the 422 until the tile is removed. Saving without the tile works.
• A caller who can read it, such as an admin, re-attaches it on the next save, as before this PR.
• The cost is the UX you described: an editor can get stuck on a tile they can't see. Accepting the stale tile would reopen exactly the path this PR closes, so I'd fix the UX in a follow-up. The options:
  • your suggestion: treat a chart that isn't a member, and that the caller can't read, like a dangling reference, i.e. render a placeholder and don't link it. This also removes the existence oracle;
  • or, when a chart is detached through the chart API, drop its tile from the layout, which fixes the drift at its source.

Test gaps.

• Added POST /api/v1/dashboard/: own chart and readable chart give 201; a foreign chart gives 422.
• Added string and float chartId: both give the access-denied 422, and membership is unchanged.
• Added the duplicate_slices copy.
• The surviving mutant at daos/dashboard.py:1110 is equivalent. The clone loop already links each clone to the new dashboard, so _member_chart_ids(dashboard.id) admits the clones before the allowance is consulted. With chart reads denied, removing either one alone still passes, and removing both fails the new test. I can drop the redundant branch here or in a follow-up, whichever you prefer.

Double access check on PUT. Agreed, it's non-blocking. A follow-up could skip the set_dash_metadata check when the update command has already validated the same layout, or cache the datasource decision per datasource within a request. I'd keep the backstop for create, copy and MCP either way.

[sadpandajoe]

LGTM