You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by di...@apache.org on 2026/10/19 14:53:34 UTC
(superset) branch master updated: fix(security): don't let dataset editorship substitute for data access (#45266)
This is an automated email from the ASF dual-hosted git repository.
geido pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 56ff8920385 fix(security): don't let dataset editorship substitute for data access (#45266)
56ff8920385 is described below
commit 56ff8920385c5b123f85f8fd2ee5ea4ece172968
Author: Geidō <60...@users.noreply.github.com>
AuthorDate: Mon Oct 19 17:53:25 2026 +0300
fix(security): don't let dataset editorship substitute for data access (#45266)
Editorship/ownership of a dataset no longer substitutes for data access. Paths that return raw rows (SQL Lab, MCP execute_sql, MetaDB, alerts/reports, query-backed chart-data) or define/repoint a dataset over a table (create, update, duplicate, v1 import, legacy /datasource/save/) now require real data access — datasource_access, or catalog/schema access for dataset definition — not editorship of a same-named dataset. The catalog/schema fallthroughs and the #20382 'explore your own da [...]
A virtual dataset's SQL is authorized as it will execute: rendered with its template_params (parsed the way execution reads them, reserved processor names dropped), compared by value, and re-checked when its SQL, params, connection, catalog or schema change. Unreadable params fail closed as a 422; unchanged (echoed) legacy params do not block metadata-only edits.
---
superset/commands/dataset/create.py | 69 ++-
superset/commands/dataset/duplicate.py | 48 +-
superset/commands/dataset/exceptions.py | 46 ++
superset/commands/dataset/importers/v1/utils.py | 194 +++++--
superset/commands/dataset/update.py | 127 ++++-
superset/common/query_context_processor.py | 13 +-
superset/extensions/metadb.py | 3 +-
superset/jinja_context.py | 17 +
superset/models/helpers.py | 72 +++
superset/security/manager.py | 65 ++-
superset/translations/messages.pot | 3 +
superset/views/datasource/views.py | 72 ++-
tests/integration_tests/security_tests.py | 113 +++-
tests/unit_tests/commands/dataset/test_create.py | 268 ++++++++-
.../unit_tests/commands/dataset/test_duplicate.py | 136 ++++-
tests/unit_tests/commands/dataset/update_test.py | 475 +++++++++++++++-
.../common/test_query_context_processor.py | 34 ++
.../datasets/commands/importers/v1/import_test.py | 608 ++++++++++++++++++++-
tests/unit_tests/models/template_params_test.py | 68 +++
tests/unit_tests/security/manager_test.py | 174 ++++++
tests/unit_tests/views/datasource/views_test.py | 295 +++++++++-
21 files changed, 2778 insertions(+), 122 deletions(-)
diff --git a/superset/commands/dataset/create.py b/superset/commands/dataset/create.py
index 5f490c2d36b..cd366f5bf56 100644
--- a/superset/commands/dataset/create.py
+++ b/superset/commands/dataset/create.py
@@ -29,7 +29,10 @@ from superset.commands.dataset.exceptions import (
DatasetExistsValidationError,
DatasetInvalidError,
DatasetSoftDeletedTwinExistsError,
+ DatasetSqlRenderValidationError,
TableNotFoundValidationError,
+ TEMPLATE_RENDER_ERRORS,
+ TemplateParamsInvalidValidationError,
)
from superset.commands.utils import populate_subjects
from superset.daos.dataset import DatasetDAO
@@ -46,7 +49,10 @@ from superset.exceptions import (
SupersetTimeoutException,
)
from superset.extensions import security_manager
-from superset.models.helpers import json_to_dict
+from superset.models.helpers import (
+ parse_template_params,
+ template_params_for_access_check,
+)
from superset.sql.parse import process_jinja_sql, Table
from superset.utils.decorators import on_error, transaction
@@ -103,6 +109,23 @@ class CreateDatasetCommand(CreateMixin, BaseCommand):
schema = self._properties.get("schema")
table_name = self._properties["table_name"]
sql = self._properties.get("sql")
+ # The access check below must render the SQL with the same template
+ # params that will be stored and executed, so a templated table
+ # reference is authorized as the table it actually resolves to rather
+ # than its ``default()``. Only a virtual dataset's SQL renders them, so
+ # a physical dataset's params are left unparsed as before; params that
+ # cannot be read as execution reads them are rejected (fail closed)
+ # rather than authorized against a guess.
+ template_params: dict[str, Any] = {}
+ template_params_valid = True
+ if sql:
+ try:
+ template_params = parse_template_params(
+ self._properties.get("template_params")
+ )
+ except ValueError:
+ template_params_valid = False
+ exceptions.append(TemplateParamsInvalidValidationError())
# Validate/Populate database
database = DatasetDAO.get_database_by_id(database_id)
@@ -117,10 +140,6 @@ class CreateDatasetCommand(CreateMixin, BaseCommand):
if sql:
try:
- template_params_raw: str | None = self._properties.get(
- "template_params"
- )
- template_params = json_to_dict(template_params_raw or "")
parse_result = process_jinja_sql(
sql,
database,
@@ -219,27 +238,37 @@ class CreateDatasetCommand(CreateMixin, BaseCommand):
exceptions.append(TableNotFoundValidationError(table))
if sql:
- try:
- security_manager.raise_for_access(
- database=database,
- sql=sql,
- catalog=catalog,
- schema=schema,
- )
- except SupersetSecurityException as ex:
- exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
- except SupersetParseError as ex:
- exceptions.append(
- ValidationError(
- f"Invalid SQL: {ex.error.message}",
- field_name="sql",
+ # Unreadable params already failed validation above; skip the check
+ # rather than report an access decision made against other params.
+ if template_params_valid:
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=sql,
+ catalog=catalog,
+ schema=schema,
+ template_params=template_params_for_access_check(
+ template_params
+ ),
+ allow_editorship=False,
)
- )
+ except SupersetSecurityException as ex:
+ exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
+ except SupersetParseError as ex:
+ exceptions.append(
+ ValidationError(
+ f"Invalid SQL: {ex.error.message}",
+ field_name="sql",
+ )
+ )
+ except TEMPLATE_RENDER_ERRORS as ex:
+ exceptions.append(DatasetSqlRenderValidationError(ex))
elif database:
try:
security_manager.raise_for_access(
database=database,
table=table,
+ allow_editorship=False,
)
except SupersetSecurityException as ex:
exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
diff --git a/superset/commands/dataset/duplicate.py b/superset/commands/dataset/duplicate.py
index fb1cbb7048e..3a85b0d4bd0 100644
--- a/superset/commands/dataset/duplicate.py
+++ b/superset/commands/dataset/duplicate.py
@@ -30,14 +30,25 @@ from superset.commands.dataset.exceptions import (
DatasetExistsValidationError,
DatasetInvalidError,
DatasetNotFoundError,
+ DatasetSqlRenderValidationError,
+ TEMPLATE_RENDER_ERRORS,
+ TemplateParamsInvalidValidationError,
)
from superset.commands.exceptions import DatasourceTypeInvalidError
from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
from superset.daos.dataset import DatasetDAO
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
-from superset.exceptions import SupersetErrorException, SupersetSecurityException
+from superset.exceptions import (
+ SupersetErrorException,
+ SupersetParseError,
+ SupersetSecurityException,
+)
from superset.extensions import db
from superset.models.core import Database
+from superset.models.helpers import (
+ parse_template_params,
+ template_params_for_access_check,
+)
from superset.sql.parse import Table
from superset.utils.decorators import on_error, transaction
@@ -134,6 +145,13 @@ class DuplicateDatasetCommand(CreateMixin, BaseCommand):
except SupersetSecurityException as ex:
raise DatasetAccessDeniedError() from ex
self._base_model = base_model
+ # The copy is a new dataset the caller owns that reads the base's
+ # SQL, so authorize it as the create path would: real data access
+ # to the tables that SQL reads, not editorship of the base (which
+ # the check above accepts). Otherwise the copy would outlive the
+ # caller's access to the base.
+ if base_model.sql:
+ self._validate_copy_sql_access(base_model, exceptions)
if self._base_model and self._base_model.kind != "virtual":
exceptions.append(DatasourceTypeInvalidError())
@@ -167,3 +185,31 @@ class DuplicateDatasetCommand(CreateMixin, BaseCommand):
if exceptions:
raise DatasetInvalidError(exceptions=exceptions)
+
+ @staticmethod
+ def _validate_copy_sql_access(
+ base_model: SqlaTable, exceptions: list[ValidationError]
+ ) -> None:
+ """Authorize the SQL the copy will run, as stored by ``run``."""
+ try:
+ template_params = parse_template_params(base_model.template_params)
+ except ValueError:
+ exceptions.append(TemplateParamsInvalidValidationError())
+ return
+ try:
+ security_manager.raise_for_access(
+ database=base_model.database,
+ sql=base_model.sql.strip().strip(";"),
+ catalog=base_model.catalog,
+ schema=base_model.schema,
+ template_params=template_params_for_access_check(template_params),
+ allow_editorship=False,
+ )
+ except SupersetSecurityException as ex:
+ raise DatasetAccessDeniedError() from ex
+ except SupersetParseError as ex:
+ exceptions.append(
+ ValidationError(f"Invalid SQL: {ex.error.message}", field_name="sql")
+ )
+ except TEMPLATE_RENDER_ERRORS as ex:
+ exceptions.append(DatasetSqlRenderValidationError(ex))
diff --git a/superset/commands/dataset/exceptions.py b/superset/commands/dataset/exceptions.py
index 5086218e096..b68a82d36d1 100644
--- a/superset/commands/dataset/exceptions.py
+++ b/superset/commands/dataset/exceptions.py
@@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from flask_babel import lazy_gettext as _
+from jinja2.exceptions import TemplateError
from marshmallow.validate import ValidationError
from superset.commands.exceptions import (
@@ -26,8 +27,31 @@ from superset.commands.exceptions import (
ImportFailedError,
UpdateFailedError,
)
+from superset.exceptions import SupersetSyntaxErrorException, SupersetTemplateException
from superset.sql.parse import Table
+#: Errors from rendering a virtual dataset's Jinja SQL with its template params
+#: -- the set ``get_rendered_sql`` reports as a query error at execution. A
+#: define/repoint path that renders the SQL to authorize it reports these as
+#: invalid input rather than letting them surface as a 500.
+TEMPLATE_RENDER_ERRORS: tuple[type[Exception], ...] = (
+ TemplateError,
+ SupersetSyntaxErrorException,
+ SupersetTemplateException,
+ TypeError,
+)
+
+
+def get_template_render_error_msg(ex: Exception) -> str:
+ """The message ``get_rendered_sql`` would report for a render error."""
+ if isinstance(ex, TemplateError):
+ detail = ex.message or str(ex)
+ elif isinstance(ex, SupersetSyntaxErrorException):
+ detail = str(ex.errors[0].message if ex.errors else ex)
+ else:
+ detail = str(ex)
+ return str(_("Error while rendering virtual dataset query: %(msg)s", msg=detail))
+
def get_dataset_exist_error_msg(table: Table) -> str:
return _("Dataset %(table)s already exists", table=table)
@@ -45,6 +69,28 @@ class MultiCatalogDisabledValidationError(ValidationError):
)
+class TemplateParamsInvalidValidationError(ValidationError):
+ """
+ Marshmallow validation error for template params that are not a JSON object
+ """
+
+ def __init__(self) -> None:
+ super().__init__(
+ [_("Template parameters must be a valid JSON object.")],
+ field_name="template_params",
+ )
+
+
+class DatasetSqlRenderValidationError(ValidationError):
+ """
+ Marshmallow validation error for SQL that cannot be rendered with its
+ template params, and so cannot be access-checked
+ """
+
+ def __init__(self, ex: Exception) -> None:
+ super().__init__([get_template_render_error_msg(ex)], field_name="sql")
+
+
class DatabaseNotFoundValidationError(ValidationError):
"""
Marshmallow validation error for database does not exist
diff --git a/superset/commands/dataset/importers/v1/utils.py b/superset/commands/dataset/importers/v1/utils.py
index 8f984b98eef..ea77b3a5f0b 100644
--- a/superset/commands/dataset/importers/v1/utils.py
+++ b/superset/commands/dataset/importers/v1/utils.py
@@ -39,7 +39,9 @@ from superset import db, security_manager
from superset.commands.dataset.exceptions import (
DatasetAccessDeniedError,
DatasetForbiddenDataURI,
+ get_template_render_error_msg,
MultiCatalogDisabledValidationError,
+ TEMPLATE_RENDER_ERRORS,
)
from superset.commands.exceptions import ImportFailedError
from superset.commands.importers.exceptions import IncorrectFormatError
@@ -52,7 +54,12 @@ from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
from superset.daos.dataset import DatasetDAO
from superset.exceptions import SupersetParseError, SupersetSecurityException
from superset.models.core import Database
-from superset.models.helpers import ChildMultipleResultsFound
+from superset.models.helpers import (
+ ChildMultipleResultsFound,
+ parse_template_params,
+ template_params_equivalent,
+ template_params_for_access_check,
+)
from superset.sql.parse import Table
from superset.utils import json
from superset.utils.core import get_user
@@ -248,23 +255,142 @@ def validate_catalog(config: dict[str, Any]) -> None:
raise MultiCatalogDisabledValidationError()
-def _get_template_params(dataset: SqlaTable) -> dict[str, Any]:
- """
- Return a dataset's template params as a dict for the access check.
+def _readable_template_params(value: Any) -> dict[str, Any] | None:
+ """Template params as execution will read them, or ``None`` if it can't.
- Delegates to ``template_params_dict`` so the check reads the params the
- same way rendering does, but tolerates the two inputs that property does
- not guard: it raises on malformed JSON and passes non-dict JSON through.
- Either yields an empty dict so the check still runs against the raw SQL.
+ A config's params arrive as a dict, which the import stores as
+ ``json.dumps(value)`` (see ``JSON_KEYS``) and execution reads back with
+ ``json_to_dict``. That round trip can change values (``json_to_dict``
+ rewrites ``", }"`` even inside a string; ``NaN`` and dates serialize
+ differently), so a dict is put through the same round trip before it is
+ compared or rendered.
+
+ ``None`` never equals readable params, so unreadable params always count
+ as a change, and the binding check below refuses to render with them.
"""
try:
- params = dataset.template_params_dict
- except json.JSONDecodeError:
- logger.warning(
- "Unable to decode template_params for dataset %s", dataset.table_name
+ if isinstance(value, dict):
+ value = json.dumps(value)
+ return parse_template_params(value)
+ except (TypeError, ValueError):
+ return None
+
+
+def _raise_for_import_binding_access(
+ config: dict[str, Any], existing: SqlaTable | None
+) -> None:
+ """Authorize the table binding a dataset import will apply.
+
+ Runs BEFORE ``import_from_dict`` mutates the row, against the *requested*
+ (``config``) binding rather than the stored dataset: the per-table lookup
+ would otherwise resolve the dataset being imported itself (whose ``perm``
+ is schema-blind), letting a ``datasource_access`` holder repoint it to
+ another schema and read it through chart-data. ``allow_editorship=False``
+ means editorship of a same-named dataset is not accepted -- defining or
+ repointing a dataset over a table requires real data access to that
+ table. Skipped when the binding is unchanged (a metadata-only re-import is
+ not a repoint), matching ``UpdateDatasetCommand``.
+ """
+ database = db.session.query(Database).filter_by(id=config["database_id"]).first()
+ if database is None:
+ # A missing database is reported downstream by import_from_dict; there
+ # is no table binding to authorize here.
+ return
+
+ # Resolve the requested binding by MERGING the config over the stored row:
+ # an overwrite only applies the keys the upload carries, so an omitted
+ # field keeps its stored value. Resolving in isolation would mis-authorize
+ # (e.g. omitted ``sql`` -> treat a virtual dataset as physical; omitted
+ # ``template_params`` -> render against ``{}`` while the stored params are
+ # kept).
+ requested_sql = (
+ config["sql"] if "sql" in config else (existing.sql if existing else None)
+ ) or None
+ requested_table_name = config.get("table_name") or (
+ existing.table_name if existing else None
+ )
+ requested_schema = config.get("schema", existing.schema if existing else None)
+ requested_catalog = config.get("catalog", existing.catalog if existing else None)
+ stored_params = (
+ _readable_template_params(existing.template_params) if existing else {}
+ )
+ if "template_params" in config:
+ requested_params = _readable_template_params(config["template_params"])
+ # Re-importing the value already stored is not a change, even when it
+ # is a legacy non-object value (``import_from_dict`` stores it as
+ # ``json.dumps(config[...])``), matching UpdateDatasetCommand's echo
+ # handling. Otherwise compared type-sensitively the way execution reads
+ # them: ``1`` and ``true`` differ even though the dicts are
+ # Python-equal; an unreadable value that is a real change fails closed.
+ config_stored_form = (
+ json.dumps(config["template_params"])
+ if config["template_params"] is not None
+ else None
+ )
+ params_unchanged = (
+ existing is not None and config_stored_form == existing.template_params
+ ) or (
+ requested_params is not None
+ and stored_params is not None
+ and template_params_equivalent(requested_params, stored_params)
+ )
+ else:
+ requested_params = stored_params
+ params_unchanged = True
+
+ if existing is not None and (
+ config["database_id"] == existing.database_id
+ and requested_sql == (existing.sql or None)
+ # A virtual dataset's table_name is only a label -- its SQL is the
+ # binding -- so renaming one is not a repoint, as with PUT and
+ # ``/datasource/save/``.
+ and (bool(requested_sql) or requested_table_name == existing.table_name)
+ and requested_schema == existing.schema
+ and requested_catalog == existing.catalog
+ and params_unchanged
+ ):
+ # Binding unchanged -- a metadata-only re-import, not a repoint.
+ # The database and template_params are part of the binding: an
+ # overwrite that keeps table/schema/catalog/sql but points at a
+ # different database, or changes the params a templated FROM renders
+ # against, still repoints the dataset and must be re-authorized
+ # (matching UpdateDatasetCommand).
+ return
+
+ if requested_sql:
+ if requested_params is None:
+ # Execution would read these params differently than any access
+ # decision made here could, so fail closed as a bad payload.
+ raise IncorrectFormatError("Invalid template_params: must be a JSON object")
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=requested_sql,
+ # Empty strings would qualify table names against a nonexistent
+ # catalog/schema, so they are normalized to None.
+ catalog=requested_catalog or None,
+ schema=requested_schema or None,
+ template_params=template_params_for_access_check(requested_params),
+ allow_editorship=False,
+ )
+ except TEMPLATE_RENDER_ERRORS as ex:
+ # SQL that can't be rendered with its params can't be
+ # access-checked: fail closed as a bad payload, not a 500.
+ raise IncorrectFormatError(
+ f"Invalid SQL: {get_template_render_error_msg(ex)}"
+ ) from ex
+ elif isinstance(requested_table_name, str):
+ # A physical dataset always carries a table_name (the import schema
+ # requires it); the guard also narrows the type for the check below.
+ security_manager.raise_for_access(
+ database=database,
+ table=Table(
+ requested_table_name,
+ requested_schema or None,
+ requested_catalog or None,
+ ),
+ allow_editorship=False,
)
- return {}
- return params if isinstance(params, dict) else {}
def import_dataset( # noqa: C901
@@ -486,6 +612,19 @@ def import_dataset( # noqa: C901
# untrusted user imports validate the catalog, like the access checks below.
if not ignore_permissions:
validate_catalog(config)
+ # Authorize the table binding the import will apply BEFORE the row is
+ # mutated, against the requested config values (see
+ # ``_raise_for_import_binding_access``).
+ try:
+ _raise_for_import_binding_access(config, existing)
+ except SupersetSecurityException as ex:
+ raise DatasetAccessDeniedError() from ex
+ # SQL that can't be parsed or rendered can't be access-checked, so fail
+ # closed, but as an invalid payload (422) rather than an access denial.
+ except SupersetParseError as ex:
+ raise IncorrectFormatError(f"Invalid SQL: {ex.error.message}") from ex
+ except TemplateError as ex:
+ raise IncorrectFormatError(f"Invalid SQL: {ex}") from ex
# TODO (betodealmeida): move this logic to import_from_dict
config = config.copy()
@@ -583,33 +722,14 @@ def import_dataset( # noqa: C901
db.session.flush()
if not ignore_permissions:
+ # Object-level access to the dataset itself (editor/owner, #20382). A
+ # change to the table binding this dataset points at was authorized
+ # before the row was mutated, in ``_raise_for_import_binding_access``
+ # above.
try:
security_manager.raise_for_access(datasource=dataset)
- # For virtual datasets, also validate access to the tables the
- # SQL references, matching the create and update commands.
- if dataset.sql:
- security_manager.raise_for_access(
- database=dataset.database,
- sql=dataset.sql,
- # Empty strings would qualify table names against a
- # nonexistent catalog/schema, so they are normalized to
- # None to let the check fall back to the defaults.
- catalog=dataset.catalog or None,
- schema=dataset.schema or None,
- # Jinja-templated SQL only reveals the tables it really
- # references once the params are applied, so they are
- # forwarded rather than left to default to empty.
- template_params=_get_template_params(dataset),
- )
except SupersetSecurityException as ex:
raise DatasetAccessDeniedError() from ex
- # SQL that can't be parsed or rendered can't be access-checked, so fail
- # closed, but as an invalid payload (422) rather than an access denial
- # or a server error.
- except SupersetParseError as ex:
- raise IncorrectFormatError(f"Invalid SQL: {ex.error.message}") from ex
- except TemplateError as ex:
- raise IncorrectFormatError(f"Invalid SQL: {ex}") from ex
# `has_table` opens a live connection to the target database to run a
# schema-introspection query. Its result is only ever consulted below to
diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py
index a23a10ca4ca..e114d933123 100644
--- a/superset/commands/dataset/update.py
+++ b/superset/commands/dataset/update.py
@@ -42,8 +42,11 @@ from superset.commands.dataset.exceptions import (
DatasetMetricsNotFoundValidationError,
DatasetNotFoundError,
DatasetSoftDeletedTwinExistsError,
+ DatasetSqlRenderValidationError,
DatasetUpdateFailedError,
MultiCatalogDisabledValidationError,
+ TEMPLATE_RENDER_ERRORS,
+ TemplateParamsInvalidValidationError,
)
from superset.commands.utils import compute_subjects, raise_if_managed_externally
from superset.connectors.sqla.models import SqlaTable, validate_stored_expression
@@ -55,6 +58,11 @@ from superset.exceptions import (
SupersetSecurityException,
)
from superset.models.core import Database
+from superset.models.helpers import (
+ parse_template_params,
+ template_params_equivalent,
+ template_params_for_access_check,
+)
from superset.sql.parse import Table
from superset.utils.decorators import on_error, transaction
@@ -69,6 +77,24 @@ DEFAULT_FOLDER_UUIDS = frozenset(
)
+def _template_params_differ(requested: str | None, stored: str | None) -> bool:
+ """
+ Could replacing ``stored`` params with ``requested`` change what a
+ templated SQL renders?
+
+ An identical value is never a change -- including a legacy value that is
+ not a JSON object (the editor echoes the stored params back on every save,
+ so a metadata-only edit must not be treated as a repoint just because the
+ stored params predate object validation). Otherwise, compared the way
+ execution reads them, so re-serialized JSON with the same values is no
+ change while values of different JSON types (e.g. ``1`` vs ``true``, which
+ Jinja distinguishes) are a change.
+ """
+ if requested == stored:
+ return False
+ return not template_params_equivalent(requested, stored)
+
+
class UpdateDatasetCommand(UpdateMixin, BaseCommand):
def __init__(
self,
@@ -148,11 +174,21 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
database_changed = new_db_connection is not None
# Detect a caller-supplied change to the source binding, inspected
- # before the catalog normalization below injects derived values.
- source_changed = database_changed or any(
+ # before the catalog normalization below injects derived values. The
+ # connection, catalog and schema decide where a virtual dataset's SQL
+ # resolves its tables; ``table_name`` only names the table a physical
+ # dataset reads (for a virtual one it is a label). An empty catalog or
+ # schema means "none", as in ``/datasource/save/``: the editor can send
+ # ``None`` back for a stored ``""``, which is not a move.
+ location_changed = database_changed or any(
field in self._properties
- and self._properties[field] != getattr(self._model, field)
- for field in ("catalog", "schema", "table_name")
+ and (self._properties[field] or None)
+ != (getattr(self._model, field) or None)
+ for field in ("catalog", "schema")
+ )
+ source_changed = location_changed or (
+ "table_name" in self._properties
+ and self._properties["table_name"] != self._model.table_name
)
catalog, schema, table = self._resolve_catalog_schema_table(db, exceptions)
@@ -191,7 +227,7 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
):
self._validate_table_access(db, table, exceptions)
- self._validate_sql_access(db, catalog, schema, exceptions)
+ self._validate_sql_access(db, catalog, schema, location_changed, exceptions)
def _get_new_database_connection(
self, database_id: int | None, exceptions: list[ValidationError]
@@ -250,7 +286,9 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
exceptions: list[ValidationError],
) -> None:
try:
- security_manager.raise_for_access(database=new_db_connection, table=table)
+ security_manager.raise_for_access(
+ database=new_db_connection, table=table, allow_editorship=False
+ )
except SupersetSecurityException as ex:
exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
else:
@@ -260,39 +298,76 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
self, db: Database, table: Table, exceptions: list[ValidationError]
) -> None:
try:
- security_manager.raise_for_access(database=db, table=table)
+ security_manager.raise_for_access(
+ database=db, table=table, allow_editorship=False
+ )
except SupersetSecurityException as ex:
exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
- def _validate_sql_access(
+ def _validate_sql_access( # pylint: disable=too-many-arguments
self,
db: Database,
catalog: str | None,
schema: str | None,
+ location_changed: bool,
exceptions: list[ValidationError],
) -> None:
- """Validate SQL query access if SQL is being updated."""
+ """Validate access to the tables a virtual dataset's SQL will read.
+
+ A virtual dataset's executed tables depend on its SQL text, its
+ template params, and the connection, catalog and schema that
+ unqualified names resolve against, so a change to any of them
+ re-authorizes the SQL -- a params-only edit can repoint a templated
+ ``FROM`` and a schema-only edit can repoint ``SELECT * FROM t``. The
+ check uses the effective (post-update) values so it matches execution.
+ """
# we know we have a valid model
self._model = cast(SqlaTable, self._model)
- sql = self._properties.get("sql")
- if sql and sql != self._model.sql:
- try:
- security_manager.raise_for_access(
- database=db,
- sql=sql,
- catalog=catalog,
- schema=schema,
- )
- except SupersetSecurityException as ex:
- exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
- except SupersetParseError as ex:
- exceptions.append(
- ValidationError(
- f"Invalid SQL: {ex.error.message}",
- field_name="sql",
- )
+ effective_sql = self._properties.get("sql", self._model.sql)
+ if not effective_sql:
+ return # physical dataset; the table binding is gated elsewhere
+
+ sql_changed = "sql" in self._properties and (
+ self._properties["sql"] != self._model.sql
+ )
+ params_changed = "template_params" in self._properties and (
+ _template_params_differ(
+ self._properties["template_params"], self._model.template_params
+ )
+ )
+ if not (sql_changed or params_changed or location_changed):
+ return
+
+ try:
+ template_params = parse_template_params(
+ self._properties.get("template_params", self._model.template_params)
+ )
+ except ValueError:
+ # Fail closed: params execution would read differently can't be
+ # authorized.
+ exceptions.append(TemplateParamsInvalidValidationError())
+ return
+ try:
+ security_manager.raise_for_access(
+ database=db,
+ sql=effective_sql,
+ catalog=catalog,
+ schema=schema,
+ template_params=template_params_for_access_check(template_params),
+ allow_editorship=False,
+ )
+ except SupersetSecurityException as ex:
+ exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
+ except SupersetParseError as ex:
+ exceptions.append(
+ ValidationError(
+ f"Invalid SQL: {ex.error.message}",
+ field_name="sql",
)
+ )
+ except TEMPLATE_RENDER_ERRORS as ex:
+ exceptions.append(DatasetSqlRenderValidationError(ex))
def _validate_semantics(self, exceptions: list[ValidationError]) -> None:
# we know we have a valid model
diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py
index 0896c39e4dd..2659f658176 100644
--- a/superset/common/query_context_processor.py
+++ b/superset/common/query_context_processor.py
@@ -1033,7 +1033,18 @@ class QueryContextProcessor:
# caller is not allowed to access.
if self._qc_datasource.type == DatasourceType.QUERY:
try:
- security_manager.raise_for_access(query=self._qc_datasource)
+ # Query-backed chart-data runs the stored query's raw SQL, so
+ # editorship of a same-named dataset must not authorize its
+ # tables -- otherwise a no-table decoy dataset named after a
+ # table would let its editor read that table through a
+ # persisted query. ``allow_editorship=False`` stops honoring
+ # editorship while keeping the regular catalog/schema/
+ # datasource_access checks this ongoing-fetch path has always
+ # used (it is deliberately not the stricter
+ # ``force_dataset_match`` that SQL Lab execution uses).
+ security_manager.raise_for_access(
+ query=self._qc_datasource, allow_editorship=False
+ )
except TemplateError as ex:
raise SupersetTemplateException(str(ex)) from ex
else:
diff --git a/superset/extensions/metadb.py b/superset/extensions/metadb.py
index ebd256f6b63..13aa8902137 100644
--- a/superset/extensions/metadb.py
+++ b/superset/extensions/metadb.py
@@ -379,7 +379,8 @@ class SupersetShillelaghAdapter(Adapter):
# verify permissions. MetaDB returns row data through a regular
# query path, so the strict scoping used by SQL Lab applies here
# too: the referenced table must resolve to a Superset dataset the
- # user has datasource_access on (or owns).
+ # user has datasource_access on (editorship alone is not enough on
+ # this strict path).
table = Table(self.table, self.schema, self.catalog)
security_manager.raise_for_access(
database=database,
diff --git a/superset/jinja_context.py b/superset/jinja_context.py
index af8aa5b7ccc..d71e3cd37bc 100644
--- a/superset/jinja_context.py
+++ b/superset/jinja_context.py
@@ -953,6 +953,23 @@ class SupersetSandboxedEnvironment(SandboxedEnvironment):
return super().is_safe_attribute(obj, attr, value)
+# Names taken by ``BaseTemplateProcessor.__init__`` as structural arguments
+# rather than passed through ``set_context``, so they are never exposed as
+# Jinja variables in the render context. A define/repoint access check renders
+# the SQL with the submitted ``template_params``; dropping these first keeps
+# that render matching execution (see ``template_params_for_access_check``).
+RESERVED_TEMPLATE_PARAM_NAMES: frozenset[str] = frozenset(
+ {
+ "database",
+ "query",
+ "table",
+ "extra_cache_keys",
+ "removed_filters",
+ "applied_filters",
+ }
+)
+
+
class BaseTemplateProcessor:
"""
Base class for database-specific jinja context
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 42dac0524ec..c8783eb7b27 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -653,6 +653,78 @@ def json_to_dict(json_str: str) -> dict[Any, Any]:
return {}
+def parse_template_params(value: str | dict[str, Any] | None) -> dict[str, Any]:
+ """
+ Read a dataset's ``template_params`` the way query execution does.
+
+ Execution merges ``json_to_dict(template_params)`` into the Jinja context,
+ so authorizing what a templated SQL reads must parse with that same
+ function to see the same values. Anything other than a JSON object is
+ rejected rather than coerced: execution would read such a value
+ differently (``dict.update`` also accepts a list of key/value pairs), so
+ no access decision based on it could be trusted.
+
+ :raises ValueError: if ``value`` is neither empty nor a JSON object
+ """
+ if isinstance(value, dict):
+ return value
+ if not value:
+ return {}
+ if not isinstance(value, str):
+ raise ValueError("Template parameters must be a JSON object")
+ params = json_to_dict(value)
+ if not isinstance(params, dict):
+ raise ValueError("Template parameters must be a JSON object")
+ return params
+
+
+def _canonical_template_params(value: str | dict[str, Any] | None) -> str | None:
+ """Canonical JSON for ``template_params``, or ``None`` if unreadable.
+
+ Serializing the parsed value makes the comparison type-sensitive the way
+ Jinja is -- ``1``, ``true`` and ``1.0`` are distinct even though Python
+ considers the dicts equal -- while ``sort_keys`` keeps it insensitive to
+ key order and whitespace.
+ """
+ try:
+ return json.dumps(parse_template_params(value), sort_keys=True)
+ except ValueError:
+ return None
+
+
+def template_params_equivalent(
+ left: str | dict[str, Any] | None,
+ right: str | dict[str, Any] | None,
+) -> bool:
+ """Whether two ``template_params`` render the same values.
+
+ Compared as execution reads them (see ``parse_template_params``), so
+ re-serialized JSON with the same values is equivalent, but values of
+ different JSON types are not. An unreadable value on either side is never
+ equivalent, so callers re-authorize rather than skip (fail closed).
+ """
+ left_canonical = _canonical_template_params(left)
+ return left_canonical is not None and left_canonical == _canonical_template_params(
+ right
+ )
+
+
+def template_params_for_access_check(params: dict[str, Any]) -> dict[str, Any]:
+ """Drop names the template processor consumes structurally.
+
+ A define/repoint access check renders the SQL with the submitted
+ ``template_params``; the names in ``RESERVED_TEMPLATE_PARAM_NAMES`` are
+ taken by the processor as constructor arguments rather than exposed as
+ Jinja variables, so execution never sees them in the render context.
+ Dropping them here keeps the authorized render matching the executed one.
+ """
+ from superset.jinja_context import ( # pylint: disable=import-outside-toplevel
+ RESERVED_TEMPLATE_PARAM_NAMES,
+ )
+
+ return {k: v for k, v in params.items() if k not in RESERVED_TEMPLATE_PARAM_NAMES}
+
+
UUID_NATIVE_TYPE_RE: re.Pattern[str] = re.compile(
r"\b(uuid|uniqueidentifier)\b", re.IGNORECASE
)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index ba2c430803d..e45ba89fe24 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -4985,6 +4985,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
template_params: Optional[dict[str, Any]] = None,
force_dataset_match: bool = False,
allow_query_authorship_bypass: bool = False,
+ allow_editorship: bool = True,
) -> None:
"""
Raise an exception if the user cannot access the resource.
@@ -5002,11 +5003,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
``catalog_access`` / ``schema_access`` fallthroughs in the
database+table / query branch are bypassed and every referenced
table must resolve to a registered Superset dataset the user
- has ``datasource_access`` on (or owns). Call sites that execute
- or return raw row data (SQL Lab, MetaDB) set this to True.
+ has ``datasource_access`` on; editorship/ownership of the
+ dataset is not sufficient here (it is accepted only on the
+ non-strict path). Call sites that execute or return raw row
+ data (SQL Lab, MetaDB, MCP ``execute_sql``, alert/report
+ queries) set this to True.
The default (False) preserves the historical semantics for
- chart-data, dataset CRUD, ``/table_metadata/``, and
- ``/select_star/``.
+ chart-data, ``/table_metadata/``, and ``/select_star/`` (dataset
+ create/update/import additionally opt out of the editorship accept
+ via ``allow_editorship`` below).
:param allow_query_authorship_bypass: When True, a SQL Lab query's
own author is granted access to that exact, already-succeeded
query without an additional dataset-level ``datasource_access``
@@ -5019,6 +5024,22 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
access, and a query's SQL may be templated such that the
tables actually touched depend on parameters supplied at
request time.
+ :param allow_editorship: When False, editorship/ownership of a
+ registered dataset no longer counts as access to the underlying
+ table (in both the database+table / query branch and the
+ datasource / query_context branch; the ``catalog_access`` /
+ ``schema_access`` / ``datasource_access`` checks still apply).
+ The paths that define or repoint a dataset over a table -- dataset
+ create, update, v1 import, and the legacy ``/datasource/save/``
+ repoint -- and query-backed chart-data set this to False so that
+ binding a dataset to (or reading a stored query against) a table
+ requires real data access to that table, not editorship of a
+ same-named dataset -- otherwise a no-table decoy dataset named
+ after the table would let its editor define a second dataset that
+ reads it. Unlike ``force_dataset_match`` this keeps the
+ catalog/schema fallthroughs, so ``schema_access`` still authorizes
+ defining a dataset. Ignored when ``force_dataset_match`` is True
+ (editorship is already excluded there).
:raises SupersetSecurityException: If the user cannot access the resource
"""
# pylint: disable=import-outside-toplevel
@@ -5376,10 +5397,37 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
catalog=effective_catalog,
)
for datasource_ in datasources:
+ # ``datasource_access`` on a registered dataset is always
+ # sufficient. Editorship of the dataset is accepted only on
+ # the non-strict path (chart-data, ``/table_metadata/``,
+ # ``/select_star/``), where it is the dataset-owner
+ # "explore your own dataset" grant from #20382.
+ #
+ # It is excluded on two kinds of call:
+ # * strict scoping (``force_dataset_match``) -- SQL Lab
+ # raw queries, MetaDB, MCP ``execute_sql`` and
+ # alert/report queries -- which return raw rows and
+ # also drop the catalog/schema fallthroughs;
+ # * ``allow_editorship=False`` -- the paths that define
+ # or repoint a dataset over a table (create, update, v1
+ # import, and the legacy ``/datasource/save/`` repoint)
+ # and query-backed chart-data (which runs a stored
+ # query's raw SQL) -- which keep the catalog/schema
+ # fallthroughs but refuse editorship.
+ # Both must follow the user's live data-access grants, not
+ # editorship of a same-named dataset: any principal able to
+ # create a dataset can make themselves an editor of one
+ # named after an arbitrary table, so accepting editorship
+ # here would let dataset creation stand in for data access
+ # and would outlive revocation of every real grant.
if self.can_access(
"datasource_access",
datasource_.perm or "",
- ) or self.is_editor(datasource_):
+ ) or (
+ not force_dataset_match
+ and allow_editorship
+ and self.is_editor(datasource_)
+ ):
# access to any datasource is sufficient
break
else:
@@ -5444,7 +5492,12 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
# A grant on a semantic view's parent layer covers the view,
# matching SemanticView.raise_for_access (sc-119501).
or self._semantic_layer_grant_allows(datasource)
- or self.is_editor(datasource)
+ # ``allow_editorship`` is honored here too so a caller that opts
+ # out of the editorship accept (e.g. dataset define/repoint
+ # validation) gets consistent behavior across both branches,
+ # rather than the flag silently applying only to the
+ # database+table / query branch above.
+ or (allow_editorship and self.is_editor(datasource))
or (
# Grant access to the datasource only if dashboard RBAC is enabled
# or the user is an embedded guest user with access to the dashboard
diff --git a/superset/translations/messages.pot b/superset/translations/messages.pot
index d606fae95f1..5d7efef10fe 100644
--- a/superset/translations/messages.pot
+++ b/superset/translations/messages.pot
@@ -13638,6 +13638,9 @@ msgstr ""
msgid "Template parameters"
msgstr ""
+msgid "Template parameters must be a valid JSON object."
+msgstr ""
+
#, python-format
msgid "Template processing error: %(error)s"
msgstr ""
diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py
index d23d5daea87..2d7a56e22cf 100644
--- a/superset/views/datasource/views.py
+++ b/superset/views/datasource/views.py
@@ -29,6 +29,8 @@ from superset import db, event_logger, security_manager
from superset.commands.dataset.exceptions import (
DatasetForbiddenError,
DatasetNotFoundError,
+ get_template_render_error_msg,
+ TEMPLATE_RENDER_ERRORS,
)
from superset.connectors.sqla.models import SqlaTable
from superset.connectors.sqla.utils import get_physical_table_metadata
@@ -38,6 +40,11 @@ from superset.daos.datasource import Datasource as DatasourceModel, DatasourceDA
from superset.daos.exceptions import DatasourceNotFound, DatasourceTypeNotSupportedError
from superset.exceptions import SupersetException, SupersetSecurityException
from superset.models.core import Database
+from superset.models.helpers import (
+ parse_template_params,
+ template_params_equivalent,
+ template_params_for_access_check,
+)
from superset.sql.parse import Table
from superset.superset_typing import FlaskResponse
from superset.utils import json
@@ -126,6 +133,7 @@ def _repoints_sql(
database_changed: bool,
requested_sql: str | None,
requested_table: Table,
+ raw_template_params: str | None,
) -> bool:
"""
Would the request change what the SQL on ``datasource`` reads?
@@ -133,20 +141,29 @@ def _repoints_sql(
Supplying ``sql`` aims the dataset at whatever that SQL reads, converting
a physical dataset on the way. Unchanged SQL still lands somewhere new
when the connection, catalog or schema it resolves unqualified names
- against moves, which ``UpdateDatasetCommand._validate_sql_access`` gates
- on changed SQL text alone and so does not yet cover.
+ against moves, or when the ``template_params`` that render a templated
+ ``FROM`` change -- the same triggers as
+ ``UpdateDatasetCommand._validate_sql_access``. An identical params value
+ (the editor echoes it back) is never a change, even a legacy non-object
+ one; otherwise they are compared as execution reads them.
"""
if not isinstance(datasource, SqlaTable) or not requested_sql:
return False
+ params_changed = raw_template_params != datasource.template_params and (
+ not template_params_equivalent(raw_template_params, datasource.template_params)
+ )
return (
database_changed
or requested_sql != datasource.sql
or (requested_table.schema, requested_table.catalog)
!= (datasource.schema or None, datasource.catalog or None)
+ or params_changed
)
-def _requested_target(datasource_dict: dict[str, Any]) -> tuple[str | None, Table]:
+def _requested_target(
+ datasource_dict: dict[str, Any],
+) -> tuple[str | None, Table, str | None]:
"""The SQL and table a save aims the dataset at.
``update_from_object`` applies an omitted key as ``None``, so the request
@@ -163,12 +180,16 @@ def _requested_target(datasource_dict: dict[str, Any]) -> tuple[str | None, Tabl
raise ValueError(field)
return value
- return string("sql"), Table(
- # ``Table`` is annotated for a table that exists, but an omitted
- # ``table_name`` lands as ``None`` and has to be compared as one.
- cast(str, string("table_name")),
- string("schema") or None,
- string("catalog") or None,
+ return (
+ string("sql"),
+ Table(
+ # ``Table`` is annotated for a table that exists, but an omitted
+ # ``table_name`` lands as ``None`` and has to be compared as one.
+ cast(str, string("table_name")),
+ string("schema") or None,
+ string("catalog") or None,
+ ),
+ string("template_params"),
)
@@ -178,6 +199,7 @@ def _authorize_repoint(
database_changed: bool,
requested_sql: str | None,
requested_table: Table,
+ raw_template_params: str | None,
) -> FlaskResponse | None:
"""Authorise the target a save aims ``datasource`` at.
@@ -194,7 +216,11 @@ def _authorize_repoint(
datasource, database_changed, requested_sql, requested_table
)
repoints_sql = _repoints_sql(
- datasource, database_changed, requested_sql, requested_table
+ datasource,
+ database_changed,
+ requested_sql,
+ requested_table,
+ raw_template_params,
)
if not (repoints_table or repoints_sql):
return None
@@ -211,16 +237,37 @@ def _authorize_repoint(
security_manager.raise_for_access(
database=target_database,
table=requested_table,
+ allow_editorship=False,
)
if repoints_sql:
+ # Only rendered on an actual repoint, so an unreadable value here
+ # is a genuine change we cannot authorize -- reject it rather than
+ # 500 (a metadata-only echo of a legacy value never reaches this).
+ try:
+ template_params = parse_template_params(raw_template_params)
+ except ValueError:
+ return json_error_response(
+ _("Template parameters must be a valid JSON object."),
+ status=422,
+ )
security_manager.raise_for_access(
database=target_database,
sql=requested_sql,
catalog=requested_table.catalog,
schema=requested_table.schema,
+ # Render with the submitted params so a templated FROM is
+ # authorized as the table it actually resolves to, minus the
+ # names the processor consumes structurally (so the render
+ # matches execution).
+ template_params=template_params_for_access_check(template_params),
+ allow_editorship=False,
)
except SupersetSecurityException as ex:
raise DatasetForbiddenError() from ex
+ except TEMPLATE_RENDER_ERRORS as ex:
+ # SQL that can't be rendered with the submitted params can't be
+ # authorized: refuse it as invalid input rather than a 500.
+ return json_error_response(get_template_render_error_msg(ex), status=422)
return None
@@ -260,7 +307,9 @@ class Datasource(BaseSupersetView):
database_changed = database_id != orm_datasource.database_id
try:
- requested_sql, requested_table = _requested_target(datasource_dict)
+ requested_sql, requested_table, raw_template_params = _requested_target(
+ datasource_dict
+ )
except ValueError as ex:
return json_error_response(
_("The `%(field)s` field must be a string.", field=str(ex)),
@@ -273,6 +322,7 @@ class Datasource(BaseSupersetView):
database_changed,
requested_sql,
requested_table,
+ raw_template_params,
):
return error
diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py
index b2f5fb935b5..174735c9d67 100644
--- a/tests/integration_tests/security_tests.py
+++ b/tests/integration_tests/security_tests.py
@@ -1917,7 +1917,7 @@ class TestSecurityManager(SupersetTestCase):
):
"""
Regression test: with force_dataset_match=True, schema_access alone
- is not sufficient. The user must have datasource_access (or edit) a
+ is not sufficient. The user must have datasource_access on a
registered Superset dataset for the referenced table.
"""
query = Mock(
@@ -1936,6 +1936,117 @@ class TestSecurityManager(SupersetTestCase):
with self.assertRaises(SupersetSecurityException): # noqa: PT027
security_manager.raise_for_access(query=query, force_dataset_match=True)
+ @patch("superset.connectors.sqla.models.SqlaTable.query_datasources_by_name")
+ @patch("superset.security.SupersetSecurityManager.is_editor")
+ @patch("superset.security.SupersetSecurityManager.can_access")
+ def test_raise_for_access_force_dataset_match_denies_editor_only(
+ self, mock_can_access, mock_is_editor, mock_query_datasources
+ ):
+ """
+ Regression test: with force_dataset_match=True, being an
+ editor of a registered dataset for the referenced table is NOT
+ sufficient. Raw row access on the strict path (SQL Lab, MetaDB, MCP
+ execute_sql) must follow live data grants, not editorship — which any
+ dataset-creating principal can grant themselves and which would
+ otherwise survive revocation of every real data-access grant.
+ """
+ query = Mock(
+ database=get_example_database(),
+ schema="bar",
+ sql="SELECT * FROM foo",
+ catalog=None,
+ )
+ # A dataset exists for the table and the user is its editor, but holds
+ # no datasource_access (nor any catalog/schema/database grant).
+ mock_query_datasources.return_value = [
+ Mock(perm="[examples].[bar].[foo](id:1)")
+ ]
+ mock_can_access.return_value = False
+ mock_is_editor.return_value = True
+
+ with self.assertRaises(SupersetSecurityException): # noqa: PT027
+ security_manager.raise_for_access(query=query, force_dataset_match=True)
+
+ @patch("superset.connectors.sqla.models.SqlaTable.query_datasources_by_name")
+ @patch("superset.security.SupersetSecurityManager.is_editor")
+ @patch("superset.security.SupersetSecurityManager.can_access")
+ def test_raise_for_access_non_strict_allows_editor(
+ self, mock_can_access, mock_is_editor, mock_query_datasources
+ ):
+ """
+ The dataset-owner "explore your own dataset" grant (#20382) is
+ preserved on the non-strict path: without force_dataset_match,
+ editorship of a registered dataset for the referenced table still
+ authorizes access even when the user holds no datasource_access.
+ """
+ query = Mock(
+ database=get_example_database(),
+ schema="bar",
+ sql="SELECT * FROM foo",
+ catalog=None,
+ )
+ mock_query_datasources.return_value = [
+ Mock(perm="[examples].[bar].[foo](id:1)")
+ ]
+ mock_can_access.return_value = False
+ mock_is_editor.return_value = True
+
+ security_manager.raise_for_access(query=query)
+
+ @patch("superset.connectors.sqla.models.SqlaTable.query_datasources_by_name")
+ @patch("superset.security.SupersetSecurityManager.is_editor")
+ @patch("superset.security.SupersetSecurityManager.can_access")
+ def test_raise_for_access_allow_editorship_false_denies_editor(
+ self, mock_can_access, mock_is_editor, mock_query_datasources
+ ):
+ """
+ With allow_editorship=False (dataset create/update SQL and table
+ validation), editorship of a registered dataset for the referenced
+ table is NOT sufficient. This stops a no-table decoy dataset named
+ after the table from letting its editor define a second dataset that
+ reads it, without ever holding data access to the table.
+ """
+ query = Mock(
+ database=get_example_database(),
+ schema="bar",
+ sql="SELECT * FROM foo",
+ catalog=None,
+ )
+ mock_query_datasources.return_value = [
+ Mock(perm="[examples].[bar].[foo](id:1)")
+ ]
+ mock_can_access.return_value = False
+ mock_is_editor.return_value = True
+
+ with self.assertRaises(SupersetSecurityException): # noqa: PT027
+ security_manager.raise_for_access(query=query, allow_editorship=False)
+
+ @patch("superset.connectors.sqla.models.SqlaTable.query_datasources_by_name")
+ @patch("superset.security.SupersetSecurityManager.is_editor")
+ @patch("superset.security.SupersetSecurityManager.can_access")
+ def test_raise_for_access_allow_editorship_false_keeps_schema_access(
+ self, mock_can_access, mock_is_editor, mock_query_datasources
+ ):
+ """
+ allow_editorship=False still keeps the catalog/schema fallthroughs:
+ a user with schema_access can define a dataset over the table even
+ though they are not an editor and hold no datasource_access. This is
+ what distinguishes it from force_dataset_match=True (which drops the
+ schema fallthrough).
+ """
+ query = Mock(
+ database=get_example_database(),
+ schema="bar",
+ sql="SELECT * FROM foo",
+ catalog=None,
+ )
+ # No registered dataset, not an editor; only schema_access holds.
+ mock_query_datasources.return_value = []
+ mock_can_access.side_effect = lambda perm, _vm: perm == "schema_access"
+ mock_is_editor.return_value = False
+
+ security_manager.raise_for_access(query=query, allow_editorship=False)
+
@patch("superset.connectors.sqla.models.SqlaTable.query_datasources_by_name")
@patch("superset.security.SupersetSecurityManager.is_editor")
@patch("superset.security.SupersetSecurityManager.can_access")
diff --git a/tests/unit_tests/commands/dataset/test_create.py b/tests/unit_tests/commands/dataset/test_create.py
index f62953fb3bc..ddc2856cc88 100644
--- a/tests/unit_tests/commands/dataset/test_create.py
+++ b/tests/unit_tests/commands/dataset/test_create.py
@@ -14,6 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from typing import Any
from unittest.mock import Mock, patch
import pytest
@@ -21,7 +22,11 @@ from marshmallow import ValidationError
from pytest_mock import MockerFixture
from superset.commands.dataset.create import CreateDatasetCommand
-from superset.commands.dataset.exceptions import DatasetInvalidError
+from superset.commands.dataset.exceptions import (
+ DatasetInvalidError,
+ DatasetSqlRenderValidationError,
+ TemplateParamsInvalidValidationError,
+)
from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import (
@@ -31,6 +36,7 @@ from superset.exceptions import (
SupersetTimeoutException,
)
from superset.models.core import Database
+from superset.sql.parse import Table
def _validate_virtual_dataset(
@@ -261,6 +267,266 @@ def test_create_dataset_invalid_sql_parse_error() -> None:
)
+def test_create_dataset_passes_allow_editorship_false() -> None:
+ """
+ CreateDatasetCommand must validate the dataset's table binding with
+ allow_editorship=False, so editorship of a same-named dataset cannot stand
+ in for data access when defining a dataset over a table.
+ """
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.backend = "postgresql"
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.get_default_catalog.return_value = None
+
+ with (
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.get_database_by_id",
+ return_value=mock_database,
+ ),
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.validate_table_exists",
+ return_value=True,
+ ),
+ patch(
+ "superset.commands.dataset.create.security_manager.raise_for_access",
+ ) as raise_for_access,
+ patch(
+ "superset.commands.dataset.create.populate_subjects",
+ return_value=[],
+ ),
+ ):
+ CreateDatasetCommand(
+ {"database": 1, "table_name": "t", "schema": "public"}
+ ).validate()
+
+ assert raise_for_access.call_args.kwargs.get("allow_editorship") is False
+
+
+def test_create_dataset_passes_template_params() -> None:
+ """
+ A virtual dataset's SQL access check must render with the submitted
+ ``template_params`` so a templated ``FROM`` is authorized as the table it
+ resolves to, not its ``default()``.
+ """
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.backend = "postgresql"
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.get_default_catalog.return_value = None
+
+ with (
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.get_database_by_id",
+ return_value=mock_database,
+ ),
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch(
+ "superset.commands.dataset.create.security_manager.raise_for_access",
+ ) as raise_for_access,
+ patch(
+ "superset.commands.dataset.create.populate_subjects",
+ return_value=[],
+ ),
+ ):
+ CreateDatasetCommand(
+ {
+ "database": 1,
+ "table_name": "t",
+ "schema": "public",
+ "sql": "SELECT * FROM {{ p }}",
+ "template_params": '{"p": "private.secret"}',
+ }
+ ).validate()
+
+ assert raise_for_access.call_args.kwargs.get("template_params") == {
+ "p": "private.secret"
+ }
+ assert raise_for_access.call_args.kwargs.get("allow_editorship") is False
+
+
+def test_create_dataset_auth_check_drops_reserved_params() -> None:
+ """
+ The SQL access check renders with the submitted params minus the names the
+ template processor consumes structurally, so the authorized render matches
+ execution (which never exposes them as Jinja variables).
+ """
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ dao, uniqueness, table_exists, subjects = _patched_create_validation(mock_database)
+
+ with (
+ dao,
+ uniqueness,
+ table_exists,
+ subjects,
+ patch(
+ "superset.commands.dataset.create.security_manager.raise_for_access",
+ ) as raise_for_access,
+ ):
+ CreateDatasetCommand(
+ {
+ "database": 1,
+ "table_name": "t",
+ "schema": "public",
+ "sql": "SELECT * FROM {{ p }}",
+ "template_params": '{"p": "public.ok", "extra_cache_keys": []}',
+ }
+ ).validate()
+
+ assert raise_for_access.call_args.kwargs.get("template_params") == {
+ "p": "public.ok"
+ }
+
+
+def _patched_create_validation(mock_database: Mock) -> tuple[Any, Any, Any, Any]:
+ """Patch the DAO/subject lookups around ``CreateDatasetCommand.validate``."""
+ return (
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.get_database_by_id",
+ return_value=mock_database,
+ ),
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch(
+ "superset.commands.dataset.create.DatasetDAO.validate_table_exists",
+ return_value=True,
+ ),
+ patch(
+ "superset.commands.dataset.create.populate_subjects",
+ return_value=[],
+ ),
+ )
+
+
+@pytest.mark.parametrize(
+ "template_params",
+ ["{bad", '"t"', '["t"]', '[["t", "private.secret"]]'],
+)
+def test_create_dataset_unreadable_template_params_rejected(
+ template_params: str,
+) -> None:
+ """
+ A virtual dataset whose params aren't a JSON object fails validation on
+ ``template_params`` (a 422) instead of escaping ``validate()`` as a 500,
+ and no access decision is made against params execution would read
+ differently (a list of pairs still reaches Jinja through ``dict.update``).
+ """
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ dao, uniqueness, table_exists, subjects = _patched_create_validation(mock_database)
+
+ with (
+ dao,
+ uniqueness,
+ table_exists,
+ subjects,
+ patch(
+ "superset.commands.dataset.create.security_manager.raise_for_access",
+ ) as raise_for_access,
+ pytest.raises(DatasetInvalidError) as excinfo,
+ ):
+ CreateDatasetCommand(
+ {
+ "database": 1,
+ "table_name": "t",
+ "schema": "public",
+ "sql": "SELECT * FROM {{ t | default('public.ok') }}",
+ "template_params": template_params,
+ }
+ ).validate()
+
+ assert any(
+ isinstance(exc, TemplateParamsInvalidValidationError)
+ for exc in excinfo.value._exceptions
+ )
+ raise_for_access.assert_not_called()
+
+
+def test_create_dataset_render_error_is_a_validation_error() -> None:
+ """
+ SQL that can't be rendered with its params (e.g. ``{{ p + 1 }}`` with
+ ``p = "a"``) can't be authorized: the create fails validation on ``sql``
+ instead of surfacing a 500.
+ """
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ dao, uniqueness, table_exists, subjects = _patched_create_validation(mock_database)
+
+ with (
+ dao,
+ uniqueness,
+ table_exists,
+ subjects,
+ patch(
+ "superset.commands.dataset.create.security_manager.raise_for_access",
+ side_effect=TypeError('can only concatenate str (not "int") to str'),
+ ),
+ pytest.raises(DatasetInvalidError) as excinfo,
+ ):
+ CreateDatasetCommand(
+ {
+ "database": 1,
+ "table_name": "t",
+ "schema": "public",
+ "sql": "SELECT {{ p + 1 }}",
+ "template_params": '{"p": "a"}',
+ }
+ ).validate()
+
+ assert any(
+ isinstance(exc, DatasetSqlRenderValidationError)
+ for exc in excinfo.value._exceptions
+ )
+
+
+def test_create_physical_dataset_template_params_not_parsed() -> None:
+ """
+ A physical dataset's params don't decide which table it reads, so they
+ are left unparsed as before and only the table binding is checked.
+ """
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ dao, uniqueness, table_exists, subjects = _patched_create_validation(mock_database)
+
+ with (
+ dao,
+ uniqueness,
+ table_exists,
+ subjects,
+ patch(
+ "superset.commands.dataset.create.security_manager.raise_for_access",
+ ) as raise_for_access,
+ ):
+ CreateDatasetCommand(
+ {
+ "database": 1,
+ "table_name": "t",
+ "schema": "public",
+ "template_params": "{bad",
+ }
+ ).validate()
+
+ raise_for_access.assert_called_once_with(
+ database=mock_database,
+ table=Table("t", "public", None),
+ allow_editorship=False,
+ )
+
+
def test_create_dataset_valid_sql_with_access_error() -> None:
"""
Test that security exceptions work correctly
diff --git a/tests/unit_tests/commands/dataset/test_duplicate.py b/tests/unit_tests/commands/dataset/test_duplicate.py
index aeb6efc176b..79b800e00c8 100644
--- a/tests/unit_tests/commands/dataset/test_duplicate.py
+++ b/tests/unit_tests/commands/dataset/test_duplicate.py
@@ -20,6 +20,7 @@ Covers both the per-object access check and the virtual dataset
duplication/validation behavior (including catalog handling).
"""
+from typing import Any
from unittest.mock import MagicMock, Mock, patch
import pytest
@@ -31,6 +32,7 @@ from superset.commands.dataset.exceptions import (
DatasetExistsValidationError,
DatasetInvalidError,
DatasetNotFoundError,
+ TemplateParamsInvalidValidationError,
)
from superset.commands.exceptions import DatasourceTypeInvalidError
from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
@@ -80,6 +82,8 @@ def test_duplicate_dataset_access_check_passes_through() -> None:
mock_dataset = MagicMock()
mock_dataset.id = 1
mock_dataset.kind = "virtual"
+ mock_dataset.sql = "SELECT * FROM t;"
+ mock_dataset.template_params = '{"x": 1}'
with patch(
"superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
@@ -105,7 +109,132 @@ def test_duplicate_dataset_access_check_passes_through() -> None:
)
command.validate() # should not raise
# Confirm access check was called with the base dataset
- mock_access.assert_called_once_with(datasource=mock_dataset)
+ mock_access.assert_any_call(datasource=mock_dataset)
+ # ... and the copy's SQL, as ``run`` will store it, was
+ # authorized without the editorship accept.
+ mock_access.assert_any_call(
+ database=mock_dataset.database,
+ sql="SELECT * FROM t",
+ catalog=mock_dataset.catalog,
+ schema=mock_dataset.schema,
+ template_params={"x": 1},
+ allow_editorship=False,
+ )
+ assert mock_access.call_count == 2
+
+
+def test_duplicate_dataset_auth_check_drops_reserved_params() -> None:
+ """
+ The copy's SQL access check renders with the base's params minus the names
+ the template processor consumes structurally, matching execution.
+ """
+ mock_dataset = MagicMock()
+ mock_dataset.id = 1
+ mock_dataset.kind = "virtual"
+ mock_dataset.sql = "SELECT * FROM {{ t }}"
+ mock_dataset.template_params = '{"t": "a", "extra_cache_keys": [], "query": 1}'
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_dataset,
+ ),
+ patch(
+ "superset.commands.dataset.duplicate.security_manager.raise_for_access"
+ ) as mock_access,
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch("superset.commands.utils.populate_subject_list", return_value=[]),
+ ):
+ DuplicateDatasetCommand(
+ {"base_model_id": 1, "table_name": "copy", "is_managed_externally": False}
+ ).validate()
+
+ sql_call = next(c for c in mock_access.call_args_list if c.kwargs.get("sql"))
+ assert sql_call.kwargs["template_params"] == {"t": "a"}
+
+
+def test_duplicate_dataset_copy_requires_data_access() -> None:
+ """
+ Editorship of the base dataset (which the ``datasource=`` check accepts)
+ is not enough to duplicate it: the copy is a new dataset the caller owns,
+ so its SQL needs real data access, as on the create path. Otherwise the
+ copy would outlive the caller's access to the base.
+ """
+ mock_dataset = MagicMock()
+ mock_dataset.id = 1
+ mock_dataset.kind = "virtual"
+ mock_dataset.sql = "SELECT * FROM t"
+ mock_dataset.template_params = None
+
+ def deny_sql(**kwargs: Any) -> None:
+ if kwargs.get("sql"):
+ raise _security_exception()
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_dataset,
+ ),
+ patch(
+ "superset.commands.dataset.duplicate.security_manager.raise_for_access",
+ side_effect=deny_sql,
+ ),
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch("superset.commands.utils.populate_subject_list", return_value=[]),
+ ):
+ command = DuplicateDatasetCommand(
+ {"base_model_id": 1, "table_name": "copy", "is_managed_externally": False}
+ )
+ with pytest.raises(DatasetAccessDeniedError):
+ command.validate()
+
+
+@pytest.mark.parametrize("template_params", ["{bad", '[["t", "secret"]]'])
+def test_duplicate_dataset_unreadable_template_params_rejected(
+ template_params: str,
+) -> None:
+ """
+ The copy renders the base's SQL with the base's params, so params that
+ execution would read differently than any check can fail validation
+ instead of being authorized against a guess.
+ """
+ mock_dataset = MagicMock()
+ mock_dataset.id = 1
+ mock_dataset.kind = "virtual"
+ mock_dataset.sql = "SELECT * FROM {{ t | default('ok') }}"
+ mock_dataset.template_params = template_params
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_dataset,
+ ),
+ patch(
+ "superset.commands.dataset.duplicate.security_manager.raise_for_access",
+ ) as mock_access,
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ),
+ patch("superset.commands.utils.populate_subject_list", return_value=[]),
+ ):
+ command = DuplicateDatasetCommand(
+ {"base_model_id": 1, "table_name": "copy", "is_managed_externally": False}
+ )
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ assert any(
+ isinstance(exc, TemplateParamsInvalidValidationError)
+ for exc in exc_info.value._exceptions
+ )
+ mock_access.assert_called_once_with(datasource=mock_dataset)
def test_duplicate_dataset_blocked_by_soft_deleted_twin(session: Session) -> None:
@@ -262,6 +391,7 @@ def test_duplicate_dataset_not_virtual() -> None:
mock_base_model.id = 1
mock_base_model.database_id = 1
mock_base_model.kind = "physical" # Not virtual
+ mock_base_model.sql = None
with (
patch(
@@ -304,6 +434,8 @@ def test_duplicate_dataset_name_exists_same_database_schema() -> None:
mock_base_model.schema = "public"
mock_base_model.catalog = None
mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
with (
patch(
@@ -407,6 +539,8 @@ def test_duplicate_dataset_catalog_passed_to_uniqueness_check() -> None:
mock_base_model.schema = "public"
mock_base_model.catalog = "prod_catalog"
mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
with (
patch(
diff --git a/tests/unit_tests/commands/dataset/update_test.py b/tests/unit_tests/commands/dataset/update_test.py
index 86bd6158107..27a878ac554 100644
--- a/tests/unit_tests/commands/dataset/update_test.py
+++ b/tests/unit_tests/commands/dataset/update_test.py
@@ -28,7 +28,9 @@ from superset.commands.dataset.exceptions import (
DatasetForbiddenError,
DatasetInvalidError,
DatasetNotFoundError,
+ DatasetSqlRenderValidationError,
MultiCatalogDisabledValidationError,
+ TemplateParamsInvalidValidationError,
)
from superset.commands.dataset.update import (
DEFAULT_COLUMNS_FOLDER_UUID,
@@ -94,6 +96,7 @@ def test_update_dataset_sql_authorized_schema(mocker: MockerFixture) -> None:
mock_dataset.catalog = "catalog"
mock_dataset.schema = "public"
mock_dataset.table_name = "test_table"
+ mock_dataset.template_params = None
mock_dataset.editors = [] # No editors to avoid computation issues
mock_dataset_dao.find_by_id.return_value = mock_dataset
@@ -110,7 +113,7 @@ def test_update_dataset_sql_authorized_schema(mocker: MockerFixture) -> None:
mocker.patch("superset.commands.utils.security_manager.is_admin", return_value=True)
# Mock security manager to allow access to the schema
- mocker.patch(
+ raise_for_access = mocker.patch(
"superset.commands.dataset.update.security_manager.raise_for_access",
)
@@ -119,6 +122,16 @@ def test_update_dataset_sql_authorized_schema(mocker: MockerFixture) -> None:
1, {"sql": "SELECT * FROM public.allowed_table"}
).run()
+ # The replacement SQL is authorized as submitted, without the editorship
+ # accept: dropping ``allow_editorship=False`` here fails this assertion.
+ raise_for_access.assert_called_once_with(
+ database=mock_database,
+ sql="SELECT * FROM public.allowed_table",
+ catalog="catalog",
+ schema="public",
+ template_params={},
+ allow_editorship=False,
+ )
# Verify the update was called
assert result == mock_dataset
mock_dataset_dao.update.assert_called_once()
@@ -139,6 +152,7 @@ def test_update_dataset_sql_unauthorized_schema(mocker: MockerFixture) -> None:
mock_dataset.catalog = "catalog"
mock_dataset.schema = "public"
mock_dataset.table_name = "test_table"
+ mock_dataset.template_params = None
mock_dataset.editors = [] # No editors to avoid computation issues
mock_dataset_dao.find_by_id.return_value = mock_dataset
@@ -178,6 +192,58 @@ def test_update_dataset_sql_unauthorized_schema(mocker: MockerFixture) -> None:
)
+def test_update_dataset_template_params_only_change_is_authorized(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A change to ``template_params`` alone (SQL text unchanged) can repoint a
+ templated ``FROM`` to a different table, so it must be re-authorized --
+ with the effective (new) params passed to ``raise_for_access``. Without
+ this the update ran no table/SQL access check at all.
+ """
+ mock_dataset_dao = mocker.patch("superset.commands.dataset.update.DatasetDAO")
+ mock_database = mocker.MagicMock()
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = "catalog"
+ mock_database.allow_multi_catalog = False
+
+ mock_dataset = mocker.MagicMock(is_managed_externally=False)
+ mock_dataset.database = mock_database
+ mock_dataset.catalog = "catalog"
+ mock_dataset.schema = "public"
+ mock_dataset.table_name = "test_table"
+ mock_dataset.sql = "SELECT * FROM {{ t }}"
+ mock_dataset.template_params = '{"t": "public.ok"}'
+ mock_dataset.editors = []
+
+ mock_dataset_dao.find_by_id.return_value = mock_dataset
+ mock_dataset_dao.get_database_by_id.return_value = mock_database
+ mock_dataset_dao.validate_update_uniqueness.return_value = True
+
+ mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_editorship",
+ )
+ mocker.patch("superset.commands.utils.security_manager.is_admin", return_value=True)
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message="You do not have access to: private.secret",
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+
+ with pytest.raises(DatasetInvalidError):
+ UpdateDatasetCommand(1, {"template_params": '{"t": "private.secret"}'}).run()
+
+ # The access check ran (params-only change) and rendered with the new params.
+ sql_call = next(c for c in raise_for_access.call_args_list if c.kwargs.get("sql"))
+ assert sql_call.kwargs["template_params"] == {"t": "private.secret"}
+ assert sql_call.kwargs["allow_editorship"] is False
+
+
def test_update_dataset_database_id_change_checks_new_database_access(
mocker: MockerFixture,
) -> None:
@@ -202,6 +268,8 @@ def test_update_dataset_database_id_change_checks_new_database_access(
mock_dataset.catalog = "catalog"
mock_dataset.schema = "public"
mock_dataset.table_name = "test_table"
+ mock_dataset.sql = None # physical dataset
+ mock_dataset.template_params = None
mock_dataset.editors = [] # No editors to avoid computation issues
mock_dataset_dao.find_by_id.return_value = mock_dataset
@@ -227,8 +295,12 @@ def test_update_dataset_database_id_change_checks_new_database_access(
with pytest.raises(DatasetInvalidError) as excinfo:
UpdateDatasetCommand(1, {"database_id": 2}).run()
- mock_raise_for_access.assert_called_once()
- assert mock_raise_for_access.call_args.kwargs["database"] is mock_new_database
+ # Authorized against the NEW connection, without the editorship accept.
+ mock_raise_for_access.assert_called_once_with(
+ database=mock_new_database,
+ table=Table("test_table", "public", "catalog"),
+ allow_editorship=False,
+ )
assert any(
"You don't have access to that database" in str(exc)
for exc in excinfo.value._exceptions
@@ -258,6 +330,8 @@ def test_update_dataset_database_id_change_allowed_with_access(
mock_dataset.catalog = "catalog"
mock_dataset.schema = "public"
mock_dataset.table_name = "test_table"
+ mock_dataset.sql = None # physical dataset
+ mock_dataset.template_params = None
mock_dataset.editors = [] # No editors to avoid computation issues
mock_dataset_dao.find_by_id.return_value = mock_dataset
@@ -275,8 +349,11 @@ def test_update_dataset_database_id_change_allowed_with_access(
result = UpdateDatasetCommand(1, {"database_id": 2}).run()
- mock_raise_for_access.assert_called_once()
- assert mock_raise_for_access.call_args.kwargs["database"] is mock_new_database
+ mock_raise_for_access.assert_called_once_with(
+ database=mock_new_database,
+ table=Table("test_table", "public", "catalog"),
+ allow_editorship=False,
+ )
assert result == mock_dataset
_, update_kwargs = mock_dataset_dao.update.call_args
assert update_kwargs["attributes"]["database"] is mock_new_database
@@ -329,6 +406,7 @@ def test_update_dataset_physical_repoint_requires_table_access(
raise_for_access.assert_called_once_with(
database=mock_database,
table=Table("restricted_table", "public", "catalog"),
+ allow_editorship=False,
)
assert any(
"You don't have access to the table" in str(exc)
@@ -336,6 +414,393 @@ def test_update_dataset_physical_repoint_requires_table_access(
)
+def _mock_virtual_dataset_update(
+ mocker: MockerFixture,
+ allow_multi_catalog: bool = False,
+ template_params: str | None = None,
+) -> tuple[MagicMock, MagicMock, MagicMock, MagicMock]:
+ """
+ Mocks for updating a virtual dataset ``SELECT * FROM t`` stored in
+ ``catalog.public`` on database 1. Returns the DAO, database 1, database 2
+ (what ``database_id: 2`` resolves to) and the dataset.
+ """
+ mock_dataset_dao = mocker.patch("superset.commands.dataset.update.DatasetDAO")
+ databases = []
+ for database_id in (1, 2):
+ database = mocker.MagicMock()
+ database.id = database_id
+ database.get_default_catalog.return_value = "catalog"
+ database.allow_multi_catalog = allow_multi_catalog
+ databases.append(database)
+ current_database, other_database = databases
+
+ dataset = mocker.MagicMock(is_managed_externally=False)
+ dataset.database = current_database
+ dataset.catalog = "catalog"
+ dataset.schema = "public"
+ dataset.table_name = "report"
+ dataset.sql = "SELECT * FROM t"
+ dataset.template_params = template_params
+ dataset.editors = []
+
+ mock_dataset_dao.find_by_id.return_value = dataset
+ mock_dataset_dao.get_database_by_id.return_value = other_database
+ mock_dataset_dao.validate_update_uniqueness.return_value = True
+ mock_dataset_dao.update.return_value = dataset
+
+ mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_editorship",
+ )
+ mocker.patch("superset.commands.utils.security_manager.is_admin", return_value=True)
+ return mock_dataset_dao, current_database, other_database, dataset
+
+
+def _deny_sql_access(**kwargs: Any) -> None:
+ """``raise_for_access`` stand-in that only denies the SQL check."""
+ if kwargs.get("sql"):
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message="You do not have access to: secret.t",
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
+def test_update_dataset_template_params_type_change_is_rechecked(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A params-only edit that changes a value's JSON type (``1`` -> ``true``)
+ can repoint a templated ``FROM`` -- Jinja distinguishes them -- so it must
+ re-authorize the SQL, even though the parsed dicts are Python-equal.
+ """
+ mock_dataset_dao, *_ = _mock_virtual_dataset_update(
+ mocker, template_params='{"x": 1}'
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=_deny_sql_access,
+ )
+
+ with pytest.raises(DatasetInvalidError):
+ UpdateDatasetCommand(1, {"template_params": '{"x": true}'}).run()
+
+ assert any(c.kwargs.get("sql") for c in raise_for_access.call_args_list)
+ mock_dataset_dao.update.assert_not_called()
+
+
+@pytest.mark.parametrize("legacy_params", ['[["t", "v"]]', "{bad", " ", "null", "42"])
+def test_update_dataset_metadata_edit_keeps_legacy_template_params(
+ mocker: MockerFixture, legacy_params: str
+) -> None:
+ """
+ A metadata-only edit must not be rejected just because the dataset's stored
+ ``template_params`` predate object validation. The editor echoes the stored
+ value back on every save, so an unchanged (even non-object) value must not
+ count as a repoint and must not trigger the SQL access check.
+ """
+ mock_dataset_dao, *_ = _mock_virtual_dataset_update(
+ mocker, template_params=legacy_params
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=_deny_sql_access,
+ )
+
+ UpdateDatasetCommand(
+ 1, {"template_params": legacy_params, "description": "notes"}
+ ).run()
+
+ raise_for_access.assert_not_called()
+ mock_dataset_dao.update.assert_called_once()
+
+
+def test_update_dataset_auth_check_drops_reserved_params(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The SQL access check renders with the submitted params minus the names the
+ template processor consumes structurally, so the authorized render matches
+ execution (which never exposes them as Jinja variables).
+ """
+ mock_dataset_dao, _, _, dataset = _mock_virtual_dataset_update(
+ mocker, template_params='{"x": 1}'
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ )
+
+ UpdateDatasetCommand(
+ 1, {"template_params": '{"x": 2, "extra_cache_keys": [], "database": "z"}'}
+ ).run()
+
+ sql_call = next(c for c in raise_for_access.call_args_list if c.kwargs.get("sql"))
+ assert sql_call.kwargs["template_params"] == {"x": 2}
+
+
+@pytest.mark.parametrize(
+ "payload, database_id, catalog, schema",
+ [
+ ({"schema": "secret"}, 1, "catalog", "secret"),
+ ({"catalog": "other"}, 1, "other", "public"),
+ ({"database_id": 2}, 2, "catalog", "public"),
+ ],
+)
+def test_update_virtual_dataset_location_change_rechecks_sql(
+ mocker: MockerFixture,
+ payload: dict[str, Any],
+ database_id: int,
+ catalog: str,
+ schema: str,
+) -> None:
+ """
+ Unqualified names in a virtual dataset's SQL resolve against its
+ connection, catalog and schema, so moving any of them -- with the SQL text
+ untouched -- re-authorizes the stored SQL against the new location, and
+ a denial stops the update.
+ """
+ mock_dataset_dao, *databases, _ = _mock_virtual_dataset_update(
+ mocker, allow_multi_catalog=True
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=_deny_sql_access,
+ )
+
+ with pytest.raises(DatasetInvalidError):
+ UpdateDatasetCommand(1, payload).run()
+
+ sql_calls = [c for c in raise_for_access.call_args_list if c.kwargs.get("sql")]
+ assert len(sql_calls) == 1
+ assert sql_calls[0].kwargs == {
+ "database": databases[database_id - 1],
+ "sql": "SELECT * FROM t",
+ "catalog": catalog,
+ "schema": schema,
+ "template_params": {},
+ "allow_editorship": False,
+ }
+ mock_dataset_dao.update.assert_not_called()
+
+
+def test_update_dataset_reserialized_template_params_is_not_a_change(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Params are compared as execution reads them, so resubmitting the same
+ values serialized differently doesn't re-check an editor's save.
+ """
+ mock_dataset_dao, *_ = _mock_virtual_dataset_update(
+ mocker, template_params='{"t": "public.ok"}'
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=_deny_sql_access,
+ )
+
+ UpdateDatasetCommand(1, {"template_params": '{ "t":"public.ok" }'}).run()
+
+ raise_for_access.assert_not_called()
+ mock_dataset_dao.update.assert_called_once()
+
+
+def test_update_dataset_different_unreadable_params_are_a_change(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Two unreadable values can't be compared as execution reads them, so
+ replacing one with another counts as a change and fails closed, rather
+ than slipping through as "both unreadable, so equal".
+ """
+ mock_dataset_dao, *_ = _mock_virtual_dataset_update(
+ mocker, template_params='[["t", "public.ok"]]'
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ )
+
+ with pytest.raises(DatasetInvalidError) as excinfo:
+ UpdateDatasetCommand(1, {"template_params": '[["t", "secret.t"]]'}).run()
+
+ assert any(
+ isinstance(exc, TemplateParamsInvalidValidationError)
+ for exc in excinfo.value._exceptions
+ )
+ raise_for_access.assert_not_called()
+ mock_dataset_dao.update.assert_not_called()
+
+
+def test_update_dataset_sql_change_renders_with_stored_params(
+ mocker: MockerFixture,
+) -> None:
+ """
+ SQL submitted without ``template_params`` runs with the stored params, so
+ it is authorized as rendered with them.
+ """
+ _mock_virtual_dataset_update(mocker, template_params='{"t": "public.ok"}')
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ )
+
+ UpdateDatasetCommand(1, {"sql": "SELECT * FROM {{ t }}"}).run()
+
+ assert raise_for_access.call_args.kwargs["template_params"] == {"t": "public.ok"}
+
+
+def test_update_dataset_render_error_is_a_validation_error(
+ mocker: MockerFixture,
+) -> None:
+ """
+ SQL that can't be rendered with its params can't be authorized: the
+ update fails validation on ``sql`` instead of surfacing a 500.
+ """
+ mock_dataset_dao, *_ = _mock_virtual_dataset_update(mocker)
+ mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=TypeError('can only concatenate str (not "int") to str'),
+ )
+
+ with pytest.raises(DatasetInvalidError) as excinfo:
+ UpdateDatasetCommand(1, {"template_params": '{"p": "a"}'}).run()
+
+ assert any(
+ isinstance(exc, DatasetSqlRenderValidationError)
+ for exc in excinfo.value._exceptions
+ )
+ mock_dataset_dao.update.assert_not_called()
+
+
+@pytest.mark.parametrize(
+ "payload",
+ [
+ {"table_name": "renamed"},
+ {"description": "notes"},
+ {"schema": "public", "sql": "SELECT * FROM t"},
+ ],
+)
+def test_update_virtual_dataset_metadata_change_skips_access_check(
+ mocker: MockerFixture, payload: dict[str, Any]
+) -> None:
+ """
+ A virtual dataset's ``table_name`` is only a label, and resubmitting the
+ stored SQL/schema changes nothing it reads, so these edits stay open to an
+ editor without access to the underlying tables.
+ """
+ mock_dataset_dao, *_ = _mock_virtual_dataset_update(mocker)
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=_deny_sql_access,
+ )
+
+ UpdateDatasetCommand(1, payload).run()
+
+ raise_for_access.assert_not_called()
+ mock_dataset_dao.update.assert_called_once()
+
+
+@pytest.mark.parametrize("sql", [None, "SELECT * FROM t"])
+@pytest.mark.parametrize(("stored", "sent"), [("", None), (None, "")])
+def test_update_dataset_empty_schema_round_trip_is_not_a_move(
+ mocker: MockerFixture, sql: str | None, stored: str | None, sent: str | None
+) -> None:
+ """
+ ``""`` and ``None`` both mean "no schema", and the editor can send either
+ back for the other (``BaseDatasource.data`` emits ``""`` as ``None``).
+ That is not a move, so a metadata save by an editor without data access
+ isn't re-checked -- physical or virtual.
+ """
+ mock_dataset_dao, _, _, dataset = _mock_virtual_dataset_update(mocker)
+ dataset.sql = sql
+ dataset.schema = stored
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=_deny_sql_access,
+ )
+
+ UpdateDatasetCommand(1, {"schema": sent, "description": "notes"}).run()
+
+ raise_for_access.assert_not_called()
+ mock_dataset_dao.update.assert_called_once()
+
+
+@pytest.mark.parametrize(
+ "template_params",
+ ["{bad", '"t"', '["t"]', '[["t", "secret.t"]]'],
+)
+def test_update_dataset_unreadable_template_params_rejected(
+ mocker: MockerFixture, template_params: str
+) -> None:
+ """
+ Params that aren't a JSON object can't be read the way execution reads
+ them (a list of pairs would still reach the Jinja context through
+ ``dict.update``), so they fail validation instead of surfacing as a 500
+ or being authorized against a guess.
+ """
+ mock_dataset_dao, *_ = _mock_virtual_dataset_update(mocker)
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ )
+
+ with pytest.raises(DatasetInvalidError) as excinfo:
+ UpdateDatasetCommand(1, {"template_params": template_params}).run()
+
+ assert any(
+ isinstance(exc, TemplateParamsInvalidValidationError)
+ for exc in excinfo.value._exceptions
+ )
+ raise_for_access.assert_not_called()
+ mock_dataset_dao.update.assert_not_called()
+
+
+def test_update_dataset_sql_change_rejects_unreadable_stored_params(
+ mocker: MockerFixture,
+) -> None:
+ """
+ SQL submitted without ``template_params`` renders against the stored
+ params, so params stored while the dataset was physical (and never
+ parsed) can't let a templated ``FROM`` skip authorization.
+ """
+ mock_dataset_dao, _, _, dataset = _mock_virtual_dataset_update(
+ mocker, template_params='[["t", "secret.t"]]'
+ )
+ dataset.sql = None # physical: its params were never parsed
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ )
+
+ with pytest.raises(DatasetInvalidError) as excinfo:
+ UpdateDatasetCommand(
+ 1, {"sql": "SELECT * FROM {{ t | default('public.ok') }}"}
+ ).run()
+
+ assert any(
+ isinstance(exc, TemplateParamsInvalidValidationError)
+ for exc in excinfo.value._exceptions
+ )
+ assert not any(c.kwargs.get("sql") for c in raise_for_access.call_args_list)
+ mock_dataset_dao.update.assert_not_called()
+
+
+def test_update_physical_dataset_template_params_not_parsed(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A physical dataset's params don't decide which table it reads, so they
+ are stored as before without being parsed or access-checked.
+ """
+ mock_dataset_dao, _, _, dataset = _mock_virtual_dataset_update(mocker)
+ dataset.sql = None
+ raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ )
+
+ UpdateDatasetCommand(1, {"template_params": "{bad"}).run()
+
+ raise_for_access.assert_not_called()
+ mock_dataset_dao.update.assert_called_once()
+
+
@pytest.mark.parametrize(
("payload, exception, error_msg"),
[
diff --git a/tests/unit_tests/common/test_query_context_processor.py b/tests/unit_tests/common/test_query_context_processor.py
index 16d52f7017a..33a1209b217 100644
--- a/tests/unit_tests/common/test_query_context_processor.py
+++ b/tests/unit_tests/common/test_query_context_processor.py
@@ -2309,6 +2309,40 @@ def test_raise_for_access_wraps_template_error_for_query_datasource():
query.validate.assert_not_called()
+def test_raise_for_access_query_datasource_disallows_editorship():
+ """
+ Query-backed chart-data runs the stored query's raw SQL, so it must
+ authorize with allow_editorship=False (editorship of a same-named dataset
+ must not stand in for data access), and a denial must stop query.validate().
+ """
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+ from superset.utils.core import DatasourceType
+
+ query = MagicMock()
+ query_context = MagicMock()
+ query_context.queries = [query]
+ query_context.datasource.type = DatasourceType.QUERY
+ processor = QueryContextProcessor(query_context)
+
+ denial = SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR,
+ message="no access",
+ level=ErrorLevel.ERROR,
+ )
+ )
+ with patch(
+ "superset.common.query_context_processor.security_manager.raise_for_access",
+ side_effect=denial,
+ ) as raise_for_access:
+ with pytest.raises(SupersetSecurityException):
+ processor.raise_for_access()
+
+ assert raise_for_access.call_args.kwargs.get("allow_editorship") is False
+ query.validate.assert_not_called()
+
+
def test_grouping_sets_fallback_handles_adhoc_and_physical_columns() -> None:
"""
The fallback used on engines without native GROUPING SETS support must
diff --git a/tests/unit_tests/datasets/commands/importers/v1/import_test.py b/tests/unit_tests/datasets/commands/importers/v1/import_test.py
index 8aa16ce09f8..352a16fe292 100644
--- a/tests/unit_tests/datasets/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/datasets/commands/importers/v1/import_test.py
@@ -748,6 +748,580 @@ def test_import_dataset_virtual_invalid_sql_is_rejected_as_bad_payload(
import_dataset(config)
+def test_import_dataset_repoint_authorizes_requested_binding_pre_mutation(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Regression guard (schema-swap): an overwrite that repoints a dataset to a
+ new schema must authorize the REQUESTED binding with allow_editorship=False
+ and do so BEFORE the row is mutated -- otherwise the per-table lookup
+ resolves the dataset itself (whose perm omits the schema) and a
+ datasource_access holder could repoint it to a schema they can't read.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ ds_uuid = str(uuid.uuid4())
+ existing = SqlaTable(
+ table_name="orders",
+ schema="public",
+ catalog=None,
+ database=database,
+ uuid=ds_uuid,
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ seen: dict[str, Any] = {}
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("table") is not None or kwargs.get("sql") is not None:
+ seen["kwargs"] = kwargs
+ seen["stored_schema"] = db.session.get(SqlaTable, existing.id).schema
+
+ mocker.patch.object(security_manager, "raise_for_access", side_effect=record)
+
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": ds_uuid,
+ "table_name": "orders",
+ "schema": "private",
+ "catalog": None,
+ "sql": "",
+ "database_id": database.id,
+ "metrics": [],
+ "columns": [],
+ }
+ )
+ import_dataset(config, overwrite=True)
+
+ assert seen["kwargs"].get("allow_editorship") is False
+ assert seen["kwargs"]["table"].schema == "private" # requested, not stored
+ assert seen["stored_schema"] == "public" # check ran before import_from_dict
+
+
+def test_import_dataset_metadata_only_skips_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Regression guard (over-restriction): a metadata-only re-import (unchanged
+ table binding) must NOT run the table/SQL access check, so an editor
+ without separate data access can still re-import non-binding changes --
+ matching UpdateDatasetCommand, which only revalidates on a source change.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ ds_uuid = str(uuid.uuid4())
+ existing = SqlaTable(
+ table_name="orders",
+ schema="public",
+ catalog=None,
+ database=database,
+ uuid=ds_uuid,
+ description="old",
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ binding_checks: list[dict[str, Any]] = []
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("table") is not None or kwargs.get("sql") is not None:
+ binding_checks.append(kwargs)
+
+ mocker.patch.object(security_manager, "raise_for_access", side_effect=record)
+
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": ds_uuid,
+ "table_name": "orders",
+ "schema": "public",
+ "catalog": None,
+ "sql": "",
+ "database_id": database.id,
+ "description": "new description",
+ "metrics": [],
+ "columns": [],
+ }
+ )
+ import_dataset(config, overwrite=True)
+
+ assert binding_checks == [] # unchanged binding -> no table/SQL access check
+
+
+def test_import_dataset_database_repoint_runs_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Regression guard: an overwrite that keeps table/schema/catalog/sql but
+ points at a DIFFERENT database still repoints the dataset, so the binding
+ check must run against the new database with allow_editorship=False -- the
+ database is part of the binding (matching UpdateDatasetCommand's repoint).
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ db_a = Database(database_name="db_a", sqlalchemy_uri="sqlite://")
+ db_b = Database(database_name="db_b", sqlalchemy_uri="sqlite://")
+ db.session.add_all([db_a, db_b])
+ db.session.flush()
+ ds_uuid = str(uuid.uuid4())
+ existing = SqlaTable(
+ table_name="orders",
+ schema="public",
+ catalog=None,
+ database=db_a,
+ uuid=ds_uuid,
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ # The access check runs before import_from_dict; stub the mutation so the
+ # test isolates whether the binding check ran (a real DB-identity change
+ # drives import_from_dict down an insert path that is orthogonal here).
+ mocker.patch.object(SqlaTable, "import_from_dict", return_value=existing)
+ binding_checks: list[dict[str, Any]] = []
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("table") is not None or kwargs.get("sql") is not None:
+ binding_checks.append(kwargs)
+
+ mocker.patch.object(security_manager, "raise_for_access", side_effect=record)
+
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": ds_uuid,
+ "table_name": "orders",
+ "schema": "public",
+ "catalog": None,
+ "sql": "",
+ "database_id": db_b.id, # repoint to a different database
+ "metrics": [],
+ "columns": [],
+ }
+ )
+ import_dataset(config, overwrite=True)
+
+ assert len(binding_checks) == 1
+ # Authorized against the REQUESTED connection, not the stored one.
+ assert binding_checks[0]["database"] is db_b
+ assert binding_checks[0]["table"].table == "orders"
+ assert binding_checks[0].get("allow_editorship") is False
+
+
+def test_import_dataset_template_params_only_change_runs_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Regression guard (templated repoint): an overwrite that keeps the SQL text
+ but changes only ``template_params`` still repoints where a Jinja ``FROM``
+ resolves, so the binding check must run against the NEW params with
+ allow_editorship=False -- the stored params are not what the overwrite
+ executes.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ ds_uuid = str(uuid.uuid4())
+ existing = SqlaTable(
+ table_name="orders",
+ schema="public",
+ catalog=None,
+ database=database,
+ sql="SELECT * FROM {{ t }}",
+ template_params=json.dumps({"t": "public.ok"}),
+ uuid=ds_uuid,
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ binding_checks: list[dict[str, Any]] = []
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("sql") is not None:
+ binding_checks.append(kwargs)
+
+ mocker.patch.object(security_manager, "raise_for_access", side_effect=record)
+
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": ds_uuid,
+ "table_name": "orders",
+ "schema": "public",
+ "catalog": None,
+ "sql": "SELECT * FROM {{ t }}", # unchanged SQL text
+ "database_id": database.id,
+ "template_params": {"t": "private.secret"}, # repoints the FROM
+ "metrics": [],
+ "columns": [],
+ }
+ )
+ import_dataset(config, overwrite=True)
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["template_params"] == {"t": "private.secret"}
+ assert binding_checks[0].get("allow_editorship") is False
+
+
+def _existing_dataset(**overrides: Any) -> tuple[SqlaTable, Database]:
+ """
+ Persist a dataset to overwrite-import -- by default a virtual one reading
+ ``SELECT * FROM orders`` in ``private``.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ existing = SqlaTable(
+ **{
+ "table_name": "orders_report",
+ "schema": "private",
+ "catalog": None,
+ "database": database,
+ "sql": "SELECT * FROM orders",
+ "uuid": str(uuid.uuid4()),
+ **overrides,
+ }
+ )
+ db.session.add(existing)
+ db.session.flush()
+ return existing, database
+
+
+def _overwrite_config(existing: SqlaTable, **overrides: Any) -> dict[str, Any]:
+ """An overwrite config for ``existing`` repeating its stored binding."""
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": str(existing.uuid),
+ "table_name": existing.table_name,
+ "schema": existing.schema,
+ "catalog": existing.catalog,
+ "sql": existing.sql,
+ "database_id": existing.database_id,
+ "template_params": None,
+ "metrics": [],
+ "columns": [],
+ **overrides,
+ }
+ )
+ return config
+
+
+def _record_binding_checks(mocker: MockerFixture) -> list[dict[str, Any]]:
+ """Allow every access check, recording the table/SQL binding ones."""
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ binding_checks: list[dict[str, Any]] = []
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("table") is not None or kwargs.get("sql") is not None:
+ binding_checks.append(kwargs)
+
+ mocker.patch.object(security_manager, "raise_for_access", side_effect=record)
+ return binding_checks
+
+
+def test_import_dataset_template_params_type_change_runs_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ An overwrite that changes only a param's JSON type (``1`` -> ``true``)
+ still repoints a templated ``FROM``, so it re-runs the binding check
+ rather than being judged unchanged by Python dict equality.
+ """
+ existing, _ = _existing_dataset(
+ sql="SELECT * FROM {{ t }}", template_params=json.dumps({"t": 1})
+ )
+ binding_checks = _record_binding_checks(mocker)
+
+ import_dataset(_overwrite_config(existing, template_params={"t": True}), True)
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["template_params"] == {"t": True}
+
+
+def test_import_dataset_reimport_keeps_legacy_nonobject_params(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Re-importing a bundle whose dataset carries a legacy non-object (but
+ executable) ``template_params`` must be treated as an unchanged binding,
+ not rejected -- the stored value is unchanged, so this is a metadata-only
+ re-import, matching UpdateDatasetCommand's echo handling.
+ """
+ existing, _ = _existing_dataset(
+ sql="SELECT * FROM {{ t }}",
+ template_params=json.dumps([["t", "public.ok"]]),
+ )
+ binding_checks = _record_binding_checks(mocker)
+
+ # The export carries the params as their parsed (list) form.
+ import_dataset(
+ _overwrite_config(existing, template_params=[["t", "public.ok"]]),
+ overwrite=True,
+ )
+
+ assert binding_checks == []
+
+
+def test_import_dataset_binding_check_drops_reserved_params(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ The import binding check renders with the submitted params minus the names
+ the processor consumes structurally, matching execution.
+ """
+ existing, _ = _existing_dataset(sql="SELECT * FROM {{ t }}")
+ binding_checks = _record_binding_checks(mocker)
+
+ import_dataset(
+ _overwrite_config(
+ existing, template_params={"t": "public.ok", "extra_cache_keys": []}
+ ),
+ True,
+ )
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["template_params"] == {"t": "public.ok"}
+
+
+def test_import_dataset_virtual_rename_skips_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ A virtual dataset's ``table_name`` is only a label, so an overwrite that
+ renames it (same SQL, connection, catalog, schema and params) is not a
+ repoint -- as with PUT and ``/datasource/save/`` -- and stays open to an
+ editor without a grant on the underlying tables.
+ """
+ existing, _ = _existing_dataset()
+ binding_checks = _record_binding_checks(mocker)
+
+ import_dataset(
+ _overwrite_config(existing, table_name="orders_report_renamed"),
+ overwrite=True,
+ )
+
+ assert binding_checks == []
+
+
+def test_import_dataset_omitted_sql_authorizes_stored_sql_at_requested_schema(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ An overwrite only applies the keys it carries, so omitting ``sql`` keeps
+ the stored SQL. Moving the schema must then authorize that stored SQL as
+ it will resolve under the requested schema -- not take the physical
+ branch and check ``<schema>.<table_name>``.
+ """
+ existing, database = _existing_dataset()
+ binding_checks = _record_binding_checks(mocker)
+ config = _overwrite_config(existing, schema="public")
+ del config["sql"]
+
+ import_dataset(config, overwrite=True)
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["database"] is database
+ assert binding_checks[0]["sql"] == "SELECT * FROM orders"
+ assert binding_checks[0]["schema"] == "public"
+ assert "table" not in binding_checks[0]
+ assert binding_checks[0].get("allow_editorship") is False
+
+
+def test_import_dataset_unreadable_stored_params_fail_closed(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Params stored on a physical dataset are never parsed, and execution would
+ still apply a list of pairs (via ``dict.update``). Converting it to a
+ templated virtual dataset while omitting ``template_params`` keeps those
+ params, so the import refuses to authorize the SQL against a guess.
+ """
+ existing, _ = _existing_dataset(
+ sql=None,
+ table_name="orders",
+ template_params='[["t", "private.secret"]]',
+ )
+ binding_checks = _record_binding_checks(mocker)
+ config = _overwrite_config(existing, sql="SELECT * FROM {{ t }}")
+ del config["template_params"]
+
+ with pytest.raises(IncorrectFormatError, match="template_params"):
+ import_dataset(config, overwrite=True)
+
+ assert binding_checks == []
+
+
+@pytest.mark.parametrize(
+ "template_params", ["{bad", '"t"', '["t"]', '[["t", "private.secret"]]']
+)
+def test_import_dataset_unreadable_config_params_fail_closed(
+ mocker: MockerFixture, session: Session, template_params: str
+) -> None:
+ """
+ Params that aren't a JSON object can't be read the way execution reads
+ them, so a virtual import carrying them is refused as a bad payload rather
+ than authorized against ``{}``.
+ """
+ existing, _ = _existing_dataset()
+ binding_checks = _record_binding_checks(mocker)
+
+ with pytest.raises(IncorrectFormatError, match="template_params"):
+ import_dataset(
+ _overwrite_config(existing, template_params=template_params),
+ overwrite=True,
+ )
+
+ assert binding_checks == []
+
+
+def test_import_dataset_params_checked_as_execution_reads_them(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ The import stores a config's params as ``json.dumps(params)`` and
+ execution reads them back with ``json_to_dict``, which rewrites ``", }"``
+ even inside a string value. The check must see what execution will see,
+ or it authorizes a branch of the template that never runs.
+ """
+ existing, _ = _existing_dataset()
+ binding_checks = _record_binding_checks(mocker)
+
+ import_dataset(
+ _overwrite_config(
+ existing,
+ sql="SELECT * FROM {{ 'secret_t' if p == 'x}' else 'ok_t' }}",
+ template_params={"p": "x, }"},
+ ),
+ overwrite=True,
+ )
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["template_params"] == {"p": "x}"}
+
+
+@pytest.mark.parametrize(
+ "overrides, expected",
+ [
+ # SQL replaced: the new SQL is authorized at the stored location.
+ ({"sql": "SELECT * FROM salaries"}, {"sql": "SELECT * FROM salaries"}),
+ # Catalog moved: unqualified names resolve elsewhere.
+ ({"catalog": "other"}, {"sql": "SELECT * FROM orders", "catalog": "other"}),
+ ],
+)
+def test_import_dataset_virtual_binding_change_runs_check(
+ mocker: MockerFixture,
+ session: Session,
+ overrides: dict[str, Any],
+ expected: dict[str, Any],
+) -> None:
+ """
+ Each part of a virtual dataset's binding other than its label is
+ re-authorized when an overwrite changes it, at the requested values.
+ """
+ existing, database = _existing_dataset()
+ binding_checks = _record_binding_checks(mocker)
+
+ import_dataset(_overwrite_config(existing, **overrides), overwrite=True)
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["database"] is database
+ assert binding_checks[0].get("allow_editorship") is False
+ for key, value in expected.items():
+ assert binding_checks[0][key] == value
+
+
+def test_import_dataset_physical_rename_runs_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ A physical dataset's ``table_name`` *is* its binding, so an overwrite that
+ only renames it points the dataset at another table and is authorized
+ against that table.
+ """
+ existing, _ = _existing_dataset(sql=None, table_name="orders", schema="public")
+ binding_checks = _record_binding_checks(mocker)
+
+ import_dataset(_overwrite_config(existing, table_name="salaries"), overwrite=True)
+
+ assert len(binding_checks) == 1
+ table = binding_checks[0]["table"]
+ assert (table.table, table.schema, table.catalog) == ("salaries", "public", None)
+ assert binding_checks[0].get("allow_editorship") is False
+
+
+def test_import_dataset_omitted_fields_keep_stored_values(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ An overwrite only applies the keys it carries: replacing the SQL while
+ omitting ``schema`` and ``template_params`` keeps the stored ones, so the
+ new SQL is authorized as it will run -- at the stored schema, rendered
+ with the stored params.
+ """
+ existing, _ = _existing_dataset(
+ sql="SELECT * FROM {{ t }}",
+ template_params=json.dumps({"t": "public.ok"}),
+ )
+ binding_checks = _record_binding_checks(mocker)
+ config = _overwrite_config(existing, sql="SELECT * FROM {{ t }} LIMIT 1")
+ del config["schema"]
+ del config["template_params"]
+
+ import_dataset(config, overwrite=True)
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["schema"] == "private"
+ assert binding_checks[0]["template_params"] == {"t": "public.ok"}
+
+
+def test_import_dataset_render_error_is_bad_payload(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ SQL that can't be rendered with its params (e.g. ``{{ p + 1 }}`` with
+ ``p = "a"``) can't be authorized: the import fails as a bad payload
+ rather than surfacing a 500.
+ """
+ existing, _ = _existing_dataset()
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+
+ def render_fails(**kwargs: Any) -> None:
+ if kwargs.get("sql"):
+ raise TypeError('can only concatenate str (not "int") to str')
+
+ mocker.patch.object(security_manager, "raise_for_access", side_effect=render_fails)
+
+ with pytest.raises(IncorrectFormatError, match="Invalid SQL"):
+ import_dataset(
+ _overwrite_config(
+ existing, sql="SELECT {{ p + 1 }}", template_params={"p": "a"}
+ ),
+ overwrite=True,
+ )
+
+
def test_import_dataset_schema_rejects_duplicate_metric_uuids() -> None:
"""
Two metrics sharing a UUID must be rejected by the import schema.
@@ -1759,21 +2333,27 @@ def test_import_dataset_access_check(
"""
Test that import_dataset raises DatasetAccessDeniedError when the user does not
have datasource-level access to the target dataset.
+
+ Only the dataset-level (``datasource=``) gate denies: the table binding
+ check that runs first is allowed, so this fails if that gate is removed
+ instead of passing on the earlier check.
"""
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import SupersetSecurityException
- mocker.patch.object(security_manager, "can_access", return_value=True)
- mocker.patch.object(
- security_manager,
- "raise_for_access",
- side_effect=SupersetSecurityException(
- SupersetError(
- error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
- message="User does not have access to this datasource",
- level=ErrorLevel.ERROR,
+ def deny_dataset_access(**kwargs: Any) -> None:
+ if kwargs.get("datasource") is not None:
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message="User does not have access to this datasource",
+ level=ErrorLevel.ERROR,
+ )
)
- ),
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ raise_for_access = mocker.patch.object(
+ security_manager, "raise_for_access", side_effect=deny_dataset_access
)
engine = db.session.get_bind()
@@ -1789,6 +2369,14 @@ def test_import_dataset_access_check(
with pytest.raises(DatasetAccessDeniedError):
import_dataset(config)
+ gates = [
+ call.kwargs["datasource"]
+ for call in raise_for_access.call_args_list
+ if call.kwargs.get("datasource") is not None
+ ]
+ assert len(gates) == 1
+ assert gates[0].uuid == uuid.UUID(config["uuid"])
+
def test_import_soft_deleted_dataset_overwrite_restores_in_place(
mocker: MockerFixture,
diff --git a/tests/unit_tests/models/template_params_test.py b/tests/unit_tests/models/template_params_test.py
new file mode 100644
index 00000000000..35cce96d814
--- /dev/null
+++ b/tests/unit_tests/models/template_params_test.py
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for the ``template_params`` helpers in ``superset.models.helpers``."""
+
+from __future__ import annotations
+
+from typing import Any
+
+from superset.jinja_context import RESERVED_TEMPLATE_PARAM_NAMES
+from superset.models.helpers import (
+ template_params_equivalent,
+ template_params_for_access_check,
+)
+
+
+def test_template_params_equivalent_is_type_sensitive() -> None:
+ """
+ Compared the way execution reads them: JSON scalar types that Jinja
+ distinguishes (``1`` vs ``true`` vs ``1.0``) are not equivalent even
+ though the parsed dicts are Python-equal, while key order and whitespace
+ do not matter.
+ """
+ assert template_params_equivalent('{"x": 1}', '{"x": true}') is False
+ assert template_params_equivalent('{"x": 1}', '{"x": 1.0}') is False
+ assert template_params_equivalent('{"x": 0}', '{"x": false}') is False
+ # Same values, reserialized / reordered -> equivalent.
+ assert template_params_equivalent('{"a": 1, "b": 2}', '{"b": 2, "a": 1}') is True
+ assert template_params_equivalent('{ "x": 1 }', '{"x": 1}') is True
+ assert template_params_equivalent({"x": 1}, '{"x": 1}') is True
+ # Empty / absent are equivalent; unreadable is never equivalent (fail closed).
+ assert template_params_equivalent(None, "") is True
+ assert template_params_equivalent("{bad", "{bad") is False
+ assert template_params_equivalent('{"x": 1}', "{bad") is False
+
+
+def test_template_params_for_access_check_drops_reserved_names() -> None:
+ """
+ Names the template processor consumes structurally are never exposed as
+ Jinja variables at execution, so they are dropped before a define/repoint
+ access check renders the SQL -- keeping the authorized render aligned with
+ execution. User params are kept.
+ """
+ params: dict[str, Any] = dict.fromkeys(RESERVED_TEMPLATE_PARAM_NAMES, "x")
+ params["user_var"] = 1
+ assert template_params_for_access_check(params) == {"user_var": 1}
+ # The reserved set matches BaseTemplateProcessor's structural arguments.
+ assert {
+ "database",
+ "query",
+ "table",
+ "extra_cache_keys",
+ "removed_filters",
+ "applied_filters",
+ } <= RESERVED_TEMPLATE_PARAM_NAMES
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index 217499e1c4a..1aaf582b66b 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -4132,6 +4132,180 @@ def test_raise_for_access_catalog_self_reference_does_not_weaken_sqllab_strict_m
sm.raise_for_access(query=query, force_dataset_match=True)
+def test_raise_for_access_editorship_denied_in_strict_mode(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Editorship of a registered dataset for the referenced table
+ must not authorize a raw query on the strict ``force_dataset_match`` path
+ (SQL Lab, MetaDB, MCP execute_sql). Any dataset-creating principal can
+ make themselves an editor of a dataset named after an arbitrary table, so
+ accepting editorship here would let dataset creation stand in for data
+ access and would outlive revocation of every real grant. The same
+ editorship stays sufficient on the non-strict path (the dataset-owner
+ "explore your own dataset" grant, #20382).
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mocker.patch.object(sm, "can_access_database", return_value=False)
+ mocker.patch.object(sm, "can_access", return_value=False)
+ mocker.patch.object(sm, "is_editor", return_value=True)
+ mocker.patch.object(sm, "is_guest_user", return_value=False)
+ SqlaTable = mocker.patch("superset.connectors.sqla.models.SqlaTable") # noqa: N806
+ # A registered dataset exists for the referenced table and the user is its
+ # editor, but holds no datasource_access (nor any catalog/schema grant).
+ SqlaTable.query_datasources_by_name.return_value = [
+ mocker.MagicMock(perm=f"[{MSSQL_CONN_NAME}].[dbo].[foo](id:1)")
+ ]
+
+ # Real Database so the SQL parse / engine-spec path is genuinely
+ # exercised, like the catalog-self-reference test above.
+ database = _mssql_database(mocker, "mssql+pymssql://user:pw@host:1433/abcm")
+ query = mocker.MagicMock(
+ database=database,
+ schema=None,
+ catalog=None,
+ sql="SELECT * FROM dbo.foo",
+ )
+
+ # Strict mode (SQL Lab / MetaDB / MCP): editorship alone is not enough.
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query=query, force_dataset_match=True)
+
+ # allow_editorship=False (dataset create/update validation): also not enough.
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query=query, allow_editorship=False)
+
+ # Non-strict mode: editorship still authorizes access (#20382).
+ sm.raise_for_access(query=query)
+
+
+def test_raise_for_access_allow_editorship_false_keeps_schema_access(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ allow_editorship=False must not drop the schema_access fallthrough: a
+ user with schema_access can still DEFINE a dataset over the table (unlike
+ force_dataset_match=True, which requires a registered dataset match).
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mocker.patch.object(sm, "can_access_database", return_value=False)
+ mocker.patch.object(sm, "is_editor", return_value=False)
+ mocker.patch.object(sm, "is_guest_user", return_value=False)
+ SqlaTable = mocker.patch("superset.connectors.sqla.models.SqlaTable") # noqa: N806
+ SqlaTable.query_datasources_by_name.return_value = []
+
+ database = _mssql_database(mocker, "mssql+pymssql://user:pw@host:1433/abcm")
+ query = mocker.MagicMock(
+ database=database, schema=None, catalog=None, sql="SELECT * FROM dbo.foo"
+ )
+ # Only schema_access holds (the dbo schema on this connection).
+ mocker.patch.object(
+ sm,
+ "can_access",
+ side_effect=lambda perm, vm: vm == f"[{MSSQL_CONN_NAME}].[dbo]",
+ )
+
+ # allow_editorship=False still passes on schema_access ...
+ sm.raise_for_access(query=query, allow_editorship=False)
+ # ... whereas the strict flag would deny it (no registered dataset).
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query=query, force_dataset_match=True)
+
+
+def test_raise_for_access_allow_editorship_false_keeps_catalog_access(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ allow_editorship=False must not drop the catalog_access fallthrough either:
+ a user with catalog_access on the referenced catalog is not over-blocked
+ when defining a dataset over a table in it.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mocker.patch.object(sm, "can_access_database", return_value=False)
+ mocker.patch.object(sm, "is_editor", return_value=False)
+ mocker.patch.object(sm, "is_guest_user", return_value=False)
+ SqlaTable = mocker.patch("superset.connectors.sqla.models.SqlaTable") # noqa: N806
+ SqlaTable.query_datasources_by_name.return_value = []
+
+ database = _mssql_database(mocker, "mssql+pymssql://user:pw@host:1433/abcm")
+ mocker.patch.object(database.db_engine_spec, "supports_catalog", True)
+ query = mocker.MagicMock(
+ database=database, schema="dbo", catalog="other", sql="SELECT * FROM dbo.foo"
+ )
+ # Only catalog_access holds (the "other" catalog on this connection).
+ mocker.patch.object(
+ sm,
+ "can_access",
+ side_effect=lambda perm, vm: perm == "catalog_access"
+ and vm == f"[{MSSQL_CONN_NAME}].[other]",
+ )
+
+ sm.raise_for_access(query=query, allow_editorship=False)
+
+
+def test_raise_for_access_allow_editorship_false_keeps_datasource_access(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ End-to-end ALLOW guard: a holder of real ``datasource_access`` on a
+ registered dataset for the referenced table passes even with
+ ``allow_editorship=False`` -- the define/repoint paths never over-block a
+ principal with genuine data access (only editorship-as-access is removed).
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mocker.patch.object(sm, "can_access_database", return_value=False)
+ mocker.patch.object(sm, "is_editor", return_value=False)
+ mocker.patch.object(sm, "is_guest_user", return_value=False)
+ SqlaTable = mocker.patch("superset.connectors.sqla.models.SqlaTable") # noqa: N806
+ SqlaTable.query_datasources_by_name.return_value = [
+ mocker.MagicMock(
+ perm=f"[{MSSQL_CONN_NAME}].[dbo].[foo](id:1)", sql=None, catalog=None
+ )
+ ]
+
+ database = _mssql_database(mocker, "mssql+pymssql://user:pw@host:1433/abcm")
+ query = mocker.MagicMock(
+ database=database, schema=None, catalog=None, sql="SELECT * FROM dbo.foo"
+ )
+ # Only datasource_access on the registered dataset holds.
+ mocker.patch.object(
+ sm,
+ "can_access",
+ side_effect=lambda perm, vm: perm == "datasource_access"
+ and vm == f"[{MSSQL_CONN_NAME}].[dbo].[foo](id:1)",
+ )
+
+ sm.raise_for_access(query=query, allow_editorship=False)
+
+
+def test_raise_for_access_datasource_branch_honors_allow_editorship(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ The ``datasource=`` branch honors ``allow_editorship`` like the per-table
+ branch: an editor holding no schema, datasource or semantic-layer grant is
+ refused when the caller opts out of the editorship accept, and keeps the
+ #20382 accept by default.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mocker.patch.object(sm, "can_access_schema", return_value=False)
+ mocker.patch.object(sm, "can_access", return_value=False)
+ mocker.patch.object(sm, "_semantic_layer_grant_allows", return_value=False)
+ mocker.patch.object(sm, "is_editor", return_value=True)
+ mocker.patch.object(sm, "is_guest_user", return_value=False)
+ datasource = mocker.MagicMock(perm="[db].[orders](id:1)")
+
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(datasource=datasource, allow_editorship=False)
+
+ sm.raise_for_access(datasource=datasource)
+
+
def test_get_datasources_accessible_by_user_schema_access(
mocker: MockerFixture,
app_context: None,
diff --git a/tests/unit_tests/views/datasource/views_test.py b/tests/unit_tests/views/datasource/views_test.py
index a5e7f22c413..47b643a3fa3 100644
--- a/tests/unit_tests/views/datasource/views_test.py
+++ b/tests/unit_tests/views/datasource/views_test.py
@@ -86,6 +86,7 @@ def _save_orm_dataset(**overrides: Any) -> MagicMock:
"schema": "public",
"catalog": None,
"sql": None,
+ "template_params": None,
"is_virtual": False,
"data": {"id": 1},
**overrides,
@@ -589,6 +590,22 @@ def test_save_repoint_to_unknown_database_is_rejected(
{"table_name": "my_table", "schema": "public"},
id="unchanged_datasource",
),
+ # Params are compared as execution reads them, so re-serialized JSON
+ # with the same values is not a repoint.
+ pytest.param(
+ {
+ "sql": "SELECT * FROM {{ t }}",
+ "is_virtual": True,
+ "template_params": '{"t": "public.orders"}',
+ },
+ {
+ "table_name": "my_table",
+ "schema": "public",
+ "sql": "SELECT * FROM {{ t }}",
+ "template_params": '{"t":"public.orders"}',
+ },
+ id="virtual_template_params_reserialized",
+ ),
],
)
def test_save_skips_table_check_when_not_repointed(
@@ -667,6 +684,8 @@ def test_save_rejects_sql_repoint_without_access(
sql=payload["sql"],
catalog=None,
schema=payload["schema"],
+ template_params={},
+ allow_editorship=False,
)
mock_get_database_by_id.assert_not_called()
@@ -697,16 +716,290 @@ def test_save_checks_both_table_and_sql_on_cross_database_repoint(
)
assert mock_security_manager.raise_for_access.call_args_list == [
- call(database=new_database, table=ANY),
+ call(database=new_database, table=ANY, allow_editorship=False),
call(
database=new_database,
sql="SELECT * FROM secret_table",
catalog=None,
schema="public",
+ template_params={},
+ allow_editorship=False,
),
]
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_save_rejects_template_params_repoint_without_access(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+) -> None:
+ """
+ With the SQL, database, schema and catalog unchanged, new
+ ``template_params`` still repoint a templated ``FROM``. The save is
+ authorised against the SQL as rendered with the submitted params, without
+ the editorship accept, and refused before anything is written.
+ """
+ mock_orm = _save_orm_dataset(
+ sql="SELECT * FROM {{ t }}",
+ is_virtual=True,
+ template_params='{"t": "public.orders"}',
+ )
+ mock_get_datasource.return_value = mock_orm
+ mock_security_manager.raise_for_access.side_effect = _security_exception()
+
+ with pytest.raises(DatasetForbiddenError):
+ _run_save(
+ database={"id": 1},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT * FROM {{ t }}",
+ template_params='{"t": "private.secrets"}',
+ )
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ database=mock_orm.database,
+ sql="SELECT * FROM {{ t }}",
+ catalog=None,
+ schema="public",
+ template_params={"t": "private.secrets"},
+ allow_editorship=False,
+ )
+ mock_orm.update_from_object.assert_not_called()
+ mock_get_database_by_id.assert_not_called()
+
+
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+@pytest.mark.parametrize("legacy_params", ['[["t", "v"]]', "{bad", " ", "42"])
+def test_save_metadata_edit_keeps_legacy_template_params(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+ legacy_params: str,
+) -> None:
+ """
+ A metadata-only save that echoes a legacy non-object ``template_params``
+ back (unchanged SQL/schema/database) is not a repoint and must not be
+ rejected -- no access check runs and nothing is written is blocked.
+ """
+ mock_orm = _save_orm_dataset(
+ sql="SELECT * FROM {{ t }}", is_virtual=True, template_params=legacy_params
+ )
+ mock_get_datasource.return_value = mock_orm
+
+ response = _run_save(
+ database={"id": 1},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT * FROM {{ t }}",
+ template_params=legacy_params,
+ )
+
+ # Not a repoint -> no access check, and the save proceeds (no 422).
+ mock_security_manager.raise_for_access.assert_not_called()
+ assert not (hasattr(response, "status_code") and response.status_code == 422)
+
+
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_save_template_params_type_change_is_rechecked(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+) -> None:
+ """
+ Changing only a param's JSON type (``1`` -> ``true``) repoints a templated
+ ``FROM`` (Jinja distinguishes them), so the save is re-authorised rather
+ than judged unchanged by Python dict equality.
+ """
+ mock_orm = _save_orm_dataset(
+ sql="SELECT * FROM {{ t }}", is_virtual=True, template_params='{"t": 1}'
+ )
+ mock_get_datasource.return_value = mock_orm
+ mock_security_manager.raise_for_access.side_effect = _security_exception()
+
+ with pytest.raises(DatasetForbiddenError):
+ _run_save(
+ database={"id": 1},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT * FROM {{ t }}",
+ template_params='{"t": true}',
+ )
+
+ assert mock_security_manager.raise_for_access.call_args.kwargs[
+ "template_params"
+ ] == {"t": True}
+ mock_orm.update_from_object.assert_not_called()
+
+
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_save_auth_check_drops_reserved_params(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+) -> None:
+ """
+ The repoint check renders with the submitted params minus the names the
+ template processor consumes structurally, matching execution.
+ """
+ mock_orm = _save_orm_dataset(
+ sql="SELECT * FROM {{ t }}", is_virtual=True, template_params='{"t": "a"}'
+ )
+ mock_get_datasource.return_value = mock_orm
+
+ _run_save(
+ database={"id": 1},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT * FROM {{ t }}",
+ template_params='{"t": "b", "extra_cache_keys": [], "database": "z"}',
+ )
+
+ assert mock_security_manager.raise_for_access.call_args.kwargs[
+ "template_params"
+ ] == {"t": "b"}
+
+
+@patch("superset.views.datasource.views._", _identity_gettext)
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+@pytest.mark.parametrize(
+ "template_params",
+ ["{bad", '"t"', '["t"]', '[["t", "private.secrets"]]'],
+)
+def test_save_rejects_unreadable_template_params(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+ template_params: str,
+) -> None:
+ """
+ Params that aren't a JSON object can't be read the way execution reads
+ them, so a save carrying SQL refuses them with a 422 -- not a 500, and not
+ an access decision made against a guess.
+ """
+ mock_orm = _save_orm_dataset(
+ sql="SELECT * FROM {{ t }}",
+ is_virtual=True,
+ template_params='{"t": "public.orders"}',
+ )
+ mock_get_datasource.return_value = mock_orm
+
+ response = _run_save(
+ database={"id": 1},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT * FROM {{ t }}",
+ template_params=template_params,
+ )
+
+ assert response.status_code == 422
+ mock_security_manager.raise_for_access.assert_not_called()
+ mock_orm.update_from_object.assert_not_called()
+
+
+@patch("superset.commands.dataset.exceptions._", _identity_gettext)
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+def test_save_render_error_is_rejected_as_invalid(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+) -> None:
+ """
+ SQL that can't be rendered with the submitted params can't be authorized:
+ the save is refused with a 422 instead of surfacing a 500.
+ """
+ mock_orm = _save_orm_dataset(sql="SELECT {{ p + 1 }}", is_virtual=True)
+ mock_get_datasource.return_value = mock_orm
+ mock_security_manager.raise_for_access.side_effect = TypeError(
+ 'can only concatenate str (not "int") to str'
+ )
+
+ response = _run_save(
+ database={"id": 1},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT {{ p + 1 }}",
+ template_params='{"p": "a"}',
+ )
+
+ assert response.status_code == 422
+ mock_orm.update_from_object.assert_not_called()
+
+
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+@pytest.mark.parametrize(
+ "submitted, rendered_with",
+ [
+ ('{"t": "public.orders"}', {"t": "public.orders"}),
+ # Omitted params land as ``None`` and replace the stored ones, which
+ # changes what the SQL reads even though nothing was submitted.
+ (None, {}),
+ ],
+)
+def test_save_rechecks_when_stored_template_params_unreadable(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+ submitted: str | None,
+ rendered_with: dict[str, Any],
+) -> None:
+ """
+ Stored params execution reads differently (a list of pairs reaches Jinja
+ through ``dict.update``) never count as unchanged, so the save is
+ authorised against the params it applies.
+ """
+ mock_orm = _save_orm_dataset(
+ sql="SELECT * FROM {{ t }}",
+ is_virtual=True,
+ template_params='[["t", "public.orders"]]',
+ )
+ mock_get_datasource.return_value = mock_orm
+ mock_security_manager.raise_for_access.side_effect = _security_exception()
+
+ with pytest.raises(DatasetForbiddenError):
+ _run_save(
+ database={"id": 1},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT * FROM {{ t }}",
+ template_params=submitted,
+ )
+
+ assert (
+ mock_security_manager.raise_for_access.call_args.kwargs["template_params"]
+ == rendered_with
+ )
+ mock_orm.update_from_object.assert_not_called()
+
+
@patch("superset.views.datasource.views.db")
@patch("superset.views.datasource.views.DatasetDAO.get_database_by_id")
@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)