From 82e6e03aa8d97ce56997f5e02ddaa2251192ee70 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 19 Sep 2022 12:12:36 +0100 Subject: [PATCH 1/4] feat: new config to filter specific users from dropdown lists --- superset/charts/api.py | 7 +++++-- superset/config.py | 5 +++++ superset/dashboards/api.py | 6 +++++- superset/datasets/api.py | 8 ++++++-- superset/queries/api.py | 7 +++++-- superset/reports/api.py | 5 ++++- superset/views/filters.py | 33 ++++++++++++++++++++++++++++++++- 7 files changed, 62 insertions(+), 9 deletions(-) diff --git a/superset/charts/api.py b/superset/charts/api.py index 8a111b005d2c2..46279e7a295a8 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -80,7 +80,7 @@ requires_json, statsd_metrics, ) -from superset.views.filters import FilterRelatedOwners +from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners logger = logging.getLogger(__name__) config = app.config @@ -234,7 +234,10 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]: "slices": ("slice_name", "asc"), "owners": ("first_name", "asc"), } - + filter_rel_fields = { + "owners": [["id", BaseFilterRelatedUsers, lambda: []]], + "created_by": [["id", BaseFilterRelatedUsers, lambda: []]], + } related_field_filters = { "owners": RelatedFieldFilter("first_name", FilterRelatedOwners), "created_by": RelatedFieldFilter("first_name", FilterRelatedOwners), diff --git a/superset/config.py b/superset/config.py index e659d7ba83023..8d00e519a8ed3 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1099,6 +1099,11 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument return msg +# Define a list of usernames to be excluded from all dropdown lists of users +# Owners, filters for created_by, etc. +EXCLUDE_USER_USERNAMES: List[str] = [] + + # This auth provider is used by background (offline) tasks that need to access # protected resources. Can be overridden by end users in order to support # custom auth mechanisms diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 9f440566cf5cb..5d503b09d908b 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -94,7 +94,7 @@ requires_json, statsd_metrics, ) -from superset.views.filters import FilterRelatedOwners +from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners logger = logging.getLogger(__name__) @@ -240,6 +240,10 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]: "owners": ("first_name", "asc"), "roles": ("name", "asc"), } + filter_rel_fields = { + "owners": [["id", BaseFilterRelatedUsers, lambda: []]], + "created_by": [["id", BaseFilterRelatedUsers, lambda: []]], + } related_field_filters = { "owners": RelatedFieldFilter("first_name", FilterRelatedOwners), "roles": RelatedFieldFilter("name", FilterRelatedRoles), diff --git a/superset/datasets/api.py b/superset/datasets/api.py index 86faa8bb9df1c..65b660dba640b 100644 --- a/superset/datasets/api.py +++ b/superset/datasets/api.py @@ -71,7 +71,7 @@ requires_json, statsd_metrics, ) -from superset.views.filters import FilterRelatedOwners +from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners logger = logging.getLogger(__name__) @@ -214,6 +214,11 @@ class DatasetRestApi(BaseSupersetModelRestApi): "extra", ] openapi_spec_tag = "Datasets" + + filter_rel_fields = { + "owners": [["id", BaseFilterRelatedUsers, lambda: []]], + "database": [["id", DatabaseFilter, lambda: []]], + } related_field_filters = { "owners": RelatedFieldFilter("first_name", FilterRelatedOwners), "database": "database_name", @@ -223,7 +228,6 @@ class DatasetRestApi(BaseSupersetModelRestApi): "id": [DatasetCertifiedFilter], } search_columns = ["id", "database", "owners", "schema", "sql", "table_name"] - filter_rel_fields = {"database": [["id", DatabaseFilter, lambda: []]]} allowed_rel_fields = {"database", "owners"} allowed_distinct_fields = {"schema"} diff --git a/superset/queries/api.py b/superset/queries/api.py index 460e2dd4667e0..83cb504937a4c 100644 --- a/superset/queries/api.py +++ b/superset/queries/api.py @@ -24,7 +24,7 @@ from superset.queries.filters import QueryFilter from superset.queries.schemas import openapi_spec_methods_override, QuerySchema from superset.views.base_api import BaseSupersetModelRestApi, RelatedFieldFilter -from superset.views.filters import FilterRelatedOwners +from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners logger = logging.getLogger(__name__) @@ -109,7 +109,10 @@ class QueryRestApi(BaseSupersetModelRestApi): "tab_name", "user.first_name", ] - + filter_rel_fields = { + "created_by": [["id", BaseFilterRelatedUsers, lambda: []]], + "user": [["id", BaseFilterRelatedUsers, lambda: []]], + } related_field_filters = { "created_by": RelatedFieldFilter("first_name", FilterRelatedOwners), "user": RelatedFieldFilter("first_name", FilterRelatedOwners), diff --git a/superset/reports/api.py b/superset/reports/api.py index 46480da9995ca..565d05c7033e5 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -57,7 +57,7 @@ requires_json, statsd_metrics, ) -from superset.views.filters import FilterRelatedOwners +from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners logger = logging.getLogger(__name__) @@ -204,10 +204,13 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: ] search_filters = {"name": [ReportScheduleAllTextFilter]} allowed_rel_fields = {"owners", "chart", "dashboard", "database", "created_by"} + filter_rel_fields = { "chart": [["id", ChartFilter, lambda: []]], "dashboard": [["id", DashboardAccessFilter, lambda: []]], "database": [["id", DatabaseFilter, lambda: []]], + "owners": [["id", BaseFilterRelatedUsers, lambda: []]], + "created_by": [["id", BaseFilterRelatedUsers, lambda: []]], } text_field_rel_fields = { "dashboard": "dashboard_title", diff --git a/superset/views/filters.py b/superset/views/filters.py index 3a503e66b614a..090234421ae63 100644 --- a/superset/views/filters.py +++ b/superset/views/filters.py @@ -14,15 +14,19 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import logging from typing import Any, cast, Optional +from flask import current_app from flask_appbuilder.models.filters import BaseFilter from flask_babel import lazy_gettext -from sqlalchemy import or_ +from sqlalchemy import and_, or_ from sqlalchemy.orm import Query from superset import security_manager +logger = logging.getLogger(__name__) + class FilterRelatedOwners(BaseFilter): # pylint: disable=too-few-public-methods @@ -48,3 +52,30 @@ def apply(self, query: Query, value: Optional[Any]) -> Query: user_model.username.ilike(like_value), ) ) + + +class BaseFilterRelatedUsers(BaseFilter): # pylint: disable=too-few-public-methods + + """ + Filter to apply on related users. Will exclude users in EXCLUDE_USER_USERNAMES + + Use in the api by adding something like: + ``` + filter_rel_fields = { + "owners": [["id", BaseFilterRelatedUsers, lambda: []]], + "created_by": [["id", BaseFilterRelatedUsers, lambda: []]], + } + ``` + """ + + name = lazy_gettext("username") + arg_name = "username" + + def apply(self, query: Query, value: Optional[Any]) -> Query: + user_model = security_manager.user_model + query_ = query.filter( + and_( + user_model.username.not_in(current_app.config["EXCLUDE_USER_USERNAMES"]) + ) + ) + return query_ From fd7599481cc69da6a38f12ca55ca15b6dff5cfec Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 19 Sep 2022 14:49:51 +0100 Subject: [PATCH 2/4] check current base test fixture users --- tests/integration_tests/base_api_tests.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/integration_tests/base_api_tests.py b/tests/integration_tests/base_api_tests.py index 16cc8cc18d045..0db8f9045ee8b 100644 --- a/tests/integration_tests/base_api_tests.py +++ b/tests/integration_tests/base_api_tests.py @@ -288,6 +288,26 @@ def test_get_filter_related_owners(self): # TODO Check me assert expected_results == sorted_results + def test_get_base_filter_related_owners(self): + """ + API: Test get base filter related owners + """ + self.login(username="admin") + uri = f"api/v1/{self.resource_name}/related/owners" + users = db.session.query(security_manager.user_model).all() + expected_users = [str(user) for user in users] + + rv = self.client.get(uri) + assert rv.status_code == 200 + response = json.loads(rv.data.decode("utf-8")) + assert response["count"] == len(users) + # This needs to be implemented like this, because ordering varies between + # postgres and mysql + response_users = [result["text"] for result in response["result"]] + raise Exception(expected_users) + for expected_user in expected_users: + assert expected_user in response_users + def test_get_ids_related_owners(self): """ API: Test get filter related owners From 9edb0105a53fa8ad2fe5826714c6e1339759b4bf Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 19 Sep 2022 15:30:12 +0100 Subject: [PATCH 3/4] fix test --- tests/integration_tests/base_api_tests.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/integration_tests/base_api_tests.py b/tests/integration_tests/base_api_tests.py index 0db8f9045ee8b..ce99a778549de 100644 --- a/tests/integration_tests/base_api_tests.py +++ b/tests/integration_tests/base_api_tests.py @@ -294,19 +294,23 @@ def test_get_base_filter_related_owners(self): """ self.login(username="admin") uri = f"api/v1/{self.resource_name}/related/owners" + self.app.config["EXCLUDE_USER_USERNAMES"] = ["gamma"] + gamma_user = ( + db.session.query(security_manager.user_model) + .filter(security_manager.user_model.username == "gamma") + .one_or_none() + ) + assert gamma_user is not None users = db.session.query(security_manager.user_model).all() - expected_users = [str(user) for user in users] rv = self.client.get(uri) assert rv.status_code == 200 response = json.loads(rv.data.decode("utf-8")) - assert response["count"] == len(users) - # This needs to be implemented like this, because ordering varies between - # postgres and mysql + assert response["count"] == len(users) - 1 response_users = [result["text"] for result in response["result"]] - raise Exception(expected_users) - for expected_user in expected_users: - assert expected_user in response_users + assert "gamma user" not in response_users + # revert the config change + self.app.config["EXCLUDE_USER_USERNAMES"] = [] def test_get_ids_related_owners(self): """ From 0d625000892c595f10477320b88faefec2c12caf Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 22 Sep 2022 12:26:19 +0100 Subject: [PATCH 4/4] address comments --- superset/config.py | 4 ++- superset/security/manager.py | 16 +++++++++++ superset/views/filters.py | 11 ++++---- tests/integration_tests/base_api_tests.py | 34 +++++++++++++++++++++-- tests/integration_tests/conftest.py | 34 ++++++++++++++++++++++- 5 files changed, 89 insertions(+), 10 deletions(-) diff --git a/superset/config.py b/superset/config.py index 8d00e519a8ed3..43567c01c5ea2 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1101,7 +1101,9 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument # Define a list of usernames to be excluded from all dropdown lists of users # Owners, filters for created_by, etc. -EXCLUDE_USER_USERNAMES: List[str] = [] +# The users can also be excluded by overriding the get_exclude_users_from_lists method +# in security manager +EXCLUDE_USERS_FROM_LISTS: Optional[List[str]] = None # This auth provider is used by background (offline) tasks that need to access diff --git a/superset/security/manager.py b/superset/security/manager.py index 260dbb49ee7d4..eaf827ef90463 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1702,6 +1702,22 @@ def on_permission_view_after_delete( :param target: The mapped instance being persisted """ + @staticmethod + def get_exclude_users_from_lists() -> List[str]: + """ + Override to dynamically identify a list of usernames to exclude from + all UI dropdown lists, owners, created_by filters etc... + + It will exclude all users from the all endpoints of the form + ``/api/v1//related/`` + + Optionally you can also exclude them using the `EXCLUDE_USERS_FROM_LISTS` + config setting. + + :return: A list of usernames + """ + return [] + def raise_for_access( # pylint: disable=too-many-arguments,too-many-locals self, diff --git a/superset/views/filters.py b/superset/views/filters.py index 090234421ae63..7f3dfd29306e7 100644 --- a/superset/views/filters.py +++ b/superset/views/filters.py @@ -57,7 +57,7 @@ def apply(self, query: Query, value: Optional[Any]) -> Query: class BaseFilterRelatedUsers(BaseFilter): # pylint: disable=too-few-public-methods """ - Filter to apply on related users. Will exclude users in EXCLUDE_USER_USERNAMES + Filter to apply on related users. Will exclude users in EXCLUDE_USERS_FROM_LISTS Use in the api by adding something like: ``` @@ -73,9 +73,10 @@ class BaseFilterRelatedUsers(BaseFilter): # pylint: disable=too-few-public-meth def apply(self, query: Query, value: Optional[Any]) -> Query: user_model = security_manager.user_model - query_ = query.filter( - and_( - user_model.username.not_in(current_app.config["EXCLUDE_USER_USERNAMES"]) - ) + exclude_users = ( + security_manager.get_exclude_users_from_lists() + if current_app.config["EXCLUDE_USERS_FROM_LISTS"] is None + else current_app.config["EXCLUDE_USERS_FROM_LISTS"] ) + query_ = query.filter(and_(user_model.username.not_in(exclude_users))) return query_ diff --git a/tests/integration_tests/base_api_tests.py b/tests/integration_tests/base_api_tests.py index ce99a778549de..66544f1447e05 100644 --- a/tests/integration_tests/base_api_tests.py +++ b/tests/integration_tests/base_api_tests.py @@ -16,6 +16,8 @@ # under the License. # isort:skip_file import json +from unittest.mock import patch + from tests.integration_tests.fixtures.world_bank_dashboard import ( load_world_bank_dashboard_with_slices, load_world_bank_data, @@ -32,6 +34,7 @@ from superset.views.base_api import BaseSupersetModelRestApi, requires_json from .base_tests import SupersetTestCase +from .conftest import with_config class Model1Api(BaseSupersetModelRestApi): @@ -288,13 +291,13 @@ def test_get_filter_related_owners(self): # TODO Check me assert expected_results == sorted_results + @with_config({"EXCLUDE_USERS_FROM_LISTS": ["gamma"]}) def test_get_base_filter_related_owners(self): """ API: Test get base filter related owners """ self.login(username="admin") uri = f"api/v1/{self.resource_name}/related/owners" - self.app.config["EXCLUDE_USER_USERNAMES"] = ["gamma"] gamma_user = ( db.session.query(security_manager.user_model) .filter(security_manager.user_model.username == "gamma") @@ -309,8 +312,33 @@ def test_get_base_filter_related_owners(self): assert response["count"] == len(users) - 1 response_users = [result["text"] for result in response["result"]] assert "gamma user" not in response_users - # revert the config change - self.app.config["EXCLUDE_USER_USERNAMES"] = [] + + @patch( + "superset.security.SupersetSecurityManager.get_exclude_users_from_lists", + return_value=["gamma"], + ) + def test_get_base_filter_related_owners_on_sm( + self, mock_get_exclude_users_from_list + ): + """ + API: Test get base filter related owners using security manager + """ + self.login(username="admin") + uri = f"api/v1/{self.resource_name}/related/owners" + gamma_user = ( + db.session.query(security_manager.user_model) + .filter(security_manager.user_model.username == "gamma") + .one_or_none() + ) + assert gamma_user is not None + users = db.session.query(security_manager.user_model).all() + + rv = self.client.get(uri) + assert rv.status_code == 200 + response = json.loads(rv.data.decode("utf-8")) + assert response["count"] == len(users) - 1 + response_users = [result["text"] for result in response["result"]] + assert "gamma user" not in response_users def test_get_ids_related_owners(self): """ diff --git a/tests/integration_tests/conftest.py b/tests/integration_tests/conftest.py index c117422adc1bc..463f93b833681 100644 --- a/tests/integration_tests/conftest.py +++ b/tests/integration_tests/conftest.py @@ -19,7 +19,7 @@ import contextlib import functools import os -from typing import Any, Callable, Optional, TYPE_CHECKING +from typing import Any, Callable, Dict, Optional, TYPE_CHECKING from unittest.mock import patch import pytest @@ -255,6 +255,38 @@ def wrapper(*args, **kwargs): return decorate +def with_config(override_config: Dict[str, Any]): + """ + Use this decorator to mock specific config keys. + + Usage: + + class TestYourFeature(SupersetTestCase): + + @with_config({"SOME_CONFIG": True}) + def test_your_config(self): + self.assertEqual(curren_app.config["SOME_CONFIG"), True) + + """ + + def decorate(test_fn): + config_backup = {} + + def wrapper(*args, **kwargs): + from flask import current_app + + for key, value in override_config.items(): + config_backup[key] = current_app.config[key] + current_app.config[key] = value + test_fn(*args, **kwargs) + for key, value in config_backup.items(): + current_app.config[key] = value + + return functools.update_wrapper(wrapper, test_fn) + + return decorate + + @pytest.fixture def virtual_dataset(): from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn