codeant-ai-for-open-source[bot] commented on code in PR #39977: URL: https://github.com/apache/superset/pull/39977#discussion_r3220682707
########## superset/commands/restore.py: ########## @@ -0,0 +1,82 @@ +# 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. +"""Base class shared by all soft-delete restore commands.""" + +from typing import Any, ClassVar, Generic, TypeVar + +from superset import security_manager +from superset.commands.base import BaseCommand +from superset.exceptions import SupersetSecurityException +from superset.models.helpers import SoftDeleteMixin + +T = TypeVar("T", bound=SoftDeleteMixin) + + +class BaseRestoreCommand(BaseCommand, Generic[T]): + """Base class for soft-delete restore commands. + + Subclasses provide the entity-specific bindings as class variables: + + - ``dao``: the DAO class (e.g. ``ChartDAO``) + - ``not_found_exc``: exception type raised when the row doesn't + exist OR isn't soft-deleted + - ``forbidden_exc``: exception type raised when the caller doesn't + have ownership of the row + + Subclasses MUST override ``run()`` only to apply the + ``@transaction(on_error=partial(on_error, reraise=<RestoreFailed>))`` + decorator with their concrete restore-failed exception type — the + body should be a single ``super().run()`` call. The base class does + not enforce this in code; Python decorators don't compose well with + class-var-driven configuration. The contract lives in this + docstring and in code review. + + The model returned from ``validate()`` is the soft-deleted row, + type-narrowed via ``Generic[T]``. ``run()`` calls ``model.restore()`` + on it (the method comes from ``SoftDeleteMixin``). + """ + + dao: ClassVar[Any] + not_found_exc: ClassVar[type[Exception]] + forbidden_exc: ClassVar[type[Exception]] + + def __init__(self, model_uuid: str) -> None: + self._model_uuid = model_uuid + + def run(self) -> None: + model = self.validate() + model.restore() + + def validate(self) -> T: # type: ignore[override] + model = self.dao.find_by_id( + self._model_uuid, + id_column="uuid", + skip_base_filter=True, + skip_visibility_filter=True, + ) + if model is None: + raise self.not_found_exc(f"No row with uuid={self._model_uuid!r}") + if model.deleted_at is None: + raise self.not_found_exc( + f"Row with uuid={self._model_uuid!r} is not soft-deleted; " + "nothing to restore" + ) + try: + security_manager.raise_for_ownership(model) + except SupersetSecurityException as ex: + raise self.forbidden_exc() from ex Review Comment: **Suggestion:** `raise_for_ownership()` re-queries the object with a plain ORM query, which is still subject to the global soft-delete visibility filter. For soft-deleted rows this re-query returns no row, so ownership check falls back to an empty owner list and incorrectly raises forbidden for non-admin users, making restore fail even for valid owners. Use an ownership check path that does not re-query through the filtered session (or passes the visibility-skip option) for restore flows. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Future chart/dashboard/dataset restore commands reject valid owners. - ⚠️ Ownership checks on soft-deleted rows inconsistent for owners. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Note the global soft-delete visibility filter `_add_soft_delete_filter` in `superset/models/helpers.py:69-107`, which applies a `deleted_at IS NULL` criterion to all ORM `SELECT`s for models inheriting `SoftDeleteMixin`, unless the `SKIP_VISIBILITY_FILTER` execution option (or `g.skip_visibility_filter`) is set. 2. Observe `BaseRestoreCommand.validate()` in `superset/commands/restore.py:64-81`: it loads the target model via `self.dao.find_by_id(..., skip_visibility_filter=True)` so soft-deleted rows can be fetched, checks `model.deleted_at is not None`, and then calls `security_manager.raise_for_ownership(model)` at line 79. 3. Inspect `SupersetSecurityManager.raise_for_ownership()` in `superset/security/manager.py:6-21`: for non-admins it re-queries the resource with `orig_resource = self.session.query(resource.__class__).get(resource.id)` using the security manager session, without setting the `SKIP_VISIBILITY_FILTER` execution option. 4. For any resource class that (a) inherits `SoftDeleteMixin` and (b) has an `owners` relationship (e.g. future chart/dashboard/dataset models once SIP-208 rollouts add the mixin), if a non-admin owner tries to restore a soft-deleted row: `validate()` will retrieve the row via `skip_visibility_filter=True`, but `raise_for_ownership()` will re-load it through the globally filtered session, `orig_resource` will resolve to `None` due to the `deleted_at IS NULL` criterion from `_add_soft_delete_filter`, `owners` will fall back to `[]`, and the method will raise `SupersetSecurityException`. `BaseRestoreCommand.validate()` catches this at `superset/commands/restore.py:78-81` and raises `forbidden_exc`, so legitimate non-admin owners are incorrectly forbidden from restoring their own soft-deleted resources. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcommands%2Frestore.py%0A%2A%2ALine%3A%2A%2A%2078%3A81%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20%60raise_for_ownership%28%29%60%20re-queries%20the%20object%20with%20a%20plain%20ORM%20query%2C%20which%20is%20still%20subject%20to%20the%20global%20soft-delete%20visibility%20filter.%20For%20soft-deleted%20rows%20this%20re-query%20returns%20no%20row%2C%20so%20ownership%20check%20falls%20back%20to%20an%20empty%20owner%20list%20and%20incorrectly%20raises%20forbidden%20for%20non-admin%20users%2C%20making%20restore%20fail%20even%20for%20valid%20owners.%20Use%20an%20ownership%20check%20path%20that%20does%20not%20re-query%20through%20the%20filtered%20session%20%28or%20passes%20the%20visibility-skip%20option%29%20for%20restore%20flows.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C% 20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcommands%2Frestore.py%0A%2A%2ALine%3A%2A%2A%2078%3A81%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20%60raise_for_ownership%28%29%60%20re-queries%20the%20object%20with%20a%20plain%20ORM%20query%2C%20which%20is%20still%20subject%20to%20the%20global%20soft-delete%20visibility%20filter.%20For%20soft-deleted%20rows%20this%20re-query%20returns%20no%20row %2C%20so%20ownership%20check%20falls%20back%20to%20an%20empty%20owner%20list%20and%20incorrectly%20raises%20forbidden%20for%20non-admin%20users%2C%20making%20restore%20fail%20even%20for%20valid%20owners.%20Use%20an%20ownership%20check%20path%20that%20does%20not%20re-query%20through%20the%20filtered%20session%20%28or%20passes%20the%20visibility-skip%20option%29%20for%20restore%20flows.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/commands/restore.py **Line:** 78:81 **Comment:** *Api Mismatch: `raise_for_ownership()` re-queries the object with a plain ORM query, which is still subject to the global soft-delete visibility filter. For soft-deleted rows this re-query returns no row, so ownership check falls back to an empty owner list and incorrectly raises forbidden for non-admin users, making restore fail even for valid owners. Use an ownership check path that does not re-query through the filtered session (or passes the visibility-skip option) for restore flows. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39977&comment_hash=78f44a444c1aa813d9a4c29e40182fadae4f12902ced5491ac3a4674b80ae347&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39977&comment_hash=78f44a444c1aa813d9a4c29e40182fadae4f12902ced5491ac3a4674b80ae347&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
