mikebridge commented on code in PR #44033: URL: https://github.com/apache/superset/pull/44033#discussion_r3992973796
########## SECURITY.md: ########## @@ -94,6 +94,10 @@ Realtime transports, including WebSocket delivery backed by Redis or Valkey Pub/ The realtime notification permission is distinct from the permission to read the underlying object. It controls whether a principal receives push notifications, not whether they may read the object once they call the protected REST API. Existing websocket connections are authorized by the JWT accepted at upgrade time; permission revocation after token minting is bounded by `WEBSOCKET_JWT_EXPIRATION_SECONDS` plus the websocket server's socket-check interval. Redis Streams are internal server-to-server coordination primitives and should not be directly exposed as an end-user subscription surface. +### Version History and Change Metadata + +An entity's version history — its change-record activity stream, field-level before/after diffs, and the author identity and timestamps attached to each change — is not part of the base *Read data* capability. Reading it requires **object-level editorship** of the entity (owner, editor, or Admin), the same capability required to restore a version. A principal who can read an entity's current state but is not an editor of it — including an **embedded guest token** and any **view-only** (read-but-not-edit) role — is not entitled to its version history, change metadata, or author identity, and such requests are refused. Related-entity records in a cross-entity activity view remain silently filtered to the reader's own read access. A bug that discloses version history, change-record diffs, or author identity to a principal lacking object-level editorship is in scope. Review Comment: You're right that this reads ahead of master — today `/versions/` and `/activity/` still take the read gate and `raise_for_editorship` only appears in restore. That's the intended ordering rather than drift, though: this paragraph documents the gate #44021 implements, and #44021's description already notes this docs PR must merge after it. The governing decision is the SC-103156 SIP, which supersedes the earlier read-posture wording — version history is edit-gated, matching the UI's edit-gated Version history menu and the restore endpoint. Concretely, #44021 replaces the per-resource `raise_for_access` dispatch in `resolve_endpoint_path_entity` with a single object-level `raise_for_editorship` gate behind a fail-closed model allowlist, with denial tests for write-capable non-editors across all three resource families. So I'd rather not take the narrowing — it would only be accurate for the window before #44021 lands and would contradict the SIP the moment it does. Would it work for you if I make the dependency explicit here instead (add "Depends on #44021 — merge after" to the description) and hold this one until #44021 merges? Happy to fold the two into a single reviewable change if you'd prefer. WDYT? ########## SECURITY.md: ########## @@ -94,6 +94,10 @@ Realtime transports, including WebSocket delivery backed by Redis or Valkey Pub/ The realtime notification permission is distinct from the permission to read the underlying object. It controls whether a principal receives push notifications, not whether they may read the object once they call the protected REST API. Existing websocket connections are authorized by the JWT accepted at upgrade time; permission revocation after token minting is bounded by `WEBSOCKET_JWT_EXPIRATION_SECONDS` plus the websocket server's socket-check interval. Redis Streams are internal server-to-server coordination primitives and should not be directly exposed as an end-user subscription surface. +### Version History and Change Metadata + +An entity's version history — its change-record activity stream, field-level before/after diffs, and the author identity and timestamps attached to each change — is not part of the base *Read data* capability. Reading it requires **object-level editorship** of the entity (owner, editor, or Admin), the same capability required to restore a version. A principal who can read an entity's current state but is not an editor of it — including an **embedded guest token** and any **view-only** (read-but-not-edit) role — is not entitled to its version history, change metadata, or author identity, and such requests are refused. Related-entity records in a cross-entity activity view remain silently filtered to the reader's own read access. A bug that discloses version history, change-record diffs, or author identity to a principal lacking object-level editorship is in scope. Review Comment: Good catch on the `has_guest_access` early return — that's exactly why #44021 doesn't lean on `raise_for_access` here at all. It refuses guest-token principals explicitly (`security_manager.is_guest_user()`) *before* the editorship check, because `is_editor` maps a guest's role subjects into the editor set, so a role granted editorship would otherwise admit every guest holding it — independent of whether `GUEST_ROLE_NAME` is `Public` or `Gamma`. Pinned by `test_editorship_gate_refuses_guest_principal`, `test_versions_refuse_guest_even_when_guest_role_is_editor`, and `test_preflight_denies_guest_principals_outright`. So the guest clause here describes an enforced entitlement once #44021 is in — same dependency as above. -- 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]
