diff --git a/superset/security/manager.py b/superset/security/manager.py index a771754b839..c3688a6fdbc 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -831,27 +831,36 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods except (json.JSONDecodeError, TypeError): return False - def has_drill_by_access( + def has_drill_access( self, form_data: dict[str, Any], dashboard: "Dashboard", datasource: "BaseDatasource | Explorable", ) -> bool: """ - Return True if the form_data is performing a supported drill by operation, - False otherwise. + Return True if the form_data is performing a supported drill operation + (Drill to Detail or Drill By), False otherwise. :param form_data: The form_data included in the request. :param dashboard: The dashboard the user is drilling from. :param datasource: The datasource being queried - :returns: Whether the user has drill by access. + :returns: Whether the user has drill access. """ from superset.models.slice import Slice + # Drill to Detail: no slice/chart context, dataset must belong to the dashboard + if ( + form_data.get("slice_id") is None + and form_data.get("chart_id") is None + and datasource in dashboard.datasources + ): + return True + + # Drill By: slice_id is 0 (sentinel), chart_id identifies the source chart, + # and the requested groupby columns must be drillable return bool( - form_data.get("type") != "NATIVE_FILTER" - and form_data.get("slice_id") == 0 + form_data.get("slice_id") == 0 and (chart_id := form_data.get("chart_id")) and ( slc := self.session.query(Slice) @@ -2736,40 +2745,51 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods ) ) or ( - # Chart. form_data.get("type") != "NATIVE_FILTER" - and (slice_id := form_data.get("slice_id")) and ( - # Direct chart access (no parent) ( - form_data.get("parent_slice_id") is None + # Chart. + (slice_id := form_data.get("slice_id")) and ( - slc := self.session.query(Slice) - .filter(Slice.id == slice_id) - .one_or_none() + # Direct chart access (no parent) + ( + form_data.get("parent_slice_id") is None + and ( + slc := self.session.query(Slice) + .filter(Slice.id == slice_id) + .one_or_none() + ) + and slc in dashboard_.slices + and slc.datasource == datasource + ) + or + # Multi-layer chart child access (has parent) + ( + ( + parent_id := form_data.get( + "parent_slice_id" + ) + ) + and ( + parent_slc := self.session.query(Slice) + .filter(Slice.id == parent_id) + .one_or_none() + ) + and parent_slc in dashboard_.slices + # Validate child is actually part of parent's config # noqa: E501 + and self._validate_child_in_parent_multilayer( # noqa: E501 + child_slice_id=slice_id, + parent_slice=parent_slc, + ) + ) ) - and slc in dashboard_.slices - and slc.datasource == datasource ) - or - # Multi-layer chart child access (has parent) - ( - (parent_id := form_data.get("parent_slice_id")) - and ( - parent_slc := self.session.query(Slice) - .filter(Slice.id == parent_id) - .one_or_none() - ) - and parent_slc in dashboard_.slices - # Validate child is actually part of parent's config - and self._validate_child_in_parent_multilayer( - child_slice_id=slice_id, - parent_slice=parent_slc, - ) + # D2D or Drill By + or self.has_drill_access( + form_data, dashboard_, datasource ) ) ) - or self.has_drill_by_access(form_data, dashboard_, datasource) ) and self.can_access_dashboard(dashboard_) ) diff --git a/superset/views/datasource/utils.py b/superset/views/datasource/utils.py index 096b8482c59..deec4067a56 100644 --- a/superset/views/datasource/utils.py +++ b/superset/views/datasource/utils.py @@ -94,13 +94,15 @@ def get_samples( # pylint: disable=too-many-arguments force: bool = False, page: int = 1, per_page: int = 1000, - payload: Optional[SamplesPayloadSchema] = None, + payload: SamplesPayloadSchema | None = None, + dashboard_id: int | None = None, ) -> dict[str, Any]: datasource = DatasourceDAO.get_datasource( datasource_type=datasource_type, database_id_or_uuid=str(datasource_id), ) + form_data = {"dashboardId": dashboard_id} if dashboard_id else None limit_clause = get_limit_clause(page, per_page) # todo(yongjie): Constructing count(*) and samples in the same query_context, @@ -112,6 +114,7 @@ def get_samples( # pylint: disable=too-many-arguments "id": datasource.id, }, queries=[limit_clause], + form_data=form_data, result_type=ChartDataResultType.SAMPLES, force=force, ) @@ -128,6 +131,7 @@ def get_samples( # pylint: disable=too-many-arguments "id": datasource.id, }, queries=[{**payload, **limit_clause}], + form_data=form_data, result_type=ChartDataResultType.DRILL_DETAIL, force=force, ) @@ -148,6 +152,7 @@ def get_samples( # pylint: disable=too-many-arguments "id": datasource.id, }, queries=[{**payload, **count_star_metric} if payload else count_star_metric], + form_data=form_data, result_type=ChartDataResultType.FULL, force=force, ) diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py index 6796fd4cdc5..c7b130a4b9a 100644 --- a/superset/views/datasource/views.py +++ b/superset/views/datasource/views.py @@ -212,16 +212,15 @@ class Datasource(BaseSupersetView): payload = SamplesPayloadSchema().load(request.json) except ValidationError as err: return json_error_response(err.messages, status=400) - + dashboard_id = None if security_manager.is_guest_user(): if not params["dashboard_id"]: return json_error_response(_("Forbidden"), status=403) + dashboard_id = params["dashboard_id"] dataset = DatasetDAO.find_by_id( params["datasource_id"], skip_base_filter=True ) - dashboard = DashboardDAO.find_by_id( - params["dashboard_id"], skip_base_filter=True - ) + dashboard = DashboardDAO.find_by_id(dashboard_id, skip_base_filter=True) if not (dashboard and dataset): return self.response_404() if not security_manager.can_drill_dataset_via_dashboard_access( @@ -237,6 +236,7 @@ class Datasource(BaseSupersetView): page=params["page"], per_page=params["per_page"], payload=payload, + dashboard_id=dashboard_id, ) return self.json_response({"result": rv}) diff --git a/tests/integration_tests/datasource_tests.py b/tests/integration_tests/datasource_tests.py index 319b8573720..1dc060c649b 100644 --- a/tests/integration_tests/datasource_tests.py +++ b/tests/integration_tests/datasource_tests.py @@ -24,7 +24,7 @@ import prison import pytest from flask import current_app -from superset import db +from superset import db, security_manager as sm from superset.commands.dataset.exceptions import DatasetNotFoundError from superset.common.utils.query_cache_manager import QueryCacheManager from superset.connectors.sqla.models import ( # noqa: F401 @@ -531,17 +531,26 @@ class TestDatasource(SupersetTestCase): self, mock_has_guest_access, mock_is_guest_user, mock_rls ): """ - Embedded user can access the /samples view. + Embedded guest user can access /samples (for D2D) via the dashboard context + passed as form_data to QueryContextFactory. """ - self.login(ADMIN_USERNAME) + # Gamma role doesn't have dataset access (mimic embedded role), + # but needs access to the /samples endpoint + gamma_role = sm.find_role("Gamma") + perm_view = sm.find_permission_view_menu("can_samples", "Datasource") + sm.add_permission_role(gamma_role, perm_view) + self.login(GAMMA_USERNAME) mock_is_guest_user.return_value = True mock_has_guest_access.return_value = True mock_rls.return_value = [] tbl = self.get_table(name="birth_names") dash = self.get_dash_by_slug("births") - uri = f"/datasource/samples?datasource_id={tbl.id}&datasource_type=table&dashboard_id={dash.id}" # noqa: E501 - resp = self.client.post(uri, json={}) - assert resp.status_code == 200 + try: + uri = f"/datasource/samples?datasource_id={tbl.id}&datasource_type=table&dashboard_id={dash.id}" # noqa: E501 + resp = self.client.post(uri, json={}) + assert resp.status_code == 200 + finally: + sm.del_permission_role(gamma_role, perm_view) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @mock.patch( diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index edc1e0a649f..9caf4706a03 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -356,7 +356,100 @@ class TestGuestUserDatasourceAccess(SupersetTestCase): } ) - def test_raise_for_access__no_chart_in_form_data(self): + def test_raise_for_access__drill_to_detail_happy_path(self): + """ + Drill to Detail: no slice_id in form_data, datasource is on the dashboard + the embedded user has access to. + """ + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.datasource, + form_data={ + "dashboardId": self.dash.id, + }, + slice_=None, + queries=[], + ) + } + ) + + def test_raise_for_access__drill_to_detail_datasource_not_on_dashboard(self): + """ + Drill to Detail is denied when the target datasource is not associated + with the dashboard the embedded user has access to. + """ + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): # noqa: PT027 + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.other_datasource, + form_data={ + "dashboardId": self.dash.id, + }, + slice_=None, + queries=[], + ) + } + ) + + def test_raise_for_access__drill_by_happy_path(self): + """ + Drill By: slice_id=0 (sentinel), chart_id points to a chart on the dashboard + whose datasource matches, the requested groupby column is drillable and the + embedded user has access to. + """ + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.datasource, + form_data={ + "dashboardId": self.dash.id, + "slice_id": 0, + "chart_id": self.chart.id, + "groupby": ["gender"], + }, + slice_=None, + queries=[], + ) + } + ) + + def test_raise_for_access__drill_by_chart_not_on_dashboard(self): + """ + Drill By is denied when chart_id refers to a chart that is not on the + dashboard the embedded user has access to. + """ + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): # noqa: PT027 + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.other_datasource, + form_data={ + "dashboardId": self.dash.id, + "slice_id": 0, + "chart_id": self.other_chart.id, + "groupby": ["gender"], + }, + slice_=None, + queries=[], + ) + } + ) + + def test_raise_for_access__drill_by_columns_not_drillable(self): + """ + Drill By is denied when the requested groupby columns are not marked as + drillable (groupby=True) on the datasource. + """ g.user = self.authorized_guest for kwarg in ["viz", "query_context"]: with self.assertRaises(SupersetSecurityException): # noqa: PT027 @@ -366,7 +459,12 @@ class TestGuestUserDatasourceAccess(SupersetTestCase): datasource=self.datasource, form_data={ "dashboardId": self.dash.id, + "slice_id": 0, + "chart_id": self.chart.id, + "groupby": ["__not_a_drillable_column__"], }, + slice_=None, + queries=[], ) } ) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index e59dd417b57..1191c7fd9d8 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1895,16 +1895,19 @@ class TestSecurityManager(SupersetTestCase): } ) - # Undefined dashboard chart. - with self.assertRaises(SupersetSecurityException): # noqa: PT027 - security_manager.raise_for_access( - **{ - kwarg: Mock( - datasource=birth_names, - form_data={"dashboardId": births.id}, - ) - } - ) + # Drill to Detail (no slice_id/chart_id): datasource on dashboard. + # Access is granted via DASHBOARD_RBAC — D2D is a valid operation + # for users who have dashboard access. + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=birth_names, + form_data={"dashboardId": births.id}, + slice_=None, + queries=[], + ) + } + ) # Ill-defined dashboard chart. with self.assertRaises(SupersetSecurityException): # noqa: PT027