rusackas commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4316316304
##########
superset/common/query_context_processor.py:
##########
@@ -331,6 +342,26 @@ def get_df_payload_result(
# nonce reads the freshly-cached result instead of recomputing
it.
self._mark_force_executed(query_obj, cache_key,
cache.result_persisted)
+ # Annotation data is fetched per requesting user (and, for chart-backed
+ # layers, scoped by the referenced chart datasource's RLS), so it is
+ # resolved and cached under its own entry — independent of whether the
+ # (shareable) dataframe above was a hit or a miss — rather than forcing
+ # every viewer of the same chart onto their own full dataframe copy.
+ annotation_data: dict[str, Any] = {}
+ if query_obj and annotation_key and cache.status != QueryStatus.FAILED:
+ try:
+ annotation_data = self._get_annotation_data_cached(
Review Comment:
I don't think the read-back runs the annotation query in the request. With
`force_cached=True` an annotation cache miss for B's scope raises
`CacheLoadError`, `_run_async` catches it and submits a fresh task under B,
which warms B's scoped entry. The cost is one extra task round trip for B after
joining A's task, not an HTTP-request timeout. The only live-in-request case is
the unresolved-scope fallback, which is deliberate. Happy to fold the
annotation key into the task key if you think the extra round trip matters, but
I'd rather not in this PR.
##########
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:
The annotation payload doesn't come from the host query's executed SQL. It's
built from each annotation layer's own source chart (its saved query context,
with overrides applied), and that's what `data_key` is derived from. So whether
the host's samples handler clears metrics doesn't change what the annotation
fetch runs. The host-side key material in there is just the shared base key, so
I don't think switching to the prepared query buys anything.
##########
superset/common/query_context_processor.py:
##########
@@ -332,6 +344,26 @@ def get_df_payload_result(
# nonce reads the freshly-cached result instead of recomputing
it.
self._mark_force_executed(query_obj, cache_key,
cache.result_persisted)
+ # Annotation data is fetched per requesting user (and, for chart-backed
+ # layers, scoped by the referenced chart datasource's RLS), so it is
+ # resolved and cached under its own entry — independent of whether the
+ # (shareable) dataframe above was a hit or a miss — rather than forcing
+ # every viewer of the same chart onto their own full dataframe copy.
+ annotation_data: dict[str, Any] = {}
+ if query_obj and annotation_key and cache.status != QueryStatus.FAILED:
+ try:
+ annotation_data = self._get_annotation_data_cached(
Review Comment:
Good catch. The sink grabbed the first cursor regardless of which database
it came from, so with a warm dataframe an annotation cursor on a different
server would get recorded against the host DB. Fixed in 4190cb49a5:
`notify_cursor` now passes the cursor's database and the task's sink ignores
any cursor that isn't the task datasource's database. Added a test for it.
##########
superset/common/query_context_processor.py:
##########
@@ -470,72 +499,223 @@ 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 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 for this query's annotation-layer payload, or ``None`` when
+ the query has no annotation layers or when some layer's access scope
+ could not be resolved (the payload is then never cached, see
+ :meth:`_annotation_source_scope`).
+
+ 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.
"""
- source_rls: dict[str, list[str] | None] = {}
- source_versions: dict[str, tuple[str, str]] = {}
- metadata_versions: dict[str, str] = {}
+ if not query_obj or not query_obj.annotation_layers:
+ return None
+ if (annotation_context := self._annotation_cache_context(query_obj))
is None:
+ return None
+ return self.query_cache_key(query_obj,
annotation_context=annotation_context)
+
+ def _annotation_cache_context(
+ self, query_obj: QueryObject
+ ) -> dict[str, Any] | None:
+ """
+ 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`, recorded per *layer* (not per
+ referenced chart) since two layers can reference the same chart
+ with different overrides, and so different scopes.
+
+ Returns ``None`` when any layer's scope cannot be resolved, meaning
+ the payload must not be cached at all.
+ """
+ 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: list[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")
+ if (scope := self._annotation_source_scope(layer)) is None:
+ return None
+ source_scope.append({"value": str(layer.get("value")), **scope})
+ if source_scope:
+ context["source_scope"] = source_scope
+
+ return context
+
+ def _annotation_source_scope(self, layer: dict[str, Any]) -> dict[str,
Any] | None:
+ """
+ Access and data-identity cache-key material for one chart-backed
+ annotation layer, or ``None`` when it cannot be resolved.
+
+ ``None`` means "do not cache": a missing chart, a chart with neither
+ a saved query context nor a resolvable datasource, or any error while
+ deriving the scope would otherwise collapse an authorized and a
+ denied requester onto one shared identity, letting the latter read
+ the former's warmed payload. The caller fetches live instead.
+
+ ``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")
+ try:
chart = (
ChartDAO.find_by_id(layer_value) if layer_value is not None
else None
)
- annotation_datasource: Datasource | None = (
- chart.resolved_datasource if chart else None
- )
- if chart and isinstance(chart.query_context, str):
+ if chart is None:
+ return None
+
+ # The saved query context is what the live fetch executes and
+ # authorizes against, so it is consulted first, independent of
+ # the chart's own datasource pointer: a chart can have its
+ # ``datasource_id`` cleared while its saved query context still
+ # targets a valid dataset, and the live fetch would still work.
+ annotation_query_context = chart.get_query_context()
+ if annotation_query_context is not None:
+ self._apply_annotation_overrides(annotation_query_context,
layer)
try:
- source: dict[str, Any] = json.loads(chart.query_context)[
- "datasource"
- ]
- annotation_datasource = DatasourceDAO.get_datasource(
- source["type"], source["id"]
- )
- except (
- ValueError,
- TypeError,
- KeyError,
- DatasourceNotFound,
- DatasourceTypeNotSupportedError,
- DatasourceValueIsIncorrect,
- ):
- annotation_datasource = None
+ annotation_query_context.raise_for_access()
+ 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
+ ]
+ annotation_datasource: Any =
annotation_query_context.datasource
+ else:
+ # Fall back to the RLS-clause identity when the chart has no
+ # saved query context to key on. resolved_datasource, not
+ # datasource: the latter is pinned to table-backed
+ # datasources and resolves to None for a semantic-view-backed
+ # chart.
+ datasource = chart.resolved_datasource
+ if datasource is None:
+ return None
+ access = security_manager.can_access_datasource(datasource)
+ data_key = security_manager.get_rls_cache_key(datasource)
+ annotation_datasource = datasource
+ scope: dict[str, Any] = {"access": access, "data_key": data_key}
+ # Semantic-view results are versioned by the layer's metadata
+ # generation and result discriminator, which the datasource-level
+ # key material above does not carry.
if isinstance(annotation_datasource, SemanticView):
- metadata_versions[str(layer_value)] = (
- annotation_datasource.metadata_generation
- )
- source_rls[str(layer.get("value"))] = (
- security_manager.get_rls_cache_key(annotation_datasource)
- if annotation_datasource
- else None
+ scope["metadata_version"] =
annotation_datasource.metadata_generation
+ if (
+ discriminator :=
annotation_datasource.result_cache_discriminator
+ ) is not None:
+ scope["result_version"] = discriminator
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Derivation can fail well beyond SupersetException: the DAO
+ # lookup and lazy ``datasource`` load are themselves live DB
+ # queries, and the RLS lookup / a virtual dataset's
+ # get_extra_cache_keys() renders Jinja, either of which can raise
+ # a driver/template error. None of that should ever 500 the whole
+ # chart-data request; fail closed instead. A fallback scope
+ # shared by every requester whose derivation failed would still
+ # be a shared identity, so the answer is "no caching", not a
+ # substitute key.
+ logger.warning(
+ "Could not derive annotation cache key for chart %s; "
+ "annotation data for this request will not be cached",
+ layer_value,
+ exc_info=True,
)
- if isinstance(annotation_datasource, SemanticView):
- discriminator: tuple[str, str] | None = (
- annotation_datasource.result_cache_discriminator
+ return None
+ return scope
+
+ def _get_annotation_data_cached(
+ self,
+ query_obj: QueryObject,
+ cache_key: str | None,
+ force_query: bool,
+ force_cached: bool | None,
+ timeout: int | None,
+ datasource_uid: str | None,
+ ) -> dict[str, Any]:
+ """
+ Fetch this query's annotation-layer payload, cached under its own
+ (user/RLS-scoped) entry, separate from the shared dataframe cache.
+
+ A ``None`` ``cache_key`` means some layer's access scope could not be
+ resolved (see :meth:`_annotation_source_scope`), so there is no key
+ under which a cached payload could be safely shared: an unresolved
+ scope looks the same for an authorized and a denied requester. The
+ payload is then fetched live and never cached. That holds under
+ ``force_cached`` too: computing here beats failing the read-back of
+ an otherwise successful async task.
+ """
+ if cache_key is None:
+ return self.get_annotation_data(query_obj)
Review Comment:
Agreed the uncaught error was wrong. I didn't go with returning a cache
miss, because the read-back after a successful task re-probes with
`force_cached` too, so an unresolved scope would miss forever and keep
resubmitting tasks. Instead the async probe in `_run_async` now catches the
failed/validation errors and returns the same 400 the sync path does
(4190cb49a5), so B gets the 'chart not found' 400 rather than a 500.
##########
superset/common/query_context_processor.py:
##########
@@ -474,73 +503,226 @@ 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 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 for this query's annotation-layer payload, or ``None`` when
+ the query has no annotation layers or when some layer's access scope
+ could not be resolved (the payload is then never cached, see
+ :meth:`_annotation_source_scope`).
+
+ 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.
"""
- source_rls: dict[str, list[str] | None] = {}
- source_versions: dict[str, tuple[str, str]] = {}
- metadata_versions: dict[str, str] = {}
+ if not query_obj or not query_obj.annotation_layers:
+ return None
+ if (annotation_context := self._annotation_cache_context(query_obj))
is None:
+ return None
+ return self.query_cache_key(query_obj,
annotation_context=annotation_context)
+
+ def _annotation_cache_context(
+ self, query_obj: QueryObject
+ ) -> dict[str, Any] | None:
+ """
+ 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`, recorded per *layer* (not per
+ referenced chart) since two layers can reference the same chart
+ with different overrides, and so different scopes.
+
+ Returns ``None`` when any layer's scope cannot be resolved, meaning
+ the payload must not be cached at all.
+ """
+ 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: list[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")
+ if (scope := self._annotation_source_scope(layer)) is None:
+ return None
+ source_scope.append({"value": str(layer.get("value")), **scope})
+ if source_scope:
+ context["source_scope"] = source_scope
+
+ return context
+
+ def _annotation_source_scope(self, layer: dict[str, Any]) -> dict[str,
Any] | None:
+ """
+ Access and data-identity cache-key material for one chart-backed
+ annotation layer, or ``None`` when it cannot be resolved.
+
+ ``None`` means "do not cache": a missing chart, a chart with neither
+ a saved query context nor a resolvable datasource, or any error while
+ deriving the scope would otherwise collapse an authorized and a
+ denied requester onto one shared identity, letting the latter read
+ the former's warmed payload. The caller fetches live instead.
+
+ ``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")
+ try:
chart = (
ChartDAO.find_by_id(layer_value) if layer_value is not None
else None
)
- annotation_datasource: Datasource | None = (
- chart.resolved_datasource if chart else None
- )
- if chart and isinstance(chart.query_context, str):
+ if chart is None:
+ return None
+
+ # The saved query context is what the live fetch executes and
+ # authorizes against, so it is consulted first, independent of
+ # the chart's own datasource pointer: a chart can have its
+ # ``datasource_id`` cleared while its saved query context still
+ # targets a valid dataset, and the live fetch would still work.
+ annotation_query_context = chart.get_query_context()
+ if annotation_query_context is not None:
+ self._apply_annotation_overrides(annotation_query_context,
layer)
try:
- source: dict[str, Any] = json.loads(chart.query_context)[
- "datasource"
- ]
- annotation_datasource = DatasourceDAO.get_datasource(
- source["type"], source["id"]
- )
- except (
- ValueError,
- TypeError,
- KeyError,
- DatasourceNotFound,
- DatasourceTypeNotSupportedError,
- DatasourceValueIsIncorrect,
- ):
- annotation_datasource = None
+ 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:
That's real, but I think it's the same limitation the regular chart cache
has. `get_rls_cache_key` only covers RLS on the datasource itself, so an adhoc
subquery over another RLS-protected table isn't in any chart's
`query_cache_key`, including the source chart's own. `data_key` here is exactly
that key, so annotations aren't any weaker than the chart they reference.
Fixing it properly means teaching the cache key about tables inside adhoc
expressions, which I'd want to do as its own change rather than in this one.
--
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]
