chore: Refactor dashboard security access (#24804)

(cherry picked from commit 5522fac)

superset-frontend/src/components/ErrorMessage/types.ts

@@ -55,6 +55,7 @@ export const ErrorTypeEnum = {
   DATABASE_SECURITY_ACCESS_ERROR: 'DATABASE_SECURITY_ACCESS_ERROR',
   QUERY_SECURITY_ACCESS_ERROR: 'QUERY_SECURITY_ACCESS_ERROR',
   MISSING_OWNERSHIP_ERROR: 'MISSING_OWNERSHIP_ERROR',
+  DASHBOARD_SECURITY_ACCESS_ERROR: 'DASHBOARD_SECURITY_ACCESS_ERROR',
 
   // Other errors
   BACKEND_TIMEOUT_ERROR: 'BACKEND_TIMEOUT_ERROR',

superset/daos/dashboard.py

@@ -26,10 +26,12 @@
 from flask_appbuilder.models.sqla.interface import SQLAInterface
 from sqlalchemy.exc import SQLAlchemyError
 
-from superset import security_manager
 from superset.daos.base import BaseDAO
 from superset.daos.exceptions import DAOConfigError, DAOCreateFailedError
-from superset.dashboards.commands.exceptions import DashboardNotFoundError
+from superset.dashboards.commands.exceptions import (
+    DashboardAccessDeniedError,
+    DashboardNotFoundError,
+)
 from superset.dashboards.filter_sets.consts import (
     DASHBOARD_ID_FIELD,
     DESCRIPTION_FIELD,
@@ -39,6 +41,7 @@
     OWNER_TYPE_FIELD,
 )
 from superset.dashboards.filters import DashboardAccessFilter, is_uuid
+from superset.exceptions import SupersetSecurityException
 from superset.extensions import db
 from superset.models.core import FavStar, FavStarClassName
 from superset.models.dashboard import Dashboard, id_or_slug_filter
@@ -77,7 +80,11 @@ def get_by_id_or_slug(cls, id_or_slug: int | str) -> Dashboard:
             raise DashboardNotFoundError()
 
         # make sure we still have basic access check from security manager
-        security_manager.raise_for_dashboard_access(dashboard)
+        try:
+            dashboard.raise_for_access()
+        except SupersetSecurityException as ex:
+            raise DashboardAccessDeniedError() from ex
+
         return dashboard
 
     @staticmethod

superset/errors.py

@@ -27,7 +27,6 @@ class SupersetErrorType(StrEnum):
     Types of errors that can exist within Superset.
 
     Keep in sync with superset-frontend/src/components/ErrorMessage/types.ts
-    and docs/src/pages/docs/Miscellaneous/issue_codes.mdx
     """
 
     # Frontend errors
@@ -66,6 +65,7 @@ class SupersetErrorType(StrEnum):
     QUERY_SECURITY_ACCESS_ERROR = "QUERY_SECURITY_ACCESS_ERROR"
     MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR"
     USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR"
+    DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR"
 
     # Other errors
     BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR"

superset/models/dashboard.py

@@ -446,6 +446,15 @@ def get(cls, id_or_slug: str | int) -> Dashboard:
         qry = db.session.query(Dashboard).filter(id_or_slug_filter(id_or_slug))
         return qry.one_or_none()
 
+    def raise_for_access(self) -> None:
+        """
+        Raise an exception if the user cannot access the resource.
+
+        :raises SupersetSecurityException: If the user cannot access the resource
+        """
+
+        security_manager.raise_for_access(dashboard=self)
+
 
 def is_uuid(value: str | int) -> bool:
     try:

superset/security/manager.py

@@ -412,16 +412,30 @@ def can_access_dashboard(self, dashboard: "Dashboard") -> bool:
         :returns: Whether the user can access the dashboard
         """
 
-        # pylint: disable=import-outside-toplevel
-        from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
-
         try:
-            self.raise_for_dashboard_access(dashboard)
-        except DashboardAccessDeniedError:
+            self.raise_for_access(dashboard=dashboard)
+        except SupersetSecurityException:
             return False
 
         return True
 
+    def get_dashboard_access_error_object(  # pylint: disable=invalid-name
+        self,
+        dashboard: "Dashboard",  # pylint: disable=unused-argument
+    ) -> 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="You don't have access to this dashboard.",
+            level=ErrorLevel.ERROR,
+        )
+
     @staticmethod
     def get_datasource_access_error_msg(datasource: "BaseDatasource") -> str:
         """
@@ -1761,8 +1775,9 @@ def get_exclude_users_from_lists() -> list[str]:
         return []
 
     def raise_for_access(
-        # pylint: disable=too-many-arguments, too-many-locals
+        # pylint: disable=too-many-arguments,too-many-branches,too-many-locals
         self,
+        dashboard: Optional["Dashboard"] = None,
         database: Optional["Database"] = None,
         datasource: Optional["BaseDatasource"] = None,
         query: Optional["Query"] = None,
@@ -1783,6 +1798,7 @@ def raise_for_access(
         """
 
         # pylint: disable=import-outside-toplevel
+        from superset import is_feature_enabled
         from superset.connectors.sqla.models import SqlaTable
         from superset.daos.dashboard import DashboardDAO
         from superset.sql_parse import Table
@@ -1856,6 +1872,45 @@ def raise_for_access(
                     self.get_datasource_access_error_object(datasource)
                 )
 
+        if dashboard:
+            if self.is_guest_user():
+                # Guest user is currently used for embedded dashboards only. If the guest
+                # user doesn't have access to the dashboard, ignore all other checks.
+                if self.has_guest_access(dashboard):
+                    return
+                raise SupersetSecurityException(
+                    self.get_dashboard_access_error_object(dashboard)
+                )
+
+            if self.is_admin() or self.is_owner(dashboard):
+                return
+
+            # RBAC and legacy (datasource inferred) access controls.
+            if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
+                if dashboard.published and {role.id for role in dashboard.roles} & {
+                    role.id for role in self.get_user_roles()
+                }:
+                    return
+            elif (
+                # To understand why we rely on status and give access to draft dashboards
+                # without roles take a look at:
+                #
+                #   - https://github.com/apache/superset/pull/24350#discussion_r1225061550
+                #   - https://github.com/apache/superset/pull/17511#issuecomment-975870169
+                #
+                not dashboard.published
+                or not dashboard.datasources
+                or any(
+                    self.can_access_datasource(datasource)
+                    for datasource in dashboard.datasources
+                )
+            ):
+                return
+
+            raise SupersetSecurityException(
+                self.get_dashboard_access_error_object(dashboard)
+            )
+
     def get_user_by_username(
         self, username: str, session: Session = None
     ) -> Optional[User]:
@@ -1991,55 +2046,6 @@ def get_rls_cache_key(self, datasource: "BaseDatasource") -> list[str]:
         guest_rls = self.get_guest_rls_filters_str(datasource)
         return guest_rls + rls_str
 
-    def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None:
-        """
-        Raise an exception if the user cannot access the dashboard.
-
-        This does not check for the required role/permission pairs, it only concerns
-        itself with entity relationships.
-
-        :param dashboard: Dashboard the user wants access to
-        :raises DashboardAccessDeniedError: If the user cannot access the resource
-        """
-
-        # pylint: disable=import-outside-toplevel
-        from superset import is_feature_enabled
-        from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
-
-        if self.is_guest_user():
-            # Guest user is currently used for embedded dashboards only. If the guest user
-            # doesn't have access to the dashboard, ignore all other checks.
-            if self.has_guest_access(dashboard):
-                return
-            raise DashboardAccessDeniedError()
-
-        if self.is_admin() or self.is_owner(dashboard):
-            return
-
-        # RBAC and legacy (datasource inferred) access controls.
-        if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
-            if dashboard.published and {role.id for role in dashboard.roles} & {
-                role.id for role in self.get_user_roles()
-            }:
-                return
-        elif (
-            # To understand why we rely on status and give access to draft dashboards
-            # without roles take a look at:
-            #
-            #   - https://github.com/apache/superset/pull/24350#discussion_r1225061550
-            #   - https://github.com/apache/superset/pull/17511#issuecomment-975870169
-            #
-            not dashboard.published
-            or not dashboard.datasources
-            or any(
-                self.can_access_datasource(datasource)
-                for datasource in dashboard.datasources
-            )
-        ):
-            return
-
-        raise DashboardAccessDeniedError()
-
     @staticmethod
     def _get_current_epoch_time() -> float:
         """This is used so the tests can mock time"""

superset/views/core.py

@@ -50,12 +50,16 @@
 from superset.daos.chart import ChartDAO
 from superset.daos.database import DatabaseDAO
 from superset.daos.datasource import DatasourceDAO
-from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
 from superset.dashboards.commands.importers.v0 import ImportDashboardsCommand
 from superset.dashboards.permalink.commands.get import GetDashboardPermalinkCommand
 from superset.dashboards.permalink.exceptions import DashboardPermalinkGetFailedError
 from superset.datasets.commands.exceptions import DatasetNotFoundError
-from superset.exceptions import CacheLoadError, DatabaseNotFound, SupersetException
+from superset.exceptions import (
+    CacheLoadError,
+    DatabaseNotFound,
+    SupersetException,
+    SupersetSecurityException,
+)
 from superset.explore.form_data.commands.create import CreateFormDataCommand
 from superset.explore.form_data.commands.get import GetFormDataCommand
 from superset.explore.form_data.commands.parameters import CommandParameters
@@ -866,8 +870,8 @@ def dashboard(
             abort(404)
 
         try:
-            security_manager.raise_for_dashboard_access(dashboard)
-        except DashboardAccessDeniedError as ex:
+            dashboard.raise_for_access()
+        except SupersetSecurityException as ex:
             return redirect_with_flash(
                 url="/dashboard/list/",
                 message=utils.error_msg_from_exception(ex),

tests/integration_tests/security/guest_token_security_tests.py

@@ -22,7 +22,6 @@
 
 from superset import db, security_manager
 from superset.daos.dashboard import EmbeddedDashboardDAO
-from superset.dashboards.commands.exceptions import DashboardAccessDeniedError
 from superset.exceptions import SupersetSecurityException
 from superset.models.dashboard import Dashboard
 from superset.security.guest_token import GuestTokenResourceType
@@ -162,19 +161,19 @@ def test_has_guest_access__unauthorized_guest_user__different_resource_type(self
     def test_raise_for_dashboard_access_as_guest(self):
         g.user = self.authorized_guest
 
-        security_manager.raise_for_dashboard_access(self.dash)
+        security_manager.raise_for_access(dashboard=self.dash)
 
-    def test_raise_for_dashboard_access_as_unauthorized_guest(self):
+    def test_raise_for_access_dashboard_as_unauthorized_guest(self):
         g.user = self.unauthorized_guest
 
-        with self.assertRaises(DashboardAccessDeniedError):
-            security_manager.raise_for_dashboard_access(self.dash)
+        with self.assertRaises(SupersetSecurityException):
+            security_manager.raise_for_access(dashboard=self.dash)
 
-    def test_raise_for_dashboard_access_as_guest_no_rbac(self):
+    def test_raise_for_access_dashboard_as_guest_no_rbac(self):
         """
         Test that guest account has no access to other dashboards.
 
-        A bug in the ``raise_for_dashboard_access`` logic allowed the guest user to
+        A bug in the ``raise_for_access`` logic allowed the guest user to
         fetch data from other dashboards, as long as the other dashboard:
 
           - was not embedded AND
@@ -193,8 +192,8 @@ def test_raise_for_dashboard_access_as_guest_no_rbac(self):
         db.session.add(dash)
         db.session.commit()
 
-        with self.assertRaises(DashboardAccessDeniedError):
-            security_manager.raise_for_dashboard_access(dash)
+        with self.assertRaises(SupersetSecurityException):
+            security_manager.raise_for_access(dashboard=dash)
 
         db.session.delete(dash)
         db.session.commit()