diff --git a/superset-frontend/spec/javascripts/views/CRUD/chart/ChartList_spec.jsx b/superset-frontend/spec/javascripts/views/CRUD/chart/ChartList_spec.jsx index 1ba734074689b..33f2385dabd51 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/chart/ChartList_spec.jsx +++ b/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, { diff --git a/superset-frontend/src/views/CRUD/chart/ChartCard.tsx b/superset-frontend/src/views/CRUD/chart/ChartCard.tsx index 8df4f02b8fbaf..6f7bc8127c187 100644 --- a/superset-frontend/src/views/CRUD/chart/ChartCard.tsx +++ b/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 = ( diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx index 48b8701f717e5..a73d2974b3e9d 100644 --- a/superset-frontend/src/views/CRUD/chart/ChartList.tsx +++ b/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[]) { diff --git a/superset/charts/api.py b/superset/charts/api.py index 50245be3a016f..9611a26fef219 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -64,7 +64,7 @@ ) 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", diff --git a/superset/constants.py b/superset/constants.py index 167e128676177..16cd4e64ab829 100644 --- a/superset/constants.py +++ b/superset/constants.py @@ -95,4 +95,11 @@ class RouteMethod: # pylint: disable=too-few-public-methods "post": "write", "put": "write", "related": "read", + "favorite_status": "write", + "import_": "write", + "cache_screenshot": "read", + "screenshot": "read", + "data": "read", + "thumbnail": "read", + "data_from_cache": "read", } diff --git a/superset/migrations/versions/ccb74baaa89b_security_converge_charts.py b/superset/migrations/versions/ccb74baaa89b_security_converge_charts.py new file mode 100644 index 0000000000000..d025cd5762f98 --- /dev/null +++ b/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 diff --git a/superset/views/chart/views.py b/superset/views/chart/views.py index 301d89c87451b..b87a2d801f1a1 100644 --- a/superset/views/chart/views.py +++ b/superset/views/chart/views.py @@ -22,7 +22,7 @@ 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) diff --git a/superset/views/core.py b/superset/views/core.py index 63591b73efb94..2ec0d3a104c47 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -722,11 +722,9 @@ def explore( # pylint: disable=too-many-locals,too-many-return-statements 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) diff --git a/tests/charts/api_tests.py b/tests/charts/api_tests.py index 092c354199b3f..a374c47baf8cf 100644 --- a/tests/charts/api_tests.py +++ b/tests/charts/api_tests.py @@ -182,6 +182,20 @@ def add_dashboard_to_chart(self): 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: diff --git a/tests/security_tests.py b/tests/security_tests.py index 13c80127583fb..4ef63922f7c80 100644 --- a/tests/security_tests.py +++ b/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", "Chart") def get_perm_tuples(role_name): @@ -647,7 +647,7 @@ def assert_can_gamma(self, perm_set): 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 @@ 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("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)