fix: Allows PUT and DELETE only for owners of dashboard filter state

superset/dashboards/filter_state/commands/create.py

@@ -16,17 +16,21 @@
 # under the License.
 from typing import Optional
 
+from flask_appbuilder.security.sqla.models import User
+
 from superset.dashboards.dao import DashboardDAO
 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, user: 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
+                cache_key(resource_id, key), [user.get_user_id(), value]
             )
         return False

superset/dashboards/filter_state/commands/delete.py

@@ -16,15 +16,24 @@
 # under the License.
 from typing import Optional
 
+from flask_appbuilder.security.sqla.models import User
+
 from superset.dashboards.dao import DashboardDAO
 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, user: 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 = cache_manager.filter_state_cache.get(cache_key(resource_id, key))
+            if entry:
+                if entry[0] != user.get_user_id():
+                    raise KeyValueAccessDeniedError()
+                return cache_manager.filter_state_cache.delete(
+                    cache_key(resource_id, key)
+                )
         return False

superset/dashboards/filter_state/commands/get.py

@@ -26,8 +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:
-            value = cache_manager.filter_state_cache.get(cache_key(resource_id, key))
+            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[1]
         return None

superset/dashboards/filter_state/commands/update.py

@@ -16,17 +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.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, user: 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 = cache_manager.filter_state_cache.get(cache_key(resource_id, key))
+            if entry:
+                user_id = user.get_user_id()
+                if entry[0] != user_id:
+                    raise KeyValueAccessDeniedError()
+                return cache_manager.filter_state_cache.set(
+                    cache_key(resource_id, key), [user_id, value]
+                )
         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

@@ -33,14 +33,14 @@ class CreateKeyValueCommand(BaseCommand, ABC):
     def __init__(
         self, user: User, resource_id: int, value: str,
     ):
-        self._actor = user
+        self._user = user
         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._user, 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, user: User, resource_id: int, key: str, value: str
+    ) -> Optional[bool]:
         ...

superset/key_value/commands/delete.py

@@ -30,13 +30,13 @@
 
 class DeleteKeyValueCommand(BaseCommand, ABC):
     def __init__(self, user: User, resource_id: int, key: str):
-        self._actor = user
+        self._user = user
         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._user, 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, user: 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

@@ -31,7 +31,7 @@
 
 class GetKeyValueCommand(BaseCommand, ABC):
     def __init__(self, user: User, resource_id: int, key: str):
-        self._actor = user
+        self._user = user
         self._resource_id = resource_id
         self._key = key
 

superset/key_value/commands/update.py

@@ -32,14 +32,14 @@ class UpdateKeyValueCommand(BaseCommand, ABC):
     def __init__(
         self, user: User, resource_id: int, key: str, value: str,
     ):
-        self._actor = user
+        self._user = user
         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._user, 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, user: User, resource_id: int, key: str, value: str
+    ) -> Optional[bool]:
         ...