[GitHub] [incubator-superset] willbarrett commented on a change in pull request #5099: Dashboard level access control

[GitBox <gi...@apache.org>]

willbarrett commented on a change in pull request #5099:
URL: https://github.com/apache/incubator-superset/pull/5099#discussion_r445153012



##########
File path: superset/migrations/versions/82c2867e532b_.py
##########
@@ -0,0 +1,22 @@
+"""empty message
+
+Revision ID: 82c2867e532b
+Revises: 76a4d742cf04
+Create Date: 2018-05-29 15:21:07.127779
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '82c2867e532b'
+down_revision = '76a4d742cf04'
+
+from alembic import op
+import sqlalchemy as sa
+
+
+def upgrade():
+    pass

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

##########
File path: superset/migrations/versions/a6bdbfe85b40_.py
##########
@@ -0,0 +1,22 @@
+"""empty message
+
+Revision ID: a6bdbfe85b40
+Revises: ('82c2867e532b', 'afb7730f6a9c')
+Create Date: 2018-06-11 15:11:43.426167
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = 'a6bdbfe85b40'
+down_revision = ('82c2867e532b', 'afb7730f6a9c')
+
+from alembic import op
+import sqlalchemy as sa
+
+
+def upgrade():
+    pass

Review comment:
       Same with this migration.

##########
File path: 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):

Review comment:
       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.

##########
File path: 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):
+        # add all_datasource_access to Gamma role
+        gamma_role = security_manager.find_role('Gamma')
+        perm_view = security_manager.find_permission_view_menu(
+            'all_datasource_access',
+            'all_datasource_access',
+        )
+        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

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

##########
File path: superset/superset_rmv.py
##########
@@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+# pylint: disable=C,R,W

Review comment:
       Please do not disable pylint on new files.

##########
File path: superset/superset_rmv.py
##########
@@ -0,0 +1,62 @@
+# -*- coding: utf-8 -*-
+# pylint: disable=C,R,W
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+from __future__ import unicode_literals
+
+import logging
+import re
+
+from flask_appbuilder.security.views import RoleModelView
+from flask_babel import lazy_gettext as _
+
+
+class SupersetRoleModelView(RoleModelView):

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

##########
File path: 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):

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org