feat(dashboard_rbac): provide data access based on dashboard access (#…

…13992)

* feat: provide data access based onb dashboard access

* chore: adjust code after CR comments

* fix: add brackets

* fix: type

* chore: add tests

* fix: pre-commit

* fix: pre-commit and lint

* fix: fix test

* fix: pre-commit

* fix: fix local pylint warnings

* revert: birth_names pylint  change bc it  affects tests

* Update superset/security/manager.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* Update superset/security/manager.py

* Update tests/utils_tests.py

* fix: after CR

* fix: after CR from ville

* chore: update roles description

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

superset-frontend/src/dashboard/components/PropertiesModal/index.jsx

@@ -399,7 +399,7 @@ class PropertiesModal extends React.PureComponent {
             />
             <p className="help-block">
               {t(
-                'Roles is a list which defines access to the dashboard. These roles are always applied in addition to restrictions on dataset level access. If no roles defined then the dashboard is available to all roles.',
+                'Roles is a list which defines access to the dashboard. Granting a role access to a dashboard will bypass dataset level checks. If no roles defined then the dashboard is available to all roles.',
               )}
             </p>
           </Col>

superset/security/manager.py

@@ -22,6 +22,7 @@
 
 from flask import current_app, g
 from flask_appbuilder import Model
+from flask_appbuilder.models.sqla.interface import SQLAInterface
 from flask_appbuilder.security.sqla.manager import SecurityManager
 from flask_appbuilder.security.sqla.models import (
     assoc_permissionview_role,
@@ -61,7 +62,6 @@
     from superset.sql_parse import Table
     from superset.viz import BaseViz
 
-
 logger = logging.getLogger(__name__)
 
 
@@ -925,7 +925,9 @@ def set_perm(  # pylint: disable=no-self-use,unused-argument
                     )
                 )
 
-    def raise_for_access(  # pylint: disable=too-many-arguments,too-many-branches
+    def raise_for_access(
+        # pylint: disable=too-many-arguments,too-many-branches,
+        # pylint: disable=too-many-locals
         self,
         database: Optional["Database"] = None,
         datasource: Optional["BaseDatasource"] = None,
@@ -996,9 +998,15 @@ def raise_for_access(  # pylint: disable=too-many-arguments,too-many-branches
 
             assert datasource
 
+            from superset.extensions import feature_flag_manager
+
             if not (
                 self.can_access_schema(datasource)
                 or self.can_access("datasource_access", datasource.perm or "")
+                or (
+                    feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC")
+                    and self.can_access_based_on_dashboard(datasource)
+                )
             ):
                 raise SupersetSecurityException(
                     self.get_datasource_access_error_object(datasource)
@@ -1101,8 +1109,8 @@ def get_rls_ids(self, table: "BaseDatasource") -> List[int]:
         ids.sort()  # Combinations rather than permutations
         return ids
 
-    # pylint: disable=no-self-use
-    def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
+    @staticmethod
+    def raise_for_dashboard_access(dashboard: "Dashboard") -> None:
         """
         Raise an exception if the user cannot access the dashboard.
 
@@ -1127,3 +1135,24 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
 
             if not can_access:
                 raise DashboardAccessDeniedError()
+
+    @staticmethod
+    def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool:
+        from superset import db
+        from superset.dashboards.filters import DashboardFilter
+        from superset.models.slice import Slice
+        from superset.models.dashboard import Dashboard
+
+        datasource_class = type(datasource)
+        query = (
+            db.session.query(datasource_class)
+            .join(Slice.table)
+            .filter(datasource_class.id == datasource.id)
+        )
+
+        query = DashboardFilter("id", SQLAInterface(Dashboard, db.session)).apply(
+            query, None
+        )
+
+        exists = db.session.query(query.exists()).scalar()
+        return exists

superset/views/dashboard/mixin.py

@@ -65,8 +65,7 @@ class DashboardMixin:  # pylint: disable=too-few-public-methods
         "owners": _("Owners is a list of users who can alter the dashboard."),
         "roles": _(
             "Roles is a list which defines access to the dashboard. "
-            "These roles are always applied in addition to restrictions on dataset "
-            "level access. "
+            "Granting a role access to a dashboard will bypass dataset level checks."
             "If no roles defined then the dashboard is available to all roles."
         ),
         "published": _(

tests/dashboards/security/security_rbac_tests.py

@@ -27,7 +27,11 @@
     create_datasource_table_to_db,
     create_slice_to_db,
 )
+from tests.fixtures.birth_names_dashboard import load_birth_names_dashboard_with_slices
 from tests.fixtures.public_role import public_role_like_gamma
+from tests.fixtures.query_context import get_query_context
+
+CHART_DATA_URI = "api/v1/chart/data"
 
 
 @mock.patch.dict(
@@ -65,16 +69,26 @@ def test_get_dashboard_view__owner_can_access(self):
         # assert
         self.assert_dashboard_view_response(response, dashboard_to_access)
 
+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_get_dashboard_view__user_can_not_access_without_permission(self):
         username = random_str()
         new_role = f"role_{random_str()}"
         self.create_user_with_roles(username, [new_role], should_create_roles=True)
-        dashboard_to_access = create_dashboard_to_db(published=True)
+        slice = (
+            db.session.query(Slice)
+            .filter_by(slice_name="Girl Name Cloud")
+            .one_or_none()
+        )
+        dashboard_to_access = create_dashboard_to_db(published=True, slices=[slice])
         self.login(username)
 
         # act
         response = self.get_dashboard_view_response(dashboard_to_access)
 
+        request_payload = get_query_context("birth_names")
+        rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+        self.assertEqual(rv.status_code, 401)
+
         # assert
         self.assert403(response)
 
@@ -98,16 +112,20 @@ def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft
         # post
         revoke_access_to_dashboard(dashboard_to_access, new_role)
 
+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_get_dashboard_view__user_access_with_dashboard_permission(self):
         # arrange
 
         username = random_str()
         new_role = f"role_{random_str()}"
         self.create_user_with_roles(username, [new_role], should_create_roles=True)
 
-        dashboard_to_access = create_dashboard_to_db(
-            published=True, slices=[create_slice_to_db()]
+        slice = (
+            db.session.query(Slice)
+            .filter_by(slice_name="Girl Name Cloud")
+            .one_or_none()
         )
+        dashboard_to_access = create_dashboard_to_db(published=True, slices=[slice])
         self.login(username)
         grant_access_to_dashboard(dashboard_to_access, new_role)
 
@@ -117,6 +135,10 @@ def test_get_dashboard_view__user_access_with_dashboard_permission(self):
         # assert
         self.assert_dashboard_view_response(response, dashboard_to_access)
 
+        request_payload = get_query_context("birth_names")
+        rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+        self.assertEqual(rv.status_code, 200)
+
         # post
         revoke_access_to_dashboard(dashboard_to_access, new_role)