feat(dashboard_rbac): dashboard_view access enforcement (#12875)

* test: dashboard_view_test failing

* test: tests works first time

* fix: pre-commit and  some refactoring

* fix:  after CR

* fix:  replace not_published with draft

* fix:  after CR

* fix: pre-commit fixes

* fix: pre-commit and lint fixes

* fix: remove unused

* fix: remove unused import

* fix: wrap the decorator to not block others

* chore: reuse dashboard from decorator into function

superset/dashboards/commands/exceptions.py

@@ -75,3 +75,7 @@ class DashboardForbiddenError(ForbiddenError):
 
 class DashboardImportError(ImportFailedError):
     message = _("Import dashboard failed for an unknown reason")
+
+
+class DashboardAccessDeniedError(ForbiddenError):
+    message = _("You don't have access to this dashboard.")

superset/models/dashboard.py

@@ -14,12 +14,15 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+from __future__ import annotations
+
 import json
 import logging
 from functools import partial
 from typing import Any, Callable, Dict, List, Set, Union
 
 import sqlalchemy as sqla
+from flask import g
 from flask_appbuilder import Model
 from flask_appbuilder.models.decorators import renders
 from flask_appbuilder.security.sqla.models import User
@@ -45,6 +48,7 @@
 from superset.connectors.base.models import BaseDatasource
 from superset.connectors.druid.models import DruidColumn, DruidMetric
 from superset.connectors.sqla.models import SqlMetric, TableColumn
+from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
 from superset.extensions import cache_manager
 from superset.models.helpers import AuditMixinNullable, ImportExportMixin
 from superset.models.slice import Slice
@@ -356,6 +360,17 @@ def export_dashboards(  # pylint: disable=too-many-locals
             indent=4,
         )
 
+    @classmethod
+    def get(cls, id_or_slug: str) -> Dashboard:
+        session = db.session()
+        qry = session.query(Dashboard)
+        if id_or_slug.isdigit():
+            qry = qry.filter_by(id=int(id_or_slug))
+        else:
+            qry = qry.filter_by(slug=id_or_slug)
+
+        return qry.one_or_none()
+
 
 OnDashboardChange = Callable[[Mapper, Connection, Dashboard], Any]
 
@@ -407,3 +422,22 @@ def clear_dashboard_cache(
     sqla.event.listen(TableColumn, "after_update", clear_dashboard_cache)
     sqla.event.listen(DruidMetric, "after_update", clear_dashboard_cache)
     sqla.event.listen(DruidColumn, "after_update", clear_dashboard_cache)
+
+
+def raise_for_dashboard_access(dashboard: Dashboard) -> None:
+    from superset.views.base import get_user_roles, is_user_admin
+    from superset.views.utils import is_owner
+
+    if is_feature_enabled("DASHBOARD_RBAC"):
+        has_rbac_access = any(
+            dashboard_role.id in [user_role.id for user_role in get_user_roles()]
+            for dashboard_role in dashboard.roles
+        )
+        can_access = (
+            is_user_admin()
+            or is_owner(dashboard, g.user)
+            or (dashboard.published and has_rbac_access)
+        )
+
+        if not can_access:
+            raise DashboardAccessDeniedError()

superset/utils/decorators.py

@@ -15,11 +15,16 @@
 # specific language governing permissions and limitations
 # under the License.
 import time
+from functools import wraps
 from typing import Any, Callable, Dict, Iterator, Union
 
 from contextlib2 import contextmanager
+from flask import Response
 
+from superset import is_feature_enabled
+from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
 from superset.stats_logger import BaseStatsLogger
+from superset.utils import core as utils
 from superset.utils.dates import now_as_float
 
 
@@ -69,3 +74,35 @@ def wrapped(*args: Any, **kwargs: Any) -> Any:
         return wrapped
 
     return decorate
+
+
+def on_security_exception(self: Any, ex: Exception) -> Response:
+    return self.response(403, **{"message": utils.error_msg_from_exception(ex)})
+
+
+# noinspection PyPackageRequirements
+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:
+            from superset.models.dashboard import (
+                Dashboard,
+                raise_for_dashboard_access,
+            )
+
+            dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"]))
+            if is_feature_enabled("DASHBOARD_RBAC"):
+                try:
+                    raise_for_dashboard_access(dashboard)
+                except DashboardAccessDeniedError as ex:
+                    return on_error(self, ex)
+                except Exception as exception:
+                    raise exception
+
+            return f(self, *args, dashboard=dashboard, **kwargs)
+
+        return wrapper
+
+    return decorator

superset/views/core.py

@@ -104,6 +104,7 @@
 from superset.utils.async_query_manager import AsyncQueryTokenException
 from superset.utils.cache import etag_cache
 from superset.utils.dates import now_as_float
+from superset.utils.decorators import check_dashboard_access
 from superset.views.base import (
     api,
     BaseSupersetView,
@@ -160,7 +161,6 @@
     "id",
 ]
 
-
 DATASOURCE_MISSING_ERR = __("The data source seems to have been deleted")
 USER_MISSING_ERR = __("The user seems to have been deleted")
 PARAMETER_MISSING_ERR = (
@@ -867,7 +867,8 @@ def remove_extra_filters(filters: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
         Those should not be saved when saving the chart"""
         return [f for f in filters if not f.get("isExtra")]
 
-    def save_or_overwrite_slice(  # pylint: disable=too-many-arguments,too-many-locals,no-self-use
+    def save_or_overwrite_slice(
+        # pylint: disable=too-many-arguments,too-many-locals,no-self-use
         self,
         slc: Optional[Slice],
         slice_add_perm: bool,
@@ -1785,33 +1786,34 @@ def publish(  # pylint: disable=no-self-use
     @has_access
     @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
+        )
+    )
     def dashboard(  # pylint: disable=too-many-locals
-        self,
-        dashboard_id_or_slug: str,
-        # this parameter is added by `log_this_with_manual_updates`,
-        # set a default value to appease pylint
+        self,  # pylint: disable=no-self-use
+        dashboard_id_or_slug: str,  # pylint: disable=unused-argument
         add_extra_log_payload: Callable[..., None] = lambda **kwargs: None,
+        dashboard: Optional[Dashboard] = None,
     ) -> FlaskResponse:
-        """Server side rendering for a dashboard"""
-        session = db.session()
-        qry = session.query(Dashboard)
-        if dashboard_id_or_slug.isdigit():
-            qry = qry.filter_by(id=int(dashboard_id_or_slug))
-        else:
-            qry = qry.filter_by(slug=dashboard_id_or_slug)
-
-        dash = qry.one_or_none()
-        if not dash:
+        """
+        Server side rendering for a dashboard
+        :param dashboard_id_or_slug: identifier for dashboard. used in the decorators
+        :param add_extra_log_payload: added by `log_this_with_manual_updates`, set a default value to appease pylint
+        :param dashboard: added by `check_dashboard_access`
+        """
+        if not dashboard:
             abort(404)
 
-        data = dash.full_data()
+        data = dashboard.full_data()
 
         if config["ENABLE_ACCESS_REQUEST"]:
             for datasource in data["datasources"].values():
                 datasource = ConnectorRegistry.get_datasource(
                     datasource_type=datasource["type"],
                     datasource_id=datasource["id"],
-                    session=session,
+                    session=db.session(),
                 )
                 if datasource and not security_manager.can_access_datasource(
                     datasource=datasource
@@ -1822,10 +1824,12 @@ def dashboard(  # pylint: disable=too-many-locals
                         ),
                         "danger",
                     )
-                    return redirect(f"/superset/request_access/?dashboard_id={dash.id}")
+                    return redirect(
+                        f"/superset/request_access/?dashboard_id={dashboard.id}"
+                    )
 
         dash_edit_perm = check_ownership(
-            dash, raise_if_false=False
+            dashboard, raise_if_false=False
         ) and security_manager.can_access("can_save_dash", "Superset")
         dash_save_perm = security_manager.can_access("can_save_dash", "Superset")
         superset_can_explore = security_manager.can_access("can_explore", "Superset")
@@ -1840,7 +1844,7 @@ def dashboard(  # pylint: disable=too-many-locals
         )
 
         add_extra_log_payload(
-            dashboard_id=dash.id,
+            dashboard_id=dashboard.id,
             dashboard_version="v2",
             dash_edit_perm=dash_edit_perm,
             edit_mode=edit_mode,
@@ -1885,8 +1889,8 @@ def dashboard(  # pylint: disable=too-many-locals
             "superset/dashboard.html",
             entry="dashboard",
             standalone_mode=standalone_mode,
-            title=dash.dashboard_title,
-            custom_css=dash.css,
+            title=dashboard.dashboard_title,
+            custom_css=dashboard.css,
             bootstrap_data=json.dumps(
                 bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser
             ),
@@ -2238,7 +2242,8 @@ def stop_query(self) -> FlaskResponse:
     @has_access_api
     @event_logger.log_this
     @expose("/validate_sql_json/", methods=["POST", "GET"])
-    def validate_sql_json(  # pylint: disable=too-many-locals,too-many-return-statements,no-self-use
+    def validate_sql_json(
+        # pylint: disable=too-many-locals,too-many-return-statements,no-self-use
         self,
     ) -> FlaskResponse:
         """Validates that arbitrary sql is acceptable for the given database.

tests/dashboards/security/security_dataset_tests.py

@@ -160,7 +160,7 @@ def test_get_dashboards__user_can_not_view_unpublished_dash(self):
         # arrange
         admin_user = security_manager.find_user(ADMIN_USERNAME)
         gamma_user = security_manager.find_user(GAMMA_USERNAME)
-        admin_and_not_published_dashboard = create_dashboard_to_db(
+        admin_and_draft_dashboard = create_dashboard_to_db(
             dashboard_title="admin_owned_unpublished_dash", owners=[admin_user]
         )
 
@@ -171,7 +171,7 @@ def test_get_dashboards__user_can_not_view_unpublished_dash(self):
 
         # assert
         self.assertNotIn(
-            admin_and_not_published_dashboard.url, get_dashboards_response_as_gamma
+            admin_and_draft_dashboard.url, get_dashboards_response_as_gamma
         )
 
     @pytest.mark.usefixtures("load_energy_table_with_slice", "load_dashboard")