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/21 13:47:22 UTC
[superset] 02/05: fix: Address regression introduced in #24789 (#25008)
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 fad872fffb5f5c3a6f1249f5c8fda6c230f74f52
Author: John Bodley <45...@users.noreply.github.com>
AuthorDate: Fri Aug 18 09:27:34 2023 -0700
fix: Address regression introduced in #24789 (#25008)
(cherry picked from commit 3f93755be27f1804bb6a08029f6115b8818467cf)
---
superset/security/manager.py | 24 +++++++-
tests/integration_tests/security_tests.py | 92 ++++++++++++++++++++++++++++---
2 files changed, 104 insertions(+), 12 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 38bef7f907..a32ef9a1b9 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1801,7 +1801,8 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
# pylint: disable=import-outside-toplevel
from superset import is_feature_enabled
from superset.connectors.sqla.models import SqlaTable
- from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
from superset.sql_parse import Table
if database and table or query:
@@ -1863,10 +1864,27 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
or self.can_access("datasource_access", datasource.perm or "")
or self.is_owner(datasource)
or (
+ # Grant access to the datasource only if dashboard RBAC is enabled
+ # 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 := DashboardDAO.find_by_id(dashboard_id))
- and self.can_access_dashboard(dashboard)
+ and (
+ dashboard_ := self.get_session.query(Dashboard)
+ .filter(Dashboard.id == dashboard_id)
+ .one_or_none()
+ )
+ and dashboard_.roles
+ and (slice_id := form_data.get("slice_id"))
+ and (
+ slc := self.get_session.query(Slice)
+ .filter(Slice.id == slice_id)
+ .one_or_none()
+ )
+ and slc in dashboard_.slices
+ and slc.datasource == datasource
+ and self.can_access_dashboard(dashboard_)
)
):
raise SupersetSecurityException(
diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py
index 139ad26342..4767e5af0e 100644
--- a/tests/integration_tests/security_tests.py
+++ b/tests/integration_tests/security_tests.py
@@ -1698,6 +1698,7 @@ class TestSecurityManager(SupersetTestCase):
security_manager.raise_for_access(viz=test_viz)
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
@with_feature_flags(DASHBOARD_RBAC=True)
@patch("superset.security.manager.g")
@patch("superset.security.SupersetSecurityManager.is_owner")
@@ -1710,12 +1711,12 @@ class TestSecurityManager(SupersetTestCase):
mock_is_owner,
mock_g,
):
- dashboard = self.get_dash_by_slug("births")
+ births = self.get_dash_by_slug("births")
+ girls = self.get_slice("Girls", db.session, expunge_from_session=False)
+ birth_names = girls.datasource
- obj = Mock(
- datasource=self.get_datasource_mock(),
- form_data={"dashboardId": dashboard.id},
- )
+ world_health = self.get_dash_by_slug("world_health")
+ treemap = self.get_slice("Treemap", db.session, expunge_from_session=False)
mock_g.user = security_manager.find_user("gamma")
mock_is_owner.return_value = False
@@ -1723,15 +1724,88 @@ class TestSecurityManager(SupersetTestCase):
mock_can_access_schema.return_value = False
for kwarg in ["query_context", "viz"]:
- dashboard.roles = []
+ births.roles = []
db.session.flush()
+ # No dashboard roles.
with self.assertRaises(SupersetSecurityException):
- security_manager.raise_for_access(**{kwarg: obj})
+ security_manager.raise_for_access(
+ **{
+ kwarg: Mock(
+ datasource=birth_names,
+ form_data={
+ "dashboardId": births.id,
+ "slice_id": girls.id,
+ },
+ )
+ }
+ )
- dashboard.roles = [self.get_role("Gamma")]
+ births.roles = [self.get_role("Gamma")]
db.session.flush()
- security_manager.raise_for_access(**{kwarg: obj})
+
+ # Undefined dashboard.
+ with self.assertRaises(SupersetSecurityException):
+ security_manager.raise_for_access(
+ **{
+ kwarg: Mock(
+ datasource=birth_names,
+ form_data={},
+ )
+ }
+ )
+
+ # Undefined dashboard chart.
+ with self.assertRaises(SupersetSecurityException):
+ security_manager.raise_for_access(
+ **{
+ kwarg: Mock(
+ datasource=birth_names,
+ form_data={"dashboardId": births.id},
+ )
+ }
+ )
+
+ # Ill-defined dashboard chart.
+ with self.assertRaises(SupersetSecurityException):
+ security_manager.raise_for_access(
+ **{
+ kwarg: Mock(
+ datasource=birth_names,
+ form_data={
+ "dashboardId": births.id,
+ "slice_id": treemap.id,
+ },
+ )
+ }
+ )
+
+ # Dashboard chart not associated with said datasource.
+ with self.assertRaises(SupersetSecurityException):
+ security_manager.raise_for_access(
+ **{
+ kwarg: Mock(
+ datasource=birth_names,
+ form_data={
+ "dashboardId": world_health.id,
+ "slice_id": treemap.id,
+ },
+ )
+ }
+ )
+
+ # Dashboard chart associated with said datasource.
+ security_manager.raise_for_access(
+ **{
+ kwarg: Mock(
+ datasource=birth_names,
+ form_data={
+ "dashboardId": births.id,
+ "slice_id": girls.id,
+ },
+ )
+ }
+ )
db.session.rollback()