You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sadpandajoe (via GitHub)" <gi...@apache.org> on 2026/10/22 22:17:44 UTC
[PR] fix(dashboard): keep guest identity independent of username value [superset]
sadpandajoe opened a new pull request, #45644:
URL: https://github.com/apache/superset/pull/45644
### SUMMARY
Guest-user handling for embedded dashboards should apply to every guest token, whatever value its `username` claim carries.
`SupersetSecurityManager.is_guest_user()` used the current user's username as a presence check, so a guest whose username claim is an empty string was not recognized as a guest and skipped the guest-specific handling that depends on `is_guest_user()`. The dashboard screenshot dispatch had the same dependency and did not forward the guest token for such a guest.
- `is_guest_user()` still evaluates the current user inside the existing `NoAuthorizationError` guard, but no longer consults its username; the user's guest marker decides, as it already does for the explicit-user form.
- The dashboard screenshot dispatch resolves the guest with `get_current_guest_user_if_guest()` (the type-based check used elsewhere) and forwards its token, so the task renders as the guest.
Non-guest, anonymous and unauthenticated behavior is unchanged.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend only).
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/security/guest_user_detection_test.py tests/unit_tests/dashboards/screenshot_guest_dispatch_test.py
```
The tests build the guest through the request loader from tokens with a non-empty and an empty username and assert identical handling for detection, dashboard access rules (security manager and UUID lookup), response field filtering, error sanitization and screenshot dispatch, plus controls showing a legitimately authorized guest is still allowed. The empty-username cases fail without the change.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags: `EMBEDDED_SUPERSET`
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep guest identity independent of username value [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45644:
URL: https://github.com/apache/superset/pull/45644#issuecomment-6352856555
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45644?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `66.66667%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 65.37%. Comparing base ([`0689df7`](https://app.codecov.io/gh/apache/superset/commit/0689df740e225fd06c391916d4bfd31c8a408221?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`917cf44`](https://app.codecov.io/gh/apache/superset/commit/917cf44a33429624353ecd023e1be6e42a6708e8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45644?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/45644?src=pr&el=tree&filepath=superset%2Fsecurity%2Fmanager.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | 50.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/45644?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (0689df7) and HEAD (917cf44). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (0689df7) | HEAD (917cf44) |
>|------|------|------|
>|python|7|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45644 +/- ##
===========================================
- Coverage 84.12% 65.37% -18.75%
===========================================
Files 3056 3056
Lines 201432 201465 +33
Branches 47097 47105 +8
===========================================
- Hits 169447 131707 -37740
- Misses 28736 66784 +38048
+ Partials 3249 2974 -275
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.87% <16.66%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.82% <66.66%> (+<0.01%)` | :arrow_up: |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.60% <16.66%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.90% <66.66%> (-34.45%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45644/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45644?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep guest identity independent of username value [superset]
Posted by "netlify[bot] (via GitHub) via notifications" <no...@superset.apache.org>.
netlify[bot] commented on PR #45644:
URL: https://github.com/apache/superset/pull/45644#issuecomment-6665850292
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 254f218f254e9c6f7f11508019d039d33244341a |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aecc968bbe2bebb29ad2c0b |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-45644--superset-docs-preview.netlify.app](https://deploy-preview-45644--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8w6IVxIdoOO4XwqoVR0giK6e-7e4r1Qaj6LwgwwANaQaO9e4222S5qaq0k1LCqkMad-WMqRn4X7ckGIvJsQxaIgWh_Y0ojw_Q) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45644--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep guest identity independent of username value [superset]
Posted by "rebenitez1802 (via GitHub) via notifications" <no...@superset.apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep guest identity independent of username value [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45644:
URL: https://github.com/apache/superset/pull/45644#issuecomment-6390984827
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging.
- Forwarding guest tokens can make screenshots contain guest-specific data; verify the screenshot cache key isolates guests with different attributes.
**Assessed commit:** `917cf44a3342`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep guest identity independent of username value [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45644:
URL: https://github.com/apache/superset/pull/45644#discussion_r4368957550
##########
superset/dashboards/api.py:
##########
@@ -2596,14 +2596,11 @@ def should_enqueue(cache_payload: ScreenshotCachePayload) -> bool:
)
return lock_response
+ guest_user = security_manager.get_current_guest_user_if_guest()
try:
cache_dashboard_screenshot.delay(
username=get_current_user(),
- guest_token=(
- g.user.guest_token
- if get_current_user() and isinstance(g.user, GuestUser)
- else None
- ),
+ guest_token=guest_user.guest_token if guest_user else None,
Review Comment:
**Suggestion:** Guest attributes can change Jinja-rendered dashboard data, but the cache key excludes them; guests sharing a dashboard digest and state can receive a screenshot rendered with another guestβs attributes.
**Assessment:** π΄ `Critical` Β· π `Occurrence: Sometimes` Β· π·οΈ `Security`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4d39c6344a6b498eb083d6628515f4d4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4d39c6344a6b498eb083d6628515f4d4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 2603:2603
**Comment:**
*Security: Guest attributes can change Jinja-rendered dashboard data, but the cache key excludes them; guests sharing a dashboard digest and state can receive a screenshot rendered with another guestβs attributes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45644&comment_hash=cf062fd17cba6339db1868b5eff9833a62ec2622d1853f64d67c3cc10a38cc22&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45644&comment_hash=cf062fd17cba6339db1868b5eff9833a62ec2622d1853f64d67c3cc10a38cc22&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org