You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2022/09/20 20:50:32 UTC
[superset] 22/29: fix: database permissions on update and delete (avoid orphaned perms) (#20081)
This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch 2.0-test
in repository https://gitbox.apache.org/repos/asf/superset.git
commit a9c04284c405b81749cbb40304f7f38b1769513e
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Tue Aug 2 18:28:46 2022 +0100
fix: database permissions on update and delete (avoid orphaned perms) (#20081)
* fix: database permissions on update and delete (avoid orphaned perms)
* fix event transaction
* fix test
* fix lint
* update datasource access permissions
* add tests
* fix import
* fix tests
* update slice and dataset perms also
* fix lint
* fix tests
* fix lint
* fix lint
* add test for edge case, small refactor
* add test for edge case, small refactor
* improve code
* fix lint
---
.../installing-superset-from-scratch.mdx | 2 +-
superset/databases/commands/create.py | 1 -
superset/databases/commands/update.py | 22 +-
superset/models/core.py | 3 +-
superset/security/manager.py | 279 ++++++++++++++++++++-
tests/integration_tests/security_tests.py | 180 +++++++++++++
6 files changed, 481 insertions(+), 6 deletions(-)
diff --git a/docs/docs/installation/installing-superset-from-scratch.mdx b/docs/docs/installation/installing-superset-from-scratch.mdx
index 3a12c9db3a..5efdb3e8f1 100644
--- a/docs/docs/installation/installing-superset-from-scratch.mdx
+++ b/docs/docs/installation/installing-superset-from-scratch.mdx
@@ -64,7 +64,7 @@ We don't recommend using the system installed Python. Instead, first install the
brew install readline pkg-config libffi openssl mysql postgres
```
-You should install a recent version of Python (the official docker image uses 3.8.12). We'd recommend using a Python version manager like [pyenv](https://github.com/pyenv/pyenv) (and also [pyenv-virtualenv](https://github.com/pyenv/pyenv-virtualenv)).
+You should install a recent version of Python (the official docker image uses 3.8.13). We'd recommend using a Python version manager like [pyenv](https://github.com/pyenv/pyenv) (and also [pyenv-virtualenv](https://github.com/pyenv/pyenv-virtualenv)).
Let's also make sure we have the latest version of `pip` and `setuptools`:
diff --git a/superset/databases/commands/create.py b/superset/databases/commands/create.py
index e91ccec45c..e798f115a5 100644
--- a/superset/databases/commands/create.py
+++ b/superset/databases/commands/create.py
@@ -65,7 +65,6 @@ class CreateDatabaseCommand(BaseCommand):
security_manager.add_permission_view_menu(
"schema_access", security_manager.get_schema_perm(database, schema)
)
- security_manager.add_permission_view_menu("database_access", database.perm)
db.session.commit()
except DAOCreateFailedError as ex:
db.session.rollback()
diff --git a/superset/databases/commands/update.py b/superset/databases/commands/update.py
index 69b6c30e71..30e67b79ca 100644
--- a/superset/databases/commands/update.py
+++ b/superset/databases/commands/update.py
@@ -46,10 +46,13 @@ class UpdateDatabaseCommand(BaseCommand):
def run(self) -> Model:
self.validate()
+ if not self._model:
+ raise DatabaseNotFoundError()
+ old_database_name = self._model.database_name
+
try:
database = DatabaseDAO.update(self._model, self._properties, commit=False)
database.set_sqlalchemy_uri(database.sqlalchemy_uri)
- security_manager.add_permission_view_menu("database_access", database.perm)
# adding a new database we always want to force refresh schema list
# TODO Improve this simplistic implementation for catching DB conn fails
try:
@@ -57,7 +60,24 @@ class UpdateDatabaseCommand(BaseCommand):
except Exception as ex:
db.session.rollback()
raise DatabaseConnectionFailedError() from ex
+ # Update database schema permissions
+ new_schemas: List[str] = []
for schema in schemas:
+ old_view_menu_name = security_manager.get_schema_perm(
+ old_database_name, schema
+ )
+ new_view_menu_name = security_manager.get_schema_perm(
+ database.database_name, schema
+ )
+ schema_pvm = security_manager.find_permission_view_menu(
+ "schema_access", old_view_menu_name
+ )
+ # Update the schema permission if the database name changed
+ if schema_pvm and old_database_name != database.database_name:
+ schema_pvm.view_menu.name = new_view_menu_name
+ else:
+ new_schemas.append(schema)
+ for schema in new_schemas:
security_manager.add_permission_view_menu(
"schema_access", security_manager.get_schema_perm(database, schema)
)
diff --git a/superset/models/core.py b/superset/models/core.py
index d21ac56dad..997759c8b1 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -795,7 +795,8 @@ class Database(
sqla.event.listen(Database, "after_insert", security_manager.set_perm)
-sqla.event.listen(Database, "after_update", security_manager.set_perm)
+sqla.event.listen(Database, "after_update", security_manager.database_after_update)
+sqla.event.listen(Database, "after_delete", security_manager.database_after_delete)
class Log(Model): # pylint: disable=too-few-public-methods
diff --git a/superset/security/manager.py b/superset/security/manager.py
index d5455e73fc..699c7472d5 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -40,9 +40,11 @@ from flask_appbuilder.security.sqla.manager import SecurityManager
from flask_appbuilder.security.sqla.models import (
assoc_permissionview_role,
assoc_user_role,
+ Permission,
PermissionView,
Role,
User,
+ ViewMenu,
)
from flask_appbuilder.security.views import (
PermissionModelView,
@@ -54,7 +56,7 @@ from flask_appbuilder.security.views import (
from flask_appbuilder.widgets import ListWidget
from flask_login import AnonymousUserMixin, LoginManager
from jwt.api_jwt import _jwt_global_obj
-from sqlalchemy import and_, or_
+from sqlalchemy import and_, inspect, or_
from sqlalchemy.engine.base import Connection
from sqlalchemy.orm import Session
from sqlalchemy.orm.mapper import Mapper
@@ -269,6 +271,14 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return None
+ @staticmethod
+ def get_database_perm(database_id: int, database_name: str) -> str:
+ return f"[{database_name}].(id:{database_id})"
+
+ @staticmethod
+ def get_dataset_perm(dataset_id: int, dataset_name: str, database_name: str) -> str:
+ return f"[{database_name}].[{dataset_name}](id:{dataset_id})"
+
def unpack_database_and_schema( # pylint: disable=no-self-use
self, schema_permission: str
) -> DatabaseAndSchema:
@@ -927,7 +937,271 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return pvm.permission.name in {"can_override_role_permissions", "can_approve"}
- def set_perm( # pylint: disable=unused-argument
+ def database_after_delete(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "Database",
+ ) -> None:
+ self._delete_vm_database_access(
+ mapper, connection, target.id, target.database_name
+ )
+
+ def _delete_vm_database_access(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ database_id: int,
+ database_name: str,
+ ) -> None:
+ view_menu_table = self.viewmenu_model.__table__ # pylint: disable=no-member
+ permission_view_menu_table = (
+ self.permissionview_model.__table__ # pylint: disable=no-member
+ )
+ view_menu_name = self.get_database_perm(database_id, database_name)
+ # Clean database access permission
+ db_pvm = self.find_permission_view_menu("database_access", view_menu_name)
+ if not db_pvm:
+ logger.warning(
+ "Could not find previous database permission %s",
+ view_menu_name,
+ )
+ return
+ connection.execute(
+ permission_view_menu_table.delete().where(
+ permission_view_menu_table.c.id == db_pvm.id
+ )
+ )
+ self.on_permission_after_delete(mapper, connection, db_pvm)
+ connection.execute(
+ view_menu_table.delete().where(view_menu_table.c.id == db_pvm.view_menu_id)
+ )
+
+ # Clean database schema permissions
+ schema_pvms = (
+ self.get_session.query(self.permissionview_model)
+ .join(self.permission_model)
+ .join(self.viewmenu_model)
+ .filter(self.permission_model.name == "schema_access")
+ .filter(self.viewmenu_model.name.like(f"[{database_name}].[%]"))
+ .all()
+ )
+ for schema_pvm in schema_pvms:
+ connection.execute(
+ permission_view_menu_table.delete().where(
+ permission_view_menu_table.c.id == schema_pvm.id
+ )
+ )
+ self.on_permission_after_delete(mapper, connection, schema_pvm)
+ connection.execute(
+ view_menu_table.delete().where(
+ view_menu_table.c.id == schema_pvm.view_menu_id
+ )
+ )
+
+ def _update_vm_database_access(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ old_database_name: str,
+ target: "Database",
+ ) -> Optional[ViewMenu]:
+ view_menu_table = self.viewmenu_model.__table__ # pylint: disable=no-member
+ new_database_name = target.database_name
+ old_view_menu_name = self.get_database_perm(target.id, old_database_name)
+ new_view_menu_name = self.get_database_perm(target.id, new_database_name)
+ db_pvm = self.find_permission_view_menu("database_access", old_view_menu_name)
+ if not db_pvm:
+ logger.warning(
+ "Could not find previous database permission %s",
+ old_view_menu_name,
+ )
+ return None
+ new_updated_pvm = self.find_permission_view_menu(
+ "database_access", new_view_menu_name
+ )
+ if new_updated_pvm:
+ logger.info(
+ "New permission [%s] already exists, deleting the previous",
+ new_view_menu_name,
+ )
+ self._delete_vm_database_access(
+ mapper, connection, target.id, old_database_name
+ )
+ return None
+ connection.execute(
+ view_menu_table.update()
+ .where(view_menu_table.c.id == db_pvm.view_menu_id)
+ .values(name=new_view_menu_name)
+ )
+ new_db_view_menu = self.find_view_menu(new_view_menu_name)
+
+ self.on_view_menu_after_update(mapper, connection, new_db_view_menu)
+ return new_db_view_menu
+
+ def _update_vm_datasources_access( # pylint: disable=too-many-locals
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ old_database_name: str,
+ target: "Database",
+ ) -> List[ViewMenu]:
+ """
+ Updates all datasource access permission when a database name changes
+
+ :param connection: Current connection (called on SQLAlchemy event listener scope)
+ :param old_database_name: the old database name
+ :param target: The new database name
+ :return: A list of changed view menus (permission resource names)
+ """
+ from superset.connectors.sqla.models import ( # pylint: disable=import-outside-toplevel
+ SqlaTable,
+ )
+ from superset.models.slice import ( # pylint: disable=import-outside-toplevel
+ Slice,
+ )
+
+ view_menu_table = self.viewmenu_model.__table__ # pylint: disable=no-member
+ sqlatable_table = SqlaTable.__table__ # pylint: disable=no-member
+ chart_table = Slice.__table__ # pylint: disable=no-member
+ new_database_name = target.database_name
+ datasets = (
+ self.get_session.query(SqlaTable)
+ .filter(SqlaTable.database_id == target.id)
+ .all()
+ )
+ updated_view_menus: List[ViewMenu] = []
+ for dataset in datasets:
+ old_dataset_vm_name = self.get_dataset_perm(
+ dataset.id, dataset.table_name, old_database_name
+ )
+ new_dataset_vm_name = self.get_dataset_perm(
+ dataset.id, dataset.table_name, new_database_name
+ )
+ new_dataset_view_menu = self.find_view_menu(new_dataset_vm_name)
+ if new_dataset_view_menu:
+ continue
+ connection.execute(
+ view_menu_table.update()
+ .where(view_menu_table.c.name == old_dataset_vm_name)
+ .values(name=new_dataset_vm_name)
+ )
+ # Update dataset (SqlaTable perm field)
+ connection.execute(
+ sqlatable_table.update()
+ .where(
+ sqlatable_table.c.id == dataset.id,
+ sqlatable_table.c.perm == old_dataset_vm_name,
+ )
+ .values(perm=new_dataset_vm_name)
+ )
+ # Update charts (Slice perm field)
+ connection.execute(
+ chart_table.update()
+ .where(chart_table.c.perm == old_dataset_vm_name)
+ .values(perm=new_dataset_vm_name)
+ )
+ self.on_view_menu_after_update(mapper, connection, new_dataset_view_menu)
+ updated_view_menus.append(self.find_view_menu(new_dataset_view_menu))
+ return updated_view_menus
+
+ def database_after_update(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "Database",
+ ) -> None:
+ # Check if database name has changed
+ state = inspect(target)
+ history = state.get_history("database_name", True)
+ if not history.has_changes() or not history.deleted:
+ return
+
+ old_database_name = history.deleted[0]
+ # update database access permission
+ self._update_vm_database_access(mapper, connection, old_database_name, target)
+ # update datasource access
+ self._update_vm_datasources_access(
+ mapper, connection, old_database_name, target
+ )
+
+ def on_view_menu_after_update(
+ self, mapper: Mapper, connection: Connection, target: ViewMenu
+ ) -> None:
+ """
+ Hook that allows for further custom operations when a new ViewMenu
+ is updated
+
+ Since the update may be performed on after_update event. We cannot
+ update ViewMenus using a session, so any SQLAlchemy events hooked to
+ `ViewMenu` will not trigger an after_update.
+
+ :param mapper: The table mapper
+ :param connection: The DB-API connection
+ :param target: The mapped instance being persisted
+ """
+
+ def on_permission_after_delete(
+ self, mapper: Mapper, connection: Connection, target: Permission
+ ) -> None:
+ """
+ Hook that allows for further custom operations when a permission
+ is deleted by sqlalchemy events.
+
+ :param mapper: The table mapper
+ :param connection: The DB-API connection
+ :param target: The mapped instance being persisted
+ """
+
+ def on_permission_after_insert(
+ self, mapper: Mapper, connection: Connection, target: Permission
+ ) -> None:
+ """
+ Hook that allows for further custom operations when a new permission
+ is created by set_perm.
+
+ Since set_perm is executed by SQLAlchemy after_insert events, we cannot
+ create new permissions using a session, so any SQLAlchemy events hooked to
+ `Permission` will not trigger an after_insert.
+
+ :param mapper: The table mapper
+ :param connection: The DB-API connection
+ :param target: The mapped instance being persisted
+ """
+
+ def on_view_menu_after_insert(
+ self, mapper: Mapper, connection: Connection, target: ViewMenu
+ ) -> None:
+ """
+ Hook that allows for further custom operations when a new ViewMenu
+ is created by set_perm.
+
+ Since set_perm is executed by SQLAlchemy after_insert events, we cannot
+ create new view_menu's using a session, so any SQLAlchemy events hooked to
+ `ViewMenu` will not trigger an after_insert.
+
+ :param mapper: The table mapper
+ :param connection: The DB-API connection
+ :param target: The mapped instance being persisted
+ """
+
+ def on_permission_view_after_insert(
+ self, mapper: Mapper, connection: Connection, target: PermissionView
+ ) -> None:
+ """
+ Hook that allows for further custom operations when a new PermissionView
+ is created by set_perm.
+
+ Since set_perm is executed by SQLAlchemy after_insert events, we cannot
+ create new pvms using a session, so any SQLAlchemy events hooked to
+ `PermissionView` will not trigger an after_insert.
+
+ :param mapper: The table mapper
+ :param connection: The DB-API connection
+ :param target: The mapped instance being persisted
+ """
+
+ def set_perm(
self, mapper: Mapper, connection: Connection, target: "BaseDatasource"
) -> None:
"""
@@ -951,6 +1225,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
)
target.perm = target_get_perm
+ # check schema perm for datasets
if (
hasattr(target, "schema_perm")
and target.schema_perm != target.get_schema_perm()
diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py
index 9de83f665a..25d946f9e5 100644
--- a/tests/integration_tests/security_tests.py
+++ b/tests/integration_tests/security_tests.py
@@ -332,6 +332,186 @@ class TestRolePermission(SupersetTestCase):
session.delete(stored_db)
session.commit()
+ def test_after_update_database__perm_database_access(self):
+ session = db.session
+ database = Database(database_name="tmp_database", sqlalchemy_uri="sqlite://")
+ session.add(database)
+ session.commit()
+ stored_db = (
+ session.query(Database).filter_by(database_name="tmp_database").one()
+ )
+
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "database_access", stored_db.perm
+ )
+ )
+
+ stored_db.database_name = "tmp_database2"
+ session.commit()
+
+ # Assert that the old permission was updated
+ self.assertIsNone(
+ security_manager.find_permission_view_menu(
+ "database_access", f"[tmp_database].(id:{stored_db.id})"
+ )
+ )
+ # Assert that the db permission was updated
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "database_access", f"[tmp_database2].(id:{stored_db.id})"
+ )
+ )
+ session.delete(stored_db)
+ session.commit()
+
+ def test_after_update_database__perm_database_access_exists(self):
+ session = db.session
+ # Add a bogus existing permission before the change
+
+ database = Database(database_name="tmp_database", sqlalchemy_uri="sqlite://")
+ session.add(database)
+ session.commit()
+ stored_db = (
+ session.query(Database).filter_by(database_name="tmp_database").one()
+ )
+ security_manager.add_permission_view_menu(
+ "database_access", f"[tmp_database2].(id:{stored_db.id})"
+ )
+
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "database_access", stored_db.perm
+ )
+ )
+
+ stored_db.database_name = "tmp_database2"
+ session.commit()
+
+ # Assert that the old permission was updated
+ self.assertIsNone(
+ security_manager.find_permission_view_menu(
+ "database_access", f"[tmp_database].(id:{stored_db.id})"
+ )
+ )
+ # Assert that the db permission was updated
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "database_access", f"[tmp_database2].(id:{stored_db.id})"
+ )
+ )
+ session.delete(stored_db)
+ session.commit()
+
+ def test_after_update_database__perm_datasource_access(self):
+ session = db.session
+ database = Database(database_name="tmp_database", sqlalchemy_uri="sqlite://")
+ session.add(database)
+ session.commit()
+
+ table1 = SqlaTable(
+ schema="tmp_schema",
+ table_name="tmp_table1",
+ database=database,
+ )
+ session.add(table1)
+ table2 = SqlaTable(
+ schema="tmp_schema",
+ table_name="tmp_table2",
+ database=database,
+ )
+ session.add(table2)
+ session.commit()
+ slice1 = Slice(
+ datasource_id=table1.id,
+ datasource_type=DatasourceType.TABLE,
+ datasource_name="tmp_table1",
+ slice_name="tmp_slice1",
+ )
+ session.add(slice1)
+ session.commit()
+ slice1 = session.query(Slice).filter_by(slice_name="tmp_slice1").one()
+ table1 = session.query(SqlaTable).filter_by(table_name="tmp_table1").one()
+ table2 = session.query(SqlaTable).filter_by(table_name="tmp_table2").one()
+
+ # assert initial perms
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "datasource_access", f"[tmp_database].[tmp_table1](id:{table1.id})"
+ )
+ )
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "datasource_access", f"[tmp_database].[tmp_table2](id:{table2.id})"
+ )
+ )
+ self.assertEqual(slice1.perm, f"[tmp_database].[tmp_table1](id:{table1.id})")
+ self.assertEqual(table1.perm, f"[tmp_database].[tmp_table1](id:{table1.id})")
+ self.assertEqual(table2.perm, f"[tmp_database].[tmp_table2](id:{table2.id})")
+
+ stored_db = (
+ session.query(Database).filter_by(database_name="tmp_database").one()
+ )
+ stored_db.database_name = "tmp_database2"
+ session.commit()
+
+ # Assert that the old permissions were updated
+ self.assertIsNone(
+ security_manager.find_permission_view_menu(
+ "datasource_access", f"[tmp_database].[tmp_table1](id:{table1.id})"
+ )
+ )
+ self.assertIsNone(
+ security_manager.find_permission_view_menu(
+ "datasource_access", f"[tmp_database].[tmp_table2](id:{table2.id})"
+ )
+ )
+
+ # Assert that the db permission was updated
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "datasource_access", f"[tmp_database2].[tmp_table1](id:{table1.id})"
+ )
+ )
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "datasource_access", f"[tmp_database2].[tmp_table2](id:{table2.id})"
+ )
+ )
+ self.assertEqual(slice1.perm, f"[tmp_database2].[tmp_table1](id:{table1.id})")
+ self.assertEqual(table1.perm, f"[tmp_database2].[tmp_table1](id:{table1.id})")
+ self.assertEqual(table2.perm, f"[tmp_database2].[tmp_table2](id:{table2.id})")
+
+ session.delete(slice1)
+ session.delete(table1)
+ session.delete(table2)
+ session.delete(stored_db)
+ session.commit()
+
+ def test_after_delete_database__perm_database_access(self):
+ session = db.session
+ database = Database(database_name="tmp_database", sqlalchemy_uri="sqlite://")
+ session.add(database)
+ session.commit()
+ stored_db = (
+ session.query(Database).filter_by(database_name="tmp_database").one()
+ )
+
+ self.assertIsNotNone(
+ security_manager.find_permission_view_menu(
+ "database_access", stored_db.perm
+ )
+ )
+ session.delete(stored_db)
+ session.commit()
+
+ # Assert that the old permission was updated
+ self.assertIsNone(
+ security_manager.find_permission_view_menu(
+ "database_access", f"[tmp_database].(id:{stored_db.id})"
+ )
+ )
+
def test_hybrid_perm_database(self):
database = Database(database_name="tmp_database3", sqlalchemy_uri="sqlite://")