sadpandajoe commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4169873660
##########
superset/common/query_context_processor.py:
##########
@@ -449,34 +481,189 @@ def query_cache_key(self, query_obj: QueryObject,
**kwargs: Any) -> str | None:
)
return cache_key
- def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str,
Any]:
+ def annotation_cache_key(self, query_obj: QueryObject) -> str | None:
+ """
+ Cache key for this query's annotation-layer payload, or ``None`` when
+ the query has no annotation layers.
+
+ Annotation payloads are fetched under the requesting user's access
+ scope, which is a stricter security requirement than the dataframe
+ itself has. Keying them separately from :meth:`query_cache_key` keeps
+ that scoping from forcing every distinct viewer of an annotated chart
+ onto their own full copy of the (potentially much larger) shared
+ dataframe: users with the same access scope share this key too.
"""
- Cache-key material binding cached annotation data to its security
- context.
+ if not query_obj or not query_obj.annotation_layers:
+ return None
+ return self.query_cache_key(
+ query_obj,
annotation_context=self._annotation_cache_context(query_obj)
+ )
- 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.
+ def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str,
Any]:
+ """
+ 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.
+
+ * 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 ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
):
continue
layer_value = layer.get("value")
+ source_scope[str(layer_value)] =
self._annotation_source_scope(layer)
+ if source_scope:
+ context["source_scope"] = source_scope
+
+ return context
+
+ def _annotation_source_scope(self, layer: dict[str, 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. When the chart has a
+ saved query context, this runs the *same* authorization path
+ :meth:`get_viz_annotation_data` executes
+ (``QueryContext.raise_for_access``) rather than the coarser
+ :meth:`SecurityManager.can_access_datasource` — a requester whose
+ access comes from a dashboard/viewer-promiscuous-mode bypass (which
+ depends on the chart's own saved ``form_data``, e.g. its
+ ``slice_id``/``dashboardId``) would otherwise still fail that coarser,
+ context-free check and collapse onto the same denied scope as a
+ genuinely unauthorized requester, letting the latter read the
+ former's cached payload. ``data_key`` is the annotation chart's own
+ query cache key(s) — derived from the same, override-applied query
+ objects actually executed (see :meth:`_apply_annotation_overrides`),
+ so it captures the datasource version, RLS clauses, and any per-user
+ Jinja/virtual-dataset RLS material exactly as the live fetch would,
+ including material an override only introduces at a finer grain.
+ Reusing this logic avoids re-deriving it and automatically inherits
+ any future correctness fixes made there.
+ """
+ layer_value = layer.get("value")
+ datasource = None
+ try:
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
+ # resolved_datasource, not datasource: the latter is pinned to
+ # table-backed datasources and resolves to None for a
+ # semantic-view-backed chart, which would otherwise collapse
+ # every requester onto the same {access: None, data_key: None}
+ # scope below regardless of their actual access.
+ datasource = chart.resolved_datasource if chart else None
+ if chart is None or datasource is None:
+ return {"access": None, "data_key": None}
Review Comment:
A chart can have its datasource_id cleared while its saved query context
still targets a valid dataset, so this gives allowed and denied viewers the
same unscoped annotation key; after an allowed viewer warms it, the denied
viewer receives those rows without source authorization. Should unresolved
scope, including derivation-error fallbacks, disable annotation-cache reuse
rather than produce a shared identity?
##########
superset/common/query_context_processor.py:
##########
@@ -449,34 +481,189 @@ def query_cache_key(self, query_obj: QueryObject,
**kwargs: Any) -> str | None:
)
return cache_key
- def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str,
Any]:
+ def annotation_cache_key(self, query_obj: QueryObject) -> str | None:
+ """
+ Cache key for this query's annotation-layer payload, or ``None`` when
+ the query has no annotation layers.
+
+ Annotation payloads are fetched under the requesting user's access
+ scope, which is a stricter security requirement than the dataframe
+ itself has. Keying them separately from :meth:`query_cache_key` keeps
+ that scoping from forcing every distinct viewer of an annotated chart
+ onto their own full copy of the (potentially much larger) shared
+ dataframe: users with the same access scope share this key too.
"""
- Cache-key material binding cached annotation data to its security
- context.
+ if not query_obj or not query_obj.annotation_layers:
+ return None
+ return self.query_cache_key(
+ query_obj,
annotation_context=self._annotation_cache_context(query_obj)
+ )
- 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.
+ def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str,
Any]:
+ """
+ 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.
+
+ * 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 ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
):
continue
layer_value = layer.get("value")
+ source_scope[str(layer_value)] =
self._annotation_source_scope(layer)
+ if source_scope:
+ context["source_scope"] = source_scope
+
+ return context
+
+ def _annotation_source_scope(self, layer: dict[str, 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. When the chart has a
+ saved query context, this runs the *same* authorization path
+ :meth:`get_viz_annotation_data` executes
+ (``QueryContext.raise_for_access``) rather than the coarser
+ :meth:`SecurityManager.can_access_datasource` — a requester whose
+ access comes from a dashboard/viewer-promiscuous-mode bypass (which
+ depends on the chart's own saved ``form_data``, e.g. its
+ ``slice_id``/``dashboardId``) would otherwise still fail that coarser,
+ context-free check and collapse onto the same denied scope as a
+ genuinely unauthorized requester, letting the latter read the
+ former's cached payload. ``data_key`` is the annotation chart's own
+ query cache key(s) — derived from the same, override-applied query
+ objects actually executed (see :meth:`_apply_annotation_overrides`),
+ so it captures the datasource version, RLS clauses, and any per-user
+ Jinja/virtual-dataset RLS material exactly as the live fetch would,
+ including material an override only introduces at a finer grain.
+ Reusing this logic avoids re-deriving it and automatically inherits
+ any future correctness fixes made there.
+ """
+ layer_value = layer.get("value")
+ datasource = None
+ try:
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
+ # resolved_datasource, not datasource: the latter is pinned to
+ # table-backed datasources and resolves to None for a
+ # semantic-view-backed chart, which would otherwise collapse
+ # every requester onto the same {access: None, data_key: None}
+ # scope below regardless of their actual access.
+ datasource = chart.resolved_datasource if chart else None
+ if chart is None or datasource is None:
+ return {"access": None, "data_key": None}
+
+ annotation_query_context = chart.get_query_context()
+ if annotation_query_context is not None:
+ self._apply_annotation_overrides(annotation_query_context,
layer)
+ try:
+ annotation_query_context.raise_for_access()
+ access: Any = True
+ except SupersetSecurityException:
+ access = False
+ data_key: Any = [
+ annotation_query_context.query_cache_key(query_object)
Review Comment:
For a saved samples context, execution clears metrics and selects all
columns after this key is derived; an RLS template that calls current_user_id()
only when metrics are absent therefore produces user-scoped rows under a shared
annotation key. Should the scope key use the result handler’s prepared query,
matching what actually executes?
##########
superset/common/query_context_processor.py:
##########
@@ -449,34 +481,189 @@ def query_cache_key(self, query_obj: QueryObject,
**kwargs: Any) -> str | None:
)
return cache_key
- def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str,
Any]:
+ def annotation_cache_key(self, query_obj: QueryObject) -> str | None:
+ """
+ Cache key for this query's annotation-layer payload, or ``None`` when
+ the query has no annotation layers.
+
+ Annotation payloads are fetched under the requesting user's access
+ scope, which is a stricter security requirement than the dataframe
+ itself has. Keying them separately from :meth:`query_cache_key` keeps
+ that scoping from forcing every distinct viewer of an annotated chart
+ onto their own full copy of the (potentially much larger) shared
+ dataframe: users with the same access scope share this key too.
"""
- Cache-key material binding cached annotation data to its security
- context.
+ if not query_obj or not query_obj.annotation_layers:
+ return None
+ return self.query_cache_key(
+ query_obj,
annotation_context=self._annotation_cache_context(query_obj)
+ )
- 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.
+ def _annotation_cache_context(self, query_obj: QueryObject) -> dict[str,
Any]:
+ """
+ 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.
+
+ * 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 ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
):
continue
layer_value = layer.get("value")
+ source_scope[str(layer_value)] =
self._annotation_source_scope(layer)
Review Comment:
When two layers reference the same chart with different overrides, the later
layer overwrites the earlier layer’s scope here; if only the earlier override
activates per-user Jinja/RLS, the second viewer can receive the first viewer’s
rows. Should the cache context retain every layer’s overridden scope rather
than just one per chart ID?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
