mikebridge opened a new pull request, #43628:
URL: https://github.com/apache/superset/pull/43628
### SUMMARY
`POST
/api/v1/{chart,dashboard,dataset}/<uuid>/versions/<version_uuid>/restore` never
checked
`is_managed_externally`. The dashboard and chart UIs hide the restore
control for externally managed
entities, but any direct caller with edit rights — a script, a notebook, an
integration — could restore an
older version over a dbt-synced dataset or an externally provisioned
dashboard. The next sync then either
overwrites the restore or resurrects content the external owner had removed.
The published documentation
(`docs/using-superset/version-history.mdx`) already states that restore "is
withheld from externally managed
entities"; this PR makes that statement true.
**The change** is one gate and one mapping:
- `BaseRestoreVersionCommand.validate()` gains a fourth check, after the
editorship check, raising a new
shared `ExternallyManagedRestoreError` (a `ForbiddenError` subclass).
Because the three per-entity restore
commands are pure declarations over that base, one edit covers charts,
dashboards, and datasets identically.
- `restore_version_endpoint()` maps it to `403 {"message": "Version restore
is unavailable for externally
managed entities."}` via `api.response(403, message=…)`. FAB's
`response_403()` always emits
`{"message": "Forbidden"}`, so a caller can distinguish a policy refusal
from a permission denial by the body.
**Design notes reviewers will want:**
- **Ordering.** The gate runs *after* `raise_for_editorship`, so a caller
with no rights to the entity still
gets the plain permission denial and is never told the entity is
externally managed. It runs *before* version
resolution, so a refused restore writes nothing — no version row, no
partial update.
- **This is the first server-side use of `is_managed_externally`.** No
update or delete command enforces the
flag today; those operations remain gated in the browser alone. After this
PR a permitted editor is refused a
version restore but can still submit an equivalent direct `PUT`. That
asymmetry is accepted deliberately: the
documentation promises the restriction for restore and for nothing else,
and widening enforcement to
update/delete is a product decision for the Versioning epic rather than
this correctness fix. Follow-up:
_to be filed before this leaves draft_.
- **Soft-delete recovery is deliberately not gated.** Recovering an archived
externally managed entity changes
visibility, not content, and is a useful escape hatch when a synced asset
is archived by mistake. The
asymmetry is pinned by a test on one and the same entity: archive →
recover succeeds → version restore
refused.
- **No `UPDATING.md` entry.** A released endpoint does change from 200 to
403 for one class of caller, but the
change brings the API into line with already-published documentation and
is a correctness fix, not a
boundary change.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — no UI change. Before/after is the API response below.
### TESTING INSTRUCTIONS
Automated (all control-first — every behavioural test was run red against
the pre-fix code first):
```bash
pytest tests/unit_tests/versioning/ # 140
passed (14 new)
pytest tests/integration_tests/charts/version_restore_tests.py \
tests/integration_tests/dashboards/version_restore_tests.py \
tests/integration_tests/datasets/version_restore_tests.py # 37
passed (4 new)
```
Manual:
1. Mark a chart externally managed (`UPDATE slices SET is_managed_externally
= true WHERE uuid = '<uuid>'`).
2. As a user who can edit it, `POST
/api/v1/chart/<uuid>/versions/<version_uuid>/restore`.
- **Before:** `200 {"message": "OK"}`; the chart is rewritten and —
because the flag is itself a versioned
column — restoring an older version also silently flips
`is_managed_externally` back to `false`.
- **After:** `403 {"message": "Version restore is unavailable for
externally managed entities."}`; chart and
version history unchanged.
3. As a user who *cannot* edit it: `403 {"message": "Forbidden"}` —
unchanged, and the body differs from step 2.
4. `GET …/versions/`, `GET …/versions/<v>/`, `GET …/activity/` all still
`200` for the managed chart.
5. `DELETE /api/v1/chart/<id>` then `POST /api/v1/chart/<uuid>/restore`
(recovery): both `200`; a version
restore immediately after is still `403`.
### ADDITIONAL INFORMATION
- [x] Has associated issue: sc-115616 (origin: scope note in the
verification review of #41551)
- [ ] Required feature flags: none new (`VERSION_HISTORY` /
`ENABLE_VERSIONING_CAPTURE`, already on by default)
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
