feat(dashboard_rbac): dashboard_view access enforcement

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/security/manager.py

@@ -58,6 +58,7 @@
     from superset.models.sql_lab import Query
     from superset.sql_parse import Table
     from superset.viz import BaseViz
+    from superset.models.dashboard import Dashboard
 
 logger = logging.getLogger(__name__)
 

superset/utils/decorators.py

@@ -18,8 +18,12 @@
 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 +73,36 @@ 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)})
+
+
+def check_dashboard_access(
+    on_error: Callable[..., Any] = on_security_exception
+) -> Callable[..., Any]:
+    def decorator(f: Callable[..., Any]) -> Callable[..., Any]:
+        def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any:
+
+            if is_feature_enabled("DASHBOARD_RBAC"):
+                try:
+                    from superset.models.dashboard import (
+                        Dashboard,
+                        raise_for_dashboard_access,
+                    )
+
+                    dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"]))
+
+                    raise_for_dashboard_access(dashboard)
+
+                except DashboardAccessDeniedError as ex:
+                    return on_error(self, ex)
+                except Exception as exception:
+                    raise exception
+
+            return f(self, *args, **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,
@@ -1785,6 +1786,11 @@ 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,
@@ -1793,14 +1799,7 @@ def dashboard(  # pylint: disable=too-many-locals
         add_extra_log_payload: Callable[..., None] = lambda **kwargs: 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()
+        dash = Dashboard.get(dashboard_id_or_slug)
         if not dash:
             abort(404)
 
@@ -1811,7 +1810,7 @@ def dashboard(  # pylint: disable=too-many-locals
                 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

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")