From 176c1df4e1ff13d25468082bb5d95d90803efb9d Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Fri, 2 Dec 2022 14:48:32 +0200 Subject: [PATCH 01/20] feat(thumbnails): add support for user specific thumbs --- superset/charts/api.py | 30 ++++++++---- superset/config.py | 48 ++++++++++++++++--- superset/dashboards/api.py | 29 +++++++---- superset/models/dashboard.py | 3 +- superset/models/slice.py | 3 +- superset/tasks/thumbnails.py | 26 +++++----- superset/thumbnails/__init__.py | 16 +++++++ superset/thumbnails/exceptions.py | 21 ++++++++ superset/thumbnails/types.py | 23 +++++++++ superset/thumbnails/utils.py | 80 +++++++++++++++++++++++++++++++ 10 files changed, 242 insertions(+), 37 deletions(-) create mode 100644 superset/thumbnails/__init__.py create mode 100644 superset/thumbnails/exceptions.py create mode 100644 superset/thumbnails/types.py create mode 100644 superset/thumbnails/utils.py diff --git a/superset/charts/api.py b/superset/charts/api.py index 046379e7f5a90..d464efb45c61a 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -18,7 +18,7 @@ import logging from datetime import datetime from io import BytesIO -from typing import Any, Optional +from typing import Any, cast, Optional from zipfile import is_zipfile, ZipFile from flask import redirect, request, Response, send_file, url_for @@ -30,7 +30,7 @@ from werkzeug.wrappers import Response as WerkzeugResponse from werkzeug.wsgi import FileWrapper -from superset import app, is_feature_enabled, thumbnail_cache +from superset import app, is_feature_enabled, thumbnail_cache, security_manager from superset.charts.commands.bulk_delete import BulkDeleteChartCommand from superset.charts.commands.create import CreateChartCommand from superset.charts.commands.delete import DeleteChartCommand @@ -75,6 +75,7 @@ from superset.extensions import event_logger from superset.models.slice import Slice from superset.tasks.thumbnails import cache_chart_thumbnail +from superset.thumbnails.utils import get_chart_digest from superset.utils.screenshots import ChartScreenshot from superset.utils.urls import get_url_path from superset.views.base_api import ( @@ -557,12 +558,13 @@ def cache_screenshot(self, pk: int, **kwargs: Any) -> WerkzeugResponse: # Don't shrink the image if thumb_size is not specified thumb_size = rison_dict.get("thumb_size") or window_size - chart = self.datamodel.get(pk, self._base_filters) + chart = cast(Slice, self.datamodel.get(pk, self._base_filters)) if not chart: return self.response_404() + chart_digest = get_chart_digest(chart) chart_url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") - screenshot_obj = ChartScreenshot(chart_url, chart.digest) + screenshot_obj = ChartScreenshot(chart_url, chart_digest) cache_key = screenshot_obj.cache_key(window_size, thumb_size) image_url = get_url_path( "ChartRestApi.screenshot", pk=chart.id, digest=cache_key @@ -572,7 +574,7 @@ def trigger_celery() -> WerkzeugResponse: logger.info("Triggering screenshot ASYNC") kwargs = { "url": chart_url, - "digest": chart.digest, + "digest": chart_digest, "force": True, "window_size": window_size, "thumb_size": thumb_size, @@ -680,16 +682,23 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: 500: $ref: '#/components/responses/500' """ - chart = self.datamodel.get(pk, self._base_filters) + chart = cast(Slice, self.datamodel.get(pk, self._base_filters)) if not chart: return self.response_404() + username = user.username if (user := security_manager.current_user) else None url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") + chart_digest = get_chart_digest(chart) if kwargs["rison"].get("force", False): logger.info( "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) ) - cache_chart_thumbnail.delay(url, chart.digest, force=True) + cache_chart_thumbnail.delay( + username=username, + url=url, + digest=chart_digest, + force=True, + ) return self.response(202, message="OK Async") # fetch the chart screenshot using the current user and cache if set screenshot = ChartScreenshot(url, chart.digest).get_from_cache( @@ -701,7 +710,12 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: logger.info( "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) ) - cache_chart_thumbnail.delay(url, chart.digest, force=True) + cache_chart_thumbnail.delay( + username=username, + url=url, + digest=chart_digest, + force=True, + ) return self.response(202, message="OK Async") # If digests if chart.digest != digest: diff --git a/superset/config.py b/superset/config.py index f163997c6ee4b..91d34f17f2d8b 100644 --- a/superset/config.py +++ b/superset/config.py @@ -21,6 +21,8 @@ at the end of this file. """ # pylint: disable=too-many-lines +from __future__ import annotations + import imp # pylint: disable=deprecated-module import importlib.util import json @@ -39,6 +41,7 @@ Literal, Optional, Set, + Tuple, Type, TYPE_CHECKING, Union, @@ -60,6 +63,7 @@ from superset.reports.types import ReportScheduleExecutor from superset.stats_logger import DummyStatsLogger from superset.superset_typing import CacheConfig +from superset.thumbnails.types import ThumbnailExecutor from superset.utils.core import is_test, NO_TIME_RANGE, parse_boolean_string from superset.utils.encrypt import SQLAlchemyUtilsAdapter from superset.utils.log import DBEventLogger @@ -72,6 +76,8 @@ from superset.connectors.sqla.models import SqlaTable from superset.models.core import Database + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice # Realtime stats logger, a StatsD implementation exists STATS_LOGGER = DummyStatsLogger() @@ -573,11 +579,41 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # This is merely a default EXTRA_SEQUENTIAL_COLOR_SCHEMES: List[Dict[str, Any]] = [] +# When executing Alerts & Reports or Thumbnails as the Selenium user, this defines +# the username of the account used to render the queries and dashboards/charts +SELENIUM_USER: Optional[str] = "admin" + # --------------------------------------------------- # Thumbnail config (behind feature flag) -# Also used by Alerts & Reports # --------------------------------------------------- -THUMBNAIL_SELENIUM_USER = "admin" + +# To be able to have different thumbnails for different users, use this config to define +# 1. which user to execute the thumbnails as 2. custom callbacks for creating the +# digests for the dashboard and chart thumbnail images. To have unique thumbnails for +# all users, use the following example config: +from hashlib import md5 + +# THUMBNAIL_EXECUTOR_CONFIG: Optional[ +# Tuple[ +# ThumbnailExecutor, +# Optional[Callable[[Dashboard, models.User], str]], +# Optional[Callable[[Slice, models.User], str]], +# ] +# ] = ( +# ThumbnailExecutor.User, +# lambda x: md5(f"{x[0].digest()}\n{x[1].id}".encode("utf-8")).hexdigest(), +# lambda x: md5(f"{x[0].digest()}\n{x[1].id}".encode("utf-8")).hexdigest(), +# ) + +THUMBNAIL_EXECUTOR_CONFIG: Optional[ + Tuple[ + ThumbnailExecutor, + Optional[Callable[[Dashboard, models.User], str]], + Optional[Callable[[Slice, models.User], str]], + ] +] = None + + THUMBNAIL_CACHE_CONFIG: CacheConfig = { "CACHE_TYPE": "NullCache", "CACHE_NO_NULL_WARNING": True, @@ -930,7 +966,7 @@ class CeleryConfig: # pylint: disable=too-few-public-methods # return f'tmp_{schema}' # Function accepts database object, user object, schema name and sql that will be run. SQLLAB_CTAS_SCHEMA_NAME_FUNC: Optional[ - Callable[["Database", "models.User", str, str], str] + Callable[[Database, models.User, str, str], str] ] = None # If enabled, it can be used to store the results of long-running queries @@ -955,8 +991,8 @@ class CeleryConfig: # pylint: disable=too-few-public-methods # Function that creates upload directory dynamically based on the # database used, user and schema provided. def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint: disable=invalid-name - database: "Database", - user: "models.User", # pylint: disable=unused-argument + database: Database, + user: models.User, # pylint: disable=unused-argument schema: Optional[str], ) -> str: # Note the final empty path enforces a trailing slash. @@ -974,7 +1010,7 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint: disable=invalid-name # db configuration and a result of this function. # mypy doesn't catch that if case ensures list content being always str -ALLOWED_USER_CSV_SCHEMA_FUNC: Callable[["Database", "models.User"], List[str]] = ( +ALLOWED_USER_CSV_SCHEMA_FUNC: Callable[[Database, models.User], List[str]] = ( lambda database, user: [UPLOADED_CSV_HIVE_NAMESPACE] if UPLOADED_CSV_HIVE_NAMESPACE else [] diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 48185ec52690b..c2856fbb48bc8 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -20,7 +20,7 @@ import logging from datetime import datetime from io import BytesIO -from typing import Any, Callable, Optional +from typing import Any, Callable, cast, Optional from zipfile import is_zipfile, ZipFile from flask import make_response, redirect, request, Response, send_file, url_for @@ -33,7 +33,7 @@ from werkzeug.wrappers import Response as WerkzeugResponse from werkzeug.wsgi import FileWrapper -from superset import is_feature_enabled, thumbnail_cache +from superset import is_feature_enabled, thumbnail_cache, security_manager from superset.charts.schemas import ChartEntityResponseSchema from superset.commands.importers.exceptions import NoValidFilesFoundError from superset.commands.importers.v1.utils import get_contents_from_bundle @@ -95,6 +95,7 @@ statsd_metrics, ) from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners +from superset.thumbnails.utils import get_dashboard_digest logger = logging.getLogger(__name__) @@ -879,7 +880,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: 500: $ref: '#/components/responses/500' """ - dashboard = self.datamodel.get(pk, self._base_filters) + dashboard = cast(Dashboard, self.datamodel.get(pk, self._base_filters)) if not dashboard: return self.response_404() @@ -887,26 +888,38 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: "Superset.dashboard", dashboard_id_or_slug=dashboard.id ) # If force, request a screenshot from the workers + dashboard_digest = get_dashboard_digest(dashboard) + username = user.username if (user := security_manager.current_user) else None if kwargs["rison"].get("force", False): - cache_dashboard_thumbnail.delay(dashboard_url, dashboard.digest, force=True) + cache_dashboard_thumbnail.delay( + url=dashboard_url, + digest=dashboard_digest, + username=username, + force=True, + ) return self.response(202, message="OK Async") # fetch the dashboard screenshot using the current user and cache if set screenshot = DashboardScreenshot( - dashboard_url, dashboard.digest + dashboard_url, dashboard_digest ).get_from_cache(cache=thumbnail_cache) # If the screenshot does not exist, request one from the workers if not screenshot: self.incr_stats("async", self.thumbnail.__name__) - cache_dashboard_thumbnail.delay(dashboard_url, dashboard.digest, force=True) + cache_dashboard_thumbnail.delay( + username=username, + url=dashboard_url, + digest=dashboard_digest, + force=True, + ) return self.response(202, message="OK Async") # If digests - if dashboard.digest != digest: + if dashboard_digest != digest: self.incr_stats("redirect", self.thumbnail.__name__) return redirect( url_for( f"{self.__class__.__name__}.thumbnail", pk=pk, - digest=dashboard.digest, + digest=dashboard_digest, ) ) self.incr_stats("from_cache", self.thumbnail.__name__) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index a98d76e58162e..4c6955cc5ba19 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -55,6 +55,7 @@ from superset.models.slice import Slice from superset.models.user_attributes import UserAttribute from superset.tasks.thumbnails import cache_dashboard_thumbnail +from superset.thumbnails.utils import get_dashboard_digest from superset.utils import core as utils from superset.utils.core import get_user_id from superset.utils.decorators import debounce @@ -253,7 +254,7 @@ def thumbnail_url(self) -> str: Returns a thumbnail URL with a HEX digest. We want to avoid browser cache if the dashboard has changed """ - return f"/api/v1/dashboard/{self.id}/thumbnail/{self.digest}/" + return f"/api/v1/dashboard/{self.id}/thumbnail/{get_dashboard_digest(self)}/" @property def changed_by_name(self) -> str: diff --git a/superset/models/slice.py b/superset/models/slice.py index 32b347266d2b5..de5903197f1ec 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -43,6 +43,7 @@ from superset.legacy import update_time_range from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.tasks.thumbnails import cache_chart_thumbnail +from superset.thumbnails.utils import get_chart_digest from superset.utils import core as utils from superset.utils.hashing import md5_sha_from_str from superset.utils.memoized import memoized @@ -245,7 +246,7 @@ def thumbnail_url(self) -> str: Returns a thumbnail URL with a HEX digest. We want to avoid browser cache if the dashboard has changed """ - return f"/api/v1/chart/{self.id}/thumbnail/{self.digest}/" + return f"/api/v1/chart/{self.id}/thumbnail/{get_chart_digest(self)}/" @property def json_data(self) -> str: diff --git a/superset/tasks/thumbnails.py b/superset/tasks/thumbnails.py index 94b83ddb372cf..0a7899478893d 100644 --- a/superset/tasks/thumbnails.py +++ b/superset/tasks/thumbnails.py @@ -20,11 +20,10 @@ import logging from typing import Optional -from flask import current_app - -from superset import security_manager, thumbnail_cache +from superset import thumbnail_cache from superset.extensions import celery_app -from superset.utils.celery import session_scope +from superset.thumbnails.utils import get_executor +from superset.utils.core import override_user from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot from superset.utils.webdriver import WindowSize @@ -33,6 +32,7 @@ @celery_app.task(name="cache_chart_thumbnail", soft_time_limit=300) def cache_chart_thumbnail( + username: Optional[str], url: str, digest: str, force: bool = False, @@ -44,10 +44,8 @@ def cache_chart_thumbnail( return None logger.info("Caching chart: %s", url) screenshot = ChartScreenshot(url, digest) - with session_scope(nullpool=True) as session: - user = security_manager.get_user_by_username( - current_app.config["THUMBNAIL_SELENIUM_USER"], session=session - ) + user = get_executor(username) + with override_user(user): screenshot.compute_and_cache( user=user, cache=thumbnail_cache, @@ -60,17 +58,19 @@ def cache_chart_thumbnail( @celery_app.task(name="cache_dashboard_thumbnail", soft_time_limit=300) def cache_dashboard_thumbnail( - url: str, digest: str, force: bool = False, thumb_size: Optional[WindowSize] = None + username: Optional[str], + url: str, + digest: str, + force: bool = False, + thumb_size: Optional[WindowSize] = None, ) -> None: if not thumbnail_cache: logging.warning("No cache set, refusing to compute") return logger.info("Caching dashboard: %s", url) screenshot = DashboardScreenshot(url, digest) - with session_scope(nullpool=True) as session: - user = security_manager.get_user_by_username( - current_app.config["THUMBNAIL_SELENIUM_USER"], session=session - ) + user = get_executor(username=username) + with override_user(user): screenshot.compute_and_cache( user=user, cache=thumbnail_cache, diff --git a/superset/thumbnails/__init__.py b/superset/thumbnails/__init__.py new file mode 100644 index 0000000000000..13a83393a9124 --- /dev/null +++ b/superset/thumbnails/__init__.py @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. diff --git a/superset/thumbnails/exceptions.py b/superset/thumbnails/exceptions.py new file mode 100644 index 0000000000000..d19973ce87588 --- /dev/null +++ b/superset/thumbnails/exceptions.py @@ -0,0 +1,21 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from superset.exceptions import SupersetException + + +class ThumbnailUserNotFoundError(SupersetException): + pass diff --git a/superset/thumbnails/types.py b/superset/thumbnails/types.py new file mode 100644 index 0000000000000..8a586666968a9 --- /dev/null +++ b/superset/thumbnails/types.py @@ -0,0 +1,23 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from enum import Enum + + +class ThumbnailExecutor(Enum): + Selenium = "selenium" + User = "user" diff --git a/superset/thumbnails/utils.py b/superset/thumbnails/utils.py new file mode 100644 index 0000000000000..0d5103056f0c8 --- /dev/null +++ b/superset/thumbnails/utils.py @@ -0,0 +1,80 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +import logging +from typing import Optional, TYPE_CHECKING, Union + +from flask import current_app, g +from flask_appbuilder.security.sqla.models import User + +from superset import security_manager +from superset.thumbnails.exceptions import ThumbnailUserNotFoundError +from superset.thumbnails.types import ThumbnailExecutor + +if TYPE_CHECKING: + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + +logger = logging.getLogger(__name__) + +EXECUTOR_INDEX = 0 +DASHBOARD_INDEX = 1 +CHART_INDEX = 2 + + +def _get_digest(model: Union[Dashboard, Slice], idx) -> str: + if config := current_app.config["THUMBNAIL_EXECUTOR_CONFIG"]: + if callback := config[idx]: + return callback(model) + + return model.digest + + +def get_chart_digest(chart: Slice) -> str: + return _get_digest(chart, DASHBOARD_INDEX) + + +def get_dashboard_digest(dashboard: Dashboard) -> str: + return _get_digest(dashboard, DASHBOARD_INDEX) + + +def _get_user(username: str) -> User: + return security_manager.find_user(username=username) + + +def get_executor(username: Optional[str]) -> User: + if config := current_app.config["THUMBNAIL_EXECUTOR_CONFIG"]: + executor = config[EXECUTOR_INDEX] + if executor == ThumbnailExecutor.Selenium: + if username := current_app.config.get("THUMBNAIL_SELENIUM_USER"): + logger.warning( + "The config `THUMBNAIL_SELENIUM_USER` is deprecated and will be " + "removed in a future version. Please use `SELENIUM_USER` instead." + ) + return _get_user(username) + elif username := current_app.config["SELENIUM_USER"]: + return _get_user(username) + raise ThumbnailUserNotFoundError( + f"`{executor}` is not a valid thumbnail executor." + ) + + if executor == ThumbnailExecutor.User: + return _get_user(username) + + raise ThumbnailUserNotFoundError("Unable to find executor") From a9f37e64dafada79d8d8fe46289bcb0ee7b40986 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Sat, 3 Dec 2022 09:28:06 +0200 Subject: [PATCH 02/20] fixes --- superset/config.py | 4 ++-- superset/dashboards/api.py | 6 +++--- superset/thumbnails/utils.py | 38 +++++++++++++++++++++++++----------- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/superset/config.py b/superset/config.py index 91d34f17f2d8b..0fc47058ffcd6 100644 --- a/superset/config.py +++ b/superset/config.py @@ -608,8 +608,8 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: THUMBNAIL_EXECUTOR_CONFIG: Optional[ Tuple[ ThumbnailExecutor, - Optional[Callable[[Dashboard, models.User], str]], - Optional[Callable[[Slice, models.User], str]], + Optional[Callable[[Dashboard, Optional[models.User]], str]], + Optional[Callable[[Slice, Optional[models.User]], str]], ] ] = None diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index c2856fbb48bc8..f34add11f1ed4 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -33,7 +33,7 @@ from werkzeug.wrappers import Response as WerkzeugResponse from werkzeug.wsgi import FileWrapper -from superset import is_feature_enabled, thumbnail_cache, security_manager +from superset import is_feature_enabled, security_manager, thumbnail_cache from superset.charts.schemas import ChartEntityResponseSchema from superset.commands.importers.exceptions import NoValidFilesFoundError from superset.commands.importers.v1.utils import get_contents_from_bundle @@ -83,6 +83,7 @@ from superset.models.dashboard import Dashboard from superset.models.embedded_dashboard import EmbeddedDashboard from superset.tasks.thumbnails import cache_dashboard_thumbnail +from superset.thumbnails.utils import get_dashboard_digest from superset.utils.cache import etag_cache from superset.utils.screenshots import DashboardScreenshot from superset.utils.urls import get_url_path @@ -95,7 +96,6 @@ statsd_metrics, ) from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners -from superset.thumbnails.utils import get_dashboard_digest logger = logging.getLogger(__name__) @@ -888,8 +888,8 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: "Superset.dashboard", dashboard_id_or_slug=dashboard.id ) # If force, request a screenshot from the workers - dashboard_digest = get_dashboard_digest(dashboard) username = user.username if (user := security_manager.current_user) else None + dashboard_digest = get_dashboard_digest(dashboard) if kwargs["rison"].get("force", False): cache_dashboard_thumbnail.delay( url=dashboard_url, diff --git a/superset/thumbnails/utils.py b/superset/thumbnails/utils.py index 0d5103056f0c8..6108cc365b637 100644 --- a/superset/thumbnails/utils.py +++ b/superset/thumbnails/utils.py @@ -18,6 +18,7 @@ from __future__ import annotations import logging +from enum import Enum from typing import Optional, TYPE_CHECKING, Union from flask import current_app, g @@ -26,6 +27,7 @@ from superset import security_manager from superset.thumbnails.exceptions import ThumbnailUserNotFoundError from superset.thumbnails.types import ThumbnailExecutor +from superset.utils.hashing import md5_sha_from_str if TYPE_CHECKING: from superset.models.dashboard import Dashboard @@ -33,34 +35,48 @@ logger = logging.getLogger(__name__) -EXECUTOR_INDEX = 0 -DASHBOARD_INDEX = 1 -CHART_INDEX = 2 +class ThumbnailConfigIndex(int, Enum): + EXECUTOR = 0 + DASHBOARD_CALLBACK = 1 + CHART_CALLBACK = 2 -def _get_digest(model: Union[Dashboard, Slice], idx) -> str: + +def _get_digest( + model: Union[Dashboard, Slice], + callback_index: ThumbnailConfigIndex, +) -> str: + user = security_manager.current_user if config := current_app.config["THUMBNAIL_EXECUTOR_CONFIG"]: - if callback := config[idx]: - return callback(model) + if callback := config[callback_index]: + return callback((model, user)) + + # default to using user-specific digest when executing as user + if config[ThumbnailConfigIndex.EXECUTOR] == ThumbnailExecutor.User: + username = user.username if user else "" + return md5_sha_from_str(f"{model.digest}\n{username}") return model.digest def get_chart_digest(chart: Slice) -> str: - return _get_digest(chart, DASHBOARD_INDEX) + return _get_digest(chart, ThumbnailConfigIndex.CHART_CALLBACK) def get_dashboard_digest(dashboard: Dashboard) -> str: - return _get_digest(dashboard, DASHBOARD_INDEX) + return _get_digest(dashboard, ThumbnailConfigIndex.DASHBOARD_CALLBACK) + +def _get_user(username: Optional[str]) -> Optional[User]: + if username: + return security_manager.find_user(username=username) -def _get_user(username: str) -> User: - return security_manager.find_user(username=username) + return None def get_executor(username: Optional[str]) -> User: if config := current_app.config["THUMBNAIL_EXECUTOR_CONFIG"]: - executor = config[EXECUTOR_INDEX] + executor = config[ThumbnailConfigIndex.EXECUTOR] if executor == ThumbnailExecutor.Selenium: if username := current_app.config.get("THUMBNAIL_SELENIUM_USER"): logger.warning( From b3d40e46e8a167cd236ffe35284027df3f511883 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 10:37:12 +0200 Subject: [PATCH 03/20] more cleanup and fix test --- docs/docs/installation/alerts-reports.mdx | 2 +- docs/docs/installation/cache.mdx | 2 +- .../installation/running-on-kubernetes.mdx | 2 +- superset/charts/api.py | 33 +- superset/cli/thumbnails.py | 2 +- superset/config.py | 65 ++-- superset/dashboards/api.py | 16 +- superset/models/dashboard.py | 23 +- superset/models/slice.py | 27 +- superset/reports/commands/alert.py | 7 +- superset/reports/commands/exceptions.py | 4 - superset/reports/commands/execute.py | 32 +- superset/reports/types.py | 10 - superset/reports/utils.py | 71 ---- superset/{thumbnails => tasks}/exceptions.py | 7 +- superset/tasks/thumbnails.py | 64 +--- superset/tasks/types.py | 44 +++ superset/tasks/utils.py | 85 +++++ superset/thumbnails/digest.py | 85 +++++ superset/thumbnails/tasks.py | 101 ++++++ superset/thumbnails/types.py | 23 -- superset/thumbnails/utils.py | 96 ------ .../superset_test_config_thumbnails.py | 2 +- tests/unit_tests/reports/__init__.py | 16 - tests/unit_tests/reports/test_utils.py | 178 ---------- tests/unit_tests/tasks/test_utils.py | 323 ++++++++++++++++++ 26 files changed, 759 insertions(+), 561 deletions(-) delete mode 100644 superset/reports/utils.py rename superset/{thumbnails => tasks}/exceptions.py (85%) create mode 100644 superset/tasks/types.py create mode 100644 superset/tasks/utils.py create mode 100644 superset/thumbnails/digest.py create mode 100644 superset/thumbnails/tasks.py delete mode 100644 superset/thumbnails/types.py delete mode 100644 superset/thumbnails/utils.py delete mode 100644 tests/unit_tests/reports/__init__.py delete mode 100644 tests/unit_tests/reports/test_utils.py create mode 100644 tests/unit_tests/tasks/test_utils.py diff --git a/docs/docs/installation/alerts-reports.mdx b/docs/docs/installation/alerts-reports.mdx index 3538ca1479e42..5551641392e67 100644 --- a/docs/docs/installation/alerts-reports.mdx +++ b/docs/docs/installation/alerts-reports.mdx @@ -90,7 +90,7 @@ REDIS_PORT = "6379" class CeleryConfig: broker_url = 'redis://%s:%s/0' % (REDIS_HOST, REDIS_PORT) - imports = ('superset.sql_lab', "superset.tasks", "superset.tasks.thumbnails", ) + imports = ('superset.sql_lab', "superset.tasks", "superset.thumbnails.tasks", ) result_backend = 'redis://%s:%s/0' % (REDIS_HOST, REDIS_PORT) worker_prefetch_multiplier = 10 task_acks_late = True diff --git a/docs/docs/installation/cache.mdx b/docs/docs/installation/cache.mdx index 9972aa4887e9c..eeb2c49a663c4 100644 --- a/docs/docs/installation/cache.mdx +++ b/docs/docs/installation/cache.mdx @@ -66,7 +66,7 @@ from s3cache.s3cache import S3Cache class CeleryConfig(object): broker_url = "redis://localhost:6379/0" - imports = ("superset.sql_lab", "superset.tasks", "superset.tasks.thumbnails") + imports = ("superset.sql_lab", "superset.tasks", "superset.thumbnails.tasks") result_backend = "redis://localhost:6379/0" worker_prefetch_multiplier = 10 task_acks_late = True diff --git a/docs/docs/installation/running-on-kubernetes.mdx b/docs/docs/installation/running-on-kubernetes.mdx index 1b75e0f5c6101..e2c1f92613255 100644 --- a/docs/docs/installation/running-on-kubernetes.mdx +++ b/docs/docs/installation/running-on-kubernetes.mdx @@ -345,7 +345,7 @@ configOverrides: class CeleryConfig(object): broker_url = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0" - imports = ('superset.sql_lab', "superset.tasks", "superset.tasks.thumbnails", ) + imports = ('superset.sql_lab', "superset.tasks", "superset.thumbnails.tasks", ) result_backend = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0" task_annotations = { 'sql_lab.get_sql_results': { diff --git a/superset/charts/api.py b/superset/charts/api.py index d464efb45c61a..4c01b9ad76e7e 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -30,7 +30,7 @@ from werkzeug.wrappers import Response as WerkzeugResponse from werkzeug.wsgi import FileWrapper -from superset import app, is_feature_enabled, thumbnail_cache, security_manager +from superset import app, is_feature_enabled, security_manager, thumbnail_cache from superset.charts.commands.bulk_delete import BulkDeleteChartCommand from superset.charts.commands.create import CreateChartCommand from superset.charts.commands.delete import DeleteChartCommand @@ -74,8 +74,7 @@ from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.extensions import event_logger from superset.models.slice import Slice -from superset.tasks.thumbnails import cache_chart_thumbnail -from superset.thumbnails.utils import get_chart_digest +from superset.thumbnails.tasks import cache_chart_thumbnail from superset.utils.screenshots import ChartScreenshot from superset.utils.urls import get_url_path from superset.views.base_api import ( @@ -562,9 +561,8 @@ def cache_screenshot(self, pk: int, **kwargs: Any) -> WerkzeugResponse: if not chart: return self.response_404() - chart_digest = get_chart_digest(chart) chart_url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") - screenshot_obj = ChartScreenshot(chart_url, chart_digest) + screenshot_obj = ChartScreenshot(chart_url, chart.digest) cache_key = screenshot_obj.cache_key(window_size, thumb_size) image_url = get_url_path( "ChartRestApi.screenshot", pk=chart.id, digest=cache_key @@ -572,14 +570,16 @@ def cache_screenshot(self, pk: int, **kwargs: Any) -> WerkzeugResponse: def trigger_celery() -> WerkzeugResponse: logger.info("Triggering screenshot ASYNC") - kwargs = { - "url": chart_url, - "digest": chart_digest, - "force": True, - "window_size": window_size, - "thumb_size": thumb_size, - } - cache_chart_thumbnail.delay(**kwargs) + username = ( + user.username if (user := security_manager.current_user) else None + ) + cache_chart_thumbnail.delay( + username=username, + chart_id=chart.id, + force=True, + window_size=window_size, + thumb_size=thumb_size, + ) return self.response( 202, cache_key=cache_key, chart_url=chart_url, image_url=image_url ) @@ -688,15 +688,13 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: username = user.username if (user := security_manager.current_user) else None url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") - chart_digest = get_chart_digest(chart) if kwargs["rison"].get("force", False): logger.info( "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) ) cache_chart_thumbnail.delay( username=username, - url=url, - digest=chart_digest, + chart_id=chart.id, force=True, ) return self.response(202, message="OK Async") @@ -712,8 +710,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: ) cache_chart_thumbnail.delay( username=username, - url=url, - digest=chart_digest, + chart_id=chart.id, force=True, ) return self.response(202, message="OK Async") diff --git a/superset/cli/thumbnails.py b/superset/cli/thumbnails.py index 5556cff92f620..46d1d126f08d3 100755 --- a/superset/cli/thumbnails.py +++ b/superset/cli/thumbnails.py @@ -69,7 +69,7 @@ def compute_thumbnails( # pylint: disable=import-outside-toplevel from superset.models.dashboard import Dashboard from superset.models.slice import Slice - from superset.tasks.thumbnails import ( + from superset.thumbnails.tasks import ( cache_chart_thumbnail, cache_dashboard_thumbnail, ) diff --git a/superset/config.py b/superset/config.py index 0fc47058ffcd6..22494160dab89 100644 --- a/superset/config.py +++ b/superset/config.py @@ -41,7 +41,6 @@ Literal, Optional, Set, - Tuple, Type, TYPE_CHECKING, Union, @@ -60,10 +59,9 @@ from superset.advanced_data_type.types import AdvancedDataType from superset.constants import CHANGE_ME_SECRET_KEY from superset.jinja_context import BaseTemplateProcessor -from superset.reports.types import ReportScheduleExecutor from superset.stats_logger import DummyStatsLogger from superset.superset_typing import CacheConfig -from superset.thumbnails.types import ThumbnailExecutor +from superset.tasks.types import ExecutorType from superset.utils.core import is_test, NO_TIME_RANGE, parse_boolean_string from superset.utils.encrypt import SQLAlchemyUtilsAdapter from superset.utils.log import DBEventLogger @@ -581,38 +579,31 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # When executing Alerts & Reports or Thumbnails as the Selenium user, this defines # the username of the account used to render the queries and dashboards/charts -SELENIUM_USER: Optional[str] = "admin" +THUMBNAIL_SELENIUM_USER: Optional[str] = "admin" # --------------------------------------------------- # Thumbnail config (behind feature flag) # --------------------------------------------------- -# To be able to have different thumbnails for different users, use this config to define -# 1. which user to execute the thumbnails as 2. custom callbacks for creating the -# digests for the dashboard and chart thumbnail images. To have unique thumbnails for -# all users, use the following example config: -from hashlib import md5 - -# THUMBNAIL_EXECUTOR_CONFIG: Optional[ -# Tuple[ -# ThumbnailExecutor, -# Optional[Callable[[Dashboard, models.User], str]], -# Optional[Callable[[Slice, models.User], str]], -# ] -# ] = ( -# ThumbnailExecutor.User, -# lambda x: md5(f"{x[0].digest()}\n{x[1].id}".encode("utf-8")).hexdigest(), -# lambda x: md5(f"{x[0].digest()}\n{x[1].id}".encode("utf-8")).hexdigest(), -# ) - -THUMBNAIL_EXECUTOR_CONFIG: Optional[ - Tuple[ - ThumbnailExecutor, - Optional[Callable[[Dashboard, Optional[models.User]], str]], - Optional[Callable[[Slice, Optional[models.User]], str]], - ] +# To be able to have different thumbnails for different users, use these configs to +# define which user to execute the thumbnails and potentially custom functions for +# calculating thumbnail digests. To have unique thumbnails for all users, use the +# following config: +# THUMBNAIL_EXECUTE_AS = [ExecutorType.INITIATOR] +THUMBNAIL_EXECUTE_AS = [ExecutorType.SELENIUM] + +# By default, thumbnail digests are calculated based on various parameters in the +# chart/dashboard metadata, and in the case of user-specific thumbnails, the +# SECRET_KEY and the username. To specify a custom digest function, use the following +# config parameters to define callbacks that receive +# 1. the model (dashboard or chart) +# 2. the executor type +# 3. the executor's username +# and return the final digest string: +THUMBNAIL_DASHBOARD_DIGEST_FUNC: Optional[ + Callable[[Dashboard, ExecutorType, str], str] ] = None - +THUMBNAIL_CHART_DIGEST_FUNC: Optional[Callable[[Slice, ExecutorType, str], str]] = None THUMBNAIL_CACHE_CONFIG: CacheConfig = { "CACHE_TYPE": "NullCache", @@ -1210,16 +1201,14 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument # creator if either is contained within the list of owners, otherwise the first owner # will be used) and finally `THUMBNAIL_SELENIUM_USER`, set as follows: # ALERT_REPORTS_EXECUTE_AS = [ -# ReportScheduleExecutor.CREATOR_OWNER, -# ReportScheduleExecutor.CREATOR, -# ReportScheduleExecutor.MODIFIER_OWNER, -# ReportScheduleExecutor.MODIFIER, -# ReportScheduleExecutor.OWNER, -# ReportScheduleExecutor.SELENIUM, +# ScheduledTaskExecutor.CREATOR_OWNER, +# ScheduledTaskExecutor.CREATOR, +# ScheduledTaskExecutor.MODIFIER_OWNER, +# ScheduledTaskExecutor.MODIFIER, +# ScheduledTaskExecutor.OWNER, +# ScheduledTaskExecutor.SELENIUM, # ] -ALERT_REPORTS_EXECUTE_AS: List[ReportScheduleExecutor] = [ - ReportScheduleExecutor.SELENIUM -] +ALERT_REPORTS_EXECUTE_AS: List[ExecutorType] = [ExecutorType.SELENIUM] # if ALERT_REPORTS_WORKING_TIME_OUT_KILL is True, set a celery hard timeout # Equal to working timeout + ALERT_REPORTS_WORKING_TIME_OUT_LAG ALERT_REPORTS_WORKING_TIME_OUT_LAG = int(timedelta(seconds=10).total_seconds()) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index f34add11f1ed4..60ca2e1a8f3e0 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -82,8 +82,7 @@ from superset.extensions import event_logger from superset.models.dashboard import Dashboard from superset.models.embedded_dashboard import EmbeddedDashboard -from superset.tasks.thumbnails import cache_dashboard_thumbnail -from superset.thumbnails.utils import get_dashboard_digest +from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.utils.cache import etag_cache from superset.utils.screenshots import DashboardScreenshot from superset.utils.urls import get_url_path @@ -889,37 +888,34 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: ) # If force, request a screenshot from the workers username = user.username if (user := security_manager.current_user) else None - dashboard_digest = get_dashboard_digest(dashboard) if kwargs["rison"].get("force", False): cache_dashboard_thumbnail.delay( - url=dashboard_url, - digest=dashboard_digest, username=username, + dashboard_id=dashboard.id, force=True, ) return self.response(202, message="OK Async") # fetch the dashboard screenshot using the current user and cache if set screenshot = DashboardScreenshot( - dashboard_url, dashboard_digest + dashboard_url, dashboard.digest ).get_from_cache(cache=thumbnail_cache) # If the screenshot does not exist, request one from the workers if not screenshot: self.incr_stats("async", self.thumbnail.__name__) cache_dashboard_thumbnail.delay( username=username, - url=dashboard_url, - digest=dashboard_digest, + dashboard_id=dashboard.id, force=True, ) return self.response(202, message="OK Async") # If digests - if dashboard_digest != digest: + if dashboard.digest != digest: self.incr_stats("redirect", self.thumbnail.__name__) return redirect( url_for( f"{self.__class__.__name__}.thumbnail", pk=pk, - digest=dashboard_digest, + digest=dashboard.digest, ) ) self.incr_stats("from_cache", self.thumbnail.__name__) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 4c6955cc5ba19..43c62b39408d6 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -54,13 +54,11 @@ from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.models.slice import Slice from superset.models.user_attributes import UserAttribute -from superset.tasks.thumbnails import cache_dashboard_thumbnail -from superset.thumbnails.utils import get_dashboard_digest +from superset.thumbnails.digest import get_dashboard_digest +from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.utils import core as utils from superset.utils.core import get_user_id from superset.utils.decorators import debounce -from superset.utils.hashing import md5_sha_from_str -from superset.utils.urls import get_url_path metadata = Model.metadata # pylint: disable=no-member config = app.config @@ -242,11 +240,7 @@ def dashboard_link(self) -> Markup: @property def digest(self) -> str: - """ - Returns a MD5 HEX digest that makes this dashboard unique - """ - unique_string = f"{self.position_json}.{self.css}.{self.json_metadata}" - return md5_sha_from_str(unique_string) + return get_dashboard_digest(self) @property def thumbnail_url(self) -> str: @@ -254,7 +248,7 @@ def thumbnail_url(self) -> str: Returns a thumbnail URL with a HEX digest. We want to avoid browser cache if the dashboard has changed """ - return f"/api/v1/dashboard/{self.id}/thumbnail/{get_dashboard_digest(self)}/" + return f"/api/v1/dashboard/{self.id}/thumbnail/{self.digest}/" @property def changed_by_name(self) -> str: @@ -330,8 +324,13 @@ def position(self) -> Dict[str, Any]: return {} def update_thumbnail(self) -> None: - url = get_url_path("Superset.dashboard", dashboard_id_or_slug=self.id) - cache_dashboard_thumbnail.delay(url, self.digest, force=True) + username = user.username if (user := security_manager.current_user) else None + cache_dashboard_thumbnail.delay( + username=username, + dashboard_id=self.id, + digest=self.digest, + force=True, + ) @debounce(0.1) def clear_cache(self) -> None: diff --git a/superset/models/slice.py b/superset/models/slice.py index de5903197f1ec..1325fafd31b36 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -42,12 +42,10 @@ from superset import db, is_feature_enabled, security_manager from superset.legacy import update_time_range from superset.models.helpers import AuditMixinNullable, ImportExportMixin -from superset.tasks.thumbnails import cache_chart_thumbnail -from superset.thumbnails.utils import get_chart_digest +from superset.thumbnails.digest import get_chart_digest +from superset.thumbnails.tasks import cache_chart_thumbnail from superset.utils import core as utils -from superset.utils.hashing import md5_sha_from_str from superset.utils.memoized import memoized -from superset.utils.urls import get_url_path from superset.viz import BaseViz, viz_types if TYPE_CHECKING: @@ -235,10 +233,7 @@ def data(self) -> Dict[str, Any]: @property def digest(self) -> str: - """ - Returns a MD5 HEX digest that makes this dashboard unique - """ - return md5_sha_from_str(self.params or "") + return get_chart_digest(self) @property def thumbnail_url(self) -> str: @@ -246,7 +241,7 @@ def thumbnail_url(self) -> str: Returns a thumbnail URL with a HEX digest. We want to avoid browser cache if the dashboard has changed """ - return f"/api/v1/chart/{self.id}/thumbnail/{get_chart_digest(self)}/" + return f"/api/v1/chart/{self.id}/thumbnail/{self.digest}/" @property def json_data(self) -> str: @@ -345,6 +340,12 @@ def get_query_context_factory(self) -> QueryContextFactory: self.query_context_factory = QueryContextFactory() return self.query_context_factory + @classmethod + def get(cls, id_: int) -> Slice: + session = db.session() + qry = session.query(Slice).filter_by(id=id_) + return qry.one_or_none() + def set_related_perm(_mapper: Mapper, _connection: Connection, target: Slice) -> None: src_class = target.cls_model @@ -359,8 +360,12 @@ def set_related_perm(_mapper: Mapper, _connection: Connection, target: Slice) -> def event_after_chart_changed( _mapper: Mapper, _connection: Connection, target: Slice ) -> None: - url = get_url_path("Superset.slice", slice_id=target.id, standalone="true") - cache_chart_thumbnail.delay(url, target.digest, force=True) + username = user.username if (user := security_manager.current_user) else None + cache_chart_thumbnail.delay( + username=username, + chart_id=target.id, + force=True, + ) sqla.event.listen(Slice, "before_insert", set_related_perm) diff --git a/superset/reports/commands/alert.py b/superset/reports/commands/alert.py index 255280704e78f..5ed6029f3cf6b 100644 --- a/superset/reports/commands/alert.py +++ b/superset/reports/commands/alert.py @@ -36,7 +36,7 @@ AlertValidatorConfigError, ) from superset.reports.models import ReportSchedule, ReportScheduleValidatorType -from superset.reports.utils import get_executor +from superset.tasks.utils import get_executor from superset.utils.core import override_user from superset.utils.retries import retry_call @@ -149,7 +149,10 @@ def _execute_query(self) -> pd.DataFrame: rendered_sql, ALERT_SQL_LIMIT ) - user = get_executor(self._report_schedule) + user = get_executor( + executor_types=app.config["ALERT_REPORTS_EXECUTE_AS"], + model=self._report_schedule, + ) with override_user(user): start = default_timer() df = self._report_schedule.database.get_df(sql=limited_rendered_sql) diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index a068b3c62860d..48dbfcaab9915 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -253,10 +253,6 @@ class ReportScheduleNotificationError(CommandException): message = _("Alert on grace period") -class ReportScheduleUserNotFoundError(CommandException): - message = _("Report Schedule user not found") - - class ReportScheduleStateNotFoundError(CommandException): message = _("Report Schedule state not found") diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py index d20775ffd6d3b..8133cec29b971 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -24,7 +24,7 @@ from celery.exceptions import SoftTimeLimitExceeded from sqlalchemy.orm import Session -from superset import app +from superset import app, security_manager from superset.commands.base import BaseCommand from superset.commands.exceptions import CommandException from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType @@ -69,7 +69,7 @@ from superset.reports.notifications import create_notification from superset.reports.notifications.base import NotificationContent from superset.reports.notifications.exceptions import NotificationError -from superset.reports.utils import get_executor +from superset.tasks.utils import get_executor from superset.utils.celery import session_scope from superset.utils.core import HeaderDataType, override_user from superset.utils.csv import get_chart_csv_data, get_chart_dataframe @@ -201,7 +201,11 @@ def _get_screenshots(self) -> List[bytes]: :raises: ReportScheduleScreenshotFailedError """ url = self._get_url() - user = get_executor(self._report_schedule) + _, username = get_executor( + executor_types=app.config["ALERT_REPORTS_EXECUTE_AS"], + model=self._report_schedule, + ) + user = security_manager.find_user(username) if self._report_schedule.chart: screenshot: Union[ChartScreenshot, DashboardScreenshot] = ChartScreenshot( url, @@ -231,7 +235,11 @@ def _get_screenshots(self) -> List[bytes]: def _get_csv_data(self) -> bytes: url = self._get_url(result_format=ChartDataResultFormat.CSV) - user = get_executor(self._report_schedule) + _, username = get_executor( + executor_types=app.config["ALERT_REPORTS_EXECUTE_AS"], + model=self._report_schedule, + ) + user = security_manager.find_user(username) auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user) if self._report_schedule.chart.query_context is None: @@ -240,7 +248,7 @@ def _get_csv_data(self) -> bytes: try: logger.info("Getting chart from %s as user %s", url, user.username) - csv_data = get_chart_csv_data(url, auth_cookies) + csv_data = get_chart_csv_data(chart_url=url, auth_cookies=auth_cookies) except SoftTimeLimitExceeded as ex: raise ReportScheduleCsvTimeout() from ex except Exception as ex: @@ -256,7 +264,11 @@ def _get_embedded_data(self) -> pd.DataFrame: Return data as a Pandas dataframe, to embed in notifications as a table. """ url = self._get_url(result_format=ChartDataResultFormat.JSON) - user = get_executor(self._report_schedule) + _, username = get_executor( + executor_types=app.config["ALERT_REPORTS_EXECUTE_AS"], + model=self._report_schedule, + ) + user = security_manager.find_user(username) auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user) if self._report_schedule.chart.query_context is None: @@ -692,12 +704,16 @@ def run(self) -> None: self.validate(session=session) if not self._model: raise ReportScheduleExecuteUnexpectedError() - user = get_executor(self._model) + _, username = get_executor( + executor_types=app.config["ALERT_REPORTS_EXECUTE_AS"], + model=self._model, + ) + user = security_manager.find_user(username) with override_user(user): logger.info( "Running report schedule %s as user %s", self._execution_id, - user.username, + username, ) ReportScheduleStateMachine( session, self._execution_id, self._model, self._scheduled_dttm diff --git a/superset/reports/types.py b/superset/reports/types.py index 7977a2defa9ad..d487e3ad23766 100644 --- a/superset/reports/types.py +++ b/superset/reports/types.py @@ -14,7 +14,6 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from enum import Enum from typing import TypedDict from superset.dashboards.permalink.types import DashboardPermalinkState @@ -22,12 +21,3 @@ class ReportScheduleExtra(TypedDict): dashboard: DashboardPermalinkState - - -class ReportScheduleExecutor(str, Enum): - SELENIUM = "selenium" - CREATOR = "creator" - CREATOR_OWNER = "creator_owner" - MODIFIER = "modifier" - MODIFIER_OWNER = "modifier_owner" - OWNER = "owner" diff --git a/superset/reports/utils.py b/superset/reports/utils.py deleted file mode 100644 index 215fca99887a5..0000000000000 --- a/superset/reports/utils.py +++ /dev/null @@ -1,71 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -from flask_appbuilder.security.sqla.models import User - -from superset import app, security_manager -from superset.reports.commands.exceptions import ReportScheduleUserNotFoundError -from superset.reports.models import ReportSchedule -from superset.reports.types import ReportScheduleExecutor - - -# pylint: disable=too-many-branches -def get_executor(report_schedule: ReportSchedule) -> User: - """ - Extract the user that should be used to execute a report schedule as. - - :param report_schedule: The report to execute - :return: User to execute the report as - """ - user_types = app.config["ALERT_REPORTS_EXECUTE_AS"] - owners = report_schedule.owners - owner_dict = {owner.id: owner for owner in owners} - for user_type in user_types: - if user_type == ReportScheduleExecutor.SELENIUM: - username = app.config["THUMBNAIL_SELENIUM_USER"] - if username and (user := security_manager.find_user(username=username)): - return user - if user_type == ReportScheduleExecutor.CREATOR_OWNER: - if (user := report_schedule.created_by) and ( - owner := owner_dict.get(user.id) - ): - return owner - if user_type == ReportScheduleExecutor.CREATOR: - if user := report_schedule.created_by: - return user - if user_type == ReportScheduleExecutor.MODIFIER_OWNER: - if (user := report_schedule.changed_by) and ( - owner := owner_dict.get(user.id) - ): - return owner - if user_type == ReportScheduleExecutor.MODIFIER: - if user := report_schedule.changed_by: - return user - if user_type == ReportScheduleExecutor.OWNER: - owners = report_schedule.owners - if len(owners) == 1: - return owners[0] - if len(owners) > 1: - if modifier := report_schedule.changed_by: - if modifier and (user := owner_dict.get(modifier.id)): - return user - if creator := report_schedule.created_by: - if creator and (user := owner_dict.get(creator.id)): - return user - return owners[0] - - raise ReportScheduleUserNotFoundError() diff --git a/superset/thumbnails/exceptions.py b/superset/tasks/exceptions.py similarity index 85% rename from superset/thumbnails/exceptions.py rename to superset/tasks/exceptions.py index d19973ce87588..6698661754e5e 100644 --- a/superset/thumbnails/exceptions.py +++ b/superset/tasks/exceptions.py @@ -14,8 +14,11 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. + +from flask_babel import lazy_gettext as _ + from superset.exceptions import SupersetException -class ThumbnailUserNotFoundError(SupersetException): - pass +class ExecutorNotFoundError(SupersetException): + message = _("Scheduled task executor not found") diff --git a/superset/tasks/thumbnails.py b/superset/tasks/thumbnails.py index 0a7899478893d..247d865001be6 100644 --- a/superset/tasks/thumbnails.py +++ b/superset/tasks/thumbnails.py @@ -15,65 +15,15 @@ # specific language governing permissions and limitations # under the License. -"""Utility functions used across Superset""" - import logging -from typing import Optional -from superset import thumbnail_cache -from superset.extensions import celery_app -from superset.thumbnails.utils import get_executor -from superset.utils.core import override_user -from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot -from superset.utils.webdriver import WindowSize +# TODO(villebro): deprecate in 3.0 +# pylint: disable=unused-wildcard-import,wildcard-import +from superset.thumbnails.tasks import * logger = logging.getLogger(__name__) - -@celery_app.task(name="cache_chart_thumbnail", soft_time_limit=300) -def cache_chart_thumbnail( - username: Optional[str], - url: str, - digest: str, - force: bool = False, - window_size: Optional[WindowSize] = None, - thumb_size: Optional[WindowSize] = None, -) -> None: - if not thumbnail_cache: - logger.warning("No cache set, refusing to compute") - return None - logger.info("Caching chart: %s", url) - screenshot = ChartScreenshot(url, digest) - user = get_executor(username) - with override_user(user): - screenshot.compute_and_cache( - user=user, - cache=thumbnail_cache, - force=force, - window_size=window_size, - thumb_size=thumb_size, - ) - return None - - -@celery_app.task(name="cache_dashboard_thumbnail", soft_time_limit=300) -def cache_dashboard_thumbnail( - username: Optional[str], - url: str, - digest: str, - force: bool = False, - thumb_size: Optional[WindowSize] = None, -) -> None: - if not thumbnail_cache: - logging.warning("No cache set, refusing to compute") - return - logger.info("Caching dashboard: %s", url) - screenshot = DashboardScreenshot(url, digest) - user = get_executor(username=username) - with override_user(user): - screenshot.compute_and_cache( - user=user, - cache=thumbnail_cache, - force=force, - thumb_size=thumb_size, - ) +logger.warning( + "The import path superset.tasks.thumbnails is deprecated and will be removed in" + "3.0. Please use superset.thumbnails.tasks instead." +) diff --git a/superset/tasks/types.py b/superset/tasks/types.py new file mode 100644 index 0000000000000..4babc53daed04 --- /dev/null +++ b/superset/tasks/types.py @@ -0,0 +1,44 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from enum import Enum + + +class ExecutorType(str, Enum): + """ + Which user should scheduled tasks be executed as. Used as follows: + For Alerts & Reports: the "model" refers to the AlertSchedule object + For Thumbnails: The "model" refers to the Slice or Dashboard object + """ + + # See the THUMBNAIL_SELENIUM_USER config parameter + SELENIUM = "selenium" + # The creator of the model + CREATOR = "creator" + # The creator of the model, if found in the owners list + CREATOR_OWNER = "creator_owner" + # The initiator of the request. In the case of Alerts & Reports, this is always + # None. For Thumbnails, this is the user that requested the thumbnail + INITIATOR = "initiator" + # The last modifier of the model + MODIFIER = "modifier" + # The last modifier of the model, if found in the owners list + MODIFIER_OWNER = "modifier_owner" + # An owner of the model. If the last modifier is in the owners list, returns that + # user. If the modifier is not found, returns the creator if found in the owners + # list. Finally, if neither are present, returns the first user in the owners list. + OWNER = "owner" diff --git a/superset/tasks/utils.py b/superset/tasks/utils.py new file mode 100644 index 0000000000000..d62bece35f090 --- /dev/null +++ b/superset/tasks/utils.py @@ -0,0 +1,85 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from typing import List, Optional, Tuple, TYPE_CHECKING, Union + +from superset import app +from superset.tasks.exceptions import ExecutorNotFoundError +from superset.tasks.types import ExecutorType + +if TYPE_CHECKING: + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + from superset.reports.models import ReportSchedule + + +# pylint: disable=too-many-branches +def get_executor( + executor_types: List[ExecutorType], + model: Union[Dashboard, ReportSchedule, Slice], + initiator: Optional[str] = None, +) -> Tuple[ExecutorType, str]: + """ + Extract the user that should be used to execute a scheduled task. Certain executor + types extract the user from the underlying object (e.g. CREATOR), the constant + Selenium user (SELENIUM), or the user that initiated the request. + + :param executor_types: The requested executor type in descending order. When the + first user is found it is returned. + :param model: The underlying object + :param initiator: The username of the user that initiated the task. For thumbnails + this is the user that requested the thumbnail, while for alerts and + reports this is None (=initiated by Celery). + :return: User to execute the report as + :raises ScheduledTaskExecutorNotFoundError: If no users were found in after + iterating through all entries in `executor_types` + """ + owners = model.owners + owner_dict = {owner.id: owner for owner in owners} + for executor_type in executor_types: + if executor_type == ExecutorType.INITIATOR and initiator: + return executor_type, initiator + if executor_type == ExecutorType.SELENIUM: + return executor_type, app.config["THUMBNAIL_SELENIUM_USER"] + if executor_type == ExecutorType.CREATOR_OWNER: + if (user := model.created_by) and (owner := owner_dict.get(user.id)): + return executor_type, owner.username + if executor_type == ExecutorType.CREATOR: + if user := model.created_by: + return executor_type, user.username + if executor_type == ExecutorType.MODIFIER_OWNER: + if (user := model.changed_by) and (owner := owner_dict.get(user.id)): + return executor_type, owner.username + if executor_type == ExecutorType.MODIFIER: + if user := model.changed_by: + return executor_type, user.username + if executor_type == ExecutorType.OWNER: + owners = model.owners + if len(owners) == 1: + return executor_type, owners[0].username + if len(owners) > 1: + if modifier := model.changed_by: + if modifier and (user := owner_dict.get(modifier.id)): + return executor_type, user.username + if creator := model.created_by: + if creator and (user := owner_dict.get(creator.id)): + return executor_type, user.username + return executor_type, owners[0].username + + raise ExecutorNotFoundError() diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py new file mode 100644 index 0000000000000..43c9c9423dfd7 --- /dev/null +++ b/superset/thumbnails/digest.py @@ -0,0 +1,85 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +from flask import current_app, g + +from superset.tasks.types import ExecutorType +from superset.tasks.utils import get_executor +from superset.utils.hashing import md5_sha_from_str + +if TYPE_CHECKING: + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + +logger = logging.getLogger(__name__) + + +def _adjust_string_for_executor( + unique_string: str, + executor_type: ExecutorType, + executor: str, +) -> str: + """ + Add the secret key and executor to the unique string if the thumbnail is + user-specific. + """ + if executor_type == ExecutorType.INITIATOR: + secret_key = current_app.config["SECRET_KEY"] + # the digest for the user=specific thumbnail needs to be unguessable and unique, + # hence the secret key and username are appended to the unique string + unique_string = f"{secret_key}\n{unique_string}\n{executor}" + + return unique_string + + +def get_dashboard_digest(dashboard: Dashboard) -> str: + config = current_app.config + executor_type, executor = get_executor( + executor_types=config["THUMBNAIL_EXECUTE_AS"], + model=dashboard, + initiator=g.user.username, + ) + if func := config["THUMBNAIL_DASHBOARD_DIGEST_FUNC"]: + return func(dashboard, executor_type, executor) + + unique_string = ( + f"{dashboard.position_json}.{dashboard.css}" + f".{dashboard.json_metadata}.{executor}" + ) + + unique_string = _adjust_string_for_executor(unique_string, executor_type, executor) + return md5_sha_from_str(unique_string) + + +def get_chart_digest(chart: Slice) -> str: + config = current_app.config + executor_type, executor = get_executor( + executor_types=config["THUMBNAIL_EXECUTE_AS"], + model=chart, + initiator=g.user.username, + ) + if func := config["THUMBNAIL_CHART_DIGEST_FUNC"]: + return func(chart, executor_type, executor) + + unique_string = f"{chart.params or ''}.{executor}" + unique_string = _adjust_string_for_executor(unique_string, executor_type, executor) + return md5_sha_from_str(unique_string) diff --git a/superset/thumbnails/tasks.py b/superset/thumbnails/tasks.py new file mode 100644 index 0000000000000..1c01673344f5c --- /dev/null +++ b/superset/thumbnails/tasks.py @@ -0,0 +1,101 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Utility functions used across Superset""" + +import logging +from typing import cast, Optional + +from flask import current_app + +from superset import security_manager, thumbnail_cache +from superset.extensions import celery_app +from superset.tasks.utils import get_executor +from superset.utils.core import override_user +from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot +from superset.utils.urls import get_url_path +from superset.utils.webdriver import WindowSize + +logger = logging.getLogger(__name__) + + +@celery_app.task(name="cache_chart_thumbnail", soft_time_limit=300) +def cache_chart_thumbnail( + username: Optional[str], + chart_id: int, + force: bool = False, + window_size: Optional[WindowSize] = None, + thumb_size: Optional[WindowSize] = None, +) -> None: + from superset.models.slice import Slice + + if not thumbnail_cache: + logger.warning("No cache set, refusing to compute") + return None + chart = cast(Slice, Slice.get(chart_id)) + url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") + logger.info("Caching chart: %s", url) + _, username = get_executor( + executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], + model=chart, + initiator=username, + ) + user = security_manager.find_user(username) + with override_user(user): + screenshot = ChartScreenshot(url, chart.digest) + screenshot.compute_and_cache( + user=user, + cache=thumbnail_cache, + force=force, + window_size=window_size, + thumb_size=thumb_size, + ) + return None + + +@celery_app.task(name="cache_dashboard_thumbnail", soft_time_limit=300) +def cache_dashboard_thumbnail( + username: Optional[str], + dashboard_id: int, + force: bool = False, + thumb_size: Optional[WindowSize] = None, +) -> None: + from superset.models.dashboard import Dashboard + + if not thumbnail_cache: + logging.warning("No cache set, refusing to compute") + return + dashboard = Dashboard.get(dashboard_id) + url = get_url_path( + "Superset.dashboard", dashboard_id_or_slug=dashboard.id + ) + + logger.info("Caching dashboard: %s", url) + _, username = get_executor( + executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], + model=dashboard, + initiator=username, + ) + user = security_manager.find_user(username) + with override_user(user): + screenshot = DashboardScreenshot(url, dashboard.digest) + screenshot.compute_and_cache( + user=user, + cache=thumbnail_cache, + force=force, + thumb_size=thumb_size, + ) diff --git a/superset/thumbnails/types.py b/superset/thumbnails/types.py deleted file mode 100644 index 8a586666968a9..0000000000000 --- a/superset/thumbnails/types.py +++ /dev/null @@ -1,23 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -from enum import Enum - - -class ThumbnailExecutor(Enum): - Selenium = "selenium" - User = "user" diff --git a/superset/thumbnails/utils.py b/superset/thumbnails/utils.py deleted file mode 100644 index 6108cc365b637..0000000000000 --- a/superset/thumbnails/utils.py +++ /dev/null @@ -1,96 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -from __future__ import annotations - -import logging -from enum import Enum -from typing import Optional, TYPE_CHECKING, Union - -from flask import current_app, g -from flask_appbuilder.security.sqla.models import User - -from superset import security_manager -from superset.thumbnails.exceptions import ThumbnailUserNotFoundError -from superset.thumbnails.types import ThumbnailExecutor -from superset.utils.hashing import md5_sha_from_str - -if TYPE_CHECKING: - from superset.models.dashboard import Dashboard - from superset.models.slice import Slice - -logger = logging.getLogger(__name__) - - -class ThumbnailConfigIndex(int, Enum): - EXECUTOR = 0 - DASHBOARD_CALLBACK = 1 - CHART_CALLBACK = 2 - - -def _get_digest( - model: Union[Dashboard, Slice], - callback_index: ThumbnailConfigIndex, -) -> str: - user = security_manager.current_user - if config := current_app.config["THUMBNAIL_EXECUTOR_CONFIG"]: - if callback := config[callback_index]: - return callback((model, user)) - - # default to using user-specific digest when executing as user - if config[ThumbnailConfigIndex.EXECUTOR] == ThumbnailExecutor.User: - username = user.username if user else "" - return md5_sha_from_str(f"{model.digest}\n{username}") - - return model.digest - - -def get_chart_digest(chart: Slice) -> str: - return _get_digest(chart, ThumbnailConfigIndex.CHART_CALLBACK) - - -def get_dashboard_digest(dashboard: Dashboard) -> str: - return _get_digest(dashboard, ThumbnailConfigIndex.DASHBOARD_CALLBACK) - - -def _get_user(username: Optional[str]) -> Optional[User]: - if username: - return security_manager.find_user(username=username) - - return None - - -def get_executor(username: Optional[str]) -> User: - if config := current_app.config["THUMBNAIL_EXECUTOR_CONFIG"]: - executor = config[ThumbnailConfigIndex.EXECUTOR] - if executor == ThumbnailExecutor.Selenium: - if username := current_app.config.get("THUMBNAIL_SELENIUM_USER"): - logger.warning( - "The config `THUMBNAIL_SELENIUM_USER` is deprecated and will be " - "removed in a future version. Please use `SELENIUM_USER` instead." - ) - return _get_user(username) - elif username := current_app.config["SELENIUM_USER"]: - return _get_user(username) - raise ThumbnailUserNotFoundError( - f"`{executor}` is not a valid thumbnail executor." - ) - - if executor == ThumbnailExecutor.User: - return _get_user(username) - - raise ThumbnailUserNotFoundError("Unable to find executor") diff --git a/tests/integration_tests/superset_test_config_thumbnails.py b/tests/integration_tests/superset_test_config_thumbnails.py index 9f621efabbf4d..58fd367c42305 100644 --- a/tests/integration_tests/superset_test_config_thumbnails.py +++ b/tests/integration_tests/superset_test_config_thumbnails.py @@ -63,7 +63,7 @@ def GET_FEATURE_FLAGS_FUNC(ff): class CeleryConfig(object): BROKER_URL = f"redis://{REDIS_HOST}:{REDIS_PORT}/{REDIS_CELERY_DB}" - CELERY_IMPORTS = ("superset.sql_lab", "superset.tasks.thumbnails") + CELERY_IMPORTS = ("superset.sql_lab", "superset.thumbnails.tasks") CELERY_ANNOTATIONS = {"sql_lab.add": {"rate_limit": "10/s"}} CONCURRENCY = 1 diff --git a/tests/unit_tests/reports/__init__.py b/tests/unit_tests/reports/__init__.py deleted file mode 100644 index 13a83393a9124..0000000000000 --- a/tests/unit_tests/reports/__init__.py +++ /dev/null @@ -1,16 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. diff --git a/tests/unit_tests/reports/test_utils.py b/tests/unit_tests/reports/test_utils.py deleted file mode 100644 index 8b4bf93e718a8..0000000000000 --- a/tests/unit_tests/reports/test_utils.py +++ /dev/null @@ -1,178 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -from dataclasses import dataclass -from typing import List, Optional, Union -from unittest.mock import patch - -import pytest -from flask_appbuilder.security.sqla.models import User - -from superset.reports.types import ReportScheduleExecutor - -SELENIUM_USER_ID = 1234 - - -def _get_users( - params: Optional[Union[int, List[int]]] -) -> Optional[Union[User, List[User]]]: - if params is None: - return None - if isinstance(params, int): - return User(id=params) - return [User(id=user) for user in params] - - -@dataclass -class ReportConfig: - owners: List[int] - creator: Optional[int] = None - modifier: Optional[int] = None - - -@pytest.mark.parametrize( - "config,report_config,expected_user", - [ - ( - [ReportScheduleExecutor.SELENIUM], - ReportConfig( - owners=[1, 2], - creator=3, - modifier=4, - ), - SELENIUM_USER_ID, - ), - ( - [ - ReportScheduleExecutor.CREATOR, - ReportScheduleExecutor.CREATOR_OWNER, - ReportScheduleExecutor.OWNER, - ReportScheduleExecutor.MODIFIER, - ReportScheduleExecutor.MODIFIER_OWNER, - ReportScheduleExecutor.SELENIUM, - ], - ReportConfig(owners=[]), - SELENIUM_USER_ID, - ), - ( - [ - ReportScheduleExecutor.CREATOR, - ReportScheduleExecutor.CREATOR_OWNER, - ReportScheduleExecutor.OWNER, - ReportScheduleExecutor.MODIFIER, - ReportScheduleExecutor.MODIFIER_OWNER, - ReportScheduleExecutor.SELENIUM, - ], - ReportConfig(owners=[], modifier=1), - 1, - ), - ( - [ - ReportScheduleExecutor.CREATOR, - ReportScheduleExecutor.CREATOR_OWNER, - ReportScheduleExecutor.OWNER, - ReportScheduleExecutor.MODIFIER, - ReportScheduleExecutor.MODIFIER_OWNER, - ReportScheduleExecutor.SELENIUM, - ], - ReportConfig(owners=[2], modifier=1), - 2, - ), - ( - [ - ReportScheduleExecutor.CREATOR, - ReportScheduleExecutor.CREATOR_OWNER, - ReportScheduleExecutor.OWNER, - ReportScheduleExecutor.MODIFIER, - ReportScheduleExecutor.MODIFIER_OWNER, - ReportScheduleExecutor.SELENIUM, - ], - ReportConfig(owners=[2], creator=3, modifier=1), - 3, - ), - ( - [ - ReportScheduleExecutor.OWNER, - ], - ReportConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=3, modifier=4), - 4, - ), - ( - [ - ReportScheduleExecutor.OWNER, - ], - ReportConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=3, modifier=8), - 3, - ), - ( - [ - ReportScheduleExecutor.MODIFIER_OWNER, - ], - ReportConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=8, modifier=9), - None, - ), - ( - [ - ReportScheduleExecutor.MODIFIER_OWNER, - ], - ReportConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=8, modifier=4), - 4, - ), - ( - [ - ReportScheduleExecutor.CREATOR_OWNER, - ], - ReportConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=8, modifier=9), - None, - ), - ( - [ - ReportScheduleExecutor.CREATOR_OWNER, - ], - ReportConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=4, modifier=8), - 4, - ), - ], -) -def test_get_executor( - config: List[ReportScheduleExecutor], - report_config: ReportConfig, - expected_user: Optional[int], -) -> None: - from superset import app, security_manager - from superset.reports.commands.exceptions import ReportScheduleUserNotFoundError - from superset.reports.models import ReportSchedule - from superset.reports.utils import get_executor - - selenium_user = User(id=SELENIUM_USER_ID) - - with patch.dict(app.config, {"ALERT_REPORTS_EXECUTE_AS": config}), patch.object( - security_manager, "find_user", return_value=selenium_user - ): - report_schedule = ReportSchedule( - id=1, - type="report", - name="test_report", - owners=_get_users(report_config.owners), - created_by=_get_users(report_config.creator), - changed_by=_get_users(report_config.modifier), - ) - if expected_user is None: - with pytest.raises(ReportScheduleUserNotFoundError): - get_executor(report_schedule) - else: - assert get_executor(report_schedule).id == expected_user diff --git a/tests/unit_tests/tasks/test_utils.py b/tests/unit_tests/tasks/test_utils.py new file mode 100644 index 0000000000000..1b7352426d433 --- /dev/null +++ b/tests/unit_tests/tasks/test_utils.py @@ -0,0 +1,323 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from contextlib import nullcontext +from dataclasses import dataclass +from enum import Enum +from typing import Any, Dict, List, Optional, Tuple, Type, Union + +import pytest +from flask_appbuilder.security.sqla.models import User + +from superset.tasks.exceptions import ExecutorNotFoundError +from superset.tasks.types import ExecutorType + +SELENIUM_USER_ID = 1234 +SELENIUM_USERNAME = "admin" + + +def _get_users( + params: Optional[Union[int, List[int]]] +) -> Optional[Union[User, List[User]]]: + if params is None: + return None + if isinstance(params, int): + return User(id=params, username=str(params)) + return [User(id=user, username=str(user)) for user in params] + + +@dataclass +class ModelConfig: + owners: List[int] + creator: Optional[int] = None + modifier: Optional[int] = None + + +class ModelType(int, Enum): + DASHBOARD = 1 + CHART = 2 + REPORT_SCHEDULE = 3 + + +@pytest.mark.parametrize( + "model_type,executor_types,model_config,initiator,expected_result", + [ + ( + ModelType.REPORT_SCHEDULE, + [ExecutorType.SELENIUM], + ModelConfig( + owners=[1, 2], + creator=3, + modifier=4, + ), + None, + (ExecutorType.SELENIUM, SELENIUM_USER_ID), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.CREATOR, + ExecutorType.CREATOR_OWNER, + ExecutorType.OWNER, + ExecutorType.MODIFIER, + ExecutorType.MODIFIER_OWNER, + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[]), + None, + (ExecutorType.SELENIUM, SELENIUM_USER_ID), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.CREATOR, + ExecutorType.CREATOR_OWNER, + ExecutorType.OWNER, + ExecutorType.MODIFIER, + ExecutorType.MODIFIER_OWNER, + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[], modifier=1), + None, + (ExecutorType.MODIFIER, 1), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.CREATOR, + ExecutorType.CREATOR_OWNER, + ExecutorType.OWNER, + ExecutorType.MODIFIER, + ExecutorType.MODIFIER_OWNER, + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[2], modifier=1), + None, + (ExecutorType.OWNER, 2), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.CREATOR, + ExecutorType.CREATOR_OWNER, + ExecutorType.OWNER, + ExecutorType.MODIFIER, + ExecutorType.MODIFIER_OWNER, + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[2], creator=3, modifier=1), + None, + (ExecutorType.CREATOR, 3), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.OWNER, + ], + ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=3, modifier=4), + None, + (ExecutorType.OWNER, 4), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.OWNER, + ], + ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=3, modifier=8), + None, + (ExecutorType.OWNER, 3), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.MODIFIER_OWNER, + ], + ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=8, modifier=9), + None, + ExecutorNotFoundError(), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.MODIFIER_OWNER, + ], + ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=8, modifier=4), + None, + (ExecutorType.MODIFIER_OWNER, 4), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.CREATOR_OWNER, + ], + ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=8, modifier=9), + None, + ExecutorNotFoundError(), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.CREATOR_OWNER, + ], + ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=4, modifier=8), + None, + (ExecutorType.CREATOR_OWNER, 4), + ), + ( + ModelType.REPORT_SCHEDULE, + [ + ExecutorType.INITIATOR, + ], + ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=4, modifier=8), + None, + ExecutorNotFoundError(), + ), + ( + ModelType.DASHBOARD, + [ + ExecutorType.INITIATOR, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + 4, + (ExecutorType.INITIATOR, 4), + ), + ( + ModelType.DASHBOARD, + [ + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + 4, + (ExecutorType.SELENIUM, SELENIUM_USER_ID), + ), + ( + ModelType.DASHBOARD, + [ + ExecutorType.INITIATOR, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + None, + ExecutorNotFoundError(), + ), + ( + ModelType.DASHBOARD, + [ + ExecutorType.CREATOR_OWNER, + ExecutorType.MODIFIER_OWNER, + ExecutorType.INITIATOR, + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + None, + (ExecutorType.SELENIUM, SELENIUM_USER_ID), + ), + ( + ModelType.CHART, + [ + ExecutorType.INITIATOR, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + 4, + (ExecutorType.INITIATOR, 4), + ), + ( + ModelType.CHART, + [ + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + 4, + (ExecutorType.SELENIUM, SELENIUM_USER_ID), + ), + ( + ModelType.CHART, + [ + ExecutorType.INITIATOR, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + None, + ExecutorNotFoundError(), + ), + ( + ModelType.CHART, + [ + ExecutorType.CREATOR_OWNER, + ExecutorType.MODIFIER_OWNER, + ExecutorType.INITIATOR, + ExecutorType.SELENIUM, + ], + ModelConfig(owners=[1], creator=2, modifier=3), + None, + (ExecutorType.SELENIUM, SELENIUM_USER_ID), + ), + ], +) +def test_get_executor( + model_type: ModelType, + executor_types: List[ExecutorType], + model_config: ModelConfig, + initiator: Optional[int], + expected_result: Tuple[int, ExecutorNotFoundError], +) -> None: + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + from superset.reports.models import ReportSchedule + from superset.tasks.utils import get_executor + + model: Type[Union[Dashboard, ReportSchedule, Slice]] + model_kwargs: Dict[str, Any] = {} + if model_type == ModelType.REPORT_SCHEDULE: + model = ReportSchedule + model_kwargs = { + "type": "report", + "name": "test_report", + } + elif model_type == ModelType.DASHBOARD: + model = Dashboard + elif model_type == ModelType.CHART: + model = Slice + else: + raise Exception(f"Unsupported model type: {model_type}") + + obj = model( + id=1, + owners=_get_users(model_config.owners), + created_by=_get_users(model_config.creator), + changed_by=_get_users(model_config.modifier), + **model_kwargs, + ) + if isinstance(expected_result, Exception): + cm = pytest.raises(type(expected_result)) + expected_executor_type = None + expected_executor = None + else: + cm = nullcontext() + expected_executor_type = expected_result[0] + expected_executor = ( + SELENIUM_USERNAME + if expected_executor_type == ExecutorType.SELENIUM + else str(expected_result[1]) + ) + + with cm: + executor_type, executor = get_executor( + executor_types=executor_types, + model=obj, + initiator=str(initiator) if initiator else None, + ) + assert executor_type == expected_executor_type + assert executor == expected_executor From 6bb44491255d7e962fdbf63fac90822c223cfb2e Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 13:36:48 +0200 Subject: [PATCH 04/20] add new tests --- superset/tasks/utils.py | 7 +- superset/thumbnails/digest.py | 4 +- tests/unit_tests/thumbnails/__init__.py | 0 tests/unit_tests/thumbnails/test_digest.py | 258 +++++++++++++++++++++ 4 files changed, 264 insertions(+), 5 deletions(-) create mode 100644 tests/unit_tests/thumbnails/__init__.py create mode 100644 tests/unit_tests/thumbnails/test_digest.py diff --git a/superset/tasks/utils.py b/superset/tasks/utils.py index d62bece35f090..ddf192e0b3300 100644 --- a/superset/tasks/utils.py +++ b/superset/tasks/utils.py @@ -19,7 +19,8 @@ from typing import List, Optional, Tuple, TYPE_CHECKING, Union -from superset import app +from flask import current_app + from superset.tasks.exceptions import ExecutorNotFoundError from superset.tasks.types import ExecutorType @@ -53,10 +54,10 @@ def get_executor( owners = model.owners owner_dict = {owner.id: owner for owner in owners} for executor_type in executor_types: + if executor_type == ExecutorType.SELENIUM: + return executor_type, current_app.config["THUMBNAIL_SELENIUM_USER"] if executor_type == ExecutorType.INITIATOR and initiator: return executor_type, initiator - if executor_type == ExecutorType.SELENIUM: - return executor_type, app.config["THUMBNAIL_SELENIUM_USER"] if executor_type == ExecutorType.CREATOR_OWNER: if (user := model.created_by) and (owner := owner_dict.get(user.id)): return executor_type, owner.username diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py index 43c9c9423dfd7..da6a56f2370bd 100644 --- a/superset/thumbnails/digest.py +++ b/superset/thumbnails/digest.py @@ -56,7 +56,7 @@ def get_dashboard_digest(dashboard: Dashboard) -> str: executor_type, executor = get_executor( executor_types=config["THUMBNAIL_EXECUTE_AS"], model=dashboard, - initiator=g.user.username, + initiator=g.user.username if hasattr(g, "user") and g.user else None, ) if func := config["THUMBNAIL_DASHBOARD_DIGEST_FUNC"]: return func(dashboard, executor_type, executor) @@ -75,7 +75,7 @@ def get_chart_digest(chart: Slice) -> str: executor_type, executor = get_executor( executor_types=config["THUMBNAIL_EXECUTE_AS"], model=chart, - initiator=g.user.username, + initiator=g.user.username if hasattr(g, "user") and g.user else None, ) if func := config["THUMBNAIL_CHART_DIGEST_FUNC"]: return func(chart, executor_type, executor) diff --git a/tests/unit_tests/thumbnails/__init__.py b/tests/unit_tests/thumbnails/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/unit_tests/thumbnails/test_digest.py b/tests/unit_tests/thumbnails/test_digest.py new file mode 100644 index 0000000000000..d492d41f70b89 --- /dev/null +++ b/tests/unit_tests/thumbnails/test_digest.py @@ -0,0 +1,258 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from contextlib import nullcontext +from typing import Any, Dict, List, Optional, TYPE_CHECKING, Union +from unittest.mock import patch + +import pytest +from flask_appbuilder.security.sqla.models import User + +from superset.tasks.exceptions import ExecutorNotFoundError +from superset.tasks.types import ExecutorType +from superset.utils.core import override_user + +if TYPE_CHECKING: + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + +_DEFAULT_DASHBOARD_KWARGS = { + "id": 1, + "position_json": '{"a": "b"}', + "css": "background-color: lightblue;", + "json_metadata": '{"c": "d"}', +} + +_DEFAULT_CHART_KWARGS = { + "id": 2, + "params": {"a": "b"}, +} + + +def CUSTOM_DASHBOARD_FUNC( + dashboard: Dashboard, + executor_type: ExecutorType, + executor: str, +) -> str: + return f"{dashboard.id}.{executor_type.value}.{executor}" + + +def CUSTOM_CHART_FUNC( + chart: Slice, + executor_type: ExecutorType, + executor: str, +) -> str: + return f"{chart.id}.{executor_type.value}.{executor}" + + +@pytest.mark.parametrize( + "dashboard_overrides,secret_key,execute_as,has_initiator,use_custom_digest,expected_result", + [ + ( + None, + "my_secret", + [ExecutorType.SELENIUM], + False, + False, + "9dfd9e0685911ca56f041e57b63bd950", + ), + ( + None, + "my_secret", + [ExecutorType.INITIATOR], + True, + False, + "7c9bccb14f0bac455c5cd0181d9c61a1", + ), + ( + None, + "my_other_secret", + [ExecutorType.INITIATOR], + True, + False, + "3fe8f5fc9fa81c78d064e2b11ba4aaa8", + ), + ( + { + "position_json": {"b": "c"}, + }, + "my_secret", + [ExecutorType.INITIATOR], + True, + False, + "bf9401fd23f778c0026e9b614b81736f", + ), + ( + { + "css": "background-color: darkblue;", + }, + "my_secret", + [ExecutorType.INITIATOR], + True, + False, + "8752d66b796f8e9f81a083b681bdefe9", + ), + ( + { + "json_metadata": {"d": "e"}, + }, + "my_secret", + [ExecutorType.INITIATOR], + True, + False, + "62e1f5bff56c57614c31beb83d67c492", + ), + ( + None, + "my_secret", + [ExecutorType.INITIATOR], + True, + True, + "1.initiator.1", + ), + ( + None, + "my_secret", + [ExecutorType.INITIATOR], + False, + False, + ExecutorNotFoundError(), + ), + ], +) +def test_dashboard_digest( + dashboard_overrides: Optional[Dict[str, Any]], + secret_key: str, + execute_as: List[ExecutorType], + has_initiator: bool, + use_custom_digest: bool, + expected_result: Union[str, Exception], +) -> None: + from superset import app + from superset.models.dashboard import Dashboard + from superset.thumbnails.digest import get_dashboard_digest + + kwargs = { + **_DEFAULT_DASHBOARD_KWARGS, + **(dashboard_overrides or {}), + } + dashboard = Dashboard(**kwargs) + user: Optional[User] = None + if has_initiator: + user = User(id=1, username="1") + func = CUSTOM_DASHBOARD_FUNC if use_custom_digest else None + + with patch.dict( + app.config, + { + "SECRET_KEY": secret_key, + "THUMBNAIL_EXECUTE_AS": execute_as, + "THUMBNAIL_DASHBOARD_DIGEST_FUNC": func, + }, + ), override_user(user): + cm = ( + pytest.raises(type(expected_result)) + if isinstance(expected_result, Exception) + else nullcontext() + ) + with cm: + assert get_dashboard_digest(dashboard=dashboard) == expected_result + + +@pytest.mark.parametrize( + "chart_overrides,secret_key,execute_as,has_initiator,use_custom_digest,expected_result", + [ + ( + None, + "my_secret", + [ExecutorType.SELENIUM], + False, + False, + "47d852b5c4df211c115905617bb722c1", + ), + ( + None, + "my_secret", + [ExecutorType.INITIATOR], + True, + False, + "97b78018f0eac76156bb54d3051faed0", + ), + ( + None, + "my_other_secret", + [ExecutorType.INITIATOR], + True, + False, + "9558cafa50981d550019c854f553b393", + ), + ( + None, + "my_secret", + [ExecutorType.INITIATOR], + True, + True, + "2.initiator.1", + ), + ( + None, + "my_secret", + [ExecutorType.INITIATOR], + False, + False, + ExecutorNotFoundError(), + ), + ], +) +def test_chart_digest( + chart_overrides: Optional[Dict[str, Any]], + secret_key: str, + execute_as: List[ExecutorType], + has_initiator: bool, + use_custom_digest: bool, + expected_result: Union[str, Exception], +) -> None: + from superset import app + from superset.models.slice import Slice + from superset.thumbnails.digest import get_chart_digest + + kwargs = { + **_DEFAULT_CHART_KWARGS, + **(chart_overrides or {}), + } + chart = Slice(**kwargs) + user: Optional[User] = None + if has_initiator: + user = User(id=1, username="1") + func = CUSTOM_CHART_FUNC if use_custom_digest else None + + with patch.dict( + app.config, + { + "SECRET_KEY": secret_key, + "THUMBNAIL_EXECUTE_AS": execute_as, + "THUMBNAIL_CHART_DIGEST_FUNC": func, + }, + ), override_user(user): + cm = ( + pytest.raises(type(expected_result)) + if isinstance(expected_result, Exception) + else nullcontext() + ) + with cm: + assert get_chart_digest(chart=chart) == expected_result From 441919544891f3e995cb05ad8bf4d8553ba19cea Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 13:44:34 +0200 Subject: [PATCH 05/20] add license headers --- tests/unit_tests/tasks/__init__.py | 16 ++++++++++++++++ tests/unit_tests/thumbnails/__init__.py | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 tests/unit_tests/tasks/__init__.py diff --git a/tests/unit_tests/tasks/__init__.py b/tests/unit_tests/tasks/__init__.py new file mode 100644 index 0000000000000..13a83393a9124 --- /dev/null +++ b/tests/unit_tests/tasks/__init__.py @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. diff --git a/tests/unit_tests/thumbnails/__init__.py b/tests/unit_tests/thumbnails/__init__.py index e69de29bb2d1d..13a83393a9124 100644 --- a/tests/unit_tests/thumbnails/__init__.py +++ b/tests/unit_tests/thumbnails/__init__.py @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. From 1f2241968413a64c0b66382b690c855c80926323 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 13:45:39 +0200 Subject: [PATCH 06/20] lint --- superset/thumbnails/tasks.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/thumbnails/tasks.py b/superset/thumbnails/tasks.py index 1c01673344f5c..e11f01ad657f6 100644 --- a/superset/thumbnails/tasks.py +++ b/superset/thumbnails/tasks.py @@ -41,6 +41,7 @@ def cache_chart_thumbnail( window_size: Optional[WindowSize] = None, thumb_size: Optional[WindowSize] = None, ) -> None: + # pylint: disable=import-outside-toplevel from superset.models.slice import Slice if not thumbnail_cache: @@ -74,15 +75,14 @@ def cache_dashboard_thumbnail( force: bool = False, thumb_size: Optional[WindowSize] = None, ) -> None: + # pylint: disable=import-outside-toplevel from superset.models.dashboard import Dashboard if not thumbnail_cache: logging.warning("No cache set, refusing to compute") return dashboard = Dashboard.get(dashboard_id) - url = get_url_path( - "Superset.dashboard", dashboard_id_or_slug=dashboard.id - ) + url = get_url_path("Superset.dashboard", dashboard_id_or_slug=dashboard.id) logger.info("Caching dashboard: %s", url) _, username = get_executor( From 552cb59b4760bcd5424eae31c089d662d1c9ec5a Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 13:55:42 +0200 Subject: [PATCH 07/20] fix alerts tests --- .../integration_tests/reports/alert_tests.py | 31 +++++++++++++------ .../reports/commands_tests.py | 6 ++-- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/tests/integration_tests/reports/alert_tests.py b/tests/integration_tests/reports/alert_tests.py index ef51bf1d0db45..502cdbff04367 100644 --- a/tests/integration_tests/reports/alert_tests.py +++ b/tests/integration_tests/reports/alert_tests.py @@ -16,7 +16,7 @@ # under the License. # pylint: disable=invalid-name, unused-argument, import-outside-toplevel from contextlib import nullcontext -from typing import List, Optional, Union +from typing import List, Optional, Tuple, Union import pandas as pd import pytest @@ -24,7 +24,7 @@ from superset.reports.commands.exceptions import AlertQueryError from superset.reports.models import ReportCreationMethod, ReportScheduleType -from superset.reports.types import ReportScheduleExecutor +from superset.tasks.types import ExecutorType from superset.utils.database import get_example_database from tests.integration_tests.test_app import app @@ -32,23 +32,34 @@ @pytest.mark.parametrize( "owner_names,creator_name,config,expected_result", [ - (["gamma"], None, [ReportScheduleExecutor.SELENIUM], "admin"), - (["gamma"], None, [ReportScheduleExecutor.OWNER], "gamma"), - (["alpha", "gamma"], "gamma", [ReportScheduleExecutor.CREATOR_OWNER], "gamma"), - (["alpha", "gamma"], "alpha", [ReportScheduleExecutor.CREATOR_OWNER], "alpha"), + (["gamma"], None, [ExecutorType.SELENIUM], (ExecutorType.SELENIUM, "admin")), + (["gamma"], None, [ExecutorType.OWNER], (ExecutorType.OWNER, "gamma")), + ( + ["alpha", "gamma"], + "gamma", + [ExecutorType.CREATOR_OWNER], + (ExecutorType.CREATOR_OWNER, "gamma"), + ), + ( + ["alpha", "gamma"], + "alpha", + [ExecutorType.CREATOR_OWNER], + (ExecutorType.CREATOR_OWNER, "alpha"), + ), ( ["alpha", "gamma"], "admin", - [ReportScheduleExecutor.CREATOR_OWNER], + [ExecutorType.CREATOR_OWNER], AlertQueryError(), ), + (["gamma"], None, [ExecutorType.INITIATOR], AlertQueryError()), ], ) def test_execute_query_as_report_executor( owner_names: List[str], creator_name: Optional[str], - config: List[ReportScheduleExecutor], - expected_result: Union[str, Exception], + config: List[ExecutorType], + expected_result: Union[Tuple[ExecutorType, str], Exception], mocker: MockFixture, app_context: None, get_user, @@ -85,7 +96,7 @@ def test_execute_query_as_report_executor( ) with cm: command.run() - assert override_user_mock.call_args[0][0].username == expected_result + assert override_user_mock.call_args[0][0] == expected_result app.config["ALERT_REPORTS_EXECUTE_AS"] = original_config diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 288e6746cc5c7..ebbba499281be 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -41,13 +41,11 @@ ReportScheduleClientErrorsException, ReportScheduleCsvFailedError, ReportScheduleCsvTimeout, - ReportScheduleForbiddenError, ReportScheduleNotFoundError, ReportSchedulePreviousWorkingError, ReportScheduleScreenshotFailedError, ReportScheduleScreenshotTimeout, ReportScheduleSystemErrorsException, - ReportScheduleUnexpectedError, ReportScheduleWorkingTimeoutError, ) from superset.reports.commands.execute import ( @@ -67,7 +65,7 @@ NotificationError, NotificationParamException, ) -from superset.reports.types import ReportScheduleExecutor +from superset.tasks.types import ExecutorType from superset.utils.database import get_example_database from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, @@ -686,7 +684,7 @@ def test_email_chart_report_schedule_alpha_owner( """ config_key = "ALERT_REPORTS_EXECUTE_AS" original_config_value = app.config[config_key] - app.config[config_key] = [ReportScheduleExecutor.OWNER] + app.config[config_key] = [ExecutorType.OWNER] # setup screenshot mock username = "" From 37ae1ba83841edae60968c047c3ccff460504c8f Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 14:53:25 +0200 Subject: [PATCH 08/20] add check for anonymous users --- superset/models/dashboard.py | 3 ++- superset/thumbnails/digest.py | 7 ++++--- superset/thumbnails/utils.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 superset/thumbnails/utils.py diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 43c62b39408d6..41f3768139ada 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -56,6 +56,7 @@ from superset.models.user_attributes import UserAttribute from superset.thumbnails.digest import get_dashboard_digest from superset.thumbnails.tasks import cache_dashboard_thumbnail +from superset.thumbnails.utils import get_initiator from superset.utils import core as utils from superset.utils.core import get_user_id from superset.utils.decorators import debounce @@ -324,7 +325,7 @@ def position(self) -> Dict[str, Any]: return {} def update_thumbnail(self) -> None: - username = user.username if (user := security_manager.current_user) else None + username = get_initiator() cache_dashboard_thumbnail.delay( username=username, dashboard_id=self.id, diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py index da6a56f2370bd..abb802c4487cb 100644 --- a/superset/thumbnails/digest.py +++ b/superset/thumbnails/digest.py @@ -20,10 +20,11 @@ import logging from typing import TYPE_CHECKING -from flask import current_app, g +from flask import current_app from superset.tasks.types import ExecutorType from superset.tasks.utils import get_executor +from superset.thumbnails.utils import get_initiator from superset.utils.hashing import md5_sha_from_str if TYPE_CHECKING: @@ -56,7 +57,7 @@ def get_dashboard_digest(dashboard: Dashboard) -> str: executor_type, executor = get_executor( executor_types=config["THUMBNAIL_EXECUTE_AS"], model=dashboard, - initiator=g.user.username if hasattr(g, "user") and g.user else None, + initiator=get_initiator(), ) if func := config["THUMBNAIL_DASHBOARD_DIGEST_FUNC"]: return func(dashboard, executor_type, executor) @@ -75,7 +76,7 @@ def get_chart_digest(chart: Slice) -> str: executor_type, executor = get_executor( executor_types=config["THUMBNAIL_EXECUTE_AS"], model=chart, - initiator=g.user.username if hasattr(g, "user") and g.user else None, + initiator=get_initiator(), ) if func := config["THUMBNAIL_CHART_DIGEST_FUNC"]: return func(chart, executor_type, executor) diff --git a/superset/thumbnails/utils.py b/superset/thumbnails/utils.py new file mode 100644 index 0000000000000..d03c4ddc3e639 --- /dev/null +++ b/superset/thumbnails/utils.py @@ -0,0 +1,28 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from typing import Optional + +from flask import g + + +def get_initiator() -> Optional[str]: + user = g.user if hasattr(g, "user") and g.user and g.user else None + if user and not user.is_anonymous: + return user.username + + return None From 7be90b706a8b306a8ca4e4476bfee1c76e77112a Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 15:34:36 +0200 Subject: [PATCH 09/20] fix more tests --- superset/charts/api.py | 14 ++-- superset/dashboards/api.py | 9 +-- superset/models/dashboard.py | 6 +- superset/models/slice.py | 4 +- superset/tasks/utils.py | 10 ++- superset/thumbnails/digest.py | 2 +- superset/thumbnails/tasks.py | 8 +-- superset/thumbnails/utils.py | 28 -------- tests/integration_tests/thumbnails_tests.py | 72 ++++++++++++++++----- 9 files changed, 84 insertions(+), 69 deletions(-) delete mode 100644 superset/thumbnails/utils.py diff --git a/superset/charts/api.py b/superset/charts/api.py index 4c01b9ad76e7e..b2ced44624ff0 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -30,7 +30,7 @@ from werkzeug.wrappers import Response as WerkzeugResponse from werkzeug.wsgi import FileWrapper -from superset import app, is_feature_enabled, security_manager, thumbnail_cache +from superset import app, is_feature_enabled, thumbnail_cache from superset.charts.commands.bulk_delete import BulkDeleteChartCommand from superset.charts.commands.create import CreateChartCommand from superset.charts.commands.delete import DeleteChartCommand @@ -75,6 +75,7 @@ from superset.extensions import event_logger from superset.models.slice import Slice from superset.thumbnails.tasks import cache_chart_thumbnail +from superset.tasks.utils import get_initiator from superset.utils.screenshots import ChartScreenshot from superset.utils.urls import get_url_path from superset.views.base_api import ( @@ -570,11 +571,8 @@ def cache_screenshot(self, pk: int, **kwargs: Any) -> WerkzeugResponse: def trigger_celery() -> WerkzeugResponse: logger.info("Triggering screenshot ASYNC") - username = ( - user.username if (user := security_manager.current_user) else None - ) cache_chart_thumbnail.delay( - username=username, + initiator=get_initiator(), chart_id=chart.id, force=True, window_size=window_size, @@ -686,14 +684,14 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: if not chart: return self.response_404() - username = user.username if (user := security_manager.current_user) else None + initiator = get_initiator() url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") if kwargs["rison"].get("force", False): logger.info( "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) ) cache_chart_thumbnail.delay( - username=username, + initiator=initiator, chart_id=chart.id, force=True, ) @@ -709,7 +707,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) ) cache_chart_thumbnail.delay( - username=username, + initiator=initiator, chart_id=chart.id, force=True, ) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 60ca2e1a8f3e0..59adcb9f2f723 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -33,7 +33,7 @@ from werkzeug.wrappers import Response as WerkzeugResponse from werkzeug.wsgi import FileWrapper -from superset import is_feature_enabled, security_manager, thumbnail_cache +from superset import is_feature_enabled, thumbnail_cache from superset.charts.schemas import ChartEntityResponseSchema from superset.commands.importers.exceptions import NoValidFilesFoundError from superset.commands.importers.v1.utils import get_contents_from_bundle @@ -83,6 +83,7 @@ from superset.models.dashboard import Dashboard from superset.models.embedded_dashboard import EmbeddedDashboard from superset.thumbnails.tasks import cache_dashboard_thumbnail +from superset.tasks.utils import get_initiator from superset.utils.cache import etag_cache from superset.utils.screenshots import DashboardScreenshot from superset.utils.urls import get_url_path @@ -887,10 +888,10 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: "Superset.dashboard", dashboard_id_or_slug=dashboard.id ) # If force, request a screenshot from the workers - username = user.username if (user := security_manager.current_user) else None + initiator = get_initiator() if kwargs["rison"].get("force", False): cache_dashboard_thumbnail.delay( - username=username, + initiator=initiator, dashboard_id=dashboard.id, force=True, ) @@ -903,7 +904,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: if not screenshot: self.incr_stats("async", self.thumbnail.__name__) cache_dashboard_thumbnail.delay( - username=username, + initiator=initiator, dashboard_id=dashboard.id, force=True, ) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 41f3768139ada..657d13cc7d5ad 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -56,7 +56,7 @@ from superset.models.user_attributes import UserAttribute from superset.thumbnails.digest import get_dashboard_digest from superset.thumbnails.tasks import cache_dashboard_thumbnail -from superset.thumbnails.utils import get_initiator +from superset.tasks.utils import get_initiator from superset.utils import core as utils from superset.utils.core import get_user_id from superset.utils.decorators import debounce @@ -325,11 +325,9 @@ def position(self) -> Dict[str, Any]: return {} def update_thumbnail(self) -> None: - username = get_initiator() cache_dashboard_thumbnail.delay( - username=username, + initiator=get_initiator(), dashboard_id=self.id, - digest=self.digest, force=True, ) diff --git a/superset/models/slice.py b/superset/models/slice.py index 1325fafd31b36..ca0b2332f243e 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -44,6 +44,7 @@ from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.thumbnails.digest import get_chart_digest from superset.thumbnails.tasks import cache_chart_thumbnail +from superset.tasks.utils import get_initiator from superset.utils import core as utils from superset.utils.memoized import memoized from superset.viz import BaseViz, viz_types @@ -360,9 +361,8 @@ def set_related_perm(_mapper: Mapper, _connection: Connection, target: Slice) -> def event_after_chart_changed( _mapper: Mapper, _connection: Connection, target: Slice ) -> None: - username = user.username if (user := security_manager.current_user) else None cache_chart_thumbnail.delay( - username=username, + initiator=get_initiator(), chart_id=target.id, force=True, ) diff --git a/superset/tasks/utils.py b/superset/tasks/utils.py index ddf192e0b3300..79fbe3a315282 100644 --- a/superset/tasks/utils.py +++ b/superset/tasks/utils.py @@ -19,7 +19,7 @@ from typing import List, Optional, Tuple, TYPE_CHECKING, Union -from flask import current_app +from flask import current_app, g from superset.tasks.exceptions import ExecutorNotFoundError from superset.tasks.types import ExecutorType @@ -84,3 +84,11 @@ def get_executor( return executor_type, owners[0].username raise ExecutorNotFoundError() + + +def get_initiator() -> Optional[str]: + user = g.user if hasattr(g, "user") and g.user and g.user else None + if user and not user.is_anonymous: + return user.username + + return None diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py index abb802c4487cb..f14cbeda4aabd 100644 --- a/superset/thumbnails/digest.py +++ b/superset/thumbnails/digest.py @@ -24,7 +24,7 @@ from superset.tasks.types import ExecutorType from superset.tasks.utils import get_executor -from superset.thumbnails.utils import get_initiator +from superset.tasks.utils import get_initiator from superset.utils.hashing import md5_sha_from_str if TYPE_CHECKING: diff --git a/superset/thumbnails/tasks.py b/superset/thumbnails/tasks.py index e11f01ad657f6..d340bdb8bda7b 100644 --- a/superset/thumbnails/tasks.py +++ b/superset/thumbnails/tasks.py @@ -35,7 +35,7 @@ @celery_app.task(name="cache_chart_thumbnail", soft_time_limit=300) def cache_chart_thumbnail( - username: Optional[str], + initiator: Optional[str], chart_id: int, force: bool = False, window_size: Optional[WindowSize] = None, @@ -53,7 +53,7 @@ def cache_chart_thumbnail( _, username = get_executor( executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], model=chart, - initiator=username, + initiator=initiator, ) user = security_manager.find_user(username) with override_user(user): @@ -70,7 +70,7 @@ def cache_chart_thumbnail( @celery_app.task(name="cache_dashboard_thumbnail", soft_time_limit=300) def cache_dashboard_thumbnail( - username: Optional[str], + initiator: Optional[str], dashboard_id: int, force: bool = False, thumb_size: Optional[WindowSize] = None, @@ -88,7 +88,7 @@ def cache_dashboard_thumbnail( _, username = get_executor( executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], model=dashboard, - initiator=username, + initiator=initiator, ) user = security_manager.find_user(username) with override_user(user): diff --git a/superset/thumbnails/utils.py b/superset/thumbnails/utils.py deleted file mode 100644 index d03c4ddc3e639..0000000000000 --- a/superset/thumbnails/utils.py +++ /dev/null @@ -1,28 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -from typing import Optional - -from flask import g - - -def get_initiator() -> Optional[str]: - user = g.user if hasattr(g, "user") and g.user and g.user else None - if user and not user.is_anonymous: - return user.username - - return None diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index 5eabc4da00090..b441886aeb50a 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -28,8 +28,9 @@ from superset.extensions import machine_auth_provider_factory from superset.models.dashboard import Dashboard from superset.models.slice import Slice +from superset.tasks.types import ExecutorType from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot -from superset.utils.urls import get_url_host, get_url_path +from superset.utils.urls import get_url_path from superset.utils.webdriver import WebDriverProxy from tests.integration_tests.conftest import with_feature_flags from tests.integration_tests.test_app import app @@ -145,24 +146,45 @@ def test_chart_thumbnail_disabled(self): self.assertEqual(rv.status_code, 404) @with_feature_flags(THUMBNAILS=True) - def test_get_async_dashboard_screenshot(self): + def test_get_async_dashboard_screenshot_as_selenium(self): """ - Thumbnails: Simple get async dashboard screenshot + Thumbnails: Simple get async dashboard screenshot as selenium user """ dashboard = db.session.query(Dashboard).all()[0] - self.login(username="admin") + self.login(username="alpha") uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" with patch( - "superset.tasks.thumbnails.cache_dashboard_thumbnail.delay" + "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" ) as mock_task: rv = self.client.get(uri) self.assertEqual(rv.status_code, 202) - expected_uri = f"{get_url_host()}superset/dashboard/{dashboard.id}/" - expected_digest = dashboard.digest - expected_kwargs = {"force": True} mock_task.assert_called_with( - expected_uri, expected_digest, **expected_kwargs + initiator="admin", + dashboard_id=dashboard.id, + force=True, + ) + + @with_feature_flags(THUMBNAILS=True) + def test_get_async_dashboard_screenshot_as_initiator(self): + """ + Thumbnails: Simple get async dashboard screenshot as initiator + """ + dashboard = db.session.query(Dashboard).all()[0] + self.login(username="alpha") + uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" + with patch( + "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" + ) as mock_task, patch.dict( + "superset.thumbnails.tasks.current_app.config", [ExecutorType.INITIATOR] + ): + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 202) + + mock_task.assert_called_with( + initiator="alpha", + dashboard_id=dashboard.id, + force=True, ) @with_feature_flags(THUMBNAILS=True) @@ -188,23 +210,39 @@ def test_get_async_dashboard_not_allowed(self): self.assertEqual(rv.status_code, 404) @with_feature_flags(THUMBNAILS=True) - def test_get_async_chart_screenshot(self): + def test_get_async_chart_screenshot_as_selenium(self): """ - Thumbnails: Simple get async chart screenshot + Thumbnails: Simple get async chart screenshot as selenium user """ chart = db.session.query(Slice).all()[0] - self.login(username="admin") + self.login(username="alpha") uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" with patch( - "superset.tasks.thumbnails.cache_chart_thumbnail.delay" + "superset.thumbnails.tasks.cache_chart_thumbnail.delay" ) as mock_task: rv = self.client.get(uri) self.assertEqual(rv.status_code, 202) - expected_uri = f"{get_url_host()}superset/slice/{chart.id}/?standalone=true" - expected_digest = chart.digest - expected_kwargs = {"force": True} mock_task.assert_called_with( - expected_uri, expected_digest, **expected_kwargs + initiator="admin", chart_id=chart.id, force=True + ) + + @with_feature_flags(THUMBNAILS=True) + def test_get_async_chart_screenshot_as_initiator(self): + """ + Thumbnails: Simple get async chart screenshot as initiator + """ + chart = db.session.query(Slice).all()[0] + self.login(username="alpha") + uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" + with patch( + "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" + ) as mock_task, patch.dict( + "superset.thumbnails.tasks.current_app.config", [ExecutorType.INITIATOR] + ): + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 202) + mock_task.assert_called_with( + initiator="alpha", chart_id=chart.id, force=True ) @with_feature_flags(THUMBNAILS=True) From 7abf5927ad0d73c8564df8dbb309c3fb9625618c Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 16:36:20 +0200 Subject: [PATCH 10/20] fix test --- tests/integration_tests/thumbnails_tests.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index b441886aeb50a..4dd3ee99b0042 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -172,12 +172,15 @@ def test_get_async_dashboard_screenshot_as_initiator(self): """ dashboard = db.session.query(Dashboard).all()[0] self.login(username="alpha") - uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" with patch( "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" ) as mock_task, patch.dict( - "superset.thumbnails.tasks.current_app.config", [ExecutorType.INITIATOR] + "superset.thumbnails.tasks.current_app.config", + { + "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], + }, ): + uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" rv = self.client.get(uri) self.assertEqual(rv.status_code, 202) @@ -233,12 +236,15 @@ def test_get_async_chart_screenshot_as_initiator(self): """ chart = db.session.query(Slice).all()[0] self.login(username="alpha") - uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" with patch( - "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" + "superset.thumbnails.tasks.cache_chart_thumbnail.delay" ) as mock_task, patch.dict( - "superset.thumbnails.tasks.current_app.config", [ExecutorType.INITIATOR] + "superset.thumbnails.tasks.current_app.config", + { + "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], + }, ): + uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" rv = self.client.get(uri) self.assertEqual(rv.status_code, 202) mock_task.assert_called_with( From 343939105b8d86928769f8ec9688a80327c23053 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Mon, 5 Dec 2022 18:54:52 +0200 Subject: [PATCH 11/20] fix thumbnail mocks --- tests/integration_tests/thumbnails_tests.py | 34 ++++++++++++--------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index 4dd3ee99b0042..1016bcbdd971d 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -171,21 +171,25 @@ def test_get_async_dashboard_screenshot_as_initiator(self): Thumbnails: Simple get async dashboard screenshot as initiator """ dashboard = db.session.query(Dashboard).all()[0] - self.login(username="alpha") + username = "alpha" + self.login(username=username) with patch( "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" - ) as mock_task, patch.dict( - "superset.thumbnails.tasks.current_app.config", + ) as mock_task, patch( + "superset.thumbnails.digest.get_initiator" + ) as mock_initiator, patch.dict( + "superset.thumbnails.digest.current_app.config", { "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], }, ): + mock_initiator.return_value = username uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" rv = self.client.get(uri) self.assertEqual(rv.status_code, 202) mock_task.assert_called_with( - initiator="alpha", + initiator=username, dashboard_id=dashboard.id, force=True, ) @@ -219,15 +223,13 @@ def test_get_async_chart_screenshot_as_selenium(self): """ chart = db.session.query(Slice).all()[0] self.login(username="alpha") - uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" with patch( - "superset.thumbnails.tasks.cache_chart_thumbnail.delay" - ) as mock_task: + "superset.thumbnails.tasks.security_manager.find_user" + ) as mock_find_user: + uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" rv = self.client.get(uri) self.assertEqual(rv.status_code, 202) - mock_task.assert_called_with( - initiator="admin", chart_id=chart.id, force=True - ) + mock_find_user.assert_called_with("admin") @with_feature_flags(THUMBNAILS=True) def test_get_async_chart_screenshot_as_initiator(self): @@ -235,20 +237,24 @@ def test_get_async_chart_screenshot_as_initiator(self): Thumbnails: Simple get async chart screenshot as initiator """ chart = db.session.query(Slice).all()[0] - self.login(username="alpha") + username = "alpha" + self.login(username=username) with patch( "superset.thumbnails.tasks.cache_chart_thumbnail.delay" - ) as mock_task, patch.dict( - "superset.thumbnails.tasks.current_app.config", + ) as mock_task, patch( + "superset.thumbnails.digest.get_initiator" + ) as mock_initiator, patch.dict( + "superset.thumbnails.digest.current_app.config", { "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], }, ): + mock_initiator.return_value = username uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" rv = self.client.get(uri) self.assertEqual(rv.status_code, 202) mock_task.assert_called_with( - initiator="alpha", chart_id=chart.id, force=True + initiator=username, chart_id=chart.id, force=True ) @with_feature_flags(THUMBNAILS=True) From 9fcd416a8af4e833ff8f68ce7b3a874a1599fb83 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 7 Dec 2022 10:41:04 +0200 Subject: [PATCH 12/20] fix tests and add docs --- docs/docs/installation/cache.mdx | 7 ++ tests/integration_tests/thumbnails_tests.py | 120 ++++++++++++-------- 2 files changed, 79 insertions(+), 48 deletions(-) diff --git a/docs/docs/installation/cache.mdx b/docs/docs/installation/cache.mdx index eeb2c49a663c4..9340ad03df648 100644 --- a/docs/docs/installation/cache.mdx +++ b/docs/docs/installation/cache.mdx @@ -53,6 +53,13 @@ FEATURE_FLAGS = { } ``` +By default thumbnails are rendered using the `THUMBNAIL_SELENIUM_USER` user account. To render thumbnails as the +logged in user (e.g. in environments that are using user impersonation), use the following configuration: + +```python +THUMBNAIL_EXECUTE_AS = [ExecutorType.INITIATOR] +``` + For this feature you will need a cache system and celery workers. All thumbnails are stored on cache and are processed asynchronously by the workers. diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index 1016bcbdd971d..76b285c1d00b3 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -16,11 +16,14 @@ # under the License. # from superset import db # from superset.models.dashboard import Dashboard + +import json import urllib.request from io import BytesIO from unittest import skipUnless from unittest.mock import ANY, call, patch +import pytest from flask_testing import LiveServerTestCase from sqlalchemy.sql import func @@ -33,6 +36,10 @@ from superset.utils.urls import get_url_path from superset.utils.webdriver import WebDriverProxy from tests.integration_tests.conftest import with_feature_flags +from tests.integration_tests.fixtures.birth_names_dashboard import ( + load_birth_names_dashboard_with_slices, + load_birth_names_data, +) from tests.integration_tests.test_app import app from .base_tests import SupersetTestCase @@ -54,11 +61,14 @@ def test_get_async_dashboard_screenshot(self): """ Thumbnails: Simple get async dashboard screenshot """ - dashboard = db.session.query(Dashboard).all()[0] with patch("superset.dashboards.api.DashboardRestApi.get") as mock_get: + rv = self.client.get("/api/v1/dashboard/") + resp = json.loads(rv.data.decode("utf-8")) + thumbnail_url = resp["result"][0]["thumbnail_url"] + response = self.url_open_auth( "admin", - f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/", + thumbnail_url, ) self.assertEqual(response.getcode(), 202) @@ -122,7 +132,15 @@ def test_screenshot_selenium_animation_wait( class TestThumbnails(SupersetTestCase): mock_image = b"bytes mock image" + digest_return_value = "foo_bar" + digest_hash = "5c7d96a3dd7a87850a2ef34087565a6e" + + def _get_thumbnail_url(self, url: str): + rv = self.client.get(url) + resp = json.loads(rv.data.decode("utf-8")) + return resp["result"][0]["thumbnail_url"] + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=False) def test_dashboard_thumbnail_disabled(self): """ @@ -134,6 +152,7 @@ def test_dashboard_thumbnail_disabled(self): rv = self.client.get(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=False) def test_chart_thumbnail_disabled(self): """ @@ -145,55 +164,51 @@ def test_chart_thumbnail_disabled(self): rv = self.client.get(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_async_dashboard_screenshot_as_selenium(self): """ Thumbnails: Simple get async dashboard screenshot as selenium user """ - dashboard = db.session.query(Dashboard).all()[0] self.login(username="alpha") - uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" with patch( - "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" - ) as mock_task: - rv = self.client.get(uri) - self.assertEqual(rv.status_code, 202) + "superset.thumbnails.digest._adjust_string_for_executor" + ) as mock_adjust_string: + mock_adjust_string.return_value = self.digest_return_value + thumbnail_url = self._get_thumbnail_url("/api/v1/dashboard/") + assert self.digest_hash in thumbnail_url + assert mock_adjust_string.call_args[0][1] == ExecutorType.SELENIUM + assert mock_adjust_string.call_args[0][2] == "admin" - mock_task.assert_called_with( - initiator="admin", - dashboard_id=dashboard.id, - force=True, - ) + rv = self.client.get(thumbnail_url) + self.assertEqual(rv.status_code, 202) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_async_dashboard_screenshot_as_initiator(self): """ Thumbnails: Simple get async dashboard screenshot as initiator """ - dashboard = db.session.query(Dashboard).all()[0] username = "alpha" self.login(username=username) - with patch( - "superset.thumbnails.tasks.cache_dashboard_thumbnail.delay" - ) as mock_task, patch( - "superset.thumbnails.digest.get_initiator" - ) as mock_initiator, patch.dict( + with patch.dict( "superset.thumbnails.digest.current_app.config", { "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], }, - ): - mock_initiator.return_value = username - uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" - rv = self.client.get(uri) - self.assertEqual(rv.status_code, 202) + ), patch( + "superset.thumbnails.digest._adjust_string_for_executor" + ) as mock_adjust_string: + mock_adjust_string.return_value = self.digest_return_value + thumbnail_url = self._get_thumbnail_url("/api/v1/dashboard/") + assert self.digest_hash in thumbnail_url + assert mock_adjust_string.call_args[0][1] == ExecutorType.INITIATOR + assert mock_adjust_string.call_args[0][2] == username - mock_task.assert_called_with( - initiator=username, - dashboard_id=dashboard.id, - force=True, - ) + rv = self.client.get(thumbnail_url) + self.assertEqual(rv.status_code, 202) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_async_dashboard_notfound(self): """ @@ -205,6 +220,7 @@ def test_get_async_dashboard_notfound(self): rv = self.client.get(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature") def test_get_async_dashboard_not_allowed(self): """ @@ -216,47 +232,51 @@ def test_get_async_dashboard_not_allowed(self): rv = self.client.get(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_async_chart_screenshot_as_selenium(self): """ Thumbnails: Simple get async chart screenshot as selenium user """ - chart = db.session.query(Slice).all()[0] self.login(username="alpha") with patch( - "superset.thumbnails.tasks.security_manager.find_user" - ) as mock_find_user: - uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" - rv = self.client.get(uri) + "superset.thumbnails.digest._adjust_string_for_executor" + ) as mock_adjust_string: + mock_adjust_string.return_value = self.digest_return_value + thumbnail_url = self._get_thumbnail_url("/api/v1/chart/") + assert self.digest_hash in thumbnail_url + assert mock_adjust_string.call_args[0][1] == ExecutorType.SELENIUM + assert mock_adjust_string.call_args[0][2] == "admin" + + rv = self.client.get(thumbnail_url) self.assertEqual(rv.status_code, 202) - mock_find_user.assert_called_with("admin") + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_async_chart_screenshot_as_initiator(self): """ Thumbnails: Simple get async chart screenshot as initiator """ - chart = db.session.query(Slice).all()[0] username = "alpha" self.login(username=username) - with patch( - "superset.thumbnails.tasks.cache_chart_thumbnail.delay" - ) as mock_task, patch( - "superset.thumbnails.digest.get_initiator" - ) as mock_initiator, patch.dict( + with patch.dict( "superset.thumbnails.digest.current_app.config", { "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], }, - ): - mock_initiator.return_value = username - uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" - rv = self.client.get(uri) + ), patch( + "superset.thumbnails.digest._adjust_string_for_executor" + ) as mock_adjust_string: + mock_adjust_string.return_value = self.digest_return_value + thumbnail_url = self._get_thumbnail_url("/api/v1/chart/") + assert self.digest_hash in thumbnail_url + assert mock_adjust_string.call_args[0][1] == ExecutorType.INITIATOR + assert mock_adjust_string.call_args[0][2] == username + + rv = self.client.get(thumbnail_url) self.assertEqual(rv.status_code, 202) - mock_task.assert_called_with( - initiator=username, chart_id=chart.id, force=True - ) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_async_chart_notfound(self): """ @@ -268,6 +288,7 @@ def test_get_async_chart_notfound(self): rv = self.client.get(uri) self.assertEqual(rv.status_code, 404) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_cached_chart_wrong_digest(self): """ @@ -285,6 +306,7 @@ def test_get_cached_chart_wrong_digest(self): rv, f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" ) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_cached_dashboard_screenshot(self): """ @@ -300,6 +322,7 @@ def test_get_cached_dashboard_screenshot(self): self.assertEqual(rv.status_code, 200) self.assertEqual(rv.data, self.mock_image) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_cached_chart_screenshot(self): """ @@ -315,6 +338,7 @@ def test_get_cached_chart_screenshot(self): self.assertEqual(rv.status_code, 200) self.assertEqual(rv.data, self.mock_image) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) def test_get_cached_dashboard_wrong_digest(self): """ From dc6246b6df7f6cad4233eaa5f8eacf0031aeff45 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 7 Dec 2022 11:00:51 +0200 Subject: [PATCH 13/20] lint --- superset/charts/api.py | 2 +- superset/dashboards/api.py | 2 +- superset/models/dashboard.py | 2 +- superset/models/slice.py | 2 +- superset/thumbnails/digest.py | 3 +-- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/superset/charts/api.py b/superset/charts/api.py index b2ced44624ff0..2abfbcb6d0f9b 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -74,8 +74,8 @@ from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.extensions import event_logger from superset.models.slice import Slice -from superset.thumbnails.tasks import cache_chart_thumbnail from superset.tasks.utils import get_initiator +from superset.thumbnails.tasks import cache_chart_thumbnail from superset.utils.screenshots import ChartScreenshot from superset.utils.urls import get_url_path from superset.views.base_api import ( diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 59adcb9f2f723..1b36ed63c31f5 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -82,8 +82,8 @@ from superset.extensions import event_logger from superset.models.dashboard import Dashboard from superset.models.embedded_dashboard import EmbeddedDashboard -from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.tasks.utils import get_initiator +from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.utils.cache import etag_cache from superset.utils.screenshots import DashboardScreenshot from superset.utils.urls import get_url_path diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 657d13cc7d5ad..1a6fe03d75306 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -54,9 +54,9 @@ from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.models.slice import Slice from superset.models.user_attributes import UserAttribute +from superset.tasks.utils import get_initiator from superset.thumbnails.digest import get_dashboard_digest from superset.thumbnails.tasks import cache_dashboard_thumbnail -from superset.tasks.utils import get_initiator from superset.utils import core as utils from superset.utils.core import get_user_id from superset.utils.decorators import debounce diff --git a/superset/models/slice.py b/superset/models/slice.py index ca0b2332f243e..a4eed0cdd7532 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -42,9 +42,9 @@ from superset import db, is_feature_enabled, security_manager from superset.legacy import update_time_range from superset.models.helpers import AuditMixinNullable, ImportExportMixin +from superset.tasks.utils import get_initiator from superset.thumbnails.digest import get_chart_digest from superset.thumbnails.tasks import cache_chart_thumbnail -from superset.tasks.utils import get_initiator from superset.utils import core as utils from superset.utils.memoized import memoized from superset.viz import BaseViz, viz_types diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py index f14cbeda4aabd..7152c344408f2 100644 --- a/superset/thumbnails/digest.py +++ b/superset/thumbnails/digest.py @@ -23,8 +23,7 @@ from flask import current_app from superset.tasks.types import ExecutorType -from superset.tasks.utils import get_executor -from superset.tasks.utils import get_initiator +from superset.tasks.utils import get_executor, get_initiator from superset.utils.hashing import md5_sha_from_str if TYPE_CHECKING: From 28cc56e7f25a627dc9824b60827ba27c642f96b8 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 7 Dec 2022 13:19:16 +0200 Subject: [PATCH 14/20] address review comments --- docs/docs/installation/cache.mdx | 2 +- superset/charts/api.py | 10 +-- superset/config.py | 2 +- superset/dashboards/api.py | 8 +- superset/models/dashboard.py | 7 +- superset/models/slice.py | 7 +- superset/tasks/types.py | 4 +- superset/tasks/utils.py | 16 ++-- superset/thumbnails/digest.py | 16 ++-- superset/thumbnails/tasks.py | 8 +- .../integration_tests/reports/alert_tests.py | 2 +- tests/integration_tests/thumbnails_tests.py | 16 ++-- tests/unit_tests/tasks/test_utils.py | 24 +++--- tests/unit_tests/thumbnails/test_digest.py | 75 ++++++------------- 14 files changed, 81 insertions(+), 116 deletions(-) diff --git a/docs/docs/installation/cache.mdx b/docs/docs/installation/cache.mdx index 9340ad03df648..9b00121a7bee3 100644 --- a/docs/docs/installation/cache.mdx +++ b/docs/docs/installation/cache.mdx @@ -57,7 +57,7 @@ By default thumbnails are rendered using the `THUMBNAIL_SELENIUM_USER` user acco logged in user (e.g. in environments that are using user impersonation), use the following configuration: ```python -THUMBNAIL_EXECUTE_AS = [ExecutorType.INITIATOR] +THUMBNAIL_EXECUTE_AS = [ExecutorType.CURRENT_USER] ``` For this feature you will need a cache system and celery workers. All thumbnails are stored on cache diff --git a/superset/charts/api.py b/superset/charts/api.py index 2abfbcb6d0f9b..bc5fc2f0a0874 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -74,7 +74,7 @@ from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.extensions import event_logger from superset.models.slice import Slice -from superset.tasks.utils import get_initiator +from superset.tasks.utils import get_current_user from superset.thumbnails.tasks import cache_chart_thumbnail from superset.utils.screenshots import ChartScreenshot from superset.utils.urls import get_url_path @@ -572,7 +572,7 @@ def cache_screenshot(self, pk: int, **kwargs: Any) -> WerkzeugResponse: def trigger_celery() -> WerkzeugResponse: logger.info("Triggering screenshot ASYNC") cache_chart_thumbnail.delay( - initiator=get_initiator(), + current_user=get_current_user(), chart_id=chart.id, force=True, window_size=window_size, @@ -684,14 +684,14 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: if not chart: return self.response_404() - initiator = get_initiator() + current_user = get_current_user() url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") if kwargs["rison"].get("force", False): logger.info( "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) ) cache_chart_thumbnail.delay( - initiator=initiator, + current_user=current_user, chart_id=chart.id, force=True, ) @@ -707,7 +707,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) ) cache_chart_thumbnail.delay( - initiator=initiator, + current_user=current_user, chart_id=chart.id, force=True, ) diff --git a/superset/config.py b/superset/config.py index 22494160dab89..1228afa0438b2 100644 --- a/superset/config.py +++ b/superset/config.py @@ -589,7 +589,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # define which user to execute the thumbnails and potentially custom functions for # calculating thumbnail digests. To have unique thumbnails for all users, use the # following config: -# THUMBNAIL_EXECUTE_AS = [ExecutorType.INITIATOR] +# THUMBNAIL_EXECUTE_AS = [ExecutorType.CURRENT_USER] THUMBNAIL_EXECUTE_AS = [ExecutorType.SELENIUM] # By default, thumbnail digests are calculated based on various parameters in the diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 1b36ed63c31f5..71c85d4688af1 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -82,7 +82,7 @@ from superset.extensions import event_logger from superset.models.dashboard import Dashboard from superset.models.embedded_dashboard import EmbeddedDashboard -from superset.tasks.utils import get_initiator +from superset.tasks.utils import get_current_user from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.utils.cache import etag_cache from superset.utils.screenshots import DashboardScreenshot @@ -888,10 +888,10 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: "Superset.dashboard", dashboard_id_or_slug=dashboard.id ) # If force, request a screenshot from the workers - initiator = get_initiator() + current_user = get_current_user() if kwargs["rison"].get("force", False): cache_dashboard_thumbnail.delay( - initiator=initiator, + current_user=current_user, dashboard_id=dashboard.id, force=True, ) @@ -904,7 +904,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: if not screenshot: self.incr_stats("async", self.thumbnail.__name__) cache_dashboard_thumbnail.delay( - initiator=initiator, + current_user=current_user, dashboard_id=dashboard.id, force=True, ) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 1a6fe03d75306..5e6af2de61bd9 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -54,7 +54,7 @@ from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.models.slice import Slice from superset.models.user_attributes import UserAttribute -from superset.tasks.utils import get_initiator +from superset.tasks.utils import get_current_user from superset.thumbnails.digest import get_dashboard_digest from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.utils import core as utils @@ -326,7 +326,7 @@ def position(self) -> Dict[str, Any]: def update_thumbnail(self) -> None: cache_dashboard_thumbnail.delay( - initiator=get_initiator(), + current_user=get_current_user(), dashboard_id=self.id, force=True, ) @@ -438,8 +438,7 @@ def export_dashboards( # pylint: disable=too-many-locals @classmethod def get(cls, id_or_slug: Union[str, int]) -> Dashboard: - session = db.session() - qry = session.query(Dashboard).filter(id_or_slug_filter(id_or_slug)) + qry = db.session.query(Dashboard).filter(id_or_slug_filter(id_or_slug)) return qry.one_or_none() diff --git a/superset/models/slice.py b/superset/models/slice.py index a4eed0cdd7532..6571f9b2c9d86 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -42,7 +42,7 @@ from superset import db, is_feature_enabled, security_manager from superset.legacy import update_time_range from superset.models.helpers import AuditMixinNullable, ImportExportMixin -from superset.tasks.utils import get_initiator +from superset.tasks.utils import get_current_user from superset.thumbnails.digest import get_chart_digest from superset.thumbnails.tasks import cache_chart_thumbnail from superset.utils import core as utils @@ -343,8 +343,7 @@ def get_query_context_factory(self) -> QueryContextFactory: @classmethod def get(cls, id_: int) -> Slice: - session = db.session() - qry = session.query(Slice).filter_by(id=id_) + qry = db.session.query(Slice).filter_by(id=id_) return qry.one_or_none() @@ -362,7 +361,7 @@ def event_after_chart_changed( _mapper: Mapper, _connection: Connection, target: Slice ) -> None: cache_chart_thumbnail.delay( - initiator=get_initiator(), + current_user=get_current_user(), chart_id=target.id, force=True, ) diff --git a/superset/tasks/types.py b/superset/tasks/types.py index 4babc53daed04..cc337a81edb6f 100644 --- a/superset/tasks/types.py +++ b/superset/tasks/types.py @@ -31,9 +31,9 @@ class ExecutorType(str, Enum): CREATOR = "creator" # The creator of the model, if found in the owners list CREATOR_OWNER = "creator_owner" - # The initiator of the request. In the case of Alerts & Reports, this is always + # The currently logged in user. In the case of Alerts & Reports, this is always # None. For Thumbnails, this is the user that requested the thumbnail - INITIATOR = "initiator" + CURRENT_USER = "current_user" # The last modifier of the model MODIFIER = "modifier" # The last modifier of the model, if found in the owners list diff --git a/superset/tasks/utils.py b/superset/tasks/utils.py index 79fbe3a315282..9c1dab82202b8 100644 --- a/superset/tasks/utils.py +++ b/superset/tasks/utils.py @@ -34,7 +34,7 @@ def get_executor( executor_types: List[ExecutorType], model: Union[Dashboard, ReportSchedule, Slice], - initiator: Optional[str] = None, + current_user: Optional[str] = None, ) -> Tuple[ExecutorType, str]: """ Extract the user that should be used to execute a scheduled task. Certain executor @@ -44,9 +44,9 @@ def get_executor( :param executor_types: The requested executor type in descending order. When the first user is found it is returned. :param model: The underlying object - :param initiator: The username of the user that initiated the task. For thumbnails - this is the user that requested the thumbnail, while for alerts and - reports this is None (=initiated by Celery). + :param current_user: The username of the user that initiated the task. For + thumbnails this is the user that requested the thumbnail, while for alerts + and reports this is None (=initiated by Celery). :return: User to execute the report as :raises ScheduledTaskExecutorNotFoundError: If no users were found in after iterating through all entries in `executor_types` @@ -56,8 +56,8 @@ def get_executor( for executor_type in executor_types: if executor_type == ExecutorType.SELENIUM: return executor_type, current_app.config["THUMBNAIL_SELENIUM_USER"] - if executor_type == ExecutorType.INITIATOR and initiator: - return executor_type, initiator + if executor_type == ExecutorType.CURRENT_USER and current_user: + return executor_type, current_user if executor_type == ExecutorType.CREATOR_OWNER: if (user := model.created_by) and (owner := owner_dict.get(user.id)): return executor_type, owner.username @@ -86,8 +86,8 @@ def get_executor( raise ExecutorNotFoundError() -def get_initiator() -> Optional[str]: - user = g.user if hasattr(g, "user") and g.user and g.user else None +def get_current_user() -> Optional[str]: + user = g.user if hasattr(g, "user") and g.user else None if user and not user.is_anonymous: return user.username diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py index 7152c344408f2..077ab89251c2a 100644 --- a/superset/thumbnails/digest.py +++ b/superset/thumbnails/digest.py @@ -23,7 +23,7 @@ from flask import current_app from superset.tasks.types import ExecutorType -from superset.tasks.utils import get_executor, get_initiator +from superset.tasks.utils import get_current_user, get_executor from superset.utils.hashing import md5_sha_from_str if TYPE_CHECKING: @@ -39,14 +39,12 @@ def _adjust_string_for_executor( executor: str, ) -> str: """ - Add the secret key and executor to the unique string if the thumbnail is + Add the executor to the unique string if the thumbnail is user-specific. """ - if executor_type == ExecutorType.INITIATOR: - secret_key = current_app.config["SECRET_KEY"] - # the digest for the user=specific thumbnail needs to be unguessable and unique, - # hence the secret key and username are appended to the unique string - unique_string = f"{secret_key}\n{unique_string}\n{executor}" + if executor_type == ExecutorType.CURRENT_USER: + # add the user id to the string to make it unique + unique_string = f"{unique_string}\n{executor}" return unique_string @@ -56,7 +54,7 @@ def get_dashboard_digest(dashboard: Dashboard) -> str: executor_type, executor = get_executor( executor_types=config["THUMBNAIL_EXECUTE_AS"], model=dashboard, - initiator=get_initiator(), + current_user=get_current_user(), ) if func := config["THUMBNAIL_DASHBOARD_DIGEST_FUNC"]: return func(dashboard, executor_type, executor) @@ -75,7 +73,7 @@ def get_chart_digest(chart: Slice) -> str: executor_type, executor = get_executor( executor_types=config["THUMBNAIL_EXECUTE_AS"], model=chart, - initiator=get_initiator(), + current_user=get_current_user(), ) if func := config["THUMBNAIL_CHART_DIGEST_FUNC"]: return func(chart, executor_type, executor) diff --git a/superset/thumbnails/tasks.py b/superset/thumbnails/tasks.py index d340bdb8bda7b..c03d13b0bdbeb 100644 --- a/superset/thumbnails/tasks.py +++ b/superset/thumbnails/tasks.py @@ -35,7 +35,7 @@ @celery_app.task(name="cache_chart_thumbnail", soft_time_limit=300) def cache_chart_thumbnail( - initiator: Optional[str], + current_user: Optional[str], chart_id: int, force: bool = False, window_size: Optional[WindowSize] = None, @@ -53,7 +53,7 @@ def cache_chart_thumbnail( _, username = get_executor( executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], model=chart, - initiator=initiator, + current_user=current_user, ) user = security_manager.find_user(username) with override_user(user): @@ -70,7 +70,7 @@ def cache_chart_thumbnail( @celery_app.task(name="cache_dashboard_thumbnail", soft_time_limit=300) def cache_dashboard_thumbnail( - initiator: Optional[str], + current_user: Optional[str], dashboard_id: int, force: bool = False, thumb_size: Optional[WindowSize] = None, @@ -88,7 +88,7 @@ def cache_dashboard_thumbnail( _, username = get_executor( executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], model=dashboard, - initiator=initiator, + current_user=current_user, ) user = security_manager.find_user(username) with override_user(user): diff --git a/tests/integration_tests/reports/alert_tests.py b/tests/integration_tests/reports/alert_tests.py index 502cdbff04367..822dc246d0496 100644 --- a/tests/integration_tests/reports/alert_tests.py +++ b/tests/integration_tests/reports/alert_tests.py @@ -52,7 +52,7 @@ [ExecutorType.CREATOR_OWNER], AlertQueryError(), ), - (["gamma"], None, [ExecutorType.INITIATOR], AlertQueryError()), + (["gamma"], None, [ExecutorType.CURRENT_USER], AlertQueryError()), ], ) def test_execute_query_as_report_executor( diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index 76b285c1d00b3..c7096aff29002 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -185,16 +185,16 @@ def test_get_async_dashboard_screenshot_as_selenium(self): @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) - def test_get_async_dashboard_screenshot_as_initiator(self): + def test_get_async_dashboard_screenshot_as_current_user(self): """ - Thumbnails: Simple get async dashboard screenshot as initiator + Thumbnails: Simple get async dashboard screenshot as current user """ username = "alpha" self.login(username=username) with patch.dict( "superset.thumbnails.digest.current_app.config", { - "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], + "THUMBNAIL_EXECUTE_AS": [ExecutorType.CURRENT_USER], }, ), patch( "superset.thumbnails.digest._adjust_string_for_executor" @@ -202,7 +202,7 @@ def test_get_async_dashboard_screenshot_as_initiator(self): mock_adjust_string.return_value = self.digest_return_value thumbnail_url = self._get_thumbnail_url("/api/v1/dashboard/") assert self.digest_hash in thumbnail_url - assert mock_adjust_string.call_args[0][1] == ExecutorType.INITIATOR + assert mock_adjust_string.call_args[0][1] == ExecutorType.CURRENT_USER assert mock_adjust_string.call_args[0][2] == username rv = self.client.get(thumbnail_url) @@ -253,16 +253,16 @@ def test_get_async_chart_screenshot_as_selenium(self): @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) - def test_get_async_chart_screenshot_as_initiator(self): + def test_get_async_chart_screenshot_as_current_user(self): """ - Thumbnails: Simple get async chart screenshot as initiator + Thumbnails: Simple get async chart screenshot as current user """ username = "alpha" self.login(username=username) with patch.dict( "superset.thumbnails.digest.current_app.config", { - "THUMBNAIL_EXECUTE_AS": [ExecutorType.INITIATOR], + "THUMBNAIL_EXECUTE_AS": [ExecutorType.CURRENT_USER], }, ), patch( "superset.thumbnails.digest._adjust_string_for_executor" @@ -270,7 +270,7 @@ def test_get_async_chart_screenshot_as_initiator(self): mock_adjust_string.return_value = self.digest_return_value thumbnail_url = self._get_thumbnail_url("/api/v1/chart/") assert self.digest_hash in thumbnail_url - assert mock_adjust_string.call_args[0][1] == ExecutorType.INITIATOR + assert mock_adjust_string.call_args[0][1] == ExecutorType.CURRENT_USER assert mock_adjust_string.call_args[0][2] == username rv = self.client.get(thumbnail_url) diff --git a/tests/unit_tests/tasks/test_utils.py b/tests/unit_tests/tasks/test_utils.py index 1b7352426d433..7854717201229 100644 --- a/tests/unit_tests/tasks/test_utils.py +++ b/tests/unit_tests/tasks/test_utils.py @@ -54,7 +54,7 @@ class ModelType(int, Enum): @pytest.mark.parametrize( - "model_type,executor_types,model_config,initiator,expected_result", + "model_type,executor_types,model_config,current_user,expected_result", [ ( ModelType.REPORT_SCHEDULE, @@ -180,7 +180,7 @@ class ModelType(int, Enum): ( ModelType.REPORT_SCHEDULE, [ - ExecutorType.INITIATOR, + ExecutorType.CURRENT_USER, ], ModelConfig(owners=[1, 2, 3, 4, 5, 6, 7], creator=4, modifier=8), None, @@ -189,11 +189,11 @@ class ModelType(int, Enum): ( ModelType.DASHBOARD, [ - ExecutorType.INITIATOR, + ExecutorType.CURRENT_USER, ], ModelConfig(owners=[1], creator=2, modifier=3), 4, - (ExecutorType.INITIATOR, 4), + (ExecutorType.CURRENT_USER, 4), ), ( ModelType.DASHBOARD, @@ -207,7 +207,7 @@ class ModelType(int, Enum): ( ModelType.DASHBOARD, [ - ExecutorType.INITIATOR, + ExecutorType.CURRENT_USER, ], ModelConfig(owners=[1], creator=2, modifier=3), None, @@ -218,7 +218,7 @@ class ModelType(int, Enum): [ ExecutorType.CREATOR_OWNER, ExecutorType.MODIFIER_OWNER, - ExecutorType.INITIATOR, + ExecutorType.CURRENT_USER, ExecutorType.SELENIUM, ], ModelConfig(owners=[1], creator=2, modifier=3), @@ -228,11 +228,11 @@ class ModelType(int, Enum): ( ModelType.CHART, [ - ExecutorType.INITIATOR, + ExecutorType.CURRENT_USER, ], ModelConfig(owners=[1], creator=2, modifier=3), 4, - (ExecutorType.INITIATOR, 4), + (ExecutorType.CURRENT_USER, 4), ), ( ModelType.CHART, @@ -246,7 +246,7 @@ class ModelType(int, Enum): ( ModelType.CHART, [ - ExecutorType.INITIATOR, + ExecutorType.CURRENT_USER, ], ModelConfig(owners=[1], creator=2, modifier=3), None, @@ -257,7 +257,7 @@ class ModelType(int, Enum): [ ExecutorType.CREATOR_OWNER, ExecutorType.MODIFIER_OWNER, - ExecutorType.INITIATOR, + ExecutorType.CURRENT_USER, ExecutorType.SELENIUM, ], ModelConfig(owners=[1], creator=2, modifier=3), @@ -270,7 +270,7 @@ def test_get_executor( model_type: ModelType, executor_types: List[ExecutorType], model_config: ModelConfig, - initiator: Optional[int], + current_user: Optional[int], expected_result: Tuple[int, ExecutorNotFoundError], ) -> None: from superset.models.dashboard import Dashboard @@ -317,7 +317,7 @@ def test_get_executor( executor_type, executor = get_executor( executor_types=executor_types, model=obj, - initiator=str(initiator) if initiator else None, + current_user=str(current_user) if current_user else None, ) assert executor_type == expected_executor_type assert executor == expected_executor diff --git a/tests/unit_tests/thumbnails/test_digest.py b/tests/unit_tests/thumbnails/test_digest.py index d492d41f70b89..72a293f85950e 100644 --- a/tests/unit_tests/thumbnails/test_digest.py +++ b/tests/unit_tests/thumbnails/test_digest.py @@ -61,11 +61,10 @@ def CUSTOM_CHART_FUNC( @pytest.mark.parametrize( - "dashboard_overrides,secret_key,execute_as,has_initiator,use_custom_digest,expected_result", + "dashboard_overrides,execute_as,has_current_user,use_custom_digest,expected_result", [ ( None, - "my_secret", [ExecutorType.SELENIUM], False, False, @@ -73,62 +72,48 @@ def CUSTOM_CHART_FUNC( ), ( None, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], True, False, - "7c9bccb14f0bac455c5cd0181d9c61a1", - ), - ( - None, - "my_other_secret", - [ExecutorType.INITIATOR], - True, - False, - "3fe8f5fc9fa81c78d064e2b11ba4aaa8", + "55fa9f78f4d8c96464fd5b369a8f2367", ), ( { "position_json": {"b": "c"}, }, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], True, False, - "bf9401fd23f778c0026e9b614b81736f", + "9725aa2717974238f03c3fc29bef243b", ), ( { "css": "background-color: darkblue;", }, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], True, False, - "8752d66b796f8e9f81a083b681bdefe9", + "234e168024483a520b705ecf71cf4fca", ), ( { "json_metadata": {"d": "e"}, }, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], True, False, - "62e1f5bff56c57614c31beb83d67c492", + "430dc5a4ab07928f4465c43a32b4c846", ), ( None, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], True, True, - "1.initiator.1", + "1.current_user.1", ), ( None, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], False, False, ExecutorNotFoundError(), @@ -137,9 +122,8 @@ def CUSTOM_CHART_FUNC( ) def test_dashboard_digest( dashboard_overrides: Optional[Dict[str, Any]], - secret_key: str, execute_as: List[ExecutorType], - has_initiator: bool, + has_current_user: bool, use_custom_digest: bool, expected_result: Union[str, Exception], ) -> None: @@ -153,14 +137,13 @@ def test_dashboard_digest( } dashboard = Dashboard(**kwargs) user: Optional[User] = None - if has_initiator: + if has_current_user: user = User(id=1, username="1") func = CUSTOM_DASHBOARD_FUNC if use_custom_digest else None with patch.dict( app.config, { - "SECRET_KEY": secret_key, "THUMBNAIL_EXECUTE_AS": execute_as, "THUMBNAIL_DASHBOARD_DIGEST_FUNC": func, }, @@ -175,11 +158,10 @@ def test_dashboard_digest( @pytest.mark.parametrize( - "chart_overrides,secret_key,execute_as,has_initiator,use_custom_digest,expected_result", + "chart_overrides,execute_as,has_current_user,use_custom_digest,expected_result", [ ( None, - "my_secret", [ExecutorType.SELENIUM], False, False, @@ -187,32 +169,21 @@ def test_dashboard_digest( ), ( None, - "my_secret", - [ExecutorType.INITIATOR], - True, - False, - "97b78018f0eac76156bb54d3051faed0", - ), - ( - None, - "my_other_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], True, False, - "9558cafa50981d550019c854f553b393", + "4f8109d3761e766e650af514bb358f10", ), ( None, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], True, True, - "2.initiator.1", + "2.current_user.1", ), ( None, - "my_secret", - [ExecutorType.INITIATOR], + [ExecutorType.CURRENT_USER], False, False, ExecutorNotFoundError(), @@ -221,9 +192,8 @@ def test_dashboard_digest( ) def test_chart_digest( chart_overrides: Optional[Dict[str, Any]], - secret_key: str, execute_as: List[ExecutorType], - has_initiator: bool, + has_current_user: bool, use_custom_digest: bool, expected_result: Union[str, Exception], ) -> None: @@ -237,14 +207,13 @@ def test_chart_digest( } chart = Slice(**kwargs) user: Optional[User] = None - if has_initiator: + if has_current_user: user = User(id=1, username="1") func = CUSTOM_CHART_FUNC if use_custom_digest else None with patch.dict( app.config, { - "SECRET_KEY": secret_key, "THUMBNAIL_EXECUTE_AS": execute_as, "THUMBNAIL_CHART_DIGEST_FUNC": func, }, From 94c5e3f566e7318a6d158634c6ae74b5fae3f35a Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Thu, 8 Dec 2022 09:33:51 +0200 Subject: [PATCH 15/20] streamline tests --- tests/integration_tests/thumbnails_tests.py | 62 ++++++++++----------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index c7096aff29002..4fc1dc5562ec2 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -20,6 +20,7 @@ import json import urllib.request from io import BytesIO +from typing import Tuple from unittest import skipUnless from unittest.mock import ANY, call, patch @@ -44,6 +45,9 @@ from .base_tests import SupersetTestCase +CHART_URL = "/api/v1/chart/" +DASHBOARD_URL = "/api/v1/dashboard/" + class TestThumbnailsSeleniumLive(LiveServerTestCase): def create_app(self): @@ -62,7 +66,7 @@ def test_get_async_dashboard_screenshot(self): Thumbnails: Simple get async dashboard screenshot """ with patch("superset.dashboards.api.DashboardRestApi.get") as mock_get: - rv = self.client.get("/api/v1/dashboard/") + rv = self.client.get(DASHBOARD_URL) resp = json.loads(rv.data.decode("utf-8")) thumbnail_url = resp["result"][0]["thumbnail_url"] @@ -135,10 +139,11 @@ class TestThumbnails(SupersetTestCase): digest_return_value = "foo_bar" digest_hash = "5c7d96a3dd7a87850a2ef34087565a6e" - def _get_thumbnail_url(self, url: str): + def _get_id_and_thumbnail_url(self, url: str) -> Tuple[int, str]: rv = self.client.get(url) resp = json.loads(rv.data.decode("utf-8")) - return resp["result"][0]["thumbnail_url"] + obj = resp["result"][0] + return obj["id"], obj["thumbnail_url"] @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=False) @@ -146,10 +151,9 @@ def test_dashboard_thumbnail_disabled(self): """ Thumbnails: Dashboard thumbnail disabled """ - dashboard = db.session.query(Dashboard).all()[0] self.login(username="admin") - uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" - rv = self.client.get(uri) + _, thumbnail_url = self._get_id_and_thumbnail_url(DASHBOARD_URL) + rv = self.client.get(thumbnail_url) self.assertEqual(rv.status_code, 404) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @@ -158,10 +162,9 @@ def test_chart_thumbnail_disabled(self): """ Thumbnails: Chart thumbnail disabled """ - chart = db.session.query(Slice).all()[0] self.login(username="admin") - uri = f"api/v1/chart/{chart}/thumbnail/{chart.digest}/" - rv = self.client.get(uri) + _, thumbnail_url = self._get_id_and_thumbnail_url(CHART_URL) + rv = self.client.get(thumbnail_url) self.assertEqual(rv.status_code, 404) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @@ -175,7 +178,7 @@ def test_get_async_dashboard_screenshot_as_selenium(self): "superset.thumbnails.digest._adjust_string_for_executor" ) as mock_adjust_string: mock_adjust_string.return_value = self.digest_return_value - thumbnail_url = self._get_thumbnail_url("/api/v1/dashboard/") + _, thumbnail_url = self._get_id_and_thumbnail_url(DASHBOARD_URL) assert self.digest_hash in thumbnail_url assert mock_adjust_string.call_args[0][1] == ExecutorType.SELENIUM assert mock_adjust_string.call_args[0][2] == "admin" @@ -200,7 +203,7 @@ def test_get_async_dashboard_screenshot_as_current_user(self): "superset.thumbnails.digest._adjust_string_for_executor" ) as mock_adjust_string: mock_adjust_string.return_value = self.digest_return_value - thumbnail_url = self._get_thumbnail_url("/api/v1/dashboard/") + _, thumbnail_url = self._get_id_and_thumbnail_url(DASHBOARD_URL) assert self.digest_hash in thumbnail_url assert mock_adjust_string.call_args[0][1] == ExecutorType.CURRENT_USER assert mock_adjust_string.call_args[0][2] == username @@ -226,10 +229,9 @@ def test_get_async_dashboard_not_allowed(self): """ Thumbnails: Simple get async dashboard not allowed """ - dashboard = db.session.query(Dashboard).all()[0] self.login(username="gamma") - uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" - rv = self.client.get(uri) + _, thumbnail_url = self._get_id_and_thumbnail_url(DASHBOARD_URL) + rv = self.client.get(thumbnail_url) self.assertEqual(rv.status_code, 404) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @@ -243,7 +245,7 @@ def test_get_async_chart_screenshot_as_selenium(self): "superset.thumbnails.digest._adjust_string_for_executor" ) as mock_adjust_string: mock_adjust_string.return_value = self.digest_return_value - thumbnail_url = self._get_thumbnail_url("/api/v1/chart/") + _, thumbnail_url = self._get_id_and_thumbnail_url(CHART_URL) assert self.digest_hash in thumbnail_url assert mock_adjust_string.call_args[0][1] == ExecutorType.SELENIUM assert mock_adjust_string.call_args[0][2] == "admin" @@ -268,7 +270,7 @@ def test_get_async_chart_screenshot_as_current_user(self): "superset.thumbnails.digest._adjust_string_for_executor" ) as mock_adjust_string: mock_adjust_string.return_value = self.digest_return_value - thumbnail_url = self._get_thumbnail_url("/api/v1/chart/") + _, thumbnail_url = self._get_id_and_thumbnail_url(CHART_URL) assert self.digest_hash in thumbnail_url assert mock_adjust_string.call_args[0][1] == ExecutorType.CURRENT_USER assert mock_adjust_string.call_args[0][2] == username @@ -294,17 +296,14 @@ def test_get_cached_chart_wrong_digest(self): """ Thumbnails: Simple get chart with wrong digest """ - chart = db.session.query(Slice).all()[0] with patch.object( ChartScreenshot, "get_from_cache", return_value=BytesIO(self.mock_image) ): self.login(username="admin") - uri = f"api/v1/chart/{chart.id}/thumbnail/1234/" - rv = self.client.get(uri) + id_, thumbnail_url = self._get_id_and_thumbnail_url(CHART_URL) + rv = self.client.get(f"api/v1/chart/{id_}/thumbnail/1234/") self.assertEqual(rv.status_code, 302) - self.assertRedirects( - rv, f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" - ) + self.assertRedirects(rv, thumbnail_url) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") @with_feature_flags(THUMBNAILS=True) @@ -312,13 +311,12 @@ def test_get_cached_dashboard_screenshot(self): """ Thumbnails: Simple get cached dashboard screenshot """ - dashboard = db.session.query(Dashboard).all()[0] with patch.object( DashboardScreenshot, "get_from_cache", return_value=BytesIO(self.mock_image) ): self.login(username="admin") - uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" - rv = self.client.get(uri) + _, thumbnail_url = self._get_id_and_thumbnail_url(DASHBOARD_URL) + rv = self.client.get(thumbnail_url) self.assertEqual(rv.status_code, 200) self.assertEqual(rv.data, self.mock_image) @@ -328,13 +326,12 @@ def test_get_cached_chart_screenshot(self): """ Thumbnails: Simple get cached chart screenshot """ - chart = db.session.query(Slice).all()[0] with patch.object( ChartScreenshot, "get_from_cache", return_value=BytesIO(self.mock_image) ): self.login(username="admin") - uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/" - rv = self.client.get(uri) + id_, thumbnail_url = self._get_id_and_thumbnail_url(CHART_URL) + rv = self.client.get(thumbnail_url) self.assertEqual(rv.status_code, 200) self.assertEqual(rv.data, self.mock_image) @@ -344,14 +341,11 @@ def test_get_cached_dashboard_wrong_digest(self): """ Thumbnails: Simple get dashboard with wrong digest """ - dashboard = db.session.query(Dashboard).all()[0] with patch.object( DashboardScreenshot, "get_from_cache", return_value=BytesIO(self.mock_image) ): self.login(username="admin") - uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/1234/" - rv = self.client.get(uri) + id_, thumbnail_url = self._get_id_and_thumbnail_url(DASHBOARD_URL) + rv = self.client.get(f"api/v1/dashboard/{id_}/thumbnail/1234/") self.assertEqual(rv.status_code, 302) - self.assertRedirects( - rv, f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/" - ) + self.assertRedirects(rv, thumbnail_url) From 5d2ce4e8d7fb4d63e85da3b6c41115ede96d0133 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Thu, 8 Dec 2022 21:00:36 +0200 Subject: [PATCH 16/20] fix config descriptions --- superset/config.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/superset/config.py b/superset/config.py index 1228afa0438b2..93e3961bc5968 100644 --- a/superset/config.py +++ b/superset/config.py @@ -577,13 +577,12 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # This is merely a default EXTRA_SEQUENTIAL_COLOR_SCHEMES: List[Dict[str, Any]] = [] -# When executing Alerts & Reports or Thumbnails as the Selenium user, this defines -# the username of the account used to render the queries and dashboards/charts -THUMBNAIL_SELENIUM_USER: Optional[str] = "admin" - # --------------------------------------------------- # Thumbnail config (behind feature flag) # --------------------------------------------------- +# When executing Alerts & Reports or Thumbnails as the Selenium user, this defines +# the username of the account used to render the queries and dashboards/charts +THUMBNAIL_SELENIUM_USER: Optional[str] = "admin" # To be able to have different thumbnails for different users, use these configs to # define which user to execute the thumbnails and potentially custom functions for @@ -594,11 +593,13 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # By default, thumbnail digests are calculated based on various parameters in the # chart/dashboard metadata, and in the case of user-specific thumbnails, the -# SECRET_KEY and the username. To specify a custom digest function, use the following -# config parameters to define callbacks that receive +# username. To specify a custom digest function, use the following config parameters +# to define callbacks that receive # 1. the model (dashboard or chart) -# 2. the executor type -# 3. the executor's username +# 2. the executor type (e.g. ExecutorType.SELENIUM) +# 3. the executor's username (note, this is the executor as defined by +# `THUMBNAIL_EXECUTE_AS`; the executor is only equal to the currently logged in +# user if the executor type is equal to `ExecutorType.CURRENT_USER`) # and return the final digest string: THUMBNAIL_DASHBOARD_DIGEST_FUNC: Optional[ Callable[[Dashboard, ExecutorType, str], str] From fdc2698ba1a40082198bdb4f041740cea08818d5 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Fri, 9 Dec 2022 15:51:54 +0200 Subject: [PATCH 17/20] move tasks back and fix digest func --- docs/docs/installation/alerts-reports.mdx | 2 +- docs/docs/installation/cache.mdx | 2 +- .../installation/running-on-kubernetes.mdx | 2 +- superset/charts/api.py | 2 +- superset/cli/thumbnails.py | 2 +- superset/dashboards/api.py | 2 +- superset/models/dashboard.py | 2 +- superset/models/slice.py | 2 +- superset/tasks/thumbnails.py | 86 +++++++++++++-- superset/thumbnails/digest.py | 3 +- superset/thumbnails/tasks.py | 101 ------------------ .../superset_test_config_thumbnails.py | 2 +- tests/unit_tests/thumbnails/test_digest.py | 10 +- 13 files changed, 94 insertions(+), 124 deletions(-) delete mode 100644 superset/thumbnails/tasks.py diff --git a/docs/docs/installation/alerts-reports.mdx b/docs/docs/installation/alerts-reports.mdx index 5551641392e67..3538ca1479e42 100644 --- a/docs/docs/installation/alerts-reports.mdx +++ b/docs/docs/installation/alerts-reports.mdx @@ -90,7 +90,7 @@ REDIS_PORT = "6379" class CeleryConfig: broker_url = 'redis://%s:%s/0' % (REDIS_HOST, REDIS_PORT) - imports = ('superset.sql_lab', "superset.tasks", "superset.thumbnails.tasks", ) + imports = ('superset.sql_lab', "superset.tasks", "superset.tasks.thumbnails", ) result_backend = 'redis://%s:%s/0' % (REDIS_HOST, REDIS_PORT) worker_prefetch_multiplier = 10 task_acks_late = True diff --git a/docs/docs/installation/cache.mdx b/docs/docs/installation/cache.mdx index 9b00121a7bee3..4838fc47e61c1 100644 --- a/docs/docs/installation/cache.mdx +++ b/docs/docs/installation/cache.mdx @@ -73,7 +73,7 @@ from s3cache.s3cache import S3Cache class CeleryConfig(object): broker_url = "redis://localhost:6379/0" - imports = ("superset.sql_lab", "superset.tasks", "superset.thumbnails.tasks") + imports = ("superset.sql_lab", "superset.tasks", "superset.tasks.thumbnails") result_backend = "redis://localhost:6379/0" worker_prefetch_multiplier = 10 task_acks_late = True diff --git a/docs/docs/installation/running-on-kubernetes.mdx b/docs/docs/installation/running-on-kubernetes.mdx index e2c1f92613255..1b75e0f5c6101 100644 --- a/docs/docs/installation/running-on-kubernetes.mdx +++ b/docs/docs/installation/running-on-kubernetes.mdx @@ -345,7 +345,7 @@ configOverrides: class CeleryConfig(object): broker_url = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0" - imports = ('superset.sql_lab', "superset.tasks", "superset.thumbnails.tasks", ) + imports = ('superset.sql_lab', "superset.tasks", "superset.tasks.thumbnails", ) result_backend = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0" task_annotations = { 'sql_lab.get_sql_results': { diff --git a/superset/charts/api.py b/superset/charts/api.py index bc5fc2f0a0874..c5e0eb77d8013 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -74,8 +74,8 @@ from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.extensions import event_logger from superset.models.slice import Slice +from superset.tasks.thumbnails import cache_chart_thumbnail from superset.tasks.utils import get_current_user -from superset.thumbnails.tasks import cache_chart_thumbnail from superset.utils.screenshots import ChartScreenshot from superset.utils.urls import get_url_path from superset.views.base_api import ( diff --git a/superset/cli/thumbnails.py b/superset/cli/thumbnails.py index 46d1d126f08d3..5556cff92f620 100755 --- a/superset/cli/thumbnails.py +++ b/superset/cli/thumbnails.py @@ -69,7 +69,7 @@ def compute_thumbnails( # pylint: disable=import-outside-toplevel from superset.models.dashboard import Dashboard from superset.models.slice import Slice - from superset.thumbnails.tasks import ( + from superset.tasks.thumbnails import ( cache_chart_thumbnail, cache_dashboard_thumbnail, ) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 71c85d4688af1..79255d19211b2 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -82,8 +82,8 @@ from superset.extensions import event_logger from superset.models.dashboard import Dashboard from superset.models.embedded_dashboard import EmbeddedDashboard +from superset.tasks.thumbnails import cache_dashboard_thumbnail from superset.tasks.utils import get_current_user -from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.utils.cache import etag_cache from superset.utils.screenshots import DashboardScreenshot from superset.utils.urls import get_url_path diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 5e6af2de61bd9..ae6bae4b733ff 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -54,9 +54,9 @@ from superset.models.helpers import AuditMixinNullable, ImportExportMixin from superset.models.slice import Slice from superset.models.user_attributes import UserAttribute +from superset.tasks.thumbnails import cache_dashboard_thumbnail from superset.tasks.utils import get_current_user from superset.thumbnails.digest import get_dashboard_digest -from superset.thumbnails.tasks import cache_dashboard_thumbnail from superset.utils import core as utils from superset.utils.core import get_user_id from superset.utils.decorators import debounce diff --git a/superset/models/slice.py b/superset/models/slice.py index 6571f9b2c9d86..657ff7d38a079 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -42,9 +42,9 @@ from superset import db, is_feature_enabled, security_manager from superset.legacy import update_time_range from superset.models.helpers import AuditMixinNullable, ImportExportMixin +from superset.tasks.thumbnails import cache_chart_thumbnail from superset.tasks.utils import get_current_user from superset.thumbnails.digest import get_chart_digest -from superset.thumbnails.tasks import cache_chart_thumbnail from superset.utils import core as utils from superset.utils.memoized import memoized from superset.viz import BaseViz, viz_types diff --git a/superset/tasks/thumbnails.py b/superset/tasks/thumbnails.py index 247d865001be6..c03d13b0bdbeb 100644 --- a/superset/tasks/thumbnails.py +++ b/superset/tasks/thumbnails.py @@ -15,15 +15,87 @@ # specific language governing permissions and limitations # under the License. +"""Utility functions used across Superset""" + import logging +from typing import cast, Optional + +from flask import current_app -# TODO(villebro): deprecate in 3.0 -# pylint: disable=unused-wildcard-import,wildcard-import -from superset.thumbnails.tasks import * +from superset import security_manager, thumbnail_cache +from superset.extensions import celery_app +from superset.tasks.utils import get_executor +from superset.utils.core import override_user +from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot +from superset.utils.urls import get_url_path +from superset.utils.webdriver import WindowSize logger = logging.getLogger(__name__) -logger.warning( - "The import path superset.tasks.thumbnails is deprecated and will be removed in" - "3.0. Please use superset.thumbnails.tasks instead." -) + +@celery_app.task(name="cache_chart_thumbnail", soft_time_limit=300) +def cache_chart_thumbnail( + current_user: Optional[str], + chart_id: int, + force: bool = False, + window_size: Optional[WindowSize] = None, + thumb_size: Optional[WindowSize] = None, +) -> None: + # pylint: disable=import-outside-toplevel + from superset.models.slice import Slice + + if not thumbnail_cache: + logger.warning("No cache set, refusing to compute") + return None + chart = cast(Slice, Slice.get(chart_id)) + url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") + logger.info("Caching chart: %s", url) + _, username = get_executor( + executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], + model=chart, + current_user=current_user, + ) + user = security_manager.find_user(username) + with override_user(user): + screenshot = ChartScreenshot(url, chart.digest) + screenshot.compute_and_cache( + user=user, + cache=thumbnail_cache, + force=force, + window_size=window_size, + thumb_size=thumb_size, + ) + return None + + +@celery_app.task(name="cache_dashboard_thumbnail", soft_time_limit=300) +def cache_dashboard_thumbnail( + current_user: Optional[str], + dashboard_id: int, + force: bool = False, + thumb_size: Optional[WindowSize] = None, +) -> None: + # pylint: disable=import-outside-toplevel + from superset.models.dashboard import Dashboard + + if not thumbnail_cache: + logging.warning("No cache set, refusing to compute") + return + dashboard = Dashboard.get(dashboard_id) + url = get_url_path("Superset.dashboard", dashboard_id_or_slug=dashboard.id) + + logger.info("Caching dashboard: %s", url) + _, username = get_executor( + executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], + model=dashboard, + current_user=current_user, + ) + user = security_manager.find_user(username) + with override_user(user): + screenshot = DashboardScreenshot(url, dashboard.digest) + screenshot.compute_and_cache( + user=user, + cache=thumbnail_cache, + force=force, + thumb_size=thumb_size, + ) diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py index 077ab89251c2a..543983e21bd90 100644 --- a/superset/thumbnails/digest.py +++ b/superset/thumbnails/digest.py @@ -60,8 +60,7 @@ def get_dashboard_digest(dashboard: Dashboard) -> str: return func(dashboard, executor_type, executor) unique_string = ( - f"{dashboard.position_json}.{dashboard.css}" - f".{dashboard.json_metadata}.{executor}" + f"{dashboard.position_json}.{dashboard.css}.{dashboard.json_metadata}" ) unique_string = _adjust_string_for_executor(unique_string, executor_type, executor) diff --git a/superset/thumbnails/tasks.py b/superset/thumbnails/tasks.py deleted file mode 100644 index c03d13b0bdbeb..0000000000000 --- a/superset/thumbnails/tasks.py +++ /dev/null @@ -1,101 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -"""Utility functions used across Superset""" - -import logging -from typing import cast, Optional - -from flask import current_app - -from superset import security_manager, thumbnail_cache -from superset.extensions import celery_app -from superset.tasks.utils import get_executor -from superset.utils.core import override_user -from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot -from superset.utils.urls import get_url_path -from superset.utils.webdriver import WindowSize - -logger = logging.getLogger(__name__) - - -@celery_app.task(name="cache_chart_thumbnail", soft_time_limit=300) -def cache_chart_thumbnail( - current_user: Optional[str], - chart_id: int, - force: bool = False, - window_size: Optional[WindowSize] = None, - thumb_size: Optional[WindowSize] = None, -) -> None: - # pylint: disable=import-outside-toplevel - from superset.models.slice import Slice - - if not thumbnail_cache: - logger.warning("No cache set, refusing to compute") - return None - chart = cast(Slice, Slice.get(chart_id)) - url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") - logger.info("Caching chart: %s", url) - _, username = get_executor( - executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], - model=chart, - current_user=current_user, - ) - user = security_manager.find_user(username) - with override_user(user): - screenshot = ChartScreenshot(url, chart.digest) - screenshot.compute_and_cache( - user=user, - cache=thumbnail_cache, - force=force, - window_size=window_size, - thumb_size=thumb_size, - ) - return None - - -@celery_app.task(name="cache_dashboard_thumbnail", soft_time_limit=300) -def cache_dashboard_thumbnail( - current_user: Optional[str], - dashboard_id: int, - force: bool = False, - thumb_size: Optional[WindowSize] = None, -) -> None: - # pylint: disable=import-outside-toplevel - from superset.models.dashboard import Dashboard - - if not thumbnail_cache: - logging.warning("No cache set, refusing to compute") - return - dashboard = Dashboard.get(dashboard_id) - url = get_url_path("Superset.dashboard", dashboard_id_or_slug=dashboard.id) - - logger.info("Caching dashboard: %s", url) - _, username = get_executor( - executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], - model=dashboard, - current_user=current_user, - ) - user = security_manager.find_user(username) - with override_user(user): - screenshot = DashboardScreenshot(url, dashboard.digest) - screenshot.compute_and_cache( - user=user, - cache=thumbnail_cache, - force=force, - thumb_size=thumb_size, - ) diff --git a/tests/integration_tests/superset_test_config_thumbnails.py b/tests/integration_tests/superset_test_config_thumbnails.py index 58fd367c42305..9f621efabbf4d 100644 --- a/tests/integration_tests/superset_test_config_thumbnails.py +++ b/tests/integration_tests/superset_test_config_thumbnails.py @@ -63,7 +63,7 @@ def GET_FEATURE_FLAGS_FUNC(ff): class CeleryConfig(object): BROKER_URL = f"redis://{REDIS_HOST}:{REDIS_PORT}/{REDIS_CELERY_DB}" - CELERY_IMPORTS = ("superset.sql_lab", "superset.thumbnails.tasks") + CELERY_IMPORTS = ("superset.sql_lab", "superset.tasks.thumbnails") CELERY_ANNOTATIONS = {"sql_lab.add": {"rate_limit": "10/s"}} CONCURRENCY = 1 diff --git a/tests/unit_tests/thumbnails/test_digest.py b/tests/unit_tests/thumbnails/test_digest.py index 72a293f85950e..c30d93f5eb81a 100644 --- a/tests/unit_tests/thumbnails/test_digest.py +++ b/tests/unit_tests/thumbnails/test_digest.py @@ -68,14 +68,14 @@ def CUSTOM_CHART_FUNC( [ExecutorType.SELENIUM], False, False, - "9dfd9e0685911ca56f041e57b63bd950", + "0f94edd2d7dfd71724ce8236c05a6bf5", ), ( None, [ExecutorType.CURRENT_USER], True, False, - "55fa9f78f4d8c96464fd5b369a8f2367", + "6b05b0eff27796f266e538a26a155cf7", ), ( { @@ -84,7 +84,7 @@ def CUSTOM_CHART_FUNC( [ExecutorType.CURRENT_USER], True, False, - "9725aa2717974238f03c3fc29bef243b", + "c9c06fd8058f8b1d3bb84e2699aaae0f", ), ( { @@ -93,7 +93,7 @@ def CUSTOM_CHART_FUNC( [ExecutorType.CURRENT_USER], True, False, - "234e168024483a520b705ecf71cf4fca", + "be15e84a631755e537bd2706beeae70a", ), ( { @@ -102,7 +102,7 @@ def CUSTOM_CHART_FUNC( [ExecutorType.CURRENT_USER], True, False, - "430dc5a4ab07928f4465c43a32b4c846", + "9996e1f6cf8ebbf2aebe2938a7d59a0e", ), ( None, From 77d32e549ee18a8c4dca3210c2451f0e9c2fbe3b Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Fri, 9 Dec 2022 16:50:49 +0200 Subject: [PATCH 18/20] fix alert executor --- superset/reports/commands/alert.py | 5 +++-- tests/integration_tests/reports/alert_tests.py | 10 +++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/superset/reports/commands/alert.py b/superset/reports/commands/alert.py index 5ed6029f3cf6b..1044b64505220 100644 --- a/superset/reports/commands/alert.py +++ b/superset/reports/commands/alert.py @@ -25,7 +25,7 @@ from celery.exceptions import SoftTimeLimitExceeded from flask_babel import lazy_gettext as _ -from superset import app, jinja_context +from superset import app, jinja_context, security_manager from superset.commands.base import BaseCommand from superset.reports.commands.exceptions import ( AlertQueryError, @@ -149,10 +149,11 @@ def _execute_query(self) -> pd.DataFrame: rendered_sql, ALERT_SQL_LIMIT ) - user = get_executor( + _, username = get_executor( executor_types=app.config["ALERT_REPORTS_EXECUTE_AS"], model=self._report_schedule, ) + user = security_manager.find_user(username) with override_user(user): start = default_timer() df = self._report_schedule.database.get_df(sql=limited_rendered_sql) diff --git a/tests/integration_tests/reports/alert_tests.py b/tests/integration_tests/reports/alert_tests.py index 822dc246d0496..6c5c41a81ff23 100644 --- a/tests/integration_tests/reports/alert_tests.py +++ b/tests/integration_tests/reports/alert_tests.py @@ -32,19 +32,19 @@ @pytest.mark.parametrize( "owner_names,creator_name,config,expected_result", [ - (["gamma"], None, [ExecutorType.SELENIUM], (ExecutorType.SELENIUM, "admin")), - (["gamma"], None, [ExecutorType.OWNER], (ExecutorType.OWNER, "gamma")), + (["gamma"], None, [ExecutorType.SELENIUM], "admin"), + (["gamma"], None, [ExecutorType.OWNER], "gamma"), ( ["alpha", "gamma"], "gamma", [ExecutorType.CREATOR_OWNER], - (ExecutorType.CREATOR_OWNER, "gamma"), + "gamma", ), ( ["alpha", "gamma"], "alpha", [ExecutorType.CREATOR_OWNER], - (ExecutorType.CREATOR_OWNER, "alpha"), + "alpha", ), ( ["alpha", "gamma"], @@ -96,7 +96,7 @@ def test_execute_query_as_report_executor( ) with cm: command.run() - assert override_user_mock.call_args[0][0] == expected_result + assert override_user_mock.call_args[0][0].username == expected_result app.config["ALERT_REPORTS_EXECUTE_AS"] = original_config From f0d709977975f3609a23e95887da1bf5f5e7463b Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Tue, 13 Dec 2022 17:52:39 +0200 Subject: [PATCH 19/20] fix digest func and update tests --- superset/thumbnails/digest.py | 3 +- tests/unit_tests/thumbnails/test_digest.py | 45 ++++++++++++++++++---- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/superset/thumbnails/digest.py b/superset/thumbnails/digest.py index 543983e21bd90..fb209fcd5072d 100644 --- a/superset/thumbnails/digest.py +++ b/superset/thumbnails/digest.py @@ -60,7 +60,8 @@ def get_dashboard_digest(dashboard: Dashboard) -> str: return func(dashboard, executor_type, executor) unique_string = ( - f"{dashboard.position_json}.{dashboard.css}.{dashboard.json_metadata}" + f"{dashboard.id}\n{dashboard.charts}\n{dashboard.position_json}\n" + f"{dashboard.css}\n{dashboard.json_metadata}" ) unique_string = _adjust_string_for_executor(unique_string, executor_type, executor) diff --git a/tests/unit_tests/thumbnails/test_digest.py b/tests/unit_tests/thumbnails/test_digest.py index c30d93f5eb81a..6e8f11f701b66 100644 --- a/tests/unit_tests/thumbnails/test_digest.py +++ b/tests/unit_tests/thumbnails/test_digest.py @@ -31,8 +31,10 @@ from superset.models.dashboard import Dashboard from superset.models.slice import Slice -_DEFAULT_DASHBOARD_KWARGS = { +_DEFAULT_DASHBOARD_KWARGS: Dict[str, Any] = { "id": 1, + "dashboard_title": "My Title", + "slices": [{"id": 1, "slice_name": "My Chart"}], "position_json": '{"a": "b"}', "css": "background-color: lightblue;", "json_metadata": '{"c": "d"}', @@ -68,14 +70,41 @@ def CUSTOM_CHART_FUNC( [ExecutorType.SELENIUM], False, False, - "0f94edd2d7dfd71724ce8236c05a6bf5", + "71452fee8ffbd8d340193d611bcd4559", ), ( None, [ExecutorType.CURRENT_USER], True, False, - "6b05b0eff27796f266e538a26a155cf7", + "209dc060ac19271b8708731e3b8280f5", + ), + ( + { + "id": 2, + }, + [ExecutorType.CURRENT_USER], + True, + False, + "06a4144466dbd5ffad0c3c2225e96296", + ), + ( + { + "dashboard_title": "My Other Title", + }, + [ExecutorType.CURRENT_USER], + True, + False, + "209dc060ac19271b8708731e3b8280f5", + ), + ( + { + "slices": [{"id": 2, "slice_name": "My Other Chart"}], + }, + [ExecutorType.CURRENT_USER], + True, + False, + "a823ece9563895ccb14f3d9095e84f7a", ), ( { @@ -84,7 +113,7 @@ def CUSTOM_CHART_FUNC( [ExecutorType.CURRENT_USER], True, False, - "c9c06fd8058f8b1d3bb84e2699aaae0f", + "33c5475f92a904925ab3ef493526e5b5", ), ( { @@ -93,7 +122,7 @@ def CUSTOM_CHART_FUNC( [ExecutorType.CURRENT_USER], True, False, - "be15e84a631755e537bd2706beeae70a", + "cec57345e6402c0d4b3caee5cfaa0a03", ), ( { @@ -102,7 +131,7 @@ def CUSTOM_CHART_FUNC( [ExecutorType.CURRENT_USER], True, False, - "9996e1f6cf8ebbf2aebe2938a7d59a0e", + "5380dcbe94621a0759b09554404f3d02", ), ( None, @@ -129,13 +158,15 @@ def test_dashboard_digest( ) -> None: from superset import app from superset.models.dashboard import Dashboard + from superset.models.slice import Slice from superset.thumbnails.digest import get_dashboard_digest kwargs = { **_DEFAULT_DASHBOARD_KWARGS, **(dashboard_overrides or {}), } - dashboard = Dashboard(**kwargs) + slices = [Slice(**slice_kwargs) for slice_kwargs in kwargs.pop("slices")] + dashboard = Dashboard(**kwargs, slices=slices) user: Optional[User] = None if has_current_user: user = User(id=1, username="1") From 74981e5249d7bc92efda6b777d5eac06a52b12a4 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 14 Dec 2022 14:14:08 +0200 Subject: [PATCH 20/20] reorder test and add UPDATING comment --- UPDATING.md | 1 + tests/unit_tests/thumbnails/test_digest.py | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 6ab1276147e38..ba9a6c1bc88a6 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -35,6 +35,7 @@ assists people when migrating to a new version. ### Breaking Changes +- [22328](https://github.com/apache/superset/pull/22328): For deployments that have enabled the "THUMBNAILS" feature flag, the function that calculates dashboard digests has been updated to consider additional properties to more accurately identify changes in the dashboard metadata. This change will invalidate all currently cached dashboard thumbnails. - [21765](https://github.com/apache/superset/pull/21765): For deployments that have enabled the "ALERT_REPORTS" feature flag, Gamma users will no longer have read and write access to Alerts & Reports by default. To give Gamma users the ability to schedule reports from the Dashboard and Explore view like before, create an additional role with "can read on ReportSchedule" and "can write on ReportSchedule" permissions. To further give Gamma users access to the "Alerts & Reports" menu and CRUD view, add "menu access on Manage" and "menu access on Alerts & Report" permissions to the role. ### Potential Downtime diff --git a/tests/unit_tests/thumbnails/test_digest.py b/tests/unit_tests/thumbnails/test_digest.py index 6e8f11f701b66..04f244e629b59 100644 --- a/tests/unit_tests/thumbnails/test_digest.py +++ b/tests/unit_tests/thumbnails/test_digest.py @@ -81,21 +81,21 @@ def CUSTOM_CHART_FUNC( ), ( { - "id": 2, + "dashboard_title": "My Other Title", }, [ExecutorType.CURRENT_USER], True, False, - "06a4144466dbd5ffad0c3c2225e96296", + "209dc060ac19271b8708731e3b8280f5", ), ( { - "dashboard_title": "My Other Title", + "id": 2, }, [ExecutorType.CURRENT_USER], True, False, - "209dc060ac19271b8708731e3b8280f5", + "06a4144466dbd5ffad0c3c2225e96296", ), ( {