From 80f2774e7ac1c8ec401093d8899836862ae0c634 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 14 Dec 2020 11:20:01 +0000 Subject: [PATCH 1/3] feat(databases): security perm simplification --- .../views/CRUD/data/database/DatabaseList.tsx | 8 +- superset/constants.py | 6 ++ superset/databases/api.py | 5 +- ...2b4c9e01447_security_converge_databases.py | 82 +++++++++++++++++++ superset/views/database/views.py | 6 +- tests/databases/api_tests.py | 18 +++- 6 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 superset/migrations/versions/42b4c9e01447_security_converge_databases.py diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx index fea9ad4393d26..b44aee250dc70 100644 --- a/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx +++ b/superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx @@ -142,11 +142,11 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) { setDatabaseModalOpen(true); } - 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 menuData: SubMenuProps = { activeChild: 'Databases', diff --git a/superset/constants.py b/superset/constants.py index 167e128676177..ee8de3be2c2c1 100644 --- a/superset/constants.py +++ b/superset/constants.py @@ -95,4 +95,10 @@ class RouteMethod: # pylint: disable=too-few-public-methods "post": "write", "put": "write", "related": "read", + "import_": "read", + "related_objects": "read", + "schemas": "read", + "select_star": "read", + "table_metadata": "read", + "test_connection": "read", } diff --git a/superset/databases/api.py b/superset/databases/api.py index 89abb27f0bc9d..33f9f38bef0df 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -38,7 +38,7 @@ from superset import event_logger 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.databases.commands.create import CreateDatabaseCommand from superset.databases.commands.delete import DeleteDatabaseCommand from superset.databases.commands.exceptions import ( @@ -92,8 +92,9 @@ class DatabaseRestApi(BaseSupersetModelRestApi): "test_connection", "related_objects", } - class_permission_name = "DatabaseView" resource_name = "database" + class_permission_name = "Database" + method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP allow_browser_login = True base_filters = [["id", DatabaseFilter, lambda: []]] show_columns = [ diff --git a/superset/migrations/versions/42b4c9e01447_security_converge_databases.py b/superset/migrations/versions/42b4c9e01447_security_converge_databases.py new file mode 100644 index 0000000000000..95e81fb1026cc --- /dev/null +++ b/superset/migrations/versions/42b4c9e01447_security_converge_databases.py @@ -0,0 +1,82 @@ +# 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 databases + +Revision ID: 42b4c9e01447 +Revises: 5daced1f0e76 +Create Date: 2020-12-14 10:49:36.110805 + +""" + +# revision identifiers, used by Alembic. +revision = "42b4c9e01447" +down_revision = "5daced1f0e76" + +import sqlalchemy as sa +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 = {"Database": ("can_read", "can_write",)} +PVM_MAP = { + Pvm("DatabaseView", "can_add"): (Pvm("Database", "can_write"),), + Pvm("DatabaseView", "can_delete"): (Pvm("Database", "can_write"),), + Pvm("DatabaseView", "can_edit",): (Pvm("Database", "can_write"),), + Pvm("DatabaseView", "can_list",): (Pvm("Database", "can_read"),), + Pvm("DatabaseView", "can_mulexport",): (Pvm("Database", "can_read"),), + Pvm("DatabaseView", "can_post",): (Pvm("Database", "can_write"),), + Pvm("DatabaseView", "can_show",): (Pvm("Database", "can_read"),), + Pvm("DatabaseView", "muldelete",): (Pvm("Database", "can_write"),), + Pvm("DatabaseView", "yaml_export",): (Pvm("Database", "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 diff --git a/superset/views/database/views.py b/superset/views/database/views.py index 1a418dbfbb275..3a68f32588d99 100644 --- a/superset/views/database/views.py +++ b/superset/views/database/views.py @@ -30,7 +30,7 @@ import superset.models.core as models from superset import app, db, is_feature_enabled from superset.connectors.sqla.models import SqlaTable -from superset.constants import RouteMethod +from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.exceptions import CertificateException from superset.sql_parse import Table from superset.typing import FlaskResponse @@ -81,6 +81,10 @@ class DatabaseView( DatabaseMixin, SupersetModelView, DeleteMixin, YamlExportMixin ): # pylint: disable=too-many-ancestors datamodel = SQLAInterface(models.Database) + + class_permission_name = "Database" + method_permission_name = MODEL_VIEW_RW_METHOD_PERMISSION_MAP + include_route_methods = RouteMethod.CRUD_SET add_template = "superset/models/database/add.html" diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 8999fb605bcae..f28f138ea4841 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -562,6 +562,20 @@ def test_get_table_metadata(self): self.assertTrue(len(response["columns"]) > 5) self.assertTrue(response.get("selectStar").startswith("SELECT")) + def test_info_security_database(self): + """ + Database API: Test info security + """ + self.login(username="admin") + params = {"keys": ["permissions"]} + uri = f"api/v1/database/_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_get_invalid_database_table_metadata(self): """ Database API: Test get invalid database from table metadata @@ -874,7 +888,9 @@ def test_export_database_not_allowed(self): argument = [database.id] uri = f"api/v1/database/export/?q={prison.dumps(argument)}" rv = self.client.get(uri) - assert rv.status_code == 401 + # export only requires can_read now, but gamma need to have explicit access to + # view the database + assert rv.status_code == 404 def test_export_database_non_existing(self): """ From b84af3e2492ebd8b84c266421cc8d30b5e25f57b Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 14 Dec 2020 13:32:40 +0000 Subject: [PATCH 2/3] fix tests --- tests/security_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/security_tests.py b/tests/security_tests.py index 13c80127583fb..44b7669fdfd0d 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", "Database") def get_perm_tuples(role_name): @@ -694,7 +694,7 @@ def assert_cannot_alpha(self, perm_set): self.assert_cannot_write("UserDBModelView", perm_set) def assert_can_admin(self, perm_set): - self.assert_can_all("DatabaseView", perm_set) + self.assert_can_all("Database", perm_set) self.assert_can_all("RoleModelView", perm_set) self.assert_can_all("UserDBModelView", perm_set) From b8a9a2ada5f34c21292237a4934f99ad1cf467be Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 14 Dec 2020 15:55:23 +0000 Subject: [PATCH 3/3] fix JS tests --- .../javascripts/views/CRUD/data/database/DatabaseList_spec.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseList_spec.jsx b/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseList_spec.jsx index 52d129c310e7f..96639df9bd642 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseList_spec.jsx +++ b/superset-frontend/spec/javascripts/views/CRUD/data/database/DatabaseList_spec.jsx @@ -62,7 +62,7 @@ const mockUser = { }; fetchMock.get(databasesInfoEndpoint, { - permissions: ['can_delete'], + permissions: ['can_write'], }); fetchMock.get(databasesEndpoint, { result: mockdatabases,