aminghadersohi commented on code in PR #40129:
URL: https://github.com/apache/superset/pull/40129#discussion_r3342336291
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -48,20 +48,86 @@ def import_chart(
overwrite: bool = False,
ignore_permissions: bool = False,
) -> Slice:
+ """Import a chart from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the chart back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission
+ (which would let them reattach dashboards to a deleted chart).
+ """
can_write = ignore_permissions or security_manager.can_access("can_write",
"Chart")
- existing = db.session.query(Slice).filter_by(uuid=config["uuid"]).first()
+ from superset.commands.importers.v1.utils import find_existing_for_import
+
user = get_user()
Review Comment:
**MEDIUM (×2) — undocumented `user=None` bypass + redundant `get_user()`
call**
**1. `user=None` path not in the permission matrix**
When `can_write=True` (e.g., via `ignore_permissions=True`) and `get_user()`
returns `None`, the ownership check at lines 88–93 is silently skipped because
the guard `if needs_mutation and can_write and user:` short-circuits. The
docstring table has no row for this case. Suggested inline comment:
```python
# When user is None (background import with ignore_permissions=True),
# the ownership check below is intentionally skipped — the caller has
# established trust at the command level.
user = get_user()
```
**2. Redundant `get_user()` call at end of function**
The pre-existing line near the end of `import_chart` reads:
```python
if (user := get_user()) and user not in chart.owners:
```
This calls `get_user()` again despite `user` already being bound here — same
request, same return value. The walrus rebinding also silently replaces the
line-83 binding in the outer scope.
Suggested fix for that line:
```python
if user and user not in chart.owners:
chart.owners.append(user)
```
--
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]
