You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by dp...@apache.org on 2020/12/15 11:27:36 UTC
[incubator-superset] branch master updated: feat(charts): security
perm simplification (#11981)
This is an automated email from the ASF dual-hosted git repository.
dpgaspar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new f79e52f feat(charts): security perm simplification (#11981)
f79e52f is described below
commit f79e52f48eb462fc7327b05b9c52b8197b395dad
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Tue Dec 15 11:27:06 2020 +0000
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
---
.../views/CRUD/chart/ChartList_spec.jsx | 2 +-
.../src/views/CRUD/chart/ChartCard.tsx | 6 +-
.../src/views/CRUD/chart/ChartList.tsx | 8 +-
superset/charts/api.py | 5 +-
superset/constants.py | 7 ++
.../ccb74baaa89b_security_converge_charts.py | 87 ++++++++++++++++++++++
superset/views/chart/views.py | 4 +-
superset/views/core.py | 6 +-
tests/charts/api_tests.py | 14 ++++
tests/security_tests.py | 32 ++------
10 files changed, 131 insertions(+), 40 deletions(-)
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 1ba7340..33f2385 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 8df4f02..6f7bc81 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 = (
<Menu>
diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx
index 1e07c03..79bb22c 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 50245be..9611a26 100644
--- a/superset/charts/api.py
+++ b/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",
diff --git a/superset/constants.py b/superset/constants.py
index 167e128..16cd4e6 100644
--- a/superset/constants.py
+++ b/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",
}
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 0000000..d025cd5
--- /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 301d89c..b87a2d8 100644
--- a/superset/views/chart/views.py
+++ b/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)
diff --git a/superset/views/core.py b/superset/views/core.py
index 63591b7..2ec0d3a 100755
--- a/superset/views/core.py
+++ b/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)
diff --git a/tests/charts/api_tests.py b/tests/charts/api_tests.py
index 092c354..a374c47 100644
--- a/tests/charts/api_tests.py
+++ b/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:
diff --git a/tests/security_tests.py b/tests/security_tests.py
index 13c8012..4ef6392 100644
--- a/tests/security_tests.py
+++ b/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)