sadpandajoe commented on code in PR #44406:
URL: https://github.com/apache/superset/pull/44406#discussion_r4151108664
##########
superset/common/query_context_processor.py:
##########
@@ -448,31 +479,167 @@ 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 ("line", "table"):
continue
layer_value = layer.get("value")
+ 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's datasource from
+ reading an authorized user's cached payload. ``data_key`` is the
+ annotation chart's own query cache key(s), which already capture the
+ datasource version, RLS clauses, and any per-user Jinja/virtual-dataset
+ RLS material — reusing it here avoids re-deriving that logic and
+ automatically inherits any future correctness fixes made there.
+ """
+ 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}
+
+ access = security_manager.can_access_datasource(datasource)
Review Comment:
With ENABLE_VIEWERS and VIEWER_PROMISCUOUS_MODE, a viewer of the referenced
chart and a non-viewer can both have `can_access_datasource(...) == False`, but
only the viewer passes the saved query context's authorization; after the
viewer warms this entry, the non-viewer receives its rows without that check.
Should cache hits validate the referenced query context, or should this scope
use the same contextual authorization as the live fetch?
##########
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:
Two viewers with different annotation RLS scopes can now join the same
dataframe-keyed async task, but it only warms the executing viewer's annotation
entry; the other viewer's post-success synchronous read-back then runs the
annotation query in the HTTP request and can time out even though the task
succeeded. Should each subscriber's scoped annotation data be populated
asynchronously before its task completion is reported?
##########
superset/common/query_context_processor.py:
##########
@@ -448,31 +479,167 @@ 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 ("line", "table"):
continue
layer_value = layer.get("value")
+ 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's datasource from
+ reading an authorized user's cached payload. ``data_key`` is the
+ annotation chart's own query cache key(s), which already capture the
+ datasource version, RLS clauses, and any per-user Jinja/virtual-dataset
+ RLS material — reusing it here avoids re-deriving that logic and
+ automatically inherits any future correctness fixes made there.
+ """
+ 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}
+
+ 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)
Review Comment:
A time-grain override can make this shared key omit per-user Jinja material:
if the saved monthly query's RLS skips `current_user_id()` but the executed
daily override calls it, the next viewer can receive the first viewer's
annotation rows. Should the scope be derived from the same overridden query
context that `get_viz_annotation_data()` executes?
--
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]
