feat(can_share): can share chart and dashboard #14076

amitmiran137 · 2021-04-11T17:24:30Z

SUMMARY

As all things are so should sharing be a managed under a permission the user entitles to.
can_share_dashboard and can_share_chart are the new permissions .
both hide/show ShareMenuItems component on the dashboard/chart menus

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

can_share.mov

TEST PLAN

check that sharing is menu option is shown by default for Admin/Gamma/Alpha
remove can share chart/dashboard on Superset from you user role
check that sharing menu option is hidden

ADDITIONAL INFORMATION

Has associated issue:
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

share dashboard can_share_dashboard

codecov · 2021-04-11T17:32:18Z

Codecov Report

Merging #14076 (88b523e) into master (80da1ca) will increase coverage by 1.31%.
The diff coverage is 100.00%.

❗ Current head 88b523e differs from pull request most recent head 2ac7224. Consider uploading reports for the commit 2ac7224 to get more accurate results

@@            Coverage Diff             @@
##           master   #14076      +/-   ##
==========================================
+ Coverage   78.25%   79.57%   +1.31%     
==========================================
  Files         942      943       +1     
  Lines       47675    47768      +93     
  Branches     5977     6029      +52     
==========================================
+ Hits        37306    38009     +703     
+ Misses      10215     9638     -577     
+ Partials      154      121      -33

Flag	Coverage Δ
cypress	`56.33% <83.33%> (+9.78%)`	⬆️
hive	`?`
javascript	`69.62% <90.90%> (+0.49%)`	⬆️
mysql	`80.72% <100.00%> (+<0.01%)`	⬆️
postgres	`80.75% <100.00%> (+<0.01%)`	⬆️
presto	`80.47% <100.00%> (+<0.01%)`	⬆️
python	`81.05% <100.00%> (-0.27%)`	⬇️
sqlite	`80.35% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/dashboard/actions/hydrate.js	`85.84% <ø> (+15.09%)`	⬆️
.../src/dashboard/components/gridComponents/Chart.jsx	`86.59% <ø> (ø)`
...nativeFilters/FilterBar/FilterSets/EditSection.tsx	`80.00% <ø> (+46.00%)`	⬆️
...tiveFilters/FilterBar/FilterSets/FilterSetUnit.tsx	`95.65% <ø> (+52.17%)`	⬆️
...tiveFilters/FilterBar/FilterSets/FiltersHeader.tsx	`91.42% <ø> (+59.99%)`	⬆️
...ents/nativeFilters/FilterBar/FilterSets/Footer.tsx	`95.23% <ø> (+47.61%)`	⬆️
...nents/nativeFilters/FilterBar/FilterSets/index.tsx	`46.46% <ø> (ø)`
...perset-frontend/src/dashboard/containers/Chart.jsx	`100.00% <ø> (ø)`
...d/components/DashboardBuilder/DashboardBuilder.tsx	`95.34% <100.00%> (ø)`
...erset-frontend/src/dashboard/components/Header.jsx	`59.70% <100.00%> (+6.76%)`	⬆️
... and 105 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80da1ca...2ac7224. Read the comment docs.

etr2460

one question, otherwise i think this makes sense. I'll defer to @dpgaspar or someone more familiar with FAB permissions to stamp though

tests/security_tests.py

superset-frontend/src/dashboard/components/SliceHeaderControls/index.jsx

* master: fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) # Conflicts: # superset-frontend/src/dashboard/actions/hydrate.js # superset/views/core.py

dpgaspar

LGTM,
Assuming that this is a frontend only permission stored on the server. We are not actually preventing user's from accessing the functionality just hiding it.

graceguo-supercat · 2021-04-20T17:01:02Z

This PR caused SHARE links (share url, share in email etc) are hidden in dashboard. @amitmiran137 @dpgaspar

amitmiran137 · 2021-04-20T17:04:38Z

If you run superset init I believe it will create by default the missing permission to Admin/Alpha/Gamma

For the rest of your roles you should add the permission manually

amitmiran137 · 2021-04-20T17:13:00Z

For backward compatibility you could create a migration that associate the permission to all existing roles

junlincc · 2021-04-20T20:16:07Z

@amitmiran137 was there a db migration made in this PR?
look at the PR description, it was not super clear about all the permission changes that are being made. More details may help prevent future confusion.

amitmiran137 · 2021-04-20T20:57:24Z

No migration were need bc the PR made sure that those 2 new permission are added by default once you run Superset init
And I as far as I know running superset init is a must command on every new deployment right after dB upgrade

* feat: share chart - can_share_chart share dashboard can_share_dashboard * fix: pre-commit * fix: userCanShare tests * fix: after hugh CR * fix: adjust after spa refactor

amitmiran137 added 2 commits April 11, 2021 15:01

feat: share chart - can_share

896b5fb

feat: share chart - can_share_chart

1c6134f

share dashboard can_share_dashboard

pull-request-size bot added the size/M label Apr 11, 2021

amitmiran137 added 2 commits April 11, 2021 20:36

fix: pre-commit

514efe3

fix: userCanShare tests

f3f2c4e

amitmiran137 requested review from hughhhh and a team and removed request for hughhhh April 11, 2021 18:38

etr2460 reviewed Apr 12, 2021

View reviewed changes

tests/security_tests.py Show resolved Hide resolved

hughhhh reviewed Apr 12, 2021

View reviewed changes

superset-frontend/src/dashboard/components/SliceHeaderControls/index.jsx Outdated Show resolved Hide resolved

amitmiran137 added 3 commits April 13, 2021 08:29

fix: after hugh CR

dc98bf6

fix: adjust after spa refactor

2ac7224

dpgaspar approved these changes Apr 13, 2021

View reviewed changes

amitmiran137 merged commit 9c5264a into master Apr 13, 2021

amitmiran137 deleted the feat/can_share_chart branch April 13, 2021 10:54

junlincc added the dashboard:security:access Related to the security access of the Dashboard label Apr 15, 2021

graceguo-supercat mentioned this pull request Apr 20, 2021

[Dashboard][Share] Share related links are gone #14251

Closed

3 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(can_share): can share chart and dashboard #14076

feat(can_share): can share chart and dashboard #14076

amitmiran137 commented Apr 11, 2021

codecov bot commented Apr 11, 2021 •

edited

Loading

etr2460 left a comment

dpgaspar left a comment

graceguo-supercat commented Apr 20, 2021 •

edited

Loading

amitmiran137 commented Apr 20, 2021 •

edited

Loading

amitmiran137 commented Apr 20, 2021

junlincc commented Apr 20, 2021

amitmiran137 commented Apr 20, 2021 •

edited

Loading

feat(can_share): can share chart and dashboard #14076

feat(can_share): can share chart and dashboard #14076

Conversation

amitmiran137 commented Apr 11, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented Apr 11, 2021 • edited Loading

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

graceguo-supercat commented Apr 20, 2021 • edited Loading

amitmiran137 commented Apr 20, 2021 • edited Loading

amitmiran137 commented Apr 20, 2021

junlincc commented Apr 20, 2021

amitmiran137 commented Apr 20, 2021 • edited Loading

codecov bot commented Apr 11, 2021 •

edited

Loading

graceguo-supercat commented Apr 20, 2021 •

edited

Loading

amitmiran137 commented Apr 20, 2021 •

edited

Loading

amitmiran137 commented Apr 20, 2021 •

edited

Loading