Dashboard level access control #5099

jasnovak · 2018-05-29T21:50:00Z

Add permissions at the dashboard level.

Create permissions for each dashboard with the format "dashboard access on [dashboard_title](dash_id:x)"
The name of the permission is updated if the dashboard title is changed.
Add a dashboard permission to a role via the Edit Role form, or, for convenience, add a role to a dashboard via the Edit Dashboard form.
Optionally, add a default role to be added to each new dashboard created from a slice.
The permission "all_dashboard_access" is added to Admin, Alpha, and Gamma roles.
To allow private dashboards, remove "all_dashboard_access" from a role. This role will then only have access to the dashboards specified. Add a dashboard's permission to a role to "publish" it.
Note that these permissions do not apply to slices. To make the data in a dashboard truly private, use dashboard permissions with a read only role that has no slice/explore access.

…into dash-access-control

mistercrunch · 2018-05-30T15:49:17Z

superset/superset_rmv.py

+                self.remove_dashboard_role(role, dash)
+
+
+class SupersetSecurityManager(SecurityManager):


SupersetSecurityManager is already defined in security.py and is the base people should use to override security-related things in their environment

Ah, this code was left over from the merge (I had defined my own SupersetSecurityManager before it was added to security.py). I've removed it. This new rolemodelview is already defined in the class in security.py.

mistercrunch · 2018-05-30T15:52:33Z

superset/views/core.py

+            {d: True for d in self.get_view_menus('dashboard_access')}
+
+        dashboard_ids = []
+        for dash in db.session.query(Dash):


There's concern in how getting the list of dashboards a user has access to is going to be an expensive operation.

Ideally here we want to really limit the number of round trips to the database (it should be one complex query really, if possible), as well as the complexity of the query (if the user has all_dashboard_access, the query shouldn't do complex join to the rbac tables)

Yes, I was concerned about that as well. But listing dashboards with this change runs at ~250ms for us, with several hundred of dashboards. I can look at limiting the round trips though.

I'm thinking here you want to only fetch the dashboard_perms if all_dashboard_access is not found, and then craft a filter(*sqla.or_(models.Dashboard.perm.in_(dashboard_perms), {pre-existing conditions here}))

…security.py

…into dash-access-control

mistercrunch · 2018-06-11T22:46:53Z

superset/migrations/versions/76a4d742cf04_dashboard_role_many_to_many.py

+
+
+def upgrade():
+    op.create_table('dashboard_role',


Hey so the model so far has been to not create these many-to-many tables, and instead creating perms for each object (table, database, schema). This has pros/cons, one of the pros being to have the security model limited to FAB's RBAC tables. Meaning roles are only made out of users and perms. Then perms act as imperfect FKs to the object table. Cons is the fact that the join is imperfect as we want the perm object to be human readable, but contains the id.

I think I like the approach here, made possible by the fact that we now have SupersetSecurityManager and can specify rolemodelview. I can see how we'd move to do pattern for Databases, DruidDatasource, SqlaTable at some point. I'm not sure what others may think about this approach.

I'm a fan of proper foreign keys in join tables over FKs in permisssion names - it makes the data model more explicit and we can more easily enforce data consistency.

micimize · 2019-08-01T16:27:22Z

@jasnovak @mistercrunch Should this be in PRs Status "Changes Requested"?

What needs to happen to get this merged? Can we just get it without the early all_dashboard_access check, and optimize later?

It's an important feature for extending superset in any not-purely-internal scenario

jay-finn · 2020-04-08T21:23:27Z

This is a much needed feature.
@jasnovak @mistercrunch are there any plans to merge this soon?
Is there a workaround for this in Superset 0.35.1 to provide users access to specific dashboards?

kalimuthu123

expecting this feature

bkyryliuk · 2020-06-24T17:23:39Z

It would be great to have some clarity on how will dashboard access work with schema / datasource access. Maybe update the description to cover those use cases.
Also will the user be able to explore from the dashboard if has dashboard access but not table access ?

willbarrett · 2020-06-24T20:29:27Z

superset/migrations/versions/82c2867e532b_.py

+
+
+def upgrade():
+    pass


If this migration is no-op, let's remove it before merge.

willbarrett · 2020-06-24T20:29:41Z

superset/migrations/versions/a6bdbfe85b40_.py

+
+
+def upgrade():
+    pass


Same with this migration.

willbarrett · 2020-06-24T20:31:16Z

superset/superset_rmv.py

+from flask_babel import lazy_gettext as _
+
+
+class SupersetRoleModelView(RoleModelView):


Let's update the file name to remove the abbreviation (superset_role_model_view.py rather than superset_rmv.py. Clarity > Brevity.

willbarrett · 2020-06-24T20:35:25Z

tests/dashboard_tests.py

@@ -266,6 +274,157 @@ def test_dashboard_with_created_by_can_be_accessed_by_public_users(self):

        assert 'Births' in self.get_resp('/superset/dashboard/births/')

+    def test_dashboard_level_access_controls(self):


Let's break up this test. There's a lot here, and additional test methods would make it clearer what's being tested at each point.

willbarrett · 2020-06-24T20:35:44Z

tests/dashboard_tests.py

+        security_manager.add_permission_role(gamma_role, perm_view)
+
+        # Gamma role has all_dashboard_access by default
+        # check that Gamma user can access all dashboards


Comments like this are a great place to break up test cases.

willbarrett · 2020-06-24T20:37:58Z

superset/superset_rmv.py

@@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+# pylint: disable=C,R,W


Please do not disable pylint on new files.

willbarrett · 2020-06-24T20:39:10Z

superset/views/core.py

@@ -96,6 +96,13 @@ def get_datasource_access_error_msg(datasource_name):
              '`all_datasource_access` permission', name=datasource_name)


+def get_dashboard_access_error_msg(dashboard_title):


Let's put free-floating functions in utils.py - this module is already enormous.

junlincc · 2021-01-31T17:37:54Z

@srinify and I are trying to clear some old PRs in the repo. @jasnovak believe that this PR is no longer active. some of the functionality is also covered by #12680 @amitmiran137.
Is it alright to close? Thanks for your contribution!

amitmiran137 · 2021-01-31T17:51:33Z

Yes please close this one 😸

junlincc · 2021-01-31T17:54:19Z

closing, in favor of #12680

jasnovak added 3 commits May 10, 2018 12:03

add dashboard level access control

1caa0d4

conform to flake8

ae284d8

Add tests. Add all_dashboard_access to Gamma.

ec3e4e4

jasnovak force-pushed the dash-access-control branch from 50e5d09 to ec3e4e4 Compare May 29, 2018 21:55

Fix flake8 issues. Merge alembic heads.

d700e01

jasnovak force-pushed the dash-access-control branch from d1bbdaf to d700e01 Compare May 29, 2018 22:26

jasnovak added 5 commits May 29, 2018 16:01

Merge branch 'master' of https://github.com/apache/incubator-superset …

f66a5ea

…into dash-access-control

Merge alembic heads

0874737

updating celery due to issue with kombu

db3c854

Fix syntax for python 3

90e8d01

fix format string

2194a3d

mistercrunch reviewed May 30, 2018

View reviewed changes

jasnovak added 4 commits May 30, 2018 09:25

Remove unused code left over from merge. rolemodelview is defined in …

b7da648

…security.py

fix flake8, update tests & security to SupersetRoleModelView

0147c3c

Merge branch 'master' of https://github.com/apache/incubator-superset …

f375a8b

…into dash-access-control

fix migration multi-heads, add back SupersetRoleModelView to core tests

94a1ad4

mistercrunch reviewed Jun 11, 2018

View reviewed changes

This was referenced Nov 30, 2018

there are no grain dashboard-level controls avaliable #2445

Closed

"Dashboard level access control" is really really needed ! #6467

Closed

kristw added enhancement:request Enhancement request submitted by anyone from the community .security labels Feb 7, 2019

amitmiran137 mentioned this pull request Jun 1, 2020

Dashboard access permission #9938

Closed

kalimuthu123 approved these changes Jun 24, 2020

View reviewed changes

willbarrett requested changes Jun 24, 2020

View reviewed changes

junlincc closed this Jan 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard level access control #5099

Dashboard level access control #5099

jasnovak commented May 29, 2018

mistercrunch May 30, 2018

jasnovak May 30, 2018

mistercrunch May 30, 2018

jasnovak May 30, 2018

mistercrunch Jun 1, 2018

mistercrunch Jun 11, 2018

willbarrett Jun 24, 2020

micimize commented Aug 1, 2019

jay-finn commented Apr 8, 2020

kalimuthu123 left a comment

bkyryliuk commented Jun 24, 2020

willbarrett Jun 24, 2020

willbarrett Jun 24, 2020

willbarrett Jun 24, 2020

willbarrett Jun 24, 2020

willbarrett Jun 24, 2020

willbarrett Jun 24, 2020

willbarrett Jun 24, 2020

junlincc commented Jan 31, 2021

amitmiran137 commented Jan 31, 2021

junlincc commented Jan 31, 2021

		self.remove_dashboard_role(role, dash)


		class SupersetSecurityManager(SecurityManager):

		from flask_babel import lazy_gettext as _


		class SupersetRoleModelView(RoleModelView):

		@@ -266,6 +274,157 @@ def test_dashboard_with_created_by_can_be_accessed_by_public_users(self):

		assert 'Births' in self.get_resp('/superset/dashboard/births/')

		def test_dashboard_level_access_controls(self):

		@@ -0,0 +1,62 @@
		# -- coding: utf-8 --
		# pylint: disable=C,R,W

		@@ -96,6 +96,13 @@ def get_datasource_access_error_msg(datasource_name):
		'`all_datasource_access` permission', name=datasource_name)


		def get_dashboard_access_error_msg(dashboard_title):

Dashboard level access control #5099

Dashboard level access control #5099

Conversation

jasnovak commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micimize commented Aug 1, 2019

jay-finn commented Apr 8, 2020

kalimuthu123 left a comment

Choose a reason for hiding this comment

bkyryliuk commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junlincc commented Jan 31, 2021

amitmiran137 commented Jan 31, 2021

junlincc commented Jan 31, 2021