feat(charts): security perm simplification (#11981)

* feat(charts): security perm simplification

* fix superset explore

* fix JS test

* fix cypress test

* fix split heads

* fix favorite permission

* fix permission

* update with new async permission

* fix new permission coming from master

* fix core permission assert

* black

* update alembic down revision

superset-frontend/spec/javascripts/views/CRUD/chart/ChartList_spec.jsx

@@ -59,7 +59,7 @@ const mockUser = {
 };
 
 fetchMock.get(chartsInfoEndpoint, {
-  permissions: ['can_list', 'can_edit', 'can_delete'],
+  permissions: ['can_read', 'can_write'],
 });
 
 fetchMock.get(chartssOwnersEndpoint, {

superset-frontend/src/views/CRUD/chart/ChartCard.tsx

@@ -59,10 +59,10 @@ export default function ChartCard({
   chartFilter,
   userId,
 }: ChartCardProps) {
-  const canEdit = hasPerm('can_edit');
-  const canDelete = hasPerm('can_delete');
+  const canEdit = hasPerm('can_write');
+  const canDelete = hasPerm('can_write');
   const canExport =
-    hasPerm('can_mulexport') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
+    hasPerm('can_read') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
 
   const menu = (
     <Menu>

superset-frontend/src/views/CRUD/chart/ChartList.tsx

@@ -152,11 +152,11 @@ function ChartList(props: ChartListProps) {
     refreshData();
   };
 
-  const canCreate = hasPerm('can_add');
-  const canEdit = hasPerm('can_edit');
-  const canDelete = hasPerm('can_delete');
+  const canCreate = hasPerm('can_write');
+  const canEdit = hasPerm('can_write');
+  const canDelete = hasPerm('can_write');
   const canExport =
-    hasPerm('can_mulexport') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
+    hasPerm('can_read') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
   const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];
 
   function handleBulkChartDelete(chartsToDelete: Chart[]) {

superset/charts/api.py

@@ -64,7 +64,7 @@ from superset.charts.schemas import (
 )
 from superset.commands.exceptions import CommandInvalidError
 from superset.commands.importers.v1.utils import remove_root
-from superset.constants import RouteMethod
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
 from superset.exceptions import SupersetSecurityException
 from superset.extensions import event_logger
 from superset.models.slice import Slice
@@ -104,7 +104,8 @@ class ChartRestApi(BaseSupersetModelRestApi):
         "viz_types",
         "favorite_status",
     }
-    class_permission_name = "SliceModelView"
+    class_permission_name = "Chart"
+    method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
     show_columns = [
         "cache_timeout",
         "dashboards.dashboard_title",

superset/constants.py

@@ -95,4 +95,11 @@ MODEL_API_RW_METHOD_PERMISSION_MAP = {
     "post": "write",
     "put": "write",
     "related": "read",
+    "favorite_status": "write",
+    "import_": "write",
+    "cache_screenshot": "read",
+    "screenshot": "read",
+    "data": "read",
+    "thumbnail": "read",
+    "data_from_cache": "read",
 }

superset/migrations/versions/ccb74baaa89b_security_converge_charts.py

@@ -0,0 +1,87 @@
+# 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 charts
+
+Revision ID: ccb74baaa89b
+Revises: 811494c0cc23
+Create Date: 2020-12-09 14:13:48.058003
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "ccb74baaa89b"
+down_revision = "40f16acf1ba7"
+
+
+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 = {"Chart": ("can_read", "can_write",)}
+PVM_MAP = {
+    Pvm("SliceModelView", "can_list"): (Pvm("Chart", "can_read"),),
+    Pvm("SliceModelView", "can_show"): (Pvm("Chart", "can_read"),),
+    Pvm("SliceModelView", "can_edit",): (Pvm("Chart", "can_write"),),
+    Pvm("SliceModelView", "can_delete",): (Pvm("Chart", "can_write"),),
+    Pvm("SliceModelView", "can_add",): (Pvm("Chart", "can_write"),),
+    Pvm("SliceModelView", "can_download",): (Pvm("Chart", "can_read"),),
+    Pvm("SliceModelView", "muldelete",): (Pvm("Chart", "can_write"),),
+    Pvm("SliceModelView", "can_mulexport",): (Pvm("Chart", "can_read"),),
+    Pvm("SliceModelView", "can_favorite_status",): (Pvm("Chart", "can_read"),),
+    Pvm("SliceModelView", "can_cache_screenshot",): (Pvm("Chart", "can_read"),),
+    Pvm("SliceModelView", "can_screenshot",): (Pvm("Chart", "can_read"),),
+    Pvm("SliceModelView", "can_data_from_cache",): (Pvm("Chart", "can_read"),),
+    Pvm("SliceAsync", "can_list",): (Pvm("Chart", "can_read"),),
+    Pvm("SliceAsync", "muldelete",): (Pvm("Chart", "can_write"),),
+}
+
+
+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/views/chart/views.py

@@ -22,7 +22,7 @@ from flask_babel import lazy_gettext as _
 
 from superset import db, is_feature_enabled
 from superset.connectors.connector_registry import ConnectorRegistry
-from superset.constants import RouteMethod
+from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod
 from superset.models.slice import Slice
 from superset.typing import FlaskResponse
 from superset.utils import core as utils
@@ -45,6 +45,8 @@ class SliceModelView(
         RouteMethod.API_READ,
         RouteMethod.API_DELETE,
     }
+    class_permission_name = "Chart"
+    method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP
 
     def pre_add(self, item: "SliceModelView") -> None:
         utils.validate_json(item.params)

superset/views/core.py

@@ -722,11 +722,9 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
             return redirect(datasource.default_endpoint)
 
         # slc perms
-        slice_add_perm = security_manager.can_access("can_add", "SliceModelView")
+        slice_add_perm = security_manager.can_access("can_write", "Chart")
         slice_overwrite_perm = is_owner(slc, g.user) if slc else False
-        slice_download_perm = security_manager.can_access(
-            "can_download", "SliceModelView"
-        )
+        slice_download_perm = security_manager.can_access("can_read", "Chart")
 
         form_data["datasource"] = str(datasource_id) + "__" + cast(str, datasource_type)
 

tests/charts/api_tests.py

@@ -182,6 +182,20 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin):
             db.session.delete(self.chart)
             db.session.commit()
 
+    def test_info_security_chart(self):
+        """
+        Chart API: Test info security
+        """
+        self.login(username="admin")
+        params = {"keys": ["permissions"]}
+        uri = f"api/v1/chart/_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 create_chart_import(self):
         buf = BytesIO()
         with ZipFile(buf, "w") as bundle:

tests/security_tests.py

@@ -48,7 +48,7 @@ from .dashboard_utils import (
 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", "Chart")
 
 
 def get_perm_tuples(role_name):
@@ -647,7 +647,7 @@ class TestRolePermission(SupersetTestCase):
         self.assert_can_read("TableModelView", perm_set)
 
         # make sure that user can create slices and dashboards
-        self.assert_can_all("SliceModelView", perm_set)
+        self.assert_can_all("Chart", perm_set)
         self.assert_can_all("DashboardModelView", perm_set)
 
         self.assertIn(("can_add_slices", "Superset"), perm_set)
@@ -832,37 +832,19 @@ class TestRolePermission(SupersetTestCase):
         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("TableModelView", gamma_perm_set)
 
         # make sure that user can create slices and dashboards
-        assert_can_all("SliceModelView")
-        assert_can_all("DashboardModelView")
+        self.assert_can_all("Chart", 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)