[dashboard] Fix, prevent delete and update on dashes not owned (apach…

…e#8911)

superset/views/base.py

@@ -155,6 +155,26 @@ def wraps(self, *args, **kwargs):
     return functools.update_wrapper(wraps, f)
 
 
+def check_ownership_and_item_exists(f):
+    """
+    A Decorator that checks if an object exists and is owned by the current user
+    """
+
+    def wraps(self, pk):  # pylint: disable=invalid-name
+        item = self.datamodel.get(
+            pk, self._base_filters  # pylint: disable=protected-access
+        )
+        if not item:
+            return self.response_404()
+        try:
+            check_ownership(item)
+        except SupersetSecurityException as e:
+            return self.response(403, message=str(e))
+        return f(self, item)
+
+    return functools.update_wrapper(wraps, f)
+
+
 def get_datasource_exist_error_msg(full_name):
     return __("Datasource %(name)s already exists", name=full_name)
 

superset/views/dashboard/api.py

@@ -24,11 +24,15 @@
 from marshmallow.validate import Length
 from sqlalchemy.exc import SQLAlchemyError
 
-import superset.models.core as models
 from superset import appbuilder
 from superset.exceptions import SupersetException
+from superset.models.dashboard import Dashboard
 from superset.utils import core as utils
-from superset.views.base import BaseSupersetModelRestApi, BaseSupersetSchema
+from superset.views.base import (
+    BaseSupersetModelRestApi,
+    BaseSupersetSchema,
+    check_ownership_and_item_exists,
+)
 
 from .mixin import DashboardMixin
 
@@ -67,7 +71,7 @@ def validate_slug_uniqueness(value):
     # slug is not required but must be unique
     if value:
         item = (
-            current_app.appbuilder.get_session.query(models.Dashboard.id)
+            current_app.appbuilder.get_session.query(Dashboard.id)
             .filter_by(slug=value)
             .one_or_none()
         )
@@ -123,7 +127,7 @@ class DashboardPostSchema(BaseDashboardSchema):
 
     @post_load
     def make_object(self, data):  # pylint: disable=no-self-use
-        instance = models.Dashboard()
+        instance = Dashboard()
         self.set_owners(instance, data["owners"])
         for field in data:
             if field == "owners":
@@ -157,7 +161,7 @@ def make_object(self, data):  # pylint: disable=no-self-use
 
 
 class DashboardRestApi(DashboardMixin, BaseSupersetModelRestApi):
-    datamodel = SQLAInterface(models.Dashboard)
+    datamodel = SQLAInterface(Dashboard)
 
     resource_name = "dashboard"
     allow_browser_login = True
@@ -207,6 +211,62 @@ class DashboardRestApi(DashboardMixin, BaseSupersetModelRestApi):
     }
     filter_rel_fields_field = {"owners": "first_name", "slices": "slice_name"}
 
+    @expose("/<pk>", methods=["PUT"])
+    @protect()
+    @check_ownership_and_item_exists
+    @safe
+    def put(self, item):  # pylint: disable=arguments-differ
+        """Changes a dashboard
+        ---
+        put:
+          parameters:
+          - in: path
+            schema:
+              type: integer
+            name: pk
+          requestBody:
+            description: Model schema
+            required: true
+            content:
+              application/json:
+                schema:
+                  $ref: '#/components/schemas/{{self.__class__.__name__}}.put'
+          responses:
+            200:
+              description: Item changed
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      result:
+                        $ref: '#/components/schemas/{{self.__class__.__name__}}.put'
+            400:
+              $ref: '#/components/responses/400'
+            401:
+              $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/401'
+            404:
+              $ref: '#/components/responses/404'
+            422:
+              $ref: '#/components/responses/422'
+            500:
+              $ref: '#/components/responses/500'
+        """
+        if not request.is_json:
+            self.response_400(message="Request is not JSON")
+        item = self.edit_model_schema.load(request.json, instance=item)
+        if item.errors:
+            return self.response_422(message=item.errors)
+        try:
+            self.datamodel.edit(item.data, raise_exception=True)
+            return self.response(
+                200, result=self.edit_model_schema.dump(item.data, many=False).data
+            )
+        except SQLAlchemyError as e:
+            return self.response_422(message=str(e))
+
     @expose("/", methods=["POST"])
     @protect()
     @safe
@@ -258,60 +318,43 @@ def post(self):
         except SQLAlchemyError as e:
             return self.response_422(message=str(e))
 
-    @expose("/<pk>", methods=["PUT"])
+    @expose("/<pk>", methods=["DELETE"])
     @protect()
+    @check_ownership_and_item_exists
     @safe
-    def put(self, pk):
-        """Changes a dashboard
+    def delete(self, item):  # pylint: disable=arguments-differ
+        """Delete Dashboard
         ---
-        put:
+        delete:
           parameters:
           - in: path
             schema:
               type: integer
             name: pk
-          requestBody:
-            description: Model schema
-            required: true
-            content:
-              application/json:
-                schema:
-                  $ref: '#/components/schemas/{{self.__class__.__name__}}.put'
           responses:
             200:
-              description: Item changed
+              description: Dashboard delete
               content:
                 application/json:
                   schema:
                     type: object
                     properties:
-                      result:
-                        $ref: '#/components/schemas/{{self.__class__.__name__}}.put'
-            400:
-              $ref: '#/components/responses/400'
+                      message:
+                        type: string
             401:
               $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/401'
             404:
               $ref: '#/components/responses/404'
             422:
               $ref: '#/components/responses/422'
             500:
               $ref: '#/components/responses/500'
         """
-        if not request.is_json:
-            self.response_400(message="Request is not JSON")
-        item = self.datamodel.get(pk, self._base_filters)
-        if not item:
-            return self.response_404()
-
-        item = self.edit_model_schema.load(request.json, instance=item)
-        if item.errors:
-            return self.response_422(message=item.errors)
         try:
-            self.datamodel.edit(item.data, raise_exception=True)
-            return self.response(
-                200, result=self.edit_model_schema.dump(item.data, many=False).data
-            )
+            self.datamodel.delete(item, raise_exception=True)
+            return self.response(200, message="OK")
         except SQLAlchemyError as e:
             return self.response_422(message=str(e))
 

superset/views/dashboard/mixin.py

@@ -16,6 +16,7 @@
 # under the License.
 from flask_babel import lazy_gettext as _
 
+from ..base import check_ownership
 from .filters import DashboardFilter
 
 
@@ -80,3 +81,6 @@ class DashboardMixin:  # pylint: disable=too-few-public-methods
         "json_metadata": _("JSON Metadata"),
         "table_names": _("Underlying Tables"),
     }
+
+    def pre_delete(self, item):  # pylint: disable=no-self-use
+        check_ownership(item)

superset/views/dashboard/views.py

@@ -84,9 +84,6 @@ def pre_update(self, item):
         check_ownership(item)
         self.pre_add(item)
 
-    def pre_delete(self, item):  # pylint: disable=no-self-use
-        check_ownership(item)
-
 
 class Dashboard(BaseSupersetView):
     """The base views for Superset!"""

tests/dashboard_api_tests.py

@@ -23,6 +23,7 @@
 
 from superset import db, security_manager
 from superset.models import core as models
+from superset.models.slice import Slice
 
 from .base_tests import SupersetTestCase
 
@@ -36,12 +37,14 @@ def insert_dashboard(
         dashboard_title: str,
         slug: str,
         owners: List[int],
+        slices: List[Slice] = None,
         position_json: str = "",
         css: str = "",
         json_metadata: str = "",
         published: bool = False,
     ) -> models.Dashboard:
         obj_owners = list()
+        slices = slices or []
         for owner in owners:
             user = db.session.query(security_manager.user_model).get(owner)
             obj_owners.append(user)
@@ -52,6 +55,7 @@ def insert_dashboard(
             position_json=position_json,
             css=css,
             json_metadata=json_metadata,
+            slices=slices,
             published=published,
         )
         db.session.add(dashboard)
@@ -113,11 +117,16 @@ def test_delete_dashboard_not_owned(self):
         user_alpha2 = self.create_user(
             "alpha2", "password", "Alpha", email="alpha2@superset.org"
         )
-        dashboard = self.insert_dashboard("title", "slug1", [user_alpha1.id])
+        existing_slice = (
+            db.session.query(Slice).filter_by(slice_name="Girl Name Cloud").first()
+        )
+        dashboard = self.insert_dashboard(
+            "title", "slug1", [user_alpha1.id], slices=[existing_slice], published=True
+        )
         self.login(username="alpha2", password="password")
         uri = f"api/v1/dashboard/{dashboard.id}"
         rv = self.client.delete(uri)
-        self.assertEqual(rv.status_code, 404)
+        self.assertEqual(rv.status_code, 403)
         db.session.delete(dashboard)
         db.session.delete(user_alpha1)
         db.session.delete(user_alpha2)
@@ -333,20 +342,25 @@ def test_update_dashboard_slug_formatting(self):
 
     def test_update_dashboard_not_owned(self):
         """
-            Dashboard API: Test update dashboard not owner
+            Dashboard API: Test update dashboard not owned
         """
         user_alpha1 = self.create_user(
             "alpha1", "password", "Alpha", email="alpha1@superset.org"
         )
         user_alpha2 = self.create_user(
             "alpha2", "password", "Alpha", email="alpha2@superset.org"
         )
-        dashboard = self.insert_dashboard("title", "slug1", [user_alpha1.id])
+        existing_slice = (
+            db.session.query(Slice).filter_by(slice_name="Girl Name Cloud").first()
+        )
+        dashboard = self.insert_dashboard(
+            "title", "slug1", [user_alpha1.id], slices=[existing_slice], published=True
+        )
         self.login(username="alpha2", password="password")
         dashboard_data = {"dashboard_title": "title1_changed", "slug": "slug1 changed"}
         uri = f"api/v1/dashboard/{dashboard.id}"
         rv = self.client.put(uri, json=dashboard_data)
-        self.assertEqual(rv.status_code, 404)
+        self.assertEqual(rv.status_code, 403)
         db.session.delete(dashboard)
         db.session.delete(user_alpha1)
         db.session.delete(user_alpha2)