From 2ae83fac8623acd20f92e9f441ce03793354e0a1 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Sun, 5 Dec 2021 22:13:09 -0300 Subject: [PATCH] fix: Allows PUT and DELETE only for owners of dashboard filter state (#17644) * fix: Allows PUT and DELETE only for owners of dashboard filter state * Converts the values to TypedDict * Fixes variable name --- .../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 | 250 ++++++++++-------- 12 files changed, 239 insertions(+), 134 deletions(-) create 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 4b8a789004..045b2faede 100644 --- a/superset/dashboards/filter_state/commands/create.py +++ b/superset/dashboards/filter_state/commands/create.py @@ -16,17 +16,23 @@ # 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, resource_id: int, key: str, value: str) -> Optional[bool]: + def create( + self, actor: User, 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), value + cache_key(resource_id, key), entry ) return False diff --git a/superset/dashboards/filter_state/commands/delete.py b/superset/dashboards/filter_state/commands/delete.py index 89ee287945..0f27911cc9 100644 --- a/superset/dashboards/filter_state/commands/delete.py +++ b/superset/dashboards/filter_state/commands/delete.py @@ -16,15 +16,27 @@ # 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, resource_id: int, key: str) -> Optional[bool]: + def delete(self, actor: User, resource_id: int, key: str) -> Optional[bool]: dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: - return cache_manager.filter_state_cache.delete(cache_key(resource_id, key)) + 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 False diff --git a/superset/dashboards/filter_state/commands/entry.py b/superset/dashboards/filter_state/commands/entry.py new file mode 100644 index 0000000000..4ac6f035ec --- /dev/null +++ b/superset/dashboards/filter_state/commands/entry.py @@ -0,0 +1,22 @@ +# 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 ea9dc9d9be..d95f6b0b94 100644 --- a/superset/dashboards/filter_state/commands/get.py +++ b/superset/dashboards/filter_state/commands/get.py @@ -17,6 +17,7 @@ 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 @@ -26,8 +27,10 @@ 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: - value = cache_manager.filter_state_cache.get(cache_key(resource_id, key)) + entry: Entry = cache_manager.filter_state_cache.get( + cache_key(resource_id, key) + ) if refreshTimeout: - cache_manager.filter_state_cache.set(key, value) - return value + cache_manager.filter_state_cache.set(key, entry) + return entry["value"] return None diff --git a/superset/dashboards/filter_state/commands/update.py b/superset/dashboards/filter_state/commands/update.py index a432df612d..1412348cf3 100644 --- a/superset/dashboards/filter_state/commands/update.py +++ b/superset/dashboards/filter_state/commands/update.py @@ -16,17 +16,31 @@ # 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, resource_id: int, key: str, value: str) -> Optional[bool]: + def update( + self, actor: User, resource_id: int, key: str, value: str + ) -> Optional[bool]: dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: - return cache_manager.filter_state_cache.set( - cache_key(resource_id, key), value + entry: Entry = cache_manager.filter_state_cache.get( + cache_key(resource_id, key) ) + 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 d67852a29b..2dc340095c 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -29,6 +29,7 @@ from superset.dashboards.commands.exceptions import ( 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__) @@ -64,7 +65,7 @@ class KeyValueRestApi(BaseApi, ABC): return self.response(201, key=key) except ValidationError as error: return self.response_400(message=error.messages) - except DashboardAccessDeniedError: + except (DashboardAccessDeniedError, KeyValueAccessDeniedError): return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -80,7 +81,7 @@ class KeyValueRestApi(BaseApi, ABC): return self.response(200, message="Value updated successfully.") except ValidationError as error: return self.response_400(message=error.messages) - except DashboardAccessDeniedError: + except (DashboardAccessDeniedError, KeyValueAccessDeniedError): return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -91,7 +92,7 @@ class KeyValueRestApi(BaseApi, ABC): if not value: return self.response_404() return self.response(200, value=value) - except DashboardAccessDeniedError: + except (DashboardAccessDeniedError, KeyValueAccessDeniedError): return self.response_403() except DashboardNotFoundError: return self.response_404() @@ -102,7 +103,7 @@ class KeyValueRestApi(BaseApi, ABC): if not result: return self.response_404() return self.response(200, message="Deleted successfully") - except DashboardAccessDeniedError: + except (DashboardAccessDeniedError, KeyValueAccessDeniedError): 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 361a3ad222..3c56f73ebe 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -31,16 +31,16 @@ logger = logging.getLogger(__name__) class CreateKeyValueCommand(BaseCommand, ABC): def __init__( - self, user: User, resource_id: int, value: str, + self, actor: User, resource_id: int, value: str, ): - self._actor = user + self._actor = actor self._resource_id = resource_id self._value = value def run(self) -> Model: try: key = token_urlsafe(48) - self.create(self._resource_id, key, self._value) + self.create(self._actor, self._resource_id, key, self._value) return key except SQLAlchemyError as ex: logger.exception("Error running create command") @@ -50,5 +50,7 @@ class CreateKeyValueCommand(BaseCommand, ABC): pass @abstractmethod - def create(self, resource_id: int, key: str, value: str) -> Optional[bool]: + def create( + self, actor: User, 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 9990c4450c..9c885d0773 100644 --- a/superset/key_value/commands/delete.py +++ b/superset/key_value/commands/delete.py @@ -29,14 +29,14 @@ logger = logging.getLogger(__name__) class DeleteKeyValueCommand(BaseCommand, ABC): - def __init__(self, user: User, resource_id: int, key: str): - self._actor = user + def __init__(self, actor: User, resource_id: int, key: str): + self._actor = actor self._resource_id = resource_id self._key = key def run(self) -> Model: try: - return self.delete(self._resource_id, self._key) + return self.delete(self._actor, self._resource_id, self._key) except SQLAlchemyError as ex: logger.exception("Error running delete command") raise KeyValueDeleteFailedError() from ex @@ -45,5 +45,5 @@ class DeleteKeyValueCommand(BaseCommand, ABC): pass @abstractmethod - def delete(self, resource_id: int, key: str) -> Optional[bool]: + def delete(self, actor: User, resource_id: int, key: str) -> Optional[bool]: ... diff --git a/superset/key_value/commands/exceptions.py b/superset/key_value/commands/exceptions.py index f8c9dbd4a4..780f705e8a 100644 --- a/superset/key_value/commands/exceptions.py +++ b/superset/key_value/commands/exceptions.py @@ -20,6 +20,7 @@ from superset.commands.exceptions import ( CommandException, CreateFailedError, DeleteFailedError, + ForbiddenError, UpdateFailedError, ) @@ -38,3 +39,7 @@ 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 32991fa96f..be56050fa0 100644 --- a/superset/key_value/commands/get.py +++ b/superset/key_value/commands/get.py @@ -30,8 +30,8 @@ logger = logging.getLogger(__name__) class GetKeyValueCommand(BaseCommand, ABC): - def __init__(self, user: User, resource_id: int, key: str): - self._actor = user + def __init__(self, actor: User, resource_id: int, key: str): + self._actor = actor 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 dac91daf31..3f322c7a27 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -30,16 +30,16 @@ logger = logging.getLogger(__name__) class UpdateKeyValueCommand(BaseCommand, ABC): def __init__( - self, user: User, resource_id: int, key: str, value: str, + self, actor: User, resource_id: int, key: str, value: str, ): - self._actor = user + self._actor = actor self._resource_id = resource_id self._key = key self._value = value def run(self) -> Model: try: - return self.update(self._resource_id, self._key, self._value) + return self.update(self._actor, self._resource_id, self._key, self._value) except SQLAlchemyError as ex: logger.exception("Error running update command") raise KeyValueUpdateFailedError() from ex @@ -48,5 +48,7 @@ class UpdateKeyValueCommand(BaseCommand, ABC): pass @abstractmethod - def update(self, resource_id: int, key: str, value: str) -> Optional[bool]: + def update( + self, actor: User, 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 1c2beef661..02af14c5fa 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -15,143 +15,181 @@ # specific language governing permissions and limitations # under the License. import json -from typing import Any from unittest.mock import patch import pytest -from flask.testing import FlaskClient +from flask_appbuilder.security.sqla.models import User 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" -class FilterStateTests: - @pytest.fixture - def client(self): - with app.test_client() as client: - with app.app_context(): - yield client +@pytest.fixture +def client(): + 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) +@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 - 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 +@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 - 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 +@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_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 +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 - @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_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 - 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_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 - @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 +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 - @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 + +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