From 422ae04dfb2cb8268e3d692445abf77efd8e1ca6 Mon Sep 17 00:00:00 2001 From: Diego Pucci Date: Tue, 10 Sep 2024 17:58:14 +0200 Subject: [PATCH 1/3] fix(Fave): Charts and Dashboards fave/unfave do not commit transactions --- superset/charts/api.py | 20 ++++--- superset/commands/chart/exceptions.py | 8 +++ superset/commands/chart/fave.py | 59 +++++++++++++++++++ superset/commands/chart/unfave.py | 59 +++++++++++++++++++ superset/commands/dashboard/exceptions.py | 8 +++ superset/commands/dashboard/fave.py | 49 +++++++++++++++ superset/commands/dashboard/unfave.py | 49 +++++++++++++++ superset/dashboards/api.py | 19 ++++-- .../charts/commands_tests.py | 28 +++++++++ .../dashboards/commands_tests.py | 33 +++++++++++ 10 files changed, 319 insertions(+), 13 deletions(-) create mode 100644 superset/commands/chart/fave.py create mode 100644 superset/commands/chart/unfave.py create mode 100644 superset/commands/dashboard/fave.py create mode 100644 superset/commands/dashboard/unfave.py diff --git a/superset/charts/api.py b/superset/charts/api.py index 57a812fb74aba..02c1418323d61 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -66,7 +66,9 @@ DashboardsForbiddenError, ) from superset.commands.chart.export import ExportChartsCommand +from superset.commands.chart.fave import FaveChartCommand from superset.commands.chart.importers.dispatcher import ImportChartsCommand +from superset.commands.chart.unfave import UnfaveChartCommand from superset.commands.chart.update import UpdateChartCommand from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand from superset.commands.exceptions import CommandException, TagForbiddenError @@ -898,11 +900,13 @@ def add_favorite(self, pk: int) -> Response: 500: $ref: '#/components/responses/500' """ - chart = ChartDAO.find_by_id(pk) - if not chart: + try: + FaveChartCommand(pk).run() + except ChartNotFoundError: return self.response_404() + except ChartForbiddenError: + return self.response_403() - ChartDAO.add_favorite(chart) return self.response(200, result="OK") @expose("//favorites/", methods=("DELETE",)) @@ -941,11 +945,13 @@ def remove_favorite(self, pk: int) -> Response: 500: $ref: '#/components/responses/500' """ - chart = ChartDAO.find_by_id(pk) - if not chart: - return self.response_404() + try: + UnfaveChartCommand(pk).run() + except ChartNotFoundError: + self.response_404() + except ChartForbiddenError: + self.response_403() - ChartDAO.remove_favorite(chart) return self.response(200, result="OK") @expose("/warm_up_cache", methods=("PUT",)) diff --git a/superset/commands/chart/exceptions.py b/superset/commands/chart/exceptions.py index 00877aa803070..72ef71f466e8e 100644 --- a/superset/commands/chart/exceptions.py +++ b/superset/commands/chart/exceptions.py @@ -154,3 +154,11 @@ class DashboardsForbiddenError(ForbiddenError): class WarmUpCacheChartNotFoundError(CommandException): status = 404 message = _("Chart not found") + + +class ChartFaveError(CommandException): + message = _("Error faving chart") + + +class ChartUnfaveError(CommandException): + message = _("Error unfaving chart") diff --git a/superset/commands/chart/fave.py b/superset/commands/chart/fave.py new file mode 100644 index 0000000000000..57efc4fbb3bdb --- /dev/null +++ b/superset/commands/chart/fave.py @@ -0,0 +1,59 @@ +# 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 functools import partial +from typing import Any + +from requests_cache import Optional + +from superset import is_feature_enabled, security_manager +from superset.commands.base import BaseCommand +from superset.commands.chart.exceptions import ( + ChartFaveError, + ChartForbiddenError, + ChartNotFoundError, +) + +from superset.daos.chart import ChartDAO +from superset.exceptions import SupersetSecurityException +from superset.models.slice import Slice +from superset.utils.decorators import on_error, transaction + +logger = logging.getLogger(__name__) + + +class FaveChartCommand(BaseCommand): + def __init__(self, chart_id: int) -> None: + self._chart_id = chart_id + self._chart: Optional[Slice] = None + + @transaction(on_error=partial(on_error, reraise=ChartFaveError)) + def run(self) -> None: + self.validate() + return ChartDAO.add_favorite(self._chart) + + def validate(self) -> None: + chart = ChartDAO.find_by_id(self._chart_id) + if not chart: + raise ChartNotFoundError() + + try: + security_manager.raise_for_ownership(chart) + except SupersetSecurityException as ex: + raise ChartForbiddenError() from ex + + self._chart = chart diff --git a/superset/commands/chart/unfave.py b/superset/commands/chart/unfave.py new file mode 100644 index 0000000000000..618aba693189c --- /dev/null +++ b/superset/commands/chart/unfave.py @@ -0,0 +1,59 @@ +# 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 functools import partial +from typing import Any + +from requests_cache import Optional + +from superset import is_feature_enabled, security_manager +from superset.commands.base import BaseCommand +from superset.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, + ChartUnfaveError, +) + +from superset.daos.chart import ChartDAO +from superset.exceptions import SupersetSecurityException +from superset.models.slice import Slice +from superset.utils.decorators import on_error, transaction + +logger = logging.getLogger(__name__) + + +class UnfaveChartCommand(BaseCommand): + def __init__(self, chart_id: int) -> None: + self._chart_id = chart_id + self._chart: Optional[Slice] = None + + @transaction(on_error=partial(on_error, reraise=ChartUnfaveError)) + def run(self) -> None: + self.validate() + return ChartDAO.remove_favorite(self._chart) + + def validate(self) -> None: + chart = ChartDAO.find_by_id(self._chart_id) + if not chart: + raise ChartNotFoundError() + + try: + security_manager.raise_for_ownership(chart) + except SupersetSecurityException as ex: + raise ChartForbiddenError() from ex + + self._chart = chart diff --git a/superset/commands/dashboard/exceptions.py b/superset/commands/dashboard/exceptions.py index 91a4ec6832222..9281119b320bf 100644 --- a/superset/commands/dashboard/exceptions.py +++ b/superset/commands/dashboard/exceptions.py @@ -84,3 +84,11 @@ class DashboardAccessDeniedError(ForbiddenError): class DashboardCopyError(CommandInvalidError): message = _("Dashboard cannot be copied due to invalid parameters.") + + +class DashboardFaveError(CommandInvalidError): + message = _("Dashboard cannot be favorited.") + + +class DashboardUnfaveError(CommandInvalidError): + message = _("Dashboard cannot be unfavorited.") diff --git a/superset/commands/dashboard/fave.py b/superset/commands/dashboard/fave.py new file mode 100644 index 0000000000000..db2b0fc586f3b --- /dev/null +++ b/superset/commands/dashboard/fave.py @@ -0,0 +1,49 @@ +# 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 functools import partial +from typing import Any + +from requests_cache import Optional + +from superset import is_feature_enabled, security_manager +from superset.commands.base import BaseCommand +from superset.commands.dashboard.exceptions import ( + DashboardFaveError, + DashboardNotFoundError, +) +from superset.daos.dashboard import DashboardDAO +from superset.models.dashboard import Dashboard +from superset.utils.decorators import on_error, transaction + +logger = logging.getLogger(__name__) + + +class FaveDashboardCommand(BaseCommand): + def __init__(self, dashboard_id: int) -> None: + self._dashboard_id = dashboard_id + self._dashboard: Optional[Dashboard] = None + + @transaction(on_error=partial(on_error, reraise=DashboardFaveError)) + def run(self) -> None: + self.validate() + return DashboardDAO.add_favorite(self._dashboard) + + def validate(self) -> None: + # Raises DashboardNotFoundError or DashboardAccessDeniedError + dashboard = DashboardDAO.get_by_id_or_slug(self._dashboard_id) + self._dashboard = dashboard diff --git a/superset/commands/dashboard/unfave.py b/superset/commands/dashboard/unfave.py new file mode 100644 index 0000000000000..8caa86c7c324e --- /dev/null +++ b/superset/commands/dashboard/unfave.py @@ -0,0 +1,49 @@ +# 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 functools import partial +from typing import Any + +from requests_cache import Optional + +from superset import is_feature_enabled, security_manager +from superset.commands.base import BaseCommand +from superset.commands.dashboard.exceptions import ( + DashboardNotFoundError, + DashboardUnfaveError, +) +from superset.daos.dashboard import DashboardDAO +from superset.models.dashboard import Dashboard +from superset.utils.decorators import on_error, transaction + +logger = logging.getLogger(__name__) + + +class UnfaveDashboardCommand(BaseCommand): + def __init__(self, dashboard_id: int) -> None: + self._dashboard_id = dashboard_id + self._dashboard: Optional[Dashboard] = None + + @transaction(on_error=partial(on_error, reraise=DashboardUnfaveError)) + def run(self) -> None: + self.validate() + return DashboardDAO.remove_favorite(self._dashboard) + + def validate(self) -> None: + # Raises DashboardNotFoundError or DashboardAccessDeniedError + dashboard = DashboardDAO.get_by_id_or_slug(self._dashboard_id) + self._dashboard = dashboard diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 193901b48139f..33ce11ec668c9 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -51,8 +51,10 @@ DashboardUpdateFailedError, ) from superset.commands.dashboard.export import ExportDashboardsCommand +from superset.commands.dashboard.fave import FaveDashboardCommand from superset.commands.dashboard.importers.dispatcher import ImportDashboardsCommand from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand +from superset.commands.dashboard.unfave import UnfaveDashboardCommand from superset.commands.dashboard.update import UpdateDashboardCommand from superset.commands.exceptions import TagForbiddenError from superset.commands.importers.exceptions import NoValidFilesFoundError @@ -1212,11 +1214,14 @@ def add_favorite(self, pk: int) -> Response: 500: $ref: '#/components/responses/500' """ - dashboard = DashboardDAO.find_by_id(pk) - if not dashboard: + try: + FaveDashboardCommand(pk).run() + + except DashboardNotFoundError: return self.response_404() + except DashboardAccessDeniedError: + return self.response_403() - DashboardDAO.add_favorite(dashboard) return self.response(200, result="OK") @expose("//favorites/", methods=("DELETE",)) @@ -1255,11 +1260,13 @@ def remove_favorite(self, pk: int) -> Response: 500: $ref: '#/components/responses/500' """ - dashboard = DashboardDAO.find_by_id(pk) - if not dashboard: + try: + UnfaveDashboardCommand(pk).run() + except DashboardNotFoundError: return self.response_404() + except DashboardAccessDeniedError: + return self.response_403() - DashboardDAO.remove_favorite(dashboard) return self.response(200, result="OK") @expose("/import/", methods=("POST",)) diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index d8522473d09f2..4c0399f0dd753 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -27,15 +27,19 @@ WarmUpCacheChartNotFoundError, ) from superset.commands.chart.export import ExportChartsCommand +from superset.commands.chart.fave import FaveChartCommand from superset.commands.chart.importers.v1 import ImportChartsCommand +from superset.commands.chart.unfave import UnfaveChartCommand from superset.commands.chart.update import UpdateChartCommand from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand from superset.commands.exceptions import CommandInvalidError from superset.commands.importers.exceptions import IncorrectVersionError from superset.connectors.sqla.models import SqlaTable +from superset.daos.chart import ChartDAO from superset.models.core import Database from superset.models.slice import Slice from superset.utils import json +from superset.utils.core import override_user from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, # noqa: F401 @@ -428,3 +432,27 @@ def test_warm_up_cache(self): self.assertEqual( result, {"chart_id": slc.id, "viz_error": None, "viz_status": "success"} ) + + +class TestFaveChartCommand(SupersetTestCase): + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_fave_unfave_chart_command(self): + """Test that a user can fave/unfave a chart""" + with self.client.application.test_request_context(): + example_chart = db.session.query(Slice).all()[0] + + # Assert that the chart exists + assert example_chart is not None + + with override_user(security_manager.find_user("admin")): + FaveChartCommand(example_chart.id).run() + + # Assert that the dashboard was faved + ids = ChartDAO.favorited_ids([example_chart]) + assert example_chart.id in ids + + UnfaveChartCommand(example_chart.id).run() + + # Assert that the chart was unfaved + ids = ChartDAO.favorited_ids([example_chart]) + assert example_chart.id not in ids diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py index f38774a1b4ff9..9cbf2b14d83a7 100644 --- a/tests/integration_tests/dashboards/commands_tests.py +++ b/tests/integration_tests/dashboards/commands_tests.py @@ -34,10 +34,13 @@ ExportDashboardsCommand, get_default_position, ) +from superset.commands.dashboard.fave import FaveDashboardCommand from superset.commands.dashboard.importers import v0, v1 +from superset.commands.dashboard.unfave import UnfaveDashboardCommand from superset.commands.exceptions import CommandInvalidError from superset.commands.importers.exceptions import IncorrectVersionError from superset.connectors.sqla.models import SqlaTable +from superset.daos.dashboard import DashboardDAO from superset.models.core import Database from superset.models.dashboard import Dashboard from superset.models.embedded_dashboard import EmbeddedDashboard @@ -759,3 +762,33 @@ def test_delete_embedded_dashboard_command(self): .one_or_none() ) assert deleted_embedded_dashboard is None + + +class TestFaveDashboardCommand(SupersetTestCase): + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_fave_unfave_dashboard_command(self): + """Test that a user can fave/unfave a dashboard""" + with self.client.application.test_request_context(): + example_dashboard = ( + db.session.query(Dashboard).filter_by(slug="world_health").one() + ) + + # Assert that the dashboard exists + assert example_dashboard is not None + + with override_user(security_manager.find_user("admin")): + with patch( + "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug", + return_value=example_dashboard, + ): + FaveDashboardCommand(example_dashboard.id).run() + + # Assert that the dashboard was faved + ids = DashboardDAO.favorited_ids([example_dashboard]) + assert example_dashboard.id in ids + + UnfaveDashboardCommand(example_dashboard.id).run() + + # Assert that the dashboard was unfaved + ids = DashboardDAO.favorited_ids([example_dashboard]) + assert example_dashboard.id not in ids From ea3daf14d521636548494213567b25c0564f2b63 Mon Sep 17 00:00:00 2001 From: Diego Pucci Date: Tue, 10 Sep 2024 18:07:33 +0200 Subject: [PATCH 2/3] fix(Ruff): Lint --- superset/commands/chart/fave.py | 8 +++----- superset/commands/chart/unfave.py | 8 +++----- superset/commands/dashboard/fave.py | 3 --- superset/commands/dashboard/unfave.py | 3 --- 4 files changed, 6 insertions(+), 16 deletions(-) diff --git a/superset/commands/chart/fave.py b/superset/commands/chart/fave.py index 57efc4fbb3bdb..564961a43d4b6 100644 --- a/superset/commands/chart/fave.py +++ b/superset/commands/chart/fave.py @@ -16,18 +16,16 @@ # under the License. import logging from functools import partial -from typing import Any from requests_cache import Optional -from superset import is_feature_enabled, security_manager +from superset import security_manager from superset.commands.base import BaseCommand from superset.commands.chart.exceptions import ( ChartFaveError, ChartForbiddenError, ChartNotFoundError, ) - from superset.daos.chart import ChartDAO from superset.exceptions import SupersetSecurityException from superset.models.slice import Slice @@ -45,12 +43,12 @@ def __init__(self, chart_id: int) -> None: def run(self) -> None: self.validate() return ChartDAO.add_favorite(self._chart) - + def validate(self) -> None: chart = ChartDAO.find_by_id(self._chart_id) if not chart: raise ChartNotFoundError() - + try: security_manager.raise_for_ownership(chart) except SupersetSecurityException as ex: diff --git a/superset/commands/chart/unfave.py b/superset/commands/chart/unfave.py index 618aba693189c..2c0ac5d23f6ca 100644 --- a/superset/commands/chart/unfave.py +++ b/superset/commands/chart/unfave.py @@ -16,18 +16,16 @@ # under the License. import logging from functools import partial -from typing import Any from requests_cache import Optional -from superset import is_feature_enabled, security_manager +from superset import security_manager from superset.commands.base import BaseCommand from superset.commands.chart.exceptions import ( ChartForbiddenError, ChartNotFoundError, ChartUnfaveError, ) - from superset.daos.chart import ChartDAO from superset.exceptions import SupersetSecurityException from superset.models.slice import Slice @@ -45,12 +43,12 @@ def __init__(self, chart_id: int) -> None: def run(self) -> None: self.validate() return ChartDAO.remove_favorite(self._chart) - + def validate(self) -> None: chart = ChartDAO.find_by_id(self._chart_id) if not chart: raise ChartNotFoundError() - + try: security_manager.raise_for_ownership(chart) except SupersetSecurityException as ex: diff --git a/superset/commands/dashboard/fave.py b/superset/commands/dashboard/fave.py index db2b0fc586f3b..4c1659d1cba0b 100644 --- a/superset/commands/dashboard/fave.py +++ b/superset/commands/dashboard/fave.py @@ -16,15 +16,12 @@ # under the License. import logging from functools import partial -from typing import Any from requests_cache import Optional -from superset import is_feature_enabled, security_manager from superset.commands.base import BaseCommand from superset.commands.dashboard.exceptions import ( DashboardFaveError, - DashboardNotFoundError, ) from superset.daos.dashboard import DashboardDAO from superset.models.dashboard import Dashboard diff --git a/superset/commands/dashboard/unfave.py b/superset/commands/dashboard/unfave.py index 8caa86c7c324e..6d8f46c0d5f20 100644 --- a/superset/commands/dashboard/unfave.py +++ b/superset/commands/dashboard/unfave.py @@ -16,14 +16,11 @@ # under the License. import logging from functools import partial -from typing import Any from requests_cache import Optional -from superset import is_feature_enabled, security_manager from superset.commands.base import BaseCommand from superset.commands.dashboard.exceptions import ( - DashboardNotFoundError, DashboardUnfaveError, ) from superset.daos.dashboard import DashboardDAO From a9743276355d271182fc8161191182596d444745 Mon Sep 17 00:00:00 2001 From: Diego Pucci Date: Thu, 12 Sep 2024 17:50:49 +0200 Subject: [PATCH 3/3] chore(Tests): Increase test coverage --- superset/charts/api.py | 8 ++-- superset/commands/chart/fave.py | 2 +- superset/commands/chart/unfave.py | 2 +- superset/commands/dashboard/fave.py | 2 +- superset/commands/dashboard/unfave.py | 2 +- superset/dashboards/api.py | 8 ++-- .../charts/commands_tests.py | 42 ++++++++++++++--- .../dashboards/commands_tests.py | 45 ++++++++++++++++--- 8 files changed, 89 insertions(+), 22 deletions(-) diff --git a/superset/charts/api.py b/superset/charts/api.py index 02c1418323d61..64177950b8437 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -66,9 +66,9 @@ DashboardsForbiddenError, ) from superset.commands.chart.export import ExportChartsCommand -from superset.commands.chart.fave import FaveChartCommand +from superset.commands.chart.fave import AddFavoriteChartCommand from superset.commands.chart.importers.dispatcher import ImportChartsCommand -from superset.commands.chart.unfave import UnfaveChartCommand +from superset.commands.chart.unfave import DelFavoriteChartCommand from superset.commands.chart.update import UpdateChartCommand from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand from superset.commands.exceptions import CommandException, TagForbiddenError @@ -901,7 +901,7 @@ def add_favorite(self, pk: int) -> Response: $ref: '#/components/responses/500' """ try: - FaveChartCommand(pk).run() + AddFavoriteChartCommand(pk).run() except ChartNotFoundError: return self.response_404() except ChartForbiddenError: @@ -946,7 +946,7 @@ def remove_favorite(self, pk: int) -> Response: $ref: '#/components/responses/500' """ try: - UnfaveChartCommand(pk).run() + DelFavoriteChartCommand(pk).run() except ChartNotFoundError: self.response_404() except ChartForbiddenError: diff --git a/superset/commands/chart/fave.py b/superset/commands/chart/fave.py index 564961a43d4b6..d45d1694761c3 100644 --- a/superset/commands/chart/fave.py +++ b/superset/commands/chart/fave.py @@ -34,7 +34,7 @@ logger = logging.getLogger(__name__) -class FaveChartCommand(BaseCommand): +class AddFavoriteChartCommand(BaseCommand): def __init__(self, chart_id: int) -> None: self._chart_id = chart_id self._chart: Optional[Slice] = None diff --git a/superset/commands/chart/unfave.py b/superset/commands/chart/unfave.py index 2c0ac5d23f6ca..d19d2b2769993 100644 --- a/superset/commands/chart/unfave.py +++ b/superset/commands/chart/unfave.py @@ -34,7 +34,7 @@ logger = logging.getLogger(__name__) -class UnfaveChartCommand(BaseCommand): +class DelFavoriteChartCommand(BaseCommand): def __init__(self, chart_id: int) -> None: self._chart_id = chart_id self._chart: Optional[Slice] = None diff --git a/superset/commands/dashboard/fave.py b/superset/commands/dashboard/fave.py index 4c1659d1cba0b..e3050c729dbec 100644 --- a/superset/commands/dashboard/fave.py +++ b/superset/commands/dashboard/fave.py @@ -30,7 +30,7 @@ logger = logging.getLogger(__name__) -class FaveDashboardCommand(BaseCommand): +class AddFavoriteDashboardCommand(BaseCommand): def __init__(self, dashboard_id: int) -> None: self._dashboard_id = dashboard_id self._dashboard: Optional[Dashboard] = None diff --git a/superset/commands/dashboard/unfave.py b/superset/commands/dashboard/unfave.py index 6d8f46c0d5f20..811a2cdd1e68d 100644 --- a/superset/commands/dashboard/unfave.py +++ b/superset/commands/dashboard/unfave.py @@ -30,7 +30,7 @@ logger = logging.getLogger(__name__) -class UnfaveDashboardCommand(BaseCommand): +class DelFavoriteDashboardCommand(BaseCommand): def __init__(self, dashboard_id: int) -> None: self._dashboard_id = dashboard_id self._dashboard: Optional[Dashboard] = None diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 33ce11ec668c9..aea2ca49bf110 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -51,10 +51,10 @@ DashboardUpdateFailedError, ) from superset.commands.dashboard.export import ExportDashboardsCommand -from superset.commands.dashboard.fave import FaveDashboardCommand +from superset.commands.dashboard.fave import AddFavoriteDashboardCommand from superset.commands.dashboard.importers.dispatcher import ImportDashboardsCommand from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand -from superset.commands.dashboard.unfave import UnfaveDashboardCommand +from superset.commands.dashboard.unfave import DelFavoriteDashboardCommand from superset.commands.dashboard.update import UpdateDashboardCommand from superset.commands.exceptions import TagForbiddenError from superset.commands.importers.exceptions import NoValidFilesFoundError @@ -1215,7 +1215,7 @@ def add_favorite(self, pk: int) -> Response: $ref: '#/components/responses/500' """ try: - FaveDashboardCommand(pk).run() + AddFavoriteDashboardCommand(pk).run() except DashboardNotFoundError: return self.response_404() @@ -1261,7 +1261,7 @@ def remove_favorite(self, pk: int) -> Response: $ref: '#/components/responses/500' """ try: - UnfaveDashboardCommand(pk).run() + DelFavoriteDashboardCommand(pk).run() except DashboardNotFoundError: return self.response_404() except DashboardAccessDeniedError: diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index 4c0399f0dd753..950c7cbc888d5 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -23,13 +23,14 @@ from superset import db, security_manager from superset.commands.chart.create import CreateChartCommand from superset.commands.chart.exceptions import ( + ChartForbiddenError, ChartNotFoundError, WarmUpCacheChartNotFoundError, ) from superset.commands.chart.export import ExportChartsCommand -from superset.commands.chart.fave import FaveChartCommand +from superset.commands.chart.fave import AddFavoriteChartCommand from superset.commands.chart.importers.v1 import ImportChartsCommand -from superset.commands.chart.unfave import UnfaveChartCommand +from superset.commands.chart.unfave import DelFavoriteChartCommand from superset.commands.chart.update import UpdateChartCommand from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand from superset.commands.exceptions import CommandInvalidError @@ -434,7 +435,7 @@ def test_warm_up_cache(self): ) -class TestFaveChartCommand(SupersetTestCase): +class TestFavoriteChartCommand(SupersetTestCase): @pytest.mark.usefixtures("load_energy_table_with_slice") def test_fave_unfave_chart_command(self): """Test that a user can fave/unfave a chart""" @@ -445,14 +446,45 @@ def test_fave_unfave_chart_command(self): assert example_chart is not None with override_user(security_manager.find_user("admin")): - FaveChartCommand(example_chart.id).run() + AddFavoriteChartCommand(example_chart.id).run() # Assert that the dashboard was faved ids = ChartDAO.favorited_ids([example_chart]) assert example_chart.id in ids - UnfaveChartCommand(example_chart.id).run() + DelFavoriteChartCommand(example_chart.id).run() # Assert that the chart was unfaved ids = ChartDAO.favorited_ids([example_chart]) assert example_chart.id not in ids + + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_fave_unfave_chart_command_not_found(self): + """Test that faving / unfaving a non-existing chart raises an exception""" + with self.client.application.test_request_context(): + example_chart_id = 1234 + + with override_user(security_manager.find_user("admin")): + with self.assertRaises(ChartNotFoundError): + AddFavoriteChartCommand(example_chart_id).run() + + with self.assertRaises(ChartNotFoundError): + DelFavoriteChartCommand(example_chart_id).run() + + @pytest.mark.usefixtures("load_energy_table_with_slice") + @patch("superset.daos.base.BaseDAO.find_by_id") + def test_fave_unfave_chart_command_forbidden(self, mock_find_by_id): + """Test that faving / unfaving raises an exception for a chart the user doesn't own""" + with self.client.application.test_request_context(): + example_chart = db.session.query(Slice).all()[0] + mock_find_by_id.return_value = example_chart + + # Assert that the chart exists + assert example_chart is not None + + with override_user(security_manager.find_user("gamma")): + with self.assertRaises(ChartForbiddenError): + AddFavoriteChartCommand(example_chart.id).run() + + with self.assertRaises(ChartForbiddenError): + DelFavoriteChartCommand(example_chart.id).run() diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py index 9cbf2b14d83a7..451d924cd6c6e 100644 --- a/tests/integration_tests/dashboards/commands_tests.py +++ b/tests/integration_tests/dashboards/commands_tests.py @@ -25,6 +25,7 @@ from superset.commands.dashboard.copy import CopyDashboardCommand from superset.commands.dashboard.delete import DeleteEmbeddedDashboardCommand from superset.commands.dashboard.exceptions import ( + DashboardAccessDeniedError, DashboardForbiddenError, DashboardInvalidError, DashboardNotFoundError, @@ -34,9 +35,9 @@ ExportDashboardsCommand, get_default_position, ) -from superset.commands.dashboard.fave import FaveDashboardCommand +from superset.commands.dashboard.fave import AddFavoriteDashboardCommand from superset.commands.dashboard.importers import v0, v1 -from superset.commands.dashboard.unfave import UnfaveDashboardCommand +from superset.commands.dashboard.unfave import DelFavoriteDashboardCommand from superset.commands.exceptions import CommandInvalidError from superset.commands.importers.exceptions import IncorrectVersionError from superset.connectors.sqla.models import SqlaTable @@ -764,7 +765,7 @@ def test_delete_embedded_dashboard_command(self): assert deleted_embedded_dashboard is None -class TestFaveDashboardCommand(SupersetTestCase): +class TestFavoriteDashboardCommand(SupersetTestCase): @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") def test_fave_unfave_dashboard_command(self): """Test that a user can fave/unfave a dashboard""" @@ -781,14 +782,48 @@ def test_fave_unfave_dashboard_command(self): "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug", return_value=example_dashboard, ): - FaveDashboardCommand(example_dashboard.id).run() + AddFavoriteDashboardCommand(example_dashboard.id).run() # Assert that the dashboard was faved ids = DashboardDAO.favorited_ids([example_dashboard]) assert example_dashboard.id in ids - UnfaveDashboardCommand(example_dashboard.id).run() + DelFavoriteDashboardCommand(example_dashboard.id).run() # Assert that the dashboard was unfaved ids = DashboardDAO.favorited_ids([example_dashboard]) assert example_dashboard.id not in ids + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + def test_fave_unfave_dashboard_command_not_found(self): + """Test that faving / unfaving a non-existing dashboard raises an exception""" + with self.client.application.test_request_context(): + example_dashboard_id = 1234 + + with override_user(security_manager.find_user("admin")): + with self.assertRaises(DashboardNotFoundError): + AddFavoriteDashboardCommand(example_dashboard_id).run() + + with self.assertRaises(DashboardNotFoundError): + DelFavoriteDashboardCommand(example_dashboard_id).run() + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + @patch("superset.models.dashboard.Dashboard.get") + def test_fave_unfave_dashboard_command_forbidden(self, mock_get): + """Test that faving / unfaving raises an exception for a dashboard the user doesn't own""" + with self.client.application.test_request_context(): + example_dashboard = ( + db.session.query(Dashboard).filter_by(slug="world_health").one() + ) + + mock_get.return_value = example_dashboard + + # Assert that the dashboard exists + assert example_dashboard is not None + + with override_user(security_manager.find_user("gamma")): + with self.assertRaises(DashboardAccessDeniedError): + AddFavoriteDashboardCommand(example_dashboard.uuid).run() + + with self.assertRaises(DashboardAccessDeniedError): + DelFavoriteDashboardCommand(example_dashboard.uuid).run()