From a07829633083d684378ed39802a89ca65c998b85 Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Wed, 13 Jan 2021 18:06:41 +0000 Subject: [PATCH] fix: impose dataset ownership check on old API (#12491) * fix: impose dataset ownership check on old API * update UPDATING.md * partially protect the old MVC also * prevent metric and column add and update --- UPDATING.md | 1 + superset/commands/exceptions.py | 2 +- superset/connectors/sqla/views.py | 22 ++++++++++++++++++++++ superset/views/datasource.py | 12 +++++++++++- 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 0e82d06f2c..75d693b2b9 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -23,6 +23,7 @@ This file documents any backwards-incompatible changes in Superset and assists people when migrating to a new version. ## Next +- [11509](https://github.com/apache/superset/pull/12491): Dataset metadata updates check user ownership, only owners or an Admin are allowed. - Security simplification (SIP-19), the following permission domains were simplified: - [12072](https://github.com/apache/superset/pull/12072): `Query` with `can_read`, `can_write` - [12036](https://github.com/apache/superset/pull/12036): `Database` with `can_read`, `can_write`. diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index 0eb5dba6d0..969475544c 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -69,7 +69,7 @@ class DeleteFailedError(CommandException): class ForbiddenError(CommandException): - status = 500 + status = 403 message = "Action is forbidden" diff --git a/superset/connectors/sqla/views.py b/superset/connectors/sqla/views.py index a9ae564e34..903cef2579 100644 --- a/superset/connectors/sqla/views.py +++ b/superset/connectors/sqla/views.py @@ -37,6 +37,7 @@ from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.typing import FlaskResponse from superset.utils import core as utils from superset.views.base import ( + check_ownership, create_table_permissions, DatasourceFilter, DeleteMixin, @@ -171,6 +172,15 @@ class TableColumnInlineView( # pylint: disable=too-many-ancestors edit_form_extra_fields = add_form_extra_fields + def pre_add(self, item: "models.SqlMetric") -> None: + check_ownership(item.table) + + def pre_update(self, item: "models.SqlMetric") -> None: + check_ownership(item.table) + + def pre_delete(self, item: "models.SqlMetric") -> None: + check_ownership(item.table) + class SqlMetricInlineView( # pylint: disable=too-many-ancestors CompactCRUDMixin, SupersetModelView @@ -245,6 +255,15 @@ class SqlMetricInlineView( # pylint: disable=too-many-ancestors edit_form_extra_fields = add_form_extra_fields + def pre_add(self, item: "models.SqlMetric") -> None: + check_ownership(item.table) + + def pre_update(self, item: "models.SqlMetric") -> None: + check_ownership(item.table) + + def pre_delete(self, item: "models.SqlMetric") -> None: + check_ownership(item.table) + class RowLevelSecurityListWidget( SupersetListWidget @@ -459,6 +478,9 @@ class TableModelView( # pylint: disable=too-many-ancestors def pre_add(self, item: "TableModelView") -> None: validate_sqlatable(item) + def pre_update(self, item: "TableModelView") -> None: + check_ownership(item) + def post_add( # pylint: disable=arguments-differ self, item: "TableModelView", diff --git a/superset/views/datasource.py b/superset/views/datasource.py index 7724d1e26c..5c9a41da41 100644 --- a/superset/views/datasource.py +++ b/superset/views/datasource.py @@ -24,8 +24,10 @@ from sqlalchemy.orm.exc import NoResultFound from superset import db from superset.connectors.connector_registry import ConnectorRegistry -from superset.exceptions import SupersetException +from superset.datasets.commands.exceptions import DatasetForbiddenError +from superset.exceptions import SupersetException, SupersetSecurityException from superset.typing import FlaskResponse +from superset.views.base import check_ownership from .base import api, BaseSupersetView, handle_api_exception, json_error_response @@ -52,6 +54,14 @@ class Datasource(BaseSupersetView): orm_datasource.database_id = database_id if "owners" in datasource_dict and orm_datasource.owner_class is not None: + # Check ownership + try: + check_ownership(orm_datasource) + except SupersetSecurityException: + return json_error_response( + f"{DatasetForbiddenError.message}", DatasetForbiddenError.status + ) + datasource_dict["owners"] = ( db.session.query(orm_datasource.owner_class) .filter(orm_datasource.owner_class.id.in_(datasource_dict["owners"]))