iop
/
superset
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

chore: Refactor dashboard security access (#24804)

This commit is contained in:
John Bodley 2023-08-09 09:25:58 -07:00 committed by GitHub
parent ec9e9a46f2
commit 5522facdc6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 98 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
superset-frontend/src/components/ErrorMessage/types.ts
View File

@ -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',

13
superset/daos/dashboard.py
View File

@ -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

2
superset/errors.py
View File

@ -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"

9
superset/models/dashboard.py
View File

@ -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:

116
superset/security/manager.py
View File

@ -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"""

12
superset/views/core.py
View File

@ -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),

17
tests/integration_tests/security/guest_token_security_tests.py
View File

@ -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()