You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ma...@apache.org on 2024/04/24 22:01:50 UTC
(superset) 01/03: fix: all_database_access should enable access to all datasets/charts/dashboards
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch all_database_access
in repository https://gitbox.apache.org/repos/asf/superset.git
commit de190b8ff90e184eb0e5a7b1c2b6f74d8f7c7695
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Wed Apr 24 10:02:32 2024 -0700
fix: all_database_access should enable access to all datasets/charts/dashboards
When granting `all_database_access`, one would expect to see all
dashboards, charts and datasets.
From my understanding `all_database_access` is practically identical to
`all_datasource_access`, and perhaps the two perms should be collapsed
into one. In the meantime, this addresses user confusion around
this perm not working according to its name.
---
superset/security/manager.py | 20 ++++-----
tests/integration_tests/base_tests.py | 5 +++
tests/integration_tests/security_tests.py | 72 ++++++++++++++++++++++++++++++-
3 files changed, 85 insertions(+), 12 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 5037e9bace..07b49543b8 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -402,23 +402,24 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return self.can_access("all_query_access", "all_query_access")
- def can_access_all_datasources(self) -> bool:
+ def can_access_all_databases(self) -> bool:
"""
- Return True if the user can access all the datasources, False otherwise.
+ Return True if the user can access all the databases, False otherwise.
- :returns: Whether the user can access all the datasources
+ :returns: Whether the user can access all the databases
"""
+ return self.can_access("all_database_access", "all_database_access")
- return self.can_access("all_datasource_access", "all_datasource_access")
-
- def can_access_all_databases(self) -> bool:
+ def can_access_all_datasources(self) -> bool:
"""
- Return True if the user can access all the databases, False otherwise.
+ Return True if the user can access all the datasources, False otherwise.
- :returns: Whether the user can access all the databases
+ :returns: Whether the user can access all the datasources
"""
- return self.can_access("all_database_access", "all_database_access")
+ return self.can_access_all_databases() or self.can_access(
+ "all_datasource_access", "all_datasource_access"
+ )
def can_access_database(self, database: "Database") -> bool:
"""
@@ -2430,7 +2431,6 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
if self.is_admin():
return
-
orig_resource = db.session.query(resource.__class__).get(resource.id)
owners = orig_resource.owners if hasattr(orig_resource, "owners") else []
diff --git a/tests/integration_tests/base_tests.py b/tests/integration_tests/base_tests.py
index 84041697f1..7b908f3633 100644
--- a/tests/integration_tests/base_tests.py
+++ b/tests/integration_tests/base_tests.py
@@ -248,8 +248,13 @@ class SupersetTestCase(TestCase):
datasource.query = Mock(return_value=results)
datasource.database = Mock()
datasource.database.db_engine_spec = Mock()
+ datasource.database.perm = "mock_database_perm"
+ datasource.schema_perm = "mock_schema_perm"
+ datasource.perm = "mock_datasource_perm"
+ datasource.__class__ = SqlaTable
datasource.database.db_engine_spec.mutate_expression_label = lambda x: x
datasource.owners = MagicMock()
+ datasource.id = 99999
return datasource
def get_resp(
diff --git a/tests/integration_tests/security_tests.py b/tests/integration_tests/security_tests.py
index 02e60a9277..0557e94752 100644
--- a/tests/integration_tests/security_tests.py
+++ b/tests/integration_tests/security_tests.py
@@ -28,8 +28,8 @@ import jwt
import prison
import pytest
-from flask import current_app
-from flask_appbuilder.security.sqla.models import Role
+from flask import current_app, g
+from flask_appbuilder.security.sqla.models import Role, User
from superset.daos.datasource import DatasourceDAO
from superset.models.dashboard import Dashboard
from superset import app, appbuilder, db, security_manager, viz
@@ -76,6 +76,60 @@ NEW_SECURITY_CONVERGE_VIEWS = (
)
+from contextlib import contextmanager
+from superset.utils.core import shortid
+
+
+@contextmanager
+def temporary_user(clone_user=None, extra_roles=None, extra_pvms=None):
+ temp_username = f"temp_user_{shortid()}"
+ print(temp_username)
+ temp_user = User(username=temp_username, email=f"{temp_username}@temp.com")
+ if clone_user:
+ temp_user.roles = clone_user.roles
+ temp_user.first_name = clone_user.first_name
+ temp_user.last_name = clone_user.last_name
+ else:
+ temp_user.first_name = "temp"
+ temp_user.last_name = "temp"
+
+ if clone_user:
+ temp_user.roles = clone_user.roles
+
+ if extra_roles:
+ temp_user.roles.extend(extra_roles)
+
+ pvms = []
+ temp_role = None
+ if extra_pvms:
+ temp_role = Role(name=f"tmp_role_{shortid()}")
+ for pvm in extra_pvms:
+ if isinstance(pvm, (tuple, list)):
+ pvms.append(security_manager.find_permission_view_menu(*pvm))
+ else:
+ pvms.append(pvm)
+ temp_role.permissions = pvms
+ temp_user.roles.append(temp_role)
+ db.session.add(temp_role)
+ db.session.commit()
+
+ # Add the temp user to the session and commit to apply changes for the test
+ db.session.add(temp_user)
+ db.session.commit()
+ previous_g_user = g.user
+ try:
+ # Yield control to the with block
+ g.user = temp_user
+ yield temp_user
+ finally:
+ # Revert changes after the test
+ if temp_role:
+ db.session.delete(temp_role)
+ db.session.delete(temp_user)
+ db.session.commit()
+ g.user = previous_g_user
+
+
def get_perm_tuples(role_name):
perm_set = set()
for perm in security_manager.find_role(role_name).permissions:
@@ -1887,6 +1941,20 @@ class TestSecurityManager(SupersetTestCase):
roles = security_manager.get_user_roles()
self.assertEqual([security_manager.get_public_role()], roles)
+ def test_all_database_access(self):
+ gamma_user = security_manager.find_user(username="gamma")
+ g.user = gamma_user
+
+ # Double checking that gamma users can't access all databases
+ assert not security_manager.can_access_all_databases()
+ assert not security_manager.can_access_datasource(self.get_datasource_mock())
+
+ all_db_pvm = ("all_database_access", "all_database_access")
+
+ with temporary_user(gamma_user, extra_pvms=[all_db_pvm]) as tmp_user:
+ assert security_manager.can_access_all_databases()
+ assert security_manager.can_access_datasource(self.get_datasource_mock())
+
class TestDatasources(SupersetTestCase):
@patch("superset.security.SupersetSecurityManager.can_access_database")