diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index f905f8cc3ad4f..2aac3ea9c4882 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/superset/commands/chart/importers/v1/utils.py @@ -20,7 +20,6 @@ from inspect import isclass from typing import Any -from flask import g from sqlalchemy.orm import Session from superset import security_manager @@ -28,7 +27,7 @@ from superset.migrations.shared.migrate_viz import processors from superset.migrations.shared.migrate_viz.base import MigrateViz from superset.models.slice import Slice -from superset.utils.core import AnnotationType +from superset.utils.core import AnnotationType, get_user def filter_chart_annotations(chart_config: dict[str, Any]) -> None: @@ -55,6 +54,12 @@ def import_chart( can_write = ignore_permissions or security_manager.can_access("can_write", "Chart") existing = session.query(Slice).filter_by(uuid=config["uuid"]).first() if existing: + if overwrite and can_write and get_user(): + if not security_manager.can_access_chart(existing): + raise ImportFailedError( + "A chart already exists and user doesn't " + "have permissions to overwrite it" + ) if not overwrite or not can_write: return existing config["id"] = existing.id @@ -77,8 +82,8 @@ def import_chart( if chart.id is None: session.flush() - if hasattr(g, "user") and g.user: - chart.owners.append(g.user) + if user := get_user(): + chart.owners.append(user) return chart diff --git a/superset/commands/dashboard/importers/v1/utils.py b/superset/commands/dashboard/importers/v1/utils.py index 712161bc16481..b8ac3144dba50 100644 --- a/superset/commands/dashboard/importers/v1/utils.py +++ b/superset/commands/dashboard/importers/v1/utils.py @@ -19,12 +19,12 @@ import logging from typing import Any -from flask import g from sqlalchemy.orm import Session from superset import security_manager from superset.commands.exceptions import ImportFailedError from superset.models.dashboard import Dashboard +from superset.utils.core import get_user logger = logging.getLogger(__name__) @@ -157,7 +157,7 @@ def import_dashboard( ) existing = session.query(Dashboard).filter_by(uuid=config["uuid"]).first() if existing: - if overwrite and can_write and hasattr(g, "user") and g.user: + if overwrite and can_write and get_user(): if not security_manager.can_access_dashboard(existing): raise ImportFailedError( "A dashboard already exists and user doesn't " @@ -191,7 +191,7 @@ def import_dashboard( if dashboard.id is None: session.flush() - if hasattr(g, "user") and g.user: - dashboard.owners.append(g.user) + if user := get_user(): + dashboard.owners.append(user) return dashboard diff --git a/superset/commands/dataset/importers/v1/utils.py b/superset/commands/dataset/importers/v1/utils.py index 5a9ac54714e02..014a864da4be3 100644 --- a/superset/commands/dataset/importers/v1/utils.py +++ b/superset/commands/dataset/importers/v1/utils.py @@ -22,7 +22,7 @@ from urllib import request import pandas as pd -from flask import current_app, g +from flask import current_app from sqlalchemy import BigInteger, Boolean, Date, DateTime, Float, String, Text from sqlalchemy.exc import MultipleResultsFound from sqlalchemy.orm import Session @@ -33,6 +33,7 @@ from superset.commands.exceptions import ImportFailedError from superset.connectors.sqla.models import SqlaTable from superset.models.core import Database +from superset.utils.core import get_user logger = logging.getLogger(__name__) @@ -176,8 +177,8 @@ def import_dataset( if data_uri and (not table_exists or force_data): load_data(data_uri, dataset, dataset.database, session) - if hasattr(g, "user") and g.user: - dataset.owners.append(g.user) + if user := get_user(): + dataset.owners.append(user) return dataset diff --git a/superset/errors.py b/superset/errors.py index 87cdf77b9944b..6be4f966ce403 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -66,6 +66,7 @@ class SupersetErrorType(StrEnum): MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR" USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR" DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR" + CHART_SECURITY_ACCESS_ERROR = "CHART_SECURITY_ACCESS_ERROR" # Other errors BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR" diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 3b046b732e30b..54d1f54866d24 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -36,7 +36,7 @@ from superset.extensions import feature_flag_manager from superset.utils.core import ( convert_legacy_filters_into_adhoc, - get_user_id, + get_user, merge_extra_filters, ) @@ -110,11 +110,10 @@ def current_user_id(self, add_to_cache_keys: bool = True) -> Optional[int]: :returns: The user ID """ - if hasattr(g, "user") and g.user: - id_ = get_user_id() + if user := get_user(): if add_to_cache_keys: - self.cache_key_wrapper(id_) - return id_ + self.cache_key_wrapper(user.id) + return user.id return None def current_username(self, add_to_cache_keys: bool = True) -> Optional[str]: diff --git a/superset/security/manager.py b/superset/security/manager.py index 8268b19411b9c..72d3be27d33cf 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -85,6 +85,7 @@ ) from superset.models.core import Database from superset.models.dashboard import Dashboard + from superset.models.slice import Slice from superset.models.sql_lab import Query from superset.sql_parse import Table from superset.viz import BaseViz @@ -422,6 +423,19 @@ def can_access_dashboard(self, dashboard: "Dashboard") -> bool: return True + def can_access_chart(self, chart: "Slice") -> bool: + """ + Return True if the user can access the specified chart, False otherwise. + :param chart: The chart + :return: Whether the user can access the chart + """ + try: + self.raise_for_access(chart=chart) + except SupersetSecurityException: + return False + + return True + def get_dashboard_access_error_object( # pylint: disable=invalid-name self, dashboard: "Dashboard", # pylint: disable=unused-argument @@ -439,6 +453,23 @@ def get_dashboard_access_error_object( # pylint: disable=invalid-name level=ErrorLevel.ERROR, ) + def get_chart_access_error_object( # pylint: disable=invalid-name + self, + dashboard: "Dashboard", # pylint: disable=unused-argument + ) -> SupersetError: + """ + Return the error object for the denied Superset dashboard. + + :param dashboard: The denied Superset dashboard + :returns: The error object + """ + + return SupersetError( + error_type=SupersetErrorType.CHART_SECURITY_ACCESS_ERROR, + message="You don't have access to this chart.", + level=ErrorLevel.ERROR, + ) + @staticmethod def get_datasource_access_error_msg(datasource: "BaseDatasource") -> str: """ @@ -1822,6 +1853,7 @@ def raise_for_access( # pylint: disable=too-many-arguments,too-many-branches,too-many-locals,too-many-statements self, dashboard: Optional["Dashboard"] = None, + chart: Optional["Slice"] = None, database: Optional["Database"] = None, datasource: Optional["BaseDatasource"] = None, query: Optional["Query"] = None, @@ -2044,6 +2076,15 @@ def raise_for_access( self.get_dashboard_access_error_object(dashboard) ) + if chart: + if self.is_admin() or self.is_owner(chart): + return + + if chart.datasource and self.can_access_datasource(chart.datasource): + return + + raise SupersetSecurityException(self.get_chart_access_error_object(chart)) + def get_user_by_username(self, username: str) -> Optional[User]: """ Retrieves a user by it's username case sensitive. Optional session parameter diff --git a/superset/utils/core.py b/superset/utils/core.py index 3a761cad1fc73..df12aafe07d95 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -1344,6 +1344,15 @@ def split_adhoc_filters_into_base_filters( # pylint: disable=invalid-name form_data["filters"] = simple_where_filters +def get_user() -> User | None: + """ + Get the current user (if defined). + + :returns: The current user + """ + return g.user if hasattr(g, "user") else None + + def get_username() -> str | None: """ Get username (if defined) associated with the current user. diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index b6adf197f5751..a72a716d1767c 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -171,7 +171,7 @@ def test_export_chart_command_no_related(self, mock_g): class TestImportChartsCommand(SupersetTestCase): - @patch("superset.commands.chart.importers.v1.utils.g") + @patch("superset.utils.core.g") @patch("superset.security.manager.g") def test_import_v1_chart(self, sm_g, utils_g): """Test that we can import a chart""" diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py index 6e9beab249248..c1b65ee74e615 100644 --- a/tests/integration_tests/dashboards/commands_tests.py +++ b/tests/integration_tests/dashboards/commands_tests.py @@ -486,7 +486,7 @@ def test_import_v0_dashboard_cli_export(self): db.session.delete(dataset) db.session.commit() - @patch("superset.commands.dashboard.importers.v1.utils.g") + @patch("superset.utils.core.g") @patch("superset.security.manager.g") def test_import_v1_dashboard(self, sm_g, utils_g): """Test that we can import a dashboard""" diff --git a/tests/integration_tests/datasets/commands_tests.py b/tests/integration_tests/datasets/commands_tests.py index b45bbdb76d5eb..7b6066a22af67 100644 --- a/tests/integration_tests/datasets/commands_tests.py +++ b/tests/integration_tests/datasets/commands_tests.py @@ -339,7 +339,7 @@ def test_import_v0_dataset_ui_export(self): db.session.delete(dataset) db.session.commit() - @patch("superset.commands.dataset.importers.v1.utils.g") + @patch("superset.utils.core.g") @patch("superset.security.manager.g") @pytest.mark.usefixtures("load_energy_table_with_slice") def test_import_v1_dataset(self, sm_g, utils_g): diff --git a/tests/unit_tests/charts/commands/importers/v1/import_test.py b/tests/unit_tests/charts/commands/importers/v1/import_test.py index 903b7468baf21..bcff3ee411ada 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py @@ -17,104 +17,130 @@ # pylint: disable=unused-argument, import-outside-toplevel, unused-import, invalid-name import copy +from collections.abc import Generator import pytest +from flask_appbuilder.security.sqla.models import Role, User from pytest_mock import MockFixture from sqlalchemy.orm.session import Session +from superset import security_manager +from superset.commands.chart.importers.v1.utils import import_chart from superset.commands.exceptions import ImportFailedError +from superset.connectors.sqla.models import Database, SqlaTable +from superset.models.slice import Slice +from superset.utils.core import override_user +from tests.integration_tests.fixtures.importexport import chart_config -def test_import_chart(mocker: MockFixture, session: Session) -> None: +@pytest.fixture +def session_with_data(session: Session) -> Generator[Session, None, None]: + engine = session.get_bind() + SqlaTable.metadata.create_all(engine) # pylint: disable=no-member + + dataset = SqlaTable( + table_name="test_table", + metrics=[], + main_dttm_col=None, + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + ) + session.add(dataset) + session.flush() + slice = Slice( + id=1, + datasource_id=dataset.id, + datasource_type="table", + datasource_name="tmp_perm_table", + slice_name="slice_name", + uuid=chart_config["uuid"], + ) + session.add(slice) + session.flush() + + yield session + session.rollback() + + +@pytest.fixture +def session_with_schema(session: Session) -> Generator[Session, None, None]: + from superset.connectors.sqla.models import SqlaTable + + engine = session.get_bind() + SqlaTable.metadata.create_all(engine) # pylint: disable=no-member + + yield session + + +def test_import_chart(mocker: MockFixture, session_with_schema: Session) -> None: """ Test importing a chart. """ - from superset import security_manager - from superset.commands.chart.importers.v1.utils import import_chart - from superset.connectors.sqla.models import SqlaTable - from superset.models.core import Database - from superset.models.slice import Slice - from tests.integration_tests.fixtures.importexport import chart_config mocker.patch.object(security_manager, "can_access", return_value=True) - engine = session.get_bind() - Slice.metadata.create_all(engine) # pylint: disable=no-member - config = copy.deepcopy(chart_config) config["datasource_id"] = 1 config["datasource_type"] = "table" - chart = import_chart(session, config) + chart = import_chart(session_with_schema, config) assert chart.slice_name == "Deck Path" assert chart.viz_type == "deck_path" assert chart.is_managed_externally is False assert chart.external_url is None + # Assert that the can write to chart was checked + security_manager.can_access.assert_called_once_with("can_write", "Chart") -def test_import_chart_managed_externally(mocker: MockFixture, session: Session) -> None: + +def test_import_chart_managed_externally( + mocker: MockFixture, session_with_schema: Session +) -> None: """ Test importing a chart that is managed externally. """ - from superset import security_manager - from superset.commands.chart.importers.v1.utils import import_chart - from superset.connectors.sqla.models import SqlaTable - from superset.models.core import Database - from superset.models.slice import Slice - from tests.integration_tests.fixtures.importexport import chart_config - mocker.patch.object(security_manager, "can_access", return_value=True) - engine = session.get_bind() - Slice.metadata.create_all(engine) # pylint: disable=no-member - config = copy.deepcopy(chart_config) config["datasource_id"] = 1 config["datasource_type"] = "table" config["is_managed_externally"] = True config["external_url"] = "https://example.org/my_chart" - chart = import_chart(session, config) + chart = import_chart(session_with_schema, config) assert chart.is_managed_externally is True assert chart.external_url == "https://example.org/my_chart" + # Assert that the can write to chart was checked + security_manager.can_access.assert_called_once_with("can_write", "Chart") + def test_import_chart_without_permission( mocker: MockFixture, - session: Session, + session_with_schema: Session, ) -> None: """ Test importing a chart when a user doesn't have permissions to create. """ - from superset import security_manager - from superset.commands.chart.importers.v1.utils import import_chart - from superset.connectors.sqla.models import SqlaTable - from superset.models.core import Database - from superset.models.slice import Slice - from tests.integration_tests.fixtures.importexport import chart_config - mocker.patch.object(security_manager, "can_access", return_value=False) - engine = session.get_bind() - Slice.metadata.create_all(engine) # pylint: disable=no-member - config = copy.deepcopy(chart_config) config["datasource_id"] = 1 config["datasource_type"] = "table" with pytest.raises(ImportFailedError) as excinfo: - import_chart(session, config) + import_chart(session_with_schema, config) assert ( str(excinfo.value) == "Chart doesn't exist and user doesn't have permission to create charts" ) + # Assert that the can write to chart was checked + security_manager.can_access.assert_called_once_with("can_write", "Chart") -def test_filter_chart_annotations(mocker: MockFixture, session: Session) -> None: +def test_filter_chart_annotations(session: Session) -> None: """ Test importing a chart. """ - from superset import security_manager from superset.commands.chart.importers.v1.utils import filter_chart_annotations from tests.integration_tests.fixtures.importexport import ( chart_config_with_mixed_annotations, @@ -127,3 +153,67 @@ def test_filter_chart_annotations(mocker: MockFixture, session: Session) -> None assert len(annotation_layers) == 1 assert all([al["annotationType"] == "FORMULA" for al in annotation_layers]) + + +def test_import_existing_chart_without_permission( + mocker: MockFixture, + session_with_data: Session, +) -> None: + """ + Test importing a chart when a user doesn't have permissions to modify. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + mocker.patch.object(security_manager, "can_access_chart", return_value=False) + + slice = ( + session_with_data.query(Slice) + .filter(Slice.uuid == chart_config["uuid"]) + .one_or_none() + ) + + with override_user("admin"): + with pytest.raises(ImportFailedError) as excinfo: + import_chart(session_with_data, chart_config, overwrite=True) + assert ( + str(excinfo.value) + == "A chart already exists and user doesn't have permissions to overwrite it" + ) + + # Assert that the can write to chart was checked + security_manager.can_access.assert_called_once_with("can_write", "Chart") + security_manager.can_access_chart.assert_called_once_with(slice) + + +def test_import_existing_chart_with_permission( + mocker: MockFixture, + session_with_data: Session, +) -> None: + """ + Test importing a chart that exists when a user has access permission to that chart. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + mocker.patch.object(security_manager, "can_access_chart", return_value=True) + + admin = User( + first_name="Alice", + last_name="Doe", + email="adoe@example.org", + username="admin", + roles=[Role(name="Admin")], + ) + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + + slice = ( + session_with_data.query(Slice) + .filter(Slice.uuid == config["uuid"]) + .one_or_none() + ) + + with override_user(admin): + import_chart(session_with_data, config, overwrite=True) + # Assert that the can write to chart was checked + security_manager.can_access.assert_called_once_with("can_write", "Chart") + security_manager.can_access_chart.assert_called_once_with(slice) diff --git a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py index 190f0660f5c49..afbce49cd96b1 100644 --- a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py +++ b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py @@ -17,32 +17,57 @@ # pylint: disable=unused-argument, import-outside-toplevel, unused-import, invalid-name import copy +from collections.abc import Generator import pytest +from flask_appbuilder.security.sqla.models import Role, User from pytest_mock import MockFixture from sqlalchemy.orm.session import Session +from superset import security_manager +from superset.commands.dashboard.importers.v1.utils import import_dashboard from superset.commands.exceptions import ImportFailedError +from superset.models.dashboard import Dashboard from superset.utils.core import override_user +from tests.integration_tests.fixtures.importexport import dashboard_config -def test_import_dashboard(mocker: MockFixture, session: Session) -> None: - """ - Test importing a dashboard. - """ - from superset import security_manager - from superset.commands.dashboard.importers.v1.utils import import_dashboard - from superset.models.slice import Slice - from tests.integration_tests.fixtures.importexport import dashboard_config +@pytest.fixture +def session_with_data(session: Session) -> Generator[Session, None, None]: + engine = session.get_bind() + Dashboard.metadata.create_all(engine) # pylint: disable=no-member + + dashboard = Dashboard( + id=100, + dashboard_title="Test dash", + slug=None, + slices=[], + published=True, + uuid=dashboard_config["uuid"], + ) + + session.add(dashboard) + session.flush() + yield session + session.rollback() - mocker.patch.object(security_manager, "can_access", return_value=True) +@pytest.fixture +def session_with_schema(session: Session) -> Generator[Session, None, None]: engine = session.get_bind() - Slice.metadata.create_all(engine) # pylint: disable=no-member + Dashboard.metadata.create_all(engine) # pylint: disable=no-member - config = copy.deepcopy(dashboard_config) + yield session + session.rollback() + + +def test_import_dashboard(mocker: MockFixture, session_with_schema: Session) -> None: + """ + Test importing a dashboard. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) - dashboard = import_dashboard(session, config) + dashboard = import_dashboard(session_with_schema, dashboard_config) assert dashboard.dashboard_title == "Test dash" assert dashboard.description is None assert dashboard.is_managed_externally is False @@ -53,26 +78,18 @@ def test_import_dashboard(mocker: MockFixture, session: Session) -> None: def test_import_dashboard_managed_externally( mocker: MockFixture, - session: Session, + session_with_schema: Session, ) -> None: """ Test importing a dashboard that is managed externally. """ - from superset import security_manager - from superset.commands.dashboard.importers.v1.utils import import_dashboard - from superset.models.slice import Slice - from tests.integration_tests.fixtures.importexport import dashboard_config - mocker.patch.object(security_manager, "can_access", return_value=True) - engine = session.get_bind() - Slice.metadata.create_all(engine) # pylint: disable=no-member - config = copy.deepcopy(dashboard_config) config["is_managed_externally"] = True config["external_url"] = "https://example.org/my_dashboard" - dashboard = import_dashboard(session, config) + dashboard = import_dashboard(session_with_schema, config) assert dashboard.is_managed_externally is True assert dashboard.external_url == "https://example.org/my_dashboard" @@ -82,25 +99,15 @@ def test_import_dashboard_managed_externally( def test_import_dashboard_without_permission( mocker: MockFixture, - session: Session, + session_with_schema: Session, ) -> None: """ Test importing a dashboard when a user doesn't have permissions to create. """ - from superset import security_manager - from superset.commands.dashboard.importers.v1.utils import import_dashboard - from superset.models.slice import Slice - from tests.integration_tests.fixtures.importexport import dashboard_config - mocker.patch.object(security_manager, "can_access", return_value=False) - engine = session.get_bind() - Slice.metadata.create_all(engine) # pylint: disable=no-member - - config = copy.deepcopy(dashboard_config) - with pytest.raises(ImportFailedError) as excinfo: - import_dashboard(session, config) + import_dashboard(session_with_schema, dashboard_config) assert ( str(excinfo.value) == "Dashboard doesn't exist and user doesn't have permission to create dashboards" @@ -112,72 +119,43 @@ def test_import_dashboard_without_permission( def test_import_existing_dashboard_without_permission( mocker: MockFixture, - session: Session, + session_with_data: Session, ) -> None: """ Test importing a dashboard when a user doesn't have permissions to create. """ - from superset import security_manager - from superset.commands.dashboard.importers.v1.utils import g, import_dashboard - from superset.models.dashboard import Dashboard - from superset.models.slice import Slice - from tests.integration_tests.fixtures.importexport import dashboard_config - mocker.patch.object(security_manager, "can_access", return_value=True) mocker.patch.object(security_manager, "can_access_dashboard", return_value=False) - mock_g = mocker.patch( - "superset.commands.dashboard.importers.v1.utils.g" - ) # Replace with the actual path to g - mock_g.user = mocker.MagicMock(return_value=True) - - engine = session.get_bind() - Slice.metadata.create_all(engine) # pylint: disable=no-member - Dashboard.metadata.create_all(engine) # pylint: disable=no-member - dashboard_obj = Dashboard( - id=100, - dashboard_title="Test dash", - slug=None, - slices=[], - published=True, - uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51", + dashboard = ( + session_with_data.query(Dashboard) + .filter(Dashboard.uuid == dashboard_config["uuid"]) + .one_or_none() ) - session.add(dashboard_obj) - session.flush() - config = copy.deepcopy(dashboard_config) - with pytest.raises(ImportFailedError) as excinfo: - import_dashboard(session, config, overwrite=True) - assert ( - str(excinfo.value) - == "A dashboard already exists and user doesn't have permissions to overwrite it" - ) + with override_user("admin"): + with pytest.raises(ImportFailedError) as excinfo: + import_dashboard(session_with_data, dashboard_config, overwrite=True) + assert ( + str(excinfo.value) + == "A dashboard already exists and user doesn't have permissions to overwrite it" + ) # Assert that the can write to dashboard was checked security_manager.can_access.assert_called_once_with("can_write", "Dashboard") - security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj) + security_manager.can_access_dashboard.assert_called_once_with(dashboard) def test_import_existing_dashboard_with_permission( mocker: MockFixture, - session: Session, + session_with_data: Session, ) -> None: """ - Test importing a dashboard when a user doesn't have permissions to create. + Test importing a dashboard that exists when a user has access permission to that dashboard. """ - from flask_appbuilder.security.sqla.models import Role, User - - from superset import security_manager - from superset.commands.dashboard.importers.v1.utils import import_dashboard - from superset.models.dashboard import Dashboard - from tests.integration_tests.fixtures.importexport import dashboard_config - mocker.patch.object(security_manager, "can_access", return_value=True) mocker.patch.object(security_manager, "can_access_dashboard", return_value=True) - engine = session.get_bind() - Dashboard.metadata.create_all(engine) # pylint: disable=no-member - admin = User( first_name="Alice", last_name="Doe", @@ -186,20 +164,14 @@ def test_import_existing_dashboard_with_permission( roles=[Role(name="Admin")], ) - dashboard_obj = Dashboard( - id=100, - dashboard_title="Test dash", - slug=None, - slices=[], - published=True, - uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51", + dashboard = ( + session_with_data.query(Dashboard) + .filter(Dashboard.uuid == dashboard_config["uuid"]) + .one_or_none() ) - session.add(dashboard_obj) - session.flush() - config = copy.deepcopy(dashboard_config) with override_user(admin): - import_dashboard(session, config, overwrite=True) + import_dashboard(session_with_data, dashboard_config, overwrite=True) # Assert that the can write to dashboard was checked security_manager.can_access.assert_called_once_with("can_write", "Dashboard") - security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj) + security_manager.can_access_dashboard.assert_called_once_with(dashboard) diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index ad6e53e9930ce..7d2b9153a392d 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -16,12 +16,16 @@ # under the License. import pytest +from flask_appbuilder.security.sqla.models import Role, User from pytest_mock import MockFixture from superset.common.query_object import QueryObject +from superset.connectors.sqla.models import Database, SqlaTable from superset.exceptions import SupersetSecurityException from superset.extensions import appbuilder +from superset.models.slice import Slice from superset.security.manager import SupersetSecurityManager +from superset.utils.core import override_user def test_security_manager(app_context: None) -> None: @@ -164,3 +168,139 @@ def test_raise_for_access_query_default_schema( == """You need access to the following tables: `public.ab_user`, `all_database_access` or `all_datasource_access` permission""" ) + + +def test_raise_for_access_chart_for_datasource_permission( + mocker: MockFixture, + app_context: None, +) -> None: + """ + Test that the security manager can raise an exception for chart access, + when the user does not have access to the chart datasource + """ + sm = SupersetSecurityManager(appbuilder) + session = sm.get_session + + engine = session.get_bind() + Slice.metadata.create_all(engine) # pylint: disable=no-member + + alpha = User( + first_name="Alice", + last_name="Doe", + email="adoe@example.org", + username="admin", + roles=[Role(name="Alpha")], + ) + + dataset = SqlaTable( + table_name="test_table", + metrics=[], + main_dttm_col=None, + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + ) + session.add(dataset) + session.flush() + + slice = Slice( + id=1, + datasource_id=dataset.id, + datasource_type="table", + datasource_name="tmp_perm_table", + slice_name="slice_name", + ) + session.add(slice) + session.flush() + + mocker.patch.object(sm, "can_access_datasource", return_value=False) + with override_user(alpha): + with pytest.raises(SupersetSecurityException) as excinfo: + sm.raise_for_access( + chart=slice, + ) + assert str(excinfo.value) == "You don't have access to this chart." + + mocker.patch.object(sm, "can_access_datasource", return_value=True) + with override_user(alpha): + sm.raise_for_access( + chart=slice, + ) + + +def test_raise_for_access_chart_on_admin( + app_context: None, +) -> None: + """ + Test that the security manager can raise an exception for chart access, + when the user does not have access to the chart datasource + """ + from flask_appbuilder.security.sqla.models import Role, User + + from superset.models.slice import Slice + from superset.utils.core import override_user + + sm = SupersetSecurityManager(appbuilder) + session = sm.get_session + + engine = session.get_bind() + Slice.metadata.create_all(engine) # pylint: disable=no-member + + admin = User( + first_name="Alice", + last_name="Doe", + email="adoe@example.org", + username="admin", + roles=[Role(name="Admin")], + ) + + slice = Slice( + id=1, + datasource_id=1, + datasource_type="table", + datasource_name="tmp_perm_table", + slice_name="slice_name", + ) + session.add(slice) + session.flush() + + with override_user(admin): + sm.raise_for_access( + chart=slice, + ) + + +def test_raise_for_access_chart_owner( + app_context: None, +) -> None: + """ + Test that the security manager can raise an exception for chart access, + when the user does not have access to the chart datasource + """ + sm = SupersetSecurityManager(appbuilder) + session = sm.get_session + + engine = session.get_bind() + Slice.metadata.create_all(engine) # pylint: disable=no-member + + alpha = User( + first_name="Alice", + last_name="Doe", + email="adoe@example.org", + username="admin", + roles=[Role(name="Alpha")], + ) + + slice = Slice( + id=1, + datasource_id=1, + datasource_type="table", + datasource_name="tmp_perm_table", + slice_name="slice_name", + owners=[alpha], + ) + session.add(slice) + session.flush() + + with override_user(alpha): + sm.raise_for_access( + chart=slice, + )