feat(datasets): security perm simplification

superset-frontend/spec/javascripts/views/CRUD/data/dataset/DatasetList_spec.jsx

@@ -58,7 +58,7 @@ const mockUser = {
 };
 
 fetchMock.get(datasetsInfoEndpoint, {
-  permissions: ['can_list', 'can_edit', 'can_add', 'can_delete'],
+  permissions: ['can_read', 'can_write'],
 });
 fetchMock.get(datasetsOwnersEndpoint, {
   result: [],

superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx

@@ -141,10 +141,10 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
     refreshData();
   };
 
-  const canEdit = hasPerm('can_edit');
-  const canDelete = hasPerm('can_delete');
-  const canCreate = hasPerm('can_add');
-  const canExport = hasPerm('can_mulexport');
+  const canEdit = hasPerm('can_write');
+  const canDelete = hasPerm('can_write');
+  const canCreate = hasPerm('can_write');
+  const canExport = hasPerm('can_read');
 
   const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
 

superset/connectors/sqla/views.py

@@ -33,7 +33,7 @@
 from superset import app, db, is_feature_enabled
 from superset.connectors.base.views import DatasourceModelView
 from superset.connectors.sqla import models
-from superset.constants import RouteMethod
+from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
 from superset.typing import FlaskResponse
 from superset.utils import core as utils
 from superset.views.base import (
@@ -55,6 +55,8 @@ class TableColumnInlineView(  # pylint: disable=too-many-ancestors
 ):
     datamodel = SQLAInterface(models.TableColumn)
     # TODO TODO, review need for this on related_views
+    class_permission_name = "Dataset"
+    method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP
     include_route_methods = RouteMethod.RELATED_VIEW_SET | RouteMethod.API_SET
 
     list_title = _("Columns")
@@ -174,6 +176,8 @@ class SqlMetricInlineView(  # pylint: disable=too-many-ancestors
     CompactCRUDMixin, SupersetModelView
 ):
     datamodel = SQLAInterface(models.SqlMetric)
+    class_permission_name = "Dataset"
+    method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP
     include_route_methods = RouteMethod.RELATED_VIEW_SET | RouteMethod.API_SET
 
     list_title = _("Metrics")
@@ -327,6 +331,8 @@ class TableModelView(  # pylint: disable=too-many-ancestors
     DatasourceModelView, DeleteMixin, YamlExportMixin
 ):
     datamodel = SQLAInterface(models.SqlaTable)
+    class_permission_name = "Dataset"
+    method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP
     include_route_methods = RouteMethod.CRUD_SET
 
     list_title = _("Tables")

superset/constants.py

@@ -82,6 +82,7 @@ class RouteMethod:  # pylint: disable=too-few-public-methods
     "list": "read",
     "muldelete": "write",
     "show": "read",
+    "yaml_export": "read",
 }
 
 MODEL_API_RW_METHOD_PERMISSION_MAP = {
@@ -95,4 +96,7 @@ class RouteMethod:  # pylint: disable=too-few-public-methods
     "post": "write",
     "put": "write",
     "related": "read",
+    "refresh": "read",
+    "related_objects": "read",
+    "import_": "write",
 }

superset/datasets/api.py

@@ -33,7 +33,7 @@
 from superset.commands.exceptions import CommandInvalidError
 from superset.commands.importers.v1.utils import remove_root
 from superset.connectors.sqla.models import SqlaTable
-from superset.constants import RouteMethod
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
 from superset.databases.filters import DatabaseFilter
 from superset.datasets.commands.bulk_delete import BulkDeleteDatasetCommand
 from superset.datasets.commands.create import CreateDatasetCommand
@@ -79,7 +79,8 @@ class DatasetRestApi(BaseSupersetModelRestApi):
 
     resource_name = "dataset"
     allow_browser_login = True
-    class_permission_name = "TableModelView"
+    class_permission_name = "Dataset"
+    method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
     include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {
         RouteMethod.EXPORT,
         RouteMethod.IMPORT,

superset/migrations/versions/45731db65d9c_security_converge_datasets.py

@@ -0,0 +1,91 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""security converge datasets
+
+Revision ID: 45731db65d9c
+Revises: 5daced1f0e76
+Create Date: 2020-12-10 15:05:44.928020
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "45731db65d9c"
+down_revision = "5daced1f0e76"
+
+from alembic import op
+from sqlalchemy.exc import SQLAlchemyError
+from sqlalchemy.orm import Session
+
+from superset.migrations.shared.security_converge import (
+    add_pvms,
+    get_reversed_new_pvms,
+    get_reversed_pvm_map,
+    migrate_roles,
+    Pvm,
+)
+
+NEW_PVMS = {"Dataset": ("can_read", "can_write",)}
+PVM_MAP = {
+    Pvm("SqlMetricInlineView", "can_add"): (Pvm("Dataset", "can_write"),),
+    Pvm("SqlMetricInlineView", "can_delete"): (Pvm("Dataset", "can_write"),),
+    Pvm("SqlMetricInlineView", "can_edit"): (Pvm("Dataset", "can_write"),),
+    Pvm("SqlMetricInlineView", "can_list"): (Pvm("Dataset", "can_read"),),
+    Pvm("SqlMetricInlineView", "can_show"): (Pvm("Dataset", "can_read"),),
+    Pvm("TableColumnInlineView", "can_add"): (Pvm("Dataset", "can_write"),),
+    Pvm("TableColumnInlineView", "can_delete"): (Pvm("Dataset", "can_write"),),
+    Pvm("TableColumnInlineView", "can_edit"): (Pvm("Dataset", "can_write"),),
+    Pvm("TableColumnInlineView", "can_list"): (Pvm("Dataset", "can_read"),),
+    Pvm("TableColumnInlineView", "can_show"): (Pvm("Dataset", "can_read"),),
+    Pvm("TableModelView", "can_add",): (Pvm("Dataset", "can_write"),),
+    Pvm("TableModelView", "can_delete",): (Pvm("Dataset", "can_write"),),
+    Pvm("TableModelView", "can_edit",): (Pvm("Dataset", "can_write"),),
+    Pvm("TableModelView", "can_list"): (Pvm("Dataset", "can_read"),),
+    Pvm("TableModelView", "can_mulexport"): (Pvm("Dataset", "can_read"),),
+    Pvm("TableModelView", "can_show"): (Pvm("Dataset", "can_read"),),
+    Pvm("TableModelView", "muldelete",): (Pvm("Dataset", "can_write"),),
+    Pvm("TableModelView", "refresh",): (Pvm("Dataset", "can_write"),),
+    Pvm("TableModelView", "yaml_export",): (Pvm("Dataset", "can_read"),),
+}
+
+
+def upgrade():
+    bind = op.get_bind()
+    session = Session(bind=bind)
+
+    # Add the new permissions on the migration itself
+    add_pvms(session, NEW_PVMS)
+    migrate_roles(session, PVM_MAP)
+    try:
+        session.commit()
+    except SQLAlchemyError as ex:
+        print(f"An error occurred while upgrading permissions: {ex}")
+        session.rollback()
+
+
+def downgrade():
+    bind = op.get_bind()
+    session = Session(bind=bind)
+
+    # Add the old permissions on the migration itself
+    add_pvms(session, get_reversed_new_pvms(PVM_MAP))
+    migrate_roles(session, get_reversed_pvm_map(PVM_MAP))
+    try:
+        session.commit()
+    except SQLAlchemyError as ex:
+        print(f"An error occurred while downgrading permissions: {ex}")
+        session.rollback()
+    pass

superset/security/manager.py

@@ -120,9 +120,7 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
     }
 
     GAMMA_READ_ONLY_MODEL_VIEWS = {
-        "SqlMetricInlineView",
-        "TableColumnInlineView",
-        "TableModelView",
+        "Dataset",
         "DruidColumnInlineView",
         "DruidDatasourceModelView",
         "DruidMetricInlineView",
@@ -160,7 +158,13 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
         "all_query_access",
     }
 
-    READ_ONLY_PERMISSION = {"can_show", "can_list", "can_get", "can_external_metadata"}
+    READ_ONLY_PERMISSION = {
+        "can_show",
+        "can_list",
+        "can_get",
+        "can_external_metadata",
+        "can_read",
+    }
 
     ALPHA_ONLY_PERMISSIONS = {
         "muldelete",

tests/datasets/api_tests.py

@@ -336,6 +336,20 @@ def test_get_dataset_info(self):
         rv = self.get_assert_metric(uri, "info")
         assert rv.status_code == 200
 
+    def test_info_security_dataset(self):
+        """
+        Dataset API: Test info security
+        """
+        self.login(username="admin")
+        params = {"keys": ["permissions"]}
+        uri = f"api/v1/dataset/_info?q={prison.dumps(params)}"
+        rv = self.get_assert_metric(uri, "info")
+        data = json.loads(rv.data.decode("utf-8"))
+        assert rv.status_code == 200
+        assert "can_read" in data["permissions"]
+        assert "can_write" in data["permissions"]
+        assert len(data["permissions"]) == 2
+
     def test_create_dataset_item(self):
         """
         Dataset API: Test create dataset item
@@ -438,23 +452,23 @@ def test_create_dataset_item_owners_invalid(self):
         expected_result = {"message": {"owners": ["Owners are invalid"]}}
         assert data == expected_result
 
+    @pytest.mark.usefixtures("load_energy_table_with_slice")
     def test_create_dataset_validate_uniqueness(self):
         """
         Dataset API: Test create dataset validate table uniqueness
         """
-        example_db = get_example_database()
+        energy_usage_ds = self.get_energy_usage_dataset()
         self.login(username="admin")
         table_data = {
-            "database": example_db.id,
-            "schema": "",
-            "table_name": "birth_names",
+            "database": energy_usage_ds.database_id,
+            "table_name": energy_usage_ds.table_name,
         }
         uri = "api/v1/dataset/"
         rv = self.post_assert_metric(uri, table_data, "post")
         assert rv.status_code == 422
         data = json.loads(rv.data.decode("utf-8"))
         assert data == {
-            "message": {"table_name": ["Datasource birth_names already exists"]}
+            "message": {"table_name": ["Datasource energy_usage already exists"]}
         }
 
     def test_create_dataset_same_name_different_schema(self):
@@ -1086,7 +1100,7 @@ def test_export_dataset_gamma(self):
 
         self.login(username="gamma")
         rv = self.client.get(uri)
-        assert rv.status_code == 401
+        assert rv.status_code == 404
 
     @patch.dict(
         "superset.extensions.feature_flag_manager._feature_flags",
@@ -1147,8 +1161,8 @@ def test_export_dataset_bundle_gamma(self):
 
         self.login(username="gamma")
         rv = self.client.get(uri)
-
-        assert rv.status_code == 401
+        # gamma users by default do not have access to this dataset
+        assert rv.status_code == 404
 
     def test_get_dataset_related_objects(self):
         """

tests/security_tests.py

@@ -48,7 +48,7 @@
 from .fixtures.energy_dashboard import load_energy_table_with_slice
 from .fixtures.unicode_dashboard import load_unicode_dashboard_with_slice
 
-NEW_SECURITY_CONVERGE_VIEWS = ("CssTemplate", "SavedQuery")
+NEW_SECURITY_CONVERGE_VIEWS = ("CssTemplate", "SavedQuery", "Dataset")
 
 
 def get_perm_tuples(role_name):
@@ -644,7 +644,7 @@ def assert_can_menu(self, view_menu, permissions_set):
         self.assertIn(("menu_access", view_menu), permissions_set)
 
     def assert_can_gamma(self, perm_set):
-        self.assert_can_read("TableModelView", perm_set)
+        self.assert_can_read("Dataset", perm_set)
 
         # make sure that user can create slices and dashboards
         self.assert_can_all("SliceModelView", perm_set)
@@ -674,7 +674,7 @@ def assert_can_gamma(self, perm_set):
     def assert_can_alpha(self, perm_set):
         self.assert_can_all("AnnotationLayerModelView", perm_set)
         self.assert_can_all("CssTemplate", perm_set)
-        self.assert_can_all("TableModelView", perm_set)
+        self.assert_can_all("Dataset", perm_set)
         self.assert_can_read("QueryView", perm_set)
         self.assertIn(("can_import_dashboards", "Superset"), perm_set)
         self.assertIn(("can_this_form_post", "CsvToDatabaseView"), perm_set)
@@ -711,7 +711,7 @@ def assert_can_admin(self, perm_set):
     def test_is_admin_only(self):
         self.assertFalse(
             security_manager._is_admin_only(
-                security_manager.find_permission_view_menu("can_list", "TableModelView")
+                security_manager.find_permission_view_menu("can_read", "Dataset")
             )
         )
         self.assertFalse(
@@ -759,15 +759,13 @@ def test_is_admin_only(self):
     def test_is_alpha_only(self):
         self.assertFalse(
             security_manager._is_alpha_only(
-                security_manager.find_permission_view_menu("can_list", "TableModelView")
+                security_manager.find_permission_view_menu("can_read", "Dataset")
             )
         )
 
         self.assertTrue(
             security_manager._is_alpha_only(
-                security_manager.find_permission_view_menu(
-                    "muldelete", "TableModelView"
-                )
+                security_manager.find_permission_view_menu("can_write", "Dataset")
             )
         )
         self.assertTrue(
@@ -788,7 +786,7 @@ def test_is_alpha_only(self):
     def test_is_gamma_pvm(self):
         self.assertTrue(
             security_manager._is_gamma_pvm(
-                security_manager.find_permission_view_menu("can_list", "TableModelView")
+                security_manager.find_permission_view_menu("can_read", "Dataset")
             )
         )
 
@@ -832,37 +830,19 @@ def test_granter_permissions(self):
         self.assert_cannot_alpha(granter_set)
 
     def test_gamma_permissions(self):
-        def assert_can_read(view_menu):
-            self.assertIn(("can_list", view_menu), gamma_perm_set)
-
-        def assert_can_write(view_menu):
-            self.assertIn(("can_add", view_menu), gamma_perm_set)
-            self.assertIn(("can_delete", view_menu), gamma_perm_set)
-            self.assertIn(("can_edit", view_menu), gamma_perm_set)
-
-        def assert_cannot_write(view_menu):
-            self.assertNotIn(("can_add", view_menu), gamma_perm_set)
-            self.assertNotIn(("can_delete", view_menu), gamma_perm_set)
-            self.assertNotIn(("can_edit", view_menu), gamma_perm_set)
-            self.assertNotIn(("can_save", view_menu), gamma_perm_set)
-
-        def assert_can_all(view_menu):
-            assert_can_read(view_menu)
-            assert_can_write(view_menu)
-
         gamma_perm_set = set()
         for perm in security_manager.find_role("Gamma").permissions:
             gamma_perm_set.add((perm.permission.name, perm.view_menu.name))
 
         # check read only perms
-        assert_can_read("TableModelView")
+        self.assert_can_read("Dataset", gamma_perm_set)
 
         # make sure that user can create slices and dashboards
-        assert_can_all("SliceModelView")
-        assert_can_all("DashboardModelView")
+        self.assert_can_all("SliceModelView", gamma_perm_set)
+        self.assert_can_all("DashboardModelView", gamma_perm_set)
 
-        assert_cannot_write("UserDBModelView")
-        assert_cannot_write("RoleModelView")
+        self.assert_cannot_write("UserDBModelView", gamma_perm_set)
+        self.assert_cannot_write("RoleModelView", gamma_perm_set)
 
         self.assertIn(("can_add_slices", "Superset"), gamma_perm_set)
         self.assertIn(("can_copy_dash", "Superset"), gamma_perm_set)