From bba486b08c98b1bf51ff755236d5fa32b3deacb8 Mon Sep 17 00:00:00 2001 From: Reese <10563996+reesercollins@users.noreply.github.com> Date: Wed, 6 Jul 2022 10:27:50 -0400 Subject: [PATCH] fix: Allow dataset owners to explore their datasets (#20382) * fix: Allow dataset owners to explore their datasets * Re-order imports * Give owners security manager permissions to their datasets * Update test suite * Add SqlaTable to is_owner types * Add owners to datasource mock * Fix VSCode import error * Fix merge error --- superset/charts/filters.py | 19 ++++++++++++++++--- superset/security/manager.py | 6 +++++- superset/views/utils.py | 3 ++- tests/integration_tests/base_tests.py | 5 +++-- tests/integration_tests/security_tests.py | 10 ++++++++-- tests/unit_tests/explore/utils_test.py | 6 ++++-- 6 files changed, 38 insertions(+), 11 deletions(-) diff --git a/superset/charts/filters.py b/superset/charts/filters.py index 9ea3050903f..c60d2859408 100644 --- a/superset/charts/filters.py +++ b/superset/charts/filters.py @@ -20,7 +20,8 @@ from flask_babel import lazy_gettext as _ from sqlalchemy import and_, or_ from sqlalchemy.orm.query import Query -from superset import security_manager +from superset import db, security_manager +from superset.connectors.sqla import models from superset.connectors.sqla.models import SqlaTable from superset.models.slice import Slice from superset.views.base import BaseFilter @@ -77,6 +78,18 @@ class ChartFilter(BaseFilter): # pylint: disable=too-few-public-methods return query perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") - return query.filter( - or_(self.model.perm.in_(perms), self.model.schema_perm.in_(schema_perms)) + owner_ids_query = ( + db.session.query(models.SqlaTable.id) + .join(models.SqlaTable.owners) + .filter( + security_manager.user_model.id + == security_manager.user_model.get_user_id() + ) + ) + return query.filter( + or_( + self.model.perm.in_(perms), + self.model.schema_perm.in_(schema_perms), + models.SqlaTable.id.in_(owner_ids_query), + ) ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 6157959aa37..d5455e73fcd 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1033,6 +1033,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods from superset.connectors.sqla.models import SqlaTable from superset.extensions import feature_flag_manager from superset.sql_parse import Table + from superset.views.utils import is_owner if database and table or query: if query: @@ -1063,7 +1064,9 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods # Access to any datasource is suffice. for datasource_ in datasources: - if self.can_access("datasource_access", datasource_.perm): + if self.can_access( + "datasource_access", datasource_.perm + ) or is_owner(datasource_, getattr(g, "user", None)): break else: denied.add(table_) @@ -1089,6 +1092,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods if not ( self.can_access_schema(datasource) or self.can_access("datasource_access", datasource.perm or "") + or is_owner(datasource, getattr(g, "user", None)) or ( should_check_dashboard_access and self.can_access_based_on_dashboard(datasource) diff --git a/superset/views/utils.py b/superset/views/utils.py index 42a86d9eb1b..1f72324a967 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -33,6 +33,7 @@ import superset.models.core as models from superset import app, dataframe, db, result_set, viz from superset.common.db_query_status import QueryStatus from superset.connectors.connector_registry import ConnectorRegistry +from superset.connectors.sqla.models import SqlaTable from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( CacheLoadError, @@ -423,7 +424,7 @@ def is_slice_in_container( return False -def is_owner(obj: Union[Dashboard, Slice], user: User) -> bool: +def is_owner(obj: Union[Dashboard, Slice, SqlaTable], user: User) -> bool: """Check if user is owner of the slice""" return obj and user in obj.owners diff --git a/tests/integration_tests/base_tests.py b/tests/integration_tests/base_tests.py index ebad0dffd63..280d58774ab 100644 --- a/tests/integration_tests/base_tests.py +++ b/tests/integration_tests/base_tests.py @@ -21,7 +21,7 @@ import imp import json from contextlib import contextmanager from typing import Any, Dict, Union, List, Optional -from unittest.mock import Mock, patch +from unittest.mock import Mock, patch, MagicMock import pandas as pd import pytest @@ -252,7 +252,7 @@ class SupersetTestCase(TestCase): @staticmethod def get_datasource_mock() -> BaseDatasource: - datasource = Mock() + datasource = MagicMock() results = Mock() results.query = Mock() results.status = Mock() @@ -266,6 +266,7 @@ class SupersetTestCase(TestCase): datasource.database = Mock() datasource.database.db_engine_spec = Mock() datasource.database.db_engine_spec.mutate_expression_label = lambda x: x + datasource.owners = MagicMock() return datasource def get_resp( diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index a70146db683..9de83f665a9 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -878,7 +878,10 @@ class TestSecurityManager(SupersetTestCase): @patch("superset.security.SupersetSecurityManager.can_access") @patch("superset.security.SupersetSecurityManager.can_access_schema") - def test_raise_for_access_datasource(self, mock_can_access_schema, mock_can_access): + @patch("superset.views.utils.is_owner") + def test_raise_for_access_datasource( + self, mock_can_access_schema, mock_can_access, mock_is_owner + ): datasource = self.get_datasource_mock() mock_can_access_schema.return_value = True @@ -886,12 +889,14 @@ class TestSecurityManager(SupersetTestCase): mock_can_access.return_value = False mock_can_access_schema.return_value = False + mock_is_owner.return_value = False with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(datasource=datasource) @patch("superset.security.SupersetSecurityManager.can_access") - def test_raise_for_access_query(self, mock_can_access): + @patch("superset.views.utils.is_owner") + def test_raise_for_access_query(self, mock_can_access, mock_is_owner): query = Mock( database=get_example_database(), schema="bar", sql="SELECT * FROM foo" ) @@ -900,6 +905,7 @@ class TestSecurityManager(SupersetTestCase): security_manager.raise_for_access(query=query) mock_can_access.return_value = False + mock_is_owner.return_value = False with self.assertRaises(SupersetSecurityException): security_manager.raise_for_access(query=query) diff --git a/tests/unit_tests/explore/utils_test.py b/tests/unit_tests/explore/utils_test.py index 9ef92872177..64aefbf43ad 100644 --- a/tests/unit_tests/explore/utils_test.py +++ b/tests/unit_tests/explore/utils_test.py @@ -271,7 +271,7 @@ def test_query_has_access(mocker: MockFixture, app_context: AppContext) -> None: ) -def test_query_no_access(mocker: MockFixture, app_context: AppContext) -> None: +def test_query_no_access(mocker: MockFixture, client, app_context: AppContext) -> None: from superset.connectors.sqla.models import SqlaTable from superset.explore.utils import check_datasource_access from superset.models.core import Database @@ -282,7 +282,9 @@ def test_query_no_access(mocker: MockFixture, app_context: AppContext) -> None: query_find_by_id, return_value=Query(database=Database(), sql="select * from foo"), ) - mocker.patch(query_datasources_by_name, return_value=[SqlaTable()]) + table = SqlaTable() + table.owners = [] + mocker.patch(query_datasources_by_name, return_value=[table]) mocker.patch(is_user_admin, return_value=False) mocker.patch(is_owner, return_value=False) mocker.patch(can_access, return_value=False)