rusackas commented on code in PR #42590:
URL: https://github.com/apache/superset/pull/42590#discussion_r3832517537
##########
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:
Added `test_query_authorship_bypass_does_not_cover_chart_data_fetch`,
matching successful author, no datasource permission, and it denies now that
the bypass needs the explicit flag.
##########
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:
Fixed, `_grant_schema_access` now reports whether it actually created the
permission-view, and teardown only deletes it when it did.
--
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]
