rebenitez1802 commented on code in PR #45644:
URL: https://github.com/apache/superset/pull/45644#discussion_r4447634918
##########
superset/dashboards/api.py:
##########
@@ -2621,14 +2621,11 @@ def should_enqueue(cache_payload:
ScreenshotCachePayload) -> bool:
)
return lock_response
+ guest_user =
security_manager.get_current_guest_user_if_guest()
Review Comment:
🔴 **Empty-username guests share one screenshot cache entry**
Forwarding the guest token here makes the task render as the guest and store
the image, but the request cache key still has no guest identity in it.
`get_dashboard_digest` (`superset/thumbnails/digest.py:108`) resolves the
executor from `get_current_user()`, which is `""` for these guests. Under the
default `THUMBNAIL_EXECUTORS = [ExecutorType.CURRENT_USER]` that raises
`ExecutorNotFoundError`, so the digest is `None` and the guest's RLS never
enters it. The permalink key is identical for every guest (`get_user_id()` is
`None`).
Result: two empty-username tokens with different `rls` (`tenant = 'A'` /
`tenant = 'B'`) on the same dashboard state get the same `cache_key`, and guest
B is served the image rendered for guest A. Before this change the task failed
with `ExecutorNotFoundError` and nothing was stored; named guests are
unaffected. `screenshot_guest_dispatch_test.py` doesn't catch it because it
mocks the digest and cache key.
Suggested fix (outside this diff, so not an applyable suggestion): key the
digest on the guest when `CURRENT_USER` can't resolve from its username.
Validated locally: each guest then gets its own key and image, and
`tests/unit_tests/{thumbnails,tasks}`, `utils/screenshot_test.py` and the two
new test files pass (775), ruff/mypy clean.
```diff
--- a/superset/thumbnails/digest.py
+++ b/superset/thumbnails/digest.py
@@ -105,16 +105,34 @@ def _adjust_string_with_rls(
return unique_string
-def get_dashboard_digest(dashboard: Dashboard) -> str | None:
+def _get_thumbnail_executor(
+ model: Dashboard | Slice,
+) -> tuple[ExecutorType, str] | None:
+ """
+ Resolve the thumbnail executor, keying an embedded guest on itself when
+ ``CURRENT_USER`` cannot resolve from its username (guest tokens may
carry
+ an empty one), so the guest's RLS still enters the digest.
+ """
+ executors = app.config["THUMBNAIL_EXECUTORS"]
try:
- executor_type, executor = get_executor(
- executors=app.config["THUMBNAIL_EXECUTORS"],
- model=dashboard,
+ return get_executor(
+ executors=executors,
+ model=model,
current_user=get_current_user(),
)
except ExecutorNotFoundError:
+ if ExecutorType.CURRENT_USER in executors and (
+ guest := security_manager.get_current_guest_user_if_guest()
+ ):
+ return ExecutorType.CURRENT_USER, guest.username
return None
+
+def get_dashboard_digest(dashboard: Dashboard) -> str | None:
+ if not (chosen := _get_thumbnail_executor(dashboard)):
+ return None
+ executor_type, executor = chosen
+
if func := app.config["THUMBNAIL_DASHBOARD_DIGEST_FUNC"]:
return func(dashboard, executor_type, executor)
@@ -132,14 +150,9 @@ def get_dashboard_digest(dashboard: Dashboard) -> str |
None:
def get_chart_digest(chart: Slice) -> str | None:
- try:
- executor_type, executor = get_executor(
- executors=app.config["THUMBNAIL_EXECUTORS"],
- model=chart,
- current_user=get_current_user(),
- )
- except ExecutorNotFoundError:
+ if not (chosen := _get_thumbnail_executor(chart)):
return None
+ executor_type, executor = chosen
if func := app.config["THUMBNAIL_CHART_DIGEST_FUNC"]:
return func(chart, executor_type, executor)
```
It would be worth adding a test with two empty-username tokens carrying
different `rls` that asserts they get different cache keys, without mocking
`dashboard.digest`.
--
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]
