From 641f3350e6e6a55be92579701916bd58f841d5bf Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 13 Sep 2022 11:41:16 +0100 Subject: [PATCH 1/6] fix: permission sqlalchemy events --- superset/security/manager.py | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index a8ffb7d03b2f6..2258b240ae81c 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1511,20 +1511,40 @@ def _insert_pvm_on_sqla_event( permission = self.find_permission(permission_name) view_menu = self.find_view_menu(view_menu_name) if not permission: - connection.execute(permission_table.insert().values(name=permission_name)) - permission = self.find_permission(permission_name) + _ = connection.execute( + permission_table.insert().values(name=permission_name) + ) + permission = connection.execute( + permission_table.select().where( + permission_table.c.name == permission_name + ) + ).fetchone() self.on_permission_after_insert(mapper, connection, permission) if not view_menu: - connection.execute(view_menu_table.insert().values(name=view_menu_name)) - view_menu = self.find_view_menu(view_menu_name) + _ = connection.execute(view_menu_table.insert().values(name=view_menu_name)) + view_menu = connection.execute( + view_menu_table.select().where(view_menu_table.c.name == view_menu_name) + ).fetchone() self.on_view_menu_after_insert(mapper, connection, view_menu) connection.execute( permission_view_table.insert().values( permission_id=permission.id, view_menu_id=view_menu.id ) ) - permission = self.find_permission_view_menu(permission_name, view_menu_name) - self.on_permission_view_after_insert(mapper, connection, permission) + permission_view = connection.execute( + permission_view_table.select().where( + permission_view_table.c.permission_id == permission.id, + permission_view_table.c.view_menu_id == view_menu.id, + ) + ).fetchone() + permission_view_model = PermissionView() + permission_view_model.metadata = None + permission_view_model.id = permission_view.id + permission_view_model.permission_id = permission.id + permission_view_model.view_menu_id = view_menu.id + permission_view_model.permission = permission + permission_view_model.view_menu = view_menu + self.on_permission_view_after_insert(mapper, connection, permission_view_model) def on_role_after_update( self, mapper: Mapper, connection: Connection, target: Role From f6d433e02a27bcf68c9367b9262949463fb2636f Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 13 Sep 2022 14:58:25 +0100 Subject: [PATCH 2/6] fix test --- superset/security/manager.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 2258b240ae81c..2182d2907f5e2 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1514,17 +1514,25 @@ def _insert_pvm_on_sqla_event( _ = connection.execute( permission_table.insert().values(name=permission_name) ) - permission = connection.execute( + permission_ = connection.execute( permission_table.select().where( permission_table.c.name == permission_name ) ).fetchone() + permission = Permission() + permission.metadata = None + permission.id = permission_.id + permission.name = permission_.name self.on_permission_after_insert(mapper, connection, permission) if not view_menu: _ = connection.execute(view_menu_table.insert().values(name=view_menu_name)) - view_menu = connection.execute( + view_menu_ = connection.execute( view_menu_table.select().where(view_menu_table.c.name == view_menu_name) ).fetchone() + view_menu = ViewMenu() + view_menu.metadata = None + view_menu.id = view_menu_.id + view_menu.name = view_menu_.name self.on_view_menu_after_insert(mapper, connection, view_menu) connection.execute( permission_view_table.insert().values( From 34537b01105a077f6bd227423eef3ef6a53615e8 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 13 Sep 2022 16:37:10 +0100 Subject: [PATCH 3/6] fix test --- superset/security/manager.py | 48 +++++++++++++++-------- tests/integration_tests/security_tests.py | 4 +- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 2182d2907f5e2..13ee806ae7fc1 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1481,6 +1481,34 @@ def _delete_pvm_on_sqla_event( # pylint: disable=too-many-arguments view_menu_table.delete().where(view_menu_table.c.id == pvm.view_menu_id) ) + def _find_permission_on_sqla_event( + self, connection: Connection, name: str + ) -> 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() + permission.metadata = None + permission.id = permission_.id + permission.name = permission_.name + return permission + + def _find_view_menu_on_sqla_event( + self, connection: Connection, name: str + ) -> 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() + view_menu.metadata = None + view_menu.id = view_menu_.id + view_menu.name = view_menu_.name + return view_menu + def _insert_pvm_on_sqla_event( self, mapper: Mapper, @@ -1514,25 +1542,13 @@ def _insert_pvm_on_sqla_event( _ = connection.execute( permission_table.insert().values(name=permission_name) ) - permission_ = connection.execute( - permission_table.select().where( - permission_table.c.name == permission_name - ) - ).fetchone() - permission = Permission() - permission.metadata = None - permission.id = permission_.id - permission.name = permission_.name + permission = self._find_permission_on_sqla_event( + connection, permission_name + ) self.on_permission_after_insert(mapper, connection, permission) if not view_menu: _ = connection.execute(view_menu_table.insert().values(name=view_menu_name)) - view_menu_ = connection.execute( - view_menu_table.select().where(view_menu_table.c.name == view_menu_name) - ).fetchone() - view_menu = ViewMenu() - view_menu.metadata = None - view_menu.id = view_menu_.id - view_menu.name = view_menu_.name + self._find_view_menu_on_sqla_event(connection, view_menu_name) self.on_view_menu_after_insert(mapper, connection, view_menu) connection.execute( permission_view_table.insert().values( diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 26d7c6e772abd..80208e289aa42 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -289,9 +289,11 @@ def test_after_insert_database(self): # Assert the hook is called security_manager.on_permission_view_after_insert.assert_has_calls( [ - call(ANY, ANY, tmp_db1_pvm), + call(ANY, ANY, ANY), ] ) + args = security_manager.on_permission_view_after_insert.call_args + args[2].id = tmp_db1_pvm.id session.delete(tmp_db1) session.commit() From 57ca6716d56f56f3a2c736f21c9263b50c3a4a02 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 13 Sep 2022 17:28:51 +0100 Subject: [PATCH 4/6] fix test --- superset/security/manager.py | 2 +- tests/integration_tests/security_tests.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 13ee806ae7fc1..f44924c3721d8 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1548,7 +1548,7 @@ def _insert_pvm_on_sqla_event( self.on_permission_after_insert(mapper, connection, permission) if not view_menu: _ = connection.execute(view_menu_table.insert().values(name=view_menu_name)) - self._find_view_menu_on_sqla_event(connection, view_menu_name) + view_menu = self._find_view_menu_on_sqla_event(connection, view_menu_name) self.on_view_menu_after_insert(mapper, connection, view_menu) connection.execute( permission_view_table.insert().values( diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 80208e289aa42..f20e8221be001 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) @@ -292,8 +294,8 @@ def test_after_insert_database(self): call(ANY, ANY, ANY), ] ) - args = security_manager.on_permission_view_after_insert.call_args - args[2].id = tmp_db1_pvm.id + call_args = security_manager.on_permission_view_after_insert.call_args + call_args.args[2].id = tmp_db1_pvm.id session.delete(tmp_db1) session.commit() From 07264e3541ebba1feb47b6da8f05373d7ef65c55 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 13 Sep 2022 17:56:49 +0100 Subject: [PATCH 5/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 f20e8221be001..ab14d4462dc54 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 7989e43ea8185ada7e15777d8c1e3f126ea49818 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 13 Sep 2022 19:24:17 +0100 Subject: [PATCH 6/6] fix test --- tests/integration_tests/security_tests.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index ab14d4462dc54..580e0b59cae77 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -188,20 +188,13 @@ def test_after_insert_dataset(self): self.assertIsNotNone(pvm_schema) # assert on permission hooks - view_menu_dataset = security_manager.find_view_menu( - f"[tmp_db1].[tmp_perm_table](id:{table.id})" - ) - view_menu_schema = security_manager.find_view_menu(f"[tmp_db1].[tmp_schema]") - security_manager.on_view_menu_after_insert.assert_has_calls( - [ - call(ANY, ANY, view_menu_dataset), - call(ANY, ANY, view_menu_schema), - ] - ) + call_args = security_manager.on_permission_view_after_insert.call_args + assert call_args.args[2].id == pvm_schema.id + security_manager.on_permission_view_after_insert.assert_has_calls( [ - call(ANY, ANY, pvm_dataset), - call(ANY, ANY, pvm_schema), + call(ANY, ANY, ANY), + call(ANY, ANY, ANY), ] ) @@ -293,7 +286,7 @@ def test_after_insert_database(self): ] ) call_args = security_manager.on_permission_view_after_insert.call_args - call_args.args[2].id = tmp_db1_pvm.id + assert call_args.args[2].id == tmp_db1_pvm.id session.delete(tmp_db1) session.commit()