mikebridge commented on code in PR #44021:
URL: https://github.com/apache/superset/pull/44021#discussion_r4007139602
##########
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:
Accepted and fixed centrally in f4bb689fa8: `is_editor` itself now refuses
guest principals (SECURITY.md's matrix grants guests no edit capability
anywhere), so all ~30 `raise_for_editorship` callers — restore included —
inherit the deny; the read-side `is_viewer` mapping is unchanged. Unit-pinned
at both altitudes: the predicate (guest refused before the admin shortcut or
any editor-list read) and the enforcement gate (`raise_for_editorship` with the
real `is_editor` bound through raises for a guest).
##########
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:
Fixed in f4bb689fa8 — the guest deny now runs before the lookup, so a guest
gets a uniform 403 whether or not the UUID exists; the unit pin asserts the DAO
is never consulted for a guest. Agreed with the grading note that the split was
inherited (master returned 200 with the full log here), but the reorder was
cheap enough to take in this 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: [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]
