diff --git a/superset/security/manager.py b/superset/security/manager.py index 128d0dda8c132..13e96c33346b3 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1862,17 +1862,23 @@ def raise_for_access( or self.is_owner(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 # and said datasource is associated with the dashboard chart in # question. form_data - and is_feature_enabled("DASHBOARD_RBAC") and (dashboard_id := form_data.get("dashboardId")) and ( dashboard_ := self.get_session.query(Dashboard) .filter(Dashboard.id == dashboard_id) .one_or_none() ) - and dashboard_.roles + and ( + (is_feature_enabled("DASHBOARD_RBAC") and dashboard_.roles) + or ( + is_feature_enabled("EMBEDDED_SUPERSET") + and self.is_guest_user() + ) + ) and ( ( # Native filter. diff --git a/superset/viz.py b/superset/viz.py index bc7e3296b5857..8b88907292922 100644 --- a/superset/viz.py +++ b/superset/viz.py @@ -1653,6 +1653,7 @@ def run_extra_queries(self) -> None: query_obj["orderby"] = [(metric, asc)] self.get_query_context_factory().create( datasource={"id": self.datasource.id, "type": self.datasource.type}, + form_data=self.form_data, queries=[query_obj], ).raise_for_access() df = self.get_df_payload(query_obj=query_obj).get("df") diff --git a/tests/integration_tests/fixtures/birth_names_dashboard.py b/tests/integration_tests/fixtures/birth_names_dashboard.py index d9a4a5d9e02d7..513a9f84a24e4 100644 --- a/tests/integration_tests/fixtures/birth_names_dashboard.py +++ b/tests/integration_tests/fixtures/birth_names_dashboard.py @@ -59,6 +59,14 @@ def load_birth_names_dashboard_with_slices_module_scope(load_birth_names_data): _cleanup(dash_id_to_delete, slices_ids_to_delete) +@pytest.fixture(scope="class") +def load_birth_names_dashboard_with_slices_class_scope(load_birth_names_data): + with app.app_context(): + dash_id_to_delete, slices_ids_to_delete = _create_dashboards() + yield + _cleanup(dash_id_to_delete, slices_ids_to_delete) + + def _create_dashboards(): table = _create_table( table_name=BIRTH_NAMES_TBL_NAME, diff --git a/tests/integration_tests/fixtures/world_bank_dashboard.py b/tests/integration_tests/fixtures/world_bank_dashboard.py index 18ceba9af20bb..a53cd76aa94fe 100644 --- a/tests/integration_tests/fixtures/world_bank_dashboard.py +++ b/tests/integration_tests/fixtures/world_bank_dashboard.py @@ -84,6 +84,14 @@ def load_world_bank_dashboard_with_slices_module_scope(load_world_bank_data): _cleanup(dash_id_to_delete, slices_ids_to_delete) +@pytest.fixture(scope="class") +def load_world_bank_dashboard_with_slices_class_scope(load_world_bank_data): + with app.app_context(): + dash_id_to_delete, slices_ids_to_delete = create_dashboard_for_loaded_data() + yield + _cleanup(dash_id_to_delete, slices_ids_to_delete) + + def create_dashboard_for_loaded_data(): with app.app_context(): table = create_table_metadata(WB_HEALTH_POPULATION, get_example_database()) diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 5f50bf4b50e11..4cd8a77f7635c 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -15,25 +15,34 @@ # specific language governing permissions and limitations # under the License. """Unit tests for Superset""" -from unittest import mock +import json +from unittest.mock import Mock, patch import pytest from flask import g from superset import db, security_manager +from superset.connectors.sqla.models import SqlaTable from superset.daos.dashboard import EmbeddedDashboardDAO from superset.exceptions import SupersetSecurityException from superset.models.dashboard import Dashboard from superset.security.guest_token import GuestTokenResourceType from superset.sql_parse import Table +from superset.utils.core import get_example_default_schema +from superset.utils.database import get_example_database from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( - load_birth_names_dashboard_with_slices, + load_birth_names_dashboard_with_slices_class_scope, load_birth_names_data, ) +from tests.integration_tests.fixtures.world_bank_dashboard import ( + load_world_bank_dashboard_with_slices, + load_world_bank_dashboard_with_slices_class_scope, + load_world_bank_data, +) -@mock.patch.dict( +@patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, ) @@ -55,7 +64,7 @@ def test_is_guest_user__guest_user(self): is_guest = security_manager.is_guest_user(self.authorized_guest()) self.assertTrue(is_guest) - @mock.patch.dict( + @patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=False, ) @@ -91,14 +100,14 @@ def test_get_guest_user_roles_implicit(self): self.assertEqual(guest.roles, roles) -@mock.patch.dict( +@patch.dict( "superset.extensions.feature_flag_manager._feature_flags", EMBEDDED_SUPERSET=True, ) -@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") +@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices_class_scope") class TestGuestUserDashboardAccess(SupersetTestCase): def setUp(self) -> None: - self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.dash = self.get_dash_by_slug("births") self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) self.authorized_guest = security_manager.get_guest_user_from_token( { @@ -195,3 +204,238 @@ def test_raise_for_access_dashboard_as_guest_no_rbac(self): db.session.delete(dash) db.session.commit() + + +@patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + EMBEDDED_SUPERSET=True, +) +@pytest.mark.usefixtures( + "create_dataset", + "load_birth_names_dashboard_with_slices_class_scope", + "load_world_bank_dashboard_with_slices_class_scope", +) +class TestGuestUserDatasourceAccess(SupersetTestCase): + """ + Guest users should only have access to datasources that are associated with a + dashboard they have access to, and only with that dashboard context present + """ + + @pytest.fixture(scope="class") + def create_dataset(self): + with self.create_app().app_context(): + dataset = SqlaTable( + table_name="dummy_sql_table", + database=get_example_database(), + schema=get_example_default_schema(), + sql="select 123 as intcol, 'abc' as strcol", + ) + session = db.session + session.add(dataset) + session.commit() + + yield dataset + + # rollback + session.delete(dataset) + session.commit() + + def setUp(self) -> None: + self.dash = self.get_dash_by_slug("births") + self.other_dash = self.get_dash_by_slug("world_health") + self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) + self.authorized_guest = security_manager.get_guest_user_from_token( + { + "user": {}, + "resources": [{"type": "dashboard", "id": str(self.embedded.uuid)}], + } + ) + self.unauthorized_guest = security_manager.get_guest_user_from_token( + { + "user": {}, + "resources": [ + {"type": "dashboard", "id": "06383667-3e02-4e5e-843f-44e9c5896b6c"} + ], + } + ) + self.chart = self.get_slice("Girls", db.session, expunge_from_session=False) + self.datasource = self.chart.datasource + self.other_chart = self.get_slice( + "Treemap", db.session, expunge_from_session=False + ) + self.other_datasource = self.other_chart.datasource + self.native_filter_datasource = ( + db.session.query(SqlaTable).filter_by(table_name="dummy_sql_table").first() + ) + self.dash.json_metadata = json.dumps( + { + "native_filter_configuration": [ + { + "id": "NATIVE_FILTER-ABCDEFGH", + "targets": [{"datasetId": self.native_filter_datasource.id}], + }, + ] + } + ) + + def test_raise_for_access__happy_path(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.datasource, + form_data={ + "dashboardId": self.dash.id, + "slice_id": self.chart.id, + }, + ) + } + ) + + def test_raise_for_access__native_filter_happy_path(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.native_filter_datasource, + form_data={ + "dashboardId": self.dash.id, + "native_filter_id": "NATIVE_FILTER-ABCDEFGH", + "type": "NATIVE_FILTER", + }, + ) + } + ) + + def test_raise_for_access__no_dashboard_in_form_data(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.datasource, + form_data={ + "slice_id": self.chart.id, + }, + ) + } + ) + + def test_raise_for_access__no_chart_in_form_data(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.datasource, + form_data={ + "dashboardId": self.dash.id, + }, + ) + } + ) + + def test_raise_for_access__chart_not_on_dashboard(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.other_datasource, + form_data={ + "dashboardId": self.dash.id, + "slice_id": self.other_chart.id, + }, + ) + } + ) + + def test_raise_for_access__chart_doesnt_belong_to_datasource(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.other_datasource, + form_data={ + "dashboardId": self.dash.id, + "slice_id": self.chart.id, + }, + ) + } + ) + + def test_raise_for_access__native_filter_no_id_in_form_data(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.native_filter_datasource, + form_data={ + "dashboardId": self.dash.id, + "type": "NATIVE_FILTER", + }, + ) + } + ) + + def test_raise_for_access__native_filter_datasource_not_associated(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.other_datasource, + form_data={ + "dashboardId": self.dash.id, + "native_filter_id": "NATIVE_FILTER-ABCDEFGH", + "type": "NATIVE_FILTER", + }, + ) + } + ) + + @patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + EMBEDDED_SUPERSET=False, + ) + def test_raise_for_access__embedded_feature_flag_off(self): + g.user = self.authorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.datasource, + form_data={ + "dashboardId": self.dash.id, + "slice_id": self.chart.id, + }, + ) + } + ) + + def test_raise_for_access__unauthorized_guest_user(self): + g.user = self.unauthorized_guest + for kwarg in ["viz", "query_context"]: + with self.assertRaises(SupersetSecurityException): + security_manager.raise_for_access( + **{ + kwarg: Mock( + datasource=self.datasource, + form_data={ + "dashboardId": self.dash.id, + "slice_id": self.chart.id, + }, + ) + } + ) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index f741ec43153e2..90f01642e6a82 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -1652,7 +1652,7 @@ def test_raise_for_access_query(self, mock_can_access, mock_is_owner): def test_raise_for_access_query_context( self, mock_can_access_schema, mock_can_access, mock_is_owner, mock_g ): - query_context = Mock(datasource=self.get_datasource_mock()) + query_context = Mock(datasource=self.get_datasource_mock(), form_data={}) mock_can_access_schema.return_value = True security_manager.raise_for_access(query_context=query_context)