fix(security): grant SQL Lab query authors an explore access bypass (#39296)

superset/commands/explore/get.py

@@ -17,7 +17,7 @@
 import contextlib
 import logging
 from abc import ABC
-from typing import Any, cast, Optional
+from typing import Any, cast, Optional, TYPE_CHECKING
 
 from flask import request
 from flask_babel import lazy_gettext as _
@@ -38,6 +38,7 @@
 from superset.explore.exceptions import WrongEndpointError
 from superset.explore.permalink.exceptions import ExplorePermalinkGetFailedError
 from superset.extensions import security_manager
+from superset.models.sql_lab import Query
 from superset.superset_typing import ExplorableData
 from superset.utils import core as utils, json
 from superset.views.utils import (
@@ -46,9 +47,51 @@
     sanitize_datasource_data,
 )
 
+if TYPE_CHECKING:
+    from superset.models.slice import Slice
+
 logger = logging.getLogger(__name__)
 
 
+def _authorize_datasource(
+    datasource: "BaseDatasource | Query", slc: Optional["Slice"]
+) -> None:
+    """
+    Raise if the current user cannot access the resource being explored.
+
+    Split out of ``GetExploreCommand.run()`` so the authorship-bypass
+    routing below is testable without a request context.
+    """
+    if slc:
+        security_manager.raise_for_access(chart=slc)
+    elif isinstance(datasource, Query):
+        # A "query" datasource_type resolves to the actual SQL Lab
+        # Query row, not a registered dataset -- its .perm is a
+        # synthetic string no role is ever granted, so the generic
+        # datasource_access check below would always fail here.
+        # Route it through the same query-authorship bypass the
+        # explore/create-chart form-data step already applies (see
+        # superset/explore/utils.py::check_query_access), so the
+        # Explore page for that transition actually opens for the
+        # query's own author.
+        #
+        # The Query is still passed as ``datasource`` too: the
+        # ``EXTRA_RAISE_FOR_ACCESS_BYPASS`` hook only ever receives
+        # ``datasource`` (never ``query``), and before this reroute the
+        # Explore GET handed it the Query under that name. Dropping it
+        # would silently stop a deployment's custom bypass from seeing
+        # the query at all. The author bypass returns before the generic
+        # ``datasource`` perm check at the tail of ``raise_for_access``,
+        # so this does not weaken anything for anyone else.
+        security_manager.raise_for_access(
+            query=datasource,
+            datasource=datasource,
+            allow_query_authorship_bypass=True,
+        )
+    else:
+        security_manager.raise_for_access(datasource=datasource)
+
+
 class GetExploreCommand(BaseCommand, ABC):
     def __init__(
         self,
@@ -124,10 +167,7 @@ def run(self) -> Optional[dict[str, Any]]:  # noqa: C901
 
         if datasource:
             datasource_name = datasource.name
-            if slc:
-                security_manager.raise_for_access(chart=slc)
-            else:
-                security_manager.raise_for_access(datasource=datasource)
+            _authorize_datasource(datasource, slc)
 
         viz_type = form_data.get("viz_type")
         if (

superset/explore/utils.py

@@ -57,7 +57,12 @@ def check_query_access(query_id: int) -> Optional[bool]:
         query = QueryDAO.find_by_id(query_id, skip_base_filter=True)
         if query:
             try:
-                security_manager.raise_for_access(query=query)
+                # This is the one-time explore/create-chart transition the
+                # query-authorship bypass in raise_for_access is meant to
+                # cover; see allow_query_authorship_bypass's docstring there.
+                security_manager.raise_for_access(
+                    query=query, allow_query_authorship_bypass=True
+                )
             except TemplateError as ex:
                 # raise_for_access() Jinja-renders the query's SQL to resolve
                 # the tables it touches; a malformed template surfaces here as

superset/security/manager.py

@@ -4316,6 +4316,7 @@ def raise_for_access(  # noqa: C901
         schema: Optional[str] = None,
         template_params: Optional[dict[str, Any]] = None,
         force_dataset_match: bool = False,
+        allow_query_authorship_bypass: bool = False,
     ) -> None:
         """
         Raise an exception if the user cannot access the resource.
@@ -4338,12 +4339,25 @@ def raise_for_access(  # noqa: C901
             The default (False) preserves the historical semantics for
             chart-data, dataset CRUD, ``/table_metadata/``, and
             ``/select_star/``.
+        :param allow_query_authorship_bypass: When True, a SQL Lab query's
+            own author is granted access to that exact, already-succeeded
+            query without an additional dataset-level ``datasource_access``
+            grant. Deliberately opt-in and scoped to the one-time
+            explore/create-chart transition: repeat, ongoing access to a
+            query-backed chart or dashboard (fetching its data, exporting
+            it) must keep going through the regular catalog/schema/
+            datasource_access checks below on every call, since authorship
+            alone does not track whether the author still holds that
+            access, and a query's SQL may be templated such that the
+            tables actually touched depend on parameters supplied at
+            request time.
         :raises SupersetSecurityException: If the user cannot access the resource
         """
         # pylint: disable=import-outside-toplevel
         from flask import current_app
 
         from superset import is_feature_enabled
+        from superset.common.db_query_status import QueryStatus
         from superset.connectors.sqla.models import SqlaTable
         from superset.models.dashboard import Dashboard
         from superset.models.slice import Slice
@@ -4366,6 +4380,14 @@ def raise_for_access(  # noqa: C901
                 )
                 return
 
+        # An ephemeral Query built below from a raw `sql=` string always
+        # stamps the *current* user as its `user_id`, since there's no real
+        # query to attribute authorship to yet -- that must not be confused
+        # with a genuine, previously-persisted Query the caller fetched by
+        # id (e.g. from SQL Lab history), whose authorship bypass further
+        # down is meant to reward a query that was already run and stored.
+        is_ephemeral_query = bool(sql and database)
+
         if sql and database:
             query = Query(
                 database=database,
@@ -4408,6 +4430,52 @@ def raise_for_access(  # noqa: C901
             if self.can_access_database(database):
                 return
 
+            # A SQL Lab query's own author should be able to explore/chart
+            # the exact query they already ran without an additional
+            # dataset-level ``datasource_access`` grant on every table it
+            # happens to touch. This mirrors the ownership bypass already
+            # granted to dataset owners via ``is_editor`` further below in
+            # this same method; the query path had no equivalent authorship
+            # bypass.
+            #
+            # Gated on the explicit ``allow_query_authorship_bypass`` opt-in
+            # rather than firing for every ``not force_dataset_match`` call:
+            # that flag is also False for call sites unrelated to the
+            # one-time explore/create-chart transition this bypass is meant
+            # to cover, e.g. fetching or exporting a query-backed chart's
+            # data (``query_context_processor.raise_for_access``), which
+            # runs on every view of an already-created chart or dashboard.
+            # Those call sites must keep re-checking catalog/schema/
+            # datasource_access on every call: authorship alone does not
+            # track whether the author still holds that access, and the
+            # query's SQL may be templated such that the tables actually
+            # touched depend on parameters supplied at request time.
+            #
+            # Does not apply to the ephemeral query built above either:
+            # that one's ``user_id`` is always the current user by
+            # construction, which would trivially bypass the very check
+            # being performed on a brand new, never-before-run SQL string.
+            #
+            # Also requires ``status == SUCCESS``: SQL Lab persists a Query
+            # row (stamped with the current user's id) *before* running the
+            # strict ``force_dataset_match`` check at execute time, and
+            # marks it FAILED rather than deleting it when that check
+            # denies the statement. Without this guard, authorship alone
+            # would let that same user replay the denied SQL through this
+            # non-strict path merely by revisiting the failed query's id,
+            # defeating the very check that just rejected it.
+            if (
+                query
+                and not is_ephemeral_query
+                and not force_dataset_match
+                and allow_query_authorship_bypass
+                and hasattr(query, "user_id")
+                and (user_id := get_user_id()) is not None
+                and query.user_id == user_id
+                and getattr(query, "status", None) == QueryStatus.SUCCESS
+            ):
+                return
+
             # Type narrow: this path only applies to SQL Lab Query objects
             if query and hasattr(query, "sql") and hasattr(query, "catalog"):
                 # Getting the default schema for a query is hard. Users can select the