From fe2fe5ae63561e8d7a0f729edc20514c2bf18926 Mon Sep 17 00:00:00 2001 From: John Bodley Date: Tue, 16 May 2023 13:42:52 -0700 Subject: [PATCH] Revert "fix(dashboard-rbac): use normal rbac when no roles chosen (#23586)" This reverts commit a8230336fffd87b1f6341896302d46d2cd12c818. --- .../creating-your-first-dashboard.mdx | 5 +- .../components/PropertiesModal/index.tsx | 2 +- superset/security/manager.py | 18 ++---- superset/utils/decorators.py | 6 +- superset/views/core.py | 51 +++++++--------- superset/views/dashboard/mixin.py | 2 +- superset/views/utils.py | 8 +-- .../security/security_rbac_tests.py | 59 +++---------------- 8 files changed, 44 insertions(+), 107 deletions(-) diff --git a/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx b/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx index 23a6b777b916d..aeea6c2014397 100644 --- a/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx +++ b/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx @@ -184,8 +184,9 @@ Non-owner users access can be managed two different ways: 1. Dataset permissions - if you add to the relevant role permissions to datasets it automatically grants implicit access to all dashboards that uses those permitted datasets 2. Dashboard roles - if you enable **DASHBOARD_RBAC** [feature flag](https://superset.apache.org/docs/installation/configuring-superset#feature-flags) then you be able to manage which roles can access the dashboard - - Granting a role access to a dashboard will bypass dataset level checks. Having dashboard access implicitly grants read access to all the featured charts in the dashboard, and thereby also all the associated datasets. - - If no roles are specified for a dashboard, regular **Dataset permissions** will apply. +- Having dashboard access implicitly grants read access to the associated datasets, therefore +all charts will load their data even if feature flag is turned on and no roles assigned +to roles the access will fallback to **Dataset permissions** diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 651985232463a..9b044519d4658 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -560,7 +560,7 @@ const PropertiesModal = ({

{t( - 'Roles is a list which defines access to the dashboard. Granting a role access to a dashboard will bypass dataset level checks. If no roles are defined, regular access permissions apply.', + 'Roles is a list which defines access to the dashboard. Granting a role access to a dashboard will bypass dataset level checks. If no roles are defined, then the dashboard is available to all roles.', )}

diff --git a/superset/security/manager.py b/superset/security/manager.py index 754a91d9077aa..9e7a8bbd4bb10 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2046,10 +2046,7 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: 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( + return (not is_feature_enabled("DASHBOARD_RBAC")) or any( dashboard_role.id in [user_role.id for user_role in self.get_user_roles()] for dashboard_role in dashboard.roles @@ -2076,18 +2073,15 @@ def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: from superset.models.dashboard import Dashboard from superset.models.slice import Slice - dashboard_ids = db.session.query(Dashboard.id) - dashboard_ids = DashboardAccessFilter( - "id", SQLAInterface(Dashboard, db.session) - ).apply(dashboard_ids, None) - datasource_class = type(datasource) query = ( - db.session.query(Dashboard.id) - .join(Slice, Dashboard.slices) + db.session.query(datasource_class) .join(Slice.table) .filter(datasource_class.id == datasource.id) - .filter(Dashboard.id.in_(dashboard_ids)) + ) + + query = DashboardAccessFilter("id", SQLAInterface(Dashboard, db.session)).apply( + query, None ) exists = db.session.query(query.exists()).scalar() diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index e77a5599057a1..7852c4ff74725 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -116,7 +116,9 @@ def on_security_exception(self: Any, ex: Exception) -> Response: # noinspection PyPackageRequirements -def check_dashboard_access(on_error: Callable[[str], Any]) -> Callable[..., Any]: +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: @@ -128,7 +130,7 @@ def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: try: current_app.appbuilder.sm.raise_for_dashboard_access(dashboard) except DashboardAccessDeniedError as ex: - return on_error(str(ex)) + return on_error(self, ex) except Exception as exception: raise exception diff --git a/superset/views/core.py b/superset/views/core.py index 7576f042e8ea5..6559db125422e 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -66,7 +66,6 @@ TableColumn, ) from superset.constants import QUERY_EARLY_CANCEL_KEY -from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.dashboards.commands.importers.v0 import ImportDashboardsCommand from superset.dashboards.dao import DashboardDAO from superset.dashboards.permalink.commands.get import GetDashboardPermalinkCommand @@ -167,7 +166,6 @@ get_form_data, get_viz, loads_request_json, - redirect_with_flash, sanitize_datasource_data, ) from superset.viz import BaseViz @@ -193,7 +191,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 = __( @@ -1828,7 +1825,9 @@ def favstar( # pylint: disable=no-self-use @expose("/dashboard//") @event_logger.log_this_with_extra_payload @check_dashboard_access( - on_error=lambda msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger") + on_error=lambda self, ex: Response( + utils.error_msg_from_exception(ex), status=403 + ) ) def dashboard( self, @@ -1846,33 +1845,25 @@ def dashboard( if not dashboard: abort(404) - has_access_ = False - for datasource in dashboard.datasources: - datasource = DatasourceDAO.get_datasource( - datasource_type=DatasourceType(datasource.type), - datasource_id=datasource.id, - session=db.session(), - ) - if datasource and security_manager.can_access_datasource( - datasource=datasource, - ): - has_access_ = True - - if has_access_ is False and config["ENABLE_ACCESS_REQUEST"]: - flash( - __(security_manager.get_datasource_access_error_msg(datasource)), - "danger", - ) - return redirect( - f"/superset/request_access/?dashboard_id={dashboard.id}" + if config["ENABLE_ACCESS_REQUEST"]: + for datasource in dashboard.datasources: + datasource = DatasourceDAO.get_datasource( + datasource_type=DatasourceType(datasource.type), + datasource_id=datasource.id, + session=db.session(), ) - - if has_access_: - break - - if dashboard.datasources and not has_access_: - flash(DashboardAccessDeniedError.message, "danger") - return redirect(DASHBOARD_LIST_URL) + if datasource and not security_manager.can_access_datasource( + datasource=datasource + ): + flash( + __( + security_manager.get_datasource_access_error_msg(datasource) + ), + "danger", + ) + return redirect( + f"/superset/request_access/?dashboard_id={dashboard.id}" + ) dash_edit_perm = security_manager.is_owner( dashboard diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index eb5416e0c50ba..9fcee061a6297 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -65,7 +65,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "roles": _( "Roles is a list which defines access to the dashboard. " "Granting a role access to a dashboard will bypass dataset level checks." - "If no roles are defined, regular access permissions apply." + "If no roles are defined then the dashboard is available to all roles." ), "published": _( "Determines whether or not this dashboard is " diff --git a/superset/views/utils.py b/superset/views/utils.py index cd84d8e0a58dd..835c8eda0d8d6 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -23,12 +23,11 @@ import msgpack import pyarrow as pa import simplejson as json -from flask import flash, g, has_request_context, redirect, request +from flask import g, has_request_context, request from flask_appbuilder.security.sqla import models as ab_models from flask_appbuilder.security.sqla.models import User from flask_babel import _ from sqlalchemy.orm.exc import NoResultFound -from werkzeug.wrappers.response import Response import superset.models.core as models from superset import app, dataframe, db, result_set, viz @@ -593,8 +592,3 @@ def get_cta_schema_name( if not func: return None return func(database, user, schema, sql) - - -def redirect_with_flash(url: str, message: str, category: str) -> Response: - flash(message=message, category=category) - return redirect(url) diff --git a/tests/integration_tests/dashboards/security/security_rbac_tests.py b/tests/integration_tests/dashboards/security/security_rbac_tests.py index a49e533f1ba41..d425c0e71118f 100644 --- a/tests/integration_tests/dashboards/security/security_rbac_tests.py +++ b/tests/integration_tests/dashboards/security/security_rbac_tests.py @@ -91,11 +91,13 @@ def test_get_dashboard_view__user_can_not_access_without_permission(self): # act response = self.get_dashboard_view_response(dashboard_to_access) - assert response.status_code == 302 request_payload = get_query_context("birth_names") rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data") - assert rv.status_code == 403 + self.assertEqual(rv.status_code, 403) + + # assert + self.assert403(response) def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft( self, @@ -112,57 +114,11 @@ def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft response = self.get_dashboard_view_response(dashboard_to_access) # assert - assert response.status_code == 302 + self.assert403(response) # post revoke_access_to_dashboard(dashboard_to_access, new_role) - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_get_dashboard_view__user_no_access_regular_rbac(self): - if backend() == "hive": - return - - slice = ( - db.session.query(Slice) - .filter_by(slice_name="Girl Name Cloud") - .one_or_none() - ) - dashboard = create_dashboard_to_db(published=True, slices=[slice]) - self.login("gamma") - - # assert redirect on regular rbac access denied - response = self.get_dashboard_view_response(dashboard) - assert response.status_code == 302 - - request_payload = get_query_context("birth_names") - rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data") - assert rv.status_code == 403 - db.session.delete(dashboard) - db.session.commit() - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_get_dashboard_view__user_access_regular_rbac(self): - if backend() == "hive": - return - - slice = ( - db.session.query(Slice) - .filter_by(slice_name="Girl Name Cloud") - .one_or_none() - ) - dashboard = create_dashboard_to_db(published=True, slices=[slice]) - self.login("gamma_sqllab") - - response = self.get_dashboard_view_response(dashboard) - - assert response.status_code == 200 - - request_payload = get_query_context("birth_names") - rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data") - assert rv.status_code == 200 - db.session.delete(dashboard) - db.session.commit() - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_get_dashboard_view__user_access_with_dashboard_permission(self): if backend() == "hive": @@ -199,14 +155,13 @@ def test_get_dashboard_view__user_access_with_dashboard_permission(self): @pytest.mark.usefixtures("public_role_like_gamma") def test_get_dashboard_view__public_user_can_not_access_without_permission(self): dashboard_to_access = create_dashboard_to_db(published=True) - grant_access_to_dashboard(dashboard_to_access, "Alpha") self.logout() # act response = self.get_dashboard_view_response(dashboard_to_access) # assert - assert response.status_code == 302 + self.assert403(response) @pytest.mark.usefixtures("public_role_like_gamma") def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft( @@ -220,7 +175,7 @@ def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_acces response = self.get_dashboard_view_response(dashboard_to_access) # assert - assert response.status_code == 302 + self.assert403(response) # post revoke_access_to_dashboard(dashboard_to_access, "Public")