You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ri...@apache.org on 2026/09/14 21:04:46 UTC
(superset) branch master updated: fix(versioning): edit-gate the version-history and activity endpoints (#44021)
This is an automated email from the ASF dual-hosted git repository.
richardfogaca pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 43a00319956 fix(versioning): edit-gate the version-history and activity endpoints (#44021)
43a00319956 is described below
commit 43a0031995636537ce5ff8909e4b7de07a0ee4d5
Author: Mike Bridge <mi...@preset.io>
AuthorDate: Mon Sep 14 15:04:38 2026 -0600
fix(versioning): edit-gate the version-history and activity endpoints (#44021)
Co-authored-by: Claude Fable 5 <no...@anthropic.com>
---
UPDATING.md | 4 +
superset/security/manager.py | 22 ++-
superset/versioning/activity/orchestrator.py | 2 +-
superset/versioning/activity/render.py | 4 +-
superset/versioning/api_helpers.py | 91 ++++++---
superset/versioning/restore.py | 2 +-
.../versioning/activity_view_tests.py | 35 ++--
.../versioning/versions_api_tests.py | 203 ++++++++++++++++++-
tests/unit_tests/versioning/test_endpoint_gate.py | 216 +++++++++++++++++++++
tests/unit_tests/versioning/test_restore.py | 2 +-
10 files changed, 519 insertions(+), 62 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index 7c71ca00551..951f2e0cf33 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -93,6 +93,10 @@ 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.
+### Version-history and activity endpoints are edit-gated
+
+Version-history and activity endpoints (`GET /api/v1/{chart,dashboard,dataset}/<uuid>/versions/…` and `…/activity/`) are now edit-gated: they require object-level editorship (owner/editor/admin) of the entity, matching the UI's edit-gated Version history menu and the restore endpoint's gate. Read-only users who could previously retrieve the full change log (author identities, field-level before/after diffs) via the API now receive 403. Embedded guest-token principals are always refused o [...]
+
### Global Async Queries re-platformed onto the Global Task Framework (breaking)
Global Async Queries (GAQ) no longer runs on its own bespoke async-events
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 2eb78f9c6bb..52f11dd4995 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -5822,22 +5822,32 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
Returns True if the current user is an editor of the resource.
Checks whether any of the user's subject IDs (user, roles, groups)
- are present in the resource's ``editors`` list.
+ are present in the resource's ``editors`` list. Embedded
+ guest-token principals are never editors, regardless of the
+ subjects their token's roles map to.
:param resource: The dashboard, dataset, chart, etc. resource
:returns: Whether the current user is an editor of the resource
"""
- from superset.subjects.utils import get_user_subject_ids, subjects_from_roles
+ from superset.subjects.utils import get_user_subject_ids
+
+ # SECURITY.md's capability matrix grants the embedded guest-token
+ # principal NO edit capability on any resource: a guest is never
+ # an editor, even when a ROLE subject its token carries has been
+ # granted editorship — mapping guest role subjects into the
+ # editor set would let any guest holding that role mutate or
+ # restore the entity (the M10 escalation). Denied here, at the
+ # predicate, so every enforcement caller of
+ # ``raise_for_editorship`` inherits the rule rather than each
+ # endpoint re-implementing the guest deny.
+ if self.is_guest_user():
+ return False
if self.is_admin():
return True
if user_id := get_user_id():
subject_ids = set(get_user_subject_ids(user_id))
- elif self.is_guest_user():
- subject_ids = {
- s.id for s in subjects_from_roles(getattr(g.user, "roles", []))
- }
else:
subject_ids = set()
if not subject_ids:
diff --git a/superset/versioning/activity/orchestrator.py b/superset/versioning/activity/orchestrator.py
index 641c23b78a5..8bc1f39db9e 100644
--- a/superset/versioning/activity/orchestrator.py
+++ b/superset/versioning/activity/orchestrator.py
@@ -250,7 +250,7 @@ def get_activity(
*resolved_entity*, when supplied, is the already-resolved live path
entity (the endpoint resolves it once via ``resolve_endpoint_path_entity``
- for the ``raise_for_access`` gate); passing it here skips a second
+ for the editorship gate); passing it here skips a second
identical ``find_active_by_uuid`` lookup and the TOCTOU window between
them. The count is post-visibility (silent visibility filter),
post-include-filter, and — when ``q`` is supplied — post-
diff --git a/superset/versioning/activity/render.py b/superset/versioning/activity/render.py
index 51b6811d4cc..4c0e63c74bd 100644
--- a/superset/versioning/activity/render.py
+++ b/superset/versioning/activity/render.py
@@ -183,8 +183,8 @@ def apply_record_decoration(
# "(deleted) <kind>" marker — so the stream stays honest
# about WHEN something changed without disclosing WHAT, WHO,
# or WHICH entity. Self-path tombstones are untouched: the
- # endpoint already gated them via ``raise_for_access`` on the
- # path entity.
+ # endpoint already gated them via ``raise_for_editorship``
+ # on the path entity (an edit gate, stricter than read).
label = API_KIND_LABEL.get(api_kind, api_kind)
record["entity_name"] = ""
record["summary"] = f"(deleted) {label}"
diff --git a/superset/versioning/api_helpers.py b/superset/versioning/api_helpers.py
index dfe0e2d63a7..58131bd24fb 100644
--- a/superset/versioning/api_helpers.py
+++ b/superset/versioning/api_helpers.py
@@ -19,8 +19,9 @@
Each ``ChartRestApi`` / ``DashboardRestApi`` / ``DatasetRestApi`` carries
the same read endpoint methods — ``list_versions`` and ``get_version`` —
plus the ``activity`` endpoint on each resource. The bodies are
-byte-for-byte identical apart from the model class and the
-``security_manager.raise_for_access`` kwarg. Extracting the bodies here
+byte-for-byte identical apart from the model class; authorization is a
+single object-level editorship gate in ``resolve_endpoint_path_entity``
+(``security_manager.raise_for_editorship``). Extracting the bodies here
lets each per-resource method collapse to a single delegation call, while
the OpenAPI docstring + FAB decorators stay at the method site where they
belong.
@@ -33,6 +34,7 @@ restore command's ``validate()``, not here.
from __future__ import annotations
+import functools
import logging
from dataclasses import dataclass
from typing import Any
@@ -43,7 +45,6 @@ from flask import Response
from flask_appbuilder import Model
from superset.daos.version import VersionDAO
-from superset.exceptions import SupersetSecurityException
from superset.extensions import db, security_manager
from superset.versioning.etag import set_version_etag_by_uuid
from superset.versioning.schemas import VersionListItemSchema
@@ -269,16 +270,23 @@ def concurrency_token_from(info: EntityVersionInfo) -> str | None:
return info.version_uuid or unversioned_entity_token(info.entity_uuid)
-# Maps the versioned model class name to the keyword argument
-# ``security_manager.raise_for_access`` expects for the per-resource
-# gate. Slice → ``chart=``, Dashboard → ``dashboard=``, SqlaTable →
-# ``datasource=``. Centralised here so /versions/ and /activity/
-# endpoints share one source of truth for the dispatch.
-_RAISE_FOR_ACCESS_KWARG: dict[str, str] = {
- "Slice": "chart",
- "Dashboard": "dashboard",
- "SqlaTable": "datasource",
-}
+@functools.cache
+def _version_endpoint_models() -> tuple[type, ...]:
+ """The exact model classes wired for the version endpoint families.
+
+ An explicit allowlist so a future entity added to the versioning
+ surface without an authorization decision fails closed rather than
+ silently inheriting a gate. Compared by class IDENTITY, not name —
+ an unrelated class that happens to be called ``Slice`` /
+ ``Dashboard`` / ``SqlaTable`` must not slip through. Deferred
+ imports keep this module out of the model-import cycle.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ return (Slice, Dashboard, SqlaTable)
class PathEntityResponseError(Exception):
@@ -286,7 +294,7 @@ class PathEntityResponseError(Exception):
Endpoints catch it and return
the carried response directly. The shape exists so the
- UUID-parse + find-by-uuid + read-access check can live in one
+ UUID-parse + find-by-uuid + editorship check can live in one
place across the ``/versions/`` and ``/activity/`` endpoint
families."""
@@ -303,8 +311,8 @@ def resolve_endpoint_path_entity(
1. Parse *uuid_str* into a UUID (or raise → 400).
2. Look up the live entity via ``VersionDAO.find_active_by_uuid``
(or raise → 404).
- 3. Run ``security_manager.raise_for_access`` with the resource-typed
- kwarg (or raise → 403).
+ 3. Enforce object-level editorship via
+ ``security_manager.raise_for_editorship`` (or raise → 403).
Returns ``(entity, entity_uuid)`` on success — the parsed UUID is
threaded out so callers don't re-parse the path-string. Raises
@@ -322,29 +330,50 @@ def resolve_endpoint_path_entity(
``api.response_400`` / ``api.response_403`` / ``api.response_404``
on it. Pass ``self`` from the endpoint method.
"""
+ # Static wiring validation runs first: an unwired model must fail
+ # closed loudly before any parsing or database work, not surface as
+ # an incidental AttributeError inside the DAO lookup.
+ if model_cls not in _version_endpoint_models():
+ raise LookupError(
+ f"Model {model_cls.__name__!r} is not wired for version endpoints"
+ )
+
try:
entity_uuid = UUID(uuid_str)
except ValueError as exc:
raise PathEntityResponseError(api.response_400(message="Invalid UUID")) from exc
+ # M10 / SECURITY.md's guest row: an embedded guest's capability is
+ # reading the dashboards its token authorizes — never their change
+ # logs (author identities, field-level diffs). ``is_editor`` itself
+ # refuses guest principals; the extra deny here runs BEFORE the
+ # database lookup so a guest probing UUIDs gets a uniform 403 whether
+ # or not the entity exists — the 403-vs-404 split would otherwise
+ # disclose which UUIDs exist to a principal with no read visibility
+ # into them.
+ if security_manager.is_guest_user():
+ raise PathEntityResponseError(api.response_403())
+
entity = VersionDAO.find_active_by_uuid(model_cls, entity_uuid)
if entity is None:
raise PathEntityResponseError(api.response_404())
-
- # Direct ``[…]`` would leak the unknown model name into a generic 500
- # via the unhandled ``KeyError`` exception text. The three resource
- # families wired today cover every key; a future entity added to the
- # versioning surface without updating this dispatch table should fail
- # closed (the test suite picks it up) rather than silently disclose.
- kwarg = _RAISE_FOR_ACCESS_KWARG.get(model_cls.__name__)
- if kwarg is None:
- raise LookupError(
- f"No raise_for_access kwarg registered for {model_cls.__name__!r}"
- )
- try:
- security_manager.raise_for_access(**{kwarg: entity})
- except SupersetSecurityException as exc:
- raise PathEntityResponseError(api.response_403()) from exc
+ # Version history is EDIT-gated, not read-gated (sc-120001 decision,
+ # following the sc-103156 SIP): the full change log — author
+ # identities, timestamps, field-level before/after diffs — is for
+ # principals who may alter the entity, matching the UI's edit-gated
+ # menu and the restore command's gate. Object-level editorship
+ # (owner/editor/admin via ``raise_for_editorship``) rather than
+ # model-level ``can_write``, so a write-capable role cannot read the
+ # history of entities it does not own. Related-entity records inside
+ # the ACTIVITY stream additionally pass per-record read-visibility
+ # filtering (AV-008's silent filter), which is unchanged.
+ # Same predicate as ``raise_for_editorship`` (the restore command's
+ # gate) without its soft-delete re-query: that re-query exists for
+ # callers holding a bypass-loaded row, and ``find_active_by_uuid``
+ # just loaded this entity live — re-fetching it would double the
+ # entity lookups on the hottest paths in this module.
+ if not security_manager.is_editor(entity):
+ raise PathEntityResponseError(api.response_403())
return entity, entity_uuid
diff --git a/superset/versioning/restore.py b/superset/versioning/restore.py
index d38a4911462..790f9c01482 100644
--- a/superset/versioning/restore.py
+++ b/superset/versioning/restore.py
@@ -63,7 +63,7 @@ logger = logging.getLogger(__name__)
#
# Unknown models fail closed (``LookupError``) rather than defaulting to a
# relation-less restore — a silently partial restore is worse than a loud
-# failure (mirrors ``_RAISE_FOR_ACCESS_KWARG`` in ``api_helpers``).
+# failure (mirrors ``_version_endpoint_models`` in ``api_helpers``).
_RESTORE_RELATIONS: dict[str, list[str]] = {
"SqlaTable": ["columns", "metrics"],
"Dashboard": [],
diff --git a/tests/integration_tests/versioning/activity_view_tests.py b/tests/integration_tests/versioning/activity_view_tests.py
index 391a44b36c3..a4ae1435427 100644
--- a/tests/integration_tests/versioning/activity_view_tests.py
+++ b/tests/integration_tests/versioning/activity_view_tests.py
@@ -123,12 +123,14 @@ class TestDashboardActivityView(SupersetTestCase):
rv = self._activity(str(dashboard.uuid), since="yesterday")
assert rv.status_code == 400
- def test_activity_allows_read_non_owner(self) -> None:
- """Activity is a read endpoint: a non-owner with read access (Alpha,
- which carries broad read + datasource access) can read a dashboard's
- activity stream — ``raise_for_access(dashboard=)`` does not reject —
- so the endpoint returns 200. Visibility of *related* rows is filtered
- separately, inside the activity layer."""
+ def test_activity_denies_write_capable_non_editor(self) -> None:
+ """sc-120001: activity is EDIT-gated.
+
+ A non-editor with broad read +
+ datasource access (Alpha) is refused — the endpoint enforces
+ object-level editorship (``raise_for_editorship``), matching the
+ UI's edit-gated menu, not the read gate. Visibility filtering of
+ *related* rows (AV-008) still applies for editors."""
_persist_fixture_state()
dashboard = _get_birth_names_dashboard()
assert dashboard is not None
@@ -136,7 +138,7 @@ class TestDashboardActivityView(SupersetTestCase):
self.login(ALPHA_USERNAME)
rv = self._activity(dashboard_uuid)
- assert rv.status_code == 200
+ assert rv.status_code == 403
def test_visibility_filter_silently_drops_inaccessible_related(self) -> None:
"""AV-008 security control: a related record whose entity the caller
@@ -922,16 +924,16 @@ class TestChartActivityView(SupersetTestCase):
rv = self._activity(str(chart.uuid), include="upstream")
assert rv.status_code == 400
- def test_chart_activity_allows_read_non_owner(self) -> None:
- """Same shape as the dashboard endpoint: a read-access non-owner
- (Alpha) can read a chart's activity, so ``raise_for_access(chart=)``
- does not reject and the endpoint returns 200."""
+ def test_chart_activity_denies_write_capable_non_editor(self) -> None:
+ """The chart activity endpoint refuses a write-capable non-editor.
+
+ Same edit gate as the dashboard endpoint (sc-120001)."""
_persist_fixture_state()
chart = self._get_birth_names_chart()
assert chart is not None
self.login(ALPHA_USERNAME)
rv = self._activity(str(chart.uuid))
- assert rv.status_code == 200
+ assert rv.status_code == 403
# ---- 200 happy paths ----
@@ -1123,15 +1125,16 @@ class TestDatasetActivityView(SupersetTestCase):
rv = self._activity(str(dataset.uuid), include="upstream")
assert rv.status_code == 400
- def test_dataset_activity_allows_read_non_owner(self) -> None:
- """A read-access non-owner (Alpha) can read a dataset's activity
- stream, so the read endpoint returns 200."""
+ def test_dataset_activity_denies_write_capable_non_editor(self) -> None:
+ """The dataset activity endpoint refuses a write-capable non-editor.
+
+ It shares the same edit gate (sc-120001)."""
_persist_fixture_state()
dataset = _get_birth_names_dataset()
assert dataset is not None
self.login(ALPHA_USERNAME)
rv = self._activity(str(dataset.uuid))
- assert rv.status_code == 200
+ assert rv.status_code == 403
# ---- 200 happy paths ----
diff --git a/tests/integration_tests/versioning/versions_api_tests.py b/tests/integration_tests/versioning/versions_api_tests.py
index 9828058416a..65aa80bfe9d 100644
--- a/tests/integration_tests/versioning/versions_api_tests.py
+++ b/tests/integration_tests/versioning/versions_api_tests.py
@@ -42,7 +42,11 @@ from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
from superset.utils import json
from tests.integration_tests.base_tests import SupersetTestCase
-from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME
+from tests.integration_tests.constants import (
+ ADMIN_USERNAME,
+ ALPHA_USERNAME,
+ GAMMA_USERNAME,
+)
from tests.integration_tests.fixtures.birth_names_dashboard import (
load_birth_names_dashboard_with_slices, # noqa: F401
load_birth_names_data, # noqa: F401
@@ -167,14 +171,186 @@ class TestChartVersionsApi(SupersetTestCase):
assert rv.status_code == 404, rv.data
def test_list_versions_denies_unauthorized_user(self) -> None:
- """The per-object access gate (``raise_for_access(chart=...)``) must
- refuse a user without access — as a 403, or 404 if the object isn't
- even visible to them."""
+ """The per-object editorship gate (``raise_for_editorship``) must
+ refuse a user who is not an editor — as a 403, or 404 if the object
+ isn't even visible to them."""
chart_uuid = str(self._girls_chart().uuid)
self.login(GAMMA_USERNAME)
rv = self.client.get(f"/api/v1/chart/{chart_uuid}/versions/")
assert rv.status_code in (403, 404), rv.data
+ def _births_dashboard(self) -> Dashboard:
+ # Commit first so fixture state created in this test process is
+ # visible to the request-side session (same idiom as
+ # ``_girls_chart`` and the activity suite's
+ # ``_persist_fixture_state``).
+ db.session.commit()
+ return db.session.query(Dashboard).filter(Dashboard.slug == "births").one()
+
+ def test_list_versions_denies_write_capable_non_editor_chart(self) -> None:
+ """sc-120001 pin: version history is EDIT-gated.
+
+ Alpha carries broad
+ read + datasource access — the OLD read gate admitted it — but is no
+ editor/owner of this chart, so the endpoint must refuse with 403.
+ (Reverted-gate control: with the read gate restored this test fails
+ with a 200.)"""
+ chart_uuid = str(self._girls_chart().uuid)
+ self.login(ALPHA_USERNAME)
+ rv = self.client.get(f"/api/v1/chart/{chart_uuid}/versions/")
+ assert rv.status_code == 403, rv.data
+
+ def test_list_versions_denies_write_capable_non_editor_dashboard(self) -> None:
+ """The dashboard endpoint refuses a write-capable non-editor.
+
+ This is QA TC-062/TC-066's leak, closed by the same shared edit
+ gate as the chart flavour."""
+ dashboard = self._births_dashboard()
+ self.login(ALPHA_USERNAME)
+ rv = self.client.get(f"/api/v1/dashboard/{dashboard.uuid}/versions/")
+ assert rv.status_code == 403, rv.data
+
+ def test_get_version_denies_write_capable_non_editor_chart(self) -> None:
+ """The chart get-one route runs the same edit gate before version
+ resolution.
+
+ The version uuid need not exist: the gate refuses the non-editor
+ before the snapshot is even looked up."""
+ chart_uuid = str(self._girls_chart().uuid)
+ self.login(ALPHA_USERNAME)
+ rv = self.client.get(f"/api/v1/chart/{chart_uuid}/versions/{MISSING_UUID}/")
+ assert rv.status_code == 403, rv.data
+
+ def test_get_version_denies_write_capable_non_editor_dashboard(self) -> None:
+ """The dashboard get-one route runs the same edit gate before
+ version resolution."""
+ dashboard = self._births_dashboard()
+ self.login(ALPHA_USERNAME)
+ rv = self.client.get(
+ f"/api/v1/dashboard/{dashboard.uuid}/versions/{MISSING_UUID}/"
+ )
+ assert rv.status_code == 403, rv.data
+
+ def test_list_versions_allows_object_editor(self) -> None:
+ """Object-level editorship admits (sc-120001 matrix positive case).
+
+ Gamma made an EDITOR of this one chart reads its history, while
+ remaining unable to read other charts' history (object-level,
+ not model-level can_write)."""
+ # pylint: disable=import-outside-toplevel
+ from superset import security_manager
+ from superset.subjects.utils import get_user_subject
+
+ chart = self._girls_chart()
+ chart_uuid = str(chart.uuid)
+ gamma = security_manager.find_user(GAMMA_USERNAME)
+ gamma_subject = get_user_subject(gamma.id)
+ assert gamma_subject is not None, "gamma user has no USER subject row"
+ original_editors = list(chart.editors)
+ chart.editors = [gamma_subject]
+ db.session.commit()
+ try:
+ self.login(GAMMA_USERNAME)
+ rv = self.client.get(f"/api/v1/chart/{chart_uuid}/versions/")
+ assert rv.status_code == 200, rv.data
+ finally:
+ chart = self._girls_chart()
+ chart.editors = original_editors
+ db.session.commit()
+
+ def test_editorship_gate_refuses_guest_principal(self) -> None:
+ """Guest principals never read change logs (sc-120001 / M10 pin).
+
+ An embedded guest-token principal is never an editor, and the
+ editorship gate the version endpoints run refuses it outright —
+ guests read embedded dashboards, never their change logs.
+ Deliberately pins the gate directly (guest HTTP-session plumbing
+ isn't worth the cost here); the endpoint→gate wiring is pinned
+ by the unit not-called test and the Alpha/Gamma HTTP matrix."""
+ # pylint: disable=import-outside-toplevel
+ from unittest.mock import patch as mock_patch
+
+ from superset import security_manager
+ from superset.exceptions import SupersetSecurityException
+ from superset.security.guest_token import GuestTokenResourceType
+ from superset.utils.core import override_user
+
+ dashboard = db.session.query(Dashboard).filter(Dashboard.slug == "births").one()
+ with mock_patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ EMBEDDED_SUPERSET=True,
+ ):
+ guest = security_manager.get_guest_user_from_token(
+ {
+ "user": {},
+ "iat": 0,
+ "exp": 9999999999,
+ "rls_rules": [],
+ "resources": [
+ {
+ "type": GuestTokenResourceType.DASHBOARD,
+ "id": str(dashboard.uuid),
+ }
+ ],
+ }
+ )
+ with override_user(guest):
+ with pytest.raises(SupersetSecurityException):
+ security_manager.raise_for_editorship(dashboard)
+
+ def test_versions_refuse_guest_even_when_guest_role_is_editor(self) -> None:
+ """A guest is refused even when its role subject holds editorship.
+
+ The cross-model round's M10 case: without the guest deny,
+ granting a role subject editorship would open every guest holding
+ that role. Guest header auth DOES bind on this route (verified —
+ the request reaches the deny), so the pin is an exact 403: a
+ regression that deleted the deny would surface here as 200, and
+ one that broke guest auth entirely would surface as 401. The
+ deny's placement before the lookup is unit-pinned
+ (test_preflight_denies_guest_principals_outright)."""
+ # pylint: disable=import-outside-toplevel
+ from unittest.mock import patch as mock_patch
+
+ from flask import current_app
+
+ from superset import security_manager
+ from superset.security.guest_token import GuestTokenResourceType
+ from superset.subjects.utils import subjects_from_roles
+
+ dashboard = self._births_dashboard()
+ guest_role = security_manager.find_role(current_app.config["GUEST_ROLE_NAME"])
+ assert guest_role is not None
+ role_subjects = list(subjects_from_roles([guest_role]))
+ assert role_subjects, "guest role has no subject row"
+ original_editors = list(dashboard.editors)
+ dashboard.editors = original_editors + role_subjects
+ db.session.commit()
+ try:
+ with mock_patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ EMBEDDED_SUPERSET=True,
+ ):
+ token = security_manager.create_guest_access_token(
+ user={"username": "vh_guest"},
+ resources=[
+ {
+ "type": GuestTokenResourceType.DASHBOARD,
+ "id": str(dashboard.uuid),
+ }
+ ],
+ rls=[],
+ )
+ rv = self.client.get(
+ f"/api/v1/dashboard/{dashboard.uuid}/versions/",
+ headers={current_app.config["GUEST_TOKEN_HEADER_NAME"]: token},
+ )
+ assert rv.status_code == 403, rv.data
+ finally:
+ dashboard = self._births_dashboard()
+ dashboard.editors = original_editors
+ db.session.commit()
+
def test_put_non_numeric_pk_does_not_500_with_capture_on(self) -> None:
"""The PUT route is ``/<pk>`` (a string); with capture on, a
non-numeric pk must not raise a raw SQL type-cast error from the
@@ -350,6 +526,25 @@ class TestDatasetVersionsApi(SupersetTestCase):
json={"description": original},
)
+ def test_list_versions_denies_write_capable_non_editor_dataset(self) -> None:
+ """The dataset endpoint refuses a write-capable non-editor.
+
+ Alpha carries all-datasource access — the flavour where
+ read-vs-edit confusion would most plausibly regress — and is
+ still refused as a non-editor."""
+ ds_uuid = str(self._dataset().uuid)
+ self.login(ALPHA_USERNAME)
+ rv = self.client.get(f"/api/v1/dataset/{ds_uuid}/versions/")
+ assert rv.status_code == 403, rv.data
+
+ def test_get_version_denies_write_capable_non_editor_dataset(self) -> None:
+ """The dataset get-one route runs the same edit gate before
+ version resolution."""
+ ds_uuid = str(self._dataset().uuid)
+ self.login(ALPHA_USERNAME)
+ rv = self.client.get(f"/api/v1/dataset/{ds_uuid}/versions/{MISSING_UUID}/")
+ assert rv.status_code == 403, rv.data
+
def test_put_override_columns_returns_version_fields(self) -> None:
"""The ``override_columns`` save is two commits / two Continuum
transactions (update + refresh); the PUT still returns populated
diff --git a/tests/unit_tests/versioning/test_endpoint_gate.py b/tests/unit_tests/versioning/test_endpoint_gate.py
new file mode 100644
index 00000000000..1a34ed8ea37
--- /dev/null
+++ b/tests/unit_tests/versioning/test_endpoint_gate.py
@@ -0,0 +1,216 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""SC-120001: the version/activity endpoint families are EDIT-gated.
+
+The shared preflight (``resolve_endpoint_path_entity``) must enforce
+object-level editorship (``raise_for_editorship``) — never the read gate
+(``raise_for_access``) — per the sc-103156 SIP decision: the full change
+log is for principals who may alter the entity, matching the UI's
+edit-gated menu and the restore command's gate. These unit pins are the
+environment-independent half of the control; the integration role matrix
+(versions_api_tests / activity_view_tests) exercises it over HTTP.
+"""
+
+from types import SimpleNamespace
+from typing import Any
+from unittest.mock import MagicMock
+
+import pytest
+from pytest_mock import MockerFixture
+
+from superset.models.dashboard import Dashboard
+from superset.versioning import api_helpers
+from superset.versioning.api_helpers import (
+ PathEntityResponseError,
+ resolve_endpoint_path_entity,
+)
+
+_UUID = "8b8c9f00-0000-4000-8000-000000000001"
+
+
+def _api() -> MagicMock:
+ api = MagicMock()
+ api.response_400.return_value = "resp-400"
+ api.response_403.return_value = "resp-403"
+ api.response_404.return_value = "resp-404"
+ return api
+
+
+def test_preflight_enforces_editorship_not_read_access(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """The preflight consults only the editorship predicate.
+
+ ``is_editor`` — the same predicate ``raise_for_editorship`` (the
+ restore command's gate) enforces, called directly because the entity
+ was just loaded live. The read gate must never run. (Reverted-gate
+ control: restoring the read gate fails the not-called assertion.)"""
+ entity = SimpleNamespace(id=1)
+ mocker.patch.object(
+ api_helpers.VersionDAO, "find_active_by_uuid", return_value=entity
+ )
+ # Explicit MagicMock: the module attribute is a werkzeug LocalProxy,
+ # whose attribute forwarding fools unittest.mock's async detection —
+ # a bare patch creates an AsyncMock whose calls return un-awaited
+ # coroutines, so raising side_effects never fire on this synchronous
+ # path.
+ sm = MagicMock()
+ sm.is_guest_user.return_value = False
+ mocker.patch.object(api_helpers, "security_manager", sm)
+
+ resolved, _ = resolve_endpoint_path_entity(_api(), Dashboard, _UUID)
+
+ assert resolved is entity
+ sm.is_editor.assert_called_once_with(entity)
+ sm.raise_for_access.assert_not_called()
+
+
+def test_preflight_maps_editorship_refusal_to_403(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """A non-editor principal gets the 403 response, nothing else. The
+ security manager is stubbed with a plain object whose editorship
+ predicate refuses — and whose read gate raises AssertionError if
+ consulted, doubling as a not-called pin on the refusal path. (A bare
+ ``mocker.patch.object`` here would yield an AsyncMock — see the
+ LocalProxy note in the sibling test.)"""
+ entity = SimpleNamespace(id=1)
+ mocker.patch.object(
+ api_helpers.VersionDAO, "find_active_by_uuid", return_value=entity
+ )
+
+ def _read_gate_must_not_run(**_kwargs: Any) -> None:
+ raise AssertionError("read gate must not be consulted")
+
+ mocker.patch.object(
+ api_helpers,
+ "security_manager",
+ SimpleNamespace(
+ is_guest_user=lambda: False,
+ is_editor=lambda _entity: False,
+ raise_for_access=_read_gate_must_not_run,
+ ),
+ )
+
+ with pytest.raises(PathEntityResponseError) as exc:
+ resolve_endpoint_path_entity(_api(), Dashboard, _UUID)
+
+ assert exc.value.response == "resp-403"
+
+
+def test_preflight_fails_closed_for_unwired_models(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """Unwired models fail closed before any parsing or database work.
+
+ The allowlist compares class IDENTITY: an unrelated class that
+ happens to be NAMED like a wired model must not slip through, and
+ the DAO is never consulted for it."""
+
+ class Slice: # same __name__ as the wired model, different class
+ pass
+
+ dao = mocker.patch.object(
+ api_helpers.VersionDAO,
+ "find_active_by_uuid",
+ return_value=SimpleNamespace(id=1),
+ )
+ mocker.patch.object(api_helpers, "security_manager")
+
+ with pytest.raises(LookupError):
+ resolve_endpoint_path_entity(_api(), Slice, _UUID)
+
+ dao.assert_not_called()
+
+
+def test_preflight_denies_guest_principals_outright(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """Guest principals are refused before any editorship evaluation.
+
+ The deny runs BEFORE the database lookup: a guest probing UUIDs
+ gets a uniform 403 whether or not the entity exists, so the
+ 403-vs-404 split cannot disclose which UUIDs exist. Neither gate may
+ even be consulted (``is_editor`` refuses guests on its own — the
+ endpoint deny is the ordering guarantee on top)."""
+ dao = mocker.patch.object(
+ api_helpers.VersionDAO,
+ "find_active_by_uuid",
+ return_value=SimpleNamespace(id=1),
+ )
+
+ def _gate_must_not_run(*_args: Any, **_kwargs: Any) -> None:
+ raise AssertionError("no gate may run for a guest principal")
+
+ mocker.patch.object(
+ api_helpers,
+ "security_manager",
+ SimpleNamespace(
+ is_guest_user=lambda: True,
+ is_editor=_gate_must_not_run,
+ raise_for_editorship=_gate_must_not_run,
+ raise_for_access=_gate_must_not_run,
+ ),
+ )
+
+ with pytest.raises(PathEntityResponseError) as exc:
+ resolve_endpoint_path_entity(_api(), Dashboard, _UUID)
+
+ assert exc.value.response == "resp-403"
+ dao.assert_not_called()
+
+
+def test_is_editor_refuses_guest_principals_before_anything_else() -> None:
+ """SECURITY.md's matrix: a guest is NEVER an editor.
+
+ Called unbound with a stub manager so the real method body runs: a
+ guest principal is refused before the admin shortcut, before subject
+ resolution, and before the resource's editor list is ever touched —
+ so every caller of ``raise_for_editorship`` (restore, delete,
+ update, the MCP tools) inherits the deny. The resource stub would
+ happily admit anyone (a bare MagicMock's ``editors`` is truthy);
+ only the early return keeps the guest out."""
+ from superset.security.manager import SupersetSecurityManager
+
+ sm = MagicMock()
+ sm.is_guest_user.return_value = True
+
+ assert SupersetSecurityManager.is_editor(sm, MagicMock()) is False
+ sm.is_admin.assert_not_called()
+
+
+def test_raise_for_editorship_inherits_the_guest_deny(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """The enforcement gate refuses a guest via the predicate it wraps.
+
+ ``raise_for_editorship`` is exercised unbound with the REAL
+ ``is_editor`` bound through, a guest principal, and a non-soft-delete
+ resource: the guest deny inside ``is_editor`` must surface as the
+ gate's SupersetSecurityException — the write-path (restore command)
+ shape of the M10 escalation."""
+ from superset.exceptions import SupersetSecurityException
+ from superset.security.manager import SupersetSecurityManager
+
+ sm = MagicMock()
+ sm.is_guest_user.return_value = True
+ sm.is_admin.return_value = False
+ sm.is_editor = lambda resource: SupersetSecurityManager.is_editor(sm, resource)
+
+ with pytest.raises(SupersetSecurityException):
+ SupersetSecurityManager.raise_for_editorship(sm, SimpleNamespace(id=1))
diff --git a/tests/unit_tests/versioning/test_restore.py b/tests/unit_tests/versioning/test_restore.py
index 4e1c2144426..a979ae5a21b 100644
--- a/tests/unit_tests/versioning/test_restore.py
+++ b/tests/unit_tests/versioning/test_restore.py
@@ -87,7 +87,7 @@ def test_restore_version_fails_closed_for_unregistered_model(
mock_db, mock_version_class
) -> None:
"""An unregistered model must raise, not silently restore without its
- child relations (mirrors _RAISE_FOR_ACCESS_KWARG's fail-closed
+ child relations (mirrors _version_endpoint_models's fail-closed
dispatch)."""
mock_db.session = _engine_with_target(MagicMock(operation_type=0))
with pytest.raises(LookupError, match="SomeNewModel"):