chore: Add granular permissions for actions in Dashboard #27029

geido · 2024-02-06T15:34:15Z

SUMMARY

This PR introduced a new permission to enable view and drilling actions on the Dashboard independently of the can_explore permission #26798. However, from recent feedback it is clear that the need is to have more granularity for these permissions.

This PR introduces the following permissions:

can_view_chart_as_table to enable the "View as table" option in the chart actions in a Dashboard
can_view_query to enable the "View query" option in the chart actions in a Dashboard

This PR also implements the following changes:

It removes the permission can_view_and_drill introduced in chore: Add permission to view and drill on Dashboard context #26798.
It ties Drill to detail to both can_explore and can_samples as can_samples is a required backend permission for Drill to detail to work
It ties Drill by to both can_explore and can_write_ExploreFormDataRestAPI as the latter is a required backend permissions for the submission of form data when interacting with Drill by

Fixes #26762
Fixes #25630

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N.A.

TESTING INSTRUCTIONS

CI should pass
A role with can_view_chart_as_table should see the View as table option without can_explore permission
A role with can_view_query should see the View query option without can_explore permission
A role without can_samples should not be able to see Drill to detail
A role without can_write_ExploreFormDataRestAPI should not see Drill by

ADDITIONAL INFORMATION

Has associated issue: Cannot use drill-by/drill-to in embedded mode without can explore on Superset permission #25630 Gamma role does not have permissions to drill #26762
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

superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx

kgabryje · 2024-02-06T15:54:29Z

superset/security/manager.py

@@ -726,7 +726,8 @@ def create_custom_permissions(self) -> None:
        self.add_permission_view_menu("can_csv", "Superset")
        self.add_permission_view_menu("can_share_dashboard", "Superset")
        self.add_permission_view_menu("can_share_chart", "Superset")
-        self.add_permission_view_menu("can_view_and_drill", "Dashboard")


I know that can_view_and_drill permission wasn't around for long so probably few people have used it, but shouldn't we create a migration for it?

It'd be nice to have a reusable function for perm migration that we could use in database migrations. Now that we have migration_utils.py in theory it could contain a function migrate_permission(before_perm, after_perm). Though in theory it should only be use for equivalent (rename) or more atomic/selective permissions.

we have this: https://github.com/apache/superset/blob/master/superset/migrations/shared/security_converge.py#L215

hi I have all roles. And can not see drill down menu in superset graphs. Why?

hi I have all roles. And can not see drill down menu in superset graphs. Why?

Just to ask basic support questions, are you not seeing them in ANY chart, or just not EVERY chart? They're still not supported everywhere. They also appear on right-cick, which might not be the most discoverable thing in the world :)

superset/security/manager.py

codecov · 2024-02-06T17:36:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (13915bb) 67.19% compared to head (3c4cdb2) 69.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27029      +/-   ##
==========================================
+ Coverage   67.19%   69.54%   +2.34%     
==========================================
  Files        1899     1899              
  Lines       74380    74385       +5     
  Branches     8275     8276       +1     
==========================================
+ Hits        49981    51728    +1747     
+ Misses      22344    20602    -1742     
  Partials     2055     2055

Flag	Coverage Δ
hive	`53.81% <100.00%> (?)`
javascript	`56.91% <100.00%> (+<0.01%)`	⬆️
mysql	`78.01% <100.00%> (+<0.01%)`	⬆️
postgres	`78.14% <100.00%> (+0.03%)`	⬆️
presto	`53.76% <100.00%> (?)`
python	`83.10% <100.00%> (+4.85%)`	⬆️
sqlite	`77.66% <100.00%> (+0.03%)`	⬆️
unit	`56.49% <0.00%> (?)`

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

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

…o/ch77449/dashboard-granular-permissions

sfirke · 2024-02-07T17:25:35Z

Would this PR address or affect open issue #25630?

It looks relevant to open issue #26762, at a glance it would not solve it (Gamma would still need can_samples) but if someone working on this PR felt in the right headspace to tackle this issue it would save on task switching costs.

…o/ch77449/dashboard-granular-permissions

geido · 2024-02-08T09:45:35Z

Would this PR address or affect open issue #25630?

It looks relevant to open issue #26762, at a glance it would not solve it (Gamma would still need can_samples) but if someone working on this PR felt in the right headspace to tackle this issue it would save on task switching costs.

@sfirke in terms of fixing the mentioned issues, I think this PR should be the solution. Drill to detail must be tied to can_samples in order to function and can_explore to respect the current permission dependency on the UI. Drill by requires can_write on ExploreFormDataRestApi or there will be an error message showing up when posting form data, together with can_explore based on current permission dependency on the UI. I haven't checked whether these permissions can work without can_explore but my objective here is to avoid a breaking change.

The View as table and View query options now can be enabled granularly and the Edit chart buttons in the respective Modals will be disabled if can_explore is OFF.

As for what concerns assigning/changing permissions of the existing roles, this is beyond the scope of this PR and I think it would be best to discuss this opportunity in the appropriate channels. Thanks!

kgabryje · 2024-02-08T12:29:31Z

@geido I think drill by needs the can_explore permission only for the "Edit chart" button. If we made that button conditionally disabled if user doesn't have access to Explore and sent the post request in line 90 in DrillByModal.tsx, then the feature should work just fine without can_explore.
That would solve the issues that @sfirke linked in his comment

geido · 2024-02-08T13:26:09Z

@geido I think drill by needs the can_explore permission only for the "Edit chart" button. If we made that button conditionally disabled if user doesn't have access to Explore and sent the post request in line 90 in DrillByModal.tsx, then the feature should work just fine without can_explore. That would solve the issues that @sfirke linked in his comment

That's right. Drill by does not need can_explore. However, removing can_explore would cause Drill by to show up and it might not be the expected behaviour for most people at this point. I think having granular permissions for both Drill by and Drill to detail could be a better approach to remove the dependency on can_explore. @yousoph what do you think?

I would aim to merge this PR as it is and continue in a follow-up.

...t/migrations/versions/2024-02-07_17-13_87d38ad83218_migrate_can_view_and_drill_permission.py

dimoha · 2024-04-04T10:26:34Z

Hi

I think that i found a small bug
#27896

can you check it?

icrc-fdeniger · 2024-04-04T12:00:12Z

hello @geido

However, removing can_explore would cause Drill by to show up and it might not be the expected behaviour for most people at this point

Is there a plan to allow end-users to be able to Drill with having can_explore ?

Thanks

rusackas · 2024-04-04T14:25:36Z

For the sake of the thread, @icrc-fdeniger opened an issue regarding that permission question: #27900

puru-khedre · 2024-04-17T06:03:29Z

Hello @geido @dpgaspar
In which release this PR is merged, I want that permissions set

Or How can I know the release which has this permissions

michael-s-molina · 2024-04-17T12:49:42Z

Hello @geido @dpgaspar In which release this PR is merged, I want that permissions set

Or How can I know the release which has this permissions

Hi @puru-khedre. You can always check the CHANGELOG to see if a particular PR was included in a release. This one was included in 4.0.0 as you can see here.

puru-khedre · 2024-04-17T16:32:55Z

Hi @puru-khedre. You can always check the CHANGELOG to see if a particular PR was included in a release. This one was included in 4.0.0 as you can see here.

Thanks @michael-s-molina

agunoglu · 2024-05-03T11:19:50Z

First of all, thank you for your answer. - I enter the system with the admin role. all roles are open. Dimension columns are marked for my fields in the dataset. [image: image.png] - FEATURE_FLAGS settings in config py file are as below. FEATURE_FLAGS = { "DASHBOARD_NATIVE_FILTERS": True, "DASHBOARD_CROSS_FILTERS": True, "DASHBOARD_DRILL_DOWN": True, "DRILL_TO_DETAIL": True, "DRILL_BY": True } -And I have all roles for my admin user but when I check the my graps I cant see drill down options as below.. How can I do it..Thanks [image: image.png] so I added the self.add_permission_view_menu("can_view_and_drill", "Dashboard") under the def create_custom_permissions(self) -> None: like you said. There is no "can_view_and_drill" in the rolls. What can I do. Thanks..?? Evan Rusackas ***@***.***>, 2 May 2024 Per, 21:14 tarihinde şunu yazdı:

…

***@***.**** commented on this pull request. ------------------------------ In superset/security/manager.py <#27029 (comment)>: > @@ -726,7 +726,8 @@ def create_custom_permissions(self) -> None: self.add_permission_view_menu("can_csv", "Superset") self.add_permission_view_menu("can_share_dashboard", "Superset") self.add_permission_view_menu("can_share_chart", "Superset") - self.add_permission_view_menu("can_view_and_drill", "Dashboard") hi I have all roles. And can not see drill down menu in superset graphs. Why? Just to ask basic support questions, are you not seeing them in ANY chart, or just not EVERY chart? They're still not supported everywhere. They also appear on right-cick, which might not be the most discoverable thing in the world :) — Reply to this email directly, view it on GitHub <#27029 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQFE5QEYTBOMEO67QYCAPKTZAJ7CDAVCNFSM6AAAAABC4GTVZ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZWGU2DCNRTGQ> . You are receiving this because you commented.Message ID: ***@***.***>

geido · 2024-05-10T15:50:29Z

Hello @agunoglu can_view_and_drill was removed. Are you running on latest master?

saurabh-innovator · 2024-07-04T05:17:26Z

Hi @geido , I want to understand that why can_view_and_drill permission was removed. Issue that I am facing is, I wish to have a read-user who can just view chart table and just drill. I don't want to expose the query/edit/chart. That was the very good use case which you implemented, so why you removed it, also How can I again implement my case?

geido · 2024-07-30T15:59:43Z

Hi @geido , I want to understand that why can_view_and_drill permission was removed. Issue that I am facing is, I wish to have a read-user who can just view chart table and just drill. I don't want to expose the query/edit/chart. That was the very good use case which you implemented, so why you removed it, also How can I again implement my case?

can_view_and_drill was in master for only a few days. You can use can_view_chart_as_table to view the chart as table.

marianysilva · 2024-07-31T13:03:28Z

Hi @geido, can_view_chart_as_table does not produce the same result as drill to detail, as shown in the image.

I have the same problem here: I can enable drill to detail by adding the can read on Explore and can samples on Datasource permissions. However, it's noted that the can explore permission is currently mandatory but gives too many privileges.

can_view_chart_as_table

drill to detail

Cannot use drill-by/drill-to without can explore on Superset permission #27900
chore: Add granular permissions for actions in Dashboard #27029
Slack thread

geido · 2024-07-31T14:42:28Z

Hi @geido, can_view_chart_as_table does not produce the same result as drill to detail, as shown in the image.

I have the same problem here: I can enable drill to detail by adding the can read on Explore and can samples on Datasource permissions. However, it's noted that the can explore permission is currently mandatory but gives too many privileges.

can_view_chart_as_table

drill to detail

Cannot use drill-by/drill-to without can explore on Superset permission #27900
chore: Add granular permissions for actions in Dashboard #27029
Slack thread

Yes, it does not. I was referring to the first part of the previous comment. Feel free to implement more granular permissions. Happy to help with reviews and guidance. Thanks!

agunoglu · 2024-08-01T13:50:34Z

hi there is no permission for can_view_chart_as_table in my superset role permissions. How can I add it. [image: image.png] Geido ***@***.***>, 31 Tem 2024 Çar, 17:42 tarihinde şunu yazdı:

…

Hi @geido <https://github.com/geido>, can_view_chart_as_table does not produce the same result as drill to detail, as shown in the image. I have the same problem here: I can enable drill to detail by adding the can read on Explore and can samples on Datasource permissions. However, it's noted that the can explore permission is currently mandatory but gives too many privileges. can_view_chart_as_table Screenshot.2024-07-31.at.10.01.12.png (view on web) <https://github.com/user-attachments/assets/ebdb4f1f-bfd2-4509-8de3-636592e6168e> drill to detail Screenshot.2024-07-31.at.09.56.52.png (view on web) <https://github.com/user-attachments/assets/eca8fb75-b840-43c6-8558-d4462104daeb> Cannot use drill-by/drill-to without can explore on Superset permission #27900 <#27900> chore: Add granular permissions for actions in Dashboard #27029 <#27029> Slack thread <https://apache-superset.slack.com/archives/C0170U650CQ/p1721237807169809> Yes, it does not. I was referring to the first part of the previous comment. Feel free to implement more granular permissions. Happy to help with reviews and guidance. Thanks! — Reply to this email directly, view it on GitHub <#27029 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQFE5QFA5EKO2PFHX57O62LZPDZW7AVCNFSM6AAAAABC4GTVZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRQGY4DQNRZHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

geido · 2024-08-01T13:58:55Z

@agunoglu hard to tell without a bit more info. Are you running on latest master? Have you updated your Superset instance to apply the new permissions?

agunoglu · 2024-08-02T05:43:34Z

we don't use the latest version for the superset. After which version it is active. Thank you very much Geido ***@***.***>, 1 Ağu 2024 Per, 16:59 tarihinde şunu yazdı:

…

@agunoglu <https://github.com/agunoglu> hard to tell without a bit more info. Are you running on latest master? Have you updated your Superset instance to apply the new permissions? — Reply to this email directly, view it on GitHub <#27029 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQFE5QHY37URRYSBRPU4WBLZPI5LVAVCNFSM6AAAAABC4GTVZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTGE2DSNBQGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

geido added 4 commits February 6, 2024 13:11

Add granular permissions

aa55625

Override API permissions POC

1cd12c6

Rename canDrillBy to permission name

c3cd6f4

Improve permissions

9902ed3

pull-request-size bot added the size/L label Feb 6, 2024

geido added 2 commits February 6, 2024 17:38

Clean up

86444e6

Lint

4dc1d3c

geido requested a review from kgabryje February 6, 2024 15:48

kgabryje reviewed Feb 6, 2024

View reviewed changes

superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx Show resolved Hide resolved

kgabryje reviewed Feb 6, 2024

View reviewed changes

geido added 2 commits February 6, 2024 17:56

Protect samples route

d66dd50

Fix permission name

c68774c

mistercrunch reviewed Feb 6, 2024

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

geido added 3 commits February 7, 2024 16:56

Rename can_view_table permission

35c08b9

Merge branch 'master' of https://github.com/apache/superset into dieg…

14d8d98

…o/ch77449/dashboard-granular-permissions

Add migration

5078188

geido requested a review from a team as a code owner February 7, 2024 15:32

github-actions bot added the risk:db-migration PRs that require a DB migration label Feb 7, 2024

Black

2ed0676

dpgaspar mentioned this pull request Feb 7, 2024

fix(ci): fix action script v7 breaking changes v2 #27040

Merged

9 tasks

Merge branch 'master' of https://github.com/apache/superset into dieg…

aad279b

…o/ch77449/dashboard-granular-permissions

Merge branch 'master' of https://github.com/apache/superset into dieg…

1c3ac61

…o/ch77449/dashboard-granular-permissions

geido requested a review from dpgaspar February 8, 2024 09:29

dpgaspar reviewed Feb 8, 2024

View reviewed changes

...t/migrations/versions/2024-02-07_17-13_87d38ad83218_migrate_can_view_and_drill_permission.py Show resolved Hide resolved

justinpark requested a review from john-bodley February 8, 2024 17:54

geido added 3 commits February 9, 2024 13:21

Add migration test

2e10102

Add license

e4e1c14

Black

3c4cdb2

geido merged commit 595c6ce into master Feb 9, 2024
43 checks passed

rusackas deleted the diego/ch77449/dashboard-granular-permissions branch February 9, 2024 16:15

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

chore: Add granular permissions for actions in Dashboard (apache#27029)

5a9ce3f

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

eschutho mentioned this pull request Apr 26, 2024

chore: Add permission to view and drill on Dashboard context #26798

Merged

9 tasks

mistercrunch mentioned this pull request Jul 8, 2024

Hiding Force refresh in Dashboard #28249

Open

3 tasks

marianysilva mentioned this pull request Jul 31, 2024

Cannot use drill-by/drill-to without can explore on Superset permission #27900

Open

3 tasks

LaurenceLong mentioned this pull request Sep 11, 2024

Drill-by alert "Failed to load dimensions for drill by" using Gamma role #30233

Open

3 tasks

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

chore: Add granular permissions for actions in Dashboard (apache#27029)

e64702b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Add granular permissions for actions in Dashboard #27029

chore: Add granular permissions for actions in Dashboard #27029

geido commented Feb 6, 2024 •

edited

Loading

kgabryje Feb 6, 2024

mistercrunch Feb 6, 2024

dpgaspar Feb 8, 2024

agunoglu May 2, 2024

rusackas May 2, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

sfirke commented Feb 7, 2024

geido commented Feb 8, 2024 •

edited

Loading

kgabryje commented Feb 8, 2024 •

edited

Loading

geido commented Feb 8, 2024

dimoha commented Apr 4, 2024

icrc-fdeniger commented Apr 4, 2024

rusackas commented Apr 4, 2024

puru-khedre commented Apr 17, 2024

michael-s-molina commented Apr 17, 2024 •

edited

Loading

puru-khedre commented Apr 17, 2024

agunoglu commented May 3, 2024 via email

geido commented May 10, 2024

saurabh-innovator commented Jul 4, 2024

geido commented Jul 30, 2024

marianysilva commented Jul 31, 2024 •

edited

Loading

geido commented Jul 31, 2024

agunoglu commented Aug 1, 2024 via email

geido commented Aug 1, 2024

agunoglu commented Aug 2, 2024 via email

chore: Add granular permissions for actions in Dashboard #27029

chore: Add granular permissions for actions in Dashboard #27029

Conversation

geido commented Feb 6, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

kgabryje Feb 6, 2024

Choose a reason for hiding this comment

mistercrunch Feb 6, 2024

Choose a reason for hiding this comment

dpgaspar Feb 8, 2024

Choose a reason for hiding this comment

agunoglu May 2, 2024

Choose a reason for hiding this comment

rusackas May 2, 2024

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2024 • edited Loading

Codecov Report

sfirke commented Feb 7, 2024

geido commented Feb 8, 2024 • edited Loading

kgabryje commented Feb 8, 2024 • edited Loading

geido commented Feb 8, 2024

dimoha commented Apr 4, 2024

icrc-fdeniger commented Apr 4, 2024

rusackas commented Apr 4, 2024

puru-khedre commented Apr 17, 2024

michael-s-molina commented Apr 17, 2024 • edited Loading

puru-khedre commented Apr 17, 2024

agunoglu commented May 3, 2024 via email

geido commented May 10, 2024

saurabh-innovator commented Jul 4, 2024

geido commented Jul 30, 2024

marianysilva commented Jul 31, 2024 • edited Loading

geido commented Jul 31, 2024

agunoglu commented Aug 1, 2024 via email

geido commented Aug 1, 2024

agunoglu commented Aug 2, 2024 via email

geido commented Feb 6, 2024 •

edited

Loading

codecov bot commented Feb 6, 2024 •

edited

Loading

geido commented Feb 8, 2024 •

edited

Loading

kgabryje commented Feb 8, 2024 •

edited

Loading

michael-s-molina commented Apr 17, 2024 •

edited

Loading

marianysilva commented Jul 31, 2024 •

edited

Loading