From c465fc5ad5166aa91471266cfb3e816e46f283bc Mon Sep 17 00:00:00 2001 From: John Bodley Date: Wed, 16 Aug 2023 11:56:32 -0700 Subject: [PATCH] fix: Address regression introduced in #24789 --- superset/security/manager.py | 24 +++++- tests/integration_tests/security_tests.py | 92 ++++++++++++++++++++--- 2 files changed, 104 insertions(+), 12 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index b996188a8ea49..99a81addc92d0 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1796,7 +1796,8 @@ def raise_for_access( # pylint: disable=import-outside-toplevel from superset import is_feature_enabled from superset.connectors.sqla.models import SqlaTable - from superset.daos.dashboard import DashboardDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice from superset.sql_parse import Table if database and table or query: @@ -1858,10 +1859,27 @@ def raise_for_access( or self.can_access("datasource_access", datasource.perm or "") or self.is_owner(datasource) or ( + # Grant access to the datasource only if dashboard RBAC is enabled + # and said datasource is associated with the dashboard chart in + # question. form_data + and is_feature_enabled("DASHBOARD_RBAC") and (dashboard_id := form_data.get("dashboardId")) - and (dashboard := DashboardDAO.find_by_id(dashboard_id)) - and self.can_access_dashboard(dashboard) + and ( + dashboard_ := self.get_session.query(Dashboard) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) + and dashboard_.roles + and (slice_id := form_data.get("slice_id")) + and ( + slc := self.get_session.query(Slice) + .filter(Slice.id == slice_id) + .one_or_none() + ) + and slc in dashboard_.slices + and slc.datasource == datasource + and self.can_access_dashboard(dashboard_) ) ): raise SupersetSecurityException( diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 139ad263425ed..4767e5af0e8cb 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1698,6 +1698,7 @@ def test_raise_for_access_viz( security_manager.raise_for_access(viz=test_viz) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") @with_feature_flags(DASHBOARD_RBAC=True) @patch("superset.security.manager.g") @patch("superset.security.SupersetSecurityManager.is_owner") @@ -1710,12 +1711,12 @@ def test_raise_for_access_rbac( mock_is_owner, mock_g, ): - dashboard = self.get_dash_by_slug("births") + births = self.get_dash_by_slug("births") + girls = self.get_slice("Girls", db.session, expunge_from_session=False) + birth_names = girls.datasource - obj = Mock( - datasource=self.get_datasource_mock(), - form_data={"dashboardId": dashboard.id}, - ) + world_health = self.get_dash_by_slug("world_health") + treemap = self.get_slice("Treemap", db.session, expunge_from_session=False) mock_g.user = security_manager.find_user("gamma") mock_is_owner.return_value = False @@ -1723,15 +1724,88 @@ def test_raise_for_access_rbac( mock_can_access_schema.return_value = False for kwarg in ["query_context", "viz"]: - dashboard.roles = [] + births.roles = [] db.session.flush() + # No dashboard roles. with self.assertRaises(SupersetSecurityException): - security_manager.raise_for_access(**{kwarg: obj}) + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=birth_names, + form_data={ + "dashboardId": births.id, + "slice_id": girls.id, + }, + ) + } + ) - dashboard.roles = [self.get_role("Gamma")] + births.roles = [self.get_role("Gamma")] db.session.flush() - security_manager.raise_for_access(**{kwarg: obj}) + + # Undefined dashboard. + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=birth_names, + form_data={}, + ) + } + ) + + # Undefined dashboard chart. + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=birth_names, + form_data={"dashboardId": births.id}, + ) + } + ) + + # Ill-defined dashboard chart. + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=birth_names, + form_data={ + "dashboardId": births.id, + "slice_id": treemap.id, + }, + ) + } + ) + + # Dashboard chart not associated with said datasource. + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=birth_names, + form_data={ + "dashboardId": world_health.id, + "slice_id": treemap.id, + }, + ) + } + ) + + # Dashboard chart associated with said datasource. + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=birth_names, + form_data={ + "dashboardId": births.id, + "slice_id": girls.id, + }, + ) + } + ) db.session.rollback()