From 1fa370c48c6992455b305f8ccf1e98045f19f663 Mon Sep 17 00:00:00 2001 From: painyjames Date: Thu, 20 Jan 2022 18:02:21 +0000 Subject: [PATCH 1/5] datasource access to allow more granular access to tables --- superset/databases/filters.py | 15 ++++++-- superset/security/manager.py | 6 ++-- tests/integration_tests/core_tests.py | 34 +++++++++++++++++++ tests/integration_tests/datasets/api_tests.py | 6 ++-- 4 files changed, 54 insertions(+), 7 deletions(-) diff --git a/superset/databases/filters.py b/superset/databases/filters.py index 6fa9339c07f2d..9ef34467b193c 100644 --- a/superset/databases/filters.py +++ b/superset/databases/filters.py @@ -27,19 +27,30 @@ class DatabaseFilter(BaseFilter): # TODO(bogdan): consider caching. def schema_access_databases(self) -> Set[str]: # noqa pylint: disable=no-self-use return { - security_manager.unpack_schema_perm(vm)[0] + security_manager.unpack_perm(vm)[0] for vm in security_manager.user_view_menu_names("schema_access") } + def datasource_access_databases( # noqa pylint: disable=no-self-use + self, + ) -> Set[str]: + return { + security_manager.unpack_perm(vm)[0] + for vm in security_manager.user_view_menu_names("datasource_access") + } + def apply(self, query: Query, value: Any) -> Query: if security_manager.can_access_all_databases(): return query database_perms = security_manager.user_view_menu_names("database_access") - # TODO(bogdan): consider adding datasource access here as well. schema_access_databases = self.schema_access_databases() + + datasource_access_databases = self.datasource_access_databases() + return query.filter( or_( self.model.perm.in_(database_perms), self.model.database_name.in_(schema_access_databases), + self.model.database_name.in_(datasource_access_databases), ) ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 9e45e88afcda6..3a6e45112f4eb 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -237,7 +237,7 @@ def get_schema_perm( # pylint: disable=no-self-use return None - def unpack_schema_perm( # pylint: disable=no-self-use + def unpack_perm( # pylint: disable=no-self-use self, schema_permission: str ) -> Tuple[str, str]: # [database_name].[schema_name] @@ -532,7 +532,7 @@ def get_schemas_accessible_by_user( # schema_access accessible_schemas = { - self.unpack_schema_perm(s)[1] + self.unpack_perm(s)[1] for s in self.user_view_menu_names("schema_access") if s.startswith(f"[{database}].") } @@ -582,7 +582,7 @@ def get_datasources_accessible_by_user( # pylint: disable=invalid-name ) if schema: names = {d.table_name for d in user_datasources if d.schema == schema} - return [d for d in datasource_names if d in names] + return [d for d in datasource_names if d.table in names] full_names = {d.full_name for d in user_datasources} return [d for d in datasource_names if f"[{database}].[{d}]" in full_names] diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index 67e5e5da0a2df..1bb0fbddc6dfa 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -161,6 +161,40 @@ def test_get_superset_tables_not_allowed(self): rv = self.client.get(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_get_superset_tables_allowed(self): + session = db.session + table_name = "energy_usage" + role_name = "dummy_role" + self.logout() + self.login(username="gamma") + gamma_user = security_manager.find_user(username="gamma") + security_manager.add_role(role_name) + dummy_role = security_manager.find_role(role_name) + gamma_user.roles.append(dummy_role) + + tbl_id = self.table_ids.get(table_name) + table = db.session.query(SqlaTable).filter(SqlaTable.id == tbl_id).first() + table_perm = table.perm + + security_manager.add_permission_role( + dummy_role, + security_manager.find_permission_view_menu("datasource_access", table_perm), + ) + + session.commit() + + example_db = utils.get_example_database() + schema_name = self.default_schema_backend_map[example_db.backend] + uri = f"superset/tables/{example_db.id}/{schema_name}/{table_name}/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + + # cleanup + gamma_user = security_manager.find_user(username="gamma") + gamma_user.roles.remove(security_manager.find_role(role_name)) + session.commit() + def test_get_superset_tables_substr(self): example_db = utils.get_example_database() if example_db.backend in {"presto", "hive"}: diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 03209fd94fdf2..24ef97934912b 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -223,8 +223,10 @@ def test_get_dataset_related_database_gamma(self): rv = self.client.get(uri) assert rv.status_code == 200 response = json.loads(rv.data.decode("utf-8")) - assert response["count"] == 0 - assert response["result"] == [] + + assert response["count"] == 1 + main_db = get_main_database() + assert filter(lambda x: x.text == main_db, response["result"]) != [] @pytest.mark.usefixtures("load_energy_table_with_slice") def test_get_dataset_item(self): From c5ddcfc5fcdb49634f8ea39d0a4f6032d2b93537 Mon Sep 17 00:00:00 2001 From: painyjames Date: Tue, 25 Jan 2022 09:16:41 +0000 Subject: [PATCH 2/5] add test for tables not allowed with out datasource permission --- tests/integration_tests/core_tests.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index 1bb0fbddc6dfa..ff690b2081488 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -195,6 +195,31 @@ def test_get_superset_tables_allowed(self): gamma_user.roles.remove(security_manager.find_role(role_name)) session.commit() + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_get_superset_tables_not_allowed_with_out_permissions(self): + session = db.session + table_name = "energy_usage" + role_name = "dummy_role_no_table_access" + self.logout() + self.login(username="gamma") + gamma_user = security_manager.find_user(username="gamma") + security_manager.add_role(role_name) + dummy_role = security_manager.find_role(role_name) + gamma_user.roles.append(dummy_role) + + session.commit() + + example_db = utils.get_example_database() + schema_name = self.default_schema_backend_map[example_db.backend] + uri = f"superset/tables/{example_db.id}/{schema_name}/{table_name}/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 404) + + # cleanup + gamma_user = security_manager.find_user(username="gamma") + gamma_user.roles.remove(security_manager.find_role(role_name)) + session.commit() + def test_get_superset_tables_substr(self): example_db = utils.get_example_database() if example_db.backend in {"presto", "hive"}: From b2b230cfae29d3043ba508e5b6032f98c65306c5 Mon Sep 17 00:00:00 2001 From: painyjames Date: Thu, 27 Jan 2022 15:06:05 +0000 Subject: [PATCH 3/5] using NamedTuple instead of Tuple --- superset/databases/filters.py | 17 ++++++----------- superset/security/manager.py | 18 ++++++++++++------ 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/superset/databases/filters.py b/superset/databases/filters.py index 9ef34467b193c..5936aa5af284a 100644 --- a/superset/databases/filters.py +++ b/superset/databases/filters.py @@ -25,27 +25,22 @@ class DatabaseFilter(BaseFilter): # TODO(bogdan): consider caching. - def schema_access_databases(self) -> Set[str]: # noqa pylint: disable=no-self-use - return { - security_manager.unpack_perm(vm)[0] - for vm in security_manager.user_view_menu_names("schema_access") - } - def datasource_access_databases( # noqa pylint: disable=no-self-use - self, + def can_access_databases( # noqa pylint: disable=no-self-use + self, view_menu_name: str, ) -> Set[str]: return { - security_manager.unpack_perm(vm)[0] - for vm in security_manager.user_view_menu_names("datasource_access") + security_manager.unpack_database_and_schema(vm).database + for vm in security_manager.user_view_menu_names(view_menu_name) } def apply(self, query: Query, value: Any) -> Query: if security_manager.can_access_all_databases(): return query database_perms = security_manager.user_view_menu_names("database_access") - schema_access_databases = self.schema_access_databases() + schema_access_databases = self.can_access_databases("schema_access") - datasource_access_databases = self.datasource_access_databases() + datasource_access_databases = self.can_access_databases("datasource_access") return query.filter( or_( diff --git a/superset/security/manager.py b/superset/security/manager.py index 3a6e45112f4eb..6b1a053ecca82 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -25,9 +25,9 @@ cast, Dict, List, + NamedTuple, Optional, Set, - Tuple, TYPE_CHECKING, Union, ) @@ -78,6 +78,11 @@ logger = logging.getLogger(__name__) +class DatabaseAndSchema(NamedTuple): + database: str + schema: str + + class SupersetSecurityListWidget(ListWidget): # pylint: disable=too-few-public-methods """ Redeclaring to avoid circular imports @@ -237,13 +242,14 @@ def get_schema_perm( # pylint: disable=no-self-use return None - def unpack_perm( # pylint: disable=no-self-use + def unpack_database_and_schema( # pylint: disable=no-self-use self, schema_permission: str - ) -> Tuple[str, str]: - # [database_name].[schema_name] + ) -> DatabaseAndSchema: + # [database_name].[schema|table] + schema_name = schema_permission.split(".")[1][1:-1] database_name = schema_permission.split(".")[0][1:-1] - return database_name, schema_name + return DatabaseAndSchema(database_name, schema_name) def can_access(self, permission_name: str, view_name: str) -> bool: """ @@ -532,7 +538,7 @@ def get_schemas_accessible_by_user( # schema_access accessible_schemas = { - self.unpack_perm(s)[1] + self.unpack_database_and_schema(s).schema for s in self.user_view_menu_names("schema_access") if s.startswith(f"[{database}].") } From 24455bfbca82a2a7c05f33d5359179b05dcb789c Mon Sep 17 00:00:00 2001 From: Victor Arbues Date: Wed, 9 Feb 2022 11:57:04 +0000 Subject: [PATCH 4/5] Update superset/databases/filters.py Co-authored-by: Yongjie Zhao --- superset/databases/filters.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/superset/databases/filters.py b/superset/databases/filters.py index 5936aa5af284a..4464b40a56b60 100644 --- a/superset/databases/filters.py +++ b/superset/databases/filters.py @@ -45,7 +45,6 @@ def apply(self, query: Query, value: Any) -> Query: return query.filter( or_( self.model.perm.in_(database_perms), - self.model.database_name.in_(schema_access_databases), - self.model.database_name.in_(datasource_access_databases), + self.model.database_name.in_([*schema_access_databases, *datasource_access_databases]), ) ) From ed7c36f0cc9c035dcb735106fc4bb78cd7ed0fc1 Mon Sep 17 00:00:00 2001 From: painyjames Date: Wed, 9 Feb 2022 13:10:45 +0000 Subject: [PATCH 5/5] correct line too long lint issue --- superset/databases/filters.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/superset/databases/filters.py b/superset/databases/filters.py index 4464b40a56b60..bee7d2c7b2134 100644 --- a/superset/databases/filters.py +++ b/superset/databases/filters.py @@ -45,6 +45,8 @@ def apply(self, query: Query, value: Any) -> Query: return query.filter( or_( self.model.perm.in_(database_perms), - self.model.database_name.in_([*schema_access_databases, *datasource_access_databases]), + self.model.database_name.in_( + [*schema_access_databases, *datasource_access_databases] + ), ) )