This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new f9109583ce fix: Allow dataset owners to explore their datasets (#20382)
f9109583ce is described below
commit f9109583ce1ede0cb2b9f4ad09452bba552a55ce
Author: Reese <[email protected]>
AuthorDate: Wed Jul 6 10:27:50 2022 -0400
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 9ea3050903..c60d285940 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")
+ 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 675cb63f1e..78c83f72eb 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1093,6 +1093,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:
@@ -1123,7 +1124,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_)
@@ -1149,6 +1152,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 94f595ce18..d696b4b744 100644
--- a/superset/views/utils.py
+++ b/superset/views/utils.py
@@ -32,6 +32,7 @@ from sqlalchemy.orm.exc import NoResultFound
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 ebad0dffd6..280d58774a 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 6311b3b982..81781d16c5 100644
--- a/tests/integration_tests/security_tests.py
+++ b/tests/integration_tests/security_tests.py
@@ -906,7 +906,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
@@ -914,12 +917,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"
)
@@ -928,6 +933,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 9ef9287217..64aefbf43a 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)
