diff --git a/superset/security/manager.py b/superset/security/manager.py index 8ff7f2f23cd83..ac49642fcb889 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -348,30 +348,28 @@ def can_access_all_queries(self) -> bool: def can_access_all_datasources(self) -> bool: """ - Return True if the user can fully access all the Superset datasources, False - otherwise. + Return True if the user can access all the datasources, False otherwise. - :returns: Whether the user can fully access all Superset datasources + :returns: Whether the user can access all the datasources """ return self.can_access("all_datasource_access", "all_datasource_access") def can_access_all_databases(self) -> bool: """ - Return True if the user can fully access all the Superset databases, False - otherwise. + Return True if the user can access all the databases, False otherwise. - :returns: Whether the user can fully access all Superset databases + :returns: Whether the user can access all the databases """ return self.can_access("all_database_access", "all_database_access") def can_access_database(self, database: "Database") -> bool: """ - Return True if the user can fully access the Superset database, False otherwise. + Return True if the user can access the specified database, False otherwise. - :param database: The Superset database - :returns: Whether the user can fully access the Superset database + :param database: The database + :returns: Whether the user can access the database """ return ( @@ -382,11 +380,11 @@ def can_access_database(self, database: "Database") -> bool: def can_access_schema(self, datasource: "BaseDatasource") -> bool: """ - Return True if the user can fully access the schema associated with the Superset + Return True if the user can access the schema associated with specified datasource, False otherwise. - :param datasource: The Superset datasource - :returns: Whether the user can fully access the datasource's schema + :param datasource: The datasource + :returns: Whether the user can access the datasource's schema """ return ( @@ -397,11 +395,10 @@ def can_access_schema(self, datasource: "BaseDatasource") -> bool: def can_access_datasource(self, datasource: "BaseDatasource") -> bool: """ - Return True if the user can fully access of the Superset datasource, False - otherwise. + Return True if the user can access the specified datasource, False otherwise. - :param datasource: The Superset datasource - :returns: Whether the user can fully access the Superset datasource + :param datasource: The datasource + :returns: Whether the user can access the datasource """ try: @@ -411,6 +408,24 @@ def can_access_datasource(self, datasource: "BaseDatasource") -> bool: return True + def can_access_dashboard(self, dashboard: "Dashboard") -> bool: + """ + Return True if the user can access the specified dashboard, False otherwise. + + :param dashboard: The dashboard + :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: + return False + + return True + @staticmethod def get_datasource_access_error_msg(datasource: "BaseDatasource") -> str: """ @@ -1995,38 +2010,42 @@ def raise_for_user_activity_access(user_id: int) -> None: 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. + + 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 - def has_rbac_access() -> bool: - if not is_feature_enabled("DASHBOARD_RBAC") or not dashboard.roles: - return True - - return any( - dashboard_role.id - in [user_role.id for user_role in self.get_user_roles()] - for dashboard_role in dashboard.roles - ) - if self.is_guest_user() and dashboard.embedded: - can_access = self.has_guest_access(dashboard) + if self.has_guest_access(dashboard): + return else: - can_access = ( - self.is_admin() - or self.is_owner(dashboard) - or (dashboard.published and has_rbac_access()) - or (not dashboard.published and not dashboard.roles) - ) + 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 ( + not dashboard.published + or not dashboard.datasources + or any( + self.can_access_datasource(datasource) + for datasource in dashboard.datasources + ) + ): + return - if not can_access: - raise DashboardAccessDeniedError() + raise DashboardAccessDeniedError() @staticmethod def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index 4ecd2eca98679..9c21e3b5ec5a5 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -19,13 +19,10 @@ import time from collections.abc import Iterator from contextlib import contextmanager -from functools import wraps from typing import Any, Callable, TYPE_CHECKING from flask import current_app, Response -from superset import is_feature_enabled -from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.utils import core as utils from superset.utils.dates import now_as_float @@ -114,27 +111,3 @@ def wrapped(*args: Any, **kwargs: Any) -> Any: def on_security_exception(self: Any, ex: Exception) -> Response: return self.response(403, **{"message": utils.error_msg_from_exception(ex)}) - - -# noinspection PyPackageRequirements -def check_dashboard_access(on_error: Callable[[str], Any]) -> Callable[..., Any]: - def decorator(f: Callable[..., Any]) -> Callable[..., Any]: - @wraps(f) - def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: - # pylint: disable=import-outside-toplevel - from superset.models.dashboard import Dashboard - - dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"])) - if is_feature_enabled("DASHBOARD_RBAC"): - try: - current_app.appbuilder.sm.raise_for_dashboard_access(dashboard) - except DashboardAccessDeniedError as ex: - return on_error(str(ex)) - except Exception as exception: - raise exception - - return f(self, *args, dashboard=dashboard, **kwargs) - - return wrapper - - return decorator diff --git a/superset/views/core.py b/superset/views/core.py index 53e088ebe63d3..82c102e68e9e7 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -135,7 +135,6 @@ ReservedUrlParameters, ) from superset.utils.dates import now_as_float -from superset.utils.decorators import check_dashboard_access from superset.views.base import ( api, BaseSupersetView, @@ -190,7 +189,6 @@ "disable_data_preview", ] -DASHBOARD_LIST_URL = "/dashboard/list/" DATASOURCE_MISSING_ERR = __("The data source seems to have been deleted") USER_MISSING_ERR = __("The user seems to have been deleted") PARAMETER_MISSING_ERR = __( @@ -1641,76 +1639,57 @@ def favstar( # pylint: disable=no-self-use @has_access @expose("/dashboard//") @event_logger.log_this_with_extra_payload - @check_dashboard_access( - on_error=lambda msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger") - ) def dashboard( self, - dashboard_id_or_slug: str, # pylint: disable=unused-argument + dashboard_id_or_slug: str, add_extra_log_payload: Callable[..., None] = lambda **kwargs: None, - dashboard: Dashboard | None = None, ) -> FlaskResponse: """ - Server side rendering for a dashboard - :param dashboard_id_or_slug: identifier for dashboard. used in the decorators + Server side rendering for a dashboard. + + :param dashboard_id_or_slug: identifier for dashboard :param add_extra_log_payload: added by `log_this_with_manual_updates`, set a default value to appease pylint - :param dashboard: added by `check_dashboard_access` """ + + dashboard = Dashboard.get(dashboard_id_or_slug) + if not dashboard: abort(404) - assert dashboard is not None - - has_access_ = False - for datasource in dashboard.datasources: - datasource = DatasourceDAO.get_datasource( - datasource_type=DatasourceType(datasource.type), - datasource_id=datasource.id, - session=db.session(), + try: + security_manager.raise_for_dashboard_access(dashboard) + except DashboardAccessDeniedError as ex: + return redirect_with_flash( + url="/dashboard/list/", + message=utils.error_msg_from_exception(ex), + category="danger", ) - if datasource and security_manager.can_access_datasource( - datasource=datasource, - ): - has_access_ = True - - if has_access_: - break - - if dashboard.datasources and not has_access_: - flash(DashboardAccessDeniedError.message, "danger") - return redirect(DASHBOARD_LIST_URL) - - dash_edit_perm = security_manager.is_owner( - dashboard - ) and security_manager.can_access("can_save_dash", "Superset") - edit_mode = ( - request.args.get(utils.ReservedUrlParameters.EDIT_MODE.value) == "true" - ) - - standalone_mode = ReservedUrlParameters.is_standalone_mode() add_extra_log_payload( dashboard_id=dashboard.id, dashboard_version="v2", - dash_edit_perm=dash_edit_perm, - edit_mode=edit_mode, + dash_edit_perm=( + security_manager.is_owner(dashboard) + and security_manager.can_access("can_save_dash", "Superset") + ), + edit_mode=( + request.args.get(ReservedUrlParameters.EDIT_MODE.value) == "true" + ), ) - bootstrap_data = { - "user": bootstrap_user_data(g.user, include_perms=True), - "common": common_bootstrap_payload(g.user), - } - return self.render_template( "superset/spa.html", entry="spa", - # dashboard title is always visible - title=dashboard.dashboard_title, + title=dashboard.dashboard_title, # dashboard title is always visible bootstrap_data=json.dumps( - bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser + { + "user": bootstrap_user_data(g.user, include_perms=True), + "common": common_bootstrap_payload(g.user), + }, + default=utils.pessimistic_json_iso_dttm_ser, ), - standalone_mode=standalone_mode, + standalone_mode=ReservedUrlParameters.is_standalone_mode(), ) @has_access diff --git a/tests/integration_tests/base_tests.py b/tests/integration_tests/base_tests.py index 0a9910266d184..c2b34b037cbaf 100644 --- a/tests/integration_tests/base_tests.py +++ b/tests/integration_tests/base_tests.py @@ -504,6 +504,48 @@ def put_assert_metric( def get_dttm(cls): return datetime.strptime("2019-01-02 03:04:05.678900", "%Y-%m-%d %H:%M:%S.%f") + def insert_dashboard( + self, + dashboard_title: str, + slug: Optional[str], + owners: list[int], + roles: list[int] = [], + created_by=None, + slices: Optional[list[Slice]] = None, + position_json: str = "", + css: str = "", + json_metadata: str = "", + published: bool = False, + certified_by: Optional[str] = None, + certification_details: Optional[str] = None, + ) -> Dashboard: + obj_owners = list() + obj_roles = list() + slices = slices or [] + for owner in owners: + user = db.session.query(security_manager.user_model).get(owner) + obj_owners.append(user) + for role in roles: + role_obj = db.session.query(security_manager.role_model).get(role) + obj_roles.append(role_obj) + dashboard = Dashboard( + dashboard_title=dashboard_title, + slug=slug, + owners=obj_owners, + roles=obj_roles, + position_json=position_json, + css=css, + json_metadata=json_metadata, + slices=slices, + published=published, + created_by=created_by, + certified_by=certified_by, + certification_details=certification_details, + ) + db.session.add(dashboard) + db.session.commit() + return dashboard + @contextmanager def db_insert_temp_object(obj: DeclarativeMeta): diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 49a6bbecbc85f..e30c5bb3781d2 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -19,7 +19,6 @@ import json from io import BytesIO from time import sleep -from typing import Optional from unittest.mock import ANY, patch from zipfile import is_zipfile, ZipFile @@ -76,48 +75,6 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi "published": False, } - def insert_dashboard( - self, - dashboard_title: str, - slug: Optional[str], - owners: list[int], - roles: list[int] = [], - created_by=None, - slices: Optional[list[Slice]] = None, - position_json: str = "", - css: str = "", - json_metadata: str = "", - published: bool = False, - certified_by: Optional[str] = None, - certification_details: Optional[str] = None, - ) -> Dashboard: - obj_owners = list() - obj_roles = list() - slices = slices or [] - for owner in owners: - user = db.session.query(security_manager.user_model).get(owner) - obj_owners.append(user) - for role in roles: - role_obj = db.session.query(security_manager.role_model).get(role) - obj_roles.append(role_obj) - dashboard = Dashboard( - dashboard_title=dashboard_title, - slug=slug, - owners=obj_owners, - roles=obj_roles, - position_json=position_json, - css=css, - json_metadata=json_metadata, - slices=slices, - published=published, - created_by=created_by, - certified_by=certified_by, - certification_details=certification_details, - ) - db.session.add(dashboard) - db.session.commit() - return dashboard - @pytest.fixture() def create_dashboards(self): with self.create_app().app_context(): @@ -506,43 +463,6 @@ def test_get_dashboard_no_data_access(self): db.session.delete(dashboard) db.session.commit() - def test_get_draft_dashboard_without_roles_by_uuid(self): - """ - Dashboard API: Test get draft dashboard without roles by uuid - """ - admin = self.get_user("admin") - dashboard = self.insert_dashboard("title", "slug1", [admin.id]) - assert not dashboard.published - assert dashboard.roles == [] - - self.login(username="gamma") - uri = f"api/v1/dashboard/{dashboard.uuid}" - rv = self.client.get(uri) - assert rv.status_code == 200 - # rollback changes - db.session.delete(dashboard) - db.session.commit() - - def test_cannot_get_draft_dashboard_with_roles_by_uuid(self): - """ - Dashboard API: Test get dashboard by uuid - """ - admin = self.get_user("admin") - admin_role = self.get_role("Admin") - dashboard = self.insert_dashboard( - "title", "slug1", [admin.id], roles=[admin_role.id] - ) - assert not dashboard.published - assert dashboard.roles == [admin_role] - - self.login(username="gamma") - uri = f"api/v1/dashboard/{dashboard.uuid}" - rv = self.client.get(uri) - assert rv.status_code == 403 - # rollback changes - db.session.delete(dashboard) - db.session.commit() - def test_get_dashboards_changed_on(self): """ Dashboard API: Test get dashboards changed on diff --git a/tests/integration_tests/dashboards/security/security_dataset_tests.py b/tests/integration_tests/dashboards/security/security_dataset_tests.py index 2eafc4b53e0cd..875eb9145e77a 100644 --- a/tests/integration_tests/dashboards/security/security_dataset_tests.py +++ b/tests/integration_tests/dashboards/security/security_dataset_tests.py @@ -22,6 +22,7 @@ from flask import escape from superset import app +from superset.dashboards.dao import DashboardDAO from superset.models import core as models from tests.integration_tests.dashboards.base_case import DashboardTestCase from tests.integration_tests.dashboards.consts import * @@ -223,7 +224,7 @@ def test_get_dashboards_api_no_data_access(self): """ admin = self.get_user("admin") title = f"title{random_str()}" - create_dashboard_to_db(title, "slug1", owners=[admin]) + dashboard = create_dashboard_to_db(title, "slug1", owners=[admin]) self.login(username="gamma") arguments = { @@ -234,3 +235,4 @@ def test_get_dashboards_api_no_data_access(self): self.assert200(rv) data = json.loads(rv.data.decode("utf-8")) self.assertEqual(0, data["count"]) + DashboardDAO.delete(dashboard) diff --git a/tests/integration_tests/dashboards/security/security_rbac_tests.py b/tests/integration_tests/dashboards/security/security_rbac_tests.py index a49e533f1ba41..03b5da7ce34e9 100644 --- a/tests/integration_tests/dashboards/security/security_rbac_tests.py +++ b/tests/integration_tests/dashboards/security/security_rbac_tests.py @@ -395,3 +395,40 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards # post for dash in published_dashboards + draft_dashboards: revoke_access_to_dashboard(dash, "Public") + + def test_get_draft_dashboard_without_roles_by_uuid(self): + """ + Dashboard API: Test get draft dashboard without roles by uuid + """ + admin = self.get_user("admin") + dashboard = self.insert_dashboard("title", "slug1", [admin.id]) + assert not dashboard.published + assert dashboard.roles == [] + + self.login(username="gamma") + uri = f"api/v1/dashboard/{dashboard.uuid}" + rv = self.client.get(uri) + assert rv.status_code == 200 + # rollback changes + db.session.delete(dashboard) + db.session.commit() + + def test_cannot_get_draft_dashboard_with_roles_by_uuid(self): + """ + Dashboard API: Test get dashboard by uuid + """ + admin = self.get_user("admin") + admin_role = self.get_role("Admin") + dashboard = self.insert_dashboard( + "title", "slug1", [admin.id], roles=[admin_role.id] + ) + assert not dashboard.published + assert dashboard.roles == [admin_role] + + self.login(username="gamma") + uri = f"api/v1/dashboard/{dashboard.uuid}" + rv = self.client.get(uri) + assert rv.status_code == 403 + # rollback changes + db.session.delete(dashboard) + db.session.commit()