feat(dashboard_rbac): dashboard_view access enforcement

superset/errors.py

@@ -48,6 +48,7 @@ class SupersetErrorType(str, Enum):
     # Security access errors
     TABLE_SECURITY_ACCESS_ERROR = "TABLE_SECURITY_ACCESS_ERROR"
     DATASOURCE_SECURITY_ACCESS_ERROR = "DATASOURCE_SECURITY_ACCESS_ERROR"
+    DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR"
     MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR"
 
     # Other errors

superset/models/dashboard.py

@@ -45,6 +45,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.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.extensions import cache_manager
 from superset.models.helpers import AuditMixinNullable, ImportExportMixin
 from superset.models.slice import Slice
@@ -407,3 +408,30 @@ 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 get_dashboard(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()
+
+
+def get_dashboard_access_error_object(
+    dashboard: Dashboard,  # pylint: disable=invalid-name
+) -> SupersetError:
+    """
+        Return the error object for the denied Superset dashboard.
+        :param dashboard: The denied Superset dashboard
+        :returns: The error object
+        """
+    return SupersetError(
+        error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
+        message=f"This dashboard requires to have one of the access roles assigned it",
+        level=ErrorLevel.ERROR,
+        extra={"dashboard": dashboard.id,},
+    )

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__)
 
@@ -417,6 +418,27 @@ def can_access_table(self, database: "Database", table: "Table") -> bool:
 
         return True
 
+    def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
+        from superset.views.base import is_user_admin
+        from superset.views.utils import is_owner
+        from superset.models.dashboard import get_dashboard_access_error_object
+        from superset.views.base import get_user_roles
+
+        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 SupersetSecurityException(
+                get_dashboard_access_error_object(dashboard)
+            )
+
     def user_view_menu_names(self, permission_name: str) -> Set[str]:
         base_query = (
             self.get_session.query(self.viewmenu_model.name)

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.exceptions import SupersetSecurityException
 from superset.stats_logger import BaseStatsLogger
+from superset.utils import core as utils
 from superset.utils.dates import now_as_float
 
 
@@ -69,3 +74,34 @@ def wrapped(*args: Any, **kwargs: Any) -> Any:
         return wrapped
 
     return decorate
+
+
+def on_security_exception(self, ex) -> Response:
+    return self.response(403, **{"message": utils.error_msg_from_exception(ex)})
+
+
+def check_permissions(
+    on_error: Callable[..., Any] = on_security_exception
+) -> Callable[..., Any]:
+    def decorator(f: Callable[..., Any]) -> Callable[..., Any]:
+        def wrapper(self, *args: Any, **kwargs: Any) -> Callable:
+
+            if is_feature_enabled("DASHBOARD_RBAC"):
+                try:
+                    from superset import security_manager
+                    from superset.models.dashboard import get_dashboard
+
+                    dashboard = get_dashboard(str(kwargs["dashboard_id_or_slug"]))
+
+                    security_manager.raise_for_dashboard_access(dashboard)
+
+                except SupersetSecurityException as ex:
+                    return on_error(self, ex)
+                except Exception as e:
+                    raise e
+
+            return f(self, *args, **kwargs)
+
+        return wrapper
+
+    return decorator

superset/views/core.py

@@ -86,7 +86,7 @@
 from superset.extensions import async_query_manager, cache_manager
 from superset.jinja_context import get_template_processor
 from superset.models.core import Database, FavStar, Log
-from superset.models.dashboard import Dashboard
+from superset.models.dashboard import Dashboard, get_dashboard
 from superset.models.datasource_access_request import DatasourceAccessRequest
 from superset.models.slice import Slice
 from superset.models.sql_lab import Query, TabState
@@ -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_permissions
 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_permissions(
+        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 = get_dashboard(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