sadpandajoe commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3684969679
##########
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.
Review Comment:
This test is intentionally red on the current implementation, so merging
this test-only change leaves the required unit-test suite permanently failing.
Could the corresponding production fix land in this PR, or should this assert
the current denial if the goal is only to document existing behavior?
##########
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:
This expectation would make authorship override every current data-access
check, but SQL Lab can retain a user-owned query as failed when authorization
rejects its SQL; the same row also outlives later permission revocation. Could
this instead cover a successfully executed query while preserving current table
access, plus denial cases for failed/revoked and other-user queries, so
authorship cannot replay rejected data through Explore?
##########
tests/integration_tests/explore/form_data/commands_tests.py:
##########
@@ -345,3 +371,74 @@ def test_delete_form_data_command_key_expired(self,
mock_g):
response = delete_command.run()
assert response is False # noqa: E712
+
+ def
test_create_form_data_command_schema_access_no_all_datasource_access(self):
+ """
+ Regression for #39296: a user who has schema_access on the schema a
+ SQL Lab query ran in (but neither all_datasource_access nor
+ datasource_access on a specific registered dataset, since an ad-hoc
+ query result is never registered as one) should still be able to
+ jump straight from SQL Lab to "Create Chart" -- i.e.
+ CreateFormDataCommand.run() with datasource_type=QUERY should not be
+ blocked purely for lacking all_datasource_access, as long as the
+ query's schema is one they're granted schema_access on.
+
+ This exercises the same non-strict (force_dataset_match=False)
+ fallthrough in SupersetSecurityManager.raise_for_access that
+ test_raise_for_access_force_dataset_match_denies_schema_only (in
+ security_tests.py) exercises for the strict SQL Lab path -- here we
+ confirm the *non*-strict Explore/"Create Chart" path grants access on
+ schema_access alone, without needing a registered dataset at all.
+ """
+ schema = get_example_default_schema()
+ database = get_example_database()
+ view_menu_name = f"[{database.database_name}].[{schema}]"
+
+ security_manager.add_role(FORM_DATA_SCHEMA_ACCESS_ROLE)
+ db.session.commit()
+ _grant_schema_access(view_menu_name)
+ gamma_user = security_manager.find_user(username="gamma")
+ gamma_user.roles.append(
+ security_manager.find_role(FORM_DATA_SCHEMA_ACCESS_ROLE)
+ )
+ db.session.commit()
+
+ query = Query(
+ sql="SELECT * FROM wb_health_population",
+ client_id="fd_sch_acc1",
+ database=database,
+ schema=schema,
+ user_id=gamma_user.id,
Review Comment:
Once the authorship bypass required by the unit test exists, making Gamma
both the active user and this query's author lets the test pass even if
`schema_access` stops working. Could this query be owned by a different user,
with a complementary no-schema denial, so the test actually pins the
schema-permission branch?
--
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]
