From 5522facdc6f22212eff82ff0d602d413da9d5613 Mon Sep 17 00:00:00 2001 From: John Bodley <4567245+john-bodley@users.noreply.github.com> Date: Wed, 9 Aug 2023 09:25:58 -0700 Subject: [PATCH] chore: Refactor dashboard security access (#24804) --- .../src/components/ErrorMessage/types.ts | 1 + superset/daos/dashboard.py | 13 +- superset/errors.py | 2 +- superset/models/dashboard.py | 9 ++ superset/security/manager.py | 116 +++++++++--------- superset/views/core.py | 12 +- .../security/guest_token_security_tests.py | 17 ++- 7 files changed, 98 insertions(+), 72 deletions(-) diff --git a/superset-frontend/src/components/ErrorMessage/types.ts b/superset-frontend/src/components/ErrorMessage/types.ts index 87ef4a1bc..d3fe5bfdf 100644 --- a/superset-frontend/src/components/ErrorMessage/types.ts +++ b/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', diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 69169e1d0..51f90709d 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -26,10 +26,12 @@ from flask_appbuilder.models.sqla import Model 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 @@ from superset.dashboards.filter_sets.consts import ( 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 @@ class DashboardDAO(BaseDAO[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 diff --git a/superset/errors.py b/superset/errors.py index 6661e9818..167ec2d3b 100644 --- a/superset/errors.py +++ b/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" diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 20f2f7e68..0fecf15a5 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -446,6 +446,15 @@ class Dashboard(Model, AuditMixinNullable, ImportExportMixin): 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: diff --git a/superset/security/manager.py b/superset/security/manager.py index c265bf2f7..b996188a8 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -410,16 +410,30 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods :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 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods 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 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods """ # 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 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods 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 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods 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""" diff --git a/superset/views/core.py b/superset/views/core.py index 79fb3fbd8..8a509783f 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -50,12 +50,16 @@ from superset.connectors.sqla.models import SqlaTable 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 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods 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), diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 73dcb1b93..cb6095c0e 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/tests/integration_tests/security/guest_token_security_tests.py @@ -22,7 +22,6 @@ from flask import g 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 @@ class TestGuestUserDashboardAccess(SupersetTestCase): 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 @@ class TestGuestUserDashboardAccess(SupersetTestCase): 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()