From 5f332381984362ffd3cc7be390b42f3d0581e4b3 Mon Sep 17 00:00:00 2001 From: reesercollins <10563996+reesercollins@users.noreply.github.com> Date: Tue, 14 Jun 2022 14:18:08 -0400 Subject: [PATCH 1/8] fix: Allow dataset owners to explore their datasets --- superset/charts/filters.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/superset/charts/filters.py b/superset/charts/filters.py index 9ea3050903fdd..965022aa614b1 100644 --- a/superset/charts/filters.py +++ b/superset/charts/filters.py @@ -20,9 +20,10 @@ from sqlalchemy import and_, or_ from sqlalchemy.orm.query import Query -from superset import security_manager +from superset import security_manager, db from superset.connectors.sqla.models import SqlaTable from superset.models.slice import Slice +from superset.connectors.sqla import models from superset.views.base import BaseFilter from superset.views.base_api import BaseFavoriteFilter @@ -77,6 +78,18 @@ def apply(self, query: Query, value: Any) -> Query: return query perms = security_manager.user_view_menu_names("datasource_access") schema_perms = security_manager.user_view_menu_names("schema_access") + 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)) + or_( + self.model.perm.in_(perms), + self.model.schema_perm.in_(schema_perms), + models.SqlaTable.id.in_(owner_ids_query), + ) ) From 7d78dcfdf3f5dc7c9374794dd32ec8acaec3e892 Mon Sep 17 00:00:00 2001 From: reesercollins <10563996+reesercollins@users.noreply.github.com> Date: Tue, 14 Jun 2022 15:24:42 -0400 Subject: [PATCH 2/8] Re-order imports --- superset/charts/filters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/charts/filters.py b/superset/charts/filters.py index 965022aa614b1..c60d285940839 100644 --- a/superset/charts/filters.py +++ b/superset/charts/filters.py @@ -20,10 +20,10 @@ from sqlalchemy import and_, or_ from sqlalchemy.orm.query import Query -from superset import security_manager, db +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.connectors.sqla import models from superset.views.base import BaseFilter from superset.views.base_api import BaseFavoriteFilter From c982f3bd899a99560e2e302915c41705358e6b42 Mon Sep 17 00:00:00 2001 From: reesercollins <10563996+reesercollins@users.noreply.github.com> Date: Tue, 14 Jun 2022 15:25:20 -0400 Subject: [PATCH 3/8] Give owners security manager permissions to their datasets --- superset/security/manager.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 890a09415ecb3..2124a65681dd3 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1067,7 +1067,10 @@ def raise_for_access( # 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 g.user in datasource_.owners + ): break else: denied.add(table_) @@ -1093,6 +1096,7 @@ def raise_for_access( if not ( self.can_access_schema(datasource) or self.can_access("datasource_access", datasource.perm or "") + or g.user in datasource.owners or ( should_check_dashboard_access and self.can_access_based_on_dashboard(datasource) From bd3a1ec620c843e465d4c8e8c76f02147118b15d Mon Sep 17 00:00:00 2001 From: reesercollins <10563996+reesercollins@users.noreply.github.com> Date: Tue, 14 Jun 2022 16:15:01 -0400 Subject: [PATCH 4/8] Update test suite --- superset/security/manager.py | 10 +++++----- tests/integration_tests/security_tests.py | 10 ++++++++-- tests/unit_tests/explore/utils_test.py | 6 ++++-- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 2124a65681dd3..76064d6d0e6ef 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1037,6 +1037,7 @@ def raise_for_access( 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: @@ -1067,10 +1068,9 @@ def raise_for_access( # Access to any datasource is suffice. for datasource_ in datasources: - if ( - self.can_access("datasource_access", datasource_.perm) - or g.user in datasource_.owners - ): + if self.can_access( + "datasource_access", datasource_.perm + ) or is_owner(datasource, getattr(g, "user", None)): break else: denied.add(table_) @@ -1096,7 +1096,7 @@ def raise_for_access( if not ( self.can_access_schema(datasource) or self.can_access("datasource_access", datasource.perm or "") - or g.user in datasource.owners + or is_owner(datasource, getattr(g, "user", None)) or ( should_check_dashboard_access and self.can_access_based_on_dashboard(datasource) diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 045e368296e85..ee39369507309 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -879,7 +879,10 @@ def test_can_access_table(self, mock_raise_for_access): @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 @@ -887,12 +890,14 @@ def test_raise_for_access_datasource(self, mock_can_access_schema, mock_can_acce 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" ) @@ -901,6 +906,7 @@ def test_raise_for_access_query(self, mock_can_access): 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 9ef92872177ee..64aefbf43adfa 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) From ce516c1599698b9d853464a91130b1d607954cfe Mon Sep 17 00:00:00 2001 From: reesercollins <10563996+reesercollins@users.noreply.github.com> Date: Tue, 14 Jun 2022 16:21:22 -0400 Subject: [PATCH 5/8] Add SqlaTable to is_owner types --- superset/security/manager.py | 2 +- superset/views/utils.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 76064d6d0e6ef..a12d940d8e9d7 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1070,7 +1070,7 @@ def raise_for_access( for datasource_ in datasources: if self.can_access( "datasource_access", datasource_.perm - ) or is_owner(datasource, getattr(g, "user", None)): + ) or is_owner(datasource_, getattr(g, "user", None)): break else: denied.add(table_) diff --git a/superset/views/utils.py b/superset/views/utils.py index 94f595ce18aab..0c0a00386f239 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -46,6 +46,7 @@ from superset.models.dashboard import Dashboard from superset.models.slice import Slice from superset.models.sql_lab import Query +from superset.superset.connectors.sqla.models import SqlaTable from superset.superset_typing import FormData from superset.utils.core import DatasourceType from superset.utils.decorators import stats_timing @@ -426,7 +427,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 From 9cb24a60da8a5012820990005dc89a0157c50871 Mon Sep 17 00:00:00 2001 From: reesercollins <10563996+reesercollins@users.noreply.github.com> Date: Tue, 14 Jun 2022 16:26:35 -0400 Subject: [PATCH 6/8] Add owners to datasource mock --- tests/integration_tests/base_tests.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/base_tests.py b/tests/integration_tests/base_tests.py index ebad0dffd63b6..280d58774ab06 100644 --- a/tests/integration_tests/base_tests.py +++ b/tests/integration_tests/base_tests.py @@ -21,7 +21,7 @@ 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 @@ def get_database_by_name(database_name: str = "main") -> Database: @staticmethod def get_datasource_mock() -> BaseDatasource: - datasource = Mock() + datasource = MagicMock() results = Mock() results.query = Mock() results.status = Mock() @@ -266,6 +266,7 @@ def get_datasource_mock() -> BaseDatasource: 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( From 1a0b774674aefb53184581533678ca679e5f82ce Mon Sep 17 00:00:00 2001 From: reesercollins <10563996+reesercollins@users.noreply.github.com> Date: Tue, 14 Jun 2022 16:29:23 -0400 Subject: [PATCH 7/8] Fix VSCode import error --- superset/views/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset/views/utils.py b/superset/views/utils.py index 0c0a00386f239..3ee0e54e37d25 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -32,7 +32,9 @@ 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.datasource.dao import DatasourceDAO +from superset.connectors.sqla.models import SqlaTable from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( CacheLoadError, @@ -46,7 +48,6 @@ from superset.models.dashboard import Dashboard from superset.models.slice import Slice from superset.models.sql_lab import Query -from superset.superset.connectors.sqla.models import SqlaTable from superset.superset_typing import FormData from superset.utils.core import DatasourceType from superset.utils.decorators import stats_timing From 4e8ff45d9904466ad422712a9872f2f967749487 Mon Sep 17 00:00:00 2001 From: Reese <10563996+reesercollins@users.noreply.github.com> Date: Tue, 21 Jun 2022 11:02:05 -0400 Subject: [PATCH 8/8] Fix merge error --- superset/views/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/superset/views/utils.py b/superset/views/utils.py index 3ee0e54e37d25..d696b4b744ac0 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -32,9 +32,8 @@ 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.datasource.dao import DatasourceDAO from superset.connectors.sqla.models import SqlaTable +from superset.datasource.dao import DatasourceDAO from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( CacheLoadError,