chore: Refactor dashboard security access

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

@@ -410,16 +410,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:
  """
@@ -1757,8 +1771,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,
@@ -1779,6 +1794,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
@@ -1852,6 +1868,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]:
@@ -1987,55 +2042,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
@@ -863,8 +867,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()