You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2026/10/16 22:12:04 UTC
(superset) 14/17: fix(datasource): apply target-table access check on same-database repoint (#44080)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch 7.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 83cfceb54588881badf957c098b086361ca218cc
Author: Shaitan <10...@users.noreply.github.com>
AuthorDate: Tue Oct 13 10:32:43 2026 +0100
fix(datasource): apply target-table access check on same-database repoint (#44080)
Co-authored-by: Claude Opus 4.8 <no...@anthropic.com>
(cherry picked from commit e7ca70c784e4586facfe54ccbe64c5ff0bb8fc28)
---
superset/connectors/sqla/models.py | 4 +-
superset/views/datasource/views.py | 176 +++++++-
tests/unit_tests/views/datasource/views_test.py | 559 +++++++++++++++++-------
3 files changed, 548 insertions(+), 191 deletions(-)
diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
index 9c684b83bcb..b22e7e9577a 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -806,7 +806,9 @@ class BaseDatasource(
match it. Since the fields are different for the different
connectors, the implementation uses ``update_from_object_fields``
which can be defined for each connector and
- defines which fields should be synced"""
+ defines which fields should be synced. Note that this replaces rather
+ than merges: a key omitted from ``obj`` clears the field rather than
+ leaving it untouched."""
for attr in self.update_from_object_fields:
setattr(self, attr, obj.get(attr))
diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py
index fcfee22c1ff..650890a884e 100644
--- a/superset/views/datasource/views.py
+++ b/superset/views/datasource/views.py
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
from collections import Counter
-from typing import Any
+from typing import Any, cast
from flask import redirect, request, url_for
from flask_appbuilder import expose, permission_name
@@ -87,6 +87,142 @@ def _load_dataset_for_samples(
return dataset, None
+def _repoints_table(
+ datasource: DatasourceModel,
+ database_changed: bool,
+ requested_sql: str | None,
+ requested_table: Table,
+) -> bool:
+ """
+ Would the request point ``datasource`` at a different physical table?
+
+ Moving to another database connection always does, whatever the target
+ looks like. Within one connection, a virtual dataset's ``table_name`` is a
+ label rather than a pointer, as in
+ ``UpdateDatasetCommand._validate_dataset_source``, so dropping the SQL
+ binds that label to a real table: a repoint even when the label is
+ unchanged.
+ """
+ # Ahead of the dataset check on purpose: a move to another connection is
+ # a repoint whatever the datasource is, and must not go unchecked.
+ if database_changed:
+ return True
+ if not isinstance(datasource, SqlaTable) or requested_sql:
+ return False
+ if datasource.is_virtual:
+ return True
+ # Compared field by field because ``Table.__eq__`` compares the ``str``
+ # rendering, which drops empty parts and so conflates distinct targets.
+ return (requested_table.table, requested_table.schema, requested_table.catalog) != (
+ datasource.table_name,
+ datasource.schema or None,
+ datasource.catalog or None,
+ )
+
+
+def _repoints_sql(
+ datasource: DatasourceModel,
+ database_changed: bool,
+ requested_sql: str | None,
+ requested_table: Table,
+) -> bool:
+ """
+ Would the request change what the SQL on ``datasource`` reads?
+
+ 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.
+ """
+ if not isinstance(datasource, SqlaTable) or not requested_sql:
+ return False
+ return (
+ database_changed
+ or requested_sql != datasource.sql
+ or (requested_table.schema, requested_table.catalog)
+ != (datasource.schema or None, datasource.catalog or None)
+ )
+
+
+def _requested_target(datasource_dict: dict[str, Any]) -> tuple[str | None, Table]:
+ """The SQL and table a save aims the dataset at.
+
+ ``update_from_object`` applies an omitted key as ``None``, so the request
+ body alone describes the target: no stored value feeds into it. The body
+ is free-form JSON, so each field is type-checked here, since the access
+ checks below parse these values and only a string survives that.
+
+ Raises ``ValueError`` naming the offending field.
+ """
+
+ def string(field: str) -> str | None:
+ value = datasource_dict.get(field)
+ if value is not None and not isinstance(value, str):
+ 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,
+ )
+
+
+def _authorize_repoint(
+ datasource: DatasourceModel,
+ database_id: int,
+ database_changed: bool,
+ requested_sql: str | None,
+ requested_table: Table,
+) -> FlaskResponse | None:
+ """Authorise the target a save aims ``datasource`` at.
+
+ Editorship of the dataset alone is not sufficient to repoint it. Ports
+ ``UpdateDatasetCommand``'s table and SQL checks, which are independent:
+ one save can do both, and each needs its own ``raise_for_access`` call
+ because passing ``sql`` builds an ephemeral query that supersedes
+ ``table``.
+
+ Returns an error response when the target cannot be resolved, and ``None``
+ once the target is authorised or the save does not repoint at all.
+ """
+ repoints_table = _repoints_table(
+ datasource, database_changed, requested_sql, requested_table
+ )
+ repoints_sql = _repoints_sql(
+ datasource, database_changed, requested_sql, requested_table
+ )
+ if not (repoints_table or repoints_sql):
+ return None
+
+ target_database = (
+ DatasetDAO.get_database_by_id(database_id)
+ if database_changed
+ else datasource.database
+ )
+ if target_database is None:
+ return json_error_response(_("Database not found."), status=422)
+ try:
+ if repoints_table:
+ security_manager.raise_for_access(
+ database=target_database,
+ table=requested_table,
+ )
+ if repoints_sql:
+ security_manager.raise_for_access(
+ database=target_database,
+ sql=requested_sql,
+ catalog=requested_table.catalog,
+ schema=requested_table.schema,
+ )
+ except SupersetSecurityException as ex:
+ raise DatasetForbiddenError() from ex
+ return None
+
+
class Datasource(BaseSupersetView):
"""Datasource-related views"""
@@ -121,25 +257,25 @@ class Datasource(BaseSupersetView):
except SupersetSecurityException as ex:
raise DatasetForbiddenError() from ex
- if database_id != orm_datasource.database_id:
- new_database = DatasetDAO.get_database_by_id(database_id)
- if new_database is None:
- return json_error_response(_("Database not found."), status=422)
- try:
- security_manager.raise_for_access(
- database=new_database,
- # Check access against the table/schema/catalog the
- # request is repointing to, not the dataset's current
- # values -- update_from_object (below) applies whatever
- # table_name/schema/catalog the request supplies.
- table=Table(
- datasource_dict.get("table_name", orm_datasource.table_name),
- datasource_dict.get("schema", orm_datasource.schema),
- datasource_dict.get("catalog", orm_datasource.catalog),
- ),
- )
- except SupersetSecurityException as ex:
- raise DatasetForbiddenError() from ex
+ database_changed = database_id != orm_datasource.database_id
+ try:
+ requested_sql, requested_table = _requested_target(datasource_dict)
+ except ValueError as ex:
+ return json_error_response(
+ _("The `%(field)s` field must be a string.", field=str(ex)),
+ status=422,
+ )
+
+ if error := _authorize_repoint(
+ orm_datasource,
+ database_id,
+ database_changed,
+ requested_sql,
+ requested_table,
+ ):
+ return error
+
+ if database_changed:
orm_datasource.database_id = database_id
duplicates = [
diff --git a/tests/unit_tests/views/datasource/views_test.py b/tests/unit_tests/views/datasource/views_test.py
index 5ba3860e27b..c1953675b3d 100644
--- a/tests/unit_tests/views/datasource/views_test.py
+++ b/tests/unit_tests/views/datasource/views_test.py
@@ -21,18 +21,27 @@ bypassing the Flask-AppBuilder permission decorator machinery.
"""
import inspect
-from unittest.mock import MagicMock, patch
+from typing import Any
+from unittest.mock import ANY, call, MagicMock, patch
import pytest
+from flask import Flask
+from superset.commands.dataset.exceptions import DatasetForbiddenError
+from superset.connectors.sqla.models import SqlaTable
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import SupersetSecurityException
+from superset.sql.parse import Table
from superset.utils import json as superset_json
-def _identity_gettext(message: str) -> str:
- """Typed stand-in for flask-babel's ``_`` in request-less unit tests."""
- return message
+def _identity_gettext(message: str, **variables: Any) -> str:
+ """Typed stand-in for flask-babel's ``_`` in request-less unit tests.
+
+ Interpolates like the real one so a caller passing variables is exercised
+ rather than raising on the extra arguments.
+ """
+ return message % variables if variables else message
def _security_exception() -> SupersetSecurityException:
@@ -59,6 +68,56 @@ def _view_self() -> MagicMock:
return self
+def _save_orm_dataset(**overrides: Any) -> MagicMock:
+ """Stand-in for the dataset ``save`` loads, defaulting to a physical one.
+
+ Specced on ``SqlaTable`` so it satisfies the dataset check in ``save``.
+ """
+ return MagicMock(
+ spec=SqlaTable,
+ **{
+ "database_id": 1,
+ "table_name": "my_table",
+ "schema": "public",
+ "catalog": None,
+ "sql": None,
+ "is_virtual": False,
+ "data": {"id": 1},
+ **overrides,
+ },
+ )
+
+
+def _assert_checked_target(table: Table, payload: dict[str, Any]) -> None:
+ """Assert the check ran against the target ``update_from_object`` applies.
+
+ An omitted key lands as ``None``, so the payload alone describes the
+ target -- the dataset's stored values never feed into it.
+ """
+ assert (table.table, table.schema, table.catalog) == (
+ payload.get("table_name"),
+ payload.get("schema"),
+ payload.get("catalog"),
+ )
+
+
+_SAVE_APP = Flask(__name__)
+
+
+def _run_save(**payload: Any) -> Any:
+ """Call the unwrapped ``save`` view with ``payload`` as the request body."""
+ with _SAVE_APP.test_request_context(
+ "/datasource/save/",
+ method="POST",
+ data={
+ "data": superset_json.dumps(
+ {"id": 1, "type": "table", "columns": [], **payload}
+ )
+ },
+ ):
+ return _get_view_func("save")(_view_self())
+
+
# ---------------------------------------------------------------------------
# Datasource.get
# ---------------------------------------------------------------------------
@@ -246,29 +305,9 @@ def test_save_always_checks_editorship_even_without_editors_field(
)
)
- from flask import Flask
-
- from superset.commands.dataset.exceptions import DatasetForbiddenError
-
- raw_save = _get_view_func("save")
- app = Flask(__name__)
- with app.test_request_context(
- "/datasource/save/",
- method="POST",
- data={
- "data": superset_json.dumps(
- {
- "id": 1,
- "type": "table",
- "database": {"id": 1},
- "columns": [],
- # 'editors' intentionally omitted
- }
- )
- },
- ):
- with pytest.raises(DatasetForbiddenError):
- raw_save(_view_self())
+ with pytest.raises(DatasetForbiddenError):
+ # 'editors' intentionally omitted
+ _run_save(database={"id": 1})
mock_security_manager.raise_for_editorship.assert_called_once_with(mock_orm)
@@ -290,29 +329,9 @@ def test_save_non_editor_with_editors_field_is_rejected(
)
)
- from flask import Flask
-
- from superset.commands.dataset.exceptions import DatasetForbiddenError
-
- raw_save = _get_view_func("save")
- app = Flask(__name__)
- with app.test_request_context(
- "/datasource/save/",
- method="POST",
- data={
- "data": superset_json.dumps(
- {
- "id": 1,
- "type": "table",
- "database": {"id": 1},
- "columns": [],
- "editors": [99], # attacker-supplied editors list
- }
- )
- },
- ):
- with pytest.raises(DatasetForbiddenError):
- raw_save(_view_self())
+ with pytest.raises(DatasetForbiddenError):
+ # editors list supplied by the caller
+ _run_save(database={"id": 1}, editors=[99])
mock_security_manager.raise_for_editorship.assert_called_once_with(mock_orm)
@@ -334,45 +353,19 @@ def test_save_rejects_repoint_to_database_without_access(
database -- the caller must also be authorised for the *new* database,
checked via ``raise_for_access`` before ``database_id`` is reassigned.
"""
- mock_orm = MagicMock()
- mock_orm.database_id = 1
- mock_orm.table_name = "my_table"
- mock_orm.schema = "public"
- mock_orm.catalog = None
- mock_orm.data = {"id": 1}
+ mock_orm = _save_orm_dataset()
mock_get_datasource.return_value = mock_orm
- # Caller owns the dataset, so that check passes...
- mock_security_manager.raise_for_editorship.return_value = None
mock_new_database = MagicMock()
mock_get_database_by_id.return_value = mock_new_database
- # ...but the caller is not authorised for the new database.
+ # The caller owns the dataset but is not authorised for the new database.
mock_security_manager.raise_for_access.side_effect = _security_exception()
- from flask import Flask
-
- from superset.commands.dataset.exceptions import DatasetForbiddenError
-
- raw_save = _get_view_func("save")
- app = Flask(__name__)
- with app.test_request_context(
- "/datasource/save/",
- method="POST",
- data={
- "data": superset_json.dumps(
- {
- "id": 1,
- "type": "table",
- # database id 999 stands in for a database the caller
- # has no explicit grant on.
- "database": {"id": 999},
- "columns": [],
- }
- )
- },
- ):
- with pytest.raises(DatasetForbiddenError):
- raw_save(_view_self())
+ payload = {"table_name": "my_table", "schema": "public"}
+ with pytest.raises(DatasetForbiddenError):
+ # database id 999 stands in for a database the caller has no explicit
+ # grant on.
+ _run_save(database={"id": 999}, **payload)
# Ownership of the dataset was checked...
mock_security_manager.raise_for_editorship.assert_called_once_with(mock_orm)
@@ -380,8 +373,7 @@ def test_save_rejects_repoint_to_database_without_access(
mock_security_manager.raise_for_access.assert_called_once()
call_kwargs = mock_security_manager.raise_for_access.call_args.kwargs
assert call_kwargs["database"] is mock_new_database
- assert call_kwargs["table"].table == "my_table"
- assert call_kwargs["table"].schema == "public"
+ _assert_checked_target(call_kwargs["table"], payload)
# The ORM object was NOT repointed since access was denied.
assert mock_orm.database_id == 1
@@ -390,53 +382,48 @@ def test_save_rejects_repoint_to_database_without_access(
@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(
+ "orm_overrides,payload",
+ [
+ # The same table carried across the repoint.
+ pytest.param(
+ {},
+ {"table_name": "my_table", "schema": "public"},
+ id="table_preserved",
+ ),
+ # A cross-database repoint can change the table in the same payload,
+ # so the check must run against the requested table, not the stale one.
+ pytest.param(
+ {"table_name": "authorised_table"},
+ {"table_name": "secret_table", "schema": "finance"},
+ id="table_changed_too",
+ ),
+ ],
+)
def test_save_allows_repoint_to_database_with_access(
mock_get_datasource: MagicMock,
mock_security_manager: MagicMock,
mock_get_database_by_id: MagicMock,
mock_db: MagicMock,
+ orm_overrides: dict[str, Any],
+ payload: dict[str, Any],
) -> None:
"""
- When the caller is authorised for the new database, ``save`` proceeds
- to repoint ``database_id``.
+ When the caller is authorised for the new database, ``save`` proceeds to
+ repoint ``database_id``, having checked the target the request applies.
"""
- mock_orm = MagicMock()
- mock_orm.database_id = 1
- mock_orm.table_name = "my_table"
- mock_orm.schema = "public"
- mock_orm.catalog = None
- mock_orm.data = {"id": 1}
+ mock_orm = _save_orm_dataset(**orm_overrides)
mock_get_datasource.return_value = mock_orm
- mock_security_manager.raise_for_editorship.return_value = None
mock_new_database = MagicMock()
mock_get_database_by_id.return_value = mock_new_database
- mock_security_manager.raise_for_access.return_value = None
-
- from flask import Flask
- raw_save = _get_view_func("save")
- app = Flask(__name__)
- with app.test_request_context(
- "/datasource/save/",
- method="POST",
- data={
- "data": superset_json.dumps(
- {
- "id": 1,
- "type": "table",
- "database": {"id": 999},
- "columns": [],
- }
- )
- },
- ):
- raw_save(_view_self())
+ _run_save(database={"id": 999}, **payload)
mock_security_manager.raise_for_access.assert_called_once()
call_kwargs = mock_security_manager.raise_for_access.call_args.kwargs
assert call_kwargs["database"] is mock_new_database
- assert call_kwargs["table"].table == "my_table"
+ _assert_checked_target(call_kwargs["table"], payload)
assert mock_orm.database_id == 999
@@ -444,61 +431,305 @@ def test_save_allows_repoint_to_database_with_access(
@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_checks_access_against_requested_table_not_stale_one(
+@pytest.mark.parametrize(
+ "orm_overrides,payload",
+ [
+ # Keeping the same ``database.id`` while changing
+ # ``table_name``/``schema``/``catalog`` still repoints the dataset via
+ # ``update_from_object``: editorship of the dataset alone must not let
+ # a caller point it at an unauthorised table in the same database.
+ pytest.param(
+ {"table_name": "authorised_table"},
+ {"table_name": "secret_table", "schema": "finance"},
+ id="same_database_repoint",
+ ),
+ # Dropping ``sql`` binds the label to a real table, so the conversion
+ # is a repoint even when the label itself is unchanged.
+ pytest.param(
+ {
+ "table_name": "secret_table",
+ "schema": "finance",
+ "sql": "SELECT 1",
+ "is_virtual": True,
+ },
+ {"table_name": "secret_table", "schema": "finance"},
+ id="virtual_becomes_physical",
+ ),
+ # An omitted key is applied as ``None`` rather than left alone, so it
+ # must read as a change and be checked against the target that lands.
+ pytest.param(
+ {"table_name": "authorised_table"},
+ {},
+ id="table_key_omitted",
+ ),
+ ],
+)
+def test_save_rejects_same_database_repoint_without_access(
mock_get_datasource: MagicMock,
mock_security_manager: MagicMock,
mock_get_database_by_id: MagicMock,
mock_db: MagicMock,
+ orm_overrides: dict[str, Any],
+ payload: dict[str, Any],
) -> None:
"""
- A request that repoints ``database.id`` can also change
- ``table_name``/``schema``/``catalog`` in the same payload --
- ``update_from_object`` applies those requested values afterwards.
- The access check must therefore be evaluated against the *requested*
- table, not the dataset's current (stale) one, or a caller could pass
- the check using a table they're authorised for while actually
- repointing to one they are not.
+ A save that repoints the dataset within its current database must be
+ authorised against the target table the request will actually apply.
"""
- mock_orm = MagicMock()
- mock_orm.database_id = 1
- mock_orm.table_name = "authorised_table"
- mock_orm.schema = "public"
- mock_orm.catalog = None
- mock_orm.data = {"id": 1}
+ mock_orm = _save_orm_dataset(**orm_overrides)
mock_get_datasource.return_value = mock_orm
- mock_security_manager.raise_for_editorship.return_value = None
+ mock_security_manager.raise_for_access.side_effect = _security_exception()
- mock_new_database = MagicMock()
- mock_get_database_by_id.return_value = mock_new_database
- mock_security_manager.raise_for_access.return_value = None
+ with pytest.raises(DatasetForbiddenError):
+ _run_save(database={"id": 1}, **payload)
- from flask import Flask
+ mock_security_manager.raise_for_access.assert_called_once()
+ call_kwargs = mock_security_manager.raise_for_access.call_args.kwargs
+ assert call_kwargs["database"] is mock_orm.database
+ _assert_checked_target(call_kwargs["table"], payload)
+ # No cross-database lookup for a same-database repoint.
+ mock_get_database_by_id.assert_not_called()
- raw_save = _get_view_func("save")
- app = Flask(__name__)
- with app.test_request_context(
- "/datasource/save/",
- method="POST",
- data={
- "data": superset_json.dumps(
- {
- "id": 1,
- "type": "table",
- "database": {"id": 999},
- "table_name": "secret_table",
- "schema": "finance",
- "columns": [],
- }
- )
- },
- ):
- raw_save(_view_self())
- call_kwargs = mock_security_manager.raise_for_access.call_args.kwargs
- assert call_kwargs["database"] is mock_new_database
- # The check ran against the requested table, not the dataset's old one.
- assert call_kwargs["table"].table == "secret_table"
- assert call_kwargs["table"].schema == "finance"
+@patch("superset.views.datasource.views._", _identity_gettext)
+@patch("superset.views.datasource.views.db")
+@patch("superset.views.datasource.views.security_manager", new_callable=MagicMock)
+@patch("superset.views.datasource.views.DatasourceDAO.get_datasource")
+@pytest.mark.parametrize("field", ["sql", "table_name", "schema", "catalog"])
+@pytest.mark.parametrize("value", [1, {"a": 1}, ["SELECT 1"], True])
+def test_save_rejects_non_string_target_field(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_db: MagicMock,
+ field: str,
+ value: Any,
+) -> None:
+ """
+ The target fields arrive as free-form JSON. A non-string is refused up
+ front rather than reaching the access check, which parses ``sql`` and
+ renders ``Table`` and so would fail on the type.
+ """
+ mock_orm = _save_orm_dataset()
+ mock_get_datasource.return_value = mock_orm
+
+ response = _run_save(database={"id": 1}, **{field: value})
+
+ assert response.status_code == 422
+ assert field in response.get_data(as_text=True)
+ mock_security_manager.raise_for_access.assert_not_called()
+ mock_orm.update_from_object.assert_not_called()
+
+
+@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")
+def test_save_repoint_to_unknown_database_is_rejected(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+) -> None:
+ """
+ A repoint naming a database that does not exist is refused before any
+ access check, rather than resolving to ``None`` and skipping one.
+ """
+ mock_orm = _save_orm_dataset()
+ mock_get_datasource.return_value = mock_orm
+ mock_get_database_by_id.return_value = None
+
+ response = _run_save(database={"id": 999}, table_name="my_table")
+
+ assert response.status_code == 422
+ mock_security_manager.raise_for_access.assert_not_called()
+ assert mock_orm.database_id == 1
+
+
+@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(
+ "orm_overrides,payload",
+ [
+ # A dataset that stays virtual keeps ``table_name`` as a label, so
+ # renaming it is not a repoint.
+ pytest.param(
+ {
+ "table_name": "my_virtual_dataset",
+ "sql": "SELECT 1",
+ "is_virtual": True,
+ },
+ {
+ "table_name": "renamed_virtual_dataset",
+ "schema": "public",
+ "sql": "SELECT 1",
+ },
+ id="virtual_dataset_renamed",
+ ),
+ # ``BaseDatasource.data`` emits an empty schema as ``None``, so a
+ # dataset stored with ``schema = ""`` round-trips through the editor as
+ # ``None``. That is not a repoint.
+ pytest.param(
+ {"schema": "", "catalog": ""},
+ {"table_name": "my_table", "schema": None, "catalog": None},
+ id="empty_schema_round_trip",
+ ),
+ # A plain save that changes neither the database nor the table (e.g. a
+ # column/metric edit); editorship already gates it, and re-checking
+ # would be a behavior change.
+ pytest.param(
+ {},
+ {"table_name": "my_table", "schema": "public"},
+ id="unchanged_datasource",
+ ),
+ ],
+)
+def test_save_skips_table_check_when_not_repointed(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+ orm_overrides: dict[str, Any],
+ payload: dict[str, Any],
+) -> None:
+ """
+ A save that does not repoint the dataset at a new physical table must not
+ trigger a fresh datasource-access check.
+ """
+ mock_get_datasource.return_value = _save_orm_dataset(**orm_overrides)
+
+ _run_save(database={"id": 1}, **payload)
+
+ mock_security_manager.raise_for_access.assert_not_called()
+ # No cross-database lookup for a same-database save.
+ 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(
+ "orm_overrides,payload",
+ [
+ # Giving a physical dataset SQL converts it to a virtual one, since
+ # ``is_virtual`` derives from whether ``sql`` is set.
+ pytest.param(
+ {},
+ {"sql": "SELECT * FROM secret_table", "schema": "public"},
+ id="physical_becomes_virtual",
+ ),
+ # Aiming an already-virtual dataset at different SQL is the same
+ # repoint, minus the conversion.
+ pytest.param(
+ {"sql": "SELECT 1", "is_virtual": True},
+ {"sql": "SELECT * FROM secret_table", "schema": "public"},
+ id="virtual_sql_replaced",
+ ),
+ # Unchanged SQL still reads elsewhere when the schema it resolves
+ # unqualified names against moves.
+ pytest.param(
+ {"sql": "SELECT * FROM t", "is_virtual": True},
+ {"sql": "SELECT * FROM t", "schema": "finance"},
+ id="virtual_schema_moved",
+ ),
+ ],
+)
+def test_save_rejects_sql_repoint_without_access(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+ orm_overrides: dict[str, Any],
+ payload: dict[str, Any],
+) -> None:
+ """
+ A save that aims the dataset's SQL somewhere new repoints it at whatever
+ that SQL reads, so it must be authorised against the SQL rather than
+ skipped because the table pointer is unchanged.
+ """
+ mock_orm = _save_orm_dataset(**orm_overrides)
+ 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", **payload)
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ database=mock_orm.database,
+ sql=payload["sql"],
+ catalog=None,
+ schema=payload["schema"],
+ )
+ 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")
+def test_save_checks_both_table_and_sql_on_cross_database_repoint(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+) -> None:
+ """
+ The table and SQL checks are independent, as in ``UpdateDatasetCommand``:
+ a save can move the dataset to another database and give it SQL at once,
+ and both have to be authorised against the new connection.
+ """
+ mock_get_datasource.return_value = _save_orm_dataset()
+ new_database = mock_get_database_by_id.return_value
+
+ _run_save(
+ database={"id": 2},
+ table_name="my_table",
+ schema="public",
+ sql="SELECT * FROM secret_table",
+ )
+
+ assert mock_security_manager.raise_for_access.call_args_list == [
+ call(database=new_database, table=ANY),
+ call(
+ database=new_database,
+ sql="SELECT * FROM secret_table",
+ catalog=None,
+ schema="public",
+ ),
+ ]
+
+
+@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_of_non_dataset_datasource_skips_table_check(
+ mock_get_datasource: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
+ mock_db: MagicMock,
+) -> None:
+ """
+ ``save`` also accepts queries, saved queries and semantic views. Those
+ models have no ``table_name``/``is_virtual`` (a query carries a
+ ``tmp_table_name`` instead), so the repoint check must not read them.
+ """
+ # Specced to a query's attribute surface: touching table_name or
+ # is_virtual on this mock raises AttributeError.
+ mock_orm = MagicMock(
+ spec=["database_id", "database", "data", "tmp_table_name", "update_from_object"]
+ )
+ mock_orm.database_id = 1
+ mock_orm.data = {"id": 1}
+ mock_get_datasource.return_value = mock_orm
+
+ _run_save(type="query", database={"id": 1})
+
+ mock_security_manager.raise_for_access.assert_not_called()
+ mock_get_database_by_id.assert_not_called()
# ---------------------------------------------------------------------------
@@ -516,8 +747,6 @@ def test_samples_returns_400_for_unsupported_datasource_type(
mock_get_samples: MagicMock,
) -> None:
"""Semantic views can't return raw samples — endpoint should refuse with 400."""
- from flask import Flask
-
mock_security_manager.is_guest_user.return_value = False
mock_json_error_response.return_value = "error-response"
@@ -547,8 +776,6 @@ def test_samples_checks_guest_access_before_datasource_capability(
mock_get_samples: MagicMock,
) -> None:
"""An unauthorized guest gets 403 before semantic-view capability errors."""
- from flask import Flask
-
mock_security_manager.is_guest_user.return_value = True
mock_json_error_response.return_value = "forbidden-response"
@@ -583,8 +810,6 @@ def test_samples_guest_with_dashboard_gets_404_for_non_dataset_type(
unrelated id spaces, so ``DatasetDAO.find_by_id`` would validate whichever
unrelated table shares the integer id.
"""
- from flask import Flask
-
mock_security_manager.is_guest_user.return_value = True
view = _view_self()
@@ -618,8 +843,6 @@ def test_samples_guest_drill_denial_returns_403(
mock_get_samples: MagicMock,
) -> None:
"""A guest failing the drill-access check gets 403 from that gate."""
- from flask import Flask
-
mock_security_manager.is_guest_user.return_value = True
mock_security_manager.can_drill_dataset_via_dashboard_access.return_value = False
mock_json_error_response.return_value = "forbidden-response"
@@ -648,8 +871,6 @@ def test_samples_proceeds_for_supported_datasource_type(
mock_get_samples: MagicMock,
) -> None:
"""A `query` datasource (supports_samples=True) bypasses the 400 short-circuit."""
- from flask import Flask
-
mock_security_manager.is_guest_user.return_value = False
mock_get_samples.return_value = {"rows": []}
@@ -684,8 +905,6 @@ def test_samples_authenticated_dataset_access_denied_returns_403_before_fetch(
authenticated per-object check only runs for dataset-backed types, which
always support samples, so this pins authorization-before-fetch rather than
a race against the ``supports_samples`` gate.)"""
- from flask import Flask
-
mock_security_manager.is_guest_user.return_value = False
mock_dataset = MagicMock()
mock_get_datasource.return_value = mock_dataset