You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/09/08 21:09:18 UTC

[PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

mikebridge opened a new pull request, #44025:
URL: https://github.com/apache/superset/pull/44025

   ### SUMMARY
   
   An entity with `is_managed_externally = True` has its source of truth outside Superset: an in-app write would be overwritten on the next external sync, which is why the UI hides the edit affordances for such entities. But the refusal was browser-only — an otherwise-authorized editor could still mutate the entity by calling `PUT /api/v1/{chart,dashboard,dataset}/<id>` directly. This is the ordinary-update sibling of the gap #44013 closed for version restore, and closing it keeps the restore and update surfaces from drifting.
   
   This PR adds a shared `raise_if_managed_externally` helper in `superset/commands/utils.py` and calls it from all three `UpdateCommand.validate()` implementations:
   
   - **Placement**: after the editorship check, so a caller with no edit rights at all keeps receiving the plain editorship denial.
   - **Response shape**: the helper raises each entity's existing editorship-denial exception (`ChartForbiddenError` / `DashboardForbiddenError` / `DatasetForbiddenError`), so the HTTP 403 body is identical to a permission denial and discloses nothing new.
   - **Deliberate exemption**: the chart *query-context-only* save path is not gated. That branch exists so report workers can persist refreshed query context (it already relaxes editorship for the same reason), and query context is derived state rather than an edit the external source of truth owns. Blocking it would break reports on externally managed charts.
   
   Scope note: this is a **data-integrity consistency fix**, not a privilege escalation — an editor holding edit rights is entitled to mutate these entities under the role/capability matrix; `is_managed_externally` is an external-sync integrity concern.
   
   There is precedent for the check in this very layer: `UpdateChartCommand` already refuses linking a chart to an externally managed *target dashboard* (`superset/commands/chart/update.py`), and version restore enforces the same gate server-side (#44013).
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   N/A — API-only behavior change. Before: `PUT` on an externally managed entity returned 200 and mutated it. After: HTTP 403, entity unchanged.
   
   ### TESTING INSTRUCTIONS
   
   - `pytest tests/unit_tests/commands/test_update_managed_externally.py` — 7 tests: parametrized across chart/dashboard/dataset, an externally managed entity is refused with the entity's 403 exception after the editorship check was consulted; the gate is inert (called, no raise) for locally managed entities; the helper refuses exactly when `is_managed_externally` is set. Reverting only the three call sites flips 6 of the 7 tests (the helper-only test stands), so the pins are non-vacuous.
   - `tests/integration_tests/charts/api_tests.py::TestChartApi::test_update_chart_refuses_externally_managed` — representative real-endpoint case: admin `PUT` on an externally managed chart returns 403 and the chart is unchanged.
   - Manual: flip `is_managed_externally` on any chart, then `PUT /api/v1/chart/<id>` with `{"slice_name": "x"}` as an admin → 403; flip it back → 200.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   🤖 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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5614146148

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
   
   <details>
   <summary>
   Bito Usage Guide
   </summary>
   
   **Commands**
         
   Type the following command in the pull request comment and save the comment.
         
   - `/review` - Manually triggers an incremental AI Review.
   - `/review full` - Manually triggers a full AI Review.
   
   - `/pause` - Pauses automatic reviews on this pull request.
   - `/resume` - Resumes automatic reviews.
   - `/resolve` - Marks all Bito-posted review comments as resolved.
   - `/abort` - Cancels all in-progress reviews.
   
         
   Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
         
   **Configuration**
   
   This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
         
   **Documentation & Help**
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
   
   </details>
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44025:
URL: https://github.com/apache/superset/pull/44025#discussion_r3995226360


##########
superset/commands/dashboard/update.py:
##########
@@ -283,17 +295,45 @@ 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()
+
+    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)
+            for key in self._AUTHORITATIVE_COLOR_KEYS

Review Comment:
   Addressed in 20f6d1d910 — `_changes_authoritative_colors` now uses a `_METADATA_MISSING` sentinel so an explicit `color_scheme: null` against a metadata blob lacking the key is detected as a change; parametrized pins cover null-vs-absent in both directions.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5638004585

   <!-- Bito Reply -->
   The flagged issue is correct. In `UpdateDashboardColorsConfigCommand`, the logic `key in self._properties and self._properties[key] != metadata.get(key)` fails to detect a change when the new value is `None` (explicitly provided) and the key is missing from the existing `metadata` (where `metadata.get(key)` returns `None`).
   
   To resolve this, you should check if the key is present in `self._properties` and compare the values directly, ensuring that `None` is treated as a distinct value from a missing key if necessary, or simply ensure the comparison handles the absence of the key correctly.
   
   Here is a corrected version of `_changes_authoritative_colors`:
   
   ```python
       def _changes_authoritative_colors(self) -> bool:
           assert self._model
           metadata = json.loads(self._model.json_metadata or "{}")
           for key in self._AUTHORITATIVE_COLOR_KEYS:
               if key in self._properties:
                   # Check if the value is actually changing
                   if self._properties[key] != metadata.get(key):
                       return True
           return False
   ```
   
   I have validated this fix against the provided diff. Would you like me to check the other comments on this PR and implement fixes for them as well?
   
   **superset/commands/dashboard/update.py**
   ```
   def _changes_authoritative_colors(self) -> bool:
           assert self._model
           metadata = json.loads(self._model.json_metadata or "{}")
           for key in self._AUTHORITATIVE_COLOR_KEYS:
               if key in self._properties:
                   # Check if the value is actually changing
                   if self._properties[key] != metadata.get(key):
                       return True
           return False
   ```


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge closed pull request #44025: fix(api): enforce is_managed_externally on chart/dashboard/dataset update
URL: https://github.com/apache/superset/pull/44025


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5669574924

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
   
   <details>
   <summary>
   Bito Usage Guide
   </summary>
   
   **Commands**
         
   Type the following command in the pull request comment and save the comment.
         
   - `/review` - Manually triggers an incremental AI Review.
   - `/review full` - Manually triggers a full AI Review.
   
   - `/pause` - Pauses automatic reviews on this pull request.
   - `/resume` - Resumes automatic reviews.
   - `/resolve` - Marks all Bito-posted review comments as resolved.
   - `/abort` - Cancels all in-progress reviews.
   
         
   Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
         
   **Configuration**
   
   This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
         
   **Documentation & Help**
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
   
   </details>
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #44025:
URL: https://github.com/apache/superset/pull/44025


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "fitzee (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5591957889

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44025?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `28.57143%` with `5 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 57.48%. Comparing base ([`7029c46`](https://app.codecov.io/gh/apache/superset/commit/7029c468c4d58b40c695b49c4f2a7806a05f1ddb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9a854d0`](https://app.codecov.io/gh/apache/superset/commit/9a854d0696a3d3bc8275633662ea34a0d43bcb78?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 1 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44025?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/commands/utils.py](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&filepath=superset%2Fcommands%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvdXRpbHMucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [superset/commands/chart/update.py](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&filepath=superset%2Fcommands%2Fchart%2Fupdate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvY2hhcnQvdXBkYXRlLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [superset/commands/dashboard/update.py](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&filepath=superset%2Fcommands%2Fdashboard%2Fupdate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGFzaGJvYXJkL3VwZGF0ZS5weQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [superset/commands/dataset/update.py](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&filepath=superset%2Fcommands%2Fdataset%2Fupdate.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNldC91cGRhdGUucHk=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (7029c46) and HEAD (9a854d0). Click for more details.
   > 
   > <details><summary>HEAD has 6 uploads less than BASE</summary>
   >
   >| Flag | BASE (7029c46) | HEAD (9a854d0) |
   >|------|------|------|
   >|python|7|1|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #44025       +/-   ##
   ===========================================
   - Coverage   79.85%   57.48%   -22.37%     
   ===========================================
     Files        2916     2916               
     Lines      170691   170697        +6     
     Branches    39550    39551        +1     
   ===========================================
   - Hits       136307    98127    -38180     
   - Misses      31867    71648    +39781     
   + Partials     2517      922     -1595     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.47% <28.57%> (-0.01%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.47% <28.57%> (-44.79%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/44025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44025?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5592386445

   ### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">🔨</span> Latest commit | 294ae151638a3eaba9ca75e8e61f073f507b59cc |
   |<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa083a54a529800081a70ff |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44025--superset-docs-preview.netlify.app](https://deploy-preview-44025--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ0MDI1LS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.H07UoyuTOLHVsZGaswWEjHhujg3a6E7ouZ3p0OVEsqw)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBDoMgEEbhu8y6kwy2KO0Neoy_MuoCkFBMNMa7y-7lJd9JuawxV_oQPehXkMal9X9k04kYw1v2qMoRCbN61r1qSQjh4Ln9ZrzmsB5f31QPiHvCvmC7txMRZzDINNF1Aw) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-44025--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5592420546

   ### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">🔨</span> Latest commit | 3c37f0837fee4dc2c8909de58a136e1b54db9a97 |
   |<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa0840a3940580008c17e9b |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44025--superset-docs-preview.netlify.app](https://deploy-preview-44025--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ0MDI1LS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.H07UoyuTOLHVsZGaswWEjHhujg3a6E7ouZ3p0OVEsqw)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBCoAgEEDRu8y6gbGsrBt0jEmHWqiJGRTR3XP3-fBeSPkIqcAM0MCaOdq99mlRtURK4ZUcF8HAkTdxKHeRHNn7B7f6q3GS_PEsrqqBmYwm7iZNvSEiY9Uo0wrfDw) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-44025--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5639848221

   The codeant/bito finding on `_changes_authoritative_colors` is correct and fixed in 20f6d1d910: an explicit `color_scheme: null` (or `label_colors: null`) compared equal to `metadata.get(key)`'s None default when the stored metadata lacked the key — yet the DAO writes every provided key, so the managed dashboard's exported `json_metadata` would have gained a literal null entry. The comparison now uses a missing-key sentinel (absent ≠ null), pinned by parametrized tests for both authoritative keys.
   
   🤖 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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44025:
URL: https://github.com/apache/superset/pull/44025#discussion_r3991634368


##########
superset/commands/dashboard/update.py:
##########
@@ -283,17 +295,45 @@ 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()
+
+    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)
+            for key in self._AUTHORITATIVE_COLOR_KEYS

Review Comment:
   **Suggestion:** An explicit `color_scheme: null` is treated as unchanged when the metadata lacks that key, but the DAO still persists the new authoritative key. [incorrect condition logic]
   
   **Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83cfe4f355f24aea904c885d8c27df50&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=83cfe4f355f24aea904c885d8c27df50&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/commands/dashboard/update.py
   **Line:** 327:329
   **Comment:**
   	*Incorrect Condition Logic: An explicit `color_scheme: null` is treated as unchanged when the metadata lacks that key, but the DAO still persists the new authoritative key.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44025&comment_hash=dbbf3faa9064eecc0c34979d023c7b6e984bb5712d4f9f8f2c2892dffbed40f7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44025&comment_hash=dbbf3faa9064eecc0c34979d023c7b6e984bb5712d4f9f8f2c2892dffbed40f7&reaction=dislike'>👎</a>



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5688925900

   **Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save 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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5640097944

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
   
   <details>
   <summary>
   Bito Usage Guide
   </summary>
   
   **Commands**
         
   Type the following command in the pull request comment and save the comment.
         
   - `/review` - Manually triggers an incremental AI Review.
   - `/review full` - Manually triggers a full AI Review.
   
   - `/pause` - Pauses automatic reviews on this pull request.
   - `/resume` - Resumes automatic reviews.
   - `/resolve` - Marks all Bito-posted review comments as resolved.
   - `/abort` - Cancels all in-progress reviews.
   
         
   Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
         
   **Configuration**
   
   This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
         
   **Documentation & Help**
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
   - <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
   
   </details>
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(api): enforce is_managed_externally on chart/dashboard/dataset update [superset]

Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44025:
URL: https://github.com/apache/superset/pull/44025#issuecomment-5591903249

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org