You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jo...@apache.org on 2022/07/06 17:25:01 UTC
[superset] branch revert-20382-fix/CLDN-1482 created (now b9de53194b)
This is an automated email from the ASF dual-hosted git repository.
johnbodley pushed a change to branch revert-20382-fix/CLDN-1482
in repository https://gitbox.apache.org/repos/asf/superset.git
at b9de53194b Revert "fix: Allow dataset owners to explore their datasets (#20382)"
This branch includes the following new commits:
new b9de53194b Revert "fix: Allow dataset owners to explore their datasets (#20382)"
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[superset] 01/01: Revert "fix: Allow dataset owners to explore their datasets (#20382)"
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
johnbodley pushed a commit to branch revert-20382-fix/CLDN-1482
in repository https://gitbox.apache.org/repos/asf/superset.git
commit b9de53194b12f4e3bc740b8bb9d358d96721439f
Author: John Bodley <45...@users.noreply.github.com>
AuthorDate: Wed Jul 6 10:24:51 2022 -0700
Revert "fix: Allow dataset owners to explore their datasets (#20382)"
This reverts commit f9109583ce1ede0cb2b9f4ad09452bba552a55ce.
---
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, 10 insertions(+), 37 deletions(-)
diff --git a/superset/charts/filters.py b/superset/charts/filters.py
index c60d285940..9ea3050903 100644
--- a/superset/charts/filters.py
+++ b/superset/charts/filters.py
@@ -20,8 +20,7 @@ from flask_babel import lazy_gettext as _
from sqlalchemy import and_, or_
from sqlalchemy.orm.query import Query
-from superset import db, security_manager
-from superset.connectors.sqla import models
+from superset import security_manager
from superset.connectors.sqla.models import SqlaTable
from superset.models.slice import Slice
from superset.views.base import BaseFilter
@@ -78,18 +77,6 @@ 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),
- models.SqlaTable.id.in_(owner_ids_query),
- )
+ or_(self.model.perm.in_(perms), self.model.schema_perm.in_(schema_perms))
)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 78c83f72eb..675cb63f1e 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1093,7 +1093,6 @@ 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:
@@ -1124,9 +1123,7 @@ 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
- ) or is_owner(datasource_, getattr(g, "user", None)):
+ if self.can_access("datasource_access", datasource_.perm):
break
else:
denied.add(table_)
@@ -1152,7 +1149,6 @@ 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 d696b4b744..94f595ce18 100644
--- a/superset/views/utils.py
+++ b/superset/views/utils.py
@@ -32,7 +32,6 @@ 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 (
@@ -427,7 +426,7 @@ def is_slice_in_container(
return False
-def is_owner(obj: Union[Dashboard, Slice, SqlaTable], user: User) -> bool:
+def is_owner(obj: Union[Dashboard, Slice], 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 280d58774a..ebad0dffd6 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, MagicMock
+from unittest.mock import Mock, patch
import pandas as pd
import pytest
@@ -252,7 +252,7 @@ class SupersetTestCase(TestCase):
@staticmethod
def get_datasource_mock() -> BaseDatasource:
- datasource = MagicMock()
+ datasource = Mock()
results = Mock()
results.query = Mock()
results.status = Mock()
@@ -266,7 +266,6 @@ 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 81781d16c5..6311b3b982 100644
--- a/tests/integration_tests/security_tests.py
+++ b/tests/integration_tests/security_tests.py
@@ -906,10 +906,7 @@ class TestSecurityManager(SupersetTestCase):
@patch("superset.security.SupersetSecurityManager.can_access")
@patch("superset.security.SupersetSecurityManager.can_access_schema")
- @patch("superset.views.utils.is_owner")
- def test_raise_for_access_datasource(
- self, mock_can_access_schema, mock_can_access, mock_is_owner
- ):
+ def test_raise_for_access_datasource(self, mock_can_access_schema, mock_can_access):
datasource = self.get_datasource_mock()
mock_can_access_schema.return_value = True
@@ -917,14 +914,12 @@ 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")
- @patch("superset.views.utils.is_owner")
- def test_raise_for_access_query(self, mock_can_access, mock_is_owner):
+ def test_raise_for_access_query(self, mock_can_access):
query = Mock(
database=get_example_database(), schema="bar", sql="SELECT * FROM foo"
)
@@ -933,7 +928,6 @@ 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 64aefbf43a..9ef9287217 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, client, app_context: AppContext) -> None:
+def test_query_no_access(mocker: MockFixture, 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,9 +282,7 @@ def test_query_no_access(mocker: MockFixture, client, app_context: AppContext) -
query_find_by_id,
return_value=Query(database=Database(), sql="select * from foo"),
)
- table = SqlaTable()
- table.owners = []
- mocker.patch(query_datasources_by_name, return_value=[table])
+ mocker.patch(query_datasources_by_name, return_value=[SqlaTable()])
mocker.patch(is_user_admin, return_value=False)
mocker.patch(is_owner, return_value=False)
mocker.patch(can_access, return_value=False)