From 959b15eeca0a1e2e93c1a8688f8629d0f6ee5776 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Thu, 20 Jan 2022 16:27:57 -0300 Subject: [PATCH] feat: Adds a key-value endpoint to store charts form data (#17882) * feat: Adds a key-value endpoint to store charts form data * Fixes linting problems * Removes the query_params from the endpoints * Refactors the commands * Removes unused imports * Changes the parameters to use dataclass * Adds more access tests * Gets the first dataset while testing * Adds unit tests for the check_access function * Changes the can_access check * Always check for dataset access --- superset/charts/commands/exceptions.py | 4 + superset/charts/form_data/__init__.py | 16 ++ superset/charts/form_data/api.py | 259 ++++++++++++++++++ .../charts/form_data/commands/__init__.py | 16 ++ superset/charts/form_data/commands/create.py | 35 +++ superset/charts/form_data/commands/delete.py | 37 +++ superset/charts/form_data/commands/get.py | 44 +++ superset/charts/form_data/commands/update.py | 40 +++ superset/charts/form_data/utils.py | 70 +++++ superset/config.py | 11 +- superset/dashboards/filter_state/api.py | 4 +- .../filter_state/commands/create.py | 21 +- .../filter_state/commands/delete.py | 20 +- .../dashboards/filter_state/commands/get.py | 16 +- .../filter_state/commands/update.py | 25 +- superset/datasets/commands/exceptions.py | 4 + superset/initialization/__init__.py | 8 +- superset/key_value/api.py | 112 ++++++-- superset/key_value/commands/create.py | 23 +- superset/key_value/commands/delete.py | 16 +- .../commands/entry.py | 0 superset/key_value/commands/get.py | 18 +- superset/key_value/commands/parameters.py | 29 ++ superset/key_value/commands/update.py | 19 +- superset/utils/cache_manager.py | 12 + .../charts/form_data/__init__.py | 16 ++ .../charts/form_data/api_tests.py | 245 +++++++++++++++++ .../dashboards/filter_state/api_tests.py | 2 +- .../unit_tests/charts/form_data/utils_test.py | 186 +++++++++++++ 29 files changed, 1179 insertions(+), 129 deletions(-) create mode 100644 superset/charts/form_data/__init__.py create mode 100644 superset/charts/form_data/api.py create mode 100644 superset/charts/form_data/commands/__init__.py create mode 100644 superset/charts/form_data/commands/create.py create mode 100644 superset/charts/form_data/commands/delete.py create mode 100644 superset/charts/form_data/commands/get.py create mode 100644 superset/charts/form_data/commands/update.py create mode 100644 superset/charts/form_data/utils.py rename superset/{dashboards/filter_state => key_value}/commands/entry.py (100%) create mode 100644 superset/key_value/commands/parameters.py create mode 100644 tests/integration_tests/charts/form_data/__init__.py create mode 100644 tests/integration_tests/charts/form_data/api_tests.py create mode 100644 tests/unit_tests/charts/form_data/utils_test.py diff --git a/superset/charts/commands/exceptions.py b/superset/charts/commands/exceptions.py index 60a62e19879d..6d5c078b1228 100644 --- a/superset/charts/commands/exceptions.py +++ b/superset/charts/commands/exceptions.py @@ -127,6 +127,10 @@ class ChartDeleteFailedReportsExistError(ChartDeleteFailedError): message = _("There are associated alerts or reports") +class ChartAccessDeniedError(ForbiddenError): + message = _("You don't have access to this chart.") + + class ChartForbiddenError(ForbiddenError): message = _("Changing this chart is forbidden") diff --git a/superset/charts/form_data/__init__.py b/superset/charts/form_data/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/superset/charts/form_data/__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/charts/form_data/api.py b/superset/charts/form_data/api.py new file mode 100644 index 000000000000..bc908b25d6b8 --- /dev/null +++ b/superset/charts/form_data/api.py @@ -0,0 +1,259 @@ +# 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. +import logging +from typing import Type + +from flask import Response +from flask_appbuilder.api import expose, protect, safe + +from superset.charts.form_data.commands.create import CreateFormDataCommand +from superset.charts.form_data.commands.delete import DeleteFormDataCommand +from superset.charts.form_data.commands.get import GetFormDataCommand +from superset.charts.form_data.commands.update import UpdateFormDataCommand +from superset.extensions import event_logger +from superset.key_value.api import KeyValueRestApi + +logger = logging.getLogger(__name__) + + +class ChartFormDataRestApi(KeyValueRestApi): + class_permission_name = "ChartFormDataRestApi" + resource_name = "chart" + openapi_spec_tag = "Chart Form Data" + + def get_create_command(self) -> Type[CreateFormDataCommand]: + return CreateFormDataCommand + + def get_update_command(self) -> Type[UpdateFormDataCommand]: + return UpdateFormDataCommand + + def get_get_command(self) -> Type[GetFormDataCommand]: + return GetFormDataCommand + + def get_delete_command(self) -> Type[DeleteFormDataCommand]: + return DeleteFormDataCommand + + @expose("//form_data", methods=["POST"]) + @protect() + @safe + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", + log_to_statsd=False, + ) + def post(self, pk: int) -> Response: + """Stores a new value. + --- + post: + description: >- + Stores a new value. + parameters: + - in: path + schema: + type: integer + name: pk + - in: query + schema: + type: integer + name: dataset_id + required: true + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/KeyValuePostSchema' + responses: + 201: + description: The value was stored successfully. + content: + application/json: + schema: + type: object + properties: + key: + type: string + description: The key to retrieve the value. + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + return super().post(pk) + + @expose("//form_data/", methods=["PUT"]) + @protect() + @safe + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", + log_to_statsd=False, + ) + def put(self, pk: int, key: str) -> Response: + """Updates an existing value. + --- + put: + description: >- + Updates an existing value. + parameters: + - in: path + schema: + type: integer + name: pk + - in: path + schema: + type: string + name: key + - in: query + schema: + type: integer + name: dataset_id + required: true + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/KeyValuePutSchema' + responses: + 200: + description: The value was stored successfully. + content: + application/json: + schema: + type: object + properties: + message: + type: string + description: The result of the operation + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + return super().put(pk, key) + + @expose("//form_data/", methods=["GET"]) + @protect() + @safe + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get", + log_to_statsd=False, + ) + def get(self, pk: int, key: str) -> Response: + """Retrives a value. + --- + get: + description: >- + Retrives a value. + parameters: + - in: path + schema: + type: integer + name: pk + - in: path + schema: + type: string + name: key + - in: query + schema: + type: integer + name: dataset_id + required: true + responses: + 200: + description: Returns the stored value. + content: + application/json: + schema: + type: object + properties: + value: + type: string + description: The stored value + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + return super().get(pk, key) + + @expose("//form_data/", methods=["DELETE"]) + @protect() + @safe + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete", + log_to_statsd=False, + ) + def delete(self, pk: int, key: str) -> Response: + """Deletes a value. + --- + delete: + description: >- + Deletes a value. + parameters: + - in: path + schema: + type: integer + name: pk + - in: path + schema: + type: string + name: key + description: The value key. + - in: query + schema: + type: integer + name: dataset_id + required: true + responses: + 200: + description: Deleted the stored value. + content: + application/json: + schema: + type: object + properties: + message: + type: string + description: The result of the operation + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + return super().delete(pk, key) diff --git a/superset/charts/form_data/commands/__init__.py b/superset/charts/form_data/commands/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/superset/charts/form_data/commands/__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/charts/form_data/commands/create.py b/superset/charts/form_data/commands/create.py new file mode 100644 index 000000000000..d7c2ad413a4f --- /dev/null +++ b/superset/charts/form_data/commands/create.py @@ -0,0 +1,35 @@ +# 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.charts.form_data.utils import check_access, get_dataset_id +from superset.extensions import cache_manager +from superset.key_value.commands.create import CreateKeyValueCommand +from superset.key_value.commands.entry import Entry +from superset.key_value.commands.parameters import CommandParameters +from superset.key_value.utils import cache_key + + +class CreateFormDataCommand(CreateKeyValueCommand): + def create(self, cmd_params: CommandParameters) -> bool: + check_access(cmd_params) + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) + value = cmd_params.value + if value: + entry: Entry = {"owner": actor.get_user_id(), "value": value} + return cache_manager.chart_form_data_cache.set(key, entry) + return False diff --git a/superset/charts/form_data/commands/delete.py b/superset/charts/form_data/commands/delete.py new file mode 100644 index 000000000000..be6c33a00508 --- /dev/null +++ b/superset/charts/form_data/commands/delete.py @@ -0,0 +1,37 @@ +# 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.charts.form_data.utils import check_access, get_dataset_id +from superset.extensions import cache_manager +from superset.key_value.commands.delete import DeleteKeyValueCommand +from superset.key_value.commands.entry import Entry +from superset.key_value.commands.exceptions import KeyValueAccessDeniedError +from superset.key_value.commands.parameters import CommandParameters +from superset.key_value.utils import cache_key + + +class DeleteFormDataCommand(DeleteKeyValueCommand): + def delete(self, cmd_params: CommandParameters) -> bool: + check_access(cmd_params) + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) + entry: Entry = cache_manager.chart_form_data_cache.get(key) + if entry: + if entry["owner"] != actor.get_user_id(): + raise KeyValueAccessDeniedError() + return cache_manager.chart_form_data_cache.delete(key) + return True diff --git a/superset/charts/form_data/commands/get.py b/superset/charts/form_data/commands/get.py new file mode 100644 index 000000000000..f23acfc2d186 --- /dev/null +++ b/superset/charts/form_data/commands/get.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 typing import Optional + +from flask import current_app as app + +from superset.charts.form_data.utils import check_access, get_dataset_id +from superset.extensions import cache_manager +from superset.key_value.commands.entry import Entry +from superset.key_value.commands.get import GetKeyValueCommand +from superset.key_value.commands.parameters import CommandParameters +from superset.key_value.utils import cache_key + + +class GetFormDataCommand(GetKeyValueCommand): + def __init__(self, cmd_params: CommandParameters) -> None: + super().__init__(cmd_params) + config = app.config["CHART_FORM_DATA_CACHE_CONFIG"] + self._refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + + def get(self, cmd_params: CommandParameters) -> Optional[str]: + check_access(cmd_params) + resource_id = cmd_params.resource_id + key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) + entry: Entry = cache_manager.chart_form_data_cache.get(key) + if entry: + if self._refresh_timeout: + cache_manager.chart_form_data_cache.set(key, entry) + return entry["value"] + return None diff --git a/superset/charts/form_data/commands/update.py b/superset/charts/form_data/commands/update.py new file mode 100644 index 000000000000..cab988da8713 --- /dev/null +++ b/superset/charts/form_data/commands/update.py @@ -0,0 +1,40 @@ +# 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.charts.form_data.utils import check_access, get_dataset_id +from superset.extensions import cache_manager +from superset.key_value.commands.entry import Entry +from superset.key_value.commands.exceptions import KeyValueAccessDeniedError +from superset.key_value.commands.parameters import CommandParameters +from superset.key_value.commands.update import UpdateKeyValueCommand +from superset.key_value.utils import cache_key + + +class UpdateFormDataCommand(UpdateKeyValueCommand): + def update(self, cmd_params: CommandParameters) -> bool: + check_access(cmd_params) + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) + value = cmd_params.value + entry: Entry = cache_manager.chart_form_data_cache.get(key) + if entry and value: + user_id = actor.get_user_id() + if entry["owner"] != user_id: + raise KeyValueAccessDeniedError() + new_entry: Entry = {"owner": actor.get_user_id(), "value": value} + return cache_manager.chart_form_data_cache.set(key, new_entry) + return False diff --git a/superset/charts/form_data/utils.py b/superset/charts/form_data/utils.py new file mode 100644 index 000000000000..c1f98a238840 --- /dev/null +++ b/superset/charts/form_data/utils.py @@ -0,0 +1,70 @@ +# 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 superset import security_manager +from superset.charts.commands.exceptions import ( + ChartAccessDeniedError, + ChartNotFoundError, +) +from superset.charts.dao import ChartDAO +from superset.datasets.commands.exceptions import ( + DatasetAccessDeniedError, + DatasetNotFoundError, +) +from superset.datasets.dao import DatasetDAO +from superset.key_value.commands.parameters import CommandParameters +from superset.views.base import is_user_admin +from superset.views.utils import is_owner + + +def get_dataset_id(cmd_params: CommandParameters) -> Optional[str]: + query_params = cmd_params.query_params + if query_params: + return query_params.get("dataset_id") + return None + + +def check_dataset_access(cmd_params: CommandParameters) -> Optional[bool]: + dataset_id = get_dataset_id(cmd_params) + if dataset_id: + dataset = DatasetDAO.find_by_id(int(dataset_id)) + if dataset: + can_access_datasource = security_manager.can_access_datasource(dataset) + if can_access_datasource: + return True + raise DatasetAccessDeniedError() + raise DatasetNotFoundError() + + +def check_access(cmd_params: CommandParameters) -> Optional[bool]: + resource_id = cmd_params.resource_id + actor = cmd_params.actor + check_dataset_access(cmd_params) + if resource_id == 0: + return True + chart = ChartDAO.find_by_id(resource_id) + if chart: + can_access_chart = ( + is_user_admin() + or is_owner(chart, actor) + or security_manager.can_access("can_read", "Chart") + ) + if can_access_chart: + return True + raise ChartAccessDeniedError() + raise ChartNotFoundError() diff --git a/superset/config.py b/superset/config.py index f5552961b397..b9a98c4ccfba 100644 --- a/superset/config.py +++ b/superset/config.py @@ -577,13 +577,22 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # Cache for filters state FILTER_STATE_CACHE_CONFIG: CacheConfig = { - "CACHE_TYPE": "filesystem", + "CACHE_TYPE": "FileSystemCache", "CACHE_DIR": os.path.join(DATA_DIR, "cache"), "CACHE_DEFAULT_TIMEOUT": int(timedelta(days=90).total_seconds()), "CACHE_THRESHOLD": 0, "REFRESH_TIMEOUT_ON_RETRIEVAL": True, } +# Cache for chart form data +CHART_FORM_DATA_CACHE_CONFIG: CacheConfig = { + "CACHE_TYPE": "FileSystemCache", + "CACHE_DIR": os.path.join(DATA_DIR, "cache"), + "CACHE_DEFAULT_TIMEOUT": int(timedelta(days=7).total_seconds()), + "CACHE_THRESHOLD": 0, + "REFRESH_TIMEOUT_ON_RETRIEVAL": True, +} + # store cache keys by datasource UID (via CacheKey) for custom processing/invalidation STORE_CACHE_KEYS_IN_METADATA_DB = False diff --git a/superset/dashboards/filter_state/api.py b/superset/dashboards/filter_state/api.py index 2a3e405f0a74..68209f3e7d67 100644 --- a/superset/dashboards/filter_state/api.py +++ b/superset/dashboards/filter_state/api.py @@ -31,9 +31,9 @@ class DashboardFilterStateRestApi(KeyValueRestApi): - class_permission_name = "FilterStateRestApi" + class_permission_name = "DashboardFilterStateRestApi" resource_name = "dashboard" - openapi_spec_tag = "Filter State" + openapi_spec_tag = "Dashboard Filter State" def get_create_command(self) -> Type[CreateFilterStateCommand]: return CreateFilterStateCommand diff --git a/superset/dashboards/filter_state/commands/create.py b/superset/dashboards/filter_state/commands/create.py index 045b2faedece..40a70ba27ce1 100644 --- a/superset/dashboards/filter_state/commands/create.py +++ b/superset/dashboards/filter_state/commands/create.py @@ -14,25 +14,22 @@ # 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_appbuilder.security.sqla.models import User - from superset.dashboards.dao import DashboardDAO -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager from superset.key_value.commands.create import CreateKeyValueCommand +from superset.key_value.commands.entry import Entry +from superset.key_value.commands.parameters import CommandParameters from superset.key_value.utils import cache_key class CreateFilterStateCommand(CreateKeyValueCommand): - def create( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def create(self, cmd_params: CommandParameters) -> bool: + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cache_key(resource_id, cmd_params.key) + value = cmd_params.value dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) - if dashboard: + if dashboard and value: entry: Entry = {"owner": actor.get_user_id(), "value": value} - return cache_manager.filter_state_cache.set( - cache_key(resource_id, key), entry - ) + return cache_manager.filter_state_cache.set(key, entry) return False diff --git a/superset/dashboards/filter_state/commands/delete.py b/superset/dashboards/filter_state/commands/delete.py index 0f27911cc91a..225dd8aa63cd 100644 --- a/superset/dashboards/filter_state/commands/delete.py +++ b/superset/dashboards/filter_state/commands/delete.py @@ -14,29 +14,25 @@ # 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_appbuilder.security.sqla.models import User - from superset.dashboards.dao import DashboardDAO -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager from superset.key_value.commands.delete import DeleteKeyValueCommand +from superset.key_value.commands.entry import Entry from superset.key_value.commands.exceptions import KeyValueAccessDeniedError +from superset.key_value.commands.parameters import CommandParameters from superset.key_value.utils import cache_key class DeleteFilterStateCommand(DeleteKeyValueCommand): - def delete(self, actor: User, resource_id: int, key: str) -> Optional[bool]: + def delete(self, cmd_params: CommandParameters) -> bool: + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cache_key(resource_id, cmd_params.key) dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: - entry: Entry = cache_manager.filter_state_cache.get( - cache_key(resource_id, key) - ) + entry: Entry = cache_manager.filter_state_cache.get(key) if entry: if entry["owner"] != actor.get_user_id(): raise KeyValueAccessDeniedError() - return cache_manager.filter_state_cache.delete( - cache_key(resource_id, key) - ) + return cache_manager.filter_state_cache.delete(key) return False diff --git a/superset/dashboards/filter_state/commands/get.py b/superset/dashboards/filter_state/commands/get.py index bb3e95aaa615..509960fdb382 100644 --- a/superset/dashboards/filter_state/commands/get.py +++ b/superset/dashboards/filter_state/commands/get.py @@ -16,16 +16,26 @@ # under the License. from typing import Optional +from flask import current_app as app + from superset.dashboards.dao import DashboardDAO from superset.extensions import cache_manager from superset.key_value.commands.get import GetKeyValueCommand +from superset.key_value.commands.parameters import CommandParameters from superset.key_value.utils import cache_key class GetFilterStateCommand(GetKeyValueCommand): - def get(self, resource_id: int, key: str, refresh_timeout: bool) -> Optional[str]: + def __init__(self, cmd_params: CommandParameters) -> None: + super().__init__(cmd_params) + config = app.config["FILTER_STATE_CACHE_CONFIG"] + self._refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + + def get(self, cmd_params: CommandParameters) -> Optional[str]: + resource_id = cmd_params.resource_id + key = cache_key(resource_id, cmd_params.key) DashboardDAO.get_by_id_or_slug(str(resource_id)) - entry = cache_manager.filter_state_cache.get(cache_key(resource_id, key)) or {} - if entry and refresh_timeout: + entry = cache_manager.filter_state_cache.get(key) or {} + if entry and self._refresh_timeout: cache_manager.filter_state_cache.set(key, entry) return entry.get("value") diff --git a/superset/dashboards/filter_state/commands/update.py b/superset/dashboards/filter_state/commands/update.py index 1412348cf355..e8d0606f672d 100644 --- a/superset/dashboards/filter_state/commands/update.py +++ b/superset/dashboards/filter_state/commands/update.py @@ -14,33 +14,28 @@ # 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_appbuilder.security.sqla.models import User - from superset.dashboards.dao import DashboardDAO -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager +from superset.key_value.commands.entry import Entry from superset.key_value.commands.exceptions import KeyValueAccessDeniedError +from superset.key_value.commands.parameters import CommandParameters from superset.key_value.commands.update import UpdateKeyValueCommand from superset.key_value.utils import cache_key class UpdateFilterStateCommand(UpdateKeyValueCommand): - def update( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def update(self, cmd_params: CommandParameters) -> bool: + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cache_key(resource_id, cmd_params.key) + value = cmd_params.value dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) - if dashboard: - entry: Entry = cache_manager.filter_state_cache.get( - cache_key(resource_id, key) - ) + if dashboard and value: + entry: Entry = cache_manager.filter_state_cache.get(key) if entry: user_id = actor.get_user_id() if entry["owner"] != user_id: raise KeyValueAccessDeniedError() new_entry: Entry = {"owner": actor.get_user_id(), "value": value} - return cache_manager.filter_state_cache.set( - cache_key(resource_id, key), new_entry - ) + return cache_manager.filter_state_cache.set(key, new_entry) return False diff --git a/superset/datasets/commands/exceptions.py b/superset/datasets/commands/exceptions.py index 44064f07cc01..34c15721abfb 100644 --- a/superset/datasets/commands/exceptions.py +++ b/superset/datasets/commands/exceptions.py @@ -179,3 +179,7 @@ class DatasetForbiddenError(ForbiddenError): class DatasetImportError(ImportFailedError): message = _("Import dataset failed for an unknown reason") + + +class DatasetAccessDeniedError(ForbiddenError): + message = _("You don't have access to this dataset.") diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 95e260d2a41d..98cd9af264cf 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -119,6 +119,7 @@ def init_views(self) -> None: from superset.cachekeys.api import CacheRestApi from superset.charts.api import ChartRestApi from superset.charts.data.api import ChartDataRestApi + from superset.charts.form_data.api import ChartFormDataRestApi from superset.connectors.druid.views import ( Druid, DruidClusterModelView, @@ -203,18 +204,19 @@ def init_views(self) -> None: appbuilder.add_api(CacheRestApi) appbuilder.add_api(ChartRestApi) appbuilder.add_api(ChartDataRestApi) + appbuilder.add_api(ChartFormDataRestApi) appbuilder.add_api(CssTemplateRestApi) + appbuilder.add_api(DashboardFilterStateRestApi) appbuilder.add_api(DashboardRestApi) appbuilder.add_api(DatabaseRestApi) appbuilder.add_api(DatasetRestApi) appbuilder.add_api(DatasetColumnsRestApi) appbuilder.add_api(DatasetMetricRestApi) + appbuilder.add_api(FilterSetRestApi) appbuilder.add_api(QueryRestApi) - appbuilder.add_api(SavedQueryRestApi) appbuilder.add_api(ReportScheduleRestApi) appbuilder.add_api(ReportExecutionLogRestApi) - appbuilder.add_api(FilterSetRestApi) - appbuilder.add_api(DashboardFilterStateRestApi) + appbuilder.add_api(SavedQueryRestApi) # # Setup regular views # diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 2dc340095cb9..85aa6e374642 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -19,17 +19,27 @@ from typing import Any from apispec import APISpec +from apispec.exceptions import DuplicateComponentNameError from flask import g, request, Response from flask_appbuilder.api import BaseApi from marshmallow import ValidationError +from superset.charts.commands.exceptions import ( + ChartAccessDeniedError, + ChartNotFoundError, +) from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.dashboards.commands.exceptions import ( DashboardAccessDeniedError, DashboardNotFoundError, ) +from superset.datasets.commands.exceptions import ( + DatasetAccessDeniedError, + DatasetNotFoundError, +) from superset.exceptions import InvalidPayloadFormatError from superset.key_value.commands.exceptions import KeyValueAccessDeniedError +from superset.key_value.commands.parameters import CommandParameters from superset.key_value.schemas import KeyValuePostSchema, KeyValuePutSchema logger = logging.getLogger(__name__) @@ -48,12 +58,15 @@ class KeyValueRestApi(BaseApi, ABC): allow_browser_login = True def add_apispec_components(self, api_spec: APISpec) -> None: - api_spec.components.schema( - KeyValuePostSchema.__name__, schema=KeyValuePostSchema, - ) - api_spec.components.schema( - KeyValuePutSchema.__name__, schema=KeyValuePutSchema, - ) + try: + api_spec.components.schema( + KeyValuePostSchema.__name__, schema=KeyValuePostSchema, + ) + api_spec.components.schema( + KeyValuePutSchema.__name__, schema=KeyValuePutSchema, + ) + except DuplicateComponentNameError: + pass super().add_apispec_components(api_spec) def post(self, pk: int) -> Response: @@ -61,52 +74,91 @@ def post(self, pk: int) -> Response: raise InvalidPayloadFormatError("Request is not JSON") try: item = self.add_model_schema.load(request.json) - key = self.get_create_command()(g.user, pk, item["value"]).run() + args = CommandParameters( + actor=g.user, + resource_id=pk, + value=item["value"], + query_params=request.args, + ) + key = self.get_create_command()(args).run() return self.response(201, key=key) - except ValidationError as error: - return self.response_400(message=error.messages) - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): - return self.response_403() - except DashboardNotFoundError: - return self.response_404() + except ValidationError as ex: + return self.response(400, message=ex.messages) + except ( + ChartAccessDeniedError, + DashboardAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DashboardNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) def put(self, pk: int, key: str) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") try: item = self.edit_model_schema.load(request.json) - result = self.get_update_command()(g.user, pk, key, item["value"]).run() + args = CommandParameters( + actor=g.user, + resource_id=pk, + key=key, + value=item["value"], + query_params=request.args, + ) + result = self.get_update_command()(args).run() if not result: return self.response_404() return self.response(200, message="Value updated successfully.") - except ValidationError as error: - return self.response_400(message=error.messages) - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): - return self.response_403() - except DashboardNotFoundError: - return self.response_404() + except ValidationError as ex: + return self.response(400, message=ex.messages) + except ( + ChartAccessDeniedError, + DashboardAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DashboardNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) def get(self, pk: int, key: str) -> Response: try: - value = self.get_get_command()(g.user, pk, key).run() + args = CommandParameters( + actor=g.user, resource_id=pk, key=key, query_params=request.args + ) + value = self.get_get_command()(args).run() if not value: return self.response_404() return self.response(200, value=value) - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): - return self.response_403() - except DashboardNotFoundError: - return self.response_404() + except ( + ChartAccessDeniedError, + DashboardAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DashboardNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) def delete(self, pk: int, key: str) -> Response: try: - result = self.get_delete_command()(g.user, pk, key).run() + args = CommandParameters( + actor=g.user, resource_id=pk, key=key, query_params=request.args + ) + result = self.get_delete_command()(args).run() if not result: return self.response_404() return self.response(200, message="Deleted successfully") - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): - return self.response_403() - except DashboardNotFoundError: - return self.response_404() + except ( + ChartAccessDeniedError, + DashboardAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DashboardNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) @abstractmethod def get_create_command(self) -> Any: diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index 3c56f73ebebe..8a7eec9298c0 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -17,30 +17,25 @@ import logging from abc import ABC, abstractmethod from secrets import token_urlsafe -from typing import Optional -from flask_appbuilder.models.sqla import Model -from flask_appbuilder.security.sqla.models import User from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand from superset.key_value.commands.exceptions import KeyValueCreateFailedError +from superset.key_value.commands.parameters import CommandParameters logger = logging.getLogger(__name__) class CreateKeyValueCommand(BaseCommand, ABC): - def __init__( - self, actor: User, resource_id: int, value: str, - ): - self._actor = actor - self._resource_id = resource_id - self._value = value - - def run(self) -> Model: + def __init__(self, cmd_params: CommandParameters): + self._cmd_params = cmd_params + + def run(self) -> str: try: key = token_urlsafe(48) - self.create(self._actor, self._resource_id, key, self._value) + self._cmd_params.key = key + self.create(self._cmd_params) return key except SQLAlchemyError as ex: logger.exception("Error running create command") @@ -50,7 +45,5 @@ def validate(self) -> None: pass @abstractmethod - def create( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def create(self, cmd_params: CommandParameters) -> bool: ... diff --git a/superset/key_value/commands/delete.py b/superset/key_value/commands/delete.py index 9c885d07732b..6d2983c06378 100644 --- a/superset/key_value/commands/delete.py +++ b/superset/key_value/commands/delete.py @@ -16,27 +16,23 @@ # under the License. import logging from abc import ABC, abstractmethod -from typing import Optional -from flask_appbuilder.models.sqla import Model -from flask_appbuilder.security.sqla.models import User from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand from superset.key_value.commands.exceptions import KeyValueDeleteFailedError +from superset.key_value.commands.parameters import CommandParameters logger = logging.getLogger(__name__) class DeleteKeyValueCommand(BaseCommand, ABC): - def __init__(self, actor: User, resource_id: int, key: str): - self._actor = actor - self._resource_id = resource_id - self._key = key + def __init__(self, cmd_params: CommandParameters): + self._cmd_params = cmd_params - def run(self) -> Model: + def run(self) -> bool: try: - return self.delete(self._actor, self._resource_id, self._key) + return self.delete(self._cmd_params) except SQLAlchemyError as ex: logger.exception("Error running delete command") raise KeyValueDeleteFailedError() from ex @@ -45,5 +41,5 @@ def validate(self) -> None: pass @abstractmethod - def delete(self, actor: User, resource_id: int, key: str) -> Optional[bool]: + def delete(self, cmd_params: CommandParameters) -> bool: ... diff --git a/superset/dashboards/filter_state/commands/entry.py b/superset/key_value/commands/entry.py similarity index 100% rename from superset/dashboards/filter_state/commands/entry.py rename to superset/key_value/commands/entry.py diff --git a/superset/key_value/commands/get.py b/superset/key_value/commands/get.py index 43a5806cb595..a697c808c01d 100644 --- a/superset/key_value/commands/get.py +++ b/superset/key_value/commands/get.py @@ -18,28 +18,22 @@ from abc import ABC, abstractmethod from typing import Optional -from flask import current_app as app -from flask_appbuilder.models.sqla import Model -from flask_appbuilder.security.sqla.models import User from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand from superset.key_value.commands.exceptions import KeyValueGetFailedError +from superset.key_value.commands.parameters import CommandParameters logger = logging.getLogger(__name__) class GetKeyValueCommand(BaseCommand, ABC): - def __init__(self, actor: User, resource_id: int, key: str): - self._actor = actor - self._resource_id = resource_id - self._key = key + def __init__(self, cmd_params: CommandParameters): + self._cmd_params = cmd_params - def run(self) -> Model: + def run(self) -> Optional[str]: try: - config = app.config["FILTER_STATE_CACHE_CONFIG"] - refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") - return self.get(self._resource_id, self._key, refresh_timeout) + return self.get(self._cmd_params) except SQLAlchemyError as ex: logger.exception("Error running get command") raise KeyValueGetFailedError() from ex @@ -48,5 +42,5 @@ def validate(self) -> None: pass @abstractmethod - def get(self, resource_id: int, key: str, refresh_timeout: bool) -> Optional[str]: + def get(self, cmd_params: CommandParameters) -> Optional[str]: ... diff --git a/superset/key_value/commands/parameters.py b/superset/key_value/commands/parameters.py new file mode 100644 index 000000000000..00a933c67c71 --- /dev/null +++ b/superset/key_value/commands/parameters.py @@ -0,0 +1,29 @@ +# 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 Dict, Optional + +from flask_appbuilder.security.sqla.models import User + + +@dataclass +class CommandParameters: + actor: User + resource_id: int + query_params: Dict[str, str] + key: Optional[str] = None + value: Optional[str] = None diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py index 3f322c7a27e2..7990d2125272 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -16,30 +16,25 @@ # under the License. import logging from abc import ABC, abstractmethod -from typing import Optional -from flask_appbuilder.models.sqla import Model -from flask_appbuilder.security.sqla.models import User from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand from superset.key_value.commands.exceptions import KeyValueUpdateFailedError +from superset.key_value.commands.parameters import CommandParameters logger = logging.getLogger(__name__) class UpdateKeyValueCommand(BaseCommand, ABC): def __init__( - self, actor: User, resource_id: int, key: str, value: str, + self, cmd_params: CommandParameters, ): - self._actor = actor - self._resource_id = resource_id - self._key = key - self._value = value + self._parameters = cmd_params - def run(self) -> Model: + def run(self) -> bool: try: - return self.update(self._actor, self._resource_id, self._key, self._value) + return self.update(self._parameters) except SQLAlchemyError as ex: logger.exception("Error running update command") raise KeyValueUpdateFailedError() from ex @@ -48,7 +43,5 @@ def validate(self) -> None: pass @abstractmethod - def update( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def update(self, cmd_params: CommandParameters) -> bool: ... diff --git a/superset/utils/cache_manager.py b/superset/utils/cache_manager.py index 5e6e96ebd253..3624d918f9c7 100644 --- a/superset/utils/cache_manager.py +++ b/superset/utils/cache_manager.py @@ -26,6 +26,7 @@ def __init__(self) -> None: self._data_cache = Cache() self._thumbnail_cache = Cache() self._filter_state_cache = Cache() + self._chart_form_data_cache = Cache() def init_app(self, app: Flask) -> None: self._cache.init_app( @@ -56,6 +57,13 @@ def init_app(self, app: Flask) -> None: **app.config["FILTER_STATE_CACHE_CONFIG"], }, ) + self._chart_form_data_cache.init_app( + app, + { + "CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], + **app.config["CHART_FORM_DATA_CACHE_CONFIG"], + }, + ) @property def data_cache(self) -> Cache: @@ -72,3 +80,7 @@ def thumbnail_cache(self) -> Cache: @property def filter_state_cache(self) -> Cache: return self._filter_state_cache + + @property + def chart_form_data_cache(self) -> Cache: + return self._chart_form_data_cache diff --git a/tests/integration_tests/charts/form_data/__init__.py b/tests/integration_tests/charts/form_data/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/integration_tests/charts/form_data/__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/integration_tests/charts/form_data/api_tests.py b/tests/integration_tests/charts/form_data/api_tests.py new file mode 100644 index 000000000000..f411c1a6edc9 --- /dev/null +++ b/tests/integration_tests/charts/form_data/api_tests.py @@ -0,0 +1,245 @@ +# 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. +import json +from unittest.mock import patch + +import pytest +from flask_appbuilder.security.sqla.models import User +from sqlalchemy.orm import Session + +from superset.connectors.sqla.models import SqlaTable +from superset.datasets.commands.exceptions import DatasetAccessDeniedError +from superset.extensions import cache_manager +from superset.key_value.commands.entry import Entry +from superset.key_value.utils import cache_key +from superset.models.slice import Slice +from tests.integration_tests.base_tests import login +from tests.integration_tests.fixtures.world_bank_dashboard import ( + load_world_bank_dashboard_with_slices, + load_world_bank_data, +) +from tests.integration_tests.test_app import app + +key = "test-key" +value = "test" + + +@pytest.fixture +def client(): + with app.test_client() as client: + with app.app_context(): + yield client + + +@pytest.fixture +def chart_id(load_world_bank_dashboard_with_slices) -> int: + with app.app_context() as ctx: + session: Session = ctx.app.appbuilder.get_session + chart = session.query(Slice).filter_by(slice_name="World's Population").one() + return chart.id + + +@pytest.fixture +def admin_id() -> int: + with app.app_context() as ctx: + session: Session = ctx.app.appbuilder.get_session + admin = session.query(User).filter_by(username="admin").one() + return admin.id + + +@pytest.fixture +def dataset_id() -> int: + with app.app_context() as ctx: + session: Session = ctx.app.appbuilder.get_session + dataset = ( + session.query(SqlaTable) + .filter_by(table_name="wb_health_population") + .first() + ) + return dataset.id + + +@pytest.fixture(autouse=True) +def cache(chart_id, admin_id, dataset_id): + app.config["CHART_FORM_DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} + cache_manager.init_app(app) + entry: Entry = {"owner": admin_id, "value": value} + cache_manager.chart_form_data_cache.set(cache_key(chart_id, key), entry) + cache_manager.chart_form_data_cache.set(cache_key(dataset_id, key), entry) + + +def test_post(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "value": value, + } + resp = client.post( + f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload + ) + assert resp.status_code == 201 + + +def test_post_bad_request(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "value": 1234, + } + resp = client.post( + f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload + ) + assert resp.status_code == 400 + + +def test_post_access_denied(client, chart_id: int, dataset_id: int): + login(client, "gamma") + payload = { + "value": value, + } + resp = client.post( + f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload + ) + assert resp.status_code == 404 + + +def test_put(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "value": "new value", + } + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) + assert resp.status_code == 200 + + +def test_put_bad_request(client, chart_id: int, dataset_id: int): + login(client, "admin") + payload = { + "value": 1234, + } + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) + assert resp.status_code == 400 + + +def test_put_access_denied(client, chart_id: int, dataset_id: int): + login(client, "gamma") + payload = { + "value": "new value", + } + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) + assert resp.status_code == 404 + + +def test_put_not_owner(client, chart_id: int, dataset_id: int): + login(client, "gamma") + payload = { + "value": "new value", + } + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) + assert resp.status_code == 404 + + +def test_get_key_not_found(client, chart_id: int, dataset_id: int): + login(client, "admin") + resp = client.get( + f"api/v1/chart/{chart_id}/form_data/unknown-key?dataset_id={dataset_id}" + ) + assert resp.status_code == 404 + + +def test_get_chart_not_found(client, dataset_id: int): + login(client, "admin") + resp = client.get(f"api/v1/chart/-1/form_data/{key}?dataset_id={dataset_id}") + assert resp.status_code == 404 + + +def test_get(client, chart_id: int, dataset_id: int): + login(client, "admin") + resp = client.get( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) + assert resp.status_code == 200 + data = json.loads(resp.data.decode("utf-8")) + assert value == data.get("value") + + +def test_get_access_denied(client, chart_id: int, dataset_id: int): + login(client, "gamma") + resp = client.get( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) + assert resp.status_code == 404 + + +def test_get_no_dataset(client): + login(client, "admin") + resp = client.get(f"api/v1/chart/0/form_data/{key}") + assert resp.status_code == 404 + + +def test_get_dataset(client, dataset_id: int): + login(client, "admin") + resp = client.get(f"api/v1/chart/0/form_data/{key}?dataset_id={dataset_id}") + assert resp.status_code == 200 + + +def test_get_dataset_not_found(client): + login(client, "admin") + resp = client.get(f"api/v1/chart/0/form_data/{key}?dataset_id=-1") + assert resp.status_code == 404 + + +@patch("superset.security.SupersetSecurityManager.can_access_datasource") +def test_get_dataset_access_denied(mock_can_access_datasource, client, dataset_id): + mock_can_access_datasource.side_effect = DatasetAccessDeniedError() + login(client, "admin") + resp = client.get(f"api/v1/chart/0/form_data/{key}?dataset_id={dataset_id}") + assert resp.status_code == 403 + + +def test_delete(client, chart_id: int, dataset_id: int): + login(client, "admin") + resp = client.delete( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) + assert resp.status_code == 200 + + +def test_delete_access_denied(client, chart_id: int, dataset_id: int): + login(client, "gamma") + resp = client.delete( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) + assert resp.status_code == 404 + + +def test_delete_not_owner(client, chart_id: int, dataset_id: int, admin_id: int): + another_key = "another_key" + another_owner = admin_id + 1 + entry: Entry = {"owner": another_owner, "value": value} + cache_manager.chart_form_data_cache.set(cache_key(chart_id, another_key), entry) + login(client, "admin") + resp = client.delete( + f"api/v1/chart/{chart_id}/form_data/{another_key}?dataset_id={dataset_id}" + ) + assert resp.status_code == 403 diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index ae667ec73423..bec6dfc5f220 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -22,8 +22,8 @@ from sqlalchemy.orm import Session from superset.dashboards.commands.exceptions import DashboardAccessDeniedError -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager +from superset.key_value.commands.entry import Entry from superset.key_value.utils import cache_key from superset.models.dashboard import Dashboard from tests.integration_tests.base_tests import login diff --git a/tests/unit_tests/charts/form_data/utils_test.py b/tests/unit_tests/charts/form_data/utils_test.py new file mode 100644 index 000000000000..1180b10d36ed --- /dev/null +++ b/tests/unit_tests/charts/form_data/utils_test.py @@ -0,0 +1,186 @@ +# 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.ctx import AppContext +from flask_appbuilder.security.sqla.models import User +from pytest import raises +from pytest_mock import MockFixture + +from superset.charts.commands.exceptions import ( + ChartAccessDeniedError, + ChartNotFoundError, +) +from superset.datasets.commands.exceptions import ( + DatasetAccessDeniedError, + DatasetNotFoundError, +) +from superset.key_value.commands.parameters import CommandParameters + +dataset_find_by_id = "superset.datasets.dao.DatasetDAO.find_by_id" +chart_find_by_id = "superset.charts.dao.ChartDAO.find_by_id" +is_user_admin = "superset.charts.form_data.utils.is_user_admin" +is_owner = "superset.charts.form_data.utils.is_owner" +can_access_datasource = ( + "superset.security.SupersetSecurityManager.can_access_datasource" +) +can_access = "superset.security.SupersetSecurityManager.can_access" + + +def test_unsaved_chart_no_dataset_id(app_context: AppContext) -> None: + from superset.charts.form_data.utils import check_access + + with raises(DatasetNotFoundError): + cmd_params = CommandParameters(resource_id=0, actor=User(), query_params={}) + check_access(cmd_params) + + +def test_unsaved_chart_unknown_dataset_id( + mocker: MockFixture, app_context: AppContext +) -> None: + from superset.charts.form_data.utils import check_access + + with raises(DatasetNotFoundError): + mocker.patch(dataset_find_by_id, return_value=None) + cmd_params = CommandParameters( + resource_id=0, actor=User(), query_params={"dataset_id": "1"} + ) + check_access(cmd_params) + + +def test_unsaved_chart_unauthorized_dataset( + mocker: MockFixture, app_context: AppContext +) -> None: + from superset.charts.form_data import utils + from superset.connectors.sqla.models import SqlaTable + + with raises(DatasetAccessDeniedError): + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=False) + cmd_params = CommandParameters( + resource_id=0, actor=User(), query_params={"dataset_id": "1"} + ) + utils.check_access(cmd_params) + + +def test_unsaved_chart_authorized_dataset( + mocker: MockFixture, app_context: AppContext +) -> None: + from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable + + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) + cmd_params = CommandParameters( + resource_id=0, actor=User(), query_params={"dataset_id": "1"} + ) + assert check_access(cmd_params) == True + + +def test_saved_chart_unknown_chart_id( + mocker: MockFixture, app_context: AppContext +) -> None: + from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable + + with raises(ChartNotFoundError): + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) + mocker.patch(chart_find_by_id, return_value=None) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) + check_access(cmd_params) + + +def test_saved_chart_unauthorized_dataset( + mocker: MockFixture, app_context: AppContext +) -> None: + from superset.charts.form_data import utils + from superset.connectors.sqla.models import SqlaTable + + with raises(DatasetAccessDeniedError): + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=False) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) + utils.check_access(cmd_params) + + +def test_saved_chart_is_admin(mocker: MockFixture, app_context: AppContext) -> None: + from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable + from superset.models.slice import Slice + + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) + mocker.patch(is_user_admin, return_value=True) + mocker.patch(chart_find_by_id, return_value=Slice()) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) + assert check_access(cmd_params) == True + + +def test_saved_chart_is_owner(mocker: MockFixture, app_context: AppContext) -> None: + from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable + from superset.models.slice import Slice + + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) + mocker.patch(is_user_admin, return_value=False) + mocker.patch(is_owner, return_value=True) + mocker.patch(chart_find_by_id, return_value=Slice()) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) + assert check_access(cmd_params) == True + + +def test_saved_chart_has_access(mocker: MockFixture, app_context: AppContext) -> None: + from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable + from superset.models.slice import Slice + + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) + mocker.patch(is_user_admin, return_value=False) + mocker.patch(is_owner, return_value=False) + mocker.patch(can_access, return_value=True) + mocker.patch(chart_find_by_id, return_value=Slice()) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) + assert check_access(cmd_params) == True + + +def test_saved_chart_no_access(mocker: MockFixture, app_context: AppContext) -> None: + from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable + from superset.models.slice import Slice + + with raises(ChartAccessDeniedError): + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) + mocker.patch(is_user_admin, return_value=False) + mocker.patch(is_owner, return_value=False) + mocker.patch(can_access, return_value=False) + mocker.patch(chart_find_by_id, return_value=Slice()) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) + check_access(cmd_params)