From 61fdb619974b6759e52a9adaf1acdd5668fc75ce Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Mon, 1 Feb 2021 19:27:37 +0200 Subject: [PATCH 01/12] test: dashboard_view_test failing --- .../security/security_rbac_tests.py | 237 +++++++++++++----- 1 file changed, 173 insertions(+), 64 deletions(-) diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index efd2c990f314c..4e4e64b492681 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -31,6 +31,133 @@ "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_not_published( + 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_not_published( + self, + ): + # arrange + dashboard_to_access = create_dashboard_to_db(published=False) + grant_access_to_dashboard(dashboard_to_access, "Public") + # 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") + + # 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 +175,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.arrange_all_owned_dashboards_scenario() + + # act + response = self.get_dashboards_list_response() + + # assert + self.assert_dashboards_list_view_response( + response, 2, owned_dashboards, not_owned_dashboards + ) + + def arrange_all_owned_dashboards_scenario(self): username = random_str() new_role = f"role_{random_str()}" owner = self.create_user_with_roles( @@ -67,16 +208,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,23 +229,11 @@ 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 - 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 = [ - 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, + not_published_dashboards, + published_dashboards, + ) = self.__arrange_only_published_permitted() # act response = self.get_dashboards_list_response() @@ -129,6 +250,23 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) for dash in published_dashboards + not_published_dashboards: revoke_access_to_dashboard(dash, new_role) + def __arrange_only_published_permitted(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 = [ + 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) + return new_role, not_published_dashboards, published_dashboards + def test_get_dashboards_list__public_user_without_any_permissions_get_empty_list( self, ): @@ -188,27 +326,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.arrange_all_owned_dashboards_scenario() # act response = self.get_dashboards_api_response() @@ -232,23 +353,11 @@ 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, + not_published_dashboards, + published_dashboards, + ) = self.__arrange_only_published_permitted() # act response = self.get_dashboards_api_response() From d2b8e0ca3d598f473ead611dc68b469a6c08d0ec Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Tue, 2 Feb 2021 17:10:50 +0200 Subject: [PATCH 02/12] test: tests works first time --- superset/errors.py | 1 + superset/models/dashboard.py | 10 ++++ superset/security/manager.py | 49 +++++++++++++++++++ superset/utils/decorators.py | 27 +++++++++- superset/views/core.py | 14 ++---- superset/views/utils.py | 1 - .../security/security_rbac_tests.py | 2 + 7 files changed, 93 insertions(+), 11 deletions(-) diff --git a/superset/errors.py b/superset/errors.py index 8b0416d5b2b3e..e8b387e81a100 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -48,6 +48,7 @@ class SupersetErrorType(str, Enum): # Security access errors TABLE_SECURITY_ACCESS_ERROR = "TABLE_SECURITY_ACCESS_ERROR" DATASOURCE_SECURITY_ACCESS_ERROR = "DATASOURCE_SECURITY_ACCESS_ERROR" + DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR" MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR" # Other errors diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 3606da8c5cb80..f062f916fabf1 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -407,3 +407,13 @@ 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 get_dashboard(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() diff --git a/superset/security/manager.py b/superset/security/manager.py index 43461f10fcfea..05d77a4baef56 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -50,6 +50,7 @@ from superset.exceptions import SupersetSecurityException from superset.utils.core import DatasourceName, RowLevelSecurityFilterType + if TYPE_CHECKING: from superset.common.query_context import QueryContext from superset.connectors.base.models import BaseDatasource @@ -417,6 +418,54 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: return True + def can_access_dashboard_by_id(self, dashboard_id_or_slug: Union[str, int]): + from superset.models.dashboard import get_dashboard + dashboard = get_dashboard( + str(dashboard_id_or_slug)) + return self.can_access_by_dashboard(dashboard) + + def can_access_by_dashboard(self, dashboard: "Dashboard") -> None: + from superset.views.base import is_user_admin + from superset.views.utils import is_owner + is_admin = is_user_admin() + if not (is_admin or is_owner(dashboard, g.user) or ( + dashboard.published and self.__can_access_by_dashboard(dashboard))): + raise SupersetSecurityException(self.get_dashboard_access_error_object(dashboard)) + + def __can_access_by_dashboard(self, dashboard: "Dashboard") -> bool: + from superset.views.base import get_user_roles + return any(dashboard_role.id in + [user_role.id for user_role in get_user_roles()] + for dashboard_role in dashboard.roles) + + def get_dashboard_access_error_object( + self, # pylint: disable=invalid-name + dashboard: "Dashboard" + ) -> SupersetError: + """ + Return the error object for the denied Superset dashboard. + :param dashboard: The denied Superset dashboard + :returns: The error object + """ + return SupersetError( + error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR, + message=self.get_access_error_msg(dashboard), + level=ErrorLevel.ERROR, + extra={ + "dashboard": dashboard.id, + }, + ) + + def get_access_error_msg(self,dashboard: "Dashboard") -> str: + """ + Return the error message for the denied Superset dashboard. + :param dashboard: The denied Superset dashboard + :returns: The error message + """ + + return f"This dashboard requires to have one of the access roles assigned it" + + def user_view_menu_names(self, permission_name: str) -> Set[str]: base_query = ( self.get_session.query(self.viewmenu_model.name) diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index 014a512f0bedc..55f7125b8e1b0 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -18,7 +18,9 @@ from typing import Any, Callable, Dict, Iterator, Union from contextlib2 import contextmanager - +from functools import wraps +from superset.utils import core as utils +from superset.exceptions import SupersetSecurityException from superset.stats_logger import BaseStatsLogger from superset.utils.dates import now_as_float @@ -69,3 +71,26 @@ def wrapped(*args: Any, **kwargs: Any) -> Any: return wrapped return decorate + + +def on_security_exception(self, ex): + return self.response(403, **{"message": utils.error_msg_from_exception(ex)}) + + +def check_permissions( on_error: Callable[..., Any] = on_security_exception) -> Callable[..., Any]: + + def decorator(f: Callable[..., Any]) -> Callable[..., Any]: + def wrapper(self, *args: Any, **kwargs: Any) -> Callable: + try: + from superset import security_manager + security_manager.can_access_dashboard_by_id(kwargs['dashboard_id_or_slug']) + except SupersetSecurityException as ex: + return on_error(self, ex) + except Exception as e: + raise e + + return f(self, *args, **kwargs) + + return wrapper + + return decorator diff --git a/superset/views/core.py b/superset/views/core.py index 47df3e865e318..74b568ea3ce41 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -86,7 +86,7 @@ from superset.extensions import async_query_manager, cache_manager from superset.jinja_context import get_template_processor from superset.models.core import Database, FavStar, Log -from superset.models.dashboard import Dashboard +from superset.models.dashboard import Dashboard, get_dashboard from superset.models.datasource_access_request import DatasourceAccessRequest from superset.models.slice import Slice from superset.models.sql_lab import Query, TabState @@ -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_permissions from superset.views.base import ( api, BaseSupersetView, @@ -1785,6 +1786,8 @@ def publish( # pylint: disable=no-self-use @has_access @expose("/dashboard//") @event_logger.log_this_with_extra_payload + @check_permissions(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, @@ -1793,14 +1796,7 @@ def dashboard( # pylint: disable=too-many-locals add_extra_log_payload: Callable[..., None] = lambda **kwargs: 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() + dash = get_dashboard(dashboard_id_or_slug) if not dash: abort(404) diff --git a/superset/views/utils.py b/superset/views/utils.py index 5f9ec10e8bb4f..618d946864e1c 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -319,7 +319,6 @@ def get_time_range_endpoints( # /superset-frontend/src/dashboard/util/componentTypes.js CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"] - def get_dashboard_extra_filters( slice_id: int, dashboard_id: int ) -> List[Dict[str, Any]]: diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index 4e4e64b492681..fb6c07056089d 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -149,6 +149,8 @@ def test_get_dashboard_view__public_user_access_with_dashboard_permission(self): ) grant_access_to_dashboard(dashboard_to_access, "Public") + self.logout() + # act response = self.get_dashboard_view_response(dashboard_to_access) From abb726df1ba06f4c25e4ce353ea71926ba1a49ca Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Tue, 2 Feb 2021 21:46:37 +0200 Subject: [PATCH 03/12] fix: pre-commit and some refactoring --- superset/models/dashboard.py | 18 ++++++ superset/security/manager.py | 61 ++++++------------- superset/utils/decorators.py | 35 +++++++---- superset/views/core.py | 9 ++- superset/views/utils.py | 1 + .../security/security_rbac_tests.py | 6 ++ 6 files changed, 71 insertions(+), 59 deletions(-) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index f062f916fabf1..f583fc5b3dc6d 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -45,6 +45,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.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.extensions import cache_manager from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.models.slice import Slice @@ -408,6 +409,7 @@ def clear_dashboard_cache( sqla.event.listen(DruidMetric, "after_update", clear_dashboard_cache) sqla.event.listen(DruidColumn, "after_update", clear_dashboard_cache) + def get_dashboard(id_or_slug: str) -> Dashboard: session = db.session() qry = session.query(Dashboard) @@ -417,3 +419,19 @@ def get_dashboard(id_or_slug: str) -> Dashboard: qry = qry.filter_by(slug=id_or_slug) return qry.one_or_none() + + +def get_dashboard_access_error_object( + dashboard: Dashboard, # pylint: disable=invalid-name +) -> SupersetError: + """ + Return the error object for the denied Superset dashboard. + :param dashboard: The denied Superset dashboard + :returns: The error object + """ + return SupersetError( + error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR, + message=f"This dashboard requires to have one of the access roles assigned it", + level=ErrorLevel.ERROR, + extra={"dashboard": dashboard.id,}, + ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 05d77a4baef56..709812f6eb4c9 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -50,7 +50,6 @@ from superset.exceptions import SupersetSecurityException from superset.utils.core import DatasourceName, RowLevelSecurityFilterType - if TYPE_CHECKING: from superset.common.query_context import QueryContext from superset.connectors.base.models import BaseDatasource @@ -59,6 +58,7 @@ from superset.models.sql_lab import Query from superset.sql_parse import Table from superset.viz import BaseViz + from superset.models.dashboard import Dashboard logger = logging.getLogger(__name__) @@ -418,53 +418,26 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: return True - def can_access_dashboard_by_id(self, dashboard_id_or_slug: Union[str, int]): - from superset.models.dashboard import get_dashboard - dashboard = get_dashboard( - str(dashboard_id_or_slug)) - return self.can_access_by_dashboard(dashboard) - - def can_access_by_dashboard(self, dashboard: "Dashboard") -> None: + def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: from superset.views.base import is_user_admin from superset.views.utils import is_owner - is_admin = is_user_admin() - if not (is_admin or is_owner(dashboard, g.user) or ( - dashboard.published and self.__can_access_by_dashboard(dashboard))): - raise SupersetSecurityException(self.get_dashboard_access_error_object(dashboard)) - - def __can_access_by_dashboard(self, dashboard: "Dashboard") -> bool: - from superset.views.base import get_user_roles - return any(dashboard_role.id in - [user_role.id for user_role in get_user_roles()] - for dashboard_role in dashboard.roles) - - def get_dashboard_access_error_object( - self, # pylint: disable=invalid-name - dashboard: "Dashboard" - ) -> SupersetError: - """ - Return the error object for the denied Superset dashboard. - :param dashboard: The denied Superset dashboard - :returns: The error object - """ - return SupersetError( - error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR, - message=self.get_access_error_msg(dashboard), - level=ErrorLevel.ERROR, - extra={ - "dashboard": dashboard.id, - }, - ) + from superset.models.dashboard import get_dashboard_access_error_object + from superset.views.base import get_user_roles - def get_access_error_msg(self,dashboard: "Dashboard") -> str: - """ - Return the error message for the denied Superset dashboard. - :param dashboard: The denied Superset dashboard - :returns: The error message - """ - - return f"This dashboard requires to have one of the access roles assigned it" + 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 SupersetSecurityException( + get_dashboard_access_error_object(dashboard) + ) def user_view_menu_names(self, permission_name: str) -> Set[str]: base_query = ( diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index 55f7125b8e1b0..b20d97fa6fb71 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -15,13 +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 functools import wraps -from superset.utils import core as utils +from flask import Response + +from superset import is_feature_enabled from superset.exceptions import SupersetSecurityException from superset.stats_logger import BaseStatsLogger +from superset.utils import core as utils from superset.utils.dates import now_as_float @@ -73,21 +76,29 @@ def wrapped(*args: Any, **kwargs: Any) -> Any: return decorate -def on_security_exception(self, ex): +def on_security_exception(self, ex) -> Response: return self.response(403, **{"message": utils.error_msg_from_exception(ex)}) -def check_permissions( on_error: Callable[..., Any] = on_security_exception) -> Callable[..., Any]: - +def check_permissions( + on_error: Callable[..., Any] = on_security_exception +) -> Callable[..., Any]: def decorator(f: Callable[..., Any]) -> Callable[..., Any]: def wrapper(self, *args: Any, **kwargs: Any) -> Callable: - try: - from superset import security_manager - security_manager.can_access_dashboard_by_id(kwargs['dashboard_id_or_slug']) - except SupersetSecurityException as ex: - return on_error(self, ex) - except Exception as e: - raise e + + if is_feature_enabled("DASHBOARD_RBAC"): + try: + from superset import security_manager + from superset.models.dashboard import get_dashboard + + dashboard = get_dashboard(str(kwargs["dashboard_id_or_slug"])) + + security_manager.raise_for_dashboard_access(dashboard) + + except SupersetSecurityException as ex: + return on_error(self, ex) + except Exception as e: + raise e return f(self, *args, **kwargs) diff --git a/superset/views/core.py b/superset/views/core.py index 74b568ea3ce41..3e10f8b753874 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1786,8 +1786,11 @@ def publish( # pylint: disable=no-self-use @has_access @expose("/dashboard//") @event_logger.log_this_with_extra_payload - @check_permissions(on_error=lambda self, ex: Response( - utils.error_msg_from_exception(ex), status=403)) + @check_permissions( + 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, @@ -1807,7 +1810,7 @@ def dashboard( # pylint: disable=too-many-locals 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 diff --git a/superset/views/utils.py b/superset/views/utils.py index 618d946864e1c..5f9ec10e8bb4f 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -319,6 +319,7 @@ def get_time_range_endpoints( # /superset-frontend/src/dashboard/util/componentTypes.js CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"] + def get_dashboard_extra_filters( slice_id: int, dashboard_id: int ) -> List[Dict[str, Any]]: diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index fb6c07056089d..69242432b0e53 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -133,6 +133,7 @@ def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_acces # 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) @@ -296,6 +297,8 @@ def test_get_dashboards_list__public_user_get_only_published_permitted_dashboard for dash in published_dashboards + not_published_dashboards: grant_access_to_dashboard(dash, "Public") + self.logout() + # act response = self.get_dashboards_list_response() @@ -380,6 +383,7 @@ 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() @@ -403,6 +407,8 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards for dash in published_dashboards + not_published_dashboards: grant_access_to_dashboard(dash, "Public") + self.logout() + # act response = self.get_dashboards_api_response() From 32288ea9bbadb93178b3bbae27eb7d4bfdacb6ca Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 20:53:33 +0200 Subject: [PATCH 04/12] fix: after CR --- superset/dashboards/commands/exceptions.py | 4 +++ superset/models/dashboard.py | 40 +++++++--------------- superset/security/manager.py | 31 +++++++++-------- superset/utils/decorators.py | 9 ++--- superset/views/core.py | 8 ++--- 5 files changed, 42 insertions(+), 50 deletions(-) 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 f583fc5b3dc6d..5c478fdb9b650 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -14,6 +14,8 @@ # 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 @@ -357,6 +359,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] @@ -408,30 +421,3 @@ 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 get_dashboard(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() - - -def get_dashboard_access_error_object( - dashboard: Dashboard, # pylint: disable=invalid-name -) -> SupersetError: - """ - Return the error object for the denied Superset dashboard. - :param dashboard: The denied Superset dashboard - :returns: The error object - """ - return SupersetError( - error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR, - message=f"This dashboard requires to have one of the access roles assigned it", - level=ErrorLevel.ERROR, - extra={"dashboard": dashboard.id,}, - ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 709812f6eb4c9..738a6ad271df2 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -46,6 +46,7 @@ from superset import sql_parse from superset.connectors.connector_registry import ConnectorRegistry from superset.constants import RouteMethod +from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetSecurityException from superset.utils.core import DatasourceName, RowLevelSecurityFilterType @@ -419,25 +420,25 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: return True def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: + + from superset.views.base import is_user_admin from superset.views.utils import is_owner - from superset.models.dashboard import get_dashboard_access_error_object from superset.views.base import get_user_roles - - 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 SupersetSecurityException( - get_dashboard_access_error_object(dashboard) + from superset import is_feature_enabled + 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() def user_view_menu_names(self, permission_name: str) -> Set[str]: base_query = ( diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index b20d97fa6fb71..4031a478ef5a2 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -22,6 +22,7 @@ from flask import Response from superset import is_feature_enabled +from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.exceptions import SupersetSecurityException from superset.stats_logger import BaseStatsLogger from superset.utils import core as utils @@ -80,7 +81,7 @@ def on_security_exception(self, ex) -> Response: return self.response(403, **{"message": utils.error_msg_from_exception(ex)}) -def check_permissions( +def check_dashboard_access( on_error: Callable[..., Any] = on_security_exception ) -> Callable[..., Any]: def decorator(f: Callable[..., Any]) -> Callable[..., Any]: @@ -89,13 +90,13 @@ def wrapper(self, *args: Any, **kwargs: Any) -> Callable: if is_feature_enabled("DASHBOARD_RBAC"): try: from superset import security_manager - from superset.models.dashboard import get_dashboard + from superset.models.dashboard import Dashboard - dashboard = get_dashboard(str(kwargs["dashboard_id_or_slug"])) + dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"])) security_manager.raise_for_dashboard_access(dashboard) - except SupersetSecurityException as ex: + except DashboardAccessDeniedError as ex: return on_error(self, ex) except Exception as e: raise e diff --git a/superset/views/core.py b/superset/views/core.py index 3e10f8b753874..b253907bc785f 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -86,7 +86,7 @@ from superset.extensions import async_query_manager, cache_manager from superset.jinja_context import get_template_processor from superset.models.core import Database, FavStar, Log -from superset.models.dashboard import Dashboard, get_dashboard +from superset.models.dashboard import Dashboard from superset.models.datasource_access_request import DatasourceAccessRequest from superset.models.slice import Slice from superset.models.sql_lab import Query, TabState @@ -104,7 +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_permissions +from superset.utils.decorators import check_dashboard_access from superset.views.base import ( api, BaseSupersetView, @@ -1786,7 +1786,7 @@ def publish( # pylint: disable=no-self-use @has_access @expose("/dashboard//") @event_logger.log_this_with_extra_payload - @check_permissions( + @check_dashboard_access( on_error=lambda self, ex: Response( utils.error_msg_from_exception(ex), status=403 ) @@ -1799,7 +1799,7 @@ def dashboard( # pylint: disable=too-many-locals add_extra_log_payload: Callable[..., None] = lambda **kwargs: None, ) -> FlaskResponse: """Server side rendering for a dashboard""" - dash = get_dashboard(dashboard_id_or_slug) + dash = Dashboard.get(dashboard_id_or_slug) if not dash: abort(404) From a8d042cc557b451a0e1879673c2d1ced73ec898d Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 20:57:30 +0200 Subject: [PATCH 05/12] fix: replace not_published with draft --- .../security/security_dataset_tests.py | 4 +- .../security/security_rbac_tests.py | 38 +++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) 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 69242432b0e53..ecf49ec213318 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -75,7 +75,7 @@ def test_get_dashboard_view__user_can_not_access_without_permission(self): # assert self.assert403(response) - def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_not_published( + def test_get_dashboard_view__user_with_dashboard_permission_can_not_access_draft( self, ): # arrange @@ -127,7 +127,7 @@ def test_get_dashboard_view__public_user_can_not_access_without_permission(self) # assert self.assert403(response) - def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_not_published( + def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft( self, ): # arrange @@ -234,7 +234,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) # arrange ( new_role, - not_published_dashboards, + draft_dashboards, published_dashboards, ) = self.__arrange_only_published_permitted() @@ -246,11 +246,11 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) response, len(published_dashboards), published_dashboards, - not_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 __arrange_only_published_permitted(self): @@ -261,14 +261,14 @@ def __arrange_only_published_permitted(self): 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) - return new_role, not_published_dashboards, published_dashboards + return new_role, draft_dashboards, published_dashboards def test_get_dashboards_list__public_user_without_any_permissions_get_empty_list( self, @@ -289,12 +289,12 @@ 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() @@ -307,11 +307,11 @@ def test_get_dashboards_list__public_user_get_only_published_permitted_dashboard response, len(published_dashboards), published_dashboards, - not_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): @@ -360,7 +360,7 @@ def test_get_dashboards_api__user_without_any_permissions_get_empty_list(self): def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): ( new_role, - not_published_dashboards, + draft_dashboards, published_dashboards, ) = self.__arrange_only_published_permitted() @@ -372,11 +372,11 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): response, len(published_dashboards), published_dashboards, - not_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( @@ -399,12 +399,12 @@ 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() @@ -417,9 +417,9 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards response, len(published_dashboards), published_dashboards, - not_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") From eb60005936d07be67d5d93e3bfd7536fac463fed Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 21:10:09 +0200 Subject: [PATCH 06/12] fix: after CR --- superset/security/manager.py | 2 +- .../security/security_rbac_tests.py | 32 ++++++------------- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 738a6ad271df2..107dbcd6233ab 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -421,11 +421,11 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: - from superset.views.base import is_user_admin from superset.views.utils import is_owner from superset.views.base import get_user_roles from superset import is_feature_enabled + if is_feature_enabled("DASHBOARD_RBAC"): has_rbac_access = any( dashboard_role.id in [user_role.id for user_role in get_user_roles()] diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index ecf49ec213318..ad40599b49647 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -181,7 +181,7 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): ( not_owned_dashboards, owned_dashboards, - ) = self.arrange_all_owned_dashboards_scenario() + ) = self._arrange_all_owned_dashboards_scenario() # act response = self.get_dashboards_list_response() @@ -191,7 +191,7 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): response, 2, owned_dashboards, not_owned_dashboards ) - def arrange_all_owned_dashboards_scenario(self): + def _arrange_all_owned_dashboards_scenario(self): username = random_str() new_role = f"role_{random_str()}" owner = self.create_user_with_roles( @@ -236,24 +236,21 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) new_role, draft_dashboards, published_dashboards, - ) = self.__arrange_only_published_permitted() + ) = self._arrange_only_published_permitted_dashboards_scenario() # act response = self.get_dashboards_list_response() # assert self.assert_dashboards_list_view_response( - response, - len(published_dashboards), - published_dashboards, - draft_dashboards, + response, len(published_dashboards), published_dashboards, draft_dashboards, ) # post for dash in published_dashboards + draft_dashboards: revoke_access_to_dashboard(dash, new_role) - def __arrange_only_published_permitted(self): + def _arrange_only_published_permitted_dashboards_scenario(self): username = random_str() new_role = f"role_{random_str()}" self.create_user_with_roles(username, [new_role], should_create_roles=True) @@ -304,10 +301,7 @@ def test_get_dashboards_list__public_user_get_only_published_permitted_dashboard # assert self.assert_dashboards_list_view_response( - response, - len(published_dashboards), - published_dashboards, - draft_dashboards, + response, len(published_dashboards), published_dashboards, draft_dashboards, ) # post @@ -334,7 +328,7 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): ( not_owned_dashboards, owned_dashboards, - ) = self.arrange_all_owned_dashboards_scenario() + ) = self._arrange_all_owned_dashboards_scenario() # act response = self.get_dashboards_api_response() @@ -362,17 +356,14 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): new_role, draft_dashboards, published_dashboards, - ) = self.__arrange_only_published_permitted() + ) = self._arrange_only_published_permitted_dashboards_scenario() # act response = self.get_dashboards_api_response() # assert self.assert_dashboards_api_response( - response, - len(published_dashboards), - published_dashboards, - draft_dashboards, + response, len(published_dashboards), published_dashboards, draft_dashboards, ) # post @@ -414,10 +405,7 @@ def test_get_dashboards_api__public_user_get_only_published_permitted_dashboards # assert self.assert_dashboards_api_response( - response, - len(published_dashboards), - published_dashboards, - draft_dashboards, + response, len(published_dashboards), published_dashboards, draft_dashboards, ) # post From a9a9a93207652caa7797e5acb0c8fc0637d3e4a9 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 21:34:28 +0200 Subject: [PATCH 07/12] fix: pre-commit fixes --- superset/utils/decorators.py | 4 ++-- tests/dashboards/security/security_rbac_tests.py | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index 4031a478ef5a2..4a4f9d75c4bdb 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -77,7 +77,7 @@ def wrapped(*args: Any, **kwargs: Any) -> Any: return decorate -def on_security_exception(self, ex) -> Response: +def on_security_exception(self: Any, ex: Exception) -> Response: return self.response(403, **{"message": utils.error_msg_from_exception(ex)}) @@ -85,7 +85,7 @@ def check_dashboard_access( on_error: Callable[..., Any] = on_security_exception ) -> Callable[..., Any]: def decorator(f: Callable[..., Any]) -> Callable[..., Any]: - def wrapper(self, *args: Any, **kwargs: Any) -> Callable: + def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: if is_feature_enabled("DASHBOARD_RBAC"): try: diff --git a/tests/dashboards/security/security_rbac_tests.py b/tests/dashboards/security/security_rbac_tests.py index ad40599b49647..41a01fadec174 100644 --- a/tests/dashboards/security/security_rbac_tests.py +++ b/tests/dashboards/security/security_rbac_tests.py @@ -181,7 +181,7 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): ( not_owned_dashboards, owned_dashboards, - ) = self._arrange_all_owned_dashboards_scenario() + ) = self._create_sample_dashboards_with_owner_access() # act response = self.get_dashboards_list_response() @@ -191,7 +191,7 @@ def test_get_dashboards_list__owner_get_all_owned_dashboards(self): response, 2, owned_dashboards, not_owned_dashboards ) - def _arrange_all_owned_dashboards_scenario(self): + def _create_sample_dashboards_with_owner_access(self): username = random_str() new_role = f"role_{random_str()}" owner = self.create_user_with_roles( @@ -236,7 +236,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) new_role, draft_dashboards, published_dashboards, - ) = self._arrange_only_published_permitted_dashboards_scenario() + ) = self._create_sample_only_published_dashboard_with_roles() # act response = self.get_dashboards_list_response() @@ -250,7 +250,7 @@ def test_get_dashboards_list__user_get_only_published_permitted_dashboards(self) for dash in published_dashboards + draft_dashboards: revoke_access_to_dashboard(dash, new_role) - def _arrange_only_published_permitted_dashboards_scenario(self): + 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) @@ -328,7 +328,7 @@ def test_get_dashboards_api__owner_get_all_owned_dashboards(self): ( not_owned_dashboards, owned_dashboards, - ) = self._arrange_all_owned_dashboards_scenario() + ) = self._create_sample_dashboards_with_owner_access() # act response = self.get_dashboards_api_response() @@ -356,7 +356,7 @@ def test_get_dashboards_api__user_get_only_published_permitted_dashboards(self): new_role, draft_dashboards, published_dashboards, - ) = self._arrange_only_published_permitted_dashboards_scenario() + ) = self._create_sample_only_published_dashboard_with_roles() # act response = self.get_dashboards_api_response() From a10d79c5b6acb74d5d5c1e54c4311dfcbd8aca88 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 21:54:07 +0200 Subject: [PATCH 08/12] fix: pre-commit and lint fixes --- superset/models/dashboard.py | 22 +++++++++++++++++++++- superset/security/manager.py | 22 ---------------------- superset/utils/decorators.py | 14 +++++++------- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 5c478fdb9b650..a63594371e910 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -22,6 +22,7 @@ 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 @@ -47,7 +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.errors import ErrorLevel, SupersetError, SupersetErrorType +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 @@ -421,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/security/manager.py b/superset/security/manager.py index 107dbcd6233ab..ab4db7ea281b6 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -46,7 +46,6 @@ from superset import sql_parse from superset.connectors.connector_registry import ConnectorRegistry from superset.constants import RouteMethod -from superset.dashboards.commands.exceptions import DashboardAccessDeniedError from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetSecurityException from superset.utils.core import DatasourceName, RowLevelSecurityFilterType @@ -419,27 +418,6 @@ def can_access_table(self, database: "Database", table: "Table") -> bool: return True - def raise_for_dashboard_access(self, dashboard: "Dashboard") -> None: - - from superset.views.base import is_user_admin - from superset.views.utils import is_owner - from superset.views.base import get_user_roles - from superset import is_feature_enabled - - 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() - def user_view_menu_names(self, permission_name: str) -> Set[str]: base_query = ( self.get_session.query(self.viewmenu_model.name) diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index 4a4f9d75c4bdb..eba05ed6aed2b 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -15,7 +15,6 @@ # 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 @@ -23,7 +22,6 @@ from superset import is_feature_enabled from superset.dashboards.commands.exceptions import DashboardAccessDeniedError -from superset.exceptions import SupersetSecurityException from superset.stats_logger import BaseStatsLogger from superset.utils import core as utils from superset.utils.dates import now_as_float @@ -89,17 +87,19 @@ def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: if is_feature_enabled("DASHBOARD_RBAC"): try: - from superset import security_manager - from superset.models.dashboard import Dashboard + from superset.models.dashboard import ( + Dashboard, + raise_for_dashboard_access, + ) dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"])) - security_manager.raise_for_dashboard_access(dashboard) + raise_for_dashboard_access(dashboard) except DashboardAccessDeniedError as ex: return on_error(self, ex) - except Exception as e: - raise e + except Exception as exception: + raise exception return f(self, *args, **kwargs) From 7250da64d0bf0636eacc8e083a9060e21d0ccdb4 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 22:08:53 +0200 Subject: [PATCH 09/12] fix: remove unused --- superset/errors.py | 1 - 1 file changed, 1 deletion(-) diff --git a/superset/errors.py b/superset/errors.py index e8b387e81a100..8b0416d5b2b3e 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -48,7 +48,6 @@ class SupersetErrorType(str, Enum): # Security access errors TABLE_SECURITY_ACCESS_ERROR = "TABLE_SECURITY_ACCESS_ERROR" DATASOURCE_SECURITY_ACCESS_ERROR = "DATASOURCE_SECURITY_ACCESS_ERROR" - DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR" MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR" # Other errors From 607d4fc4e1ea55bd0be031dff4e7df480ae393fe Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 22:09:55 +0200 Subject: [PATCH 10/12] fix: remove unused import --- superset/security/manager.py | 1 - 1 file changed, 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index ab4db7ea281b6..43461f10fcfea 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -58,7 +58,6 @@ from superset.models.sql_lab import Query from superset.sql_parse import Table from superset.viz import BaseViz - from superset.models.dashboard import Dashboard logger = logging.getLogger(__name__) From 1ac6d8b43ebb28de5f243cf9fdf840aae57c0959 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Wed, 3 Feb 2021 22:55:01 +0200 Subject: [PATCH 11/12] fix: wrap the decorator to not block others --- superset/utils/decorators.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index eba05ed6aed2b..a736ebf17b077 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -15,6 +15,7 @@ # 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 @@ -79,10 +80,12 @@ 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: if is_feature_enabled("DASHBOARD_RBAC"): From 34deaf50b55bf9ec190d919b46a3bf1593a769e4 Mon Sep 17 00:00:00 2001 From: amitmiran137 Date: Thu, 4 Feb 2021 09:21:18 +0200 Subject: [PATCH 12/12] chore: reuse dashboard from decorator into function --- superset/utils/decorators.py | 15 ++++++-------- superset/views/core.py | 38 +++++++++++++++++++++--------------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index a736ebf17b077..e5cf713696fdd 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -87,24 +87,21 @@ def check_dashboard_access( 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: - from superset.models.dashboard import ( - Dashboard, - raise_for_dashboard_access, - ) - - dashboard = Dashboard.get(str(kwargs["dashboard_id_or_slug"])) - raise_for_dashboard_access(dashboard) - except DashboardAccessDeniedError as ex: return on_error(self, ex) except Exception as exception: raise exception - return f(self, *args, **kwargs) + return f(self, *args, dashboard=dashboard, **kwargs) return wrapper diff --git a/superset/views/core.py b/superset/views/core.py index b253907bc785f..cf0ca7d4dddd5 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -161,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 = ( @@ -868,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, @@ -1792,18 +1792,21 @@ def publish( # pylint: disable=no-self-use ) ) 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""" - dash = Dashboard.get(dashboard_id_or_slug) - 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(): @@ -1821,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") @@ -1839,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, @@ -1884,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 ), @@ -2237,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.