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

This reverts commit a823033.

Author: john-bodley

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

@@ -184,8 +184,9 @@ 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
-   - 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.
+- 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**
 
 <img src={useBaseUrl("/img/tutorial/tutorial_dashboard_access.png" )} />
 

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

@@ -560,7 +560,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, regular access permissions apply.',
+                '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.',
               )}
             </p>
           </Col>

superset/security/manager.py

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

superset/utils/decorators.py

@@ -116,7 +116,9 @@ def on_security_exception(self: Any, ex: Exception) -> Response:
 
 
 # noinspection PyPackageRequirements
-def check_dashboard_access(on_error: Callable[[str], Any]) -> Callable[..., Any]:
+def check_dashboard_access(
+    on_error: Callable[..., Any] = on_security_exception
+) -> Callable[..., Any]:
     def decorator(f: Callable[..., Any]) -> Callable[..., Any]:
         @wraps(f)
         def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any:
@@ -128,7 +130,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(str(ex))
+                    return on_error(self, ex)
                 except Exception as exception:
                     raise exception
 

superset/views/core.py

@@ -66,7 +66,6 @@
     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,7 +166,6 @@
     get_form_data,
     get_viz,
     loads_request_json,
-    redirect_with_flash,
     sanitize_datasource_data,
 )
 from superset.viz import BaseViz
@@ -193,7 +191,6 @@
     "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 = __(
@@ -1828,7 +1825,9 @@ 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 msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger")
+        on_error=lambda self, ex: Response(
+            utils.error_msg_from_exception(ex), status=403
+        )
     )
     def dashboard(
         self,
@@ -1846,33 +1845,25 @@ def dashboard(
         if not dashboard:
             abort(404)
 
-        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",
-                )
-                return redirect(
-                    f"/superset/request_access/?dashboard_id={dashboard.id}"
+        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(),
                 )
-
-            if has_access_:
-                break
-
-        if dashboard.datasources and not has_access_:
-            flash(DashboardAccessDeniedError.message, "danger")
-            return redirect(DASHBOARD_LIST_URL)
+                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}"
+                    )
 
         dash_edit_perm = security_manager.is_owner(
             dashboard

superset/views/dashboard/mixin.py

@@ -65,7 +65,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, regular access permissions apply."
+            "If no roles are defined then the dashboard is available to all roles."
         ),
         "published": _(
             "Determines whether or not this dashboard is "

superset/views/utils.py

@@ -23,12 +23,11 @@
 import msgpack
 import pyarrow as pa
 import simplejson as json
-from flask import flash, g, has_request_context, redirect, request
+from flask import g, has_request_context, 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
@@ -593,8 +592,3 @@ 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,11 +91,13 @@ 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")
-        assert rv.status_code == 403
+        self.assertEqual(rv.status_code, 403)
+
+        # assert
+        self.assert403(response)
 
     def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft(
         self,
@@ -112,57 +114,11 @@ def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft
         response = self.get_dashboard_view_response(dashboard_to_access)
 
         # assert
-        assert response.status_code == 302
+        self.assert403(response)
 
         # 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":
@@ -199,14 +155,13 @@ 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
-        assert response.status_code == 302
+        self.assert403(response)
 
     @pytest.mark.usefixtures("public_role_like_gamma")
     def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft(
@@ -220,7 +175,7 @@ def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_acces
         response = self.get_dashboard_view_response(dashboard_to_access)
 
         # assert
-        assert response.status_code == 302
+        self.assert403(response)
 
         # post
         revoke_access_to_dashboard(dashboard_to_access, "Public")