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

@@ -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(
+        owner_ids_query = (
-            or_(self.model.perm.in_(perms), self.model.schema_perm.in_(schema_perms))
+            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),
+            )
         )

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)

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
 

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(

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)

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)