mikebridge commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5605216568
@aminghadersohi — requesting review on the `is_managed_externally`
server-side gate (sc-120011), the update-path counterpart of the restore gate
in #44013. Data-integrity fix, not privilege escalation: an
otherwise-authorized editor could mutate an externally managed
chart/dashboard/dataset via direct `PUT` (the UI only hides the affordances),
and the change would be overwritten on the next external sync.
Shape: one shared `raise_if_managed_externally` helper called after the
editorship check in all three `UpdateCommand.validate()`s, raising each
entity's existing 403 exception so the refusal is indistinguishable from a
permission denial. The PR went through two adversarial review rounds beyond my
own lens gate, which hardened it well past the original scope — worth knowing
as you review:
- both initially-proposed "derived state" exemptions turned out to be live
integrity holes and were closed: the chart **query-context-only save is gated**
(stored query context is executable — report execution runs it; nothing
server-side writes it, so report machinery is unaffected), and the dashboard
**colors sync is narrowed to derived values** (`color_scheme`/`label_colors`
refused when changed on a managed dashboard; the background view-time sync
keeps flowing);
- `PUT /dataset/<pk>/refresh` is gated (fetch_metadata persists column
metadata the external sync owns);
- the **legacy Explore overwrite** (`/superset/explore/`,
`action=overwrite`) is gated — it writes `query_context` via `ChartDAO.update`,
bypassing the command layer. En route this fixed a latent pre-existing bug:
that path's editorship denial passed a flask-babel LazyString to
`json_error_response` and has been shipping empty `{}` 403 bodies (both denials
now eager-translate);
- the PUT schemas **discard `is_managed_externally` on load** (declared for
the OpenAPI contract, popped in `pre_load`) — it was client-writable, and a
client-set `true` would have been irreversible once the gate exists.
Remaining direct DAO writers (MCP tools, certification paths) are
inventoried in a follow-up ticket rather than folded in. Tests: 15 unit
(parametrized across the three entities; reverting the production change flips
7) + 2 integration (REST PUT 403 + legacy overwrite 403, both asserting the
body). CI fully green at cf2f4cbd04 including the DB matrix, unit lane, and
openapi-spec-drift (the two commits after f7d40c0ff7 are test-only:
mock-fixture flag pins and a docstring nit).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
--
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]
