From c9b0f6582f16bedf8466974691b6b1cde30114c0 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 | 17 +++++++++++++++-- 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, 37 insertions(+), 10 deletions(-) diff --git a/superset/charts/filters.py b/superset/charts/filters.py index 9ea3050903fdd..c60d285940839 100644 --- a/superset/charts/filters.py +++ b/superset/charts/filters.py @@ -20,7 +20,8 @@ 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 @@ 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), + ) ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 675cb63f1e41d..78c83f72ebe27 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1093,6 +1093,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: @@ -1123,7 +1124,9 @@ 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 is_owner(datasource_, getattr(g, "user", None)): break else: denied.add(table_) @@ -1149,6 +1152,7 @@ def raise_for_access( 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 94f595ce18aab..d696b4b744ac0 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -32,6 +32,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.sqla.models import SqlaTable from superset.datasource.dao import DatasourceDAO from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( @@ -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 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( diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py index 6311b3b982147..81781d16c5d35 100644 --- a/tests/integration_tests/security_tests.py +++ b/tests/integration_tests/security_tests.py @@ -906,7 +906,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 @@ -914,12 +917,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" ) @@ -928,6 +933,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)