aminghadersohi commented on code in PR #44021:
URL: https://github.com/apache/superset/pull/44021#discussion_r3993624319
##########
UPDATING.md:
##########
@@ -69,6 +69,8 @@ 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 (`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. Related-entity visibility filtering inside the activity stream is
unchanged.
Review Comment:
Duplicated entry — 72 and 73 are the same bullet, 73 being the superset (it
adds the guest sentence). Suggestion deletes 72. Both also land under `###
Tagging is on by default` rather than a heading of their own.
```suggestion
```
##########
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``
Review Comment:
Related records still carry `changed_by`/`from_value`/`to_value` under the
read-based visibility filter, so a path-entity editor reads field-level diffs
of an entity they only have read on. Unchanged vs master and scoped out in the
body — noting it as a residual gap against "history is edit-gated".
##########
tests/unit_tests/versioning/test_endpoint_gate.py:
##########
@@ -0,0 +1,172 @@
+# 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.exceptions import SupersetSecurityException
+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 gate.
+
+ 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.raise_for_editorship.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 gate raises the
+ real exception type — 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 _deny(_entity: Any) -> None:
+ raise SupersetSecurityException(MagicMock())
+
+ 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,
+ raise_for_editorship=_deny,
+ 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(
Review Comment:
`Python-Unit` is cancelled at this head and has no completed run on any of
the 4 commits — this suite has never executed in CI. The integration guest pin
that did run accepts 401, so it passes even if guest auth never binds; this
unit pin is the real M10 evidence. A rebase (behind 3) re-triggers it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
