From a34ffbe7a51ac933ba9f9b406a21f802010701e3 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 16 Sep 2022 11:36:07 +0100 Subject: [PATCH 1/6] fix: dataset after insert when db relation does not exist --- superset/security/manager.py | 44 ++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index f44924c3721d8..ca105f2239297 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1101,7 +1101,9 @@ def _update_vm_database_access( .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) + new_db_view_menu = self._find_view_menu_on_sqla_event( + connection, new_view_menu_name + ) self.on_view_menu_after_update(mapper, connection, new_db_view_menu) return new_db_view_menu @@ -1155,7 +1157,9 @@ def _update_vm_datasources_access( # pylint: disable=too-many-locals .values(name=new_dataset_vm_name) ) # After update refresh - new_dataset_view_menu = self.find_view_menu(new_dataset_vm_name) + new_dataset_view_menu = self._find_view_menu_on_sqla_event( + connection, new_dataset_vm_name + ) # Update dataset (SqlaTable perm field) connection.execute( @@ -1194,11 +1198,19 @@ def dataset_after_insert( :param target: The changed dataset object :return: """ + from superset.models.core import Database + try: dataset_perm = target.get_perm() + database = target.database except DatasetInvalidPermissionEvaluationException: - logger.warning("Dataset has no database refusing to set permission") - return + logger.warning( + "Dataset has no database will retry with database_id to set permission" + ) + database = self.get_session.query(Database).get(target.database_id) + dataset_perm = self.get_dataset_perm( + target.id, target.table_name, database.database_name + ) dataset_table = target.__table__ self._insert_pvm_on_sqla_event( @@ -1214,7 +1226,7 @@ def dataset_after_insert( if target.schema: dataset_schema_perm = self.get_schema_perm( - target.database.database_name, target.schema + database.database_name, target.schema ) self._insert_pvm_on_sqla_event( mapper, connection, "schema_access", dataset_schema_perm @@ -1484,12 +1496,23 @@ def _delete_pvm_on_sqla_event( # pylint: disable=too-many-arguments def _find_permission_on_sqla_event( self, connection: Connection, name: str ) -> Permission: + """ + Find a FAB Permission using a SQLA connection. + + A session.query may not return the latest results on newly created/updated + objects/rows using connection. On this case we should use a connection also + + :param connection: SQLAlchemy connection + :param name: The permission name (it's unique) + :return: Permission + """ permission_table = self.permission_model.__table__ # pylint: disable=no-member permission_ = connection.execute( permission_table.select().where(permission_table.c.name == name) ).fetchone() permission = Permission() + # ensures this object is never persisted permission.metadata = None permission.id = permission_.id permission.name = permission_.name @@ -1498,12 +1521,23 @@ def _find_permission_on_sqla_event( def _find_view_menu_on_sqla_event( self, connection: Connection, name: str ) -> ViewMenu: + """ + Find a FAB ViewMenu using a SQLA connection. + + A session.query may not return the latest results on newly created/updated + objects/rows using connection. On this case we should use a connection also + + :param connection: SQLAlchemy connection + :param name: The ViewMenu name (it's unique) + :return: ViewMenu + """ view_menu_table = self.viewmenu_model.__table__ # pylint: disable=no-member view_menu_ = connection.execute( view_menu_table.select().where(view_menu_table.c.name == name) ).fetchone() view_menu = ViewMenu() + # ensures this object is never persisted view_menu.metadata = None view_menu.id = view_menu_.id view_menu.name = view_menu_.name From 525cb487915de97b78a082ab438d78f086e7ed1d Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 16 Sep 2022 12:15:14 +0100 Subject: [PATCH 2/6] fix test --- superset/security/manager.py | 2 +- tests/integration_tests/security_tests.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index ca105f2239297..6305238caa18f 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1198,7 +1198,7 @@ def dataset_after_insert( :param target: The changed dataset object :return: """ - from superset.models.core import Database + from superset.models.core import Database # pylint disable=import-outside-toplevel try: dataset_perm = target.get_perm() diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 580e0b59cae77..651e6ad9d4ba5 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -106,6 +106,7 @@ class TestRolePermission(SupersetTestCase): """Testing export role permissions.""" def setUp(self): + return schema = get_example_default_schema() session = db.session security_manager.add_role(SCHEMA_ACCESS_ROLE) @@ -133,6 +134,7 @@ def setUp(self): session.commit() def tearDown(self): + return session = db.session ds = ( session.query(SqlaTable) @@ -249,7 +251,7 @@ def test_after_insert_dataset_table_none(self): session.query(SqlaTable).filter_by(table_name="tmp_perm_table").one() ) # Assert no permission is created - self.assertIsNone( + self.assertIsNotNone( security_manager.find_permission_view_menu( "datasource_access", stored_table.perm ) From eaebf1e77f64a79de4102b4fa2199dc59a316672 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 16 Sep 2022 13:59:06 +0100 Subject: [PATCH 3/6] fix test --- tests/integration_tests/security_tests.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 651e6ad9d4ba5..dcc3b4f7b2a9e 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -106,7 +106,6 @@ class TestRolePermission(SupersetTestCase): """Testing export role permissions.""" def setUp(self): - return schema = get_example_default_schema() session = db.session security_manager.add_role(SCHEMA_ACCESS_ROLE) @@ -134,7 +133,6 @@ def setUp(self): session.commit() def tearDown(self): - return session = db.session ds = ( session.query(SqlaTable) From 62bc59382288f3e557475d3674b6aa2bebdf7222 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 16 Sep 2022 15:38:17 +0100 Subject: [PATCH 4/6] lint --- superset/security/manager.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 6305238caa18f..07d783eb7ec81 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1198,7 +1198,9 @@ def dataset_after_insert( :param target: The changed dataset object :return: """ - from superset.models.core import Database # pylint disable=import-outside-toplevel + from superset.models.core import ( # pylint disable=import-outside-toplevel + Database, + ) try: dataset_perm = target.get_perm() From c3cf96ea7fe30a7698df410a65e229158dfec36e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 16 Sep 2022 18:31:27 +0100 Subject: [PATCH 5/6] lint --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 07d783eb7ec81..260dbb49ee7d4 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1198,7 +1198,7 @@ def dataset_after_insert( :param target: The changed dataset object :return: """ - from superset.models.core import ( # pylint disable=import-outside-toplevel + from superset.models.core import ( # pylint: disable=import-outside-toplevel Database, ) From e25513353aa4cdfc76001fd8499f592c825e3a55 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 16 Sep 2022 18:34:33 +0100 Subject: [PATCH 6/6] fix comment --- tests/integration_tests/security_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index dcc3b4f7b2a9e..58bfb36d69e63 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -248,7 +248,7 @@ def test_after_insert_dataset_table_none(self): stored_table = ( session.query(SqlaTable).filter_by(table_name="tmp_perm_table").one() ) - # Assert no permission is created + # Assert permission is created self.assertIsNotNone( security_manager.find_permission_view_menu( "datasource_access", stored_table.perm