rusackas commented on code in PR #43395:
URL: https://github.com/apache/superset/pull/43395#discussion_r3832327735
##########
superset/dashboards/api.py:
##########
@@ -2019,6 +2062,12 @@ def screenshot(self, pk: int, digest: str) ->
WerkzeugResponse:
# fetch the dashboard screenshot using the current user and cache if
set
if cache_payload := DashboardScreenshot.get_from_cache_key(digest):
+ # The digest is caller-supplied and cache entries are shared across
+ # every dashboard (and, via the same backend, charts) -- without
+ # this check any cache_key learned for one dashboard would serve
+ # its image under a different, merely-accessible `pk`.
+ if cache_payload.get_scope() != f"dashboard:{dashboard.id}":
+ return self.response_404()
Review Comment:
Good catch, fixed. should_trigger_task() now takes an expected_scope and
treats an UPDATED entry with a mismatched or missing scope as a cache miss, so
it gets recomputed and re-scoped instead of 404ing forever.
##########
superset/utils/screenshots.py:
##########
@@ -315,6 +341,7 @@ def compute_and_cache( # pylint: disable=too-many-arguments
window_size = window_size or self.window_size
thumb_size = thumb_size or self.thumb_size
logger.info("Processing url for thumbnail: %s", cache_key)
+ cache_payload.set_scope(self.cache_scope)
Review Comment:
Same fix as the dashboards/api.py thread: should_trigger_task() now treats
an UPDATED entry with a scope that doesn't match the caller's expected_scope as
a cache miss, so legacy unscoped entries get recomputed and re-scoped instead
of getting stuck.
##########
superset/thumbnails/digest.py:
##########
@@ -62,9 +62,18 @@ def _adjust_string_with_rls(
Add the RLS filters to the unique string based on current executor.
"""
+ # Prefer the ambient guest user (the actual requesting principal) over a
+ # DB-user lookup by username: for guest requests `executor` is the
+ # token-supplied username, which can collide with a real DB username. If
+ # find_user() were tried first, a collision would compute RLS under the
+ # unrelated DB user's identity, and the token's own per-token rls claims
+ # (surfaced via get_guest_rls_filters(), which reads the ambient guest
+ # user installed by override_user() below) would never enter the digest --
+ # letting two guest tokens with the same username but different rls
+ # collide on one cache entry.
user = (
- security_manager.find_user(executor)
- or security_manager.get_current_guest_user_if_guest()
+ security_manager.get_current_guest_user_if_guest()
+ or security_manager.find_user(executor)
Review Comment:
Leaving this one. The digest's ambient-guest preference is actually
load-bearing for cache_dashboard_screenshot's guest_token path, where the
render is forced to the guest regardless of what get_executor() resolves, so
gating strictly on executor_type == CURRENT_USER would fix the
chart-thumbnail-task case but break the digest for the permalink/report
screenshot path this PR itself just fixed in cda3b2f6cf. Needs a look at how to
distinguish the two call paths before touching this.
--
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]
