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

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

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

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

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

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

superset/key_value/api.py

@@ -29,6 +29,7 @@
     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 @@ 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:
+        except (DashboardAccessDeniedError, KeyValueAccessDeniedError):
             return self.response_403()
         except DashboardNotFoundError:
             return self.response_404()
@@ -80,7 +81,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:
+        except (DashboardAccessDeniedError, KeyValueAccessDeniedError):
             return self.response_403()
         except DashboardNotFoundError:
             return self.response_404()
@@ -91,7 +92,7 @@ def get(self, pk: int, key: str) -> Response:
             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 @@ def delete(self, pk: int, key: str) -> Response:
             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()

superset/key_value/commands/create.py

@@ -31,16 +31,16 @@
 
 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 @@ def validate(self) -> None:
         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]:
         ...

superset/key_value/commands/delete.py

@@ -29,14 +29,14 @@
 
 
 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 @@ def validate(self) -> None:
         pass
 
     @abstractmethod
-    def delete(self, resource_id: int, key: str) -> Optional[bool]:
+    def delete(self, actor: User, resource_id: int, key: str) -> Optional[bool]:
         ...

superset/key_value/commands/exceptions.py

@@ -20,6 +20,7 @@
     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.")

superset/key_value/commands/get.py

@@ -30,8 +30,8 @@
 
 
 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
 

superset/key_value/commands/update.py

@@ -30,16 +30,16 @@
 
 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 @@ def validate(self) -> None:
         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]:
         ...