You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2023/08/10 16:16:38 UTC
[superset] 04/10: chore: Refactor dashboard security access (#24804)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 804cc36080f200a48ac21f7d7187d19002967c74
Author: John Bodley <45...@users.noreply.github.com>
AuthorDate: Wed Aug 9 09:25:58 2023 -0700
chore: Refactor dashboard security access (#24804)
(cherry picked from commit 5522facdc6f22212eff82ff0d602d413da9d5613)
---
.../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 87ef4a1bc4..d3fe5bfdf7 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 69169e1d02..51f90709da 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 6661e98183..167ec2d3b4 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 20f2f7e685..0fecf15a55 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 f57e730af0..7db81a8415 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -412,16 +412,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:
"""
@@ -1761,8 +1775,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,
@@ -1783,6 +1798,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
@@ -1856,6 +1872,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]:
@@ -1991,55 +2046,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 4508ec70c0..1b50a59e36 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
@@ -866,8 +870,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 73dcb1b932..cb6095c0e7 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()