fix(Fave): Charts and Dashboards fave/unfave do not commit transactions (#30215)

superset/charts/api.py

@@ -66,7 +66,9 @@ from superset.commands.chart.exceptions import (
     DashboardsForbiddenError,
 )
 from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.fave import AddFavoriteChartCommand
 from superset.commands.chart.importers.dispatcher import ImportChartsCommand
+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
@@ -898,11 +900,13 @@ class ChartRestApi(BaseSupersetModelRestApi):
             500:
               $ref: '#/components/responses/500'
         """
-        chart = ChartDAO.find_by_id(pk)
-        if not chart:
+        try:
+            AddFavoriteChartCommand(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("/<pk>/favorites/", methods=("DELETE",))
@@ -941,11 +945,13 @@ class ChartRestApi(BaseSupersetModelRestApi):
             500:
               $ref: '#/components/responses/500'
         """
-        chart = ChartDAO.find_by_id(pk)
-        if not chart:
-            return self.response_404()
+        try:
+            DelFavoriteChartCommand(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",))

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")

superset/commands/chart/fave.py

@@ -0,0 +1,57 @@
+# 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 requests_cache import Optional
+
+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
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class AddFavoriteChartCommand(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

superset/commands/chart/unfave.py

@@ -0,0 +1,57 @@
+# 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 requests_cache import Optional
+
+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
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class DelFavoriteChartCommand(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

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.")

superset/commands/dashboard/fave.py

@@ -0,0 +1,46 @@
+# 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 requests_cache import Optional
+
+from superset.commands.base import BaseCommand
+from superset.commands.dashboard.exceptions import (
+    DashboardFaveError,
+)
+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 AddFavoriteDashboardCommand(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

superset/commands/dashboard/unfave.py

@@ -0,0 +1,46 @@
+# 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 requests_cache import Optional
+
+from superset.commands.base import BaseCommand
+from superset.commands.dashboard.exceptions import (
+    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 DelFavoriteDashboardCommand(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

superset/dashboards/api.py

@@ -51,8 +51,10 @@ from superset.commands.dashboard.exceptions import (
     DashboardUpdateFailedError,
 )
 from superset.commands.dashboard.export import ExportDashboardsCommand
+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 DelFavoriteDashboardCommand
 from superset.commands.dashboard.update import UpdateDashboardCommand
 from superset.commands.exceptions import TagForbiddenError
 from superset.commands.importers.exceptions import NoValidFilesFoundError
@@ -1217,11 +1219,14 @@ class DashboardRestApi(BaseSupersetModelRestApi):
             500:
               $ref: '#/components/responses/500'
         """
-        dashboard = DashboardDAO.find_by_id(pk)
-        if not dashboard:
-            return self.response_404()
+        try:
+            AddFavoriteDashboardCommand(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("/<pk>/favorites/", methods=("DELETE",))
@@ -1260,11 +1265,13 @@ class DashboardRestApi(BaseSupersetModelRestApi):
             500:
               $ref: '#/components/responses/500'
         """
-        dashboard = DashboardDAO.find_by_id(pk)
-        if not dashboard:
+        try:
+            DelFavoriteDashboardCommand(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",))

tests/integration_tests/charts/commands_tests.py

@@ -23,19 +23,24 @@ from flask import g  # noqa: F401
 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 AddFavoriteChartCommand
 from superset.commands.chart.importers.v1 import ImportChartsCommand
+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
 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 +433,58 @@ class TestChartWarmUpCacheCommand(SupersetTestCase):
         self.assertEqual(
             result, {"chart_id": slc.id, "viz_error": None, "viz_status": "success"}
         )
+
+
+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"""
+        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")):
+                AddFavoriteChartCommand(example_chart.id).run()
+
+                # Assert that the dashboard was faved
+                ids = ChartDAO.favorited_ids([example_chart])
+                assert example_chart.id in ids
+
+                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()

tests/integration_tests/dashboards/commands_tests.py

@@ -25,6 +25,7 @@ from superset import db, security_manager
 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,10 +35,13 @@ from superset.commands.dashboard.export import (
     ExportDashboardsCommand,
     get_default_position,
 )
+from superset.commands.dashboard.fave import AddFavoriteDashboardCommand
 from superset.commands.dashboard.importers import v0, v1
+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
+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 +763,67 @@ class TestDeleteEmbeddedDashboardCommand(SupersetTestCase):
                 .one_or_none()
             )
             assert deleted_embedded_dashboard is None
+
+
+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"""
+        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,
+                ):
+                    AddFavoriteDashboardCommand(example_dashboard.id).run()
+
+                    # Assert that the dashboard was faved
+                    ids = DashboardDAO.favorited_ids([example_dashboard])
+                    assert example_dashboard.id in ids
+
+                    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()