fitzee commented on code in PR #44025:
URL: https://github.com/apache/superset/pull/44025#discussion_r3995893954
##########
superset/commands/dashboard/update.py:
##########
@@ -119,6 +128,9 @@ def validate(self) -> None:
except SupersetSecurityException as ex:
raise DashboardForbiddenError() from ex
+ if self._refuses_externally_managed:
+ raise_if_managed_externally(self._model, DashboardForbiddenError)
Review Comment:
**The blanket gate also blocks `published`, a Superset-local field not owned
by the external sync.** `savePublished` (`dashboardState.ts:243-255`) issues
`PUT /api/v1/dashboard/<id>` with `{published: isPublished}`, which routes
through `UpdateDashboardCommand` → this gate → 403 on a managed dashboard.
Publish/unpublish is local visibility state, not part of the external source of
truth, so toggling it now fails with an error toast even for an owner. Worth
confirming this is intended — if not, `published`-only PUTs (and possibly other
local-only fields) should be exempted the way colors are. No test currently
pins the behavior either way.
##########
superset/commands/dashboard/update.py:
##########
@@ -283,17 +295,53 @@ def run(self) -> Model:
class UpdateDashboardColorsConfigCommand(UpdateDashboardCommand):
+ # The blanket gate is skipped so background colors sync (fired while a
+ # dashboard is merely viewed) keeps working for externally managed
+ # dashboards -- but only for the DERIVED color values. The authoritative
+ # inputs are the dashboard's real content, owned by the external source
+ # of truth; validate() refuses a payload that would change them.
+ _refuses_externally_managed = False
+
+ #: json_metadata keys a colors-config save may NOT change on an
+ #: externally managed dashboard. The other accepted keys
+ #: (color_scheme_domain, shared_label_colors, map_label_colors) are
+ #: derived from these plus chart state (see
+ #: DashboardDAO.update_colors_config).
+ _AUTHORITATIVE_COLOR_KEYS: tuple[str, ...] = ("color_scheme",
"label_colors")
+
def __init__(
self, model_id: int, data: dict[str, Any], mark_updated: bool = True
) -> None:
super().__init__(model_id, data)
self._mark_updated = mark_updated
+ def validate(self) -> None:
+ super().validate()
+ assert self._model
+ if self._model.is_managed_externally and
self._changes_authoritative_colors():
+ raise DashboardForbiddenError()
+
+ #: Sentinel distinguishing "key absent from stored metadata" from an
+ #: explicit null: an incoming ``color_scheme: null`` on a dashboard
+ #: whose metadata lacks the key would otherwise compare equal to the
+ #: ``.get()`` default and slip the gate — yet the DAO would then write
+ #: a literal null key into the exported json_metadata, a real change.
+ _METADATA_MISSING: object = object()
+
+ def _changes_authoritative_colors(self) -> bool:
+ assert self._model
+ metadata = json.loads(self._model.json_metadata or "{}")
+ return any(
+ key in self._properties
+ and self._properties[key] != metadata.get(key,
self._METADATA_MISSING)
Review Comment:
**This defeats the derived-colors carve-out for the common managed-dashboard
case.** The background sync `storeDashboardColorConfig`
(`superset-frontend/src/dashboard/actions/dashboardState.ts:1271`) *always*
sends `label_colors: metadata.label_colors || {}` as a top-level key, and that
body becomes `self._properties`. For a managed dashboard whose stored
`json_metadata` has **no** `label_colors` key (the common case), this computes
`self._properties['label_colors']` = `{}` vs `metadata.get('label_colors',
_METADATA_MISSING)` = the sentinel → `{} != sentinel` → `True` →
`_changes_authoritative_colors()` → `DashboardForbiddenError` (403).
So the fire-and-forget colors sync 403s on every load, and the derived
values (`color_scheme_domain`/`shared_label_colors`/`map_label_colors`) it was
meant to persist never get written — permanently defeating the exemption this
subclass exists to preserve. It's not even a real change: empty-`{}` vs
absent-key is semantically identical. The same shape applies to `color_scheme`
when a caller coerces it to `''` (dashboardState.ts:534) against a dashboard
with no stored `color_scheme`. Suggest normalizing absent ≡ empty (treat
`{}`/`''`/absent as equal) before the sentinel comparison.
##########
superset/commands/dashboard/update.py:
##########
@@ -56,6 +57,14 @@
class UpdateDashboardCommand(UpdateMixin, BaseCommand):
+ #: Ordinary edits of an externally managed dashboard are refused
+ #: server-side (see ``raise_if_managed_externally``).
+ #: ``UpdateDashboardColorsConfigCommand`` flips this off so background
+ #: colors sync keeps working while a dashboard is merely viewed -- but
+ #: only for derived color values; its validate() override refuses
+ #: changes to the authoritative inputs.
+ _refuses_externally_managed: bool = True
Review Comment:
**Two sibling commands silently inherit this gate with no test coverage.**
`UpdateDashboardNativeFiltersCommand` (line 253) and
`UpdateDashboardChartCustomizationsCommand` (line 268) both subclass
`UpdateDashboardCommand` and call `super().validate()`, so `PUT
/api/v1/dashboard/<id>/filters` and `.../chart_customizations` now also 403 on
managed dashboards. Only the colors path got an explicit carve-out and tests.
If either of these is ever persisted as a background/derived write while a
dashboard is merely viewed, it regresses exactly like #1 — and there's no test
pinning either sibling's new behavior. Worth a deliberate decision (gate or
exempt) plus a test for each.
##########
UPDATING.md:
##########
@@ -69,6 +69,9 @@ tags are included in asset export and import.
Set `FEATURE_FLAGS = {"TAGGING_SYSTEM": False}` to restore the previous
behavior. Existing tag rows are left untouched.
+### Updates of externally managed entities are refused server-side
Review Comment:
**Nit:** this `### …` heading is inserted with no blank line above it
(directly after `…Existing tag rows are left untouched.`). The repo's
`.markdownlint.json` only disables `no-bare-urls` and `line-length`, so MD022
(blanks-around-headings) is active and would flag this; it's also inconsistent
with the blank-line spacing every other heading in the file uses. (GFM's ATX
headings *do* interrupt paragraphs, so it still renders as a heading on GitHub
— this is a lint/consistency nit, not a rendering break.) Add a blank line
before the heading.
##########
superset/charts/schemas.py:
##########
@@ -356,6 +357,25 @@ class ChartPutSchema(Schema):
Schema to update or patch a chart
"""
+ # pylint: disable=unused-argument
+ @pre_load
+ def _discard_is_managed_externally(
Review Comment:
**Nit (DRY):** this 15-line `@pre_load _discard_is_managed_externally` hook
+ the re-declared `is_managed_externally` field + docstring are copy-pasted
verbatim into `superset/charts/schemas.py`, `superset/dashboards/schemas.py`,
and `superset/datasets/schemas.py`. A future change to the discard semantics
(e.g. also stripping a sibling field, or fixing the pop logic) has to be made
in three places and can drift. A shared `DiscardManagedExternallyMixin` would
centralize it.
--
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]
