diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index 2c27fb9fb064..1fb99abe3ed5 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -45,6 +45,7 @@ from superset.exceptions import ( QueryObjectValidationError, SupersetException, + SupersetSecurityException, ) from superset.explorables.base import Explorable from superset.extensions import cache_manager, security_manager @@ -60,7 +61,6 @@ get_column_name, get_column_names_from_columns, get_column_names_from_metrics, - get_user_id, is_adhoc_column, is_adhoc_metric, ) @@ -428,7 +428,7 @@ def query_cache_key(self, query_obj: QueryObject, **kwargs: Any) -> str | None: 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. + # key must also bind the annotation sources' security scope. if query_obj and query_obj.annotation_layers: kwargs["annotation_context"] = self._annotation_cache_context(query_obj) @@ -447,29 +447,98 @@ def query_cache_key(self, query_obj: QueryObject, **kwargs: Any) -> str | None: 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. + Cache-key material binding annotation data to its security *scope* so + users with the same access share a cache entry and users with a + different scope -- or no access -- never read each other's data. + + Annotation payloads are fetched under the requesting user's permissions + and stored on the same cache entry as the dataframe, so the key binds + the inputs that determine what a user may see: + + * NATIVE layers: the ``can_read`` permission on ``Annotation``, the only + user-dependent dimension of these global records. + * Chart-backed (``line``/``table``) layers: see + :meth:`_annotation_source_scope`. """ - source_rls: dict[str, list[str] | None] = {} + context: dict[str, Any] = {} + + if any( + layer.get("sourceType") == "NATIVE" for layer in query_obj.annotation_layers + ): + context["annotation_read"] = security_manager.can_access( + "can_read", "Annotation" + ) + + source_scope: dict[str, Any] = {} 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 + source_scope[str(layer_value)] = self._annotation_source_scope(layer_value) + if source_scope: + context["source_scope"] = source_scope + + return context + + def _annotation_source_scope(self, layer_value: Any) -> dict[str, Any]: + """ + Access and data-identity cache-key material for one chart-backed + annotation layer. + + ``access`` keeps a user denied the referenced chart from reading an + authorized user's cached payload. ``data_key`` is the annotation chart's + own query cache key, capturing the datasource version, RLS clauses, and + per-user Jinja/virtual-dataset RLS material. + """ + chart = ChartDAO.find_by_id(layer_value) if layer_value is not None else None + datasource = chart.datasource if chart else None + if chart is None or datasource is None: + return {"access": None, "data_key": None} + + try: + annotation_query_context = chart.get_query_context() + if annotation_query_context is None: + # No saved query context to key on: the fetch itself fails for + # every user, so fall back to the datasource's access + RLS + # identity. + return { + "access": security_manager.can_access_datasource(datasource), + "data_key": security_manager.get_rls_cache_key(datasource), + } + # Bind the *same* authorization the fetch performs, not just + # ``can_access_datasource``: get_viz_annotation_data validates the + # annotation chart's query context, whose access check also honors + # promiscuous-chart-access (VIEWER_PROMISCUOUS_MODE) and guest-token + # scopes -- branches ``can_access_datasource`` skips. Keying on the + # narrower check would let a promiscuous-granted viewer collapse onto + # a truly-denied user's entry and read their cached payload. + try: + security_manager.raise_for_access( + query_context=annotation_query_context + ) + access: Any = True + except SupersetSecurityException: + access = False + data_key: Any = [ + annotation_query_context.query_cache_key(query_object) + for query_object in annotation_query_context.queries + ] + except SupersetException: + # The annotation fetch raises these same errors and persists + # nothing, so a fallback key never stores real data; fail closed so + # this scope can't silently dedupe onto a successfully-derived one. + # Other errors propagate rather than weakening the key. + logger.warning( + "Could not derive annotation cache key for chart %s; " + "falling back to a fail-closed scope", + layer_value, + exc_info=True, ) - return {"user_id": get_user_id(), "source_rls": source_rls} + return { + "access": False, + "data_key": security_manager.get_rls_cache_key(datasource), + } + return {"access": access, "data_key": data_key} def get_query_result(self, query_object: QueryObject) -> QueryResult: """ diff --git a/superset/datasource/api.py b/superset/datasource/api.py index 3b16dc9413d3..286a1a4ff26c 100644 --- a/superset/datasource/api.py +++ b/superset/datasource/api.py @@ -48,6 +48,7 @@ from superset.semantic_layers.mapper import SUPPORTED_FILTER_OPERATORS from superset.superset_typing import FlaskResponse from superset.utils import json +from superset.utils.cache import set_data_cache_if_within_size from superset.utils.core import ( apply_max_row_limit, DatasourceType, @@ -281,7 +282,7 @@ def get_column_values( # Every distinct search term is its own key, so a few users typing # would otherwise pin one entry per keystroke for the full timeout. timeout = min(timeout, SEARCH_CACHE_TIMEOUT) - cache_manager.data_cache.set(cache_key, payload, timeout=timeout) + set_data_cache_if_within_size(cache_key, payload, timeout=timeout) logger.debug( "column-values cache MISS: uid=%s col=%s", datasource.uid, column_name ) @@ -575,7 +576,7 @@ def compatible(self, datasource_type: str, datasource_id: int) -> FlaskResponse: timeout = datasource.cache_timeout or app.config.get( "CACHE_DEFAULT_TIMEOUT", 300 ) - cache_manager.data_cache.set(cache_key, result, timeout=timeout) + set_data_cache_if_within_size(cache_key, result, timeout=timeout) return self.response(200, result=result) diff --git a/superset/sql/execution/executor.py b/superset/sql/execution/executor.py index 94c7ab49c5a9..f26a51735a9f 100644 --- a/superset/sql/execution/executor.py +++ b/superset/sql/execution/executor.py @@ -1023,11 +1023,10 @@ def _store_in_cache( "total_execution_time_ms": result.total_execution_time_ms, } - cache_manager.data_cache.set( - cache_key, - cached_data, - timeout=timeout, - ) + # Apply the same size cap as the chart-data path. + from superset.utils.cache import set_data_cache_if_within_size + + set_data_cache_if_within_size(cache_key, cached_data, timeout=timeout) def _connection_carries_user_identity(self) -> bool: """ diff --git a/superset/utils/cache.py b/superset/utils/cache.py index fb0d31811c85..f1f17292c2bb 100644 --- a/superset/utils/cache.py +++ b/superset/utils/cache.py @@ -54,6 +54,50 @@ def generate_cache_key(values_dict: dict[str, Any], key_prefix: str = "") -> str return cache_key +def oversized_data_cache_value(cache_key: str, cache_value: Any) -> bool: + """Whether ``cache_value`` exceeds ``DATA_CACHE_MAX_VALUE_SIZE``. + + Shared size guard so DATA-cache writers that store raw + (non-``QueryCacheManager``) payloads -- and so can't use + :func:`set_and_log_cache`, which wraps the value -- can also skip oversized + entries that would flood the cache backend. Returns ``False`` (never blocks) + when the cap is disabled (``None``), avoiding serialization overhead. + """ + max_value_size = app.config.get("DATA_CACHE_MAX_VALUE_SIZE") + if max_value_size is None: + return False + value_size = len(pickle.dumps(cache_value, protocol=pickle.HIGHEST_PROTOCOL)) + if value_size > max_value_size: + logger.warning( + "Skipping cache set for key %s: serialized value size %d bytes " + "exceeds DATA_CACHE_MAX_VALUE_SIZE (%d bytes)", + cache_key, + value_size, + max_value_size, + ) + app.config["STATS_LOGGER"].incr("skip_cache_value_too_large") + return True + return False + + +def set_data_cache_if_within_size( + cache_key: str, cache_value: Any, timeout: int | None = None +) -> bool: + """Write to the DATA cache unless the value exceeds the size cap. + + Wraps ``data_cache.set`` for writers that store raw payloads outside the + ``QueryCacheManager`` contract (and so can't use :func:`set_and_log_cache`). + + :returns: whether the value was persisted. + """ + if oversized_data_cache_value(cache_key, cache_value): + return False + return ( + cache_manager.data_cache.set(cache_key, cache_value, timeout=timeout) + is not False + ) + + def set_and_log_cache( cache_instance: Cache, cache_key: str, @@ -88,24 +132,10 @@ def set_and_log_cache( ) value = {**cache_value, "dttm": dttm} - # Skip caching results that are too large to protect the cache backend - # (e.g. Redis/Memcached) from being flooded by huge result sets. The chart - # still renders; the value is simply not cached, causing a re-query on the - # next load instead of a cache hit. Disabled when DATA_CACHE_MAX_VALUE_SIZE - # is None (the default), in which case no serialization overhead is incurred. - max_value_size = app.config.get("DATA_CACHE_MAX_VALUE_SIZE") - if max_value_size is not None: - value_size = len(pickle.dumps(value, protocol=pickle.HIGHEST_PROTOCOL)) - if value_size > max_value_size: - logger.warning( - "Skipping cache set for key %s: serialized value size %d bytes " - "exceeds DATA_CACHE_MAX_VALUE_SIZE (%d bytes)", - cache_key, - value_size, - max_value_size, - ) - app.config["STATS_LOGGER"].incr("skip_cache_value_too_large") - return False + # Skip oversized results to protect the cache backend; the chart still + # renders and simply re-queries on the next load. + if oversized_data_cache_value(cache_key, value): + return False # Flask-Caching's set() returns bool | None: cachelib backends can report # a failed write by returning False without raising, while some backends diff --git a/tests/unit_tests/common/test_query_context_processor.py b/tests/unit_tests/common/test_query_context_processor.py index 5dc21d12b854..4add0d9bc659 100644 --- a/tests/unit_tests/common/test_query_context_processor.py +++ b/tests/unit_tests/common/test_query_context_processor.py @@ -30,7 +30,8 @@ normalize_contribution_totals, QueryContextProcessor, ) -from superset.exceptions import QueryObjectValidationError +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import QueryObjectValidationError, SupersetSecurityException from superset.utils.core import GenericDataType from superset.utils.date_parser import get_past_or_future @@ -115,22 +116,160 @@ 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.""" +def _annotation_contexts(query_obj: MagicMock) -> list[Any]: + return [ + call.kwargs["annotation_context"] for call in query_obj.cache_key.call_args_list + ] + + +def _access_denied() -> SupersetSecurityException: + return SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + message="denied", + level=ErrorLevel.ERROR, + ) + ) + + +def test_annotation_cache_key_dedupes_native_layer_across_users(processor): + """Two users who can both read annotations share one cache entry. + + NATIVE annotation records are global, so the key binds only the + ``can_read`` annotation permission, not the requesting user id -- avoiding + a per-user copy of the (potentially large) cached result. + """ query_obj = MagicMock() query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a", "value": 1}] + with patch("superset.common.query_context_processor.security_manager") as sm: + sm.can_access = MagicMock(return_value=True) + processor.query_cache_key(query_obj) + processor.query_cache_key(query_obj) + contexts = _annotation_contexts(query_obj) + assert contexts[0] == contexts[1] == {"annotation_read": True} + + +def test_annotation_cache_key_separates_native_layer_by_permission(processor): + """A user who cannot read annotations must not share the cache entry of a + user who can, preserving the annotation read gate from the fetch path.""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "NATIVE", "name": "a", "value": 1}] + with patch("superset.common.query_context_processor.security_manager") as sm: + sm.can_access = MagicMock(side_effect=[True, False]) + processor.query_cache_key(query_obj) + processor.query_cache_key(query_obj) + contexts = _annotation_contexts(query_obj) + assert contexts[0] != contexts[1] + + +def test_annotation_cache_key_dedupes_chart_layer_for_shared_scope(processor): + """Chart-backed layers dedupe when access and the referenced chart's data + cache key (RLS/Jinja scope) match across users.""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "line", "name": "a", "value": 7}] + chart = MagicMock() + chart.get_query_context.return_value.queries = [MagicMock()] + chart.get_query_context.return_value.query_cache_key.return_value = "ak" with ( + patch("superset.common.query_context_processor.security_manager") as sm, patch( - "superset.common.query_context_processor.get_user_id", - side_effect=[1, 2], + "superset.common.query_context_processor.ChartDAO.find_by_id", + return_value=chart, ), - patch("superset.common.query_context_processor.security_manager"), ): + sm.raise_for_access = MagicMock(return_value=None) 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 - ] + contexts = _annotation_contexts(query_obj) + assert contexts[0] == contexts[1] + assert contexts[0]["source_scope"]["7"] == {"access": True, "data_key": ["ak"]} + + +def test_annotation_cache_key_separates_chart_layer_by_rls(processor): + """Users whose annotation-source RLS differs get distinct cache keys, since + the referenced chart's own cache key differs (preserves RLS correctness).""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "line", "name": "a", "value": 7}] + chart = MagicMock() + chart.get_query_context.return_value.queries = [MagicMock()] + chart.get_query_context.return_value.query_cache_key.side_effect = ["ak1", "ak2"] + with ( + patch("superset.common.query_context_processor.security_manager") as sm, + patch( + "superset.common.query_context_processor.ChartDAO.find_by_id", + return_value=chart, + ), + ): + sm.raise_for_access = MagicMock(return_value=None) + processor.query_cache_key(query_obj) + processor.query_cache_key(query_obj) + contexts = _annotation_contexts(query_obj) + # Assert the concrete scope so this fails on the pre-fix key shape (which has + # no ``source_scope``) rather than passing merely because two MagicMock RLS + # objects happen to differ. + assert contexts[0]["source_scope"]["7"] == {"access": True, "data_key": ["ak1"]} + assert contexts[1]["source_scope"]["7"] == {"access": True, "data_key": ["ak2"]} + assert contexts[0] != contexts[1] + + +def test_annotation_cache_key_separates_chart_layer_by_access(processor): + """A user denied access to the annotation-referenced chart must not read the + cache entry of a user who has access.""" + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "line", "name": "a", "value": 7}] + chart = MagicMock() + chart.get_query_context.return_value.queries = [MagicMock()] + chart.get_query_context.return_value.query_cache_key.return_value = "ak" + with ( + patch("superset.common.query_context_processor.security_manager") as sm, + patch( + "superset.common.query_context_processor.ChartDAO.find_by_id", + return_value=chart, + ), + ): + sm.raise_for_access = MagicMock(side_effect=[None, _access_denied()]) + processor.query_cache_key(query_obj) + processor.query_cache_key(query_obj) + contexts = _annotation_contexts(query_obj) + # Assert the concrete scope so this fails on the pre-fix key shape (which has + # no ``source_scope``) and genuinely guards the access dimension. + assert contexts[0]["source_scope"]["7"] == {"access": True, "data_key": ["ak"]} + assert contexts[1]["source_scope"]["7"] == {"access": False, "data_key": ["ak"]} + + +def test_annotation_cache_key_promiscuous_viewer_not_collapsed_with_denied(processor): + """A promiscuous-mode chart viewer must not share a denied user's entry. + + Under ``ENABLE_VIEWERS`` + ``VIEWER_PROMISCUOUS_MODE`` the annotation fetch + grants a chart viewer access through ``has_promiscuous_chart_access()`` -- + an OR-branch of ``raise_for_access`` that ``can_access_datasource`` skips. + Both users are denied at the datasource level, so keying on + ``can_access_datasource`` alone would collapse the promiscuous viewer (real + gate allows) onto the truly-denied user and serve them the cached payload. + Binding the real ``raise_for_access`` gate keeps the keys distinct. + """ + query_obj = MagicMock() + query_obj.annotation_layers = [{"sourceType": "line", "name": "a", "value": 7}] + chart = MagicMock() + chart.get_query_context.return_value.queries = [MagicMock()] + chart.get_query_context.return_value.query_cache_key.return_value = "ak" + with ( + patch("superset.common.query_context_processor.security_manager") as sm, + patch( + "superset.common.query_context_processor.ChartDAO.find_by_id", + return_value=chart, + ), + ): + # Datasource-level access is denied for both; the pre-fix logic would key + # both on can_access_datasource=False and collapse them. + sm.can_access_datasource = MagicMock(return_value=False) + # Real fetch gate: promiscuous viewer passes, denied user raises. + sm.raise_for_access = MagicMock(side_effect=[None, _access_denied()]) + processor.query_cache_key(query_obj) # promiscuous viewer + processor.query_cache_key(query_obj) # denied user + contexts = _annotation_contexts(query_obj) + assert contexts[0]["source_scope"]["7"] == {"access": True, "data_key": ["ak"]} + assert contexts[1]["source_scope"]["7"] == {"access": False, "data_key": ["ak"]} assert contexts[0] != contexts[1] diff --git a/tests/unit_tests/utils/cache_test.py b/tests/unit_tests/utils/cache_test.py index c1fbafb7cf40..370d4191a7a2 100644 --- a/tests/unit_tests/utils/cache_test.py +++ b/tests/unit_tests/utils/cache_test.py @@ -412,3 +412,66 @@ def test_set_and_log_cache_set_failure_logs(mocker: MockerFixture) -> None: mock_logger.warning.assert_called_once_with("Could not cache key %s", "my_key") mock_logger.exception.assert_called_once_with(boom) + + +def test_set_data_cache_if_within_size_stores_raw_value(mocker: MockerFixture) -> None: + """Under the cap the wrapper writes the raw value through unchanged -- no + ``dict``-wrapping like set_and_log_cache -- and reports it persisted.""" + from superset.utils.cache import set_data_cache_if_within_size + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=10 * 1024 * 1024) + cache_manager = mocker.patch("superset.utils.cache.cache_manager") + cache_manager.data_cache.set.return_value = True + payload = {"records": [1, 2, 3]} + + assert set_data_cache_if_within_size("k", payload, timeout=42) is True + + cache_manager.data_cache.set.assert_called_once_with("k", payload, timeout=42) + # The value is passed through byte-identically (same object, not wrapped). + assert cache_manager.data_cache.set.call_args.args[1] is payload + + +def test_set_data_cache_if_within_size_skips_oversized(mocker: MockerFixture) -> None: + """Over the cap the wrapper skips the write, counts the skip stat, and + reports the value was not persisted.""" + from superset.utils.cache import set_data_cache_if_within_size + + config = _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=10) + cache_manager = mocker.patch("superset.utils.cache.cache_manager") + + assert set_data_cache_if_within_size("k", {"records": "x" * 1000}) is False + + cache_manager.data_cache.set.assert_not_called() + config["STATS_LOGGER"].incr.assert_called_once_with("skip_cache_value_too_large") + + +def test_set_data_cache_if_within_size_disabled_no_serialization( + mocker: MockerFixture, +) -> None: + """With the cap disabled (``None``) the guard never serializes the value.""" + from superset.utils.cache import set_data_cache_if_within_size + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=None) + cache_manager = mocker.patch("superset.utils.cache.cache_manager") + cache_manager.data_cache.set.return_value = True + mock_dumps = mocker.patch("superset.utils.cache.pickle.dumps") + + assert set_data_cache_if_within_size("k", {"records": [1]}) is True + + mock_dumps.assert_not_called() + cache_manager.data_cache.set.assert_called_once() + + +def test_oversized_data_cache_value(mocker: MockerFixture) -> None: + """oversized_data_cache_value returns True only when the serialized value + exceeds the cap, and False (never blocking) when the cap is disabled.""" + from superset.utils.cache import oversized_data_cache_value + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=10) + assert oversized_data_cache_value("k", {"records": "x" * 1000}) is True + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=10 * 1024 * 1024) + assert oversized_data_cache_value("k", {"records": "x" * 1000}) is False + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=None) + assert oversized_data_cache_value("k", {"records": "x" * 1000}) is False