diff --git a/superset/dashboards/commands/exceptions.py b/superset/dashboards/commands/exceptions.py index 03413aa25e392..ee85c1f391808 100644 --- a/superset/dashboards/commands/exceptions.py +++ b/superset/dashboards/commands/exceptions.py @@ -75,3 +75,7 @@ class DashboardForbiddenError(ForbiddenError): class DashboardImportError(ImportFailedError): message = _("Import dashboard failed for an unknown reason") + + +class DashboardAccessDeniedError(ForbiddenError): + message = _("You don't have access to this dashboard.") diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 3606da8c5cb80..a63594371e910 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -14,12 +14,15 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations + import json import logging from functools import partial from typing import Any, Callable, Dict, List, Set, Union import sqlalchemy as sqla +from flask import g from flask_appbuilder import Model from flask_appbuilder.models.decorators import renders from flask_appbuilder.security.sqla.models import User @@ -45,6 +48,7 @@ from superset.connectors.base.models import BaseDatasource from superset.connectors.druid.models import DruidColumn, DruidMetric from superset.connectors.sqla.models import SqlMetric, TableColumn +from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.extensions import cache_manager from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.models.slice import Slice @@ -356,6 +360,17 @@ def export_dashboards( # pylint: disable=too-many-locals indent=4, ) + @classmethod + def get(cls, id_or_slug: str) -> Dashboard: + session = db.session() + qry = session.query(Dashboard) + if id_or_slug.isdigit(): + qry = qry.filter_by(id=int(id_or_slug)) + else: + qry = qry.filter_by(slug=id_or_slug) + + return qry.one_or_none() + OnDashboardChange = Callable[[Mapper, Connection, Dashboard], Any] @@ -407,3 +422,22 @@ def clear_dashboard_cache( sqla.event.listen(TableColumn, "after_update", clear_dashboard_cache) sqla.event.listen(DruidMetric, "after_update", clear_dashboard_cache) sqla.event.listen(DruidColumn, "after_update", clear_dashboard_cache) + + +def raise_for_dashboard_access(dashboard: Dashboard) -> None: + from superset.views.base import get_user_roles, is_user_admin + from superset.views.utils import is_owner + + if is_feature_enabled("DASHBOARD_RBAC"): + has_rbac_access = any( + dashboard_role.id in [user_role.id for user_role in get_user_roles()] + for dashboard_role in dashboard.roles + ) + can_access = ( + is_user_admin() + or is_owner(dashboard, g.user) + or (dashboard.published and has_rbac_access) + ) + + if not can_access: + raise DashboardAccessDeniedError() diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index 014a512f0bedc..e5cf713696fdd 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -15,11 +15,16 @@ # specific language governing permissions and limitations # under the License. import time +from functools import wraps from typing import Any, Callable, Dict, Iterator, Union from contextlib2 import contextmanager +from flask import Response +from superset import is_feature_enabled +from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.stats_logger import BaseStatsLogger +from superset.utils import core as utils from superset.utils.dates import now_as_float @@ -69,3 +74,35 @@ def wrapped(*args: Any, **kwargs: Any) -> Any: return wrapped return decorate + + +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[..., Any] = on_security_exception +) -> Callable[..., Any]: + def decorator(f: Callable[..., Any]) -> Callable[..., Any]: + @wraps(f) + def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: + from superset.models.dashboard import ( + Dashboard, + raise_for_dashboard_access, + ) + + dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"])) + if is_feature_enabled("DASHBOARD_RBAC"): + try: + raise_for_dashboard_access(dashboard) + except DashboardAccessDeniedError as ex: + return on_error(self, 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 47df3e865e318..cf0ca7d4dddd5 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -104,6 +104,7 @@ from superset.utils.async_query_manager import AsyncQueryTokenException from superset.utils.cache import etag_cache from superset.utils.dates import now_as_float +from superset.utils.decorators import check_dashboard_access from superset.views.base import ( api, BaseSupersetView, @@ -160,7 +161,6 @@ "id", ] - DATASOURCE_MISSING_ERR = __("The data source seems to have been deleted") USER_MISSING_ERR = __("The user seems to have been deleted") PARAMETER_MISSING_ERR = ( @@ -867,7 +867,8 @@ def remove_extra_filters(filters: List[Dict[str, Any]]) -> List[Dict[str, Any]]: Those should not be saved when saving the chart""" return [f for f in filters if not f.get("isExtra")] - def save_or_overwrite_slice( # pylint: disable=too-many-arguments,too-many-locals,no-self-use + def save_or_overwrite_slice( + # pylint: disable=too-many-arguments,too-many-locals,no-self-use self, slc: Optional[Slice], slice_add_perm: bool, @@ -1785,33 +1786,34 @@ def publish( # pylint: disable=no-self-use @has_access @expose("/dashboard//") @event_logger.log_this_with_extra_payload + @check_dashboard_access( + on_error=lambda self, ex: Response( + utils.error_msg_from_exception(ex), status=403 + ) + ) def dashboard( # pylint: disable=too-many-locals - self, - dashboard_id_or_slug: str, - # this parameter is added by `log_this_with_manual_updates`, - # set a default value to appease pylint + self, # pylint: disable=no-self-use + dashboard_id_or_slug: str, # pylint: disable=unused-argument add_extra_log_payload: Callable[..., None] = lambda **kwargs: None, + dashboard: Optional[Dashboard] = None, ) -> FlaskResponse: - """Server side rendering for a dashboard""" - session = db.session() - qry = session.query(Dashboard) - if dashboard_id_or_slug.isdigit(): - qry = qry.filter_by(id=int(dashboard_id_or_slug)) - else: - qry = qry.filter_by(slug=dashboard_id_or_slug) - - dash = qry.one_or_none() - if not dash: + """ + Server side rendering for a dashboard + :param dashboard_id_or_slug: identifier for dashboard. used in the decorators + :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` + """ + if not dashboard: abort(404) - data = dash.full_data() + data = dashboard.full_data() if config["ENABLE_ACCESS_REQUEST"]: for datasource in data["datasources"].values(): datasource = ConnectorRegistry.get_datasource( datasource_type=datasource["type"], datasource_id=datasource["id"], - session=session, + session=db.session(), ) if datasource and not security_manager.can_access_datasource( datasource=datasource @@ -1822,10 +1824,12 @@ def dashboard( # pylint: disable=too-many-locals ), "danger", ) - return redirect(f"/superset/request_access/?dashboard_id={dash.id}") + return redirect( + f"/superset/request_access/?dashboard_id={dashboard.id}" + ) dash_edit_perm = check_ownership( - dash, raise_if_false=False + dashboard, raise_if_false=False ) and security_manager.can_access("can_save_dash", "Superset") dash_save_perm = security_manager.can_access("can_save_dash", "Superset") superset_can_explore = security_manager.can_access("can_explore", "Superset") @@ -1840,7 +1844,7 @@ def dashboard( # pylint: disable=too-many-locals ) add_extra_log_payload( - dashboard_id=dash.id, + dashboard_id=dashboard.id, dashboard_version="v2", dash_edit_perm=dash_edit_perm, edit_mode=edit_mode, @@ -1885,8 +1889,8 @@ def dashboard( # pylint: disable=too-many-locals "superset/dashboard.html", entry="dashboard", standalone_mode=standalone_mode, - title=dash.dashboard_title, - custom_css=dash.css, + title=dashboard.dashboard_title, + custom_css=dashboard.css, bootstrap_data=json.dumps( bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser ), @@ -2238,7 +2242,8 @@ def stop_query(self) -> FlaskResponse: @has_access_api @event_logger.log_this @expose("/validate_sql_json/", methods=["POST", "GET"]) - def validate_sql_json( # pylint: disable=too-many-locals,too-many-return-statements,no-self-use + def validate_sql_json( + # pylint: disable=too-many-locals,too-many-return-statements,no-self-use self, ) -> FlaskResponse: """Validates that arbitrary sql is acceptable for the given database. diff --git a/tests/dashboards/security/security_dataset_tests.py b/tests/dashboards/security/security_dataset_tests.py index 842f5c09f3968..9edc2ab0a12dc 100644 --- a/tests/dashboards/security/security_dataset_tests.py +++ b/tests/dashboards/security/security_dataset_tests.py @@ -160,7 +160,7 @@ def test_get_dashboards__user_can_not_view_unpublished_dash(self): # arrange admin_user = security_manager.find_user(ADMIN_USERNAME) gamma_user = security_manager.find_user(GAMMA_USERNAME) - admin_and_not_published_dashboard = create_dashboard_to_db( + admin_and_draft_dashboard = create_dashboard_to_db( dashboard_title="admin_owned_unpublished_dash", owners=[admin_user] ) @@ -171,7 +171,7 @@ def test_get_dashboards__user_can_not_view_unpublished_dash(self): # assert self.assertNotIn( - admin_and_not_published_dashboard.url, get_dashboards_response_as_gamma + admin_and_draft_dashboard.url, get_dashboards_response_as_gamma ) @pytest.mark.usefixtures("load_energy_table_with_slice", "load_dashboard") diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index efd2c990f314c..41a01fadec174 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -31,6 +31,136 @@ "superset.extensions.feature_flag_manager._feature_flags", DASHBOARD_RBAC=True, ) class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): + def test_get_dashboard_view__admin_can_access(self): + # arrange + dashboard_to_access = create_dashboard_to_db( + owners=[], slices=[create_slice_to_db()], published=False + ) + self.login("admin") + + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert_dashboard_view_response(response, dashboard_to_access) + + def test_get_dashboard_view__owner_can_access(self): + # arrange + username = random_str() + new_role = f"role_{random_str()}" + owner = self.create_user_with_roles( + username, [new_role], should_create_roles=True + ) + dashboard_to_access = create_dashboard_to_db( + owners=[owner], slices=[create_slice_to_db()], published=False + ) + self.login(username) + + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert_dashboard_view_response(response, dashboard_to_access) + + def test_get_dashboard_view__user_can_not_access_without_permission(self): + username = random_str() + new_role = f"role_{random_str()}" + self.create_user_with_roles(username, [new_role], should_create_roles=True) + dashboard_to_access = create_dashboard_to_db(published=True) + self.login(username) + + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert403(response) + + def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft( + self, + ): + # arrange + dashboard_to_access = create_dashboard_to_db(published=False) + username = random_str() + new_role = f"role_{random_str()}" + self.create_user_with_roles(username, [new_role], should_create_roles=True) + grant_access_to_dashboard(dashboard_to_access, new_role) + self.login(username) + + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert403(response) + + # post + revoke_access_to_dashboard(dashboard_to_access, new_role) + + def test_get_dashboard_view__user_access_with_dashboard_permission(self): + # arrange + + username = random_str() + new_role = f"role_{random_str()}" + self.create_user_with_roles(username, [new_role], should_create_roles=True) + + dashboard_to_access = create_dashboard_to_db( + published=True, slices=[create_slice_to_db()] + ) + self.login(username) + grant_access_to_dashboard(dashboard_to_access, new_role) + + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert_dashboard_view_response(response, dashboard_to_access) + + # post + revoke_access_to_dashboard(dashboard_to_access, new_role) + + def test_get_dashboard_view__public_user_can_not_access_without_permission(self): + dashboard_to_access = create_dashboard_to_db(published=True) + self.logout() + + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert403(response) + + def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft( + self, + ): + # arrange + dashboard_to_access = create_dashboard_to_db(published=False) + grant_access_to_dashboard(dashboard_to_access, "Public") + self.logout() + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert403(response) + + # post + revoke_access_to_dashboard(dashboard_to_access, "Public") + + def test_get_dashboard_view__public_user_access_with_dashboard_permission(self): + # arrange + dashboard_to_access = create_dashboard_to_db( + published=True, slices=[create_slice_to_db()] + ) + grant_access_to_dashboard(dashboard_to_access, "Public") + + self.logout() + + # act + response = self.get_dashboard_view_response(dashboard_to_access) + + # assert + self.assert_dashboard_view_response(response, dashboard_to_access) + + # post + revoke_access_to_dashboard(dashboard_to_access, "Public") + def test_get_dashboards_list__admin_get_all_dashboards(self): # arrange create_dashboard_to_db( @@ -48,6 +178,20 @@ def test_get_dashboards_list__admin_get_all_dashboards(self): def test_get_dashboards_list__owner_get_all_owned_dashboards(self): # arrange + ( + not_owned_dashboards, + owned_dashboards, + ) = self._create_sample_dashboards_with_owner_access() + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response( + response, 2, owned_dashboards, not_owned_dashboards + ) + + def _create_sample_dashboards_with_owner_access(self): username = random_str() new_role = f"role_{random_str()}" owner = self.create_user_with_roles( @@ -67,16 +211,8 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): slices=[create_slice_to_db(datasource_id=table.id)], published=True ) ] - self.login(username) - - # act - response = self.get_dashboards_list_response() - - # assert - self.assert_dashboards_list_view_response( - response, 2, owned_dashboards, not_owned_dashboards - ) + return not_owned_dashboards, owned_dashboards def test_get_dashboards_list__user_without_any_permissions_get_empty_list(self): @@ -96,38 +232,40 @@ def test_get_dashboards_list__user_without_any_permissions_get_empty_list(self): def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self): # arrange + ( + new_role, + draft_dashboards, + published_dashboards, + ) = self._create_sample_only_published_dashboard_with_roles() + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response( + response, len(published_dashboards), published_dashboards, draft_dashboards, + ) + + # post + for dash in published_dashboards + draft_dashboards: + revoke_access_to_dashboard(dash, new_role) + + def _create_sample_only_published_dashboard_with_roles(self): username = random_str() new_role = f"role_{random_str()}" self.create_user_with_roles(username, [new_role], should_create_roles=True) - published_dashboards = [ create_dashboard_to_db(published=True), create_dashboard_to_db(published=True), ] - not_published_dashboards = [ + draft_dashboards = [ create_dashboard_to_db(published=False), create_dashboard_to_db(published=False), ] - - for dash in published_dashboards + not_published_dashboards: + for dash in published_dashboards + draft_dashboards: grant_access_to_dashboard(dash, new_role) - self.login(username) - - # act - response = self.get_dashboards_list_response() - - # assert - self.assert_dashboards_list_view_response( - response, - len(published_dashboards), - published_dashboards, - not_published_dashboards, - ) - - # post - for dash in published_dashboards + not_published_dashboards: - revoke_access_to_dashboard(dash, new_role) + return new_role, draft_dashboards, published_dashboards def test_get_dashboards_list__public_user_without_any_permissions_get_empty_list( self, @@ -148,27 +286,26 @@ def test_get_dashboards_list__public_user_get_only_published_permitted_dashboard create_dashboard_to_db(published=True), create_dashboard_to_db(published=True), ] - not_published_dashboards = [ + draft_dashboards = [ create_dashboard_to_db(published=False), create_dashboard_to_db(published=False), ] - for dash in published_dashboards + not_published_dashboards: + for dash in published_dashboards + draft_dashboards: grant_access_to_dashboard(dash, "Public") + self.logout() + # act response = self.get_dashboards_list_response() # assert self.assert_dashboards_list_view_response( - response, - len(published_dashboards), - published_dashboards, - not_published_dashboards, + response, len(published_dashboards), published_dashboards, draft_dashboards, ) # post - for dash in published_dashboards + not_published_dashboards: + for dash in published_dashboards + draft_dashboards: revoke_access_to_dashboard(dash, "Public") def test_get_dashboards_api__admin_get_all_dashboards(self): @@ -188,27 +325,10 @@ def test_get_dashboards_api__admin_get_all_dashboards(self): def test_get_dashboards_api__owner_get_all_owned_dashboards(self): # arrange - username = random_str() - new_role = f"role_{random_str()}" - owner = self.create_user_with_roles( - username, [new_role], should_create_roles=True - ) - database = create_database_to_db() - table = create_datasource_table_to_db(db_id=database.id, owners=[owner]) - first_dash = create_dashboard_to_db( - owners=[owner], slices=[create_slice_to_db(datasource_id=table.id)] - ) - second_dash = create_dashboard_to_db( - owners=[owner], slices=[create_slice_to_db(datasource_id=table.id)] - ) - owned_dashboards = [first_dash, second_dash] - not_owned_dashboards = [ - create_dashboard_to_db( - slices=[create_slice_to_db(datasource_id=table.id)], published=True - ) - ] - - self.login(username) + ( + not_owned_dashboards, + owned_dashboards, + ) = self._create_sample_dashboards_with_owner_access() # act response = self.get_dashboards_api_response() @@ -232,43 +352,29 @@ def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): self.assert_dashboards_api_response(response, 0) def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): - username = random_str() - new_role = f"role_{random_str()}" - self.create_user_with_roles(username, [new_role], should_create_roles=True) - # arrange - published_dashboards = [ - create_dashboard_to_db(published=True), - create_dashboard_to_db(published=True), - ] - not_published_dashboards = [ - create_dashboard_to_db(published=False), - create_dashboard_to_db(published=False), - ] - - for dash in published_dashboards + not_published_dashboards: - grant_access_to_dashboard(dash, new_role) - - self.login(username) + ( + new_role, + draft_dashboards, + published_dashboards, + ) = self._create_sample_only_published_dashboard_with_roles() # act response = self.get_dashboards_api_response() # assert self.assert_dashboards_api_response( - response, - len(published_dashboards), - published_dashboards, - not_published_dashboards, + response, len(published_dashboards), published_dashboards, draft_dashboards, ) # post - for dash in published_dashboards + not_published_dashboards: + for dash in published_dashboards + draft_dashboards: revoke_access_to_dashboard(dash, new_role) def test_get_dashboards_api__public_user_without_any_permissions_get_empty_list( self, ): create_dashboard_to_db(published=True) + self.logout() # act response = self.get_dashboards_api_response() @@ -284,25 +390,24 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards create_dashboard_to_db(published=True), create_dashboard_to_db(published=True), ] - not_published_dashboards = [ + draft_dashboards = [ create_dashboard_to_db(published=False), create_dashboard_to_db(published=False), ] - for dash in published_dashboards + not_published_dashboards: + for dash in published_dashboards + draft_dashboards: grant_access_to_dashboard(dash, "Public") + self.logout() + # act response = self.get_dashboards_api_response() # assert self.assert_dashboards_api_response( - response, - len(published_dashboards), - published_dashboards, - not_published_dashboards, + response, len(published_dashboards), published_dashboards, draft_dashboards, ) # post - for dash in published_dashboards + not_published_dashboards: + for dash in published_dashboards + draft_dashboards: revoke_access_to_dashboard(dash, "Public")