From 520418048bb77447d0ca20a834cf27ff91386f5f Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 17 Sep 2026 21:52:46 +0000 Subject: [PATCH 1/3] fix(cache): dedupe annotation-layer data cache keys across users When a chart has annotation layers, the DATA cache key bound the raw requesting user id, forcing one cache copy per user even when they would see identical annotation data. Bind the annotation *security scope* instead: NATIVE layers bind the `can_read` Annotation permission; chart-backed layers bind datasource access plus the annotation chart's own query cache key (which folds in datasource version, RLS clauses, and per-user Jinja/virtual-dataset RLS). Fall back closed on SupersetException. Also route three direct data_cache.set writers (SQL executor result cache, datasource column-values and metrics/dimensions endpoints) through a shared size-cap wrapper so oversized values can't bypass DATA_CACHE_MAX_VALUE_SIZE. Co-Authored-By: Claude Opus 4.8 --- superset/common/query_context_processor.py | 106 ++++++++++++++--- superset/datasource/api.py | 5 +- superset/sql/execution/executor.py | 10 +- superset/utils/cache.py | 68 ++++++++--- .../common/test_query_context_processor.py | 108 ++++++++++++++++-- 5 files changed, 249 insertions(+), 48 deletions(-) diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index 2c27fb9fb064..564d94de3f7e 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -60,7 +60,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 +427,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 +446,100 @@ 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 cached annotation data to the *security + scope* that produced it, rather than to the individual requesting user. + + Annotation payloads are fetched under the requesting user's security + context and stored on the same cache entry as the dataframe. Binding the + raw user id fanned the same (potentially large) result out to one cache + copy per user. Instead bind only the inputs that actually determine + which annotation data a user can see, so users who share that scope + share a cache entry while users with a different scope -- or no access + -- never read each other's data: + + * NATIVE layers expose global annotation records gated solely by the + ``can_read`` permission on ``Annotation``; a single access flag + captures the only user-dependent dimension. + * Chart-backed (``line``/``table``) layers run a query against the + referenced chart's datasource. Bind both whether the user can access + that datasource (its access check otherwise only runs on a cache miss, + not on a hit) and the annotation chart's own data cache key, which + already folds in the datasource's RLS clauses and any per-user + Jinja/virtual-dataset RLS predicates. """ - 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 + 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`` distinguishes users who may fetch the referenced chart's data + from those who may not, so a denied user never reads an authorized + user's cached annotation payload. ``data_key`` is the annotation chart's + own query cache key, which captures the datasource version, RLS clauses, + and any per-user Jinja/virtual-dataset RLS material -- everything that + makes the fetched annotation data differ between users. Users who match + on both dedupe onto a single cache entry. + """ + 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: + access = security_manager.can_access_datasource(datasource) + # The annotation chart's own query cache key already captures the + # datasource version, RLS clauses, and any per-user Jinja / + # virtual-dataset RLS material. Fall back to the RLS-clause identity + # only when the chart has no saved query context to key on. + annotation_query_context = chart.get_query_context() + data_key: Any = ( + [ + annotation_query_context.query_cache_key(query_object) + for query_object in annotation_query_context.queries + ] + if annotation_query_context is not None + else security_manager.get_rls_cache_key(datasource) ) - 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 + except SupersetException: + # Only the annotation fetch's own failure mode is swallowed here: + # ``get_viz_annotation_data`` surfaces exactly these + # ``SupersetException``-family errors (access, RLS, query build) and + # marks the result FAILED so nothing is persisted. Because key + # derivation fails on the same inputs, a fallback key is never used + # to store real data; it only needs to avoid silently deduping this + # scope onto a successfully-derived one, so it fails closed. Any + # other (unexpected) error propagates 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..77275333959b 100644 --- a/superset/sql/execution/executor.py +++ b/superset/sql/execution/executor.py @@ -1023,11 +1023,11 @@ def _store_in_cache( "total_execution_time_ms": result.total_execution_time_ms, } - cache_manager.data_cache.set( - cache_key, - cached_data, - timeout=timeout, - ) + # Guard against flooding the cache backend with an oversized result; + # mirrors the cap enforced by set_and_log_cache on 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..a314ffdf9d62 100644 --- a/superset/utils/cache.py +++ b/superset/utils/cache.py @@ -54,6 +54,56 @@ 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``. + + Centralizes the size guard so any DATA-cache writer -- including those that + store raw (non-``QueryCacheManager``) payloads and therefore cannot use + :func:`set_and_log_cache`, which wraps the value -- can skip oversized + entries and keep a single large result from flooding the cache backend + (e.g. Redis/Memcached). Returns ``False`` (never blocks) when the cap is + disabled (``DATA_CACHE_MAX_VALUE_SIZE`` is ``None``), so no serialization + overhead is incurred in that case. Emits the same warning and + ``skip_cache_value_too_large`` stat as the wrapped path when it trips. + """ + 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. + + A thin, shape-preserving wrapper around ``data_cache.set`` for writers that + store raw payloads outside the ``QueryCacheManager`` result contract (and so + cannot use :func:`set_and_log_cache`, which wraps the value in a dict). + Applying the same :func:`oversized_data_cache_value` guard here keeps a + single oversized entry from flooding the cache backend on those paths too. + + :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, @@ -91,21 +141,9 @@ def set_and_log_cache( # 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 + # next load instead of a cache hit. + 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..0ea5768740ff 100644 --- a/tests/unit_tests/common/test_query_context_processor.py +++ b/tests/unit_tests/common/test_query_context_processor.py @@ -115,25 +115,117 @@ 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 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.can_access_datasource = MagicMock(return_value=True) 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.can_access_datasource = MagicMock(return_value=True) + 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 datasource 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.can_access_datasource = MagicMock(side_effect=[True, False]) + 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_get_data_table_like(processor, mock_query_context): df = pd.DataFrame({"col1": [1, 2, 3], "col2": ["a", "b", "c"]}) coltypes = [GenericDataType.NUMERIC, GenericDataType.STRING] From 8c4495564279f64e9647f1eecd816d5c2eac26b9 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 17 Sep 2026 21:59:09 +0000 Subject: [PATCH 2/3] docs(cache): tighten annotation cache-key comments Co-Authored-By: Claude Opus 4.8 --- superset/common/query_context_processor.py | 61 ++++++++-------------- superset/sql/execution/executor.py | 3 +- superset/utils/cache.py | 26 ++++----- 3 files changed, 32 insertions(+), 58 deletions(-) diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index 564d94de3f7e..a8716b377df1 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -446,26 +446,18 @@ 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 the *security - scope* that produced it, rather than to the individual requesting user. - - Annotation payloads are fetched under the requesting user's security - context and stored on the same cache entry as the dataframe. Binding the - raw user id fanned the same (potentially large) result out to one cache - copy per user. Instead bind only the inputs that actually determine - which annotation data a user can see, so users who share that scope - share a cache entry while users with a different scope -- or no access - -- never read each other's data: - - * NATIVE layers expose global annotation records gated solely by the - ``can_read`` permission on ``Annotation``; a single access flag - captures the only user-dependent dimension. - * Chart-backed (``line``/``table``) layers run a query against the - referenced chart's datasource. Bind both whether the user can access - that datasource (its access check otherwise only runs on a cache miss, - not on a hit) and the annotation chart's own data cache key, which - already folds in the datasource's RLS clauses and any per-user - Jinja/virtual-dataset RLS predicates. + 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`. """ context: dict[str, Any] = {} @@ -492,13 +484,10 @@ 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`` distinguishes users who may fetch the referenced chart's data - from those who may not, so a denied user never reads an authorized - user's cached annotation payload. ``data_key`` is the annotation chart's - own query cache key, which captures the datasource version, RLS clauses, - and any per-user Jinja/virtual-dataset RLS material -- everything that - makes the fetched annotation data differ between users. Users who match - on both dedupe onto a single cache entry. + ``access`` keeps a user denied the referenced chart's datasource 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 @@ -507,10 +496,8 @@ def _annotation_source_scope(self, layer_value: Any) -> dict[str, Any]: try: access = security_manager.can_access_datasource(datasource) - # The annotation chart's own query cache key already captures the - # datasource version, RLS clauses, and any per-user Jinja / - # virtual-dataset RLS material. Fall back to the RLS-clause identity - # only when the chart has no saved query context to key on. + # Fall back to the RLS-clause identity when the chart has no saved + # query context to key on. annotation_query_context = chart.get_query_context() data_key: Any = ( [ @@ -521,14 +508,10 @@ def _annotation_source_scope(self, layer_value: Any) -> dict[str, Any]: else security_manager.get_rls_cache_key(datasource) ) except SupersetException: - # Only the annotation fetch's own failure mode is swallowed here: - # ``get_viz_annotation_data`` surfaces exactly these - # ``SupersetException``-family errors (access, RLS, query build) and - # marks the result FAILED so nothing is persisted. Because key - # derivation fails on the same inputs, a fallback key is never used - # to store real data; it only needs to avoid silently deduping this - # scope onto a successfully-derived one, so it fails closed. Any - # other (unexpected) error propagates rather than weakening the key. + # 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", diff --git a/superset/sql/execution/executor.py b/superset/sql/execution/executor.py index 77275333959b..f26a51735a9f 100644 --- a/superset/sql/execution/executor.py +++ b/superset/sql/execution/executor.py @@ -1023,8 +1023,7 @@ def _store_in_cache( "total_execution_time_ms": result.total_execution_time_ms, } - # Guard against flooding the cache backend with an oversized result; - # mirrors the cap enforced by set_and_log_cache on the chart-data path. + # 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) diff --git a/superset/utils/cache.py b/superset/utils/cache.py index a314ffdf9d62..f1f17292c2bb 100644 --- a/superset/utils/cache.py +++ b/superset/utils/cache.py @@ -57,14 +57,11 @@ def generate_cache_key(values_dict: dict[str, Any], key_prefix: str = "") -> str def oversized_data_cache_value(cache_key: str, cache_value: Any) -> bool: """Whether ``cache_value`` exceeds ``DATA_CACHE_MAX_VALUE_SIZE``. - Centralizes the size guard so any DATA-cache writer -- including those that - store raw (non-``QueryCacheManager``) payloads and therefore cannot use - :func:`set_and_log_cache`, which wraps the value -- can skip oversized - entries and keep a single large result from flooding the cache backend - (e.g. Redis/Memcached). Returns ``False`` (never blocks) when the cap is - disabled (``DATA_CACHE_MAX_VALUE_SIZE`` is ``None``), so no serialization - overhead is incurred in that case. Emits the same warning and - ``skip_cache_value_too_large`` stat as the wrapped path when it trips. + 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: @@ -88,11 +85,8 @@ def set_data_cache_if_within_size( ) -> bool: """Write to the DATA cache unless the value exceeds the size cap. - A thin, shape-preserving wrapper around ``data_cache.set`` for writers that - store raw payloads outside the ``QueryCacheManager`` result contract (and so - cannot use :func:`set_and_log_cache`, which wraps the value in a dict). - Applying the same :func:`oversized_data_cache_value` guard here keeps a - single oversized entry from flooding the cache backend on those paths too. + 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. """ @@ -138,10 +132,8 @@ 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. + # 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 From 9e447db41e915f1c854f58b7e5061189175aea78 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 17 Sep 2026 23:11:27 +0000 Subject: [PATCH 3/3] fix(cache): bind the real annotation-fetch gate in the cache key The chart-backed annotation cache-key bound access via can_access_datasource (form_data=None), which under-approximates the actual fetch gate. get_viz_annotation_data validates the annotation chart's query context, whose raise_for_access also honors has_promiscuous_chart_access (VIEWER_PROMISCUOUS_MODE) and guest-token scopes. Under those configs a promiscuous-granted viewer and a truly-denied user both fail can_access_datasource and would collapse onto one {access:False} entry, letting the denied user read the viewer's cached annotation payload (the real gate only runs on a cache miss). Bind access on the same raise_for_access(query_context=...) the fetch performs, keeping the except SupersetException fail-closed fallback. Also add focused tests for set_data_cache_if_within_size / oversized_data_cache_value. Co-Authored-By: Claude Opus 4.8 --- superset/common/query_context_processor.py | 46 +++++++++----- .../common/test_query_context_processor.py | 59 +++++++++++++++-- tests/unit_tests/utils/cache_test.py | 63 +++++++++++++++++++ 3 files changed, 147 insertions(+), 21 deletions(-) diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index a8716b377df1..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 @@ -484,10 +485,10 @@ 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's datasource 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. + ``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 @@ -495,18 +496,33 @@ def _annotation_source_scope(self, layer_value: Any) -> dict[str, Any]: return {"access": None, "data_key": None} try: - access = security_manager.can_access_datasource(datasource) - # Fall back to the RLS-clause identity when the chart has no saved - # query context to key on. annotation_query_context = chart.get_query_context() - data_key: Any = ( - [ - annotation_query_context.query_cache_key(query_object) - for query_object in annotation_query_context.queries - ] - if annotation_query_context is not None - else security_manager.get_rls_cache_key(datasource) - ) + 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 diff --git a/tests/unit_tests/common/test_query_context_processor.py b/tests/unit_tests/common/test_query_context_processor.py index 0ea5768740ff..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 @@ -121,6 +122,16 @@ def _annotation_contexts(query_obj: MagicMock) -> list[Any]: ] +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. @@ -166,7 +177,7 @@ def test_annotation_cache_key_dedupes_chart_layer_for_shared_scope(processor): return_value=chart, ), ): - sm.can_access_datasource = MagicMock(return_value=True) + 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) @@ -189,7 +200,7 @@ def test_annotation_cache_key_separates_chart_layer_by_rls(processor): return_value=chart, ), ): - sm.can_access_datasource = MagicMock(return_value=True) + 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) @@ -202,8 +213,8 @@ def test_annotation_cache_key_separates_chart_layer_by_rls(processor): def test_annotation_cache_key_separates_chart_layer_by_access(processor): - """A user denied access to the annotation-referenced datasource must not - read the cache entry of a user who has access.""" + """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() @@ -216,7 +227,7 @@ def test_annotation_cache_key_separates_chart_layer_by_access(processor): return_value=chart, ), ): - sm.can_access_datasource = MagicMock(side_effect=[True, False]) + 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) @@ -226,6 +237,42 @@ def test_annotation_cache_key_separates_chart_layer_by_access(processor): 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] + + 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] 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