fix(Dashboard): Copying a Dashboard does not commit the transaction (#29776)

Author: geido

superset/commands/dashboard/copy.py

@@ -0,0 +1,53 @@
+# 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 superset import is_feature_enabled, security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.dashboard.exceptions import (
+    DashboardCopyError,
+    DashboardForbiddenError,
+    DashboardInvalidError,
+)
+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 CopyDashboardCommand(BaseCommand):
+    def __init__(self, original_dash: Dashboard, data: dict[str, Any]) -> None:
+        self._original_dash = original_dash
+        self._properties = data.copy()
+
+    @transaction(on_error=partial(on_error, reraise=DashboardCopyError))
+    def run(self) -> Dashboard:
+        self.validate()
+        return DashboardDAO.copy_dashboard(self._original_dash, self._properties)
+
+    def validate(self) -> None:
+        if not self._properties.get("dashboard_title") or not self._properties.get(
+            "json_metadata"
+        ):
+            raise DashboardInvalidError()
+        if is_feature_enabled("DASHBOARD_RBAC") and not security_manager.is_owner(
+            self._original_dash
+        ):
+            raise DashboardForbiddenError()

superset/commands/dashboard/exceptions.py

@@ -76,3 +76,7 @@ class DashboardImportError(ImportFailedError):
 
 class DashboardAccessDeniedError(ForbiddenError):
     message = _("You don't have access to this dashboard.")
+
+
+class DashboardCopyError(CommandInvalidError):
+    message = _("Dashboard cannot be copied due to invalid parameters.")

superset/dashboards/api.py

@@ -34,10 +34,12 @@
 
 from superset import db, is_feature_enabled, thumbnail_cache
 from superset.charts.schemas import ChartEntityResponseSchema
+from superset.commands.dashboard.copy import CopyDashboardCommand
 from superset.commands.dashboard.create import CreateDashboardCommand
 from superset.commands.dashboard.delete import DeleteDashboardCommand
 from superset.commands.dashboard.exceptions import (
     DashboardAccessDeniedError,
+    DashboardCopyError,
     DashboardCreateFailedError,
     DashboardDeleteFailedError,
     DashboardForbiddenError,
@@ -1606,9 +1608,11 @@ def copy_dash(self, original_dash: Dashboard) -> Response:
             return self.response_400(message=error.messages)
 
         try:
-            dash = DashboardDAO.copy_dashboard(original_dash, data)
+            dash = CopyDashboardCommand(original_dash, data).run()
         except DashboardForbiddenError:
             return self.response_403()
+        except DashboardCopyError:
+            return self.response_400()
 
         return self.response(
             200,

tests/integration_tests/dashboards/commands_tests.py

@@ -22,7 +22,12 @@
 from werkzeug.utils import secure_filename
 
 from superset import db, security_manager
-from superset.commands.dashboard.exceptions import DashboardNotFoundError
+from superset.commands.dashboard.copy import CopyDashboardCommand
+from superset.commands.dashboard.exceptions import (
+    DashboardForbiddenError,
+    DashboardInvalidError,
+    DashboardNotFoundError,
+)
 from superset.commands.dashboard.export import (
     append_charts,
     ExportDashboardsCommand,
@@ -36,6 +41,7 @@
 from superset.models.dashboard import Dashboard
 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.importexport import (
     chart_config,
@@ -660,3 +666,57 @@ def test_import_v1_dashboard_validation(self):
                 "table_name": ["Missing data for required field."],
             }
         }
+
+
+class TestCopyDashboardCommand(SupersetTestCase):
+    @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+    def test_copy_dashboard_command(self):
+        """Test that an admin user can copy a dashboard"""
+        with self.client.application.test_request_context():
+            example_dashboard = (
+                db.session.query(Dashboard).filter_by(slug="world_health").one()
+            )
+            copy_data = {"dashboard_title": "Copied Dashboard", "json_metadata": "{}"}
+
+            with override_user(security_manager.find_user("admin")):
+                command = CopyDashboardCommand(example_dashboard, copy_data)
+                copied_dashboard = command.run()
+
+            assert copied_dashboard.dashboard_title == "Copied Dashboard"
+            assert copied_dashboard.slug != example_dashboard.slug
+            assert copied_dashboard.slices == example_dashboard.slices
+
+            db.session.delete(copied_dashboard)
+            db.session.commit()
+
+    @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+    def test_copy_dashboard_command_no_access(self):
+        """Test that a non-owner user cannot copy a dashboard if DASHBOARD_RBAC is enabled"""
+        with self.client.application.test_request_context():
+            example_dashboard = (
+                db.session.query(Dashboard).filter_by(slug="world_health").one()
+            )
+            copy_data = {"dashboard_title": "Copied Dashboard", "json_metadata": "{}"}
+
+            with override_user(security_manager.find_user("gamma")):
+                with patch(
+                    "superset.commands.dashboard.copy.is_feature_enabled",
+                    return_value=True,
+                ):
+                    command = CopyDashboardCommand(example_dashboard, copy_data)
+                    with self.assertRaises(DashboardForbiddenError):
+                        command.run()
+
+    @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+    def test_copy_dashboard_command_invalid_data(self):
+        """Test that invalid data raises a DashboardInvalidError"""
+        with self.client.application.test_request_context():
+            example_dashboard = (
+                db.session.query(Dashboard).filter_by(slug="world_health").one()
+            )
+        invalid_copy_data = {"dashboard_title": "", "json_metadata": "{}"}
+
+        with override_user(security_manager.find_user("admin")):
+            command = CopyDashboardCommand(example_dashboard, invalid_copy_data)
+            with self.assertRaises(DashboardInvalidError):
+                command.run()