fix: Allow non-owners to fave/unfave charts (#38095)

superset/commands/chart/fave.py

@@ -17,15 +17,12 @@
 import logging
 from functools import partial
 
-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
 
@@ -48,9 +45,4 @@ class AddFavoriteChartCommand(BaseCommand):
         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

@@ -17,15 +17,12 @@
 import logging
 from functools import partial
 
-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
 
@@ -48,9 +45,4 @@ class DelFavoriteChartCommand(BaseCommand):
         if not chart:
             raise ChartNotFoundError()
 
-        try:
-            security_manager.raise_for_ownership(chart)
-        except SupersetSecurityException as ex:
-            raise ChartForbiddenError() from ex
-
         self._chart = chart

tests/integration_tests/charts/commands_tests.py

@@ -25,7 +25,6 @@ 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,
 )
@@ -668,8 +667,8 @@ class TestFavoriteChartCommand(SupersetTestCase):
 
     @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"""  # noqa: E501
+    def test_fave_unfave_chart_command_non_owner(self, mock_find_by_id):
+        """Test that faving / unfaving a chart the user doesn't own works properly"""  # noqa: E501
         with self.client.application.test_request_context():
             example_chart = db.session.query(Slice).all()[0]
             mock_find_by_id.return_value = example_chart
@@ -678,8 +677,12 @@ class TestFavoriteChartCommand(SupersetTestCase):
             assert example_chart is not None
 
             with override_user(security_manager.find_user("gamma")):
-                with self.assertRaises(ChartForbiddenError):  # noqa: PT027
-                    AddFavoriteChartCommand(example_chart.id).run()
+                AddFavoriteChartCommand(example_chart.id).run()
+                ids = ChartDAO.favorited_ids([example_chart])
 
-                with self.assertRaises(ChartForbiddenError):  # noqa: PT027
-                    DelFavoriteChartCommand(example_chart.id).run()
+                assert example_chart.id in ids
+
+                DelFavoriteChartCommand(example_chart.id).run()
+                ids = ChartDAO.favorited_ids([example_chart])
+
+                assert example_chart.id not in ids