fix: Chart can be added to dashboard by non-owner via save as option

superset-frontend/src/explore/components/SaveModal.tsx

@@ -125,7 +125,9 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
     if (dashboardId) {
       try {
         const result = await this.loadDashboard(dashboardId);
-        if (result) {
+        if (
+          result?.owners.some((owner: any) => owner.id === this.props.userId)
+        ) {
           this.setState({
             dashboard: { label: result.dashboard_title, value: result.id },
           });

superset/charts/api.py

@@ -43,6 +43,7 @@
     ChartInvalidError,
     ChartNotFoundError,
     ChartUpdateFailedError,
+    DashboardsForbiddenError,
 )
 from superset.charts.commands.export import ExportChartsCommand
 from superset.charts.commands.importers.dispatcher import ImportChartsCommand
@@ -314,6 +315,8 @@ def post(self) -> Response:
               $ref: '#/components/responses/400'
             401:
               $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/403'
             422:
               $ref: '#/components/responses/422'
             500:
@@ -327,6 +330,8 @@ def post(self) -> Response:
         try:
             new_model = CreateChartCommand(item).run()
             return self.response(201, id=new_model.id, result=item)
+        except DashboardsForbiddenError as ex:
+            return self.response(ex.status, message=ex.message)
         except ChartInvalidError as ex:
             return self.response_422(message=ex.normalized_messages())
         except ChartCreateFailedError as ex:

superset/charts/commands/create.py

@@ -22,9 +22,11 @@
 from flask_appbuilder.models.sqla import Model
 from marshmallow import ValidationError
 
+from superset import security_manager
 from superset.charts.commands.exceptions import (
     ChartCreateFailedError,
     ChartInvalidError,
+    DashboardsForbiddenError,
     DashboardsNotFoundValidationError,
 )
 from superset.commands.base import BaseCommand, CreateMixin
@@ -69,6 +71,9 @@ def validate(self) -> None:
         dashboards = DashboardDAO.find_by_ids(dashboard_ids)
         if len(dashboards) != len(dashboard_ids):
             exceptions.append(DashboardsNotFoundValidationError())
+        for dash in dashboards:
+            if not security_manager.is_owner(dash):
+                raise DashboardsForbiddenError()
         self._properties["dashboards"] = dashboards
 
         try:

superset/charts/commands/exceptions.py

@@ -155,6 +155,10 @@ class ChartImportError(ImportFailedError):
     message = _("Import chart failed for an unknown reason")
 
 
+class DashboardsForbiddenError(ForbiddenError):
+    message = _("Changing one or more of these dashboards is forbidden")
+
+
 class WarmUpCacheChartNotFoundError(CommandException):
     status = 404
     message = _("Chart not found")

tests/integration_tests/charts/api_tests.py

@@ -466,7 +466,7 @@ def test_create_chart(self):
             "certification_details": "Sample certification",
         }
         self.login(username="admin")
-        uri = f"api/v1/chart/"
+        uri = "api/v1/chart/"
         rv = self.post_assert_metric(uri, chart_data, "post")
         self.assertEqual(rv.status_code, 201)
         data = json.loads(rv.data.decode("utf-8"))
@@ -484,7 +484,7 @@ def test_create_simple_chart(self):
             "datasource_type": "table",
         }
         self.login(username="admin")
-        uri = f"api/v1/chart/"
+        uri = "api/v1/chart/"
         rv = self.post_assert_metric(uri, chart_data, "post")
         self.assertEqual(rv.status_code, 201)
         data = json.loads(rv.data.decode("utf-8"))
@@ -503,7 +503,7 @@ def test_create_chart_validate_owners(self):
             "owners": [1000],
         }
         self.login(username="admin")
-        uri = f"api/v1/chart/"
+        uri = "api/v1/chart/"
         rv = self.post_assert_metric(uri, chart_data, "post")
         self.assertEqual(rv.status_code, 422)
         response = json.loads(rv.data.decode("utf-8"))
@@ -521,7 +521,7 @@ def test_create_chart_validate_params(self):
             "params": '{"A:"a"}',
         }
         self.login(username="admin")
-        uri = f"api/v1/chart/"
+        uri = "api/v1/chart/"
         rv = self.post_assert_metric(uri, chart_data, "post")
         self.assertEqual(rv.status_code, 400)
 
@@ -560,6 +560,31 @@ def test_create_chart_validate_datasource(self):
             response, {"message": {"datasource_id": ["Datasource does not exist"]}}
         )
 
+    @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+    def test_create_chart_validate_user_is_dashboard_owner(self):
+        """
+        Chart API: Test create validate user is dashboard owner
+        """
+        dash = db.session.query(Dashboard).filter_by(slug="world_health").first()
+        # Must be published so that alpha user has read access to dash
+        dash.published = True
+        db.session.commit()
+        chart_data = {
+            "slice_name": "title1",
+            "datasource_id": 1,
+            "datasource_type": "table",
+            "dashboards": [dash.id],
+        }
+        self.login(username="alpha")
+        uri = "api/v1/chart/"
+        rv = self.post_assert_metric(uri, chart_data, "post")
+        self.assertEqual(rv.status_code, 403)
+        response = json.loads(rv.data.decode("utf-8"))
+        self.assertEqual(
+            response,
+            {"message": "Changing one or more of these dashboards is forbidden"},
+        )
+
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_update_chart(self):
         """