fitzee commented on code in PR #44021:
URL: https://github.com/apache/superset/pull/44021#discussion_r3995783832
##########
superset/versioning/api_helpers.py:
##########
@@ -283,18 +298,26 @@ def resolve_endpoint_path_entity(
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}"
- )
+ # 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). Denied explicitly
+ # BEFORE the editorship check: ``is_editor`` maps a guest's ROLE
+ # subjects into the editor set, so a role subject granted editorship
+ # would otherwise admit every guest holding that role.
+ if security_manager.is_guest_user():
+ raise PathEntityResponseError(api.response_403())
+ # 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.
try:
- security_manager.raise_for_access(**{kwarg: entity})
+ security_manager.raise_for_editorship(entity)
Review Comment:
**Fix altitude: guest-escalation is only closed at the read endpoints.** The
comment above (307-318) correctly notes that `is_editor` maps a guest's *role*
subjects into the editor set, so a role subject granted editorship would
otherwise admit every guest holding that role. But that escalation is guarded
only here, in `resolve_endpoint_path_entity`.
`raise_for_editorship` has ~30 other callers with no guest pre-deny —
notably the **restore** command (a destructive write). A guest whose token
carries a role subject granted editorship on an entity still passes `is_editor`
→ `raise_for_editorship` succeeds → the guest can mutate/restore the entity,
the same M10 escalation these read endpoints now block.
Consider making guests never count as editors inside `is_editor` /
`raise_for_editorship` itself, so every caller inherits the fix rather than
each endpoint re-implementing the guest deny. (Reachability over HTTP depends
on whether guest-token auth binds to these resource routes — the same
uncertainty the guest test below hedges on — so this is plausible rather than
proven, but the fix belongs centrally regardless.)
##########
superset/versioning/api_helpers.py:
##########
@@ -283,18 +298,26 @@ def resolve_endpoint_path_entity(
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}"
- )
+ # 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). Denied explicitly
+ # BEFORE the editorship check: ``is_editor`` maps a guest's ROLE
+ # subjects into the editor set, so a role subject granted editorship
+ # would otherwise admit every guest holding that role.
+ if security_manager.is_guest_user():
Review Comment:
**403-vs-404 enables UUID existence enumeration.** The guest deny runs
*after* `find_active_by_uuid` / `response_404` (line 297-299), so a guest
probing `/api/v1/chart/<uuid>/versions/` gets **403 for existing** entities and
**404 for non-existing** ones. That difference discloses which entity UUIDs
exist to a principal that should have no read visibility into them at all.
Placing the guest deny *before* the DB lookup would make existence
indistinguishable.
##########
tests/integration_tests/versioning/versions_api_tests.py:
##########
@@ -167,14 +171,186 @@ def test_get_version_404_on_unknown_version(self) ->
None:
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: ``is_editor`` maps a guest's
+ ROLE subjects into the editor set, so without the explicit guest
+ deny at the choke point, granting a role subject editorship would
+ open every guest holding that role. Layered refusal is accepted
+ here (401 if guest header auth doesn't bind on this route, 403
+ from the deny) — the invariant pinned is never-200; the explicit
+ pre-editorship deny itself 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 in (401, 403), rv.data
Review Comment:
**This test can pass without exercising the guest deny it documents.**
`assert rv.status_code in (401, 403)` — if the guest token never authenticates
on this route (401), the request short-circuits *before* reaching the
`is_guest_user()` deny in `resolve_endpoint_path_entity`. A regression that
deleted that deny line would still leave this test green on the 401 branch, so
it doesn't actually pin the M10 bypass it describes. The docstring acknowledges
this and leans on `test_preflight_denies_guest_principals_outright` as the real
pin — worth either asserting `== 403` (if guest auth does bind here) or
dropping the HTTP claim so the coverage isn't overstated.
##########
superset/versioning/api_helpers.py:
##########
@@ -274,6 +281,14 @@ 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():
Review Comment:
**Nit (perf):** `_version_endpoint_models()` runs three deferred imports and
rebuilds the tuple on every request, just for a membership check against a
compile-time-constant 3-element allowlist. Imports are `sys.modules`-cached so
the cost is small, but this could be memoized once (module-level lazy singleton
/ `functools.cache`) to keep the shared choke point allocation-free.
##########
superset/versioning/api_helpers.py:
##########
@@ -283,18 +298,26 @@ def resolve_endpoint_path_entity(
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}"
- )
+ # 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). Denied explicitly
+ # BEFORE the editorship check: ``is_editor`` maps a guest's ROLE
+ # subjects into the editor set, so a role subject granted editorship
+ # would otherwise admit every guest holding that role.
+ if security_manager.is_guest_user():
+ raise PathEntityResponseError(api.response_403())
+ # 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.
try:
- security_manager.raise_for_access(**{kwarg: entity})
+ security_manager.raise_for_editorship(entity)
Review Comment:
**Possible double DB round-trip per request.** `find_active_by_uuid` already
loaded the entity above (query 1); `raise_for_editorship` here then, for a
`SoftDeleteMixin` resource, re-queries the row with
`SKIP_VISIBILITY_FILTER_CLASSES` (query 2) before checking `is_editor`. On the
hot `list_versions`/`get_version`/`activity` paths this doubles entity lookups
vs. the previous `raise_for_access` gate. If the already-resolved entity can be
passed through instead of re-fetched, it saves a round-trip. Minor — flagging
since it's on the hottest path in this module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
