fix(dashboard-rbac): use normal rbac when no roles chosen (#23586)

(cherry picked from commit a823033)

Author: villebro

docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx

@@ -184,9 +184,8 @@ Non-owner users access can be managed two different ways:
 
 1. Dataset permissions - if you add to the relevant role permissions to datasets it automatically grants implicit access to all dashboards that uses those permitted datasets
 2. Dashboard roles - if you enable **DASHBOARD_RBAC** [feature flag](https://superset.apache.org/docs/installation/configuring-superset#feature-flags) then you be able to manage which roles can access the dashboard
-- Having dashboard access implicitly grants read access to the associated datasets, therefore
-all charts will load their data even if feature flag is turned on and no roles assigned
-to roles the access will fallback to **Dataset permissions**
+   - Granting a role access to a dashboard will bypass dataset level checks. Having dashboard access implicitly grants read access to all the featured charts in the dashboard, and thereby also all the associated datasets.
+   - If no roles are specified for a dashboard, regular **Dataset permissions** will apply.
 
 <img src={useBaseUrl("/img/tutorial/tutorial_dashboard_access.png" )} />
 

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

@@ -486,7 +486,7 @@ const PropertiesModal = ({
             </StyledFormItem>
             <p className="help-block">
               {t(
-                '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 are 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 are defined, regular access permissions apply.',
               )}
             </p>
           </Col>

superset/security/manager.py

@@ -2044,7 +2044,10 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
         from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
 
         def has_rbac_access() -> bool:
-            return (not is_feature_enabled("DASHBOARD_RBAC")) or any(
+            if not is_feature_enabled("DASHBOARD_RBAC") or not dashboard.roles:
+                return True
+
+            return any(
                 dashboard_role.id
                 in [user_role.id for user_role in self.get_user_roles()]
                 for dashboard_role in dashboard.roles
@@ -2071,15 +2074,18 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool:
         from superset.models.dashboard import Dashboard
         from superset.models.slice import Slice
 
+        dashboard_ids = db.session.query(Dashboard.id)
+        dashboard_ids = DashboardAccessFilter(
+            "id", SQLAInterface(Dashboard, db.session)
+        ).apply(dashboard_ids, None)
+
         datasource_class = type(datasource)
         query = (
-            db.session.query(datasource_class)
+            db.session.query(Dashboard.id)
+            .join(Slice, Dashboard.slices)
             .join(Slice.table)
             .filter(datasource_class.id == datasource.id)
-        )
-
-        query = DashboardAccessFilter("id", SQLAInterface(Dashboard, db.session)).apply(
-            query, None
+            .filter(Dashboard.id.in_(dashboard_ids))
         )
 
         exists = db.session.query(query.exists()).scalar()

superset/utils/decorators.py

@@ -116,9 +116,7 @@ def on_security_exception(self: Any, ex: Exception) -> Response:
 
 
 # noinspection PyPackageRequirements
-def check_dashboard_access(
-    on_error: Callable[..., Any] = on_security_exception
-) -> Callable[..., Any]:
+def check_dashboard_access(on_error: Callable[[str], Any]) -> Callable[..., Any]:
     def decorator(f: Callable[..., Any]) -> Callable[..., Any]:
         @wraps(f)
         def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any:
@@ -130,7 +128,7 @@ def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any:
                 try:
                     current_app.appbuilder.sm.raise_for_dashboard_access(dashboard)
                 except DashboardAccessDeniedError as ex:
-                    return on_error(self, ex)
+                    return on_error(str(ex))
                 except Exception as exception:
                     raise exception
 

superset/views/core.py

@@ -67,6 +67,7 @@
     TableColumn,
 )
 from superset.constants import QUERY_EARLY_CANCEL_KEY
+from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
 from superset.dashboards.commands.importers.v0 import ImportDashboardsCommand
 from superset.dashboards.dao import DashboardDAO
 from superset.dashboards.permalink.commands.get import GetDashboardPermalinkCommand
@@ -167,6 +168,7 @@
     get_form_data,
     get_viz,
     loads_request_json,
+    redirect_with_flash,
     sanitize_datasource_data,
 )
 from superset.viz import BaseViz
@@ -192,6 +194,7 @@
     "disable_data_preview",
 ]
 
+DASHBOARD_LIST_URL = "/dashboard/list/"
 DATASOURCE_MISSING_ERR = __("The data source seems to have been deleted")
 USER_MISSING_ERR = __("The user seems to have been deleted")
 PARAMETER_MISSING_ERR = __(
@@ -1823,9 +1826,7 @@ def favstar(  # pylint: disable=no-self-use
     @expose("/dashboard/<dashboard_id_or_slug>/")
     @event_logger.log_this_with_extra_payload
     @check_dashboard_access(
-        on_error=lambda self, ex: Response(
-            utils.error_msg_from_exception(ex), status=403
-        )
+        on_error=lambda msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger")
     )
     def dashboard(
         self,
@@ -1843,25 +1844,33 @@ def dashboard(
         if not dashboard:
             abort(404)
 
-        if config["ENABLE_ACCESS_REQUEST"]:
-            for datasource in dashboard.datasources:
-                datasource = DatasourceDAO.get_datasource(
-                    datasource_type=DatasourceType(datasource.type),
-                    datasource_id=datasource.id,
-                    session=db.session(),
+        has_access_ = False
+        for datasource in dashboard.datasources:
+            datasource = DatasourceDAO.get_datasource(
+                datasource_type=DatasourceType(datasource.type),
+                datasource_id=datasource.id,
+                session=db.session(),
+            )
+            if datasource and security_manager.can_access_datasource(
+                datasource=datasource,
+            ):
+                has_access_ = True
+
+            if has_access_ is False and config["ENABLE_ACCESS_REQUEST"]:
+                flash(
+                    __(security_manager.get_datasource_access_error_msg(datasource)),
+                    "danger",
                 )
-                if datasource and not security_manager.can_access_datasource(
-                    datasource=datasource
-                ):
-                    flash(
-                        __(
-                            security_manager.get_datasource_access_error_msg(datasource)
-                        ),
-                        "danger",
-                    )
-                    return redirect(
-                        f"/superset/request_access/?dashboard_id={dashboard.id}"
-                    )
+                return redirect(
+                    f"/superset/request_access/?dashboard_id={dashboard.id}"
+                )
+
+            if has_access_:
+                break
+
+        if dashboard.datasources and not has_access_:
+            flash(DashboardAccessDeniedError.message, "danger")
+            return redirect(DASHBOARD_LIST_URL)
 
         dash_edit_perm = security_manager.is_owner(
             dashboard

superset/views/dashboard/mixin.py

@@ -66,7 +66,7 @@ class DashboardMixin:  # pylint: disable=too-few-public-methods
         "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 are defined then the dashboard is available to all roles."
+            "If no roles are defined, regular access permissions apply."
         ),
         "published": _(
             "Determines whether or not this dashboard is "

superset/views/utils.py

@@ -23,11 +23,12 @@
 import msgpack
 import pyarrow as pa
 import simplejson as json
-from flask import g, has_request_context, request
+from flask import flash, g, has_request_context, redirect, request
 from flask_appbuilder.security.sqla import models as ab_models
 from flask_appbuilder.security.sqla.models import User
 from flask_babel import _
 from sqlalchemy.orm.exc import NoResultFound
+from werkzeug.wrappers.response import Response
 
 import superset.models.core as models
 from superset import app, dataframe, db, result_set, viz
@@ -592,3 +593,8 @@ def get_cta_schema_name(
     if not func:
         return None
     return func(database, user, schema, sql)
+
+
+def redirect_with_flash(url: str, message: str, category: str) -> Response:
+    flash(message=message, category=category)
+    return redirect(url)

tests/integration_tests/dashboards/security/security_rbac_tests.py

@@ -91,13 +91,11 @@ def test_get_dashboard_view__user_can_not_access_without_permission(self):
 
         # act
         response = self.get_dashboard_view_response(dashboard_to_access)
+        assert response.status_code == 302
 
         request_payload = get_query_context("birth_names")
         rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
-        self.assertEqual(rv.status_code, 403)
-
-        # assert
-        self.assert403(response)
+        assert rv.status_code == 403
 
     def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft(
         self,
@@ -114,11 +112,57 @@ def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft
         response = self.get_dashboard_view_response(dashboard_to_access)
 
         # assert
-        self.assert403(response)
+        assert response.status_code == 302
 
         # 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_no_access_regular_rbac(self):
+        if backend() == "hive":
+            return
+
+        slice = (
+            db.session.query(Slice)
+            .filter_by(slice_name="Girl Name Cloud")
+            .one_or_none()
+        )
+        dashboard = create_dashboard_to_db(published=True, slices=[slice])
+        self.login("gamma")
+
+        # assert redirect on regular rbac access denied
+        response = self.get_dashboard_view_response(dashboard)
+        assert response.status_code == 302
+
+        request_payload = get_query_context("birth_names")
+        rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+        assert rv.status_code == 403
+        db.session.delete(dashboard)
+        db.session.commit()
+
+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+    def test_get_dashboard_view__user_access_regular_rbac(self):
+        if backend() == "hive":
+            return
+
+        slice = (
+            db.session.query(Slice)
+            .filter_by(slice_name="Girl Name Cloud")
+            .one_or_none()
+        )
+        dashboard = create_dashboard_to_db(published=True, slices=[slice])
+        self.login("gamma_sqllab")
+
+        response = self.get_dashboard_view_response(dashboard)
+
+        assert response.status_code == 200
+
+        request_payload = get_query_context("birth_names")
+        rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
+        assert rv.status_code == 200
+        db.session.delete(dashboard)
+        db.session.commit()
+
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_get_dashboard_view__user_access_with_dashboard_permission(self):
         if backend() == "hive":
@@ -155,13 +199,14 @@ def test_get_dashboard_view__user_access_with_dashboard_permission(self):
     @pytest.mark.usefixtures("public_role_like_gamma")
     def test_get_dashboard_view__public_user_can_not_access_without_permission(self):
         dashboard_to_access = create_dashboard_to_db(published=True)
+        grant_access_to_dashboard(dashboard_to_access, "Alpha")
         self.logout()
 
         # act
         response = self.get_dashboard_view_response(dashboard_to_access)
 
         # assert
-        self.assert403(response)
+        assert response.status_code == 302
 
     @pytest.mark.usefixtures("public_role_like_gamma")
     def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft(
@@ -175,7 +220,7 @@ def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_acces
         response = self.get_dashboard_view_response(dashboard_to_access)
 
         # assert
-        self.assert403(response)
+        assert response.status_code == 302
 
         # post
         revoke_access_to_dashboard(dashboard_to_access, "Public")