You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by dp...@apache.org on 2020/01/06 15:29:51 UTC
[incubator-superset] branch master updated: [dashboard] Fix,
prevent delete and update on dashes not owned (#8911)
This is an automated email from the ASF dual-hosted git repository.
dpgaspar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new 2726f21 [dashboard] Fix, prevent delete and update on dashes not owned (#8911)
2726f21 is described below
commit 2726f21cbc52bd45a80b8584f1f85523e8766b24
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Mon Jan 6 15:29:39 2020 +0000
[dashboard] Fix, prevent delete and update on dashes not owned (#8911)
---
superset/views/base.py | 20 +++++++
superset/views/dashboard/api.py | 111 ++++++++++++++++++++++++++------------
superset/views/dashboard/mixin.py | 4 ++
superset/views/dashboard/views.py | 3 --
tests/dashboard_api_tests.py | 24 +++++++--
5 files changed, 120 insertions(+), 42 deletions(-)
diff --git a/superset/views/base.py b/superset/views/base.py
index 3a5dd64..25aa689 100644
--- a/superset/views/base.py
+++ b/superset/views/base.py
@@ -155,6 +155,26 @@ def handle_api_exception(f):
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)
diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py
index 5068478..5b6f535 100644
--- a/superset/views/dashboard/api.py
+++ b/superset/views/dashboard/api.py
@@ -24,11 +24,15 @@ from marshmallow import fields, post_load, pre_load, Schema, ValidationError
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 @@ class DashboardPutSchema(BaseDashboardSchema):
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,39 +318,33 @@ class DashboardRestApi(DashboardMixin, BaseSupersetModelRestApi):
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:
@@ -298,20 +352,9 @@ class DashboardRestApi(DashboardMixin, BaseSupersetModelRestApi):
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))
diff --git a/superset/views/dashboard/mixin.py b/superset/views/dashboard/mixin.py
index b3b5a8d..e0fa46d 100644
--- a/superset/views/dashboard/mixin.py
+++ b/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)
diff --git a/superset/views/dashboard/views.py b/superset/views/dashboard/views.py
index bef9e7b..c5e891d 100644
--- a/superset/views/dashboard/views.py
+++ b/superset/views/dashboard/views.py
@@ -84,9 +84,6 @@ class DashboardModelView(
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!"""
diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py
index 9a60d0a..31303aa 100644
--- a/tests/dashboard_api_tests.py
+++ b/tests/dashboard_api_tests.py
@@ -23,6 +23,7 @@ from flask_appbuilder.security.sqla import models as ab_models
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 @@ class DashboardApiTests(SupersetTestCase):
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 @@ class DashboardApiTests(SupersetTestCase):
position_json=position_json,
css=css,
json_metadata=json_metadata,
+ slices=slices,
published=published,
)
db.session.add(dashboard)
@@ -113,11 +117,16 @@ class DashboardApiTests(SupersetTestCase):
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,7 +342,7 @@ class DashboardApiTests(SupersetTestCase):
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"
@@ -341,12 +350,17 @@ class DashboardApiTests(SupersetTestCase):
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)