sadpandajoe commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3830920693
##########
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:
The successful-status guard handles failed rows, but this still returns
before current table checks for query-backed chart data. After the user loses
access, the saved query can still be loaded and exported through that path;
could authorship be limited to the Create Chart transition or recheck current
table access?
##########
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 (
+ query
+ and not is_ephemeral_query
+ and not force_dataset_match
Review Comment:
Removing this guard would let a successful query author bypass strict
result, CSV, and streaming-export access, but the new tests only cover the
default path. Could this add a strict denial case with a matching successful
author and no datasource permission?
##########
tests/integration_tests/explore/form_data/commands_tests.py:
##########
@@ -27,14 +27,41 @@
from superset.commands.explore.form_data.get import GetFormDataCommand
from superset.commands.explore.form_data.parameters import CommandParameters
from superset.commands.explore.form_data.update import UpdateFormDataCommand
+from superset.common.db_query_status import QueryStatus
from superset.connectors.sqla.models import SqlaTable
from superset.models.slice import Slice
from superset.models.sql_lab import Query
from superset.utils import json
-from superset.utils.core import DatasourceType, get_example_default_schema
+from superset.utils.core import (
+ DatasourceType,
+ get_example_default_schema,
+ override_user,
+)
from superset.utils.database import get_example_database
from tests.integration_tests.base_tests import SupersetTestCase
+# Mirrors the SCHEMA_ACCESS_ROLE pattern in
tests/integration_tests/security_tests.py:
+# a role granting only schema_access on one schema, no all_datasource_access
and no
+# per-table datasource_access.
+FORM_DATA_SCHEMA_ACCESS_ROLE = "form_data_schema_access_role"
+
+
+def _grant_schema_access(view_menu_name: str) -> None:
+ permission = "schema_access"
+ security_manager.add_permission_view_menu(permission, view_menu_name)
+ perm_view = security_manager.find_permission_view_menu(permission,
view_menu_name)
+ security_manager.add_permission_role(
+ security_manager.find_role(FORM_DATA_SCHEMA_ACCESS_ROLE), perm_view
+ )
+
+
+def _revoke_schema_access(view_menu_name: str) -> None:
+ pv = security_manager.find_permission_view_menu("schema_access",
view_menu_name)
+ security_manager.del_permission_role(
+ security_manager.find_role(FORM_DATA_SCHEMA_ACCESS_ROLE), pv
+ )
+ security_manager.del_permission_view_menu("schema_access", view_menu_name)
Review Comment:
This teardown removes the schema permission-view menu even when it existed
before the test, which also drops its other role associations and makes later
schema-access tests order-dependent. Could it delete the menu only when this
fixture created it?
##########
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:
This returns before parsing or authorizing the SQL that chart data will
execute. A user can save a successful templated query, then supply different
template parameters that point at a table they cannot access; could this
revalidate the rendered SQL (or restrict the bypass to the exact executed SQL)?
--
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]
