From 8c5b6b12632380a004e5a3b4c1e5716915c1ae9c Mon Sep 17 00:00:00 2001 From: Amit Miran <47772523+amitmiran137@users.noreply.github.com> Date: Tue, 13 Apr 2021 16:23:31 +0300 Subject: [PATCH] feat(dashboard_rbac): provide data access based on dashboard access (#13992) * feat: provide data access based onb dashboard access * chore: adjust code after CR comments * fix: add brackets * fix: type * chore: add tests * fix: pre-commit * fix: pre-commit and lint * fix: fix test * fix: pre-commit * fix: fix local pylint warnings * revert: birth_names pylint change bc it affects tests * Update superset/security/manager.py Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * Update superset/security/manager.py * Update tests/utils_tests.py * fix: after CR * fix: after CR from ville * chore: update roles description Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> --- .../components/PropertiesModal/index.jsx | 2 +- superset/security/manager.py | 37 +++++++++++++++++-- superset/views/dashboard/mixin.py | 3 +- .../security/security_rbac_tests.py | 28 ++++++++++++-- 4 files changed, 60 insertions(+), 10 deletions(-) diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx index e58c715d51..c1114e74d7 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx @@ -399,7 +399,7 @@ class PropertiesModal extends React.PureComponent { />

{t( - 'Roles is a list which defines access to the dashboard. These roles are always applied in addition to restrictions on dataset level access. If no roles 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 defined then the dashboard is available to all roles.', )}

diff --git a/superset/security/manager.py b/superset/security/manager.py index 6472af6844..7a62e1a436 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -22,6 +22,7 @@ from typing import Any, Callable, cast, List, Optional, Set, Tuple, TYPE_CHECKIN from flask import current_app, g from flask_appbuilder import Model +from flask_appbuilder.models.sqla.interface import SQLAInterface from flask_appbuilder.security.sqla.manager import SecurityManager from flask_appbuilder.security.sqla.models import ( assoc_permissionview_role, @@ -61,7 +62,6 @@ if TYPE_CHECKING: from superset.sql_parse import Table from superset.viz import BaseViz - logger = logging.getLogger(__name__) @@ -925,7 +925,9 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods ) ) - def raise_for_access( # pylint: disable=too-many-arguments,too-many-branches + def raise_for_access( + # pylint: disable=too-many-arguments,too-many-branches, + # pylint: disable=too-many-locals self, database: Optional["Database"] = None, datasource: Optional["BaseDatasource"] = None, @@ -996,9 +998,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods assert datasource + from superset.extensions import feature_flag_manager + if not ( self.can_access_schema(datasource) or self.can_access("datasource_access", datasource.perm or "") + or ( + feature_flag_manager.is_feature_enabled("DASHBOARD_RBAC") + and self.can_access_based_on_dashboard(datasource) + ) ): raise SupersetSecurityException( self.get_datasource_access_error_object(datasource) @@ -1101,8 +1109,8 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods ids.sort() # Combinations rather than permutations return ids - # pylint: disable=no-self-use - def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: + @staticmethod + def raise_for_dashboard_access(dashboard: "Dashboard") -> None: """ Raise an exception if the user cannot access the dashboard. @@ -1127,3 +1135,24 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods if not can_access: raise DashboardAccessDeniedError() + + @staticmethod + def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool: + from superset import db + from superset.dashboards.filters import DashboardFilter + from superset.models.slice import Slice + from superset.models.dashboard import Dashboard + + datasource_class = type(datasource) + query = ( + db.session.query(datasource_class) + .join(Slice.table) + .filter(datasource_class.id == datasource.id) + ) + + query = DashboardFilter("id", SQLAInterface(Dashboard, db.session)).apply( + query, None + ) + + exists = db.session.query(query.exists()).scalar() + return exists diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py index 273cfdf0b3..9a5510728b 100644 --- a/superset/views/dashboard/mixin.py +++ b/superset/views/dashboard/mixin.py @@ -65,8 +65,7 @@ class DashboardMixin: # pylint: disable=too-few-public-methods "owners": _("Owners is a list of users who can alter the dashboard."), "roles": _( "Roles is a list which defines access to the dashboard. " - "These roles are always applied in addition to restrictions on dataset " - "level access. " + "Granting a role access to a dashboard will bypass dataset level checks." "If no roles defined then the dashboard is available to all roles." ), "published": _( diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 19885d9582..42cab6c7d9 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -27,7 +27,11 @@ from tests.dashboards.superset_factory_util import ( create_datasource_table_to_db, create_slice_to_db, ) +from tests.fixtures.birth_names_dashboard import load_birth_names_dashboard_with_slices from tests.fixtures.public_role import public_role_like_gamma +from tests.fixtures.query_context import get_query_context + +CHART_DATA_URI = "api/v1/chart/data" @mock.patch.dict( @@ -65,16 +69,26 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): # assert self.assert_dashboard_view_response(response, dashboard_to_access) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") 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) + slice = ( + db.session.query(Slice) + .filter_by(slice_name="Girl Name Cloud") + .one_or_none() + ) + dashboard_to_access = create_dashboard_to_db(published=True, slices=[slice]) self.login(username) # act response = self.get_dashboard_view_response(dashboard_to_access) + request_payload = get_query_context("birth_names") + rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data") + self.assertEqual(rv.status_code, 401) + # assert self.assert403(response) @@ -98,6 +112,7 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): # 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_access_with_dashboard_permission(self): # arrange @@ -105,9 +120,12 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): 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()] + slice = ( + db.session.query(Slice) + .filter_by(slice_name="Girl Name Cloud") + .one_or_none() ) + dashboard_to_access = create_dashboard_to_db(published=True, slices=[slice]) self.login(username) grant_access_to_dashboard(dashboard_to_access, new_role) @@ -117,6 +135,10 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity): # assert self.assert_dashboard_view_response(response, dashboard_to_access) + request_payload = get_query_context("birth_names") + rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data") + self.assertEqual(rv.status_code, 200) + # post revoke_access_to_dashboard(dashboard_to_access, new_role)