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 aeea6c201439..23a6b777b916 100644 --- a/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx +++ b/docs/docs/creating-charts-dashboards/creating-your-first-dashboard.mdx @@ -184,9 +184,8 @@ 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 -- 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** + - 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. diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 9b044519d465..651985232463 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, then the dashboard is available to all 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.', )}

diff --git a/superset/security/manager.py b/superset/security/manager.py index 9e7a8bbd4bb1..754a91d9077a 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -2046,7 +2046,10 @@ def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: from superset.dashboards.commands.exceptions import DashboardAccessDeniedError def has_rbac_access() -> bool: - return (not is_feature_enabled("DASHBOARD_RBAC")) or any( + 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 @@ -2073,15 +2076,18 @@ 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(datasource_class) + db.session.query(Dashboard.id) + .join(Slice, Dashboard.slices) .join(Slice.table) .filter(datasource_class.id == datasource.id) - ) - - query = DashboardAccessFilter("id", SQLAInterface(Dashboard, db.session)).apply( - query, None + .filter(Dashboard.id.in_(dashboard_ids)) ) exists = db.session.query(query.exists()).scalar() diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index 7852c4ff7472..e77a5599057a 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -116,9 +116,7 @@ def on_security_exception(self: Any, ex: Exception) -> Response: # noinspection PyPackageRequirements -def check_dashboard_access( - on_error: Callable[..., Any] = on_security_exception -) -> Callable[..., Any]: +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: @@ -130,7 +128,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(self, ex) + return on_error(str(ex)) except Exception as exception: raise exception diff --git a/superset/views/core.py b/superset/views/core.py index 6559db125422..7576f042e8ea 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -66,6 +66,7 @@ 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 @@ -166,6 +167,7 @@ get_form_data, get_viz, loads_request_json, + redirect_with_flash, sanitize_datasource_data, ) from superset.viz import BaseViz @@ -191,6 +193,7 @@ "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 = __( @@ -1825,9 +1828,7 @@ def favstar( # pylint: disable=no-self-use @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 - ) + on_error=lambda msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger") ) def dashboard( self, @@ -1845,25 +1846,33 @@ def dashboard( if not dashboard: abort(404) - 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(), + 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", ) - 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}" - ) + return redirect( + f"/superset/request_access/?dashboard_id={dashboard.id}" + ) + + 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 diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index 9fcee061a629..eb5416e0c50b 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 then the dashboard is available to all roles." + "If no roles are defined, regular access permissions apply." ), "published": _( "Determines whether or not this dashboard is " diff --git a/superset/views/utils.py b/superset/views/utils.py index 835c8eda0d8d..cd84d8e0a58d 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -23,11 +23,12 @@ import msgpack import pyarrow as pa import simplejson as json -from flask import g, has_request_context, request +from flask import flash, g, has_request_context, redirect, 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 @@ -592,3 +593,8 @@ 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 d425c0e71118..a49e533f1ba4 100644 --- a/tests/integration_tests/dashboards/security/security_rbac_tests.py +++ b/tests/integration_tests/dashboards/security/security_rbac_tests.py @@ -91,13 +91,11 @@ 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") - self.assertEqual(rv.status_code, 403) - - # assert - self.assert403(response) + assert rv.status_code == 403 def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft( self, @@ -114,11 +112,57 @@ def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft response = self.get_dashboard_view_response(dashboard_to_access) # assert - self.assert403(response) + assert response.status_code == 302 # 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": @@ -155,13 +199,14 @@ 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 - self.assert403(response) + assert response.status_code == 302 @pytest.mark.usefixtures("public_role_like_gamma") def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft( @@ -175,7 +220,7 @@ def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_acces response = self.get_dashboard_view_response(dashboard_to_access) # assert - self.assert403(response) + assert response.status_code == 302 # post revoke_access_to_dashboard(dashboard_to_access, "Public")