From bc56eea50ebe3c1a750c68b7472bb9fc9bf560a5 Mon Sep 17 00:00:00 2001 From: Phillip Kelley-Dotson Date: Tue, 7 Dec 2021 13:51:55 -0800 Subject: [PATCH] Revert "fix: Allows PUT and DELETE only for owners of dashboard filter state (#17644)" This reverts commit 2ae83fac8623acd20f92e9f441ce03793354e0a1. --- .../filter_state/commands/create.py | 10 +- .../filter_state/commands/delete.py | 16 +- .../dashboards/filter_state/commands/entry.py | 22 -- .../dashboards/filter_state/commands/get.py | 9 +- .../filter_state/commands/update.py | 20 +- superset/key_value/api.py | 9 +- superset/key_value/commands/create.py | 10 +- superset/key_value/commands/delete.py | 8 +- superset/key_value/commands/exceptions.py | 5 - superset/key_value/commands/get.py | 4 +- superset/key_value/commands/update.py | 10 +- .../dashboards/filter_state/api_tests.py | 280 ++++++++---------- 12 files changed, 149 insertions(+), 254 deletions(-) delete mode 100644 superset/dashboards/filter_state/commands/entry.py diff --git a/superset/dashboards/filter_state/commands/create.py b/superset/dashboards/filter_state/commands/create.py index 045b2faedece0..4b8a7890049aa 100644 --- a/superset/dashboards/filter_state/commands/create.py +++ b/superset/dashboards/filter_state/commands/create.py @@ -16,23 +16,17 @@ # under the License. from typing import Optional -from flask_appbuilder.security.sqla.models import User - from superset.dashboards.dao import DashboardDAO -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager from superset.key_value.commands.create import CreateKeyValueCommand from superset.key_value.utils import cache_key class CreateFilterStateCommand(CreateKeyValueCommand): - def create( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def create(self, resource_id: int, key: str, value: str) -> Optional[bool]: dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: - entry: Entry = {"owner": actor.get_user_id(), "value": value} return cache_manager.filter_state_cache.set( - cache_key(resource_id, key), entry + cache_key(resource_id, key), value ) return False diff --git a/superset/dashboards/filter_state/commands/delete.py b/superset/dashboards/filter_state/commands/delete.py index 0f27911cc91a4..89ee287945e4a 100644 --- a/superset/dashboards/filter_state/commands/delete.py +++ b/superset/dashboards/filter_state/commands/delete.py @@ -16,27 +16,15 @@ # under the License. from typing import Optional -from flask_appbuilder.security.sqla.models import User - from superset.dashboards.dao import DashboardDAO -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager from superset.key_value.commands.delete import DeleteKeyValueCommand -from superset.key_value.commands.exceptions import KeyValueAccessDeniedError from superset.key_value.utils import cache_key class DeleteFilterStateCommand(DeleteKeyValueCommand): - def delete(self, actor: User, resource_id: int, key: str) -> Optional[bool]: + def delete(self, resource_id: int, key: str) -> Optional[bool]: dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: - entry: Entry = cache_manager.filter_state_cache.get( - cache_key(resource_id, key) - ) - if entry: - if entry["owner"] != actor.get_user_id(): - raise KeyValueAccessDeniedError() - return cache_manager.filter_state_cache.delete( - cache_key(resource_id, key) - ) + return cache_manager.filter_state_cache.delete(cache_key(resource_id, key)) return False diff --git a/superset/dashboards/filter_state/commands/entry.py b/superset/dashboards/filter_state/commands/entry.py deleted file mode 100644 index 4ac6f035ece66..0000000000000 --- a/superset/dashboards/filter_state/commands/entry.py +++ /dev/null @@ -1,22 +0,0 @@ -# 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. -from typing import TypedDict - - -class Entry(TypedDict): - owner: int - value: str diff --git a/superset/dashboards/filter_state/commands/get.py b/superset/dashboards/filter_state/commands/get.py index d95f6b0b94ecb..ea9dc9d9be707 100644 --- a/superset/dashboards/filter_state/commands/get.py +++ b/superset/dashboards/filter_state/commands/get.py @@ -17,7 +17,6 @@ from typing import Optional from superset.dashboards.dao import DashboardDAO -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager from superset.key_value.commands.get import GetKeyValueCommand from superset.key_value.utils import cache_key @@ -27,10 +26,8 @@ class GetFilterStateCommand(GetKeyValueCommand): def get(self, resource_id: int, key: str, refreshTimeout: bool) -> Optional[str]: dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: - entry: Entry = cache_manager.filter_state_cache.get( - cache_key(resource_id, key) - ) + value = cache_manager.filter_state_cache.get(cache_key(resource_id, key)) if refreshTimeout: - cache_manager.filter_state_cache.set(key, entry) - return entry["value"] + cache_manager.filter_state_cache.set(key, value) + return value return None diff --git a/superset/dashboards/filter_state/commands/update.py b/superset/dashboards/filter_state/commands/update.py index 1412348cf3555..a432df612dc93 100644 --- a/superset/dashboards/filter_state/commands/update.py +++ b/superset/dashboards/filter_state/commands/update.py @@ -16,31 +16,17 @@ # under the License. from typing import Optional -from flask_appbuilder.security.sqla.models import User - from superset.dashboards.dao import DashboardDAO -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager -from superset.key_value.commands.exceptions import KeyValueAccessDeniedError from superset.key_value.commands.update import UpdateKeyValueCommand from superset.key_value.utils import cache_key class UpdateFilterStateCommand(UpdateKeyValueCommand): - def update( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def update(self, resource_id: int, key: str, value: str) -> Optional[bool]: dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: - entry: Entry = cache_manager.filter_state_cache.get( - cache_key(resource_id, key) + return cache_manager.filter_state_cache.set( + cache_key(resource_id, key), value ) - if entry: - user_id = actor.get_user_id() - if entry["owner"] != user_id: - raise KeyValueAccessDeniedError() - new_entry: Entry = {"owner": actor.get_user_id(), "value": value} - return cache_manager.filter_state_cache.set( - cache_key(resource_id, key), new_entry - ) return False diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 2dc340095cb99..d67852a29bbec 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -29,7 +29,6 @@ DashboardNotFoundError, ) from superset.exceptions import InvalidPayloadFormatError -from superset.key_value.commands.exceptions import KeyValueAccessDeniedError from superset.key_value.schemas import KeyValuePostSchema, KeyValuePutSchema logger = logging.getLogger(__name__) @@ -65,7 +64,7 @@ def post(self, pk: int) -> Response: return self.response(201, key=key) except ValidationError as error: return self.response_400(message=error.messages) - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): + except DashboardAccessDeniedError: return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -81,7 +80,7 @@ def put(self, pk: int, key: str) -> Response: return self.response(200, message="Value updated successfully.") except ValidationError as error: return self.response_400(message=error.messages) - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): + except DashboardAccessDeniedError: return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -92,7 +91,7 @@ def get(self, pk: int, key: str) -> Response: if not value: return self.response_404() return self.response(200, value=value) - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): + except DashboardAccessDeniedError: return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -103,7 +102,7 @@ def delete(self, pk: int, key: str) -> Response: if not result: return self.response_404() return self.response(200, message="Deleted successfully") - except (DashboardAccessDeniedError, KeyValueAccessDeniedError): + except DashboardAccessDeniedError: return self.response_403() except DashboardNotFoundError: return self.response_404() diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index 3c56f73ebebed..361a3ad222809 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -31,16 +31,16 @@ class CreateKeyValueCommand(BaseCommand, ABC): def __init__( - self, actor: User, resource_id: int, value: str, + self, user: User, resource_id: int, value: str, ): - self._actor = actor + self._actor = user self._resource_id = resource_id self._value = value def run(self) -> Model: try: key = token_urlsafe(48) - self.create(self._actor, self._resource_id, key, self._value) + self.create(self._resource_id, key, self._value) return key except SQLAlchemyError as ex: logger.exception("Error running create command") @@ -50,7 +50,5 @@ def validate(self) -> None: pass @abstractmethod - def create( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def create(self, resource_id: int, key: str, value: str) -> Optional[bool]: ... diff --git a/superset/key_value/commands/delete.py b/superset/key_value/commands/delete.py index 9c885d07732be..9990c4450c484 100644 --- a/superset/key_value/commands/delete.py +++ b/superset/key_value/commands/delete.py @@ -29,14 +29,14 @@ class DeleteKeyValueCommand(BaseCommand, ABC): - def __init__(self, actor: User, resource_id: int, key: str): - self._actor = actor + def __init__(self, user: User, resource_id: int, key: str): + self._actor = user self._resource_id = resource_id self._key = key def run(self) -> Model: try: - return self.delete(self._actor, self._resource_id, self._key) + return self.delete(self._resource_id, self._key) except SQLAlchemyError as ex: logger.exception("Error running delete command") raise KeyValueDeleteFailedError() from ex @@ -45,5 +45,5 @@ def validate(self) -> None: pass @abstractmethod - def delete(self, actor: User, resource_id: int, key: str) -> Optional[bool]: + def delete(self, resource_id: int, key: str) -> Optional[bool]: ... diff --git a/superset/key_value/commands/exceptions.py b/superset/key_value/commands/exceptions.py index 780f705e8ad56..f8c9dbd4a4ff9 100644 --- a/superset/key_value/commands/exceptions.py +++ b/superset/key_value/commands/exceptions.py @@ -20,7 +20,6 @@ CommandException, CreateFailedError, DeleteFailedError, - ForbiddenError, UpdateFailedError, ) @@ -39,7 +38,3 @@ class KeyValueDeleteFailedError(DeleteFailedError): class KeyValueUpdateFailedError(UpdateFailedError): message = _("An error occurred while updating the value.") - - -class KeyValueAccessDeniedError(ForbiddenError): - message = _("You don't have permission to modify the value.") diff --git a/superset/key_value/commands/get.py b/superset/key_value/commands/get.py index be56050fa0637..32991fa96f543 100644 --- a/superset/key_value/commands/get.py +++ b/superset/key_value/commands/get.py @@ -30,8 +30,8 @@ class GetKeyValueCommand(BaseCommand, ABC): - def __init__(self, actor: User, resource_id: int, key: str): - self._actor = actor + def __init__(self, user: User, resource_id: int, key: str): + self._actor = user self._resource_id = resource_id self._key = key diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py index 3f322c7a27e25..dac91daf31035 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -30,16 +30,16 @@ class UpdateKeyValueCommand(BaseCommand, ABC): def __init__( - self, actor: User, resource_id: int, key: str, value: str, + self, user: User, resource_id: int, key: str, value: str, ): - self._actor = actor + self._actor = user self._resource_id = resource_id self._key = key self._value = value def run(self) -> Model: try: - return self.update(self._actor, self._resource_id, self._key, self._value) + return self.update(self._resource_id, self._key, self._value) except SQLAlchemyError as ex: logger.exception("Error running update command") raise KeyValueUpdateFailedError() from ex @@ -48,7 +48,5 @@ def validate(self) -> None: pass @abstractmethod - def update( - self, actor: User, resource_id: int, key: str, value: str - ) -> Optional[bool]: + def update(self, resource_id: int, key: str, value: str) -> Optional[bool]: ... diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index 02af14c5fabc9..1c2beef661624 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -15,181 +15,143 @@ # specific language governing permissions and limitations # under the License. import json +from typing import Any from unittest.mock import patch import pytest -from flask_appbuilder.security.sqla.models import User +from flask.testing import FlaskClient from sqlalchemy.orm import Session from superset import app from superset.dashboards.commands.exceptions import DashboardAccessDeniedError -from superset.dashboards.filter_state.commands.entry import Entry from superset.extensions import cache_manager from superset.key_value.utils import cache_key from superset.models.dashboard import Dashboard -from tests.integration_tests.base_tests import login from tests.integration_tests.fixtures.world_bank_dashboard import ( load_world_bank_dashboard_with_slices, ) from tests.integration_tests.test_app import app +dashboardId = 985374 key = "test-key" value = "test" -@pytest.fixture -def client(): - with app.test_client() as client: - with app.app_context(): - yield client - - -@pytest.fixture -def dashboard_id(load_world_bank_dashboard_with_slices) -> int: - with app.app_context() as ctx: - session: Session = ctx.app.appbuilder.get_session - dashboard = session.query(Dashboard).filter_by(slug="world_health").one() - return dashboard.id - - -@pytest.fixture -def admin_id() -> int: - with app.app_context() as ctx: - session: Session = ctx.app.appbuilder.get_session - admin = session.query(User).filter_by(username="admin").one_or_none() - return admin.id - - -@pytest.fixture(autouse=True) -def cache(dashboard_id, admin_id): - app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} - cache_manager.init_app(app) - entry: Entry = {"owner": admin_id, "value": value} - cache_manager.filter_state_cache.set(cache_key(dashboard_id, key), entry) - - -def test_post(client, dashboard_id: int): - login(client, "admin") - payload = { - "value": value, - } - resp = client.post(f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload) - assert resp.status_code == 201 - - -def test_post_bad_request(client, dashboard_id: int): - login(client, "admin") - payload = { - "value": 1234, - } - resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload - ) - assert resp.status_code == 400 - - -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_post_access_denied(mock_raise_for_dashboard_access, client, dashboard_id: int): - login(client, "admin") - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - payload = { - "value": value, - } - resp = client.post(f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload) - assert resp.status_code == 403 - - -def test_put(client, dashboard_id: int): - login(client, "admin") - payload = { - "value": "new value", - } - resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload - ) - assert resp.status_code == 200 - - -def test_put_bad_request(client, dashboard_id: int): - login(client, "admin") - payload = { - "value": 1234, - } - resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload - ) - assert resp.status_code == 400 - - -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_put_access_denied(mock_raise_for_dashboard_access, client, dashboard_id: int): - login(client, "admin") - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - payload = { - "value": "new value", - } - resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload - ) - assert resp.status_code == 403 - - -def test_put_not_owner(client, dashboard_id: int): - login(client, "gamma") - payload = { - "value": "new value", - } - resp = client.put( - f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload - ) - assert resp.status_code == 403 - - -def test_get_key_not_found(client): - login(client, "admin") - resp = client.get("unknown-key") - assert resp.status_code == 404 - - -def test_get_dashboard_not_found(client): - login(client, "admin") - resp = client.get(f"api/v1/dashboard/{-1}/filter_state/{key}/") - assert resp.status_code == 404 - - -def test_get(client, dashboard_id: int): - login(client, "admin") - resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") - assert resp.status_code == 200 - data = json.loads(resp.data.decode("utf-8")) - assert value == data.get("value") - - -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_get_access_denied(mock_raise_for_dashboard_access, client, dashboard_id): - login(client, "admin") - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") - assert resp.status_code == 403 - - -def test_delete(client, dashboard_id: int): - login(client, "admin") - resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") - assert resp.status_code == 200 - - -@patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") -def test_delete_access_denied( - mock_raise_for_dashboard_access, client, dashboard_id: int -): - login(client, "admin") - mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") - assert resp.status_code == 403 - - -def test_delete_not_owner(client, dashboard_id: int): - login(client, "gamma") - resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") - assert resp.status_code == 403 +class FilterStateTests: + @pytest.fixture + def client(self): + with app.test_client() as client: + with app.app_context(): + yield client + + @pytest.fixture + def dashboard_id(self, load_world_bank_dashboard_with_slices) -> int: + with app.app_context() as ctx: + session: Session = ctx.app.appbuilder.get_session + dashboard = session.query(Dashboard).filter_by(slug="world_health").one() + return dashboard.id + + @pytest.fixture + def cache(self, dashboard_id): + app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} + cache_manager.init_app(app) + cache_manager.filter_state_cache.set(cache_key(dashboard_id, key), value) + + def setUp(self): + self.login(username="admin") + + def test_post(self, client, dashboard_id: int): + payload = { + "value": value, + } + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload + ) + assert resp.status_code == 201 + + def test_post_bad_request(self, client, dashboard_id: int): + payload = { + "value": 1234, + } + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 400 + + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_post_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id: int + ): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + payload = { + "value": value, + } + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload + ) + assert resp.status_code == 403 + + def test_put(self, client, dashboard_id: int): + payload = { + "value": "new value", + } + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 200 + + def test_put_bad_request(self, client, dashboard_id: int): + payload = { + "value": 1234, + } + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 400 + + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_put_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id: int + ): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + payload = { + "value": "new value", + } + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 403 + + def test_get_key_not_found(self, client): + resp = client.get("unknown-key") + assert resp.status_code == 404 + + def test_get_dashboard_not_found(self, client): + resp = client.get(f"api/v1/dashboard/{-1}/filter_state/{key}/") + assert resp.status_code == 404 + + def test_get(self, client, dashboard_id: int): + resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + assert resp.status_code == 200 + data = json.loads(resp.data.decode("utf-8")) + assert value == data.get("value") + + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_get_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id + ): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + assert resp.status_code == 403 + + def test_delete(self, client, dashboard_id: int): + resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + assert resp.status_code == 200 + + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_delete_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id: int + ): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") + assert resp.status_code == 403