rusackas commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3832515977
##########
tests/unit_tests/explore/utils_test.py:
##########
@@ -362,3 +362,67 @@ def test_query_no_access(mocker: MockerFixture, client) ->
None:
datasource_id=1,
datasource_type=DatasourceType.QUERY,
)
+
+
+def test_unsaved_query_explore_allows_the_query_author(
+ mocker: MockerFixture, client
+) -> None:
+ """
+ Regression for #39296: clicking "Create Chart" straight from a SQL Lab
+ query (no "Save dataset" step first) sends ``DatasourceType.QUERY`` into
+ ``CreateFormDataCommand``, which is the command backing that button (see
+ ``superset/commands/explore/form_data/create.py``). That command calls
+ this exact ``check_access`` function with ``chart_id=None``.
+
+ Unlike the TABLE path (``check_access`` -> ``can_access_datasource`` ->
+ ``raise_for_access(datasource=...)``), which grants access to a
+ dataset's *owners* via ``is_editor`` regardless of catalog/schema/table
+ permissions, the QUERY path has no equivalent "you authored this" bypass:
+ ``raise_for_access``'s ``query=`` branch (``superset/security/manager.py``)
+ only ever checks catalog/schema/table-level ``datasource_access``, and
+ never looks at ``Query.user_id`` at all. So a user who just ran this
+ exact query in SQL Lab themselves (and therefore has execution rights on
+ the connection) but lacks that dataset-level permission is denied here,
+ even though the identical underlying data becomes explorable to them the
+ moment it's saved as a dataset, since ``populate_owners()``
+ (``superset/commands/utils.py``) would make them an owner at that point.
+ That inconsistency, not a missing owner field, is the crux of #39296.
+
+ This test sets the query's ``user_id`` to match the current user (i.e.
+ the user IS the query's own author) and asserts access should be
+ granted, the behavior a fix should produce. It's expected to currently
+ FAIL: no code path today grants a bypass for query authorship, so
+ ``raise_for_access`` denies even the query's own author. A red result
+ here is the TDD signal that the reported gap is real; a future fix
+ adding that bypass should turn this green.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.explore.utils import check_access as check_chart_access
+ from superset.models.sql_lab import Query
+
+ current_user = User(id=1)
+
+ database = mocker.MagicMock()
+ database.get_default_catalog.return_value = None
+ database.get_default_schema_for_query.return_value = "public"
+ mocker.patch(
+ query_find_by_id,
+ return_value=Query(
+ database=database, sql="select * from foo", user_id=current_user.id
+ ),
+ )
+ mocker.patch(query_datasources_by_name, return_value=[SqlaTable()])
+ mocker.patch(is_admin, return_value=False)
+ mocker.patch(is_editor, return_value=False)
+ # No catalog/schema/dataset-level datasource_access grant of any kind:
+ # the only thing that should let this through is query authorship.
+ mocker.patch(can_access, return_value=False)
+
+ with override_user(current_user):
+ # A user exploring a query they themselves just ran in SQL Lab
+ # should not be denied for lack of an unrelated dataset grant.
+ check_chart_access(
Review Comment:
Good catch. Scoped the bypass behind an explicit
`allow_query_authorship_bypass` flag, set only by the explore/create-chart call
site (`superset/explore/utils.py`). Chart-data fetch and export go through the
regular checks again.
##########
superset/security/manager.py:
##########
@@ -3905,6 +3914,47 @@ 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.
+ #
+ # Scoped to ``not force_dataset_match``: that flag is set by the
+ # call sites that execute a query or return its row data (SQL
+ # Lab execute, results/export, MetaDB), where every SQL Lab
+ # query's author trivially equals the current user and an
+ # unscoped bypass would erase the per-table
+ # catalog/schema/datasource_access checks below entirely. It is
+ # left unset only by the explore/form-data path this bypass is
+ # meant to cover.
+ #
+ # 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 (
Review Comment:
Same fix covers this. With the bypass scoped to the explore/create-chart
transition, `QueryContextProcessor.raise_for_access` never takes it, so a
templated chart-data request still gets the regular table checks.
--
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]
