You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2023/08/30 12:41:06 UTC
[superset] 07/11: fix: Allow embedded guest user datasource access with dashboard context (#25081)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6a461260fc45be524148f628b19a11eb274ef9d0
Author: Jack Fragassi <jf...@gmail.com>
AuthorDate: Mon Aug 28 09:48:21 2023 -0700
fix: Allow embedded guest user datasource access with dashboard context (#25081)
(cherry picked from commit 2b8d8da22acc6ffbd49ca256b08aa2fe60e0d718)
---
superset/security/manager.py | 10 +-
superset/viz.py | 1 +
.../fixtures/birth_names_dashboard.py | 8 +
.../fixtures/world_bank_dashboard.py | 8 +
.../security/guest_token_security_tests.py | 258 ++++++++++++++++++++-
tests/integration_tests/security_tests.py | 2 +-
6 files changed, 277 insertions(+), 10 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index b1cb15aadf..6c47c6e163 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1886,17 +1886,23 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
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 3051f104e2..4c240efccc 100644
--- a/superset/viz.py
+++ b/superset/viz.py
@@ -1654,6 +1654,7 @@ class FilterBoxViz(BaseViz):
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 d9a4a5d9e0..513a9f84a2 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 18ceba9af2..a53cd76aa9 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 5f50bf4b50..4cd8a77f76 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 @@ class TestGuestUserSecurity(SupersetTestCase):
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 @@ class TestGuestUserSecurity(SupersetTestCase):
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 @@ class TestGuestUserDashboardAccess(SupersetTestCase):
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 90be0edd17..9eaabf3680 100644
--- a/tests/integration_tests/security_tests.py
+++ b/tests/integration_tests/security_tests.py
@@ -1649,7 +1649,7 @@ class TestSecurityManager(SupersetTestCase):
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)