rusackas commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3707412059
##########
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:
The production fix landed in this PR already, this isn't test-only anymore.
The unit test's green against current code.
##########
tests/integration_tests/explore/form_data/commands_tests.py:
##########
@@ -345,3 +371,140 @@ 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()
+ # raise_for_access qualifies the query's tables against the
+ # database's default catalog (e.g. the Postgres database name),
+ # so the granted schema_access permission must be built the same
+ # way -- get_schema_perm() falls back to the plain [db].[schema]
+ # form when the backend (e.g. sqlite, mysql) doesn't support
+ # catalogs at all.
+ view_menu_name = security_manager.get_schema_perm(
+ database.database_name, database.get_default_catalog(), 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:
Same fix as the thread above on this test, requiring `status == SUCCESS` for
the bypass means it no longer fires here, so this is actually testing
`schema_access` now.
##########
tests/integration_tests/explore/form_data/commands_tests.py:
##########
@@ -345,3 +371,140 @@ 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()
+ # raise_for_access qualifies the query's tables against the
+ # database's default catalog (e.g. the Postgres database name),
+ # so the granted schema_access permission must be built the same
+ # way -- get_schema_perm() falls back to the plain [db].[schema]
+ # form when the backend (e.g. sqlite, mysql) doesn't support
+ # catalogs at all.
+ view_menu_name = security_manager.get_schema_perm(
+ database.database_name, database.get_default_catalog(), 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,
Review Comment:
Same as above, the status fix closes this one too.
--
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]
