diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index d8150956e6d..17852041edb 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -58,6 +58,7 @@ from superset.utils.core import ( get_column_name, get_column_names_from_columns, get_column_names_from_metrics, + get_user_id, is_adhoc_column, is_adhoc_metric, ) @@ -270,6 +271,11 @@ class QueryContextProcessor: datasource = self._qc_datasource extra_cache_keys = datasource.get_extra_cache_keys(query_obj.to_dict()) + # Annotation data is cached on the same entry as the dataframe, so the + # key must also bind the annotation sources' security context. + if query_obj and query_obj.annotation_layers: + kwargs["annotation_context"] = self._annotation_cache_context(query_obj) + cache_key = ( query_obj.cache_key( datasource=datasource.uid, @@ -283,6 +289,32 @@ class QueryContextProcessor: ) return cache_key + def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str, Any]: + """ + Cache-key material binding cached annotation data to its security + context. + + Annotation payloads are fetched per requesting user and stored on the + same cache entry as the dataframe, so the key also binds the requesting + user and, for chart-backed layers, the RLS clauses of the referenced + chart's datasource. + """ + source_rls: dict[str, list[str] | None] = {} + for layer in query_obj.annotation_layers: + if layer.get("sourceType") not in ("line", "table"): + continue + layer_value = layer.get("value") + chart = ( + ChartDAO.find_by_id(layer_value) if layer_value is not None else None + ) + annotation_datasource = chart.datasource if chart else None + source_rls[str(layer.get("value"))] = ( + security_manager.get_rls_cache_key(annotation_datasource) + if annotation_datasource + else None + ) + return {"user_id": get_user_id(), "source_rls": source_rls} + def get_query_result(self, query_object: QueryObject) -> QueryResult: """ Returns a pandas dataframe based on the query object. @@ -636,6 +668,11 @@ class QueryContextProcessor: if layer["sourceType"] == "NATIVE" ] layer_ids = [layer["value"] for layer in annotation_layers] + # Enforce the annotation read permission before returning layer records. + if layer_ids and not security_manager.can_access("can_read", "Annotation"): + raise QueryObjectValidationError( + _("You don't have access to annotation layers") + ) layer_objects = { layer_object.id: layer_object for layer_object in AnnotationLayerDAO.find_by_ids(layer_ids) @@ -645,6 +682,15 @@ class QueryContextProcessor: for layer in annotation_layers: layer_id = layer["value"] layer_name = layer["name"] + # A request may reference a layer id that does not exist; treat it + # as a validation error rather than failing on the missing key. + if (layer_object := layer_objects.get(layer_id)) is None: + raise QueryObjectValidationError( + _( + "Annotation layer with ID %(layer_id)s was not found", + layer_id=layer_id, + ) + ) columns = [ "start_dttm", "end_dttm", @@ -652,7 +698,6 @@ class QueryContextProcessor: "long_descr", "json_metadata", ] - layer_object = layer_objects[layer_id] records = [ {column: getattr(annotation, column) for column in columns} for annotation in layer_object.annotation diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 04fc082f540..437aa111c7a 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -1267,6 +1267,34 @@ def get_dataset_id_from_context(metric_key: str) -> int: raise SupersetTemplateException(exc_message) +def guest_user_can_access_dataset(dataset: SqlaTable) -> bool: + """ + Whether the current guest (embedded) user may read the given dataset. + + Guest access is granted per dashboard, so the dataset must back at least + one chart on a dashboard the guest token covers; a ``datasets`` allowlist + on the token further restricts the reachable IDs. + + :param dataset: a dataset resolved without the DAO base filter. + :returns: whether the guest user may read the dataset. + """ + guest_user = security_manager.get_current_guest_user_if_guest() + if not guest_user: + return False + + allowed_datasets: list[int] | None = guest_user.guest_token.get("datasets") + if allowed_datasets is not None and ( + not isinstance(allowed_datasets, list) or dataset.id not in allowed_datasets + ): + return False + + return any( + security_manager.has_guest_access(dashboard) + for slc in dataset.slices + for dashboard in slc.dashboards + ) + + def metric_macro( env: Environment, context: dict[str, Any], @@ -1289,8 +1317,9 @@ def metric_macro( if not dataset_id: dataset_id = get_dataset_id_from_context(metric_key) - # Embedded user access is validated at the dashboard level, so we bypass - # the regular DAO filter for them + # Embedded (guest) user access is validated at the dashboard level, so the + # regular DAO filter is bypassed for them and dashboard-level scope is + # enforced explicitly below. dataset = DatasetDAO.find_by_id( dataset_id, skip_base_filter=security_manager.is_guest_user(), @@ -1298,6 +1327,11 @@ def metric_macro( if not dataset: raise DatasetNotFoundError(f"Dataset ID {dataset_id} not found.") + # With the base filter skipped, scope a guest to datasets reachable through + # a dashboard their token grants; reuse the not-found error for consistency. + if security_manager.is_guest_user() and not guest_user_can_access_dataset(dataset): + raise DatasetNotFoundError(f"Dataset ID {dataset_id} not found.") + metrics: dict[str, str] = { metric.metric_name: metric.expression for metric in dataset.metrics } diff --git a/superset/security/manager.py b/superset/security/manager.py index 5dc044d4a89..cee80db335c 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -752,15 +752,24 @@ def _native_filter_query_modified( query: Any, allowed_columns: set[str], allowed_metrics: set[str] ) -> bool: """Whether a single query in a native-filter request reads beyond its targets.""" - # Columns and group-by may only reference target column(s); adhoc (free-form - # SQL) columns cannot be validated, so reject them. - for key in ("columns", "groupby"): + # Columns, group-by, and series columns may only reference target column(s); + # adhoc (free-form SQL) columns cannot be validated, so reject them. + for key in ("columns", "groupby", "series_columns"): for col in getattr(query, key, None) or []: if not isinstance(col, str) or col not in allowed_columns: return True for metric in getattr(query, "metrics", None) or []: if not _native_filter_term_allowed(metric, allowed_columns, allowed_metrics): return True + # A series-limit metric ranks the top-N groups in the inner query, so it is + # a value-returning term and is validated like a metric. ``QueryObject`` + # renames the deprecated ``timeseries_limit_metric`` payload key onto this + # attribute, so both spellings are covered. + series_limit_metric = getattr(query, "series_limit_metric", None) + if series_limit_metric and not _native_filter_term_allowed( + series_limit_metric, allowed_columns, allowed_metrics + ): + return True # order-by entries are ``(expression, asc)`` pairs. for order in getattr(query, "orderby", None) or []: expr = order[0] if isinstance(order, (list, tuple)) and order else order @@ -782,8 +791,9 @@ def _native_filter_request_modified(query_context: "QueryContext") -> bool: A native filter may only read the column(s) it targets on the dashboard it belongs to. The request is treated as modified (and therefore rejected for guest users) when it cannot be tied to a native filter on the requesting - dashboard, or when any value-returning term (column, group-by, metric, or - order-by) references something other than a target column, a simple + dashboard, or when any value-returning term (column, group-by, series + column, metric, series-limit metric, or order-by) references something + other than a target column, a simple aggregate over a target column, or the filter's configured sort metric. Free-form SQL terms and saved metrics other than the configured sort metric are rejected. Row-restricting clauses (``filter``/``extras``) are not @@ -4244,6 +4254,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods child_slice_id=slice_id, parent_slice=parent_slc, ) + # Bind the request to the child + # chart's own datasource, mirroring + # the direct-chart leg above. + and ( + child_slc := self.session.query(Slice) + .filter(Slice.id == slice_id) + .one_or_none() + ) + and child_slc.datasource == datasource ) ) ) diff --git a/tests/unit_tests/common/test_query_context_processor.py b/tests/unit_tests/common/test_query_context_processor.py index d81dd495e3a..dc50853c039 100644 --- a/tests/unit_tests/common/test_query_context_processor.py +++ b/tests/unit_tests/common/test_query_context_processor.py @@ -27,6 +27,7 @@ from superset.common.chart_data import ChartDataResultFormat, ChartDataResultTyp from superset.common.chart_data_timing import QueryDataResult, QueryTiming from superset.common.db_query_status import QueryStatus from superset.common.query_context_processor import QueryContextProcessor +from superset.exceptions import QueryObjectValidationError from superset.utils.core import GenericDataType from superset.utils.date_parser import get_past_or_future @@ -98,6 +99,25 @@ def processor(mock_query_context): return processor +def test_query_cache_key_binds_annotation_data_to_requesting_user(processor): + """The cache key for annotated queries must differ per requesting user.""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a", "value": 1}] + with ( + patch( + "superset.common.query_context_processor.get_user_id", + side_effect=[1, 2], + ), + patch("superset.common.query_context_processor.security_manager"), + ): + processor.query_cache_key(query_obj) + processor.query_cache_key(query_obj) + contexts = [ + call.kwargs["annotation_context"] for call in query_obj.cache_key.call_args_list + ] + assert contexts[0] != contexts[1] + + def test_get_data_table_like(processor, mock_query_context): df = pd.DataFrame({"col1": [1, 2, 3], "col2": ["a", "b", "c"]}) coltypes = [GenericDataType.NUMERIC, GenericDataType.STRING] @@ -2377,3 +2397,26 @@ def test_relative_offset_preserves_inner_bounds( # for #40501. Without the fix, inner_from/to_dttm == shifted dates. assert captured[0]["inner_from_dttm"] == pd.Timestamp("2026-05-01") assert captured[0]["inner_to_dttm"] == pd.Timestamp("2026-05-28") + + +def test_get_native_annotation_data_requires_annotation_read_access(): + """Native annotation layers are only served to users who can read them.""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a", "value": 1}] + with ( + patch( + "superset.common.query_context_processor.security_manager" + ) as security_manager_mock, + patch( + "superset.common.query_context_processor.AnnotationLayerDAO.find_by_ids", + return_value=[], + ) as find_by_ids_mock, + ): + # ``can_access`` is synchronous; force a plain Mock so the patched + # manager doesn't hand back a truthy coroutine that slips past the + # ``not can_access(...)`` guard. + security_manager_mock.can_access = MagicMock(return_value=False) + with pytest.raises(QueryObjectValidationError): + QueryContextProcessor.get_native_annotation_data(query_obj) + security_manager_mock.can_access.assert_called_once_with("can_read", "Annotation") + find_by_ids_mock.assert_not_called() diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py index 4b067ddfb29..2b50b9d6a84 100644 --- a/tests/unit_tests/jinja_context_test.py +++ b/tests/unit_tests/jinja_context_test.py @@ -1096,6 +1096,34 @@ def test_metric_macro_with_dataset_id(mocker: MockerFixture) -> None: mock_get_form_data.assert_not_called() +def test_metric_macro_guest_user_dataset_out_of_scope(mocker: MockerFixture) -> None: + """ + Test that ``metric_macro`` denies a guest user a dataset that is not + reachable through any dashboard their guest token grants. + """ + mocker.patch("superset.security_manager.is_guest_user", return_value=True) + guest_user = mocker.MagicMock() + guest_user.guest_token = {} + mocker.patch( + "superset.security_manager.get_current_guest_user_if_guest", + return_value=guest_user, + ) + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + DatasetDAO.find_by_id.return_value = SqlaTable( + id=1, + table_name="test_dataset", + metrics=[ + SqlMetric(metric_name="count", expression="COUNT(*)"), + ], + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + schema="my_schema", + sql=None, + ) + env = SandboxedEnvironment(undefined=DebugUndefined) + with pytest.raises(DatasetNotFoundError): + metric_macro(env, {}, "count", 1) + + def test_metric_macro_recursive(mocker: MockerFixture) -> None: """ Test the ``metric_macro`` when the definition is recursive. @@ -1732,6 +1760,13 @@ def test_metric_macro_embedded_user_skips_base_filter(mocker: MockerFixture) -> mock_is_guest_user = mocker.patch("superset.security_manager.is_guest_user") mock_is_guest_user.return_value = True + # Dashboard-level guest scope is asserted separately; here the dataset is + # in scope so the test can focus on the base-filter bypass. + mocker.patch( + "superset.jinja_context.guest_user_can_access_dataset", + return_value=True, + ) + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 DatasetDAO.find_by_id.return_value = SqlaTable( table_name="test_dataset", diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index b940225c6cb..8ad1660cb65 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -140,6 +140,69 @@ def test_raise_for_access_guest_user_ok_subset( sm.raise_for_access(query_context=query_context) +def test_raise_for_access_guest_user_deck_multi_child_requires_child_datasource( + mocker: MockerFixture, + app_context: None, +) -> None: + """ + The deck.gl multi-layer child leg must bind the requested datasource to + the child chart: a valid parent/child pair does not authorize querying + an arbitrary dataset. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "is_guest_user", return_value=True) + mocker.patch.object(sm, "can_access", return_value=False) + mocker.patch.object(sm, "can_access_schema", return_value=False) + mocker.patch.object(sm, "is_editor", return_value=False) + mocker.patch.object(sm, "can_access_dashboard", return_value=True) + mocker.patch.object(sm, "get_current_guest_user_if_guest", return_value=None) + mocker.patch( + "superset.is_feature_enabled", + side_effect=lambda feature: feature == "EMBEDDED_SUPERSET", + ) + mocker.patch( + "superset.security.manager.query_context_modified", + return_value=False, + ) + + child_datasource = mocker.MagicMock() + other_datasource = mocker.MagicMock() + + parent_slc = mocker.MagicMock() + parent_slc.params = json.dumps({"viz_type": "deck_multi", "deck_slices": [42]}) + child_slc = mocker.MagicMock() + child_slc.datasource = child_datasource + + dashboard = mocker.MagicMock() + dashboard.slices = [parent_slc] + + query_mock = mocker.patch.object(sm.session, "query") + query_mock.return_value.filter.return_value.one_or_none.side_effect = [ + dashboard, + parent_slc, + child_slc, + dashboard, + parent_slc, + child_slc, + ] + + query_context = mocker.MagicMock() + query_context.form_data = { + "dashboardId": 10, + "slice_id": 42, + "parent_slice_id": 41, + } + + # Requesting the child's own datasource is allowed. + query_context.datasource = child_datasource + sm.raise_for_access(query_context=query_context) + + # The same chart context with any other datasource is rejected. + query_context.datasource = other_datasource + with pytest.raises(SupersetSecurityException): + sm.raise_for_access(query_context=query_context) + + def test_raise_for_access_guest_user_tampered_id( mocker: MockerFixture, app_context: None, @@ -1458,6 +1521,32 @@ def test_query_context_modified_native_filter_arbitrary_saved_metric_blocked( assert query_context_modified(qc) +def test_query_context_modified_native_filter_series_limit_terms_blocked( + mocker: MockerFixture, +) -> None: + """A series-limit metric or series column beyond the target is modified.""" + query = SimpleNamespace( + columns=["region"], + metrics=[], + groupby=[], + series_columns=["region"], + series_limit=5, + series_limit_metric={ + "expressionType": "SIMPLE", + "column": {"column_name": "salary"}, + "aggregate": "MAX", + }, + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + query = SimpleNamespace( + columns=["region"], metrics=[], groupby=[], series_columns=["ssn"] + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + def test_query_context_modified_native_filter_orderby_arbitrary_column_blocked( mocker: MockerFixture, ) -> None: