mikebridge commented on code in PR #44025:
URL: https://github.com/apache/superset/pull/44025#discussion_r4007280859
##########
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:
Confirmed and fixed in f3c23dcfa9 — the gate now compares EFFECTIVE color
state (absent ≡ null ≡ empty), so the background sync's `label_colors: {}`
passes on a managed dashboard whose metadata lacks the key. This deliberately
supersedes the earlier null-vs-absent sentinel pin, which overcorrected
byte-equality into a view-time 403; the real controls hold both ways (clearing
a key that holds state refuses; setting a value on an absent key refuses).
##########
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:
Fixed in f3c23dcfa9 — agreed it's local state: `published`-only updates now
pass the managed gate (savePublished PUTs exactly `{published}`); bundling any
content field re-arms it. Both directions pinned.
##########
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:
Pinned in f3c23dcfa9 as deliberate: native-filters and chart-customizations
are dashboard CONTENT owned by the external source of truth, written at edit
time (not view-time background syncs like colors), so the inherited refusal is
intended — now declared by a parametrized test over both siblings.
##########
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:
Done in f3c23dcfa9 — shared `DiscardIsManagedExternallyMixin` in
`superset/utils/schema.py`, mixed into all three PUT schemas; the parametrized
discard test still covers 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:
Fixed in f3c23dcfa9 — blank line added before the heading.
--
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]
