rusackas commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3707410675
##########
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()
Review Comment:
This mirrors `TestRolePermission` in `security_tests.py`, the pattern this
helper is based on, which also creates the role/perm view outside a try. Not
something I want to change just for this test.
##########
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="form_data_schema_access_test",
+ database=database,
+ schema=schema,
+ user_id=gamma_user.id,
+ )
+ db.session.add(query)
+ db.session.commit()
+
+ try:
+ with override_user(gamma_user):
+ # Sanity check: gamma should NOT have all_datasource_access,
+ # or this test wouldn't be exercising the reported gap.
+ assert not security_manager.can_access_all_datasources()
+
+ args = CommandParameters(
+ datasource_id=query.id,
+ datasource_type=DatasourceType.QUERY,
+ chart_id=None,
+ tab_id=1,
+ form_data=json.dumps(
+ {"datasource": f"{query.id}__{DatasourceType.QUERY}"}
+ ),
+ )
+ # Should NOT raise: schema_access on the query's schema is
+ # sufficient for the non-strict Explore access check, even
+ # without all_datasource_access or a registered dataset.
+ key = CreateFormDataCommand(args).run()
+ assert isinstance(key, str)
Review Comment:
Same as `test_create_form_data_command_type_as_string` a few tests up, that
one leaves its cache key behind too. Not a new pattern this PR introduces, so
I'll leave it as-is.
##########
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, fixed. Added a `status == SUCCESS` requirement to the bypass, so
a query that got denied at execute time and sits around FAILED can't be
replayed through this path. Leaving the later-revocation case alone though,
that's the same tradeoff the existing dataset-owner bypass already makes.
##########
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:
Good catch, fixed. Turns out the status fix above covers this too, a freshly
committed Query defaults to `status=pending`, so the authorship bypass doesn't
fire here anymore and this test actually exercises `schema_access`.
--
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]
