You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/08/09 15:55:35 UTC
(superset) 03/03: fix(datasets): verify database access before repointing a dataset's database_id
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix-authz-guard-gaps
in repository https://gitbox.apache.org/repos/asf/superset.git
commit c0273d9534e03e8ee1846c92d8069a500fd7e3b1
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Sun Aug 9 08:20:05 2026 -0700
fix(datasets): verify database access before repointing a dataset's database_id
Both the legacy Datasource.save view and UpdateDatasetCommand let an
editor change a dataset's database_id to any value without checking
whether they have access to the target database connection -- only
ownership of the dataset itself was verified. Editorship of a dataset
does not imply access to every database connection in the deployment.
Route the database_id change through security_manager.raise_for_access
with the resolved Database object (and the dataset's table) before the
new id is persisted, mirroring the check CreateDatasetCommand already
performs when attaching a new physical dataset to a database. When
access is denied, database_id is left unchanged.
---
superset/commands/dataset/update.py | 17 +++-
superset/views/datasource/views.py | 18 +++-
tests/unit_tests/commands/dataset/update_test.py | 104 +++++++++++++++++++++++
tests/unit_tests/views/datasource/views_test.py | 94 +++++++++++++++++---
4 files changed, 217 insertions(+), 16 deletions(-)
diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py
index 338603c1e40..057ebf7de13 100644
--- a/superset/commands/dataset/update.py
+++ b/superset/commands/dataset/update.py
@@ -127,9 +127,7 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
new_db_connection: Database | None = None
if database_id and database_id != self._model.database.id:
- if new_db_connection := DatasetDAO.get_database_by_id(database_id):
- self._properties["database"] = new_db_connection
- else:
+ if not (new_db_connection := DatasetDAO.get_database_by_id(database_id)):
exceptions.append(DatabaseNotFoundValidationError())
db = new_db_connection or self._model.database
default_catalog = db.get_default_catalog()
@@ -164,6 +162,19 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
catalog,
)
+ # Repointing to a different database connection requires access to
+ # that connection, independent of the caller's editorship of this
+ # dataset -- only persist the change once that's confirmed.
+ if new_db_connection:
+ try:
+ security_manager.raise_for_access(
+ database=new_db_connection, table=table
+ )
+ except SupersetSecurityException as ex:
+ exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
+ else:
+ self._properties["database"] = new_db_connection
+
# Validate uniqueness
if not DatasetDAO.validate_update_uniqueness(
db,
diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py
index 7cb96074f3e..b5b5650c56e 100644
--- a/superset/views/datasource/views.py
+++ b/superset/views/datasource/views.py
@@ -83,13 +83,29 @@ class Datasource(BaseSupersetView):
orm_datasource = DatasourceDAO.get_datasource(
DatasourceType(datasource_type), datasource_id
)
- orm_datasource.database_id = database_id
try:
security_manager.raise_for_editorship(orm_datasource)
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,
+ table=Table(
+ orm_datasource.table_name,
+ orm_datasource.schema,
+ orm_datasource.catalog,
+ ),
+ )
+ except SupersetSecurityException as ex:
+ raise DatasetForbiddenError() from ex
+ orm_datasource.database_id = database_id
+
duplicates = [
name
for name, count in Counter(
diff --git a/tests/unit_tests/commands/dataset/update_test.py b/tests/unit_tests/commands/dataset/update_test.py
index d4742ebd494..6d343ee00b3 100644
--- a/tests/unit_tests/commands/dataset/update_test.py
+++ b/tests/unit_tests/commands/dataset/update_test.py
@@ -174,6 +174,110 @@ def test_update_dataset_sql_unauthorized_schema(mocker: MockerFixture) -> None:
)
+def test_update_dataset_database_id_change_checks_new_database_access(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Changing ``database_id`` alone (with no ``sql`` in the payload) must
+ still be authorised against the new database connection: ownership of
+ the dataset (``raise_for_editorship``) is not enough. When the caller
+ isn't authorised for the new database, the update is rejected and the
+ dataset is not repointed.
+ """
+ mock_dataset_dao = mocker.patch("superset.commands.dataset.update.DatasetDAO")
+ mock_current_database = mocker.MagicMock()
+ mock_current_database.id = 1
+
+ mock_new_database = mocker.MagicMock()
+ mock_new_database.id = 2
+ mock_new_database.get_default_catalog.return_value = "catalog"
+ mock_new_database.allow_multi_catalog = False
+
+ mock_dataset = mocker.MagicMock()
+ mock_dataset.database = mock_current_database
+ mock_dataset.catalog = "catalog"
+ mock_dataset.schema = "public"
+ mock_dataset.table_name = "test_table"
+ mock_dataset.editors = [] # No editors to avoid computation issues
+
+ mock_dataset_dao.find_by_id.return_value = mock_dataset
+ mock_dataset_dao.get_database_by_id.return_value = mock_new_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)
+
+ mock_raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_access",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR,
+ message="You don't have access to that database",
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+
+ 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
+ assert any(
+ "You don't have access to that database" in str(exc)
+ for exc in excinfo.value._exceptions
+ )
+ # The update never runs, so the dataset is never repointed.
+ mock_dataset_dao.update.assert_not_called()
+
+
+def test_update_dataset_database_id_change_allowed_with_access(
+ mocker: MockerFixture,
+) -> None:
+ """
+ When the caller is authorised for the new database, changing
+ ``database_id`` alone succeeds and the dataset is repointed to it.
+ """
+ mock_dataset_dao = mocker.patch("superset.commands.dataset.update.DatasetDAO")
+ mock_current_database = mocker.MagicMock()
+ mock_current_database.id = 1
+
+ mock_new_database = mocker.MagicMock()
+ mock_new_database.id = 2
+ mock_new_database.get_default_catalog.return_value = "catalog"
+ mock_new_database.allow_multi_catalog = False
+
+ mock_dataset = mocker.MagicMock()
+ mock_dataset.database = mock_current_database
+ mock_dataset.catalog = "catalog"
+ mock_dataset.schema = "public"
+ mock_dataset.table_name = "test_table"
+ mock_dataset.editors = [] # No editors to avoid computation issues
+
+ mock_dataset_dao.find_by_id.return_value = mock_dataset
+ mock_dataset_dao.get_database_by_id.return_value = mock_new_database
+ mock_dataset_dao.validate_update_uniqueness.return_value = True
+ mock_dataset_dao.update.return_value = mock_dataset
+
+ mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_editorship",
+ )
+ mocker.patch("superset.commands.utils.security_manager.is_admin", return_value=True)
+ mock_raise_for_access = mocker.patch(
+ "superset.commands.dataset.update.security_manager.raise_for_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
+ assert result == mock_dataset
+ _, update_kwargs = mock_dataset_dao.update.call_args
+ assert update_kwargs["attributes"]["database"] is mock_new_database
+
+
@pytest.mark.parametrize(
("payload, exception, error_msg"),
[
diff --git a/tests/unit_tests/views/datasource/views_test.py b/tests/unit_tests/views/datasource/views_test.py
index 69cd79fcc41..51d021c56d1 100644
--- a/tests/unit_tests/views/datasource/views_test.py
+++ b/tests/unit_tests/views/datasource/views_test.py
@@ -313,30 +313,41 @@ def test_save_non_editor_with_editors_field_is_rejected(
@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_repoints_database_id_without_checking_new_database_access(
+def test_save_rejects_repoint_to_database_without_access(
mock_get_datasource: MagicMock,
mock_security_manager: MagicMock,
+ mock_get_database_by_id: MagicMock,
mock_db: MagicMock,
) -> None:
"""
``save`` lets a dataset editor supply a different ``database.id`` in the
- request body. The ORM object's ``database_id`` is reassigned to the
- caller-supplied value before (and independent of) the only permission
- check performed, ``raise_for_editorship``, which verifies ownership of
- the *dataset* being edited but never checks whether the caller has
- access to the *new* database being pointed at.
+ request body. Ownership of the *dataset* being edited
+ (``raise_for_editorship``) is not sufficient to repoint it to a new
+ 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_get_datasource.return_value = mock_orm
- # Caller owns the dataset, so the only check performed here passes.
+ # 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.
+ 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(
@@ -355,14 +366,73 @@ def test_save_repoints_database_id_without_checking_new_database_access(
)
},
):
- raw_save(_view_self())
+ with pytest.raises(DatasetForbiddenError):
+ raw_save(_view_self())
- # The ORM object was repointed to the caller-supplied database id.
- assert mock_orm.database_id == 999
# Ownership of the dataset was checked...
mock_security_manager.raise_for_editorship.assert_called_once_with(mock_orm)
- # ...but nothing checked whether the caller may access database 999.
- mock_security_manager.raise_for_access.assert_not_called()
+ # ...and access to the new database was checked too.
+ 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"
+ # The ORM object was NOT repointed since access was denied.
+ 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")
+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,
+) -> None:
+ """
+ When the caller is authorised for the new database, ``save`` proceeds
+ to repoint ``database_id``.
+ """
+ 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_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())
+
+ 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 mock_orm.database_id == 999
# ---------------------------------------------------------------------------