fix: embedded dashboard check

[betodealmeida]

SUMMARY

When a guest user with an embedded dashboard token tries to access a non-embedded dashboard we allow as long as the other dashboard is in draft or the other dashboard has no datasources or at least one datasource that the user has access.

This PR fixes the logic, so that guest users have access only if they have the proper token. I also left some comments to the discussion about the raise_for_dashboard_access, because the logic is very convoluted, non-intuitive, and remains so for backwards compatibility reasons.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

Merging #24690 (a748425) into master (c53b249) will increase coverage by 0.04%.
The diff coverage is 81.17%.

❗ Current head a748425 differs from pull request most recent head 04e65d5. Consider uploading reports for the commit 04e65d5 to get more accurate results

@@            Coverage Diff             @@
##           master   #24690      +/-   ##
==========================================
+ Coverage   68.97%   69.02%   +0.04%     
==========================================
  Files        1907     1902       -5     
  Lines       74153    73940     -213     
  Branches     8182     8160      -22     
==========================================
- Hits        51148    51037     -111     
+ Misses      20882    20785      -97     
+ Partials     2123     2118       -5     

Flag	Coverage Δ
hive	54.12% <23.52%> (?)
mysql	79.36% <78.82%> (?)
postgres	79.44% <78.82%> (-0.12%)	⬇️
presto	54.02% <23.52%> (-0.02%)	⬇️
python	83.45% <81.17%> (+0.13%)	⬆️
sqlite	78.03% <70.58%> (-0.09%)	⬇️
unit	54.83% <27.05%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
...hart-echarts/src/MixedTimeseries/transformProps.ts	0.78% <ø> (ø)
...plugins/plugin-chart-echarts/src/utils/controls.ts	71.42% <ø> (-3.58%)	⬇️
.../plugin-chart-pivot-table/src/plugin/buildQuery.ts	76.92% <ø> (+5.49%)	⬆️
...d/plugins/plugin-chart-table/src/transformProps.ts	65.59% <ø> (-0.37%)	⬇️
...d/src/SqlLab/components/AceEditorWrapper/index.tsx	68.91% <ø> (+1.84%)	⬆️
...c/SqlLab/components/RunQueryActionButton/index.tsx	78.78% <ø> (-0.63%)	⬇️
...frontend/src/SqlLab/components/SqlEditor/index.jsx	59.88% <ø> (ø)
...ntend/src/SqlLab/components/TableElement/index.tsx	82.92% <ø> (+0.99%)	⬆️
...-frontend/src/components/DeprecatedSelect/utils.ts	50.00% <ø> (+8.33%)	⬆️
superset-frontend/src/components/Icons/Icon.tsx	0.00% <ø> (ø)
... and 59 more

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[michael-s-molina]

Thanks for adding the references. I agree that this logic is hard to understand and only exists for backward compatibility. I added a card to our major versions board to revisit this in 4.0 and break compatibility in favor of a clear and more restrictive logic.

[john-bodley]

I second @michael-s-molina's comment. I think the security logic for dashboard access is rather convoluted and difficult to grok which is definitely undesirable from a security perspective, i.e., there likely are holes which can be exploited.

I wonder if in 4.0 if dashboard RBAC should no longer be a feature but rather the default which will likely simplify things even if (per @michael-s-molina's comment) this breaks compatibility.

[michael-s-molina]

I wonder if in 4.0 if dashboard RBAC should no longer be a feature but rather the default which will likely simplify things even if (per @michael-s-molina's comment) this breaks compatibility.

Interesting. I'll add this comment to the card to help the discussion.

[michael-s-molina]

LGTM